Problem/Motivation

If registrations are disabled on a site, but a user that exists in the identity portal tries to log in, the module will try to create the user, fail silently (messages will be added in watchdog and messenger) and try to redirect to the afterLogin route.

It will also trigger a LogginEvent, which is wrong as well.

If this route happens to be /user, you will end up in an infinite loop, as it will try to log the user in again. If it's not, then the user will see the error messages in a 403 page, if the "Post login path" route was restricted. If the page is public, then they will just see the error messages.

So, 2 out of 3 outcomes are not desired here.

Watchdog messages are:
- Failed to create user. User registration is disabled. Name: john-doe, email: john-doe@test.com
- Login for user john-doe prevented. Account is blocked. <=== this is wrong, it's just because it is the anonymous user, which is not active.

Proposed resolution

I suggest:
- Checking that registrations are disabled before trying to create a user, and redirect to the front page in that case.
- Checking that the user is authenticated before triggering the LoggingEvent

Remaining tasks

MR. I have the code already, I'm creating it.

Issue fork social_auth-3473324

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

fjgarlin created an issue. See original summary.

fjgarlin opened merge request !31

fjgarlin’s picture

fjgarlin commented
Status: Active » Needs review

The changes are ready for review.

heddn’s picture

heddn
Primary language English
Location Illinois
commented

Can/should we add a test to https://git.drupalcode.org/project/social_auth/-/blob/4.1.x/tests/src/Fu...?

wells’s picture

wells
he/him/his
Primary language English
Location Seattle, WA
commented
Status: Needs review » Needs work

Yes, we should include a test for this one.

fjgarlin’s picture

fjgarlin commented

I'm not sure I'd know how to begin. We'd need to mock remote data, set an integration and settings in it (like disallow registrations) and then test the workflow. I don't see any test doing something similar yet.

For context, I found this bug integrating the new D10 version of www.drupal.org with the new keycloak system. We want registrations to always go via D7, therefore not allowing registrations on D10.

When working with existing users, everything works well, but when the user does not exist yet in D10, then we get the above watchdog messages and behaviour.

If I get some boiler plate code for the above from any of you that know the module better I could finish it off. In this case, writing the fix seems way easier than writing the test for it.

wells’s picture

wells
he/him/his
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review

Ack. Good point. This would be difficult to test and I don't want to hold it up on that. Happy to merge it if someone else is able to do some functional review and get it to RTBC. Otherwise, I'll try to do so eventually. Thanks, @fjgarlin.

fjgarlin’s picture

fjgarlin commented

I actually found another case of infinite loop to "user.login" or a nasty error which is not captured. 

The website encountered an unexpected error. Try again later.

TypeError: Stevenmaguire\OAuth2\Client\Provider\Keycloak::getResourceOwner(): Argument #1 ($token) must be of type League\OAuth2\Client\Token\AccessToken, null given, called in /app/web/modules/contrib/social_auth_keycloak/src/KeycloakAuthManager.php on line 62 in Stevenmaguire\OAuth2\Client\Provider\Keycloak->getResourceOwner() (line 269 of /app/vendor/stevenmaguire/oauth2-keycloak/src/Provider/Keycloak.php).
Drupal\social_auth_keycloak\KeycloakAuthManager->getUserInfo() (Line: 308)
Drupal\social_auth\Controller\OAuth2ControllerBase->processCallback() (Line: 197)
Drupal\social_auth\Controller\OAuth2ControllerBase->callback()
call_user_func_array() (Line: 123)

If the credentials are wrong, we don't get a token back, so I'm checking for that before going any further. I discovered that the "processCallback" function does not handle any possible error, so I added a differentiation and multiple error codes that can help when debugging.

So, the current MR avoid two cases of infinite redirects to "user.login" and a case for an exception being thrown, and instead it captures the errors and redirects to the homepage instead.

--
Note that the phpcs warnings in the pipeline do not come from this MR. These are already present in the main branch.

wells’s picture

wells
he/him/his
Primary language English
Location Seattle, WA
commented
Status: Needs review » Needs work

Thanks for your work on this, @fjgarlin -- I left a question on the MR that I think will ultimately lead to some additional changes needed. Let me know your thoughts.

fjgarlin’s picture

fjgarlin commented
Status: Needs work » Needs review

Maybe you couldn’t see the whole MR code? The changes you were asking about were addressed already and shown in the MR.

fjgarlin’s picture

fjgarlin commented

I refactored the solution as per the feedback in the MR. It can be reviewed again. The approach is the same, it's about catching the error in the "processCallback" function, and then being able to act on it in the "callback" method to avoid an infinite login loop.

  • wells committed 42940076 on 4.1.x authored by fjgarlin 
    Issue #3473324 by fjgarlin, wells, heddn: Failed login can still...
wells’s picture

wells
he/him/his
Primary language English
Location Seattle, WA
commented
Status: Needs review » Fixed

Looks good to go -- thanks!

Status: Fixed » Closed (fixed)

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