feat(auth): assign a default role to authenticated users by dcmcand · Pull Request #72 · nebari-dev/nebari-frames

dcmcand · 2026-08-20T21:48:50Z

Closes #22.

What this does

Signing in was not enough to get access: a user needed a membership, created by an admin invite or by seed.adminEmail. A valid SSO user who was never invited hit the "No organization access" screen even for reads.

FRAMES_DEFAULT_ROLE (chart: auth.defaultRole) now grants such a caller a baseline role in seed.orgSlug. Empty means deny, so a deployment that wants explicit membership is unchanged. An invalid role, or a role with no seed.orgSlug, is a startup error rather than a silent denial - and the chart rejects the same mistake at helm install time rather than crash-looping the pod.

Decisions worth reviewing

The baseline membership is persisted on first request, rather than being request-scoped. That is what makes these users show up in the members list and promotable from the admin UI (verified: AdminMembersPage already falls back to userSub when email is empty, so no frontend change was needed). The cost is that a baseline row is indistinguishable from an admin-authored one, which is the root of several papercuts now tracked in Track membership provenance so default-provisioned rows are distinguishable #66.
Existing memberships and pending invites both outrank the baseline, so an admin's explicit choice is never silently downgraded.
Enabling this moves the access boundary onto the identity provider, and turns member removal into a demotion rather than a revocation. Both are documented next to the value, in chart/README.md, and repeated as a startup warning.

Also in here

server.New now takes the FrameService instead of a store.Repository. It previously built its own service while main.go built a second one for the MCP endpoint - two instances that could be configured differently and disagree about who may do what. There is now exactly one in non-test code.

Review history

Two rounds of independent review before this PR. Five defects were found and fixed; the three that matter for reviewers:

Fail-open privilege escalation. Invite emails are typed by hand while the claim comes from the IdP. A case mismatch used to mean a clean denial; with a baseline role it silently granted that role instead - which can outrank the invited one. Reproduced: invited as viewer, claim Boss@X.io → publisher. The invite lookup is now case-insensitive and activation keys off the stored address.
A regression that hit the feature-disabled path. The fix for (1) made ActivatePendingMembership's UPDATE case-insensitive and unscoped, so two case-variant invites both matched, both were given the same user_sub, and the unique index rolled the statement back - locking the user out with a 500 and leaking raw SQL, with FRAMES_DEFAULT_ROLE unset. Now scoped to a single row by rowid.
Zero-admins trap. Once any user held a baseline row, both admin bootstrap paths in seed.Run no-opped, so enabling the default role before configuring an admin could leave an org with no admin and no in-product recovery. seed.Run now promotes the configured admin when CountAdmins is zero, while leaving a deliberate demotion alone.

Also fixed: a subjectless token used to write a row that impersonates a pending invite, and promoteIfNoAdmins could crash-loop the pod when seed.orgSlug changed while the database persisted.

Testing

go test ./... -race green, golangci-lint run ./... clean, helm lint clean, and every chart combination rendered (no default role; with and without seed.orgSlug; NebariApp auth; dev mode).

Two tests deliberately run against real SQLite rather than the in-memory fake, because the fake does not enforce the partial unique indexes that make the interesting cases fail - the deleted-and-recreated-account collision, and single-row invite activation. Both were mutation-checked: removing the handling they cover makes them fail.

Hardening folded in after review

With no users to protect, two follow-ups were fixed here rather than deferred:

Membership emails are not canonicalized, so case-variant invites can coexist and precedence is arbitrary #65 - emails are now canonical at rest. Addresses are folded and trimmed at the store boundary and the unique index is case-insensitive, so two invites for one person can no longer coexist and which one a login activates is never arbitrary. The migration drops the old index, collapses duplicates on the folded key, and only then folds the survivors, preferring an activated membership over an unclaimed invite. Activation stays scoped to a single row, because uniqueness is per (org, email) and the same person can hold invites in two orgs.
No request body size limit: an authenticated viewer can OOM the pod #70 (Connect half) - request bodies are capped at 8 MiB. A body was previously read in full before RBAC or the content limit was consulted, so an authenticated caller with no write permission could make a single-replica deployment buffer an arbitrary amount. The test asserts resource_exhausted specifically: an earlier version asserted only "not 200" and passed with the cap removed, because an oversized body fails JSON parsing anyway.

