@robert-castelo, can you review and merge these two MRs, one each for the 3.x and 4.x branches, please?
john franklin → created an issue.
john franklin → created an issue.
john franklin → created an issue.
john franklin → created an issue.
john franklin → created an issue.
john franklin → created an issue.
john franklin → created an issue.
There's no easy way to do this without duplicating the SVG Image → module. SVG support should be added to core directly.
I've added a patch that falls back to the original ImageItem and ImageWidget versions of the file widget, removing the false hope of SVG support.
Merged with latest 8.x-2.x, cleaned up the residuals, merged. Thanks for the patch, @arunsahijpal!
Noting support for Drupal 11, dropping support for Drupal 8, closing this as Drupal 11 is out and this module supports it.
john franklin → created an issue.
Thank you for reviewing the MR. I think #1 makes sense and I'll update the MR to go that route. #2 is more complicated than I was intending.
For clarity, my road map is:
step 1, refactor code into KeyPair class, no implementation changes (this MR)
step 2, change the class implementation of KeyPair to replace phpseclib with base PHP OpenSSL calls in a way that doesn't change the behavior of KeyPair. (near future MR, possible new major version)
step 3, add new functionality to KeyPair, such as key generation, again using base PHP OpenSSL calls. (future MR)
@roderik, can you take a minute to review this?
john franklin → created an issue.
Drupal 11 is out and the 1.x-dev branch supports D11. There is no need to keep this ticket open any longer.
Drupal 11 is out. There's no need to keep this open any longer.
@jsacksick - can you add a unit test that demonstrates the facets issues you're experiencing?
@jsacksick - can you add a unit test that demonstrates the facets issues you're experiencing?
Shifting back to needs work to handle the merge conflict.
Closing as a duplicate of 🐛 Error: Call to a member function load() on null BgImgFieldFormatter Active .
I just merged @nicholass's patch in 🐛 Error: Call to a member function load() on null BgImgFieldFormatter Active , which cleans up some of the phpcs findings. Make sure you're using the latest 8.x-2.x.
Thanks for the patch. This cleans up so much in the module!
john franklin → made their first commit to this issue’s fork.
john franklin → created an issue.
The constraint violation in the users_revision table does imply a problem with the user_revision module.
It may be triggered by a User::save() call in legal. I'll take a look at it.
Thanks for the updates @kul.pratap. A completely green pipeline is a lovely thing to see! I updated the cspell handling to inject unknown words in the `.gitlab-ci.yml` file instead of recreating the whole .cspell.json file, and backported the changes to the 3.x branch.
@robert castelo, there are two MRs attached here:
MR 22 - Applies updates to fix the "validate" class of tests in the pipeline for 3.0.x. This is as close as we're going to get to green until that last phpunit test is fixed in 3.0.x.
MR-24 - Applies updates to fix the "validate" class of tests in the pipeline for 4.0.x. This brings the pipeline to full green.
I don't think it's appropriate for me to RTBC this as I'm one of the people contributing the fixes. @riddhi.addweb, can you check on 3.x or 4.x?
Thank you, @robert castelo.
I'll re-roll the phpcs patch to address 3.x and 4.x later tonight.
It has been about a month. Have you had a chance to look at it, or will you be able to this weekend?
MR 22 is the correct one. I've hidden 23 and deleted the branch from git. MR 22 passed PHPCS in the pipelines. I'll rerun it and see what comes back.
john franklin → changed the visibility of the branch 3471496-test-pipelines to hidden.
And thank you for looking at the patch. I appreciate it.
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.
Contacted @Robert Castelo via his Drupal profile contact form.
john franklin → created an issue.
john franklin → created an issue.
Adding related issues.
Fixed.
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.
Now with passing tests, including the password reset test.
john franklin → created an issue.
john franklin → created an issue.
Fixed. All tests pass!
john franklin → made their first commit to this issue’s fork.
john franklin → created an issue.
john franklin → created an issue.
Can this patch be updated to create a new radioactivity
entity if there is a field reference but no entity? The module does not create entities when we add a new radioactivity field to a content type/taxonomy/etc. with exiting content.
It seems sensible to (offer an option to) create entities on demand. The downside of doing so is we have to call save()
on the entity during processing, which may trigger a lot of other activity, including adding the node to a workflow queue.
Option B is to update the field data directly without calling save() on the parent entity, which bypasses revision histories and may have its own issues.
Thanks for the patch @MakaziMtingwa. The WSOD is gone and the drush command is a workable solution that keeps this patch simple.
Done is done.
The "test-pipelines" MR is 3.0.x, minus the LICENSE.txt file. The unit tests fail in exactly the same way when running against 3.0.x as they do the fix-phpcs-issues MR, I have to consider the MR to be valid.
There is one test failing in the pipelines. The same test fails in other recent MRs.
john franklin → created an issue.
@roderik Since you've marked this as RTBC, I'll merge the branch so we have some schema in place.
The patch focused on what I saw in some sample configs, without making any judgement about whether or not it is appropriate it is to include, or if anything is missing.
If I'm going to update to make config consistent, then I'm inclined to exclude from the config anything extracted from the key that is just stored for caching purposes. That would mean keeping the fingerprint to verify a key passed in by environment variable is the expected key, and excluding format, key_size, etc.
Closing as stale.
John Franklin → created an issue.
Does the patch in 🐛 Don't log out users who do not accept the T&C Needs review fix TFA?
@Anaconda777, do you have an old patch from 🐛 Don't log out users who do not accept the T&C Needs review applied to your site? The LegalNavigationLock is introduced by the patch in that issue and doesn't exist in the current code base.
@aleix, does the patch in 🐛 Don't log out users who do not accept the T&C Needs review work for Commerce Checkout?
Thanks for the amazing work here @TR! I realize this was a huge task that was unexpectedly thrown into your lap, and if there's somewhere we can make a contribution as a small token of gratitude for your efforts please let us know.
Hear, hear!
Fixed, thanks for the patch!
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?
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.
John Franklin → changed the visibility of the branch 2897486-dont-logout-the to hidden.
John Franklin → changed the visibility of the branch 3.0.x to hidden.
TIL. Thanks for the patch!
John Franklin → made their first commit to this issue’s fork.
On second thought, based on Drupal's module composer.json guidelines → , there's no need for the composer.json at all.
Done and done. Thanks for the patch.
John Franklin → made their first commit to this issue’s fork.
The patch in the MR will fix up the parameters and fix the issue. This requires a patch from ✨ Add native support for Auth0 Needs review : https://www.drupal.org/files/issues/2023-03-29/openid_connect-3327237-26... → .
I'll merge the MR when they have theirs merged. Marking "postponed" until they do. Until then, their patch plus this one will fix the issue. Note: you have to include the logout redirect URL in the login.gov application. 🐛 Show the logout redirect URL in the plugin form Needs review adds the logout redirect URL to the plugin edit form for easy copy/paste.
MR attached, marking Needs Review
Is there a more generic way we could implement the logout query override so the plugins are returning the URL or maybe just the query parameters rather than implementing a non-standard hook_alter? We already have OpenIDConnectClientBase::getRedirectUrl()
At first I thought a getRedirectLogoutUrl()
would work, but the URL requires the path from the main openid_connect
settings, which feels inappropriate to read from inside a plugin. Maybe OpenIDConnectClientBase::getLogoutUrlOptions()
similar to OpenIDConnectClientBase::getUrlOptions()
?
Noting here: Login_gov also needs to fix up the logout redirect parameters in a very similar way.
🐛
Handle logout URL
Active
depends on the alterLogoutRedirectionQuery()
. I'm going to hold off merging the login_gov MR until this one is finalized.
John Franklin → created an issue.
Marking as "needs information." Will close in 30 days.
That doesn't look right. user_password()
is a fallback for when the password_generator
service doesn't exist.
It may be better to drop support for 9.x entirely and simplify that function a one-line wapper for the password_generator
service.
John Franklin → created an issue.
John Franklin → created an issue.
MR filed, assigning to @roderik for review.
John Franklin → made their first commit to this issue’s fork.
John Franklin → created an issue.
Leaving open for additional patches from Project Bot
Merging and leaving open for additional Project Update Bot patches.
@theprodigy, have you had any other issues with it? This module isn't so much in beta as it requires the 3.x version of OpenID Connect, which at the time you tried it, was missing a required patch.
They've since posted a new alpha of the OIDC module and login_gov should work with a straight-up composer require drupal/login_gov
.
Fixed.
Good catch @mitthukumawat! Thanks for the patch!
Thanks.
It took me a long time to get around to applying for security opt-in permissions, too. Now that I have it, I'm trying to put it to good use.
Regarding the three bullets you have listed there:
- Config schema -- it's a good thing to add, but its absence doesn't break anything. If I get around to it, I'll file an MR.
- I'd suggest closing the phpseclib2 ticket. It's only used to parse the keys and certs, so dropping the dependency entirely and doing it in straight PHP is an option. I've worked with the built-in PHP encryption API before.
- Pretty much anything I do with the module, I'll file an MR for and let you take a look at it. Many eyes make all bugs shallow, as they say.
RTBC+1. Tested with openid_connect + login_gov.
Submitted MR
MR created
John Franklin → created an issue.
John Franklin → created an issue.