Don't log out users who do not accept the T&C

+1 for 6

Thanks for the great patch Tamás!
Password reset works as expected, also the LegalNavigationLock is a great addon to module, furthermore this one could solve & close other tickets as well... Like 3005208, perhaps 2638224 too.

This seems to work a bit better than #3005208: Logging users out during T&C check breaks other modules. Still has the issue of blocks that should be restricted by user role are visible when viewing the Terms & Conditions.

Applied the patch from #6. Issue we were having is after coming from one time link in email, the user confirmed T&C but was asked to confirm his new password with a current password. This looks to be solved with the patch (Current password field is not there). However, after navigating to other pages on the website and comming back to the edit user profile, in a attempt to set the new password, the "Current password" field was there again. For our issue, I will try https://www.drupal.org/project/prlp (found this via issue https://www.drupal.org/forum/support/post-installation/2013-01-24/how-to...)

Reroll of patch from #6. Some of the issues addressed by #6 were fixed in other patches.

#10 didn't work for me. I get access denied on /legal_accept. Looking at the code there's nothing that actually sets the parameter or tokens that the access method is checking for.
#6 worked fine, but my drupal site is acting as an identity provider. When authenticating for access to another site (service provider), the legal module gets skipped. I ended up adding our legal text in a block to the user login page.

Sorry about that. Bad re-roll in #10. Ignore it. This one works better.

Fix an issue with a test that was already fixed.

Once more with feeling.

I updated the patch with some code for handling routes in the fork 2897486-dont-logout-the and opened a merge request.
The first commit on the branch is the patch from #14.

Most of the code I added/modified came from the related issue : https://www.drupal.org/project/legal/issues/3005208

This other issue was closed but I think some good things could be taken from the patches there.

- Access to the superglobals were cleaned up
- Some processing done during the login was abstracted in a somewhat "cleaner" way

The patches there were maybe too broad and hard to review but I think once this present issue is closed, we could open subsequent ones to try and get the rest of the code from issue 3005208 merged.

@gilles_webstanz why did you revert all of the changes on MR1?

sorry @wells it was one of my first contribution...I made a mistake with the MR...

I have this problem with 2.0 version, and this patch #15 does not apply to that.
(Users are logged out randomly after accepting legal during logging in)

Reroll of #15.

Hi,

Applied the patch #23 and was able to accept the terms and after about 20 tries no users were logged out.
So it works, but there is a new error with php 8.1

Deprecated function:

Deprecated function: Constant FILTER_SANITIZE_STRING is deprecated in Drupal\legal\Form\LegalLogin->submitForm() (line 171 of
contrib/legal/src/Form/LegalLogin.php)

And

Deprecated function: strlen(): Passing null to parameter #1 ($string) of type string is deprecated in Drupal\Component\Utility\Unicode::validateUtf8() (line 478 of /var/www/html/web/core/lib/Drupal/Component/Utility/Unicode.php)

And
Deprecated function: str_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated in Drupal\Component\Utility\Xss::filter()

Updated patch and fixed php errors regarding FILTER_SANITIZE_STRING

Hello,

Applied the patch #25 but got stuck in an infinite redirection loop to "/legal_accept". I believe it is because in the "LegalNavigationLock.php" file, the condition is based upon the current path and because my website is multilingual, the path is prefixed by the current language so the current path is "/en/legal_accept" rather than "/legal_accept" and it's never the expected path to test against.

I modified the patch to check the current route name instead of the current path.

Hi,
Just to notify that this is still an issue on 3.0.x.

Regards

Patch re-roll for D10 & Legal v3.0.x

Re-roll of #26 for Legal 3.0.x including the LegalNavigationLock EventSubscriber, which was left out of #28.

Better patch, including hook_legal_allowed_routes_alter() and excluding anonymous and blocked users who just tried to log in.

Hi,

Patch #30 works for me with D9 php8.1
Had previously patch #26 which did log user out.

Super great!

Re-roll of #30 for Legal v3.0.x D10

Re-roll of #32

The previous patches break CSS/JS aggregation. Attached is a patch that allows those routes, too.

I've pushed up a new MR for this issue that does the following:

* Reverts #3074688 and #3414370. They are unnecessary and duplicate password reset handling from core.
* Re-rolls #37 for 3.0.x-dev, current as of this writing
* Restores the allowed_path handling that @Radelson added in Dec 2020

I have tested it with the standard password reset process and with OIDC Connect
I have *not* tested it with other password management modules, like PLRP.
There is some cleanup to satisfy phpcs and phpstan remaining, although maybe that should be a separate issue after this is merged.]

