Skip to content

Build manifest — SAML SSO on allauth (#697): prompts, decisions, and the auth-stack consolidation #711

Description

@surfdoc

Build manifest — SAML SSO on allauth (#697): prompts, decisions, and the auth-stack consolidation

What this is. The detailed record of how PR #697 (practitioner SAML SSO moved to django-allauth[saml]) was built with Claude — what was asked, what was decided at each fork, and why the code is shaped the way it is — deeper than user documentation. Read this before the PR: the diff shows what changed; this shows why, so no one has to reverse-engineer the security-sensitive decisions later. The step-by-step operator runbook (IdP SocialApp setup, signing flags, stable-uid requirement, back-out) lives in jupyterhealth/software-documentation#115, not here.

Raw provenance. §1 is the distilled decision log; the appendix has the actual prompts verbatim. The full session (Claude's responses, the three adversarial review passes, the image-build verification) can be linked as a gist for the unabridged version.


1. The ask, and the human decisions that drove it

  1. Origin — Simon's direction (2026-08-04). On the dependency-unblock PR (chore(deps): unblock security updates — django-saml2-auth-community + uv migration (RFC 0002) #679, which swapped to the maintained community fork as a stopgap), Simon said: "We're now using allauth for the patient login and password reset flows, so if we can use that for SAML too that would be ideal. At the time I chose the grafana saml because we weren't using allauth and it was a lighter option." → the decision to move SAML onto allauth rather than stay on the vendored fork.
  2. Build (human): "let's use superpowers and app-extension to perform all of these recommendations… implement what Simon recommended." → build the django-allauth[saml] migration.
  3. Greenfield finding (verified during the audit): SAML was checked for reachability and found never operable in any deployment — no xmlsec1 binary in the production image, auth.sso.saml2 = 0, no IdP metadata ever configured. So the migration is greenfield: there is no live behavior to preserve or stay bug-compatible with.
  4. Adversarial review (human): "evaluate PR 697… three subagents each performing their own independent review" (correctness/security, DRY/KISS, tests/docs). This surfaced three real findings, all fixed before human review — see §3.
  5. Concision (human): "the RFCs, PRs and comments are overly verbose… edit them to be more concise."

Key point for review: the scope — deleting ~250 lines of vendored ACS/user-creation code, removing the dependency-unblock resolver override, closing the xmlsec1 deployment gap — followed from Simon's one-line direction plus the greenfield finding, each recorded above, not from the tool freelancing.

2. What was built, and why

before (#679) after (#697)
Package django-saml2-auth-community (+ pysaml2, pyopenssl) django-allauth[saml] (+ python3-saml, xmlsec)
ACS endpoint vendored acs() at /sso/acs/ allauth's at /allauth/saml/<org>/acs/
IdP config auth.sso.idp_metadata_url JheSetting + trigger hook SocialApp row (provider saml) in Django admin
User provisioning vendored get_or_create_user() JheSocialAccountAdapter (~30 lines)
Login button auth.sso.saml2 JheSetting flag and exactly one saml SocialApp (else the button hides instead of 500ing the login page)
Domain restriction auth.sso.valid_domains seeded but never read enforced only when SAML would create a brand-new account — existing accounts (matched by email or previously linked) never pass through it; revoke = deactivate the user
Resolver override pyopenssl>=26.2 required deleted

Provisioning: every IdP-asserted user is created as a practitioner (JheUser.save() auto-creates the profile and applies auth.default_orgs); identifier = the SAML uid (a mapped attribute, NameID as fallback — a stable mapping is required, see the runbook); email_is_verified is set. No asserted email → signup fails closed ("Sign Up Closed"), never the type-any-email form.

Existing accounts were the subtle part: email-authentication logs an existing password-account practitioner straight in via SAML and links the SocialAccount — without it, allauth's enumeration-safe default dead-ends every existing same-email user on a fake "verification e-mail sent" page. Enabled per-IdP via "email_authentication": true in the SocialApp JSON (the global stays at allauth's default False). Two adapter guards ride this path (core/adapters.py): existing patient accounts are rejected (SAML is a practitioner entrance), and a verified allauth EmailAddress row is backfilled at link time — without it, allauth's anti-pre-registration guard would wipe the practitioner's password, since JHE's own signup never creates those rows.

Alternatives rejected: staying on the community fork (permanent second auth stack + vendored code + the override forever; Simon's direction was explicit); dropping SAML (a product decision nobody made — allauth keeps it at near-zero cost); IdP config in JheSettings via a custom get_app (more glue for a setting never once populated; SocialApp admin is the native pattern).

3. Accepted risks, and the adversarial-review hardening

The three independent review passes surfaced three real findings, all fixed before human review:

  • Existing-practitioner dead-end → the email-authentication + EmailAddress backfill above; the backfill also neutralizes allauth's password-wipe on matched accounts.
  • Login-page 500 on a flag/SocialApp mismatch → the button now hides instead of 500ing the login page.
  • Domain-gate scope: valid_domains applies only at first-time signup, not per-login (after the first rollout, most logins skip it) → documented; revoke = deactivate the user.

Accepted risks carried into the PR:

  • SOCIALACCOUNT_LOGIN_ON_GET = True is a login-CSRF surface, but the harmful variant (victim logged into an attacker's account) is blocked regardless: SP-initiated responses must carry an InResponseTo matching state in the victim's own session, and unsolicited IdP-initiated assertions are rejected by default (reject_idp_initiated_sso). The residual is a drive-by self-login, accepted for a practitioner portal in exchange for one-click SSO.
  • Email-authentication makes the per-IdP "email_authentication"/"verified_email" declarations security-relevant — set only for IdPs that verify mailbox ownership.
  • SP-side signing flags default to off in allauth — mitigated by the runbook's required advanced block.
  • python3-saml/xmlsec is a heavier binary dependency than pysaml2 — but it's the maintained, allauth-blessed path, and it makes request signing actually work.

4. The xmlsec correction (why a manifest beats reverse-engineering)

An earlier working assumption — "the missing-xmlsec1 deployment gap carries over to python3-saml" — was disproven by actually building the production image: python3-saml signs in-process through the xmlsec Python binding, whose wheels bundle libxmlsec1/libxml2, so there is no xmlsec1 binary requirement and no Dockerfile change. A speculative adapter override was likewise deleted once introspection against installed allauth 65.18 showed the hypothesized exception path didn't exist. Neither correction is visible in the final diff — exactly the kind of decision this manifest exists to preserve.

Verification: 988 backend tests (11 new, incl. an ACS integration flow), repo-pinned pre-commit, and a Docker image build + in-container import check.


Appendix: the prompts, verbatim

The human side of the build session (Claude Code, 2026-08-04), in order — not paraphrased. (Typos are the originals; Simon's quote is relayed as pasted.)

  1. Simon's direction (22:36) — relayed verbatim: "We're now using allauth for the patient login and password reset flows, so if we can use that for SAML too that would be ideal. At the time I chose the grafana saml because we weren't using allauth and it was a lighter option." → the origin of the migration.
  2. Build (22:42)"Okay let's use superpowers and app-extension to perform all of these recommendations. ensure that we update the RFC accordingly with Simon's requested changes etc."
  3. Reaffirm scope (22:52)"…I would like to implement what simon recommended."
  4. Adversarial review (23:17)"you are a senior level software engineer and I need you to evaluate PR 697 to ensure that it identifies and corrects any issues that might be raised on a PR including DRY and KISS and verbose comments. I want you to spin up three subagents each performing their own independent review." → surfaced the three §3 findings.
  5. Concision (23:32)"The RFCs as well as the PRs and comments are overly verbose and confusing and at time redundant… edit them so that they are more concise and easier to read."
  6. Merge-order questions (23:40, 23:43)"can you explain why 679 is needed given that we now have 697?" / "I am wondering if I can't merge 679 to take care of the high vulnerabilities?" → the chore(deps): unblock security updates — django-saml2-auth-community + uv migration (RFC 0002) #679-first, feat(auth): migrate SAML SSO to django-allauth[saml]; retire the resolver override (stacked on #679) #697-follows sequencing.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions