fix: allow social account linking only to useraccounts of type PERSON

[gounux]

This PR intends to allow the creation of allauth's SocialAccount only to useraccount having the PERSON type. Thus disabling Social Accounts creation and SSO login for email addresses related to Organization.

It has been observed that if an Organization and a User have the same email address, login via SSO using this email address may result in being connected as the Organization, which is not alright. The meaning of this PR is also to avoid that.

Steps to reproduce :

1. in Django Admin create an Organization o with email address A
2. in Django Admin create a People p with same email address A
3. login using a 3rd party IDP with this same email address A

-> without the changes brought by this PR, you are logged in as the Organizationo, created at step 1.
-> with the changes, you are logged in as the People p, created at step 2.

Other cases covered by this PR :

Login with 3rd party IDP with only the Organization having the A email address -> blocked

1. delete any Social Account related to the email address A mentioned above, in Django Admin
2. change the p People's email address
   -> there is only the Organization o having the A email address
3. login using a 3rd party IDP with this same email address A
   -> it fails, since SSO login for Organization is not allowed, with an error message displayed : Can login via SSO only to user accounts, not organizations.

Login with 3rd party IDP with no A email address anywhere -> new Social Account and new QFieldCloud user created

1. change the o Organization's email address
   -> there is no more Organization neither People having this A email address
2. login using a 3rd party IDP with this same email address A
   -> you're logged in as a new QFieldCloud user, that was created during the login and to which a new Social Account is binded (for this you need QFIELDCLOUD_ACCOUNT_ADAPTER=qfieldcloud.core.adapters.AccountAdapterSignUpOpen is the .env environment file)

[duke-nyuki]

Task linked: QF-7958 SSO: fix getting account linked with organization

[lukasgraf]

I would try to emulate allauth's SocialLogin._lookup_by_email as much as possible here. And just where it gets the results from filter_users_by_email then filter that list of users to only keep the ones that match type == User.Type.PERSON.

Because with this implementation, we're missing several parts of the functionality that allauth does during that lookup:

• Making sure to prefer verified email addresses
• Lower casing emails to make lookups case insensitive
• Making unicode-aware comparisons for the email addresses
• Properly setting sociallogin._did_authenticate_by_email = email

[lukasgraf]

I'm not sure if the exception handling here (and above, for missing email) is ideal.

For one, this code path will also be taken when a user signs up using QField or the QFieldSync plugin. In that case, messages.error() will have zero effect, and the user never sees that message.

The other thing is that this would lead to the following situation: When an Organization exists with a particular email address, but no Person, then a user with that email in his IDP account would not be able to sign up, because we prevent it here.

I think we should instead modify the original behavior of django-allauth as little as possible, and only solve the issue of not allowing organizations / teams to be picked during lookup. And this would mean an implementation like this:

• Check if the account is anything else than a Person
• If yes:
  • Restore the sociallogin object to exactly the state it was before allauth did the lookup that found the origanization via the email lookup. That means undoing these changes:
    • Reset sociallogin.user
    • Reset sociallogin._did_authenticate_by_email
  • Then, we perform another email lookup ourselves. Emulating as much as possible from the original SocialLogin._lookup_by_email, but: Instead of directly using the results of filter_users_by_email(email, prefer_verified=True) here, first filter them to throw out anything that is not a Person. Then continue as normal.

For the case described above (org exists with email, but no person), this should then result in sociallogin.is_existing being False, because we reset it, and didn't find an existing user, and allauth proceeding normally, which means it will create the account and link it.

[gounux]

Okay I see, thanks ! Regarding the exception handling, should we just throw an Exception ?

The other thing is that this would lead to the following situation: When an Organization exists with a particular email address, but no Person, then a user with that email in his IDP account would not be able to sign up, because we prevent it here.

Yes, that's the tricky point. And indeed it makes sense to allow such a case, rather than throwing and blocking it.

Though, after having tried to implement the logic, resetting the sociallogin.user to None creates a problem later on, since the is_finalized property is called again, and the user property is None so this if self.user.pk is None call fails.

For the case described above (org exists with email, but no person), this should then result in sociallogin.is_existing being False, because we reset it, and didn't find an existing user, and allauth proceeding normally, which means it will create the account and link it.

Wondering what the "Reset sociallogin.user" means here and how we could get in the same state before the lookup, need to investigate allauth deeper,

Also, I tried to create a new Person and assign it to the sociallogin, somehow the flow then lands into the allauth's raw Social Signup form, saying You are about to use your <IDP> account to login to QFieldCloud. As a final step, please complete the following form: + Email and Username pre-filled. With a An account already exists with this email address. Please sign in to that account first, then connect your Google account. after validating.
IDK if this is a good approach to dig further,

[lukasgraf]

Though, after having tried to implement the logic, resetting the sociallogin.user to None creates a problem later on, since the is_finalized property is called again, and the user property is None so this if self.user.pk is None call fails.

Ahh, right :-/
django-allauth does this weird thing during signup / login where it first creates a "fake" / empty user, and assigns it to the sociallogin object. The further it progresses during the flow and gets more pieces information (is the useracount already linked to a socialaccount? Does the user with that email exist? What profile metadata did the IDP provide? ...) it then continously enriches / populates that empty user with more data. In the case of new user creation, it then saves it, and otherwise (I think, if I remember correctly) it replaces it with a real user that is re-fetched from the DB.

It's all a bit convoluted how allauth does this, and the exact order in which those things happen make it difficult to customize the behavior.

So I think I agree with you here - not sure if this approach is worth following.

Monkey patching the filter_users_by_email would be much simpler and cleaner, I think. It should be as simple as patching it like this, more or less (untested):

from django.allauth.account import utils as allauth_utils

orig_filter_users_by_email = allauth_utils.filter_users_by_email


def patched_filter_users_by_email(
    email: str, is_active: Optional[bool] = None, prefer_verified: bool = False
) -> List:

    users = orig_filter_users_by_email()
    return list(filter(lambda user: user.type == User.Type.PERSON, users))

allauth_utils.filter_users_by_email = patched_filter_users_by_email

The main challenge with this approach is running this code that does the patching in the right module / at the right time. It needs to run before any modules have already imported the filter_users_by_email function, otherwise they will not be affected by the patch.

Let's maybe discuss this tomorrow.

[gounux]

Thanks a lot @lukasgraf :)

So this commit (51bef4e) intends to patch the several places where allauth is using the filter_users_by_email, identified with a grep.

Current feeling is that it's quite hacky, also, what if some new versions of allauth change the way this utils filter function is used, or at new code places ? Will it be necessary to update this patch ?

Anyway, LF to review / discussion regarding this,

Also, with this patch applied I tested those different cases :

• Only org has the email address -> a new QFieldCloud user is created, social login does not result in linking the Org account so this case is okay I believe ✅
• When there is already an existing user with the email address -> raw allauth sign up form is displayed, email pre-filled but username not pre-filled, and An account already exists with this email address. Please sign in to that account first, then connect your <IDP> account., so this case is KO I guess ❌

[lukasgraf]

As discussed:

• I would wrap up the patching into its own function and own module, and just make that a one line call from apps.py, to keep it self contained
• It should only be necessary to patch the function in the module where it is defined, but not in the places where it's imported (IF our patch is early enough -> test)
• The second case that you describe that fails is surprising to me. d.allauth should just link the account in that case. I have a feeling that this is caused by something else, and not the patch

I'd also like to hear what @suricactus thinks about the monkey patch approach.