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.
Tested against the login_gov + key + oidc_connect. Could drop support for Drupal 8.x.
This is functionally the same as the Project Bot patch in 📌 Automated Drupal 11 compatibility fixes for key RTBC , and cleaner to boot. Tested against login_gov + oidc_connect + key_asymmetric.
Merging this will make it just a little bit easier for modules that depend on Key to test against D11.
While it may be a bit of a head scratcher to set the core_version_requirement: to '>=8.9 <11 || ^11', it's not wrong. The MR in
📌
Allow installation and testing with Drupal 11
Fixed
is better, but this works. Marking this as RTBC.
John Franklin → created an issue.
John Franklin → created an issue.
Added an autocompleting textfield widget that searches for the route name based on the user's input. The autocomplete function searches the path aliases and router for the path or the explicit name of the route.
@maxilein: fixed in 1.0.1. Try it again, please.
Applied a modified patch that uses dependency injection instead of inline services.
There is one patch from Project Update Bot to replace a call to file_save_data() that is already in the 3.x branch. The other items cited are deprecations against OpenID Connect itself. Near as I can tell, all that needs to be done to be Drupal 11 ready is to tag this module as such in the .info.yml file. (So far as we can tell at the Drupal 11.0-alpha1 point, anyway.)
The patch in #36 breaks CSS/JS aggregation. Attached is a patch that allows those routes, too.
This only occurs with the patch from 🐛 Don't log out users who do not accept the T&C Needs review applied.
John Franklin → created an issue.
John Franklin → made their first commit to this issue’s fork.
John Franklin → created an issue.
John Franklin → created an issue.
John Franklin → created an issue.
John Franklin → created an issue.
Thanks for the patch, @mattsqd!
John Franklin → created an issue.
Thank you!
A stable 3.x would be ideal, but even a new alpha release would let login.gov → work without a patch.
Applied the patch from Update Bot, closing.
Fixed phpcs issues.
Closing as a nonsense issue.
John Franklin → created an issue.
The boring technical reason is that the commit message only mentions me. The root cause is @manuel.adan probably didn't notice the patch I posted was just your patch posted to make more obvious to others, or maybe he has some automated process. You should have gotten top billing on the commit.
Editing the commit isn't feasible, but we could add another patch to the README with a commit message that mentions you and even the hash of the commit for this issue. Something like "Issue #3219736 by neschi: Credit where credit is due for f5b91ade."
Done. Bumping the version requirements, because OpenID Connect 3.0-alpha is the first that both works with this module and supports D10.
@SomebodySysop, there is a Drupal 10 release now.
Closing, fixed in 1.x-dev.
The libraries.yml is missing a URL key for the license. It is just a noisy error. I'll get a patch up for it.
Better patch, including hook_legal_allowed_routes_alter() and excluding anonymous and blocked users who just tried to log in.
Re-roll of #26 for Legal 3.0.x including the LegalNavigationLock EventSubscriber, which was left out of #28.
Closing as fixed. @SomebodySysop, you can pull the latest dev version until we can get a new release cut.
Thanks for the patch, @Project Update Bot.
laura.gates → credited John Franklin → .
John Franklin → created an issue.
The 2.x version suffers the same and patch in #2 applies cleanly. This is a simple and obvious fix that can be applied in 2.x and cherry-picked into 3.x.
The current version supports 8, 9, and 10. Closing.
Looking to current External Authentication codebase, I see no routes, not configurations, no forms. That's great! It's pure API and it should stay as it is.
The need to validate claims, both per se and against existing users, is a common enough use case that the functionality should exist. I'm running into this same issue on my own projects. Yes, the Drupal API allows for users with missing or duplicate email addresses, and the UI does a lot of the user validation work. However, with External Auth, that validation is bypassed, and it falls on each module building on External Auth to add it back in, which strikes me as duplication of effort.
I can see the merits of keeping External Auth as an API-only module. That said, just as External Auth handles the common parts of any external authentication solution, IMHO it should also provide that common validation, while allowing it to be optional.
There are two major ways I can see the optional validation happening while keeping External Auth as an API-only module:
1) provide hooks/functions/API parameters/etc. to allow calling modules to trigger the validation.
2) provide a separate sub-module that implements the validation which site builders can optionally enable if they need it.
The first favors module developers who need to add a '#validate' => TRUE somewhere, or need to call $this->externalAuth->validateData() (or similar). The second favors site builders, but may require some refactoring of the events triggered by External Auth, perhaps incorporated into
✨
Redo authmap_alter / register events.
Needs review
.
Thoughts?
p.s. I'm intentionally not re-opening this without a clearer idea of where the community wants to go.
Reroll of #13 against 3.8.
+1 this patch.