Skip to content
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

Jump to bottom

Add basic support for SSO using OpenID Connect #1159

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

Add basic support for SSO using OpenID Connect #1159

wants to merge 10 commits into from
+159 −10

Conversation

lukasgraf
Copy link

@lukasgraf lukasgraf commented Mar 13, 2025 •
edited
Loading

This adds basic support for SSO with OAuth2 / OpenID Connect to QFC, using django-allauth's socialaccount features.

Functionality not yet contained in this PR:

  • QGIS Authentication Middleware
  • API Endpoint for listing enabled providers

With this, we set up the basic building blocks to support authentication using django-allauth's socialaccount providers. No actual provider is configured to be used yet - so this PR should be safe to deploy, and not affect the default behavior yet. Everything will be in place though so that a specific provider can simply be added to SOCIALACCOUNT_PROVIDERS in settings.py.

Changes include:

  • Install the django-allauth[socialaccount] extra to pull in necessary dependencies
  • Prepare Google and Microsoft as potential providers (not configured yet though)
  • Customize the admin login page so that social logins can be chosen, if they are configured. This is currently hidden behind an sso=1 query string parameter while the feature is still in development.
  • Show the SocialAccount related models in the admin panel
  • Improve django-allauth's error logging
  • Configure various SOCIALACCOUNT_ settings to get the login, signup and account linking flow to work the way we want
  • Customize username generation for signups via social logins

Testing locally

To test these changes locally, add a SOCIALACCOUNT_PROVIDERS definition to your settings.py:

SOCIALACCOUNT_PROVIDERS = {
    "google": {
        "APP": {
            "client_id": "<your-client-id>",
            "secret": "<your-client-secret>",
            "key": "",
        }
    }
}

Then visit the login page, and append &sso=1 to the URL:
sso_login

@lukasgraf
Add django-allauth[socialaccount] extra:
3460865 
This will pull in the necessary dependencies for django-allauth to handle
OpenID Connect / OAuth2 flows.
@lukasgraf
Add Google and Microsoft as potentially supported IDPs:
beace4d 
This adds Google and Microsoft as potentially supported OpenID Connect
providers. It does not yet enable them, so they can't be used to log in yet.

Enabling them will require to add them to the SOCIALACCOUNT_PROVIDERS setting,
and define their configuration (client ID, etc..). Or add a configuration for
them as a SocialApp in the database, through the admin panel.

So this change just prepares QFC to possibly handle these two providers, once
we decide to add configuration for them in a particular deployment.
@lukasgraf
Customize admin login page to offer social logins:
5220a11 
This is a combination standard admin login page template from Jazzmin, plus
the snippet from django-allauth that adds the "Or use a third-party" with the
provider list at the bottom.

The (potential) SSO login options for the Jazzmin admin login page are
hidden behind a ?sso=1 query string parameter.

Doing this leaves the standard admin login page unchanged, and allows us for
a soft rollout of the feature. This is not a security measure at all, but a
"feature flag", intended to not accidentally confuse users / admins while the
feature is still in development.
@lukasgraf
Unhide socialaccount models from admin panel:
16c57d3 
Since the SocialAccount related models from django-allauth are now actually
used in QFC, they should not be hidden in the admin panel any more.

This allows us to easily inspect / debug linked social accounts for users.

It also lets us create SocialApp configurations through the web (in the
Django database) as an alternative to defining them in settings.py.
@lukasgraf
Add adapter that logs stack trace on 3rd party authentication errors:
4409f45 
The default django-allauth "Third-Party Login Failure" page is
decidedly unhelpful, because it is completely devoid of any error details.

And when errors happen in the callback view from the external provider, they
are usually quite tricky to debug already.

This adapter therefore makes sure the error details are logged, and especially
the stack trace is logged as well.