If you need the patch, please grab the "plain diff" at the top of the ticket.

Added in the following to the MR:

* Save the destination to the session under the legal.orginal_destination key, including the full URL for password resets.
* Fixed a couple phpcs issues
* Fixed the tests to work with the patch

@Robert Castelo, can you take a look a this MR, please?

The getUrl() in the test runner is returning http://localhost/web/web/user (note extra "/web") instead of http://localhost/web/user. No idea why.

Now with passing tests, including the password reset test.

I'm requesting priority review of this patch. If committed, I believe that this will close a number of issues in the queue, including:

• #3447348: Fix and add remaining test
• #3261075: Module Legal breaks the Drupal Commerce Checkout flow
• #3181699: Unable to Login With TFA
• #3409589: LegalNavigationLock
• #3336723: Destination is lost after user logs in
• #3239754: Not working with social auth modules
• #3116700: Compatibility with commerce checkout
• #3261074: Breaks the Drupal Commerce Checkout flow
• #3125093: Problem while setting destination on one time login link

Yes, this patch is a major change to the behavior and architecture of the module, enough so that a new major is warranted. However, it also simplifies the module such that we don't need to special case things like user password resets or retaining the destination query parameter, and we work seamlessly with external auth mechanisms. Most government websites require a T&C acknowledgment and more and more are moving to SSO solutions (e.g., PIV, login.gov) for authentication, making this patch a necessity.

Here is a brief description of what the patch does:

When the user first logs in, a check is made if they must acknowledge the T&C (legal_user_login()) and sets the legal.accecpt_form_lock and legal.initial_redirect flags in their session. The special casing of the destination parameter is removed, as is reimplementation of the password reset logic from the core User module. Redirecting in legal_user_login() is no longer necessary as the new LegalNavigationLock EventSubcriber will capture the user.

The new LegalNavigationLock EventSubscriber prevents the user from navigating to anywhere except legal banner page until they acknowledge the T&C. The user is no longer logged out, and we don't need to call exit() at the end of legal_user_login().

On the first redirect, the LegalNavigationLock saves off the request path and query. This saves the case of ?destination=path/to/page and retains the password reset token when coming in from a password reset link. If they user attempts to go anywhere else on the site, they are redirected back to the LegalLogin form page.

Once the user accepts the T&C, we redirect them to the original path with query parameters. As the session is never destroyed, and the query tokens are preserved, complex login processes like the password reset link are allowed to proceed and work as if LegalLogin never happened.

The LegalNavigationLock allows a handful of paths. The user.logout route allows users to break out of the legal page by logging out. The system.css_asset and system.js_asset paths are allowed so consolidated CSS and JS is generated and returned as expected. Two alter hooks, hook_legal_allowed_paths_alter() and hook_legal_allowed_routes_alter allow other modules to exempt certain paths and routes. For example, the OpenIDConnect module should add their openid_connect.logout route that overrides the user.logout route and sites that have a longer explanation of their terms and conditions may exempt that one page.

Additional tidying:

• Update to the latest gitlab-ci.yml template.
• Cleans up some dependency injections. More in #3471496: Fix PHPCS issues, which I will re-roll after this patch is merged.
• Since we no longer logout the user while accepting T&C, cookies are replaced with flags saved in the session.
• Flags set in the session are namespaced legal.flag_name

Credit and thanks to @yce for the initial implementation of the LegalNavigationLock in #6 five and a half years ago now.

Adding related issues.

I appreciate the amount of work that's gone into this patch, it's a major change though, and if I understand correctly triggers some logic on every request (?), so I think it needs a very cautious release.

What I plan to do is review the code and test Legal features by hand over the next few weeks, after which I'll release it as Legal 4.0.x-alpha and get wider feedback before moving to a beta and then a full release.

I'm not going to make any changes to the 3.0.x branch for a while, except adding the one line Drupal 11 patch, so in the mean time if you need to apply your patch to your project that should be easy to do in your Composer file.

If you have some tests run by hand that are not covered by the existing unit tests, please post them here and let's get them added to the unit test suite. This patch gets the unit tests to pass for the first time in a long time, and I'd like to see the unit tests provide full-coverage testing and remain maintained moving forward.

And thank you for looking at the patch. I appreciate it.

It has been about a month. Have you had a chance to look at it, or will you be able to this weekend?

Thank you, @robert castelo.

I'll re-roll the phpcs patch to address 3.x and 4.x later tonight.

Have released this as Legal 4.0.0.-alpha1

Couldn't find a way to credit the developers involved without merging to Master, but have added a description to the project page about the release and have given credit there.

Happy to add a commit in future and add the credits on that.