Skip to content

Add optional SAML 2.0 SSO support#153

Closed
rodchristiansen wants to merge 1 commit intograhamgilbert:masterfrom
rodchristiansen:feature/sso
Closed

Add optional SAML 2.0 SSO support#153
rodchristiansen wants to merge 1 commit intograhamgilbert:masterfrom
rodchristiansen:feature/sso

Conversation

@rodchristiansen
Copy link

Adds optional SAML 2.0 Single Sign-On using djangosaml2/pysaml2. Entirely opt-in via environment variable — no change to deployments that don't enable it.

Supported identity providers

  • Microsoft Entra ID (Azure AD)
  • Okta
  • OneLogin
  • Any SAML 2.0-compliant IdP

Quick start

Set SAML_ENABLED=true and SAML_METADATA_URL in the environment. Example for Entra ID:

SAML_ENABLED=true
HOST_NAME=https://crypt.example.com
SAML_METADATA_URL=https://login.microsoftonline.com/TENANT_ID/federationmetadata/2007-06/federationmetadata.xml?appid=APP_ID

Changes

  • fvserver/saml_settings.py — new SAML configuration module built from environment variables
  • fvserver/system_settings.py — conditionally loads SAML config, adds djangosaml2 to INSTALLED_APPS and SamlSessionMiddleware, registers Saml2Backend
  • fvserver/urls.py — conditionally mounts /saml/ URL namespace
  • setup/requirements.txt — adds djangosaml2==1.9.3 and pysaml2==7.5.0

New endpoints (when enabled)

Endpoint Purpose
/saml/login/ Initiate SSO login
/saml/acs/ Assertion Consumer Service
/saml/metadata/ SP metadata XML
/saml/sls/ Single Logout Service

Local Django login at /login/ continues to work alongside SAML.

Add optional SAML 2.0 Single Sign-On integration using djangosaml2.
Enable by setting SAML_ENABLED=true environment variable.

Features:
- Support for Microsoft Entra ID, Okta, OneLogin, and any SAML 2.0 IdP
- Environment variable based configuration (no code changes needed)
- Automatic user provisioning on first login
- Coexists with existing Django authentication

New files:
- fvserver/saml_settings.py - SAML configuration module
- docs/SAML-SSO.md - Setup and configuration guide

Modified:
- fvserver/system_settings.py - Conditional SAML loading
- fvserver/urls.py - Conditional SAML URL routing
- setup/requirements.txt - Add djangosaml2 and pysaml2
Copilot AI review requested due to automatic review settings March 3, 2026 05:00
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds an opt-in SAML 2.0 SSO integration for the Django app using djangosaml2 / pysaml2, enabled via environment variables and conditionally wired into settings + URL routing.

Changes:

  • Adds djangosaml2 and pysaml2 dependencies.
  • Conditionally augments Django settings (apps, middleware, auth backend) when SAML_ENABLED=true.
  • Conditionally mounts the /saml/ URL namespace and introduces an environment-driven fvserver/saml_settings.py.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
setup/requirements.txt Adds SAML library dependencies.
fvserver/urls.py Conditionally includes djangosaml2 URL routes under /saml/.
fvserver/system_settings.py Adds SAML_ENABLED toggle and conditionally configures SAML app/middleware/backend/settings.
fvserver/saml_settings.py New module constructing pysaml2 / djangosaml2 settings from environment variables.
Comments suppressed due to low confidence (1)

