docs(sso): RFC proposal for org SSO (postguard#143)#12
docs(sso): RFC proposal for org SSO (postguard#143)#12dobby-coder[bot] wants to merge 2 commits into
Conversation
Proposes data model (org_identity_providers, org_users dependency on #74), OIDC-first flow, feature flag FF_SSO, and a 5-PR rollout plan. Opens with a request-for-comments; no code changes. Implementation PRs follow once the shape is agreed. Relates to encryption4all/postguard#143 Related to encryption4all/postguard#74, encryption4all/postguard#146 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Keeping this as draft on purpose — it is an RFC, not a deliverable. Unblocks to ready-for-review once the five open questions at the bottom of the doc have answers and the overall phasing looks right to whoever owns product direction. cc @rubenhensen. |
- Strike the "prerequisite blocker" section: the users table, sessions.user_id and user_type='user' all landed in #15. Quote the as-landed schema from src/lib/server/db/schema.ts rather than the org_users shape originally proposed. - Spell out the additive migration SSO still needs (auth_source, sso_subject, last_login_at on users; partial unique index on (org_id, sso_subject)). Drop role/status — out of scope for SSO. - Update the auth-flow pseudocode to reference users (not org_users) and to note last_login_at should be bumped on callback success. - Strike chunk 2 in the rollout plan (landed as #15) and renumber chunks 3-7 accordingly. - Add postguard-business#15 to the "Related" links and flag the refresh date in the status line. No prose rewrite; no change to the auth flow, security considerations, or open questions. The landed schema matched the proposal closely enough that the plan survives intact — the drift is mostly additive.
|
Refreshed the RFC to reflect the Diff summary:
No change to the auth flow, security considerations, or the five open questions — the landed schema matched closely enough that only the scaffolding section needed a rewrite. Keeping as draft until the five open questions at the bottom get answers. |
|
/dobby but this on the back burner. I don't want to think about implementing this right now. |
|
Dobby hears you, master rubenhensen! Dobby will put this SSO RFC on the back burner. The pull request stays as a draft, simmering quietly, ready for when master wants to think about it again. Dobby will not bother master about it! |
|
Rule-compliance sweep — fixed two issues in the PR body:
Doc itself ( |
There was a problem hiding this comment.
Reviewed the proposal end-to-end against the on-main schema and the existing auth/session code. The technical foundations are sound and the doc is accurate enough to merge as an RFC. Cannot submit a formal approval (this PR was opened by the same bot account doing the review), so leaving this as a comment-review with a clear LGTM. Two small accuracy nits worth fixing before this lands (or as part of the follow-up migration PR):
1. user_type values — 'user' is proposed, not landed.
The doc says (around line 60): "Sessions already carry user_type ('org' | 'admin' | 'user') and user_id referencing users.id. The SSO callback creates a session row with user_type='user' the same way Yivi login does."
On main today, every caller types userType as 'org' | 'admin' only:
src/lib/server/auth/session.ts— both theSessiontype and thecreateSessionparameter are'org' | 'admin'src/app.d.ts—userType: 'org' | 'admin'
#15 landed the users table and sessions.user_id, but the 'user' discriminant is not in the code yet. Suggest rewording to "we will add 'user' as a third value", since otherwise reviewers comparing the doc to the schema get confused.
2. org_users vs users naming drift.
The doc switched to building on the landed users table, but three places still say org_users:
- line 43: "rather than introducing a second
org_userstable" (intentional — fine to keep) - line 115 (in the
org_identity_providersblock): "auto-create org_users on first login if email domain matches org.domain" - line 149 (in the flow): JIT step implicitly creates an
org_usersrow — surrounding prose
Worth a global s/org_users/users/ in the JIT bullets so the implementation PR isn't ambiguous.
Things that look correct and are worth keeping as-is:
- OIDC-first / SAML deferred — correct call given target IdPs.
openid-client(panva) — actively maintained, conformance-certified. Good choice over hand-rolled JWT verification.- State + PKCE mandatory, JWKS signature + iss/aud/exp checks. No gaps.
- Envelope encryption for
client_secret_encryptedwith key id, key sourced from env. Sensible. require_yivi_for_signingdefaulting to true is the right conservative default — keeps the Yivi-as-crypto-primitive invariant intact, and the framing in the Signing and decryption section matches how the encryption layer actually works in this repo.unique(org_id)onorg_identity_providersfor phase 1 (one IdP per org) is fine; data model leaves room to drop that later.- Partial unique index
(org_id, sso_subject) where sso_subject is not nullis the correct shape — avoids collisions while keeping Yivi rows unconstrained. - Feature flag matches the existing
FF_*pattern; off-by-default + 404 on disabled routes is what other flagged features in this repo do. - Rollout chunked into independently reviewable PRs, with #15 already excluded as landed.
LGTM as an RFC. The five open questions (slug vs domain, JIT default, SAML phasing, signing semantics, extension scope) are scope decisions for the maintainer thread, not blockers on the doc itself. Apply the two nits above either inline here or as the first commit of the migration PR (chunk 3).
|
/dobby we will keep the yivi login for now. Move this to the lowest priority on the project board |
|
This agent is on it, @rubenhensen! Will move issue #12 to the lowest priority on the project board right away. |
|
Done — set Priority to P2 and moved this PR to the bottom of the org project board. Yivi login stays as-is; this RFC remains open as the lowest-priority item for now. |
Summary
org_identity_providers), auth flow, feature flagFF_SSO, and a 5-PR rollout plan.userstable that already landed in encryption4all/postguard-business#15; notes the overlap with AD sync (encryption4all/postguard#146).Keeping this as a draft on purpose: I'd like the plan reviewed before any schema migration or route lands.
What's in the doc
docs/proposals/sso.mdcovers:userstable — landed in encryption4all/postguard-business#15. This proposal adds three additive columns (auth_source,sso_subject,last_login_at) rather than introducing a second table.org_identity_providerswith encrypted client secret, claim mapping, JIT flag, and arequire_yivi_for_signingpolicy flagopenid-client(no hand-rolled JWT verification)Open questions (please answer in-thread)
The doc ends with five explicit questions for @rubenhensen — all material scope decisions I couldn't pin down from the issue body alone:
/auth/sso/[slug]vs/auth/sso/[domain]for the entry URL?Links
Reviewer quickstart
No code, pure markdown — just read the rendered proposal:
https://github.com/encryption4all/postguard-business/blob/feat/sso-proposal/docs/proposals/sso.md
Test plan
🤖 Generated with Claude Code