Review round two

Two blockers from @jbouder, both in paths the suite structurally could not reach - migrations only ever ran against a fresh database, and nothing exercised a membership read racing an activation. Each was reproduced before being changed, and each fix is mutation-verified.

The migration aborted on exactly the data it exists to repair. Folding ran while the old case-sensitive index was still in place, so folding Boss@X.io where boss@x.io already existed in that org failed the constraint, goose rolled back, and main exits - a crash-looping pod. Reproduced against a hand-built v4 schema. Statement order is now drop → dedupe on the folded key → fold → create the NOCASE index, and TestRunOnCaseVariantLegacyData builds that legacy schema and migrates forward so the path is covered rather than assumed.
Baseline provisioning could lower an established role. It wrote with UpsertMembership, which is UPDATE-first, and the "no membership" read that leads there is not atomic with the write. Two requests on an invited user's first page load: the first activates the invite as publisher, the second holds the stale read, falls through to provisioning, and its UPDATE rewrites the row to the baseline. Verified publisher becomes viewer, persistently; where the invite was the seed.adminEmail bootstrap, the org loses its only admin. Provisioning now uses an insert-only CreateMembership and re-reads on conflict.

That second fix widens the Repository interface with an insert-only write, which is a design change arriving via review rather than by intent - worth a look on its own terms.

Follow-ups filed

Membership emails are not canonicalized, so case-variant invites can coexist and precedence is arbitrary #65 - membership emails are not canonicalized at rest, so case-variant invites can still coexist
Track membership provenance so default-provisioned rows are distinguishable #66 - track membership provenance so default-provisioned rows are distinguishable, which would supersede the CountAdmins heuristic here

Test evidence

Beyond go test ./... -race, golangci-lint, and helm lint/helm template, this was exercised end to end against a real Keycloak, with no runtime patching - everything the loop needs is in the committed realm file.

Torn down first (make dev-clean, containers removed, ports confirmed clear, dev DB deleted), then:

FRAMES_DEFAULT_ROLE=viewer make dev-auth

All three users authenticated through the genuine authorization-code + PKCE flow against the Keycloak login form - the same flow the SPA uses, not a password grant.

Roles resolved through the product's own paths:

user	how they got there	resulting role
`dev`	seeded admin invite (`SEED_ADMIN_EMAIL`)	`admin`
`alice`	invited as publisher by that admin over the API	`publisher` - the invite outranks the default
`bob`	never invited, simply logged in	`viewer` - via `FRAMES_DEFAULT_ROLE`

bob's auto-provisioned membership is persisted and appears in the admin members list against his real subject, which is the property that motivated persisting it:

dev@localhost      admin      sub=094f66b0
alice@localhost    publisher  sub=97d4723d
bob@localhost      viewer     sub=db932704

The sub regression this branch surfaced. Keycloak 25 moved the sub claim into the basic client scope, which the dev realm's client never requested - so its access tokens identify nobody. That was already true before this branch; it was just silent, because the backend fell through to matching by email and the invite it matched was never actually activated. The empty-subject guard added here makes it loud. Reproduced by detaching the scope from a running realm:

token has sub: None
GetMe -> {"code":"unauthenticated","message":"authentication required"}

Fixed in the realm rather than by weakening the guard: a token that identifies nobody cannot be authorized, and the store treats an empty user_sub as a pending invite, so provisioning one writes a row that impersonates an invite. Verified by importing the fixed file into a throwaway Keycloak and confirming the client comes up with the scope and issues a token carrying sub.

The migration, exercised for real