fvserver/system_settings.py:226

  • These globals()[...] assignments are redundant because the settings are already present in this module namespace via the from fvserver.saml_settings import ... import above. Keeping both patterns increases maintenance cost and can confuse readers about where settings are defined; consider removing the globals() writes and just rely on the imported names.
    # Export SAML settings for djangosaml2
    globals()["SAML_CONFIG"] = SAML_CONFIG
    globals()["SAML_ATTRIBUTE_MAPPING"] = SAML_ATTRIBUTE_MAPPING
    globals()["SAML_CREATE_UNKNOWN_USER"] = SAML_CREATE_UNKNOWN_USER
    globals()["SAML_DJANGO_USER_MAIN_ATTRIBUTE"] = SAML_DJANGO_USER_MAIN_ATTRIBUTE
    globals()["SAML_USE_NAME_ID_AS_USERNAME"] = SAML_USE_NAME_ID_AS_USERNAME
    globals()["SAML_DJANGO_USER_MAIN_ATTRIBUTE_LOOKUP"] = SAML_DJANGO_USER_MAIN_ATTRIBUTE_LOOKUP
    globals()["SESSION_EXPIRE_AT_BROWSER_CLOSE"] = SESSION_EXPIRE_AT_BROWSER_CLOSE

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +216 to +217
# Override login URL to use SAML
LOGIN_URL = "/saml/login/"
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

When SAML is enabled, this overrides Django's LOGIN_URL to /saml/login/. That means any @login_required view will redirect to SAML rather than the existing /login/ form, which conflicts with the PR description that local Django login “continues to work alongside SAML”. Consider leaving LOGIN_URL as /login/ and exposing SAML only via /saml/login/, or gate this behavior behind a separate env flag (e.g., SAML_DEFAULT_LOGIN=true).

Copilot uses AI. Check for mistakes.

# SP Configuration from environment
HOST_NAME = os.environ.get('HOST_NAME', 'https://localhost').rstrip('/')
SAML_SP_ENTITY_ID = os.environ.get('SAML_SP_ENTITY_ID', f'{HOST_NAME}/saml/metadata')
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

Default SAML_SP_ENTITY_ID is missing the trailing slash (/saml/metadata vs /saml/metadata/). djangosaml2’s metadata endpoint (and the PR description) uses /saml/metadata/, so this default entity ID is likely to mismatch what the SP actually serves and what IdPs will store. Consider defaulting to ${HOST_NAME}/saml/metadata/ (or otherwise ensuring it exactly matches the routed metadata URL).

Suggested change
SAML_SP_ENTITY_ID = os.environ.get('SAML_SP_ENTITY_ID', f'{HOST_NAME}/saml/metadata')
SAML_SP_ENTITY_ID = os.environ.get('SAML_SP_ENTITY_ID', f'{HOST_NAME}/saml/metadata/')

Copilot uses AI. Check for mistakes.
Comment on lines +64 to +67
'want_assertions_signed': SAML_WANT_ASSERTIONS_SIGNED,
'want_response_signed': SAML_WANT_RESPONSE_SIGNED,
'allow_unsolicited': True,
'idp': {
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

allow_unsolicited is set to True by default. Accepting unsolicited assertions generally weakens SSO security because it allows IdP-initiated flows without a prior AuthnRequest and can make some replay/phishing scenarios easier. Consider defaulting this to False and making it explicitly opt-in via an environment variable.

Copilot uses AI. Check for mistakes.
Comment on lines +103 to +109
# Session settings
SESSION_EXPIRE_AT_BROWSER_CLOSE = True

# URL configuration
LOGIN_REDIRECT_URL = '/'
LOGOUT_REDIRECT_URL = '/'
LOGIN_URL = '/saml/login/'
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

These URL settings (LOGIN_REDIRECT_URL, LOGOUT_REDIRECT_URL, LOGIN_URL) are defined in saml_settings.py but are not imported/exported into the actual Django settings module (system_settings.py). As written, they won’t take effect and may mislead operators reading this file. Consider removing them from this module or wiring them through system_settings.py when SAML_ENABLED is true.

Suggested change
# Session settings
SESSION_EXPIRE_AT_BROWSER_CLOSE = True
# URL configuration
LOGIN_REDIRECT_URL = '/'
LOGOUT_REDIRECT_URL = '/'
LOGIN_URL = '/saml/login/'
# Session settings (Django session behavior should be configured in the main settings module)
SESSION_EXPIRE_AT_BROWSER_CLOSE = True

Copilot uses AI. Check for mistakes.
@grahamgilbert
Copy link
Owner

Thanks for this, but we’re moving away from the python codebase in #149

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants