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

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 patch in #36 breaks CSS/JS aggregation. Attached is a patch that allows those routes, too.

John Franklin → changed the visibility of the branch 3.0.x to hidden.

John Franklin → changed the visibility of the branch 2897486-dont-logout-the to hidden.

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

* Reverts #3074688 and #3414370 that is unnecessary and duplicated 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?

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:

• 🐛 Fix and add remaining test Needs work
• 🐛 Module Legal breaks the Drupal Commerce Checkout flow Needs review
• 🐛 Unable to Login With TFA Needs work
• 🐛 LegalNavigationLock Postponed: needs info
• 🐛 Destination is lost after user logs in Postponed: needs info
• 🐛 Not working with social auth modules Active
• 🐛 Compatibility with commerce checkout Postponed: needs info
• ✨ Breaks the Drupal Commerce Checkout flow Postponed: needs info
• 🐛 Problem while setting destination on one time login link Active

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 🐛 Fix PHPCS issues Needs review , 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.

Automatically closed - issue fixed for 2 weeks with no activity.