The unit test builds a synthetic v4 schema, so the migration was also run against a database created by main itself. main was checked out, make dev-auth started, and the data was made through the app: invites, a login that activates one, and two published frames including an inheriting child. Because main's index is case-sensitive, it accepts the collisions that the migration has to repair.

Nine membership rows before, on schema 4:

sub=58520b51  dev@localhost        admin       <- activated
''            viewer@dev           viewer
''            publisher@dev        publisher
''            admin@dev            admin
sub=de42d1aa  alice@localhost      publisher   <- activated
''            Alice@Localhost      admin       <- case variant of the row above
''            carol@localhost      viewer
''            CAROL@LOCALHOST      publisher   <- case variant, both pending
''            '  dave@localhost  ' viewer      <- stray whitespace

The server was stopped, the branch checked out, and the same database reused. Migration to schema 5 succeeded and the app came up in about two seconds. Seven rows after, all folded:

PASS  row count went 9 -> 7 (two collisions collapsed)
PASS  all addresses are folded and trimmed
PASS  alice's ACTIVATED membership survived, not the pending case-variant
PASS  carol collapsed to the newest pending invite
PASS  dave's whitespace was trimmed
PASS  frames and versions untouched
PASS  the new index rejects a case variant

Then, against that migrated data: alice keeps the publisher role she had before, dev is still admin, bob (never invited) is admitted by the new default role, frames published by the old binary are still readable with their content intact, and MCP can read and update one of them with its inheritance preserved. A case-variant invite is now refused rather than silently creating a second row.

Collision precedence. An activated membership always outranks a pending invite, so a real member is never replaced by an unclaimed invitation. Among activated rows the oldest wins, as the longer-standing membership. Among pending invites the newest wins: a second invite is the admin saying what they want now, and keeping the earlier one would silently reinstate a role they had already replaced.

Verified against main-created data with both collision shapes present - carol invited viewer then re-invited publisher under a different casing, and alice activated as publisher with a later pending admin invite:

pre-migration                          post-migration
  pending    carol@localhost  viewer     pending    carol@localhost  publisher
  pending    CAROL@LOCALHOST  publisher
  activated  alice@localhost  publisher  activated  alice@localhost  publisher
  pending    Alice@Localhost  admin

Not covered: no cluster deployment. The chart changes were verified with helm lint and by rendering every combination (no default role; with and without seed.orgSlug; NebariApp auth mode; dev mode), but never actually installed. The e2e-sandbox CI job cannot cover it either - it is red on main for unrelated reasons, failing inside the sandbox action's own platform setup (no deployment/daemonset/statefulset found in keycloak) before nebari-frames is deployed at all.

Closes #22. Signing in was not enough to get access: a user needed a membership, created by an admin invite or by seed.adminEmail. A valid SSO user who was never invited was locked out entirely, even for reads. FRAMES_DEFAULT_ROLE (chart: auth.defaultRole) now grants such a caller a baseline role in seed.orgSlug. Empty means deny, so the fail-closed behavior of a deployment that wants explicit membership is unchanged. An invalid role, or a role with no seed.orgSlug, is a startup error rather than a silent denial; the chart rejects the same mistake at install time. The baseline membership is persisted on first request so these users appear in the members list and can be promoted there. Existing memberships and pending invites both outrank it. Enabling this moves the access boundary onto the identity provider, and makes member removal a demotion rather than a revocation. Both are documented alongside the value. Three problems this created, and how they are handled: - Invite emails are typed by hand while the claim comes from the IdP, so a case difference used to mean a clean denial. With a baseline role it would instead grant that role, which can outrank the invited one. The invite lookup is now case-insensitive and activation keys off the stored address, so the invite still wins. - A token with an empty sub would provision a row that impersonates a pending invite, because the store overloads an empty user_sub. Such a token is now rejected before any store access. - Once any user held a baseline row, both admin bootstrap paths in seed.Run no-opped, so enabling the default role before configuring an admin could leave an org with no admin and no way to make one. seed.Run now promotes the configured admin when the org has none, while leaving a deliberate demotion alone. server.New takes the FrameService instead of building its own: it previously constructed one while main.go built a second for the MCP endpoint, which would have let the two paths disagree about the default role.

