Password can not be reset, when user hasn't accepted the legal terms yet

Tested and works as intended.

This seems a bug report. (Password reset links no longer work.)

The patch doesn't apply to the latest development snapshot.

Hi,

I had to stop using this module due to forgot password issues. Users are able to get into the website, but they're not able to change their password. It doesn't prompt for change after accepting and if they try to change, they of course need their previous password in order to change it.

Is there a newer version of this patch?

Similar issue regarding one time login link https://www.drupal.org/project/legal/issues/2841132

Rerolled patch #10 as it failed to apply, please review.

A fix for this item needs to be committed. Left unchecked this issue makes the legal module completely unusable as it breaks absolutely critical site functionality (password reset requests).

Previous patches for this have provided optional configuration to skip terms and conditions on password reset requests, but it seems logical to me that the optional configuration is superfluous considering not enabling it means password resets are broken and there is already an option provided to display terms and conditions on user profile edit pages.

My suggestion would be to simply update the module to never display terms and conditions for password resets requests. I'll roll a patch for this momentarily.

A very minimal patch to simply skip T&C when one time login links have been used for password resets:

I've had this issue for a while and unfortunately disabled T&C... I re-enabled the requirement to test the patch.

Was still having the Forgot password issue before applying the patch... applied it.

Plain user login appears to still work fine with T&C

Forgot password - After clicking the link to log in, it immediately brought me to the password change screen. The simple skip code appears to work.

With the previous patch, it was still going through the hook in some occasions, so I mostly just moved the code to the beginning of the hook to bypass it straight away.

Just to say that the patch in #23 worked for me using v.2.0.0 of the module.

I hope you'll get the failing tests resolved quickly and then get this whole patch committed!

Here is an attempt to fix the test failures in patch #23, kindly review.

Thank you!

Please correct me if am wrong, since we are skipping T&C here when using a one-time login link for password resets, I think we can remove the following code from the test:

/tests/src/Functional/PasswordResetTest.php
-    // Accept T&Cs and submit form.
-    $edit = ['edit-legal-accept' => TRUE];
-    $this->submitForm($edit, 'Confirm', 'legal-login');

-    // Check user is redirected to their user page.
-    $current_url = $this->getUrl();
-    $expected_url = $this->baseUrl . '/user/' . $this->uid . '/edit';
-    $this->assertEquals($current_url, $expected_url);

Thanks!

Thank you for the patch. I have opened a merge request, because I think this is the way to go now. I have also applied your patch https://www.drupal.org/files/issues/2021-06-01/3074688-28.patch.

The patch works, but not for all cases. We have a language based page and the method Drupal::request()->getPathInfo() returns something like /en/user/reset/2356/1644301971/tKWfVzXsUCKSiZJ4rhwMGYtaE7ezaRxppTPri_QnEH8. So the check, if it is a one time link, does not work. I changed that to str_contains( \Drupal::request()->getPathInfo(),'/user/reset/'). Furthermore, I have removed the deprecated code in the test class.

Important note :

Now we skip the legal terms and a user can log in without confirmation. This is definitely not what we want, so make sure that in the legal configuration the following setting is checked:

By that option the user have to confirm, when setting or changing the password.

We just updated our last merge request. Now, the module can handle password reset links without any leaks. You have to confirm the terms and get redirected to the user edit form to set or change a password. So nobody can log in without confirmation. It does not depend on any configuration settings.

The above change in #32 only work for core >= 9.3.0. For those still below that, in legal/src/Form/LegalLogin.php:200 change

\Drupal::request()->getSession()->set('pass_reset_' . $user->id(), $_GET['pass-reset-token']);

back to

$_SESSION['pass_reset_' . $user->id()] = $_GET['pass-reset-token'];

to match core/modules/user/src/AccountForm.php:135

#28 Worked for me, D9.3 version 2.0 PHP 8.1
and language set in the URL "language=en"

I created a new MR for 3.0.x.

The patch in #28 no longer applies when using 3.0.1 and the older issue is still occurring - if there is a T&C change to accept when a user also needs to do a password reset, this user will see "You are not authorized to access this page. " and the logs will show:

Path: /legal_accept?destination=/user/6393/edit&token=Y4TDAQZgW2GfBznyr3WLYHSzLF-7cHrU6OmrNRC6SuQ. Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException: in Drupal\Core\Routing\AccessAwareRouter->checkAccess() (line 121 of /var/www/html/drupal/web/core/lib/Drupal/Core/Routing/AccessAwareRouter.php).

Curious though, I have been unable to reproduce this now, with different users. Maybe was a browser cache issue. Fresh browser, fresh user and i am able to reset the password and accept a new T&C.

