Consider removing or fixing SOCIALACCOUNT_EMAIL_AUTHENTICATION_AUTO_CONNECT

[ryanhiebert]

In #3397 my colleague and I advocated for a way to auto-connect users for email authentication. However, I think that the way it is implemented doesn't give appropriate hooks for oversight before the connections are created, and changing that sounds like a difficult task that I'm not sure is worthwhile.

The main problem is that auto-connection happens before any signal like pre_social_login, so even though I'm locking down some restrictions to what social apps can be used in various scenarios, the social account is created and connected already, even though the SSO restrictions would stop the user from logging in.

We could try moving the auto-connection to after where we call that pre_social_login, but after seeing how simple the call is, I'm wondering whether the better approach might be to encourage people in my scenario to just call sociallogin.connect(context.request, self.user) from that hook.

As an aside, I've delayed setting up SCIM, so for the moment I'm neither using the setting nor the approach I suggested to call sociallogin.connect from the pre_social_login hook.

[pennersr]

"Removing" causes backwards compatibility issues, so there should be a good reason to have it removed. "Fixing" suggests there is an issue/bug -- is there?

[ryanhiebert]

I suppose "bug" depends on your vantage point. I can't use it because the pre_social_auth hook, where I check for some constraints before permitting SSO to go forward, doesn't happen until after the connection has been made. In other words, there's no opportunity to reject the social login before it creates the social account.

In a slightly different scenario than I've ended up with, I'd like to be able to check some things about the user before I connect it. For example, it's fine to connect to the user if the user was logged in before the SSO launch (or if this particular launch was for a user that is only associated to one tenant), but if they are associated to multiple tenants I need to verify that this SSO provider is acceptable to the pre-existing user.

[ryanhiebert]

I advocated for something like this setting, and if I had to do it over again I'm not sure I would do that, so I'm feeling guilty for putting that burden of maintenance on you.

[pennersr]

Isn't it possible to add your checks to the can_authenticate_by_email() adapter method?

https://github.com/pennersr/django-allauth/blob/main/allauth/socialaccount/models.py#L339

That one is called just before the connect occurs.

[ryanhiebert]

I don't think so: it only gets the email as a parameter. pre_social_login gets me the sociallogin, which includes both the identified user (if it exists) as well as the social app. I use the social app to determine what tenant the launch is for, and it's the combination of the tenant settings and user settings that determine whether I can let the login go forward.

[pennersr]

so I'm feeling guilty for putting that burden of maintenance on you.

No worries -- but given that it is in I am a bit reluctant to break backwards compatibility on this,

pre_social_login gets me the sociallogin

The first parameter passed to can_authenticate_by_email() is the login. Can you have another look?

[ryanhiebert]

The first parameter passed to can_authenticate_by_email() is the login

So it is, I apologize. I had not considered that method appropriately. Thank you for correcting me and getting me to take another look at it.

I see two challenges that make me think that it may not not the optimal place to do the checking that I do.

At this point in the request lifecycle, the user has not yet been looked up, so if I want to be inspecting properties of the user so that I can use them to help me determine whether they are allowed to log in via this SSO, I'll have to do that lookup myself. This is a sign that I might not be using the right hook, but it can work.

The bigger challenge that I see is that this isn't the right place to do this same checking on subsequent logins, because it won't be run if there is a social account. The validation that I do in the pre_social_login method is important even after the first time, such as ensuring that my tenant's SSO is still enabled and that my user is still active in that particular tenant. So I'd still need to override a different hook with the same functionality.

For a first-time login I want to make these checks before I add a social account, but I still need to do these checks every other time they authenticate as well. Except for this challenge for the account auto-connect, pre_social_auth seems ideally placed for that purpose. There's a signals.social_account_updated signal that gets called for when the social account already exists, but doesn't immediately feel like the Right Way to do this.

[pennersr]

Would #3665 solve this issue?

[ryanhiebert]

Yes, I believe it would.

[ryanhiebert]

I expect I'll be able to use the feature with that adjustment.

[pennersr]

Great, merged.