Follow-up to the default-role work, from a second review pass. Making the invite lookup case-insensitive introduced a worse failure than the one it fixed. ActivatePendingMembership's UPDATE was case-insensitive and unscoped, so two invites differing only in case (which the store still permits, see #65) both matched, the statement tried to give both the same user_sub, and the unique index on user_sub rolled the whole thing back. The user was then locked out with an internal error on every request, and the raw SQL escaped in the response. This happened with FRAMES_DEFAULT_ROLE unset, so it hit deployments not using the feature at all. The UPDATE is now scoped to a single row by rowid. The baseline membership stored the email claim verbatim, so a claim of " Bob@X.io " persisted with its whitespace and a later invite typed canonically did not collide with it: the admin got a success and the invite silently never applied. Addresses written by this path are now canonicalized through store.CanonicalEmail, and the fake enforces the same comparison rules as SQLite so the divergence stops hiding this class of bug. Retrofitting existing rows and making the index case-insensitive are tracked in #65 and #66; the README now describes what actually happens rather than promising an error that does not occur. promoteIfNoAdmins could exit the server. A membership is unique per subject across all orgs, so the configured admin's row may belong to a different org than the one being seeded - which is what happens when seed.orgSlug changes while the database persists. Promoting by (orgID, sub) matched nothing, and the resulting error crash-looped the pod on an opaque "not found". Mismatched orgs and a missing row are now both skipped with a warning. Also: the concurrency test's comment claimed it proved a loser re-reads the winner's row, which it could not - all racers assume the same role. The comment now states what the test actually pins and points at the test that does cover it.

…ubject Keycloak 25 moved the `sub` claim into the `basic` client scope, and the dev realm's frames-web client never requested it. Its access tokens therefore carry no subject at all, which the SPA sends to the backend. Until now that failure was quiet. The backend read an empty subject and fell through to matching the caller by email, so `make dev-auth` appeared to work for the seeded dev user - while the invite it matched was never actually activated, and every request re-ran the activation. The empty-subject guard added in this branch makes it loud instead: a token with no subject is rejected, so logging in returns {"code":"unauthenticated","message":"authentication required"} Verified by detaching the scope from a running realm and calling GetMe, then by importing this file into a throwaway Keycloak and confirming the client comes up with the scope and issues a token carrying `sub`. The guard is right - a token that identifies nobody cannot be authorized, and the store treats an empty user_sub as a pending invite - so the fix belongs in the realm, not in the check.

… one role The dev realm had a single user, and `dev@localhost` is seeded as the org admin by SEED_ADMIN_EMAIL. There was therefore no way to exercise the behaviour this branch adds: FRAMES_DEFAULT_ROLE only applies to a user the store has never seen, and an admin is not that. alice and bob are ordinary realm users with no Frames membership. The loop they enable, verified against `make dev-auth`: dev seeded admin alice invited by that admin as publisher, so the invite outranks the default role bob never invited, so FRAMES_DEFAULT_ROLE admits him as viewer and his membership is written on first request Passwords match the usernames, in keeping with dev/dev.

