Account created on 30 December 2009, about 15 years ago
#

Merge Requests

More

Recent comments

🇺🇸United States John Franklin

@robert-castelo, can you review and merge these two MRs, one each for the 3.x and 4.x branches, please?

🇺🇸United States John Franklin

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.

🇺🇸United States John Franklin

Merged with latest 8.x-2.x, cleaned up the residuals, merged. Thanks for the patch, @arunsahijpal!

🇺🇸United States John Franklin

Noting support for Drupal 11, dropping support for Drupal 8, closing this as Drupal 11 is out and this module supports it.

🇺🇸United States John Franklin

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)

🇺🇸United States John Franklin

Drupal 11 is out and the 1.x-dev branch supports D11. There is no need to keep this ticket open any longer.

🇺🇸United States John Franklin

Drupal 11 is out. There's no need to keep this open any longer.

🇺🇸United States John Franklin

@jsacksick - can you add a unit test that demonstrates the facets issues you're experiencing?

🇺🇸United States John Franklin

@jsacksick - can you add a unit test that demonstrates the facets issues you're experiencing?

🇺🇸United States John Franklin

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.

🇺🇸United States John Franklin

Thanks for the patch. This cleans up so much in the module!

🇺🇸United States John Franklin

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.

🇺🇸United States John Franklin

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?

🇺🇸United States John Franklin

Thank you, @robert castelo.

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

🇺🇸United States John Franklin

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

🇺🇸United States John Franklin

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.

🇺🇸United States John Franklin

john franklin changed the visibility of the branch 3471496-test-pipelines to hidden.

🇺🇸United States John Franklin

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

🇺🇸United States John Franklin

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.

🇺🇸United States John Franklin

Contacted @Robert Castelo via his Drupal profile contact form.

🇺🇸United States John Franklin

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

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.

🇺🇸United States John Franklin

Now with passing tests, including the password reset test.

🇺🇸United States John Franklin

john franklin made their first commit to this issue’s fork.

🇺🇸United States John Franklin

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.

🇺🇸United States John Franklin

Thanks for the patch @MakaziMtingwa. The WSOD is gone and the drush command is a workable solution that keeps this patch simple.

🇺🇸United States John Franklin

Done is done.

🇺🇸United States John Franklin

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.

🇺🇸United States John Franklin

There is one test failing in the pipelines. The same test fails in other recent MRs.

🇺🇸United States John Franklin

@roderik Since you've marked this as RTBC, I'll merge the branch so we have some schema in place.

🇺🇸United States John Franklin

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.

🇺🇸United States John Franklin

Does the patch in 🐛 Don't log out users who do not accept the T&C Needs review fix TFA?

🇺🇸United States John Franklin

@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.

🇺🇸United States John Franklin

@aleix, does the patch in 🐛 Don't log out users who do not accept the T&C Needs review work for Commerce Checkout?

🇺🇸United States John Franklin

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!

🇺🇸United States John Franklin

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?

🇺🇸United States John Franklin

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.

🇺🇸United States John Franklin

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

🇺🇸United States John Franklin

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

🇺🇸United States John Franklin

John Franklin made their first commit to this issue’s fork.

🇺🇸United States John Franklin

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.

🇺🇸United States John Franklin

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.

🇺🇸United States John Franklin

Marking as "needs information." Will close in 30 days.

🇺🇸United States John Franklin

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.

🇺🇸United States John Franklin

Leaving open for additional patches from Project Bot

🇺🇸United States John Franklin

Merging and leaving open for additional Project Update Bot patches.

🇺🇸United States John Franklin

@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.

🇺🇸United States John Franklin

Good catch @mitthukumawat! Thanks for the patch!

🇺🇸United States John Franklin

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.
🇺🇸United States John Franklin

RTBC+1. Tested with openid_connect + login_gov.

Production build 0.71.5 2024