It also adds the stack trace to the template context. This would allow the
stack trace to be displayed in the socialaccount/authentication_error.html
template. We don't do this yet, because in production this could pose a
security risk (displaying stack traces to users, especially authentication
related ones). But in scenarios where this is deemed acceptable (some on-prem
deployments for example) the information is there, and can be displayed with
a simple template override.
@lukasgraf
Set SOCIALACCOUNT_QUERY_EMAIL = True:
c1b0de6 
This makes sure that the SocialAccount providers always get queried for the
user's email address, among other needed scopes.

The email address is required for the way we want to link social accounts to
possibly already existing user accounts in QFC, and setting this globally
avoids having to explicitly set this in the scope when configuring the
provider (which is an easy mistake to make, and leads to strange results).
@lukasgraf
Set SOCIALACCOUNT_EMAIL_AUTHENTICATION = True:
47762be 
This will (globally) allow any social account providers to be used to
authenticate with an existing user based on the email address that the
social account provider reports.

This has the following security implication:
If we ever configure and allow a malicious or negligent social account
provider to be used, that provider (IDP), or one of its users, could take over
an existing QFC account.

There are two scenarios to consider:

1) The provider itself is malicious.
The provider could then spoof any email address they want in the userinfo
they return to us, we trust it, and allow them to authenticate with an existing
user account that matches that email address.

2) The provider is negligent, and doesn't validate email addresses.
A user of that provider could sign up with the IDP using a fake email address
that doesn't get validated by the provider. The user would then be able to
authenticate as an existing QFC user with that email address.

Having said that, the big players  in terms of OpenID Connect IDPs (Google,
Microsoft, GitHub, ...) all do validate their user's email addresses (or even
control them). Therefore I think the risk of setting this globally (instead
of per-provider) is acceptable, compared to the risk of forgetting to set
EMAIL_AUTHENTICATION for every configured provider.
@lukasgraf
Set SOCIALACCOUNT_EMAIL_AUTHENTICATION_AUTO_CONNECT = True:
6126232 
This makes sure that users are not only able to authenticate using a social
account (via a matching email address), but that an existing QFC user account
then automatically gets linked with that SocialAccount.

This has the following side effect:
If the existing QFC user account has an email address that has NOT been
verified, the accounts password will be scrambled (made unusable).

The reason for this is a security concern:
This prevents "account squatting" - where a malicious actor would first
register with an email address of the victim, QFC doesn't validate that email,
and the malicious actor gets to set his own password.
When the victim then at some later point logs in using a social account, using
that email address, and his account gets auto-linked, then the attacker would
have a backdoor to the legitimate owner of that email address's QFC account,
by means of the password.

QFC currently does do email address validation. When checking in production,
we found that only 0.1% of users didn't have a validated email address, and
they didn't seem to be primary emails. In addition, password reset is possible
once the user logs into QFC using the social account. So their password
getting scrambled wouldn't be the end of the world, just a minor annoyance.
@lukasgraf
Set SOCIALACCOUNT_LOGIN_ON_GET = True:
7fe646d 
This skips the secondary form that requires you to confirm that you *really*
want to log in using <provider>, by not requiring POST requests to trigger
social logins.

The django-allauth docs mention a security concern about this, but after
looking into it, I think it's pretty much a non-issue:

The scenario they describe in the django-allauth 0.47.0 release notes assumes
that there is what's called an "open redirect" vulnerability on the IDP side.
That is a major, and long known security vulnerability that no big player like
Google, Facebook or Microsoft should have. I therefore think we can safely
enable this.
@lukasgraf
Customize username generation for signup with social login:
9443258 
When a user signs up via username and password, we try to respect their
choice of username, and just delegate to the default implementation to
avoid collisions.

For users that directly sign up via a social login however, we:
- Take the local part of their email (part before the '@' sign)
- Append a random 4-digit suffix to make it likely to be unique and
  not communicate any information about the existence of other users
- Let generate_unique_username() normalize the username and ensure its
  uniqueness.
@lukasgraf lukasgraf marked this pull request as ready for review March 14, 2025 19:33
@lukasgraf lukasgraf requested review from suricactus and gounux March 14, 2025 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@suricactus suricactus Awaiting requested review from suricactus

@gounux gounux Awaiting requested review from gounux

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lukasgraf @suricactus