Closes #65. Part of #70. Emails were stored exactly as typed, and the unique index compared them case-sensitively, so two invites for the same person could coexist in one org. The earlier fix on this branch made lookups case-insensitive, which removed the privilege escalation but left the ambiguity: a lookup could match both rows and picked with LIMIT 1, so which invite a login activated was arbitrary and could be the more privileged one. Addresses are now folded and trimmed at the store boundary, and the unique index is case-insensitive, so the ambiguity cannot arise. A migration folds existing rows first and resolves any collision folding creates, preferring an activated membership over an unclaimed invite so a real member is never dropped in favour of an invitation. Activation stays scoped to a single row. Uniqueness is per (org, email) rather than global, so the same person can hold pending invites in two orgs, and the activation UPDATE matches on email alone - it would otherwise try to give both rows the same user_sub and fail the unique index on user_sub. The test now covers that case rather than the case-variant one, which is no longer reachable. Separately, the Connect handler now caps request bodies at 8 MiB. A body was previously read in full before RBAC or the content limit was consulted, so an authenticated caller with no write permission could make a single-replica deployment buffer an arbitrary amount. The cap sits well above frames.MaxContentBytes so a publish at the content limit still fits with its framing. The MCP endpoint needs the same treatment; that half lives with the branch that owns it.

jbouder

Two blockers inline. Both sit in paths the test suite structurally can't reach (migrations only ever run against a fresh DB in tests; nothing exercises concurrent activation), so everything being green doesn't cover them. Rest of the review looked good — the fail-open escalation catch is the right one.

…t-only Both from review. Both sit in paths the suite structurally could not reach: migrations only ever ran against a fresh database, and nothing exercised a membership read racing an activation. **The migration aborted on the data it exists to repair.** The fold ran while the old case-sensitive unique index was still in place, so folding Boss@X.io to boss@x.io where boss@x.io already existed in that org failed the constraint. Goose rolled back, migrations.Run returned the error, and main exits - a crash-looping pod recoverable only by hand. Reproduced by building the v4 schema with two case-variant invites in one org. The index is now dropped first, duplicates are collapsed on the folded key, and only then are the survivors folded. TestRunOnCaseVariantLegacyData builds that legacy schema and migrates forward, so this path is covered rather than assumed. **Baseline provisioning could lower an established role.** It wrote with UpsertMembership, which is UPDATE-first in the SQLite implementation, and the "no membership" read that leads here is not atomic with the write. Two requests on an invited user's first page load: the first activates the invite as publisher; the second, holding the stale read, falls through to provisioning and its UPDATE rewrites that row to the baseline. Verified publisher becomes viewer, persistently. Where the invite was the seed.adminEmail bootstrap, the org loses its only admin. Provisioning now inserts and never updates, via a new insert-only CreateMembership. On conflict it re-reads: a row for this subject is authoritative whatever its role, and no row means the address belongs to another subject, which is a denial. That also makes the concurrent-insert case in the comment reachable, which it was not before.

Where two pending invites for the same person differ only in case, the migration kept the oldest. A second invite is the admin saying what they want now, so keeping the earlier one silently reinstates a role they had already replaced - someone re-invited as publisher would come back as a viewer. An activated membership still outranks any pending invite, and among activated rows the oldest still wins as the longer-standing membership. Only the pending-versus-pending tiebreak changes. The legacy-data test now covers all three shapes: activated beating pending, newest pending beating oldest, and a trimmed address with no collision.

…-authenticated-users

Covers what this branch changes about the shape of the system, so the next agent does not have to reconstruct it from the diff: caller resolution now has a fixed precedence ending in the baseline role, the baseline write is insert-only and why, emails are canonical at rest, the FrameService is injected so both endpoints share one, and request bodies are capped. Also notes the testing gap this branch was caught by: the normal suite only ever runs migrations against a fresh database, so a migration that touches existing rows needs the legacy-data test.