I've created a patch from the MR in #37 for Legal 3.0.1

Could someone, who is running into this issue, please add clear steps to the issue summary how to reproduce this in 3.0.x?
In which cases does this happen?
Does the switch in #31 affect this issue?

This should definitely be fixed and I think it's major if it still happens as described, but the issue summary is poor for reproduction.

Further report: #3326543: Password reset not working

I updated the issue summary to provide clear steps on how to reproduce this issue in 3.0.x.

Does the switch in #31 affect this issue?

It doesn't, but it generally seems, that this issue is not as bad as before, since when the user already accepted the legal terms, the issue won't come up.

"3074688-password-reset-links-3.0.x" doesn't look finished yet. I can't find the time to have a detailed look into this, but I can tell, that this still needs work.

The old MR 7 can be closed I guess.

It doesn't, but it generally seems, that this issue is not as bad as before, since when the user already accepted the legal terms, the issue won't come up.

Ok, I didn't think about changing legal terms, which need to get accepted once again. As this is quite common and will trigger this issue, I'll set this issue to major.

Ok, I added "testPasswordResetWithoutLegalAccepted()", "testPasswordResetWithLegalAccepted()" and generally refactored the tests in #3414370: Reset notice is showing up twice (from core and legal module) and don't disappear after password change, because that issue is also indirectly affected by the problem described inside this issue.

Test-wise, we should wait for that issue to get merged, before we touch the tests here.

Alright, the provided patch works great!

Let's wait for #3414370: Reset notice is showing up twice (from core and legal module) and don't disappear after password change to get merged, so we can start with the remaining test and check if this MR introduces any regressions.

POSTPONED on #3414370: Reset notice is showing up twice (from core and legal module) and don't disappear after password change

@Grevil I guess you mean MR!11?

@Anybody when exactly?

The old MR 7 can be closed I guess.

?

I meant MR 7 there.

@Grevil not sure I understand this issue....manually testing I wasn't able to reproduce the issue at all.

Am I right in thinking that this is now just an issue with automated tests?

@Ron Collins which version of the module did you test? Did you test with any other patches / MR's applied?

Am I right in thinking that this is now just an issue with automated tests?

I don't think so. This issue exists in 3.0.x. We'll add clear steps to reproduce this. Like in #3414370: Reset notice is showing up twice (from core and legal module) and don't disappear after password change we have to ensure we describe all possible paths and have tests for them.

@Robert Castelo, I can provide more specific steps to reproduce if needed!

Steps to reproduce:

• Install the module.
• Create new T&C.
• Reset the password of a non-admin user who has NOT accepted the T&C yet.
• Use the one time login link, and you will be forwarded to the Password reset page:

• After pressing Login, the T&C need to be accepted first, as the user hasn't accepted them yet:

• Accept the terms, and you will now be forwarded to the user edit page. BUT it is required for you to enter your current password to change the password. As the user obviously forgot it (hence he is trying to reset his password), he can not change it and needs to get a new one time login link, if he wants to log in ever again:

@anybody 3.0.1 is installed.

There was one other patch applied: https://www.drupal.org/files/issues/2023-01-23/3283807-8-deprecated-func...

@Robert Castelo any reason this was merged without any status change at all?

This issue was still missing test implementations. Also, no credits were given yet. @danielspeicher, @jurgenhaas and others contributed quite a bit here!

I'll create a follow-up issue for fixing / adding the missing tests.

@Grevil I've merged but still need to make some changes before I consider this issue resolved, which I'm currently working on.

I've given attribution when I merged, maybe that commit is still local and I need to push to origin.

Commit:
e99a8547c3f9d5cee4845be049b7f2acabec9274 [e99a854]

Issue #3074688 by johne, jurgenhaas, ankithashetty, danielspeicher, tobiasb, Grevil, JayDarnell, Ron Collins, mrinalini9, fjgarlin: [PP-1] Password can not be reset, when user hasn't accepted the legal terms yet

Ok.

Credit through commit isn't enough anymore, you need to additionally credit users through the "Credit & commiting" section when adding a new comment in the issue.

@Robert Castelo I finished the tests in #3447348: Fix and add remaining test now.

EDIT: Ok, they still currently fail. Needs some further work. No more noise here.

@Grevil I fixed the tests and also an issue with the one time login message not appearing on the password reset page.

I already credited everyone who contributed code in the "Credit & committing" section when merging the issue through the issue page, let me know if there's anything else that needs to be done to give credit.

Everyone seems to be properly credited now, thanks! :)