Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gounux wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c23d1b2
fix: allow social account linking only to useraccount of type `PERSON`
gounux Feb 18, 2026
d2bb8f7
fix: remove `pre_social_login` override in the SocialAccountAdapter
gounux Feb 19, 2026
51bef4e
feat: patch allauth modules to filter users by email and consider onl…
gounux Feb 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 45 additions & 1 deletion docker-app/qfieldcloud/core/adapters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,21 @@
from allauth.account import app_settings
from allauth.account.adapter import DefaultAccountAdapter
from allauth.account.models import EmailConfirmationHMAC
from allauth.core.exceptions import ImmediateHttpResponse
from allauth.socialaccount.adapter import DefaultSocialAccountAdapter
from allauth.socialaccount.models import SocialLogin
from allauth.socialaccount.providers.oauth2.provider import OAuth2Provider
from constance import config
from django.contrib import messages
from django.contrib.auth.models import AbstractUser
from django.core.exceptions import ValidationError
from django.http import HttpRequest
from django.shortcuts import redirect
from django.utils import timezone
from invitations.adapters import BaseInvitationsAdapter

from qfieldcloud.authentication.sso.provider_styles import SSOProviderStyles
from qfieldcloud.core.models import Person
from qfieldcloud.core.models import Person, User

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -194,6 +197,47 @@ class SocialAccountAdapter(DefaultSocialAccountAdapter):
Logs stack trace and error details on 3rd party authentication errors.
"""

def pre_social_login(self, request: HttpRequest, sociallogin: SocialLogin) -> None:
"""
Invoked just after a user successfully authenticates via a
social provider, but before the login is actually processed
(and before the pre_social_login signal is emitted).

This is a good hook to check that the user is of type `PERSON`,
and not of type `ORGANIZATION` or `TEAM`,
as SSO login should only be allowed for user accounts.
"""

# check if allauth can find a useraccount with this email,
# meaning that the accounts would be linked and a social account created.
if sociallogin.is_existing:
# here we check that the user trying to login is of type `PERSON`.
user = sociallogin.user

if user.type != User.Type.PERSON:
# if the user is not of type `PERSON`,
# we try to find a user with the same email that is of type `PERSON`,
# and link the social account to that user instead.
# Otherwise we block the social account link to an Organization.

email = sociallogin.account.extra_data.get("email", "")

if not email:
messages.error(
request, "No email returned by the Identity Provider."
)
raise ImmediateHttpResponse(redirect("account_login"))

try:
person_user = User.objects.get(email=email, type=User.Type.PERSON)
Copy link
Contributor

@lukasgraf lukasgraf Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

sociallogin.user = person_user
except User.DoesNotExist:
messages.error(
request,
"Can login via SSO only to user accounts, not organizations.",
)
raise ImmediateHttpResponse(redirect("account_login"))
Copy link
Contributor

@lukasgraf lukasgraf Feb 18, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@gounux gounux Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Contributor

@lukasgraf lukasgraf Feb 18, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@gounux gounux Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ❌

Copy link
Contributor

@lukasgraf lukasgraf Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


def on_authentication_error(
self,
request: HttpRequest,
Expand Down
Loading