misd-service-development / drupal-raven Goto Github PK

If the drupal site remains in maintenance mode and the admin users session expires there is no way back into the drupal site (including the backdoor url) without going to the database and taking the site out of maintenance mode.

When clicking 'Cancel' on the Raven login page a version 1 response is currently returned, which ignores query strings on the URL. This means that the user is returned to the root of the site (eg http://localhost/drupal/) rather than the authentication page (eg http://localhost/drupal/?q=raven/auth).

Drupal users remain logged in to Drupal even though raven authentication session has been specifically ended by the user. Drupal session lasts for 23 days by default and may not be obvious to users the session is still active. Closing the browser does not end the session.

Suggestions:

Drupal to check state of raven authentication on any action.
Drupal session to end on browser close.
Drupal session to timeout within a reasonable/configurable time.
Any unsaved form data should be retained in case the user logs back in to save the data.

I'm using r4032login to automatically redirect to raven/login. If I have a URL with e.g. spaces, the login fails and hits https://github.com/misd-service-development/drupal-raven/blob/master/raven.module#L274 . By way of example, the following should be visible to anyone who has Raven-authenticated:

http://www.ch.cam.ac.uk/intranet/system/chemistry_raven_downloads/foo%20bar

but if one follows the above link, note that the &params= query param on the Raven login page is

&params=http%3A//www.ch.cam.ac.uk/intranet/system/chemistry_raven_downloads/foo%2520bar

so the % in the URI-encoded space looks to have been URI-encoded, mangling the URL. If you're not already logged in to Raven, this provokes a "Warning: Raven authentication failure."

Analysis will follow with a pull request shortly.

Currently the module places a 'Raven log in' link in the navigation block. This should be removed and instead links added to the user login block and the log in/create new account page (and when non-Raven logins are disabled, replace the forms entirely).

#18 changed the protocol version to 3, but it turns out that clicking cancel returns a version 1 response, which contains a different number of fields and so currently breaks.

A read of the version 3 protocol says that responses can be in any version, so the module needs to be updated to handle version 1 and 2 responses.

Currently anyone with a Raven account can sign up by logging in. It should respect the Drupal settings (eg only administrators can create an account).

It might be useful to provide Raven-specific settings (if Raven isn't enforced).

If the module is set to override the normal Drupal paths, the check is case-sensitive. So /UsEr, for example, isn't blocked out and the normal Drupal login is still accessible.

The 'Have a Raven account? You can log in with Raven instead.' message appears on the admin's user create page:

The module should be covered by tests. It might need to interact with the test Raven server.

When a user has an account created but is blocked, going to /user (for example) results in an infinite loop as they're sent to /user after logging in to Raven, which causes Drupal to redirect back to Raven. It must always send the user to the login failure page.

Currently, on sites that support both Raven and Drupal authentication, Raven users can 'request a new password' - which sends them a one-time login link.

Steps to recreate:
On a site that has Drupal and Raven auth enabled
Check that you have a 'Raven' account on the site
Go to /user
Click on 'request a new password'
Enter your CRSid, continue
A one-time login link is sent to the email address of the account
However the user cannot change their password on their profile, this is already blocked by the Raven module (desired behaviour!).

Preferred behaviour:
When a Raven user requests a new password:
A one-time-login link should not be sent.
The user is redirected to /user.
A message is displayed: "You cannot reset your Raven password here, see the [Raven webpages] for more information about resetting your password."

I can have a go at putting together a patch for this, but it's not top priority for us at the moment =] Are there any drawbacks to doing this?

The module can't be tested (see #4) using responses from the real Raven services, it will have to use a mock of the Raven test service instead (for which we can produce signed responses). This involves using a different certificate file and kid.

This must only be enable-able when testing.

Certain checks defined in http://raven.cam.ac.uk/project/waa2wls-protocol.txt aren't followed (eg check 2).

Use language prefixes in multilingual sites (eg Russian page URLs are prefixed with ru/) breaks login overriding (eg user/ is overriden, but ru/user/ isn't).

The user login block won't appear if you're on the normal login page, but if you're on the backdoor login page is available it will still appear. A hook should be added to hide it.

The module respects Drupal's account creation rules, which is right by default, but it would be useful to be able to override it for Drupal logins. So if you're using both normal and Raven logins, normal logins could be restricted but Drupal logins aren't.

When the module is installed the variables raven_login_override and raven_backdoor_login are set to false and true respectively. The admin form, however, sets these to 1 or 0. As a strict comparison on true is used in raven_backdoor_login_is_enabled() (introduced in a7acaba), access is always disabled.

Currently URLs with query strings are not supported as final destinations, eg:
/user/login?destination=page%3Freset%3D1

The module fails on the raven_signature_check on line 205, producing a 'raven authentication failure' error message.

I think this is because we're trying to url decode the data, which is causing a mismatch because of the special characters it generates, which is broken by the base64_decode.

Removing the url decode from the raven_signature_check seems to fix the issue. PR incoming.

As per http://raven.cam.ac.uk/project/waa2wls-protocol.txt the 303 status code should be used rather than 302.

When an administrators adds a user they have to add in a password, this isn't great for Raven-only users as they have to manually make one up. An option should be added to select whether the user should log in with Raven only or not (defaulted to true if the Raven override is enabled), which hides the password field if true.

The user_password() function should be used to generate a random one to avoid any potential security issues.

The 'Log in with Raven' link is still added to the user log in block when the user is already logged in.