…truncation Rebased onto main, which brought #72, #73, #76, and #77. The rebase produced no git conflicts but two packages that did not compile, plus a migration collision that stopped the server booting. Fixes those, the codec data-loss bug, and the smaller items from the same review. Blockers: - Port the MCP write surface to the free-form body. `write.go` declared ten slot-named input fields against a `frames.Doc` that no longer has them. It now carries `body` and `template`: `body` because that is what a Frame's content is, and `template` because the reflective guard in `resources_test.go` asked for a decision and the answer is yes — without it `create_frame` cannot make a template at all, and an omitted-means-keep pointer stops `update_frame` de-listing one by accident. The guard now walks `frames.Doc` alone, since `SlotTable` is gone. - Renumber `005_frame_is_template.sql` to 006. main's `005_canonical_membership_ email.sql` claimed the same version, and goose rejects duplicates at provider construction, so no migration ran at all. Adds the missing `-- +goose Down`. `migrate_legacy_test.go` builds a schema as of 004 and did not carry the frames table, so 006's ALTER failed there as "no such table"; the fixture now carries every table a later migration touches, and asserts a pre-006 frame comes forward as not-a-template. - Match the frontmatter delimiter only at column 0. `TrimSpace(line) == "---"` let an indented `---` inside a YAML block scalar close the frontmatter, which truncated the document and dropped every field after it — including documents the exporter itself produced, where the error named a field the author never touched. Table-driven cases cover the block scalar in both positions, a `---` in the body, and a full round trip through the codec's own output. The unqualified "lossless" claims are replaced with the two normalizations that actually happen. Should-fix: - `frame-yaml.ts` claimed to mirror `legacy.go` and did not: no two-space continuation indent, and `.trim()` where Go trims newlines only. That reaches storage, since restoring a legacy version re-serializes the TypeScript render as canonical content — one rule with a nested list became three flat rules, permanently. The port is now faithful, and both sides are pinned to one shared fixture, `testdata/legacy-slots/`, compared whole rather than by substring. Substring assertions over single-line values are what let the drift through. Also: - Pin the legacy-vs-`body:` precedence, which was silent and untested, and stop `Parse` naming the unexported `frames.docYAML` in unknown-key errors that reach API clients unwrapped. The error now lists the recognized keys. - Cover `is_template` at the sqlite level: four SELECT lists and four Scan calls, none of them exercised by the service tests, which use `store.NewMemory()`. Asserts both the true and false case and the columns either side, so a scan-order mismatch fails. - Guard the empty version in `compose.go`'s `Inherits from:` line, matching `MarshalMarkdown`. - Give the one page-level heading that cannot use `PageHeader` the classes `PageHeader` exists to hold steady. - Rewrite the parts of AGENTS.md and the three design docs that described the deleted slot design as current. The MCP doc's §3.4 gets the struck-through supersede treatment #73 set the precedent for on that same file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

dcmcand added 4 commits August 20, 2026 14:14

dcmcand requested a review from jbouder August 20, 2026 23:12

jbouder requested changes Aug 21, 2026

View reviewed changes

Comment thread backend/internal/store/sqlite/migrations/005_canonical_membership_email.sql Outdated

Comment thread backend/internal/orgs/orgs.go Outdated

dcmcand added 4 commits August 21, 2026 09:23

Merge remote-tracking branch 'origin/main' into feat/default-role-for…

412041d

…-authenticated-users

jbouder approved these changes Aug 21, 2026

View reviewed changes

dcmcand merged commit 365627e into main Aug 21, 2026
4 of 5 checks passed

dcmcand deleted the feat/default-role-for-authenticated-users branch August 21, 2026 18:08

dcmcand mentioned this pull request Aug 21, 2026

No request body size limit: an authenticated viewer can OOM the pod #70

Closed

jbouder mentioned this pull request Aug 24, 2026

Free-form markdown frame bodies, with a Nebari design system pass #75

Open

Navigation Menu

Uh oh!

feat(auth): assign a default role to authenticated users - #72

feat(auth): assign a default role to authenticated users#72
dcmcand merged 9 commits into
mainfrom
feat/default-role-for-authenticated-users

dcmcand commented Aug 20, 2026 •

edited

Loading

Uh oh!

jbouder left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

dcmcand commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this does

Decisions worth reviewing

Also in here

Review history

Testing

Hardening folded in after review

Review round two

Follow-ups filed

Test evidence

The migration, exercised for real

Uh oh!

jbouder left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dcmcand commented Aug 20, 2026 •

edited

Loading