Password reset workflow is broken when T&Cs have been updated

This is also a problem if an admin creates a user account and sends out an email to register using the same method.
The approval page comes after the login, before the change password page and the pass-reset-token mechanism gets lost and you have to enter your old password again.
For users created without a random password assigned and emailed that's very confusing.

I've taken the idea from @barrya's snippet in #2 and used the logic from http://api.drupal.org/api/drupal/modules%21user%21user.api.php/function/... to improve it.

I've pulled the latest branch from git, incorporated the above patch and I am still having new user registration issues using the one time login. (ie this is the one where the site admin creates a new user profile and sends it out to clients). This is where a user won't know their password, but the current password is still being asked for before the new password can be corrected.

Steps:
1) install legal module, configure, etc
2) admin creates new user, checks the box to notify user of profile
3) from the email, select the one-time login url and open
4) Say yes, I want to log in,
5) Accept the legal module
6) Try to save a new password (the current password box shouldn't be here, this is one-time-login!)

Wouldn't a more usable and easier way be to detect if the user is coming in over a password reset link and not show the modified T/C just the once? From a usability perspective, a user who uses the one time login is expecting to change their password right away, and any intermediate steps between them and a P/W change (Including what page the set message gets output on) can really change the perceived experience.

I would rework legal to not show the updated T/C if we detect a password reset or one time login. As soon as the password is updated, we can render the new T/C anyway.

Shnapoo,

Thanks for the patch!

I'm reading through it for review, and was wondering what the logic was for this: empty($_POST['form_id'])

It seems like this would eval true in the event that no form was ever submitted. For example, if a user was already logged in and the t/c changed at some point, because that user already has a session, they do not process the login form, ergo empty($_POST['form_id']) will be true.

@Shnapoo,

Ok, I missed the fact that we were working in hook_user_login. Having empty() there makes a lot more sense when we are sure that it was a login op that got us there in the first place.

Considering that, I do think the code looks good, though perhaps we should get a second opinion before switching to RTBC?

Tnx for the patch in #8. Was working on the same solution today and found this patch. Tested it and it works for me.

Just had a conference call with a client yesterday who pointed out a pretty big shortcoming.

The user edit page still requires the user to re-agree, but it doesn't clearly call it out that this is an expected behavior. A user who fills out just the password reset and hits enter, or a user who is so used to seeing the legal consent text at the bottom of user/%uid/edit there in a reference capacity will trigger a form validation error in legal module. This is good. It seems, however, that once this is done, the password reset function reverts back to the default state of needing the old pass before changing it to the desired pass. This is bad.

Possible solutions:
1. (Best) Do not ever show a valid legal form in any capacity on the user edit page. Show the agreed text elsewhere. Because really?? Who in their right mind would go to user/edit to view the agreement. Instead, show it on user, or as a menu_local_task (bonus points for showing the full agreement history w/ revisions?)
2. (Not Great, but passable) Keep showing the most currently agreed version of legal on user/edit, and always keep it as read only with no validation on submit. After all, the user isn't going to get very far without seeing the dedicated re-agree page.
3. (Really, really Poor) set message that they also need to re-agree at the same time as the password reset, and if they don't notice they need to re-agree, well, they get to go through another round of password resetting.

Bumping up to critical as this issue is a huge UX bug

Hmm. I haven't tested it on a bare d7 site--just one that has had a lot of changes to various things, so it is entirely possible that the one artifact of the password reset reverting back is caused by one of the many other custom modules we have running.

Perhaps as an intermediate step, #3 would not be so unreasonable in this context, however it isn't the long term solution.

What do you think between:
1. Do #3 now, and I'll create a separate issue for #1
2. Just roll #1 in with this issue.

Rerolled patch with drupal_set_message to be displayed any time the T&C are required on the user/edit form.

So, if this still looks good to people, can someone mark it RTBC? I'd hate for it to get lost to the past.

Looks good to me, no issues thus far. Marked as RTBC to try and prevent this issue lingering in the halls of forgotten bugs.

I just jumped onto this thread - I had exactly the same issue; I confirm that the patch, as proposed in #19, appears to work fine for me. It would be great if this could be rolled into a non-dev release of this module.

Thanks to those who have worked on this!

reroll. applies to 7.x-1.x tested to work. RTBC pretty please.

This is strange. #19 used to work for me in that past, or so I thought. The fact is that it is not working now.

I trace the issue down to the fact that the patch is using $_POST inside hook_user_login() instead of the submitted values passed to the hook implementation.

The patch makes the hook implementation code look like ...

/**
 * Implements hook_user_login().
 */
function legal_user_login(&$edit, $account) {
  global $user;
  global $language;

  // don't block the password reset form
  if (empty($_POST['form_id']) || $_POST['form_id'] == 'user_pass_reset') {
    return;
  }

... but it should rather be using $edit['form_id'] instead of $_POST['form_id'] like ...

/**
 * Implements hook_user_login().
 */
function legal_user_login(&$edit, $account) {
  global $user;
  global $language;

  // don't block the password reset form
  if (empty($edit['form_id']) || $edit['form_id'] == 'user_pass_reset') {
    return;
  }

I have attached a patch that fixes that. Hopefully it will help.

Cheers

I am not sure why the patch is being ignored by the test-bot. Here is another attempt with the same patch.

This patch worked for us, Thanks!

I don't think this patch (#28) is 100% correct. In my case, drupal_set_message() called from legal_form_user_profile_form_alter() was causing the message to reappear one too many times. Aside from that, just disabling the password reset form block is insufficient. There are validation and UX issues to contend with.

For my case, I didn't fix the Legal module per se, but I was to fix the overall functionality by doing the following:

- Install/activate/configure Password Reset Landing Page (PRLP) (found it to be better than Password Hustle.
- Modify legal.module: legal_user_login() as follows:
------------
// Deal with destination from password reset one time login link.
// Removed 'user'/'reset' check because PRLP is being used.
// Destination logic changed to rely on $_REQUEST['destination'], then PRLP setting for Login Destination, and finally, the fallback - the user's edit page.

if (!empty($_REQUEST['destination'])) {
$query = array('destination' => $_REQUEST['destination']);
} else {
$query = array('destination' => variable_get('prlp_destination', "user/$uid/edit"));
}

unset($_GET['destination']);
-------------

So now the user is asked to create a password upfront, and PRLP's Login Destination setting is used.

Hi @thisisjoe, I think that you are not looking at what this patch was trying to fix. The original issue was "Password reset workflow is broken when T&Cs have been updated".

During the password reset process, you get asked for the previous password which you don't have (that's the whole point of trying to reset it). Patch #19 was the one believed to have finally solved this. However, instead of using the $edit['form_id'], it was using $_POST['form_id'] which was inaccurate because the latter will have been set by the T&C form if that one was presented to the user.

You can easily reproduce this behaviour by doing the following:

1. Do not install the patches.
2. Create a new user.
3. For the first time that user logs in, do not use the password set for that user. Pretend that you do not know it and trying to reset the password.
4. Keep some notes about your experience
5. Install the patches
6. Do steps 2, 3 and 4 with another new user account.
7. Let us know about your experience.

Please note that this patch is not compatible with alternate login methods such as fboauth module and probably others.

The problem is:

  // don't block the password reset form
  if (empty($edit['form_id']) || $edit['form_id'] == 'user_pass_reset') {
    return;
  }

During an oauth login, there is no form submit occurring, so empty($edit['form_id']) will be true and the user will not be prompted to accept the terms of service on first login (and never if the user always logs in using oauth).

Frankly I don't understand why the patch returns when $edit['form_id'] is empty..? I have successfully changed it to:

  // don't block the password reset form
  if (isset($edit['form_id']) && $edit['form_id'] == 'user_pass_reset') {
    return;
  }

The patch in #28 (same as the one in #26) did not work for me. I do not see how it could work for anyone, since $edit is passed $form_state, so instead of $edit['form_id'] the code should test $edit['values']['form_id'].

To answer the question in #32, hook_login() is invoked twice when someone uses a one-time login link. The first time, $edit['values']['form_id'] == 'user_pass_reset', and the second time $edit = array(). I checked this by putting in a lot of watchdog() calls, but you can also see it from these lines in user_pass_reset():

          $user = $account;
          // user_login_finalize() also updates the login timestamp of the
          // user, which invalidates further use of the one-time login link.
          user_login_finalize();
          drupal_set_message(t('You have just used your one-time login link. It is no longer necessary to use this link to log in. Please change your password.'));
          // Let the user's password be changed without the current password check.
          $token = drupal_hash_base64(drupal_random_bytes(55));
          $_SESSION['pass_reset_' . $user->uid] = $token;
          drupal_goto('user/' . $user->uid . '/edit', array('query' => array('pass-reset-token' => $token)));

(A form callback exits with drupal_goto(). Ugh!)

So I kept the check that @cedric did not like, simplifying empty($edit['form_id']) to empty($edit).

The original code had these lines further down in legal_user_login():

  // Deal with destination from password reset one time login link.
  if (arg(0) == 'user' && arg(1) == 'reset') {
    $query = array('destination' => "user/$uid/edit");
  }

As a general rule, I prefer testing a function's arguments to the global arg(0) and arg(1) (essentially, the path is '/user/reset/...'). But maybe the original test would play better with fboauth. @cedric, are you interested in testing that?

After removing those lines, I replaced

  $query = NULL;
  if (!empty($_REQUEST['destination'])) {
    $query = array('destination' => $_REQUEST['destination']);
  }

with a single assignment, using the ternary ?:. (Is this a symptom of learning C in high school?)

In my testing, this patch does not interfere with the Legal module during ordinary login. After using a one-time login link, the user is redirected to the account page, where the T&C is a required field and the user can reset his or her password. BUT the user can decide to ignore this form, and just request a one-time login every time (s)he visits the site. I think this is a flaw shared by all the patches so far on this issue.

As a last thought, I double checked and did not see the problem described in #15 with my patch.

Thanks. This patch works for me.

Is any parts of the workflow in this issue related to (or helpful for) any of these issues?
#840068: Doesn't work with facebook connect (Drupal for facebook)
#1280518: Registration Form T&C Accept Bypass
#2085227: One-time login links fails when ToS needs to be (re-)verified

All somewhat related to broken registration and/or login process.

@Leeteq:

I do not know, and I am not likely to test. Because of the limitations I mentioned in #33, I have decided not to use Legal on my site. I am using a custom module, instead. One of my coworkers is experimenting with using hook_init() to present the T&C form, and that should work with users who log in using FB or some other external authentication.

If there is interest in a radical patch (rewriting how Legal presents the confirmation form) instead of minor changes (like my patch in #33) then I might try one using my coworker's approach. I think that would be outside the scope of this issue.

Had exactly the same issue. Patch in #33 works.

For all those who get into this issue because of Drupal Commerce's checkout process, patch in #33 works nice.

New to this module, but I think #33 introduces a new problem: if the site is configured such that only an admin can create a user, right now when the user clicks the one-time login link after receiving the new registration email, they will be allowed to view privileged pages because the user is considered logged in once they make it to the change password page. Thus a user may now bypass the terms by simply navigating away from the user edit page.

Seems like the site disclaimer module forces the user to acknowledge the terms correctly, but unfortunately that module doesn't log the date/time of the user acceptance of the terms.

Without #33, although the change password screen is never shown which is also not desired, at least the user MUST accept the terms before continuing to view privileged pages, which I need for one site in particular.

I believe @benjifisher has stated something similar, but I wanted to explicitly point out that #33 allows the user to bypass the form save and still view the site, whereas with no patch, this is not possible.

@webdrips:

I guess my comments in #33 were too long. Near the end, I wrote

BUT the user can decide to ignore this form, and just request a one-time login every time (s)he visits the site. I think this is a flaw shared by all the patches so far on this issue.

See also #36.

@benjifisher

#33 gives an error when $edit['values']['form_id'] is not set.

Notice: Undefined index: values in legal_user_login() (line 383).

@haggag:

OK, should be pretty easy to fix. How do you reproduce the error? (It looks like just a notice, but I like code that does not generate too many notices.) For this line, see also #32 above.

In line with the comments above re: #33, I don't think this should be RTBC. In my testing, this basically allows users to browse the site without accepting the new T&C's (or accepting them at all, in the case of a new user requiring e-mail verification.) Since that's a key feature of this module, I don't think allowing that kind of access bypass is acceptable.

The enclosed patch plays off user.module's handling of password reset. Legal module operates by essentially logging the user out, requiring acceptance of terms, and then logging them back in. User module stores a password reset token in the session, which is consulted when the token is passed in the URL query string.

This alternate patch regenerates the session after the user is programmatically logged out, stores the token on that session, and adjusts the destination path accordingly. I think this is preferable since it leaves the core module functionality in place.

Another thought: the module's tests probably need to reflect the mission-critical requirement that T&C acceptance is required for site usage. The fact that #33 passes the existing bank of tests reflects the fact this isn't the case.

I tend to agree with logic laid out in comment #43 and can verify that the patch fixed the problem for me.

#43 seems solid; we are missing the tests on this, but perhaps we can spin off another issue for that and get this into Dev?

Yes, agreed, separate tests task just filed here:
#2173849: Write tests for broken password reset workflow patch

Please push #43 to -dev.

Patch doesn't entirely work:

* click the reset link
* the 'one time login' form is shown
* click the login button
* the T&C form is shown
* accept T&Cs
* the user edit form is shown

However, what is missing is the message telling me to change my password.

Here's an updated patch that adds a test.

(The patch doesn't work at all on my site which uses hook_user_login() to redirect users on login, but that's another story -- I probably need to make my code aware of what Legal module does.)

I can confirm #48 works and this critical issue still exists.

Fixed in Legal 7.x-1.5

Thanks to everyone who participated in this improvement, specially bradjones1 for creating the correct solution, and Joachim for adding tests and moving it forward.

I've added some code to display the change password reminder message on the account page after accepting new T&Cs.

All tests passed.

Related: Legal module password reset workflow is broken

I'm still having this issue in 1.5

This got committed, so marking fixed.