From 6aed57e2175c8587fe62c37db394e650307b0321 Mon Sep 17 00:00:00 2001 From: Chuck McAndrew <6248903+dcmcand@users.noreply.github.com> Date: Thu, 20 Aug 2026 14:14:28 -0700 Subject: [PATCH 1/8] feat(auth): assign a default role to authenticated users 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. --- README.md | 38 ++- backend/cmd/server/main.go | 44 +++- backend/cmd/server/main_test.go | 80 ++++++ backend/internal/frames/service.go | 25 +- backend/internal/orgs/orgs.go | 130 ++++++++-- backend/internal/orgs/orgs_test.go | 325 +++++++++++++++++++++++- backend/internal/rbac/rbac.go | 11 + backend/internal/rbac/rbac_test.go | 28 ++ backend/internal/seed/seed.go | 59 ++++- backend/internal/seed/seed_test.go | 82 ++++++ backend/internal/server/server.go | 10 +- backend/internal/server/server_test.go | 13 +- backend/internal/store/memory.go | 5 +- backend/internal/store/sqlite/sqlite.go | 7 +- chart/README.md | 1 + chart/templates/deployment.yaml | 8 + chart/values.schema.json | 1 + chart/values.yaml | 8 + 18 files changed, 831 insertions(+), 44 deletions(-) diff --git a/README.md b/README.md index ff79f0c..9e9a1d5 100644 --- a/README.md +++ b/README.md @@ -50,7 +50,7 @@ Starts Keycloak in Docker (`:8081`) with an auto-imported realm and runs the bac ### Troubleshooting -**"No organization access" after login.** This is intentional fail-closed behavior: a signed-in user who is not a member of any org is denied. Locally, `make dev` seeds you (`dev-user`) as an admin, and `make dev-auth` seeds `dev@localhost` as a pending admin that activates on first login - so neither should show this page. If you see it against a real deployment, ask an org admin to add your email. +**"No organization access" after login.** This is intentional fail-closed behavior: a signed-in user who is not a member of any org is denied. Locally, `make dev` seeds you (`dev-user`) as an admin, and `make dev-auth` seeds `dev@localhost` as a pending admin that activates on first login - so neither should show this page. If you see it against a real deployment, ask an org admin to add your email, or set `auth.defaultRole` to admit every authenticated user at a baseline role (see [Default role for authenticated users](#default-role-for-authenticated-users)). **`disk I/O error` / `database is locked` on startup.** A previous dev backend was left running (e.g. `make dev` was suspended with Ctrl-Z or killed with `kill -9` instead of stopped with a single Ctrl-C) and still holds the SQLite lock. Run `make dev-clean` to stop the orphan (it frees ports `:5173`/`:8080`), clear the dev DB and its `-wal`/`-shm` files, and reset Keycloak, then start again. Always stop a dev loop with a single **Ctrl-C** so both processes shut down cleanly. @@ -81,6 +81,7 @@ For a standalone binary or a non-chart deployment, the equivalent environment va | `OIDC_DEVICE_CLIENT_ID` | OIDC client id for the device-flow login used by `frames auth login`. | | `OIDC_GROUPS_CLAIM` | Claim to read group membership from. Defaults to `groups`. | | `FRAMES_DEV_MODE` | Set to exactly `true` to disable auth (dev only). | +| `FRAMES_DEFAULT_ROLE` | Baseline role for an authenticated user with no membership: unset (deny, the default), `viewer`, `publisher`, or `admin`. Requires `SEED_ORG_SLUG`. | `FRAMES_DEV_MODE=true` short-circuits everything else. Otherwise both `OIDC_ISSUER_URL` and `OIDC_CLIENT_ID` are required, or the server fails fast on startup with a message naming the missing variable. @@ -90,6 +91,7 @@ Beyond the auth block above, the values most people end up touching are: - `nebariapp.hostname` - the hostname the operator routes to the app. - `seed.orgSlug`, `seed.orgDisplayName`, `seed.adminEmail` - the organization created on first boot, and the email that is reconciled to the first real admin on their first login. +- `auth.defaultRole` - baseline role for any authenticated user who has no membership yet. Empty (the default) keeps today's behavior: a signed-in user with no invite is denied. See [Default role for authenticated users](#default-role-for-authenticated-users). - `persistence.size`, `persistence.storageClass` - PVC size and storage class for the SQLite database. - `mcp.enabled`, `mcp.publicUrl` - whether the `/mcp` endpoint is mounted, and an override for its public URL when it can't be derived from `nebariapp.hostname`. - `branding.*` - white-label the app: title, logo (light and dark), favicon, and theme colors. Delivered at runtime, so no image rebuild is needed; leaving the block empty keeps the built-in Nebari branding. See [Branding](chart/README.md#branding). @@ -111,12 +113,44 @@ helm install nebari-frames oci://quay.io/nebari/charts/nebari-frames --version 0 Installing from a git checkout also works, as shown in [`chart/README.md`](chart/README.md#install-on-nebari). +### Default role for authenticated users + +By default, signing in is not enough: a user needs a membership, created by an admin invite or by +`seed.adminEmail`. A valid SSO user with no invite gets the "No organization access" screen. + +Setting `auth.defaultRole` (env `FRAMES_DEFAULT_ROLE`) changes that. Any authenticated user with no +membership is admitted to `seed.orgSlug` at that role, and the membership is written on their first +request, so they appear in the admin members list and can be promoted from there. + +```bash +helm upgrade --install nebari-frames ... \ + --set auth.defaultRole=viewer \ + --set seed.orgSlug=my-org +``` + +Consequences to weigh before enabling it: + +- **The identity provider becomes the access boundary.** Anyone the realm admits can read every + Frame shared with the org. This is only safe if realm registration is closed or SSO-gated. +- **Removing a member stops being a revocation.** The removed user is re-provisioned at the default + role on their next request. To actually revoke access, disable the user in Keycloak, or unset + `auth.defaultRole` and manage membership explicitly. +- **"Add member" stops working for anyone who has already signed in.** They already hold a + membership, so adding them by email fails with "a member with that email already exists". Change + their role from the members list instead. +- **Set `seed.adminEmail` (or `seed.adminSub`) as well.** A user who has signed in at the baseline + role already has a membership, which is why the server promotes the configured admin whenever the + organization has none. That recovery only works if an admin is configured, so configure one. + +An invalid role, or a role set without `seed.orgSlug`, fails at startup with a message naming the +variable rather than silently denying every request. + ## Known Limitations - **SQLite is single-writer.** `replicaCount` must stay `1`; the chart defaults to it and documents why in [`chart/README.md`](chart/README.md#values-reference). There is no highly-available mode yet. - **OIDC discovery happens from inside the pod.** The backend resolves and validates the issuer URL itself at startup, so the pod must be able to resolve the issuer's hostname and trust its TLS certificate. This fails on clusters where the external Keycloak hostname isn't resolvable in-cluster, or where Keycloak serves a certificate the pod doesn't already trust. - **One organization in the MVP.** `seed.orgSlug` seeds a single organization; there's no cross-org sharing or multi-org UI yet. -- **Role assignment is per-membership.** Each org membership carries its own role today. Keycloak group-to-role mapping is tracked in [#21](https://github.com/nebari-dev/nebari-frames/issues/21) and a default role for new members in [#22](https://github.com/nebari-dev/nebari-frames/issues/22). +- **Role assignment is per-membership.** Each org membership carries its own role today. `auth.defaultRole` sets the floor for users with no membership; Keycloak group-to-role mapping, which would raise it per group, is tracked in [#21](https://github.com/nebari-dev/nebari-frames/issues/21). ## Troubleshooting a Nebari Deployment diff --git a/backend/cmd/server/main.go b/backend/cmd/server/main.go index fa7fdce..aeb8eaf 100644 --- a/backend/cmd/server/main.go +++ b/backend/cmd/server/main.go @@ -16,6 +16,8 @@ import ( "github.com/nebari-dev/nebari-frames/backend/internal/devfixture" "github.com/nebari-dev/nebari-frames/backend/internal/frames" mcppkg "github.com/nebari-dev/nebari-frames/backend/internal/mcp" + "github.com/nebari-dev/nebari-frames/backend/internal/orgs" + "github.com/nebari-dev/nebari-frames/backend/internal/rbac" "github.com/nebari-dev/nebari-frames/backend/internal/seed" "github.com/nebari-dev/nebari-frames/backend/internal/server" sqlitestore "github.com/nebari-dev/nebari-frames/backend/internal/store/sqlite" @@ -71,6 +73,17 @@ func main() { slog.Error("invalid auth configuration", "error", err) os.Exit(1) } + defaultMembership, err := selectDefaultMembership(os.Getenv("FRAMES_DEFAULT_ROLE"), os.Getenv("SEED_ORG_SLUG")) + if err != nil { + slog.Error("invalid default role configuration", "error", err) + os.Exit(1) + } + if defaultMembership.Role != "" { + slog.Warn("FRAMES_DEFAULT_ROLE is set - every authenticated user gets this role in the org, "+ + "and removing a member only demotes them to it; revoke access in the identity provider instead", + "role", string(defaultMembership.Role), "org", defaultMembership.OrgSlug) + } + var validator auth.TokenValidator if devMode { slog.Warn("FRAMES_DEV_MODE=true - authentication DISABLED; injecting fixed dev-user identity") @@ -94,9 +107,9 @@ func main() { } // Kept as a server.Mounter (interface) so a disabled endpoint is a nil // interface, not a typed-nil *Component that would satisfy a != nil check. + framesService := frames.NewService(repo, frames.WithDefaultMembership(defaultMembership)) var mcpMounter server.Mounter if mcpCfg.PublicURL != "" || devMode { - framesService := frames.NewService(repo) mcpMounter = mcppkg.NewComponent(mcpCfg, framesService, mcpValidator) } @@ -109,7 +122,7 @@ func main() { srv := &http.Server{ Addr: ":" + port, - Handler: server.New(repo, validator, authCfg, brandingCfg, devMode, mcpMounter).Handler(), + Handler: server.New(framesService, validator, authCfg, brandingCfg, devMode, mcpMounter).Handler(), ReadHeaderTimeout: 10 * time.Second, ReadTimeout: 30 * time.Second, WriteTimeout: 60 * time.Second, @@ -129,6 +142,33 @@ func envOr(key, fallback string) string { return fallback } +// selectDefaultMembership resolves FRAMES_DEFAULT_ROLE into the baseline +// membership granted to authenticated callers with no stored membership. An +// empty role means deny, preserving the fail-closed default. Anything set but +// unusable is a startup error rather than a silent denial, because an operator +// who configured a default role and got none would have no way to tell. +func selectDefaultMembership(roleEnv, orgSlug string) (orgs.DefaultMembership, error) { + roleEnv = strings.TrimSpace(roleEnv) + orgSlug = strings.TrimSpace(orgSlug) + if roleEnv == "" { + return orgs.DefaultMembership{}, nil + } + role, ok := rbac.ParseRole(roleEnv) + if !ok { + return orgs.DefaultMembership{}, fmt.Errorf( + "FRAMES_DEFAULT_ROLE=%q is not a valid role; use one of viewer, publisher, admin, or leave it unset to require explicit membership", + roleEnv, + ) + } + if orgSlug == "" { + return orgs.DefaultMembership{}, fmt.Errorf( + "FRAMES_DEFAULT_ROLE=%q needs an organization to grant access to, but SEED_ORG_SLUG is not set", + roleEnv, + ) + } + return orgs.DefaultMembership{Role: role, OrgSlug: orgSlug}, nil +} + // selectAuthMode resolves the auth bootstrap decision from environment values. // FRAMES_DEV_MODE=true (exactly) disables auth. Otherwise OIDC_ISSUER_URL and // OIDC_CLIENT_ID are both required; a missing one is a fatal misconfiguration. diff --git a/backend/cmd/server/main_test.go b/backend/cmd/server/main_test.go index fdbf341..564888b 100644 --- a/backend/cmd/server/main_test.go +++ b/backend/cmd/server/main_test.go @@ -3,6 +3,9 @@ package main import ( "strings" "testing" + + "github.com/nebari-dev/nebari-frames/backend/internal/orgs" + "github.com/nebari-dev/nebari-frames/backend/internal/rbac" ) func TestSelectAuthMode(t *testing.T) { @@ -46,3 +49,80 @@ func TestSelectAuthMode(t *testing.T) { }) } } + +func TestSelectDefaultMembership(t *testing.T) { + tests := []struct { + name string + roleEnv string + orgSlug string + want orgs.DefaultMembership + wantErr bool + errNames string // substring the error must mention + }{ + { + name: "unset denies, which is the fail-closed default", + roleEnv: "", + orgSlug: "acme", + want: orgs.DefaultMembership{}, + }, + { + name: "unset with no org is also fine", + roleEnv: "", + orgSlug: "", + want: orgs.DefaultMembership{}, + }, + { + name: "viewer resolves against the seeded org", + roleEnv: "viewer", + orgSlug: "acme", + want: orgs.DefaultMembership{Role: rbac.RoleViewer, OrgSlug: "acme"}, + }, + { + name: "surrounding whitespace is tolerated", + roleEnv: " viewer\n", + orgSlug: "acme", + want: orgs.DefaultMembership{Role: rbac.RoleViewer, OrgSlug: "acme"}, + }, + { + name: "admin is accepted, however unwise", + roleEnv: "admin", + orgSlug: "acme", + want: orgs.DefaultMembership{Role: rbac.RoleAdmin, OrgSlug: "acme"}, + }, + { + name: "an unknown role fails fast", + roleEnv: "superuser", + orgSlug: "acme", + wantErr: true, + errNames: "FRAMES_DEFAULT_ROLE", + }, + { + name: "a role with no seeded org fails fast rather than silently denying", + roleEnv: "viewer", + orgSlug: "", + wantErr: true, + errNames: "SEED_ORG_SLUG", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := selectDefaultMembership(tt.roleEnv, tt.orgSlug) + if tt.wantErr { + if err == nil { + t.Fatalf("want error, got config %+v", got) + } + if !strings.Contains(err.Error(), tt.errNames) { + t.Errorf("error %q should name %q", err, tt.errNames) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != tt.want { + t.Errorf("got %+v, want %+v", got, tt.want) + } + }) + } +} diff --git a/backend/internal/frames/service.go b/backend/internal/frames/service.go index dc1f242..e312c05 100644 --- a/backend/internal/frames/service.go +++ b/backend/internal/frames/service.go @@ -21,14 +21,29 @@ import ( ) type Service struct { - repo store.Repository - lookup rbac.GrantLookup + repo store.Repository + lookup rbac.GrantLookup + defaultMembership orgs.DefaultMembership +} + +// Option configures a Service. Options exist so that adding configuration does +// not churn every NewService call site; the zero configuration is fail-closed. +type Option func(*Service) + +// WithDefaultMembership grants authenticated callers with no stored membership +// a baseline role in the named org. Omit it to deny such callers (the default). +func WithDefaultMembership(def orgs.DefaultMembership) Option { + return func(s *Service) { s.defaultMembership = def } } var _ framesv1connect.FrameServiceHandler = (*Service)(nil) -func NewService(repo store.Repository) *Service { - return &Service{repo: repo, lookup: grantLookup{repo}} +func NewService(repo store.Repository, opts ...Option) *Service { + s := &Service{repo: repo, lookup: grantLookup{repo}} + for _, opt := range opts { + opt(s) + } + return s } // grantLookup adapts store grants to rbac.Grant. @@ -351,7 +366,7 @@ func (s *Service) ResolveFrame(ctx context.Context, req *connect.Request[framesv // --- helpers --- func (s *Service) resolveCaller(ctx context.Context) (rbac.Caller, error) { - caller, err := orgs.ResolveCaller(ctx, s.repo) + caller, err := orgs.ResolveCaller(ctx, s.repo, s.defaultMembership) if err != nil { switch { case errors.Is(err, orgs.ErrNoClaims): diff --git a/backend/internal/orgs/orgs.go b/backend/internal/orgs/orgs.go index 33aa2af..66bf3e5 100644 --- a/backend/internal/orgs/orgs.go +++ b/backend/internal/orgs/orgs.go @@ -4,10 +4,15 @@ package orgs import ( "context" "errors" + "log/slog" + "strings" + + "google.golang.org/protobuf/types/known/timestamppb" "github.com/nebari-dev/nebari-frames/backend/internal/auth" "github.com/nebari-dev/nebari-frames/backend/internal/rbac" "github.com/nebari-dev/nebari-frames/backend/internal/store" + framesv1 "github.com/nebari-dev/nebari-frames/gen/go/frames/v1" ) var ( @@ -15,34 +20,47 @@ var ( ErrNoMembership = errors.New("user has no org membership") ) +// DefaultMembership configures the baseline access granted to an authenticated +// caller the store has never seen. The zero value denies such callers, which +// keeps a deployment that wants membership to be explicit fail-closed. +// +// Enabling it moves the trust boundary onto the identity provider: anyone the +// realm admits gets the baseline role. It also means RemoveOrgMember stops +// being a revocation, since the removed user is re-provisioned at the baseline +// on their next request. +type DefaultMembership struct { + Role rbac.Role // baseline role; empty means deny + OrgSlug string // org the baseline membership joins; empty means deny +} + +// enabled reports whether a baseline membership should be provisioned. Both +// fields are required: a role with nowhere to put it is not a usable config. +func (d DefaultMembership) enabled() bool { return d.Role != "" && d.OrgSlug != "" } + // ResolveCaller builds an rbac.Caller from the request's auth claims and the // user's org membership. -func ResolveCaller(ctx context.Context, repo store.Repository) (rbac.Caller, error) { +func ResolveCaller(ctx context.Context, repo store.Repository, def DefaultMembership) (rbac.Caller, error) { claims, ok := auth.ClaimsFromContext(ctx) if !ok { return rbac.Caller{}, ErrNoClaims } + // A subject is what identifies the caller, and the store overloads an empty + // user_sub to mean "pending invite". Provisioning a subjectless caller would + // therefore write a row that impersonates an invite, carry the default role, + // and occupy the (org, email) slot so a real invite for that address could + // never be created. OIDC requires `sub`, so this is a malformed token. + if claims.Subject == "" { + return rbac.Caller{}, ErrNoClaims + } m, err := repo.GetMembership(ctx, claims.Subject) if err != nil { if !errors.Is(err, store.ErrNotFound) { return rbac.Caller{}, err } - // No sub-keyed membership: try matching a pending invite by email. - if claims.Email == "" { - return rbac.Caller{}, ErrNoMembership - } - pending, perr := repo.GetPendingMembershipByEmail(ctx, claims.Email) - if perr != nil { - if errors.Is(perr, store.ErrNotFound) { - return rbac.Caller{}, ErrNoMembership - } - return rbac.Caller{}, perr - } - if aerr := repo.ActivatePendingMembership(ctx, claims.Email, claims.Subject); aerr != nil { - return rbac.Caller{}, aerr + m, err = withoutMembership(ctx, repo, claims, def) + if err != nil { + return rbac.Caller{}, err } - m = pending - m.UserSub = claims.Subject } return rbac.Caller{ Subject: claims.Subject, @@ -51,3 +69,83 @@ func ResolveCaller(ctx context.Context, repo store.Repository) (rbac.Caller, err Role: rbac.Role(m.Role), }, nil } + +// withoutMembership resolves a caller with no sub-keyed membership: first a +// pending invite matched by email, then the configured default membership. The +// invite is tried first so an admin's explicitly chosen role is never silently +// downgraded to the baseline. +func withoutMembership(ctx context.Context, repo store.Repository, claims *auth.Claims, def DefaultMembership) (*framesv1.Membership, error) { + // The claim is trimmed before lookup, and the lookup itself is + // case-insensitive: an invite is typed by a human while the claim comes from + // the identity provider, and the two disagreeing on case must not cause the + // invite to be missed. Missing it would silently hand the caller the default + // role, which can outrank the role they were actually invited with. + if email := strings.TrimSpace(claims.Email); email != "" { + pending, err := repo.GetPendingMembershipByEmail(ctx, email) + switch { + case err == nil: + // Activate by the stored address, not the claim: they may differ in + // case, and the update must match the row the lookup returned. + if aerr := repo.ActivatePendingMembership(ctx, pending.Email, claims.Subject); aerr != nil { + return nil, aerr + } + pending.UserSub = claims.Subject + return pending, nil + case !errors.Is(err, store.ErrNotFound): + return nil, err + } + } + return provisionDefault(ctx, repo, claims, def) +} + +// provisionDefault writes the baseline membership for a caller the store has +// never seen. It is deliberately the last resort: with no default configured it +// denies, which is the pre-existing behavior. +func provisionDefault(ctx context.Context, repo store.Repository, claims *auth.Claims, def DefaultMembership) (*framesv1.Membership, error) { + if !def.enabled() { + return nil, ErrNoMembership + } + org, err := repo.GetOrgBySlug(ctx, def.OrgSlug) + if err != nil { + if errors.Is(err, store.ErrNotFound) { + // A default role pointed at an org that does not exist is a + // misconfiguration. Deny rather than fail open or surface a 500: + // the operator asked for baseline access to *that* org, not to any. + slog.Warn("orgs: default role configured for unknown org, denying caller", + "org_slug", def.OrgSlug, "subject", claims.Subject) + return nil, ErrNoMembership + } + return nil, err + } + m := &framesv1.Membership{ + OrgId: org.Id, + UserSub: claims.Subject, + Role: string(def.Role), + Email: claims.Email, + AddedAt: timestamppb.Now(), + } + if err := repo.UpsertMembership(ctx, m); err != nil { + if !errors.Is(err, store.ErrAlreadyExists) { + return nil, err + } + // Two ways to land here. Either a concurrent first request won the + // insert, in which case re-reading finds the winner's row and it is + // authoritative - not our assumed role. Or the store already holds a + // membership for this email under a different subject, which is what a + // deleted-and-recreated identity-provider account looks like: the store + // keeps one membership per (org, email), so the stale row blocks the new + // one. That is a denial, not a server fault, so it must not escape as a + // raw store error and become a 500. + existing, rerr := repo.GetMembership(ctx, claims.Subject) + if rerr == nil { + return existing, nil + } + if errors.Is(rerr, store.ErrNotFound) { + slog.Warn("orgs: cannot provision default membership, email already belongs to another subject; denying caller", + "org_slug", def.OrgSlug, "subject", claims.Subject) + return nil, ErrNoMembership + } + return nil, rerr + } + return m, nil +} diff --git a/backend/internal/orgs/orgs_test.go b/backend/internal/orgs/orgs_test.go index 482852d..f528f88 100644 --- a/backend/internal/orgs/orgs_test.go +++ b/backend/internal/orgs/orgs_test.go @@ -3,13 +3,18 @@ package orgs_test import ( "context" "errors" + "sync" "testing" + "google.golang.org/protobuf/types/known/timestamppb" + framesv1 "github.com/nebari-dev/nebari-frames/gen/go/frames/v1" "github.com/nebari-dev/nebari-frames/backend/internal/auth" "github.com/nebari-dev/nebari-frames/backend/internal/orgs" "github.com/nebari-dev/nebari-frames/backend/internal/rbac" "github.com/nebari-dev/nebari-frames/backend/internal/store" + sqlitestore "github.com/nebari-dev/nebari-frames/backend/internal/store/sqlite" + "github.com/nebari-dev/nebari-frames/backend/internal/store/sqlite/migrations" ) func TestResolveCallerReconcilesPendingByEmail(t *testing.T) { @@ -20,7 +25,7 @@ func TestResolveCallerReconcilesPendingByEmail(t *testing.T) { // inject claims for a user with no sub-keyed membership yet ctx = auth.WithClaims(ctx, &auth.Claims{Subject: "sub-new", Email: "new@x.io"}) - caller, err := orgs.ResolveCaller(ctx, repo) + caller, err := orgs.ResolveCaller(ctx, repo, orgs.DefaultMembership{}) if err != nil { t.Fatalf("expected reconciliation, got err %v", err) } @@ -68,7 +73,7 @@ func TestResolveCaller(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - caller, err := orgs.ResolveCaller(tt.ctx, repo) + caller, err := orgs.ResolveCaller(tt.ctx, repo, orgs.DefaultMembership{}) if tt.wantErr != nil { if !errors.Is(err, tt.wantErr) { t.Fatalf("want error %v, got %v", tt.wantErr, err) @@ -87,3 +92,319 @@ func TestResolveCaller(t *testing.T) { }) } } + +func TestResolveCallerDefaultRole(t *testing.T) { + const orgSlug = "acme" + + // newRepo returns a store holding one org (acme) plus whatever the caller + // seeds on top of it. + newRepo := func(t *testing.T, seed func(context.Context, store.Repository)) store.Repository { + t.Helper() + ctx := context.Background() + repo := store.NewMemory() + if err := repo.CreateOrg(ctx, &framesv1.Org{Id: "o1", Slug: orgSlug, DisplayName: "Acme"}); err != nil { + t.Fatalf("seed org: %v", err) + } + if seed != nil { + seed(ctx, repo) + } + return repo + } + + tests := []struct { + name string + seed func(context.Context, store.Repository) + cfg orgs.DefaultMembership + // claims injected for the request under test + subject string + email string + + wantErr error + wantRole rbac.Role + wantOrgID string + wantPersisted bool + }{ + { + name: "no default configured denies an unknown caller", + cfg: orgs.DefaultMembership{}, + subject: "stranger", + wantErr: orgs.ErrNoMembership, + }, + { + name: "default role admits an unknown caller and persists the membership", + cfg: orgs.DefaultMembership{Role: rbac.RoleViewer, OrgSlug: orgSlug}, + subject: "stranger", + email: "stranger@x.io", + wantRole: rbac.RoleViewer, + wantOrgID: "o1", + wantPersisted: true, + }, + { + name: "an existing membership outranks the default", + seed: func(ctx context.Context, repo store.Repository) { + _ = repo.UpsertMembership(ctx, &framesv1.Membership{OrgId: "o1", UserSub: "u1", Role: "admin"}) + }, + cfg: orgs.DefaultMembership{Role: rbac.RoleViewer, OrgSlug: orgSlug}, + subject: "u1", + wantRole: rbac.RoleAdmin, + wantOrgID: "o1", + wantPersisted: true, + }, + { + name: "a pending invite outranks the default", + seed: func(ctx context.Context, repo store.Repository) { + _ = repo.AddPendingMembership(ctx, &framesv1.Membership{OrgId: "o1", Role: "publisher", Email: "invited@x.io"}) + }, + cfg: orgs.DefaultMembership{Role: rbac.RoleViewer, OrgSlug: orgSlug}, + subject: "sub-invited", + email: "invited@x.io", + wantRole: rbac.RolePublisher, + wantOrgID: "o1", + wantPersisted: true, + }, + { + name: "an unresolvable default org denies rather than failing open", + cfg: orgs.DefaultMembership{Role: rbac.RoleViewer, OrgSlug: "does-not-exist"}, + subject: "stranger", + wantErr: orgs.ErrNoMembership, + }, + { + name: "a default role with no org slug denies", + cfg: orgs.DefaultMembership{Role: rbac.RoleViewer}, + subject: "stranger", + wantErr: orgs.ErrNoMembership, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + repo := newRepo(t, tt.seed) + ctx := auth.WithClaims(context.Background(), &auth.Claims{Subject: tt.subject, Email: tt.email}) + + caller, err := orgs.ResolveCaller(ctx, repo, tt.cfg) + if tt.wantErr != nil { + if !errors.Is(err, tt.wantErr) { + t.Fatalf("want error %v, got caller %+v err %v", tt.wantErr, caller, err) + } + // A denied caller must never leave a membership row behind. + if _, gerr := repo.GetMembership(ctx, tt.subject); !errors.Is(gerr, store.ErrNotFound) { + t.Fatalf("denied caller should not be persisted, GetMembership err = %v", gerr) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if caller.Role != tt.wantRole { + t.Errorf("role = %q, want %q", caller.Role, tt.wantRole) + } + if caller.OrgID != tt.wantOrgID { + t.Errorf("orgID = %q, want %q", caller.OrgID, tt.wantOrgID) + } + if caller.Subject != tt.subject { + t.Errorf("subject = %q, want %q", caller.Subject, tt.subject) + } + if tt.wantPersisted { + m, gerr := repo.GetMembership(ctx, tt.subject) + if gerr != nil { + t.Fatalf("membership should be persisted: %v", gerr) + } + if rbac.Role(m.Role) != tt.wantRole { + t.Errorf("persisted role = %q, want %q", m.Role, tt.wantRole) + } + } + }) + } +} + +// A second request from the same defaulted user must not create a duplicate +// row nor reset a role an admin has since changed. +func TestResolveCallerDefaultRoleIsNotReappliedOnLaterRequests(t *testing.T) { + ctx := context.Background() + repo := store.NewMemory() + if err := repo.CreateOrg(ctx, &framesv1.Org{Id: "o1", Slug: "acme"}); err != nil { + t.Fatalf("seed org: %v", err) + } + cfg := orgs.DefaultMembership{Role: rbac.RoleViewer, OrgSlug: "acme"} + ctx = auth.WithClaims(ctx, &auth.Claims{Subject: "u1", Email: "u1@x.io"}) + + if _, err := orgs.ResolveCaller(ctx, repo, cfg); err != nil { + t.Fatalf("first resolve: %v", err) + } + // An admin promotes the user. + if err := repo.UpdateMembershipRole(ctx, "o1", "u1", "", "publisher"); err != nil { + t.Fatalf("promote: %v", err) + } + + caller, err := orgs.ResolveCaller(ctx, repo, cfg) + if err != nil { + t.Fatalf("second resolve: %v", err) + } + if caller.Role != rbac.RolePublisher { + t.Fatalf("role = %q, want publisher: the default must not overwrite a promoted role", caller.Role) + } +} + +// A Keycloak account that is deleted and recreated arrives with a new subject +// but the same email. The store holds one membership per (org, email), so +// provisioning the baseline row collides with the stale one. That must deny +// cleanly rather than surface as an internal error, so the operator sees the +// same "no access" page they would for any other denial. +// +// This runs against the real SQLite store on purpose: the in-memory fake does +// not enforce the (org_id, email) unique index, so the collision is invisible +// there. +func TestResolveCallerDefaultRoleEmailCollisionDenies(t *testing.T) { + ctx := context.Background() + db, err := sqlitestore.Open(t.TempDir() + "/t.db") + if err != nil { + t.Fatalf("open: %v", err) + } + t.Cleanup(func() { _ = db.Close() }) + if err := migrations.Run(ctx, db); err != nil { + t.Fatalf("migrate: %v", err) + } + repo := sqlitestore.New(db) + + if err := repo.CreateOrg(ctx, &framesv1.Org{Id: "o1", Slug: "acme", DisplayName: "Acme", CreatedAt: timestamppb.Now()}); err != nil { + t.Fatalf("create org: %v", err) + } + // The stale membership from the user's previous Keycloak account. + if err := repo.UpsertMembership(ctx, &framesv1.Membership{ + OrgId: "o1", UserSub: "old-sub", Role: "viewer", Email: "user@x.io", AddedAt: timestamppb.Now(), + }); err != nil { + t.Fatalf("seed stale membership: %v", err) + } + + cfg := orgs.DefaultMembership{Role: rbac.RoleViewer, OrgSlug: "acme"} + newCtx := auth.WithClaims(ctx, &auth.Claims{Subject: "new-sub", Email: "user@x.io"}) + + caller, err := orgs.ResolveCaller(newCtx, repo, cfg) + if !errors.Is(err, orgs.ErrNoMembership) { + t.Fatalf("want ErrNoMembership, got caller %+v err %v", caller, err) + } +} + +// A token that carries no subject cannot identify anyone. Because the store +// treats an empty user_sub as a pending invite, provisioning such a caller would +// write a row that masquerades as an invite, hand it the default role, and squat +// the (org, email) slot so a real invite for that address can never be created. +// Reject it at the authorization boundary, before any store access. +func TestResolveCallerRejectsEmptySubject(t *testing.T) { + ctx := context.Background() + repo := store.NewMemory() + if err := repo.CreateOrg(ctx, &framesv1.Org{Id: "o1", Slug: "acme"}); err != nil { + t.Fatalf("seed org: %v", err) + } + cfg := orgs.DefaultMembership{Role: rbac.RoleAdmin, OrgSlug: "acme"} + ctx = auth.WithClaims(ctx, &auth.Claims{Subject: "", Email: "ghost@x.io"}) + + caller, err := orgs.ResolveCaller(ctx, repo, cfg) + if !errors.Is(err, orgs.ErrNoClaims) { + t.Fatalf("want ErrNoClaims for a subjectless token, got caller %+v err %v", caller, err) + } + members, err := repo.ListMembershipsByOrg(ctx, "o1") + if err != nil { + t.Fatalf("list: %v", err) + } + if len(members) != 0 { + t.Errorf("a subjectless caller wrote %d membership row(s): %+v", len(members), members) + } +} + +// Identity providers do not guarantee the case of the email claim, and an +// invite is typed by hand. Before a default role existed, a case mismatch just +// denied the user and the admin noticed. With a default role it would instead +// hand out the baseline role - which can be *higher* than the role they were +// invited with - so the mismatch must not silently escalate. +func TestResolveCallerMatchesInviteEmailCaseInsensitively(t *testing.T) { + tests := []struct { + name string + inviteEmail string + claimEmail string + }{ + {name: "exact match", inviteEmail: "boss@x.io", claimEmail: "boss@x.io"}, + {name: "claim is upper", inviteEmail: "boss@x.io", claimEmail: "Boss@X.io"}, + {name: "invite is upper", inviteEmail: "Boss@X.IO", claimEmail: "boss@x.io"}, + {name: "claim has surrounding space", inviteEmail: "boss@x.io", claimEmail: " boss@x.io "}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + repo := store.NewMemory() + if err := repo.CreateOrg(ctx, &framesv1.Org{Id: "o1", Slug: "acme"}); err != nil { + t.Fatalf("seed org: %v", err) + } + if err := repo.AddPendingMembership(ctx, &framesv1.Membership{ + OrgId: "o1", Role: "viewer", Email: tt.inviteEmail, + }); err != nil { + t.Fatalf("invite: %v", err) + } + // The default role deliberately outranks the invited role, so an + // escalation is visible as a role mismatch rather than a denial. + cfg := orgs.DefaultMembership{Role: rbac.RolePublisher, OrgSlug: "acme"} + ctx = auth.WithClaims(ctx, &auth.Claims{Subject: "s1", Email: tt.claimEmail}) + + caller, err := orgs.ResolveCaller(ctx, repo, cfg) + if err != nil { + t.Fatalf("resolve: %v", err) + } + if caller.Role != rbac.RoleViewer { + t.Errorf("role = %q, want viewer: the invite must win over the default role", caller.Role) + } + }) + } +} + +// Concurrent first requests from the same user must converge on one membership +// row, and every caller must see the role that actually got stored rather than +// the one it optimistically assumed. Runs against real SQLite because the +// in-memory fake does not enforce the unique index that makes one writer lose. +func TestResolveCallerDefaultRoleConcurrentFirstRequests(t *testing.T) { + ctx := context.Background() + db, err := sqlitestore.Open(t.TempDir() + "/t.db") + if err != nil { + t.Fatalf("open: %v", err) + } + t.Cleanup(func() { _ = db.Close() }) + if err := migrations.Run(ctx, db); err != nil { + t.Fatalf("migrate: %v", err) + } + repo := sqlitestore.New(db) + if err := repo.CreateOrg(ctx, &framesv1.Org{Id: "o1", Slug: "acme", CreatedAt: timestamppb.Now()}); err != nil { + t.Fatalf("create org: %v", err) + } + + cfg := orgs.DefaultMembership{Role: rbac.RoleViewer, OrgSlug: "acme"} + reqCtx := auth.WithClaims(ctx, &auth.Claims{Subject: "u1", Email: "u1@x.io"}) + + const n = 8 + roles := make([]rbac.Role, n) + errs := make([]error, n) + var wg sync.WaitGroup + for i := range n { + wg.Add(1) + go func(i int) { + defer wg.Done() + caller, err := orgs.ResolveCaller(reqCtx, repo, cfg) + roles[i], errs[i] = caller.Role, err + }(i) + } + wg.Wait() + + for i := range n { + if errs[i] != nil { + t.Fatalf("request %d failed: %v", i, errs[i]) + } + if roles[i] != rbac.RoleViewer { + t.Errorf("request %d role = %q, want viewer", i, roles[i]) + } + } + members, err := repo.ListMembershipsByOrg(ctx, "o1") + if err != nil { + t.Fatalf("list: %v", err) + } + if len(members) != 1 { + t.Errorf("got %d membership rows, want exactly 1", len(members)) + } +} diff --git a/backend/internal/rbac/rbac.go b/backend/internal/rbac/rbac.go index cc0d730..c51ee9d 100644 --- a/backend/internal/rbac/rbac.go +++ b/backend/internal/rbac/rbac.go @@ -20,6 +20,17 @@ const ( PermDelete Permission = "delete" ) +// ParseRole resolves a configured role name to a Role, reporting whether it is +// one of the defined roles. Matching is exact: a typo in an operator's config +// must fail loudly at startup rather than silently deny every request. +func ParseRole(s string) (Role, bool) { + switch Role(s) { + case RoleViewer, RolePublisher, RoleAdmin: + return Role(s), true + } + return "", false +} + // Caller is the resolved identity + org membership of the requester. type Caller struct { Subject string diff --git a/backend/internal/rbac/rbac_test.go b/backend/internal/rbac/rbac_test.go index d74d305..9d10f43 100644 --- a/backend/internal/rbac/rbac_test.go +++ b/backend/internal/rbac/rbac_test.go @@ -54,3 +54,31 @@ func TestCanPublish(t *testing.T) { t.Fatal("admin must publish") } } + +func TestParseRole(t *testing.T) { + tests := []struct { + name string + in string + want rbac.Role + wantK bool + }{ + {name: "viewer", in: "viewer", want: rbac.RoleViewer, wantK: true}, + {name: "publisher", in: "publisher", want: rbac.RolePublisher, wantK: true}, + {name: "admin", in: "admin", want: rbac.RoleAdmin, wantK: true}, + {name: "empty is not a role", in: "", wantK: false}, + {name: "unknown role", in: "superuser", wantK: false}, + {name: "case sensitive", in: "Viewer", wantK: false}, + {name: "surrounding whitespace is not trimmed", in: " viewer", wantK: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, ok := rbac.ParseRole(tt.in) + if ok != tt.wantK { + t.Fatalf("ParseRole(%q) ok = %v, want %v", tt.in, ok, tt.wantK) + } + if ok && got != tt.want { + t.Errorf("ParseRole(%q) = %q, want %q", tt.in, got, tt.want) + } + }) + } +} diff --git a/backend/internal/seed/seed.go b/backend/internal/seed/seed.go index ebceaa6..f138d22 100644 --- a/backend/internal/seed/seed.go +++ b/backend/internal/seed/seed.go @@ -6,6 +6,8 @@ import ( "context" "crypto/rand" "errors" + "log/slog" + "strings" "github.com/oklog/ulid/v2" "google.golang.org/protobuf/types/known/timestamppb" @@ -44,14 +46,30 @@ func Run(ctx context.Context, repo store.Repository, cfg Config) error { return err } if cfg.AdminSub != "" { - if _, err := repo.GetMembership(ctx, cfg.AdminSub); errors.Is(err, store.ErrNotFound) { + existing, err := repo.GetMembership(ctx, cfg.AdminSub) + switch { + case errors.Is(err, store.ErrNotFound): if err := repo.UpsertMembership(ctx, &framesv1.Membership{ OrgId: org.Id, UserSub: cfg.AdminSub, Role: "admin", AddedAt: timestamppb.Now(), }); err != nil { return err } - } else if err != nil { + case err != nil: return err + default: + // The configured admin already has a membership. That is the normal + // case on restart, but it is also what a default-role deployment + // produces for someone who merely signed in - and skipping here + // would leave the org with no admin and no way to make one. Promote + // only when the org has none, so a deliberate demotion sticks. + promoted, err := promoteIfNoAdmins(ctx, repo, org.Id, existing) + if err != nil { + return err + } + if promoted { + slog.Warn("seed: promoted configured admin because the organization had none", + "org", org.Slug, "user_sub", cfg.AdminSub) + } } } @@ -67,7 +85,19 @@ func Run(ctx context.Context, repo store.Repository, cfg Config) error { return err } for _, m := range members { - if m.Email == cfg.AdminEmail && m.UserSub != "" { + if strings.EqualFold(m.Email, cfg.AdminEmail) && m.UserSub != "" { + // Already an active member. Same reasoning as the AdminSub path: + // with a default role configured this is what a user who simply + // signed in looks like, so promote when the org has no admin + // rather than silently doing nothing. + promoted, perr := promoteIfNoAdmins(ctx, repo, org.Id, m) + if perr != nil { + return perr + } + if promoted { + slog.Warn("seed: promoted configured admin because the organization had none", + "org", org.Slug, "email", cfg.AdminEmail) + } return nil } } @@ -81,3 +111,26 @@ func Run(ctx context.Context, repo store.Repository, cfg Config) error { return nil } + +// promoteIfNoAdmins raises m to admin when its org has no admin at all, +// reporting whether it did. It is the break-glass path for a deployment that +// enabled a default role before configuring an admin: without it the org can +// reach zero admins, and every route that could fix that requires an admin. +func promoteIfNoAdmins(ctx context.Context, repo store.Repository, orgID string, m *framesv1.Membership) (bool, error) { + if m.Role == "admin" { + return false, nil + } + admins, err := repo.CountAdmins(ctx, orgID) + if err != nil { + return false, err + } + if admins > 0 { + // Someone can still administer the org, so an explicit demotion of the + // configured admin is a decision to respect, not a state to undo. + return false, nil + } + if err := repo.UpdateMembershipRole(ctx, orgID, m.UserSub, m.Email, "admin"); err != nil { + return false, err + } + return true, nil +} diff --git a/backend/internal/seed/seed_test.go b/backend/internal/seed/seed_test.go index d5256b2..9107da5 100644 --- a/backend/internal/seed/seed_test.go +++ b/backend/internal/seed/seed_test.go @@ -176,3 +176,85 @@ func TestSeed_Run(t *testing.T) { }) } } + +// With auth.defaultRole enabled, any user who has signed in already holds a +// membership, which used to make the configured admin bootstrap a no-op: the +// sub path saw an existing membership and returned, and the email path saw an +// active member with that address and returned. An operator who enabled the +// default role before configuring an admin could therefore end up with an org +// that has no admin at all and no in-product way to create one. +// +// seed.Run now promotes the configured admin when the org has none, and leaves +// roles alone when an admin already exists. +func TestRun_PromotesConfiguredAdminWhenOrgHasNone(t *testing.T) { + tests := []struct { + name string + // existing memberships before seeding + seed func(context.Context, *store.Memory) + cfg seed.Config + wantSub string // membership to inspect afterwards + // expected role of wantSub after Run + wantRole string + }{ + { + name: "a baseline-provisioned user configured as admin by sub is promoted", + seed: func(ctx context.Context, repo *store.Memory) { + _ = repo.UpsertMembership(ctx, &framesv1.Membership{OrgId: "o1", UserSub: "boss-sub", Role: "viewer", Email: "boss@x.io"}) + }, + cfg: seed.Config{OrgSlug: "acme", AdminSub: "boss-sub"}, + wantSub: "boss-sub", + wantRole: "admin", + }, + { + name: "a baseline-provisioned user configured as admin by email is promoted", + seed: func(ctx context.Context, repo *store.Memory) { + _ = repo.UpsertMembership(ctx, &framesv1.Membership{OrgId: "o1", UserSub: "boss-sub", Role: "viewer", Email: "boss@x.io"}) + }, + cfg: seed.Config{OrgSlug: "acme", AdminEmail: "boss@x.io"}, + wantSub: "boss-sub", + wantRole: "admin", + }, + { + name: "a deliberate demotion is left alone while another admin exists", + seed: func(ctx context.Context, repo *store.Memory) { + _ = repo.UpsertMembership(ctx, &framesv1.Membership{OrgId: "o1", UserSub: "boss-sub", Role: "viewer", Email: "boss@x.io"}) + _ = repo.UpsertMembership(ctx, &framesv1.Membership{OrgId: "o1", UserSub: "other-admin", Role: "admin", Email: "other@x.io"}) + }, + cfg: seed.Config{OrgSlug: "acme", AdminSub: "boss-sub"}, + wantSub: "boss-sub", + wantRole: "viewer", + }, + { + name: "an existing admin is untouched", + seed: func(ctx context.Context, repo *store.Memory) { + _ = repo.UpsertMembership(ctx, &framesv1.Membership{OrgId: "o1", UserSub: "boss-sub", Role: "admin", Email: "boss@x.io"}) + }, + cfg: seed.Config{OrgSlug: "acme", AdminSub: "boss-sub"}, + wantSub: "boss-sub", + wantRole: "admin", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + repo := store.NewMemory() + if err := repo.CreateOrg(ctx, &framesv1.Org{Id: "o1", Slug: "acme", DisplayName: "Acme"}); err != nil { + t.Fatalf("create org: %v", err) + } + tt.seed(ctx, repo) + + if err := seed.Run(ctx, repo, tt.cfg); err != nil { + t.Fatalf("run: %v", err) + } + + m, err := repo.GetMembership(ctx, tt.wantSub) + if err != nil { + t.Fatalf("get membership %q: %v", tt.wantSub, err) + } + if m.Role != tt.wantRole { + t.Errorf("role = %q, want %q", m.Role, tt.wantRole) + } + }) + } +} diff --git a/backend/internal/server/server.go b/backend/internal/server/server.go index af13711..c7a9bb8 100644 --- a/backend/internal/server/server.go +++ b/backend/internal/server/server.go @@ -8,8 +8,6 @@ import ( "github.com/nebari-dev/nebari-frames/backend/internal/auth" "github.com/nebari-dev/nebari-frames/backend/internal/branding" - "github.com/nebari-dev/nebari-frames/backend/internal/frames" - "github.com/nebari-dev/nebari-frames/backend/internal/store" "github.com/nebari-dev/nebari-frames/gen/go/frames/v1/framesv1connect" webui "github.com/nebari-dev/nebari-frames/web" ) @@ -30,8 +28,12 @@ type Mounter interface { // requests pass through with stub claims and /readyz always returns 200. Pass a // non-nil mcpMounter to also mount the MCP endpoint routes. A zero brandingCfg // serves an empty config document, leaving the SPA on its built-in defaults. +// +// The FrameService is injected rather than constructed here so that the Connect +// and MCP endpoints share one instance: two separately built services could be +// configured differently and disagree about who may do what. func New( - repo store.Repository, + svc framesv1connect.FrameServiceHandler, validator auth.TokenValidator, authCfg auth.Config, brandingCfg branding.Config, @@ -48,7 +50,7 @@ func New( mux.HandleFunc("/config.json", handleBranding(brandingCfg)) interceptor := auth.NewInterceptor(validator, devMode) path, handler := framesv1connect.NewFrameServiceHandler( - frames.NewService(repo), + svc, connect.WithInterceptors(interceptor), ) mux.Handle(path, handler) diff --git a/backend/internal/server/server_test.go b/backend/internal/server/server_test.go index 4343314..f88dea7 100644 --- a/backend/internal/server/server_test.go +++ b/backend/internal/server/server_test.go @@ -11,6 +11,7 @@ import ( "github.com/nebari-dev/nebari-frames/backend/internal/auth" "github.com/nebari-dev/nebari-frames/backend/internal/branding" + "github.com/nebari-dev/nebari-frames/backend/internal/frames" "github.com/nebari-dev/nebari-frames/backend/internal/server" "github.com/nebari-dev/nebari-frames/backend/internal/store" ) @@ -38,7 +39,7 @@ func TestServer_Healthz(t *testing.T) { }, } - srv := server.New(store.NewMemory(), nil, auth.Config{}, branding.Config{}, true, nil) // dev mode + srv := server.New(frames.NewService(store.NewMemory()), nil, auth.Config{}, branding.Config{}, true, nil) // dev mode ts := httptest.NewServer(srv.Handler()) t.Cleanup(ts.Close) @@ -84,7 +85,7 @@ func TestServer_AuthConfig(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - srv := server.New(store.NewMemory(), nil, tt.cfg, branding.Config{}, true, nil) + srv := server.New(frames.NewService(store.NewMemory()), nil, tt.cfg, branding.Config{}, true, nil) ts := httptest.NewServer(srv.Handler()) t.Cleanup(ts.Close) resp, err := http.Get(ts.URL + "/auth/config") @@ -121,7 +122,7 @@ func TestServer_AuthConfig(t *testing.T) { } func TestServer_AuthConfig_MethodNotAllowed(t *testing.T) { - srv := server.New(store.NewMemory(), nil, auth.Config{IssuerURL: "https://oidc.example", ClientID: "web"}, branding.Config{}, true, nil) + srv := server.New(frames.NewService(store.NewMemory()), nil, auth.Config{IssuerURL: "https://oidc.example", ClientID: "web"}, branding.Config{}, true, nil) ts := httptest.NewServer(srv.Handler()) t.Cleanup(ts.Close) @@ -178,7 +179,7 @@ func TestServer_Branding(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - srv := server.New(store.NewMemory(), nil, auth.Config{}, tt.cfg, true, nil) + srv := server.New(frames.NewService(store.NewMemory()), nil, auth.Config{}, tt.cfg, true, nil) ts := httptest.NewServer(srv.Handler()) t.Cleanup(ts.Close) @@ -207,7 +208,7 @@ func TestServer_Branding(t *testing.T) { } func TestServer_Branding_MethodNotAllowed(t *testing.T) { - srv := server.New(store.NewMemory(), nil, auth.Config{}, branding.Config{}, true, nil) + srv := server.New(frames.NewService(store.NewMemory()), nil, auth.Config{}, branding.Config{}, true, nil) ts := httptest.NewServer(srv.Handler()) t.Cleanup(ts.Close) @@ -258,7 +259,7 @@ func TestServer_Readyz(t *testing.T) { } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - srv := server.New(store.NewMemory(), tc.validator, auth.Config{}, branding.Config{}, tc.devMode, nil) + srv := server.New(frames.NewService(store.NewMemory()), tc.validator, auth.Config{}, branding.Config{}, tc.devMode, nil) ts := httptest.NewServer(srv.Handler()) t.Cleanup(ts.Close) resp, err := http.Get(ts.URL + "/readyz") diff --git a/backend/internal/store/memory.go b/backend/internal/store/memory.go index 046aa04..785705c 100644 --- a/backend/internal/store/memory.go +++ b/backend/internal/store/memory.go @@ -108,7 +108,8 @@ func (m *Memory) GetPendingMembershipByEmail(_ context.Context, email string) (* m.mu.RLock() defer m.mu.RUnlock() for _, mem := range m.memberships { - if mem.UserSub == "" && mem.Email == email { + // Mirrors the SQLite COLLATE NOCASE lookup. + if mem.UserSub == "" && strings.EqualFold(mem.Email, email) { return mem, nil } } @@ -256,7 +257,7 @@ func (m *Memory) ActivatePendingMembership(_ context.Context, email, sub string) m.mu.Lock() defer m.mu.Unlock() for _, e := range m.memberships { - if e.UserSub == "" && e.Email == email { + if e.UserSub == "" && strings.EqualFold(e.Email, email) { e.UserSub = sub return nil } diff --git a/backend/internal/store/sqlite/sqlite.go b/backend/internal/store/sqlite/sqlite.go index 9e7c010..f15363d 100644 --- a/backend/internal/store/sqlite/sqlite.go +++ b/backend/internal/store/sqlite/sqlite.go @@ -190,7 +190,10 @@ func (r *Repository) GetPendingMembershipByEmail(ctx context.Context, email stri var added string var e sql.NullString err := r.db.QueryRowContext(ctx, - `SELECT org_id, user_sub, role, added_at, email FROM org_memberships WHERE email = ? AND user_sub = '' LIMIT 1`, email). + // COLLATE NOCASE: identity providers do not guarantee the case of the + // email claim and invites are typed by hand, so a case difference must + // not cause the invite to be missed. + `SELECT org_id, user_sub, role, added_at, email FROM org_memberships WHERE email = ? COLLATE NOCASE AND user_sub = '' LIMIT 1`, email). Scan(&m.OrgId, &m.UserSub, &m.Role, &added, &e) if errors.Is(err, sql.ErrNoRows) { return nil, store.ErrNotFound @@ -445,7 +448,7 @@ func (r *Repository) AddPendingMembership(ctx context.Context, m *framesv1.Membe func (r *Repository) ActivatePendingMembership(ctx context.Context, email, sub string) error { res, err := r.db.ExecContext(ctx, - `UPDATE org_memberships SET user_sub = ? WHERE email = ? AND user_sub = ''`, sub, email) + `UPDATE org_memberships SET user_sub = ? WHERE email = ? COLLATE NOCASE AND user_sub = ''`, sub, email) if err != nil { return err } diff --git a/chart/README.md b/chart/README.md index 4c943ee..9a87d45 100644 --- a/chart/README.md +++ b/chart/README.md @@ -111,6 +111,7 @@ The server writes structured JSON logs to stdout, ready for collection by the pl | `seed.orgDisplayName` | Display name for the seeded organization. | | `seed.adminEmail` | Email of the first admin (reconciled to the OIDC subject on first login). | | `auth.devMode` | When `true` (and NebariApp auth is off), disables auth and uses a fixed dev identity. Local use only. | +| `auth.defaultRole` | Baseline role for an authenticated user with no membership: `""` (deny, the default), `viewer`, `publisher`, or `admin`. Requires `seed.orgSlug` (the install fails without it). Enabling it makes the identity provider the access boundary, turns member removal into a demotion rather than a revocation, and makes "Add member" fail for users who have already signed in. Configure `seed.adminEmail` too: the server promotes the configured admin when the org has none, which is the only recovery path if every admin is lost. | | `auth.oidc.issuerUrl` | OIDC issuer URL for self-managed auth (used when NebariApp is off and dev mode is off). | | `auth.oidc.clientId` | OIDC client id for the SPA. | | `auth.oidc.deviceClientId` | OIDC client id for the device-code flow (CLI login). | diff --git a/chart/templates/deployment.yaml b/chart/templates/deployment.yaml index 393fe90..039a8f1 100644 --- a/chart/templates/deployment.yaml +++ b/chart/templates/deployment.yaml @@ -62,6 +62,14 @@ spec: - name: SEED_ADMIN_EMAIL value: {{ .Values.seed.adminEmail | quote }} {{- end }} + {{- if .Values.auth.defaultRole }} + - name: FRAMES_DEFAULT_ROLE + value: {{ .Values.auth.defaultRole | quote }} + {{- /* The backend also enforces this, but failing here turns a + crash-looping pod into an install-time error. Assigned to a + throwaway so `required` emits nothing into the manifest. */}} + {{- $_ := required "auth.defaultRole requires seed.orgSlug: the baseline role needs an organization to grant access to" .Values.seed.orgSlug }} + {{- end }} {{- if and .Values.nebariapp.enabled .Values.nebariapp.auth.enabled }} - name: OIDC_ISSUER_URL valueFrom: diff --git a/chart/values.schema.json b/chart/values.schema.json index e95f7cf..9992aaf 100644 --- a/chart/values.schema.json +++ b/chart/values.schema.json @@ -59,6 +59,7 @@ "type": "object", "properties": { "devMode": { "type": "boolean" }, + "defaultRole": { "type": "string", "enum": ["", "viewer", "publisher", "admin"] }, "oidc": { "type": "object", "additionalProperties": false, diff --git a/chart/values.yaml b/chart/values.yaml index 5834d38..9b88c04 100644 --- a/chart/values.yaml +++ b/chart/values.yaml @@ -77,6 +77,14 @@ seed: # else -> auth.oidc.* (self-managed OIDC) auth: devMode: false + # Baseline role granted to any authenticated user who has no membership yet: + # "" (deny, the default), viewer, publisher, or admin. Setting it means anyone + # the identity provider admits gets this role in seed.orgSlug, so the realm + # becomes the access boundary. It also makes removing a member a demotion + # rather than a revocation - the user is re-provisioned at this role on their + # next request - so revoke access in the identity provider instead. Requires + # seed.orgSlug; the server refuses to start without it. + defaultRole: "" oidc: issuerUrl: "" clientId: "" From c29b691243234d6fa3c9766b7207570fea55b441 Mon Sep 17 00:00:00 2001 From: Chuck McAndrew <6248903+dcmcand@users.noreply.github.com> Date: Thu, 20 Aug 2026 14:30:52 -0700 Subject: [PATCH 2/8] fix(auth): scope invite activation and canonicalize baseline emails 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. --- README.md | 13 ++-- backend/internal/orgs/orgs.go | 8 ++- backend/internal/orgs/orgs_test.go | 70 ++++++++++++++++++-- backend/internal/seed/seed.go | 19 ++++++ backend/internal/seed/seed_test.go | 59 +++++++++++++++++ backend/internal/store/memory.go | 8 ++- backend/internal/store/sqlite/sqlite.go | 12 +++- backend/internal/store/sqlite/sqlite_test.go | 38 +++++++++++ backend/internal/store/store.go | 13 ++++ 9 files changed, 225 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index 9e9a1d5..e2f7b28 100644 --- a/README.md +++ b/README.md @@ -136,11 +136,16 @@ Consequences to weigh before enabling it: role on their next request. To actually revoke access, disable the user in Keycloak, or unset `auth.defaultRole` and manage membership explicitly. - **"Add member" stops working for anyone who has already signed in.** They already hold a - membership, so adding them by email fails with "a member with that email already exists". Change - their role from the members list instead. -- **Set `seed.adminEmail` (or `seed.adminSub`) as well.** A user who has signed in at the baseline + membership, so adding them by email is rejected as already present. Change their role from the + members list instead. If their sign-in address differs from the one you invite (a different + address, not just different capitalization), the invite is accepted and then never applies, because + they already have a membership - see + [#66](https://github.com/nebari-dev/nebari-frames/issues/66). +- **Set `seed.adminSub` (or `seed.adminEmail`) as well.** A user who has signed in at the baseline role already has a membership, which is why the server promotes the configured admin whenever the - organization has none. That recovery only works if an admin is configured, so configure one. + organization has none. That recovery only works if an admin is configured, so configure one - + and prefer `seed.adminSub`, since it identifies the user by their stable subject rather than by an + address that may not match what their token carries. An invalid role, or a role set without `seed.orgSlug`, fails at startup with a message naming the variable rather than silently denying every request. diff --git a/backend/internal/orgs/orgs.go b/backend/internal/orgs/orgs.go index 66bf3e5..6423471 100644 --- a/backend/internal/orgs/orgs.go +++ b/backend/internal/orgs/orgs.go @@ -5,7 +5,6 @@ import ( "context" "errors" "log/slog" - "strings" "google.golang.org/protobuf/types/known/timestamppb" @@ -80,7 +79,7 @@ func withoutMembership(ctx context.Context, repo store.Repository, claims *auth. // the identity provider, and the two disagreeing on case must not cause the // invite to be missed. Missing it would silently hand the caller the default // role, which can outrank the role they were actually invited with. - if email := strings.TrimSpace(claims.Email); email != "" { + if email := store.CanonicalEmail(claims.Email); email != "" { pending, err := repo.GetPendingMembershipByEmail(ctx, email) switch { case err == nil: @@ -121,7 +120,10 @@ func provisionDefault(ctx context.Context, repo store.Repository, claims *auth.C OrgId: org.Id, UserSub: claims.Subject, Role: string(def.Role), - Email: claims.Email, + // Canonical, not verbatim: a row carrying the claim's stray whitespace or + // casing would not collide with a later invite typed normally, so that + // invite would be accepted and then never take effect. + Email: store.CanonicalEmail(claims.Email), AddedAt: timestamppb.Now(), } if err := repo.UpsertMembership(ctx, m); err != nil { diff --git a/backend/internal/orgs/orgs_test.go b/backend/internal/orgs/orgs_test.go index f528f88..c07ad24 100644 --- a/backend/internal/orgs/orgs_test.go +++ b/backend/internal/orgs/orgs_test.go @@ -356,10 +356,19 @@ func TestResolveCallerMatchesInviteEmailCaseInsensitively(t *testing.T) { } } -// Concurrent first requests from the same user must converge on one membership -// row, and every caller must see the role that actually got stored rather than -// the one it optimistically assumed. Runs against real SQLite because the -// in-memory fake does not enforce the unique index that makes one writer lose. +// Concurrent first requests from the same user must converge on exactly one +// membership row, with every caller succeeding and agreeing on the role. All +// racers share one configuration because that is what a single process does - +// the default role comes from one environment variable. +// +// What this does NOT prove is that a caller which loses the insert re-reads the +// winner's row instead of returning its own assumption: with identical config +// both answers are the same role. That property is pinned by +// TestResolveCallerDefaultRoleEmailCollisionDenies, which fails if the +// ErrAlreadyExists handling is removed. +// +// Runs against real SQLite because the in-memory fake does not enforce the +// unique index that makes a writer lose. func TestResolveCallerDefaultRoleConcurrentFirstRequests(t *testing.T) { ctx := context.Background() db, err := sqlitestore.Open(t.TempDir() + "/t.db") @@ -408,3 +417,56 @@ func TestResolveCallerDefaultRoleConcurrentFirstRequests(t *testing.T) { t.Errorf("got %d membership rows, want exactly 1", len(members)) } } + +func TestResolveCallerDefaultRoleStoresCanonicalEmail(t *testing.T) { + tests := []struct { + name string + claim string + want string + }{ + {name: "surrounding whitespace is stripped", claim: " bob@x.io ", want: "bob@x.io"}, + {name: "case is folded", claim: "Bob@X.io", want: "bob@x.io"}, + {name: "both", claim: " BOB@X.IO\n", want: "bob@x.io"}, + {name: "already canonical", claim: "bob@x.io", want: "bob@x.io"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + repo := store.NewMemory() + if err := repo.CreateOrg(ctx, &framesv1.Org{Id: "o1", Slug: "acme"}); err != nil { + t.Fatalf("seed org: %v", err) + } + cfg := orgs.DefaultMembership{Role: rbac.RoleViewer, OrgSlug: "acme"} + if _, err := orgs.ResolveCaller(auth.WithClaims(ctx, &auth.Claims{Subject: "s1", Email: tt.claim}), repo, cfg); err != nil { + t.Fatalf("resolve: %v", err) + } + m, err := repo.GetMembership(ctx, "s1") + if err != nil { + t.Fatalf("get membership: %v", err) + } + if m.Email != tt.want { + t.Errorf("stored email = %q, want %q", m.Email, tt.want) + } + }) + } +} + +// A baseline row must not let a later invite look like it succeeded when it can +// never take effect. The admin should get a conflict they can act on. +func TestAddingAnInviteForAnExistingBaselineMemberConflicts(t *testing.T) { + ctx := context.Background() + repo := store.NewMemory() + if err := repo.CreateOrg(ctx, &framesv1.Org{Id: "o1", Slug: "acme"}); err != nil { + t.Fatalf("seed org: %v", err) + } + cfg := orgs.DefaultMembership{Role: rbac.RoleViewer, OrgSlug: "acme"} + if _, err := orgs.ResolveCaller(auth.WithClaims(ctx, &auth.Claims{Subject: "s1", Email: "Bob@X.io"}), repo, cfg); err != nil { + t.Fatalf("resolve: %v", err) + } + // Any casing of the same address must be recognised as already present. + for _, email := range []string{"bob@x.io", "Bob@X.io", "BOB@X.IO"} { + if err := repo.AddPendingMembership(ctx, &framesv1.Membership{OrgId: "o1", Role: "admin", Email: email}); !errors.Is(err, store.ErrAlreadyExists) { + t.Errorf("AddPendingMembership(%q) err = %v, want ErrAlreadyExists: a dead invite must not look like a success", email, err) + } + } +} diff --git a/backend/internal/seed/seed.go b/backend/internal/seed/seed.go index f138d22..f84f731 100644 --- a/backend/internal/seed/seed.go +++ b/backend/internal/seed/seed.go @@ -120,6 +120,18 @@ func promoteIfNoAdmins(ctx context.Context, repo store.Repository, orgID string, if m.Role == "admin" { return false, nil } + // A membership is unique per subject across all orgs, so the configured + // admin's existing 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) would match nothing and the + // resulting error would exit the server on startup, so leave it alone: the + // operator's admin is a member of somewhere else, and this org's admin + // problem is not fixable by rewriting that row. + if m.OrgId != orgID { + slog.Warn("seed: configured admin belongs to a different organization, not promoting", + "configured_org", orgID, "membership_org", m.OrgId, "user_sub", m.UserSub) + return false, nil + } admins, err := repo.CountAdmins(ctx, orgID) if err != nil { return false, err @@ -130,6 +142,13 @@ func promoteIfNoAdmins(ctx context.Context, repo store.Repository, orgID string, return false, nil } if err := repo.UpdateMembershipRole(ctx, orgID, m.UserSub, m.Email, "admin"); err != nil { + // Nothing matched. Startup must not fail over a membership that moved or + // vanished between the read and the update. + if errors.Is(err, store.ErrNotFound) { + slog.Warn("seed: configured admin membership not found while promoting, skipping", + "org", orgID, "user_sub", m.UserSub) + return false, nil + } return false, err } return true, nil diff --git a/backend/internal/seed/seed_test.go b/backend/internal/seed/seed_test.go index 9107da5..67bc851 100644 --- a/backend/internal/seed/seed_test.go +++ b/backend/internal/seed/seed_test.go @@ -258,3 +258,62 @@ func TestRun_PromotesConfiguredAdminWhenOrgHasNone(t *testing.T) { }) } } + +// A membership is unique per subject across all orgs, so the configured admin's +// existing membership may belong to a different org than the one being seeded - +// which happens as soon as seed.orgSlug changes while the database persists. +// Promoting a row in the wrong org matches nothing, and returning that error +// from Run makes the server exit, crash-looping on an opaque "not found". +func TestRun_DoesNotFailWhenConfiguredAdminBelongsToAnotherOrg(t *testing.T) { + ctx := context.Background() + repo := store.NewMemory() + if err := seed.Run(ctx, repo, seed.Config{OrgSlug: "old", AdminSub: "root"}); err != nil { + t.Fatalf("first seed: %v", err) + } + old, err := repo.GetOrgBySlug(ctx, "old") + if err != nil { + t.Fatalf("get old org: %v", err) + } + // A user who picked up a membership in the original org. + if err := repo.UpsertMembership(ctx, &framesv1.Membership{OrgId: old.Id, UserSub: "boss", Role: "viewer"}); err != nil { + t.Fatalf("seed membership: %v", err) + } + + // The operator changes seed.orgSlug and names that user as admin. + if err := seed.Run(ctx, repo, seed.Config{OrgSlug: "new", AdminSub: "boss"}); err != nil { + t.Fatalf("second seed must not fail the server startup: %v", err) + } +} + +// A stale pending invite must not prevent the break-glass promote. A pending row +// is not an admin (CountAdmins ignores it), so an org holding only a pending +// invite plus baseline-role members has zero admins; naming a signed-in user via +// seed.adminSub has to still work. Recovery by email cannot help here - the +// invite is for an address that user never signed in with - which is why +// adminSub is the reliable lever. +func TestRun_AStalePendingInviteDoesNotBlockPromotion(t *testing.T) { + ctx := context.Background() + repo := store.NewMemory() + if err := repo.CreateOrg(ctx, &framesv1.Org{Id: "o1", Slug: "acme"}); err != nil { + t.Fatalf("create org: %v", err) + } + // An invite for the configured admin that has not been claimed. + if err := repo.AddPendingMembership(ctx, &framesv1.Membership{OrgId: "o1", Role: "admin", Email: "boss@x.io"}); err != nil { + t.Fatalf("invite: %v", err) + } + // The same person signed in under a different address and got the baseline. + if err := repo.UpsertMembership(ctx, &framesv1.Membership{OrgId: "o1", UserSub: "boss-sub", Role: "viewer", Email: "boss.work@x.io"}); err != nil { + t.Fatalf("baseline row: %v", err) + } + if n, _ := repo.CountAdmins(ctx, "o1"); n != 0 { + t.Fatalf("precondition: want 0 admins, got %d", n) + } + + // Seeding with the sub of the signed-in user must promote them. + if err := seed.Run(ctx, repo, seed.Config{OrgSlug: "acme", AdminSub: "boss-sub", AdminEmail: "boss@x.io"}); err != nil { + t.Fatalf("run: %v", err) + } + if n, _ := repo.CountAdmins(ctx, "o1"); n != 1 { + t.Errorf("admins = %d, want 1: the org must not be left without one", n) + } +} diff --git a/backend/internal/store/memory.go b/backend/internal/store/memory.go index 785705c..550c8c4 100644 --- a/backend/internal/store/memory.go +++ b/backend/internal/store/memory.go @@ -109,7 +109,7 @@ func (m *Memory) GetPendingMembershipByEmail(_ context.Context, email string) (* defer m.mu.RUnlock() for _, mem := range m.memberships { // Mirrors the SQLite COLLATE NOCASE lookup. - if mem.UserSub == "" && strings.EqualFold(mem.Email, email) { + if mem.UserSub == "" && CanonicalEmail(mem.Email) == CanonicalEmail(email) { return mem, nil } } @@ -240,7 +240,9 @@ func (m *Memory) AddPendingMembership(_ context.Context, mem *framesv1.Membershi m.mu.Lock() defer m.mu.Unlock() for _, e := range m.memberships { - if e.OrgId == mem.OrgId && e.Email == mem.Email { + // Case-insensitive so the fake rejects what a canonicalized store would; + // the SQLite index is still case-sensitive, tracked in #65. + if e.OrgId == mem.OrgId && CanonicalEmail(e.Email) == CanonicalEmail(mem.Email) { return ErrAlreadyExists } } @@ -257,7 +259,7 @@ func (m *Memory) ActivatePendingMembership(_ context.Context, email, sub string) m.mu.Lock() defer m.mu.Unlock() for _, e := range m.memberships { - if e.UserSub == "" && strings.EqualFold(e.Email, email) { + if e.UserSub == "" && CanonicalEmail(e.Email) == CanonicalEmail(email) { e.UserSub = sub return nil } diff --git a/backend/internal/store/sqlite/sqlite.go b/backend/internal/store/sqlite/sqlite.go index f15363d..113cf6c 100644 --- a/backend/internal/store/sqlite/sqlite.go +++ b/backend/internal/store/sqlite/sqlite.go @@ -447,8 +447,18 @@ func (r *Repository) AddPendingMembership(ctx context.Context, m *framesv1.Membe } func (r *Repository) ActivatePendingMembership(ctx context.Context, email, sub string) error { + // Scoped to a single row by rowid. The email is the address the lookup + // returned, so an exact match is right here - and case-variant invites can + // coexist (#65), so an unscoped predicate could match several rows and try to + // give them all the same user_sub, violating the unique index on user_sub and + // rolling back the whole statement. res, err := r.db.ExecContext(ctx, - `UPDATE org_memberships SET user_sub = ? WHERE email = ? COLLATE NOCASE AND user_sub = ''`, sub, email) + `UPDATE org_memberships SET user_sub = ? + WHERE rowid = ( + SELECT rowid FROM org_memberships + WHERE email = ? AND user_sub = '' + ORDER BY rowid LIMIT 1 + )`, sub, email) if err != nil { return err } diff --git a/backend/internal/store/sqlite/sqlite_test.go b/backend/internal/store/sqlite/sqlite_test.go index bbac0d4..2a89a32 100644 --- a/backend/internal/store/sqlite/sqlite_test.go +++ b/backend/internal/store/sqlite/sqlite_test.go @@ -697,3 +697,41 @@ func TestSQLite_PublishAtomicRollback(t *testing.T) { }) } } + +// Activating an invite must touch exactly one row. Two pending invites differing +// only in case can coexist (see #65), and a case-insensitive UPDATE with no row +// scoping would try to give both the same user_sub, violating the unique index +// on user_sub and rolling the whole statement back - locking the user out with +// an internal error instead of activating their invite. +func TestActivatePendingMembershipTouchesOneRow(t *testing.T) { + r := newRepo(t) + ctx := context.Background() + seedOrg(t, r, "o1", "acme") + now := timestamppb.Now() + + if err := r.AddPendingMembership(ctx, &framesv1.Membership{OrgId: "o1", Role: "viewer", Email: "boss@x.io", AddedAt: now}); err != nil { + t.Fatalf("first invite: %v", err) + } + // Permitted today because the unique index on (org_id, email) is + // case-sensitive; tracked in #65. + if err := r.AddPendingMembership(ctx, &framesv1.Membership{OrgId: "o1", Role: "publisher", Email: "Boss@X.io", AddedAt: now}); err != nil { + t.Fatalf("second invite: %v", err) + } + + if err := r.ActivatePendingMembership(ctx, "boss@x.io", "s1"); err != nil { + t.Fatalf("activate: %v", err) + } + members, err := r.ListMembershipsByOrg(ctx, "o1") + if err != nil { + t.Fatalf("list: %v", err) + } + activated := 0 + for _, m := range members { + if m.UserSub == "s1" { + activated++ + } + } + if activated != 1 { + t.Errorf("activated %d rows, want exactly 1: %+v", activated, members) + } +} diff --git a/backend/internal/store/store.go b/backend/internal/store/store.go index 15e70e6..2a951bf 100644 --- a/backend/internal/store/store.go +++ b/backend/internal/store/store.go @@ -6,6 +6,7 @@ package store import ( "context" "errors" + "strings" framesv1 "github.com/nebari-dev/nebari-frames/gen/go/frames/v1" ) @@ -15,6 +16,18 @@ var ( ErrAlreadyExists = errors.New("already exists") ) +// CanonicalEmail normalizes an address for storage and comparison. Identity +// providers do not guarantee the case or surrounding whitespace of an email +// claim, and invites are typed by hand, so an address has to be reduced to one +// form before it can be compared or constrained. +// +// Applied to rows this package writes. Rows written before it existed are not +// retrofitted, and the SQLite unique index on (org_id, email) is still +// case-sensitive; both are tracked in #65. +func CanonicalEmail(email string) string { + return strings.ToLower(strings.TrimSpace(email)) +} + // Grant is a permission grant on a frame (whole-frame only in MVP). type Grant struct { SubjectType string // "user" | "org" From 7d21ae5230242e020e63dbb4be6a3a45376d0819 Mon Sep 17 00:00:00 2001 From: Chuck McAndrew <6248903+dcmcand@users.noreply.github.com> Date: Thu, 20 Aug 2026 15:49:14 -0700 Subject: [PATCH 3/8] fix(dev): request the basic client scope so dev-auth tokens carry a subject 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. --- dev/keycloak/frames-realm.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev/keycloak/frames-realm.json b/dev/keycloak/frames-realm.json index 808bda7..f504900 100644 --- a/dev/keycloak/frames-realm.json +++ b/dev/keycloak/frames-realm.json @@ -11,7 +11,7 @@ "directAccessGrantsEnabled": false, "redirectUris": ["http://localhost:5173/*", "http://localhost:8080/*"], "webOrigins": ["http://localhost:5173", "http://localhost:8080"], - "defaultClientScopes": ["email", "profile", "roles", "web-origins"], + "defaultClientScopes": ["basic", "email", "profile", "roles", "web-origins"], "attributes": { "pkce.code.challenge.method": "S256", "post.logout.redirect.uris": "http://localhost:5173/*##http://localhost:8080/*" From 21954300098749b15ee0a208adb1b54d5686fdf7 Mon Sep 17 00:00:00 2001 From: Chuck McAndrew <6248903+dcmcand@users.noreply.github.com> Date: Thu, 20 Aug 2026 15:58:57 -0700 Subject: [PATCH 4/8] test(dev): seed alice and bob so the dev realm can exercise more than 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. --- dev/keycloak/frames-realm.json | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/dev/keycloak/frames-realm.json b/dev/keycloak/frames-realm.json index f504900..b84f133 100644 --- a/dev/keycloak/frames-realm.json +++ b/dev/keycloak/frames-realm.json @@ -41,6 +41,28 @@ "credentials": [ { "type": "password", "value": "dev", "temporary": false } ] + }, + { + "username": "alice", + "email": "alice@localhost", + "emailVerified": true, + "enabled": true, + "firstName": "Alice", + "lastName": "Publisher", + "credentials": [ + { "type": "password", "value": "alice", "temporary": false } + ] + }, + { + "username": "bob", + "email": "bob@localhost", + "emailVerified": true, + "enabled": true, + "firstName": "Bob", + "lastName": "Viewer", + "credentials": [ + { "type": "password", "value": "bob", "temporary": false } + ] } ] } From 73210adc1dcef50ae847a269c46ee35828210564 Mon Sep 17 00:00:00 2001 From: Chuck McAndrew <6248903+dcmcand@users.noreply.github.com> Date: Thu, 20 Aug 2026 16:21:01 -0700 Subject: [PATCH 5/8] fix: canonicalize membership emails and cap Connect request bodies 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. --- backend/internal/server/server.go | 9 ++ backend/internal/server/server_test.go | 35 +++++++ backend/internal/store/memory.go | 3 +- .../005_canonical_membership_email.sql | 41 ++++++++ backend/internal/store/sqlite/sqlite.go | 14 +-- backend/internal/store/sqlite/sqlite_test.go | 95 +++++++++++++++---- 6 files changed, 169 insertions(+), 28 deletions(-) create mode 100644 backend/internal/store/sqlite/migrations/005_canonical_membership_email.sql diff --git a/backend/internal/server/server.go b/backend/internal/server/server.go index c7a9bb8..2868d0a 100644 --- a/backend/internal/server/server.go +++ b/backend/internal/server/server.go @@ -12,6 +12,14 @@ import ( webui "github.com/nebari-dev/nebari-frames/web" ) +// MaxRequestBytes caps a single request body. It is deliberately larger than +// frames.MaxContentBytes so a publish at the content limit still fits with its +// protocol framing, while bounding what an authenticated caller can make the +// server buffer. Without it, a body is read in full before RBAC or any content +// limit is consulted, so a caller with no write permission at all could exhaust +// the memory of a deployment that is pinned to a single replica. +const MaxRequestBytes = 8 << 20 // 8 MiB + // Server wraps the combined HTTP mux that serves /healthz and the FrameService. type Server struct{ handler http.Handler } @@ -52,6 +60,7 @@ func New( path, handler := framesv1connect.NewFrameServiceHandler( svc, connect.WithInterceptors(interceptor), + connect.WithReadMaxBytes(MaxRequestBytes), ) mux.Handle(path, handler) if mcpMounter != nil { diff --git a/backend/internal/server/server_test.go b/backend/internal/server/server_test.go index f88dea7..8441b9e 100644 --- a/backend/internal/server/server_test.go +++ b/backend/internal/server/server_test.go @@ -1,6 +1,7 @@ package server_test import ( + "bytes" "context" "encoding/json" "io" @@ -273,3 +274,37 @@ func TestServer_Readyz(t *testing.T) { }) } } + +// An authenticated caller with no write permission can still make the server +// read a request body. Without a cap the whole thing is buffered before RBAC or +// any content limit is consulted, so a single request can exhaust the memory of +// a deployment that is pinned to one replica. +func TestConnectRejectsOversizedRequestBody(t *testing.T) { + srv := server.New(frames.NewService(store.NewMemory()), nil, auth.Config{}, branding.Config{}, true, nil) + ts := httptest.NewServer(srv.Handler()) + defer ts.Close() + + // Valid JSON, so a rejection cannot be mistaken for a parse failure: without + // the cap this body is read in full and the request proceeds to auth. + padding := strings.Repeat("a", server.MaxRequestBytes+1024) + body := []byte(`{"changelog":"` + padding + `"}`) + req, err := http.NewRequest(http.MethodPost, + ts.URL+"/frames.v1.FrameService/PublishFrame", bytes.NewReader(body)) + if err != nil { + t.Fatalf("request: %v", err) + } + req.Header.Set("Content-Type", "application/json") + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("post: %v", err) + } + defer func() { _ = resp.Body.Close() }() + + payload, _ := io.ReadAll(resp.Body) + // Connect reports an over-large message as resource_exhausted. Anything else + // (including a parse or auth error) means the body was read in full first. + if !strings.Contains(string(payload), "resource_exhausted") { + t.Fatalf("want resource_exhausted for an over-large body, got status %d body %s", + resp.StatusCode, string(payload)[:min(200, len(payload))]) + } +} diff --git a/backend/internal/store/memory.go b/backend/internal/store/memory.go index 550c8c4..758355b 100644 --- a/backend/internal/store/memory.go +++ b/backend/internal/store/memory.go @@ -82,6 +82,7 @@ func (m *Memory) GetMembership(_ context.Context, userSub string) (*framesv1.Mem func (m *Memory) UpsertMembership(_ context.Context, mem *framesv1.Membership) error { m.mu.Lock() defer m.mu.Unlock() + mem.Email = CanonicalEmail(mem.Email) for i, existing := range m.memberships { if existing.UserSub == mem.UserSub && mem.UserSub != "" { m.memberships[i] = mem @@ -249,7 +250,7 @@ func (m *Memory) AddPendingMembership(_ context.Context, mem *framesv1.Membershi m.memberships = append(m.memberships, &framesv1.Membership{ OrgId: mem.OrgId, Role: mem.Role, - Email: mem.Email, + Email: CanonicalEmail(mem.Email), AddedAt: mem.AddedAt, }) return nil diff --git a/backend/internal/store/sqlite/migrations/005_canonical_membership_email.sql b/backend/internal/store/sqlite/migrations/005_canonical_membership_email.sql new file mode 100644 index 0000000..6829569 --- /dev/null +++ b/backend/internal/store/sqlite/migrations/005_canonical_membership_email.sql @@ -0,0 +1,41 @@ +-- +goose Up +-- Canonicalize membership emails and make their uniqueness case-insensitive. +-- +-- Identity providers do not guarantee the case of the email claim and invites +-- are typed by hand, so the two routinely disagree. With a case-sensitive +-- unique index, two invites for the same person could coexist and which one a +-- login activated was arbitrary. + +-- Fold existing addresses to the canonical form first, so the new index can be +-- built. Rows written before this migration may carry stray case or whitespace. +UPDATE org_memberships + SET email = lower(trim(email)) + WHERE email IS NOT NULL AND email <> lower(trim(email)); + +-- Folding can collide: two rows in one org whose addresses differed only in +-- case are now identical. Keep the oldest (lowest rowid) and drop the rest, +-- preferring an activated membership over a pending invite so a real member is +-- never discarded in favour of an unclaimed invitation. +DELETE FROM org_memberships + WHERE email IS NOT NULL + AND rowid NOT IN ( + SELECT rowid FROM ( + SELECT rowid, + ROW_NUMBER() OVER ( + PARTITION BY org_id, email + ORDER BY CASE WHEN user_sub <> '' THEN 0 ELSE 1 END, rowid + ) AS rn + FROM org_memberships + WHERE email IS NOT NULL + ) + WHERE rn = 1 + ); + +DROP INDEX idx_membership_email; +CREATE UNIQUE INDEX idx_membership_email + ON org_memberships(org_id, email COLLATE NOCASE) WHERE email IS NOT NULL; + +-- +goose Down +DROP INDEX idx_membership_email; +CREATE UNIQUE INDEX idx_membership_email + ON org_memberships(org_id, email) WHERE email IS NOT NULL; diff --git a/backend/internal/store/sqlite/sqlite.go b/backend/internal/store/sqlite/sqlite.go index 113cf6c..5790a52 100644 --- a/backend/internal/store/sqlite/sqlite.go +++ b/backend/internal/store/sqlite/sqlite.go @@ -135,7 +135,7 @@ func (r *Repository) GetMembership(ctx context.Context, userSub string) (*frames func (r *Repository) UpsertMembership(ctx context.Context, m *framesv1.Membership) error { res, err := r.db.ExecContext(ctx, `UPDATE org_memberships SET org_id=?, role=?, email=? WHERE user_sub=? AND user_sub <> ''`, - m.OrgId, m.Role, nullStr(m.Email), m.UserSub) + m.OrgId, m.Role, nullStr(store.CanonicalEmail(m.Email)), m.UserSub) if err != nil { return err } @@ -144,7 +144,7 @@ func (r *Repository) UpsertMembership(ctx context.Context, m *framesv1.Membershi } _, err = r.db.ExecContext(ctx, `INSERT INTO org_memberships (org_id, user_sub, role, added_at, email) VALUES (?, ?, ?, ?, ?)`, - m.OrgId, m.UserSub, m.Role, m.AddedAt.AsTime().UTC().Format(time.RFC3339), nullStr(m.Email)) + m.OrgId, m.UserSub, m.Role, m.AddedAt.AsTime().UTC().Format(time.RFC3339), nullStr(store.CanonicalEmail(m.Email))) if isUnique(err) { return store.ErrAlreadyExists } @@ -193,7 +193,7 @@ func (r *Repository) GetPendingMembershipByEmail(ctx context.Context, email stri // COLLATE NOCASE: identity providers do not guarantee the case of the // email claim and invites are typed by hand, so a case difference must // not cause the invite to be missed. - `SELECT org_id, user_sub, role, added_at, email FROM org_memberships WHERE email = ? COLLATE NOCASE AND user_sub = '' LIMIT 1`, email). + `SELECT org_id, user_sub, role, added_at, email FROM org_memberships WHERE email = ? COLLATE NOCASE AND user_sub = '' LIMIT 1`, store.CanonicalEmail(email)). Scan(&m.OrgId, &m.UserSub, &m.Role, &added, &e) if errors.Is(err, sql.ErrNoRows) { return nil, store.ErrNotFound @@ -439,7 +439,7 @@ func (r *Repository) FrameGrants(ctx context.Context, frameID string) ([]store.G func (r *Repository) AddPendingMembership(ctx context.Context, m *framesv1.Membership) error { _, err := r.db.ExecContext(ctx, `INSERT INTO org_memberships (org_id, user_sub, role, added_at, email) VALUES (?, '', ?, ?, ?)`, - m.OrgId, m.Role, m.AddedAt.AsTime().UTC().Format(time.RFC3339), nullStr(m.Email)) + m.OrgId, m.Role, m.AddedAt.AsTime().UTC().Format(time.RFC3339), nullStr(store.CanonicalEmail(m.Email))) if isUnique(err) { return store.ErrAlreadyExists } @@ -458,7 +458,7 @@ func (r *Repository) ActivatePendingMembership(ctx context.Context, email, sub s SELECT rowid FROM org_memberships WHERE email = ? AND user_sub = '' ORDER BY rowid LIMIT 1 - )`, sub, email) + )`, sub, store.CanonicalEmail(email)) if err != nil { return err } @@ -476,7 +476,7 @@ func (r *Repository) UpdateMembershipRole(ctx context.Context, orgID, userSub, e `UPDATE org_memberships SET role = ? WHERE org_id = ? AND user_sub = ?`, role, orgID, userSub) } else { res, err = r.db.ExecContext(ctx, - `UPDATE org_memberships SET role = ? WHERE org_id = ? AND email = ? AND user_sub = ''`, role, orgID, email) + `UPDATE org_memberships SET role = ? WHERE org_id = ? AND email = ? AND user_sub = ''`, role, orgID, store.CanonicalEmail(email)) } if err != nil { return err @@ -495,7 +495,7 @@ func (r *Repository) DeleteMembership(ctx context.Context, orgID, userSub, email `DELETE FROM org_memberships WHERE org_id = ? AND user_sub = ?`, orgID, userSub) } else { res, err = r.db.ExecContext(ctx, - `DELETE FROM org_memberships WHERE org_id = ? AND email = ? AND user_sub = ''`, orgID, email) + `DELETE FROM org_memberships WHERE org_id = ? AND email = ? AND user_sub = ''`, orgID, store.CanonicalEmail(email)) } if err != nil { return err diff --git a/backend/internal/store/sqlite/sqlite_test.go b/backend/internal/store/sqlite/sqlite_test.go index 2a89a32..915de8c 100644 --- a/backend/internal/store/sqlite/sqlite_test.go +++ b/backend/internal/store/sqlite/sqlite_test.go @@ -698,40 +698,95 @@ func TestSQLite_PublishAtomicRollback(t *testing.T) { } } -// Activating an invite must touch exactly one row. Two pending invites differing -// only in case can coexist (see #65), and a case-insensitive UPDATE with no row -// scoping would try to give both the same user_sub, violating the unique index -// on user_sub and rolling the whole statement back - locking the user out with -// an internal error instead of activating their invite. +// Activating an invite must touch exactly one row. Emails are unique per org, +// not globally, so the same person can hold a pending invite in two orgs - and +// the activation UPDATE matches on email alone. Without row scoping it would +// try to give both rows the same user_sub, violating the unique index on +// user_sub and rolling the statement back, which locks the user out with an +// internal error instead of activating their invite. func TestActivatePendingMembershipTouchesOneRow(t *testing.T) { r := newRepo(t) ctx := context.Background() seedOrg(t, r, "o1", "acme") + seedOrg(t, r, "o2", "globex") now := timestamppb.Now() - if err := r.AddPendingMembership(ctx, &framesv1.Membership{OrgId: "o1", Role: "viewer", Email: "boss@x.io", AddedAt: now}); err != nil { - t.Fatalf("first invite: %v", err) - } - // Permitted today because the unique index on (org_id, email) is - // case-sensitive; tracked in #65. - if err := r.AddPendingMembership(ctx, &framesv1.Membership{OrgId: "o1", Role: "publisher", Email: "Boss@X.io", AddedAt: now}); err != nil { - t.Fatalf("second invite: %v", err) + for _, org := range []string{"o1", "o2"} { + if err := r.AddPendingMembership(ctx, &framesv1.Membership{ + OrgId: org, Role: "viewer", Email: "boss@x.io", AddedAt: now, + }); err != nil { + t.Fatalf("invite in %s: %v", org, err) + } } if err := r.ActivatePendingMembership(ctx, "boss@x.io", "s1"); err != nil { t.Fatalf("activate: %v", err) } - members, err := r.ListMembershipsByOrg(ctx, "o1") - if err != nil { - t.Fatalf("list: %v", err) - } activated := 0 - for _, m := range members { - if m.UserSub == "s1" { - activated++ + for _, org := range []string{"o1", "o2"} { + members, err := r.ListMembershipsByOrg(ctx, org) + if err != nil { + t.Fatalf("list %s: %v", org, err) + } + for _, m := range members { + if m.UserSub == "s1" { + activated++ + } } } if activated != 1 { - t.Errorf("activated %d rows, want exactly 1: %+v", activated, members) + t.Errorf("activated %d rows, want exactly 1", activated) } } + +// Emails are canonical at rest, and uniqueness ignores case: two invites for +// the same person cannot coexist, so which one a login activates is never +// arbitrary. +func TestMembershipEmailsAreCanonical(t *testing.T) { + ctx := context.Background() + now := timestamppb.Now() + + t.Run("a case variant invite conflicts with an existing one", func(t *testing.T) { + r := newRepo(t) + seedOrg(t, r, "o1", "acme") + if err := r.AddPendingMembership(ctx, &framesv1.Membership{OrgId: "o1", Role: "viewer", Email: "boss@x.io", AddedAt: now}); err != nil { + t.Fatalf("first invite: %v", err) + } + for _, variant := range []string{"Boss@X.io", "BOSS@X.IO", " boss@x.io "} { + err := r.AddPendingMembership(ctx, &framesv1.Membership{OrgId: "o1", Role: "admin", Email: variant, AddedAt: now}) + if !errors.Is(err, store.ErrAlreadyExists) { + t.Errorf("AddPendingMembership(%q) err = %v, want ErrAlreadyExists", variant, err) + } + } + }) + + t.Run("addresses are stored folded and trimmed", func(t *testing.T) { + r := newRepo(t) + seedOrg(t, r, "o1", "acme") + if err := r.AddPendingMembership(ctx, &framesv1.Membership{OrgId: "o1", Role: "viewer", Email: " Mixed@Case.IO ", AddedAt: now}); err != nil { + t.Fatalf("invite: %v", err) + } + members, err := r.ListMembershipsByOrg(ctx, "o1") + if err != nil { + t.Fatalf("list: %v", err) + } + if len(members) != 1 || members[0].Email != "mixed@case.io" { + t.Errorf("stored email = %q, want %q", members[0].Email, "mixed@case.io") + } + }) + + t.Run("an active membership is stored folded too", func(t *testing.T) { + r := newRepo(t) + seedOrg(t, r, "o1", "acme") + if err := r.UpsertMembership(ctx, &framesv1.Membership{OrgId: "o1", UserSub: "s1", Role: "viewer", Email: "Upper@Case.IO", AddedAt: now}); err != nil { + t.Fatalf("upsert: %v", err) + } + m, err := r.GetMembership(ctx, "s1") + if err != nil { + t.Fatalf("get: %v", err) + } + if m.Email != "upper@case.io" { + t.Errorf("stored email = %q, want folded", m.Email) + } + }) +} From 75c22cb9596664c69cf382471db0199d053ad26a Mon Sep 17 00:00:00 2001 From: Chuck McAndrew <6248903+dcmcand@users.noreply.github.com> Date: Fri, 21 Aug 2026 09:23:40 -0700 Subject: [PATCH 6/8] fix: reorder the email migration and make baseline provisioning insert-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. --- backend/internal/orgs/orgs.go | 21 +++-- backend/internal/orgs/provision_race_test.go | 80 ++++++++++++++++++ backend/internal/store/memory.go | 22 +++++ .../005_canonical_membership_email.sql | 27 +++--- .../sqlite/migrations/migrate_legacy_test.go | 82 +++++++++++++++++++ backend/internal/store/sqlite/sqlite.go | 12 +++ backend/internal/store/store.go | 6 ++ 7 files changed, 230 insertions(+), 20 deletions(-) create mode 100644 backend/internal/orgs/provision_race_test.go create mode 100644 backend/internal/store/sqlite/migrations/migrate_legacy_test.go diff --git a/backend/internal/orgs/orgs.go b/backend/internal/orgs/orgs.go index 6423471..c66e61d 100644 --- a/backend/internal/orgs/orgs.go +++ b/backend/internal/orgs/orgs.go @@ -126,18 +126,21 @@ func provisionDefault(ctx context.Context, repo store.Repository, claims *auth.C Email: store.CanonicalEmail(claims.Email), AddedAt: timestamppb.Now(), } - if err := repo.UpsertMembership(ctx, m); err != nil { + // Insert-only. UpsertMembership is UPDATE-first, so using it here would let + // a caller acting on a stale "no membership" read rewrite a row that another + // request established in the meantime - turning an activated invite's role + // into the baseline, permanently. + if err := repo.CreateMembership(ctx, m); err != nil { if !errors.Is(err, store.ErrAlreadyExists) { return nil, err } - // Two ways to land here. Either a concurrent first request won the - // insert, in which case re-reading finds the winner's row and it is - // authoritative - not our assumed role. Or the store already holds a - // membership for this email under a different subject, which is what a - // deleted-and-recreated identity-provider account looks like: the store - // keeps one membership per (org, email), so the stale row blocks the new - // one. That is a denial, not a server fault, so it must not escape as a - // raw store error and become a 500. + // Something else holds this slot. Either a concurrent request already + // wrote a membership for this subject - possibly by activating an invite, + // so its role outranks the baseline we assumed - or the (org, email) pair + // belongs to a different subject, which is what a deleted-and-recreated + // identity-provider account looks like. Re-reading distinguishes them: + // a row for our subject is authoritative, and no row means the address is + // taken, which is a denial rather than a server fault. existing, rerr := repo.GetMembership(ctx, claims.Subject) if rerr == nil { return existing, nil diff --git a/backend/internal/orgs/provision_race_test.go b/backend/internal/orgs/provision_race_test.go new file mode 100644 index 0000000..67798b0 --- /dev/null +++ b/backend/internal/orgs/provision_race_test.go @@ -0,0 +1,80 @@ +package orgs_test + +import ( + "context" + "testing" + + "google.golang.org/protobuf/types/known/timestamppb" + + "github.com/nebari-dev/nebari-frames/backend/internal/auth" + "github.com/nebari-dev/nebari-frames/backend/internal/orgs" + "github.com/nebari-dev/nebari-frames/backend/internal/rbac" + "github.com/nebari-dev/nebari-frames/backend/internal/store" + sqlitestore "github.com/nebari-dev/nebari-frames/backend/internal/store/sqlite" + "github.com/nebari-dev/nebari-frames/backend/internal/store/sqlite/migrations" + framesv1 "github.com/nebari-dev/nebari-frames/gen/go/frames/v1" +) + +// staleRepo makes only the FIRST membership read stale, reproducing the +// interleaving where a caller reads "no membership" just before a concurrent +// request activates the pending invite. Later reads see the truth, as they would +// in a real process. +type staleRepo struct { + store.Repository + staleReads int +} + +func (r *staleRepo) GetMembership(ctx context.Context, sub string) (*framesv1.Membership, error) { + if r.staleReads > 0 { + r.staleReads-- + return nil, store.ErrNotFound + } + return r.Repository.GetMembership(ctx, sub) +} + +// Provisioning the baseline must never lower a role another request has already +// established. The membership read and the write are not atomic, so a caller can +// arrive here holding a stale "no membership" answer; an UPDATE-first write would +// then rewrite the activated invite's role to the baseline, permanently. If the +// invite was the seed.adminEmail bootstrap, the org loses its only admin. +func TestResolveCallerDoesNotDowngradeAConcurrentlyActivatedInvite(t *testing.T) { + ctx := context.Background() + db, _ := sqlitestore.Open(t.TempDir() + "/t.db") + t.Cleanup(func() { _ = db.Close() }) + if err := migrations.Run(ctx, db); err != nil { + t.Fatalf("migrate: %v", err) + } + real := sqlitestore.New(db) + _ = real.CreateOrg(ctx, &framesv1.Org{Id: "o1", Slug: "acme", CreatedAt: timestamppb.Now()}) + _ = real.AddPendingMembership(ctx, &framesv1.Membership{OrgId: "o1", Role: "publisher", Email: "boss@x.io", AddedAt: timestamppb.Now()}) + + // R1 activates the invite: boss becomes publisher. + c1, err := orgs.ResolveCaller(auth.WithClaims(ctx, &auth.Claims{Subject: "boss-sub", Email: "boss@x.io"}), real, + orgs.DefaultMembership{Role: rbac.RoleViewer, OrgSlug: "acme"}) + if err != nil { + t.Fatalf("first request: %v", err) + } + if c1.Role != rbac.RolePublisher { + t.Fatalf("first request role = %q, want publisher from the invite", c1.Role) + } + + // R2 read no membership before R1's activation landed, and its pending + // lookup now finds nothing, so it falls through to provisioning. + stale := &staleRepo{Repository: real, staleReads: 1} + c2, err := orgs.ResolveCaller(auth.WithClaims(ctx, &auth.Claims{Subject: "boss-sub", Email: "boss@x.io"}), stale, + orgs.DefaultMembership{Role: rbac.RoleViewer, OrgSlug: "acme"}) + if err != nil { + t.Fatalf("second request: %v", err) + } + if c2.Role != rbac.RolePublisher { + t.Errorf("second request role = %q, want publisher: it must report the stored row, not its assumed baseline", c2.Role) + } + + after, err := real.GetMembership(ctx, "boss-sub") + if err != nil { + t.Fatalf("read back: %v", err) + } + if after.Role != "publisher" { + t.Errorf("stored role = %q, want publisher: the baseline write overwrote an established role", after.Role) + } +} diff --git a/backend/internal/store/memory.go b/backend/internal/store/memory.go index 758355b..270fd0e 100644 --- a/backend/internal/store/memory.go +++ b/backend/internal/store/memory.go @@ -93,6 +93,28 @@ func (m *Memory) UpsertMembership(_ context.Context, mem *framesv1.Membership) e return nil } +func (m *Memory) CreateMembership(_ context.Context, mem *framesv1.Membership) error { + m.mu.Lock() + defer m.mu.Unlock() + email := CanonicalEmail(mem.Email) + for _, existing := range m.memberships { + if existing.UserSub == mem.UserSub && mem.UserSub != "" { + return ErrAlreadyExists + } + if email != "" && existing.OrgId == mem.OrgId && CanonicalEmail(existing.Email) == email { + return ErrAlreadyExists + } + } + m.memberships = append(m.memberships, &framesv1.Membership{ + OrgId: mem.OrgId, + UserSub: mem.UserSub, + Role: mem.Role, + Email: email, + AddedAt: mem.AddedAt, + }) + return nil +} + func (m *Memory) ListMembershipsByOrg(_ context.Context, orgID string) ([]*framesv1.Membership, error) { m.mu.RLock() defer m.mu.RUnlock() diff --git a/backend/internal/store/sqlite/migrations/005_canonical_membership_email.sql b/backend/internal/store/sqlite/migrations/005_canonical_membership_email.sql index 6829569..4191f40 100644 --- a/backend/internal/store/sqlite/migrations/005_canonical_membership_email.sql +++ b/backend/internal/store/sqlite/migrations/005_canonical_membership_email.sql @@ -5,24 +5,25 @@ -- are typed by hand, so the two routinely disagree. With a case-sensitive -- unique index, two invites for the same person could coexist and which one a -- login activated was arbitrary. +-- +-- Statement order matters. The old index is case-sensitive, so folding +-- addresses while it is still in place fails the moment two rows in one org +-- differ only in case - which is precisely the data this migration exists to +-- repair. Drop the index first, collapse the duplicates on the *folded* key, +-- and only then fold the surviving rows. --- Fold existing addresses to the canonical form first, so the new index can be --- built. Rows written before this migration may carry stray case or whitespace. -UPDATE org_memberships - SET email = lower(trim(email)) - WHERE email IS NOT NULL AND email <> lower(trim(email)); +DROP INDEX idx_membership_email; --- Folding can collide: two rows in one org whose addresses differed only in --- case are now identical. Keep the oldest (lowest rowid) and drop the rest, --- preferring an activated membership over a pending invite so a real member is --- never discarded in favour of an unclaimed invitation. +-- Collapse rows that are about to become identical. Keeping the oldest (lowest +-- rowid), but preferring an activated membership over a pending invite, so a +-- real member is never discarded in favour of an unclaimed invitation. DELETE FROM org_memberships WHERE email IS NOT NULL AND rowid NOT IN ( SELECT rowid FROM ( SELECT rowid, ROW_NUMBER() OVER ( - PARTITION BY org_id, email + PARTITION BY org_id, lower(trim(email)) ORDER BY CASE WHEN user_sub <> '' THEN 0 ELSE 1 END, rowid ) AS rn FROM org_memberships @@ -31,11 +32,15 @@ DELETE FROM org_memberships WHERE rn = 1 ); -DROP INDEX idx_membership_email; +UPDATE org_memberships + SET email = lower(trim(email)) + WHERE email IS NOT NULL AND email <> lower(trim(email)); + CREATE UNIQUE INDEX idx_membership_email ON org_memberships(org_id, email COLLATE NOCASE) WHERE email IS NOT NULL; -- +goose Down +-- Folded addresses cannot be restored; only the index shape is reversible. DROP INDEX idx_membership_email; CREATE UNIQUE INDEX idx_membership_email ON org_memberships(org_id, email) WHERE email IS NOT NULL; diff --git a/backend/internal/store/sqlite/migrations/migrate_legacy_test.go b/backend/internal/store/sqlite/migrations/migrate_legacy_test.go new file mode 100644 index 0000000..fc88251 --- /dev/null +++ b/backend/internal/store/sqlite/migrations/migrate_legacy_test.go @@ -0,0 +1,82 @@ +package migrations_test + +import ( + "context" + "database/sql" + "testing" + + _ "modernc.org/sqlite" + + "github.com/nebari-dev/nebari-frames/backend/internal/store/sqlite/migrations" +) + +// Migrations must survive the data they exist to repair. The suite otherwise +// only ever runs them against a fresh database, where nothing needs repairing - +// so this builds the schema as of migration 004, seeds the case-variant invites +// that the case-sensitive unique index used to permit, and migrates forward. A +// failure here is a crash-looping pod: migrations.Run's error reaches main, +// which exits. +func TestRunOnCaseVariantLegacyData(t *testing.T) { + ctx := context.Background() + db, err := sql.Open("sqlite", t.TempDir()+"/legacy.db") + if err != nil { + t.Fatalf("open: %v", err) + } + defer func() { _ = db.Close() }() + + stmts := []string{ + `CREATE TABLE goose_db_version (id INTEGER PRIMARY KEY AUTOINCREMENT, version_id INTEGER NOT NULL, is_applied INTEGER NOT NULL, tstamp TIMESTAMP DEFAULT (datetime('now')))`, + `INSERT INTO goose_db_version (version_id, is_applied) VALUES (0,1),(1,1),(2,1),(3,1),(4,1)`, + `CREATE TABLE orgs (id TEXT PRIMARY KEY, slug TEXT NOT NULL UNIQUE, display_name TEXT NOT NULL, created_at TEXT NOT NULL)`, + `CREATE TABLE org_memberships (org_id TEXT NOT NULL REFERENCES orgs(id), user_sub TEXT NOT NULL DEFAULT '', email TEXT, role TEXT NOT NULL, added_at TEXT NOT NULL)`, + `CREATE UNIQUE INDEX idx_membership_sub ON org_memberships(user_sub) WHERE user_sub <> ''`, + `CREATE UNIQUE INDEX idx_membership_email ON org_memberships(org_id, email) WHERE email IS NOT NULL`, + `INSERT INTO orgs VALUES ('o1','acme','Acme','2026-01-01T00:00:00Z')`, + `INSERT INTO org_memberships (org_id,user_sub,email,role,added_at) VALUES ('o1','','boss@x.io','viewer','2026-01-01T00:00:00Z')`, + `INSERT INTO org_memberships (org_id,user_sub,email,role,added_at) VALUES ('o1','s2','Boss@X.io','admin','2026-01-02T00:00:00Z')`, + } + for _, q := range stmts { + if _, err := db.ExecContext(ctx, q); err != nil { + t.Fatalf("setup %q: %v", q[:40], err) + } + } + + if err := migrations.Run(ctx, db); err != nil { + t.Fatalf("migrations must not fail on pre-existing case-variant rows: %v", err) + } + + rows, err := db.QueryContext(ctx, `SELECT user_sub, email, role FROM org_memberships ORDER BY rowid`) + if err != nil { + t.Fatalf("query: %v", err) + } + defer func() { _ = rows.Close() }() + type row struct{ sub, email, role string } + var got []row + for rows.Next() { + var r row + if err := rows.Scan(&r.sub, &r.email, &r.role); err != nil { + t.Fatalf("scan: %v", err) + } + got = append(got, r) + } + if err := rows.Err(); err != nil { + t.Fatalf("rows: %v", err) + } + + // One row survives per (org, folded address). The activated membership is + // kept over the pending invite, so the real member is not replaced by an + // invitation that nobody has claimed. + if len(got) != 1 { + t.Fatalf("got %d rows, want 1: %+v", len(got), got) + } + if got[0].sub != "s2" || got[0].email != "boss@x.io" || got[0].role != "admin" { + t.Errorf("surviving row = %+v, want the activated admin membership with a folded address", got[0]) + } + + // The new index must reject a case variant rather than storing both. + if _, err := db.ExecContext(ctx, + `INSERT INTO org_memberships (org_id,user_sub,email,role,added_at) VALUES ('o1','s3','BOSS@X.IO','viewer','2026-01-03T00:00:00Z')`, + ); err == nil { + t.Error("a case-variant address was accepted; the index is not case-insensitive") + } +} diff --git a/backend/internal/store/sqlite/sqlite.go b/backend/internal/store/sqlite/sqlite.go index 5790a52..0aac913 100644 --- a/backend/internal/store/sqlite/sqlite.go +++ b/backend/internal/store/sqlite/sqlite.go @@ -151,6 +151,18 @@ func (r *Repository) UpsertMembership(ctx context.Context, m *framesv1.Membershi return err } +// CreateMembership inserts a membership, never updating an existing row. Both +// unique indexes (user_sub, and org_id+email) surface as ErrAlreadyExists. +func (r *Repository) CreateMembership(ctx context.Context, m *framesv1.Membership) error { + _, err := r.db.ExecContext(ctx, + `INSERT INTO org_memberships (org_id, user_sub, role, added_at, email) VALUES (?, ?, ?, ?, ?)`, + m.OrgId, m.UserSub, m.Role, m.AddedAt.AsTime().UTC().Format(time.RFC3339), nullStr(store.CanonicalEmail(m.Email))) + if isUnique(err) { + return store.ErrAlreadyExists + } + return err +} + // nullStr maps "" to SQL NULL so the partial unique email index ignores it. func nullStr(s string) any { if s == "" { diff --git a/backend/internal/store/store.go b/backend/internal/store/store.go index 2a951bf..275ab1a 100644 --- a/backend/internal/store/store.go +++ b/backend/internal/store/store.go @@ -61,6 +61,12 @@ type Repository interface { GetOrgBySlug(ctx context.Context, slug string) (*framesv1.Org, error) GetMembership(ctx context.Context, userSub string) (*framesv1.Membership, error) UpsertMembership(ctx context.Context, m *framesv1.Membership) error + // CreateMembership inserts a membership and never updates one. It returns + // ErrAlreadyExists when the subject or the (org, email) pair is taken. + // Provisioning must not use UpsertMembership: that is UPDATE-first, so a + // caller acting on a stale "no membership" read would rewrite a role another + // request had just established. + CreateMembership(ctx context.Context, m *framesv1.Membership) error ListMembershipsByOrg(ctx context.Context, orgID string) ([]*framesv1.Membership, error) GetPendingMembershipByEmail(ctx context.Context, email string) (*framesv1.Membership, error) CountAdmins(ctx context.Context, orgID string) (int, error) From 93710b5691d8ac6df45111e6344f8920bc30090a Mon Sep 17 00:00:00 2001 From: Chuck McAndrew <6248903+dcmcand@users.noreply.github.com> Date: Fri, 21 Aug 2026 09:46:45 -0700 Subject: [PATCH 7/8] fix(migration): keep the newest pending invite when addresses collide 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. --- .../005_canonical_membership_email.sql | 15 +++++--- .../sqlite/migrations/migrate_legacy_test.go | 34 +++++++++++++++---- 2 files changed, 38 insertions(+), 11 deletions(-) diff --git a/backend/internal/store/sqlite/migrations/005_canonical_membership_email.sql b/backend/internal/store/sqlite/migrations/005_canonical_membership_email.sql index 4191f40..3118300 100644 --- a/backend/internal/store/sqlite/migrations/005_canonical_membership_email.sql +++ b/backend/internal/store/sqlite/migrations/005_canonical_membership_email.sql @@ -14,9 +14,14 @@ DROP INDEX idx_membership_email; --- Collapse rows that are about to become identical. Keeping the oldest (lowest --- rowid), but preferring an activated membership over a pending invite, so a --- real member is never discarded in favour of an unclaimed invitation. +-- Collapse rows that are about to become identical. +-- +-- An activated membership always outranks a pending invite, so a real member is +-- never discarded in favour of an unclaimed invitation. Among activated rows the +-- oldest wins: it is the longer-standing membership. Among pending invites the +-- newest wins, because a second invite for the same person is the admin saying +-- what they want now - keeping the older one would silently reinstate a role +-- they had already replaced. DELETE FROM org_memberships WHERE email IS NOT NULL AND rowid NOT IN ( @@ -24,7 +29,9 @@ DELETE FROM org_memberships SELECT rowid, ROW_NUMBER() OVER ( PARTITION BY org_id, lower(trim(email)) - ORDER BY CASE WHEN user_sub <> '' THEN 0 ELSE 1 END, rowid + ORDER BY + CASE WHEN user_sub <> '' THEN 0 ELSE 1 END, + CASE WHEN user_sub <> '' THEN rowid ELSE -rowid END ) AS rn FROM org_memberships WHERE email IS NOT NULL diff --git a/backend/internal/store/sqlite/migrations/migrate_legacy_test.go b/backend/internal/store/sqlite/migrations/migrate_legacy_test.go index fc88251..5d6aeac 100644 --- a/backend/internal/store/sqlite/migrations/migrate_legacy_test.go +++ b/backend/internal/store/sqlite/migrations/migrate_legacy_test.go @@ -32,8 +32,14 @@ func TestRunOnCaseVariantLegacyData(t *testing.T) { `CREATE UNIQUE INDEX idx_membership_sub ON org_memberships(user_sub) WHERE user_sub <> ''`, `CREATE UNIQUE INDEX idx_membership_email ON org_memberships(org_id, email) WHERE email IS NOT NULL`, `INSERT INTO orgs VALUES ('o1','acme','Acme','2026-01-01T00:00:00Z')`, + // An activated membership alongside a pending invite for the same person. `INSERT INTO org_memberships (org_id,user_sub,email,role,added_at) VALUES ('o1','','boss@x.io','viewer','2026-01-01T00:00:00Z')`, `INSERT INTO org_memberships (org_id,user_sub,email,role,added_at) VALUES ('o1','s2','Boss@X.io','admin','2026-01-02T00:00:00Z')`, + // Two pending invites for the same person, the later one at a different role. + `INSERT INTO org_memberships (org_id,user_sub,email,role,added_at) VALUES ('o1','','carol@x.io','viewer','2026-01-03T00:00:00Z')`, + `INSERT INTO org_memberships (org_id,user_sub,email,role,added_at) VALUES ('o1','','CAROL@X.IO','publisher','2026-01-04T00:00:00Z')`, + // Stray whitespace, no collision. + `INSERT INTO org_memberships (org_id,user_sub,email,role,added_at) VALUES ('o1','',' dave@x.io ','viewer','2026-01-05T00:00:00Z')`, } for _, q := range stmts { if _, err := db.ExecContext(ctx, q); err != nil { @@ -63,14 +69,28 @@ func TestRunOnCaseVariantLegacyData(t *testing.T) { t.Fatalf("rows: %v", err) } - // One row survives per (org, folded address). The activated membership is - // kept over the pending invite, so the real member is not replaced by an - // invitation that nobody has claimed. - if len(got) != 1 { - t.Fatalf("got %d rows, want 1: %+v", len(got), got) + // One row survives per (org, folded address), and every address is folded. + if len(got) != 3 { + t.Fatalf("got %d rows, want 3: %+v", len(got), got) } - if got[0].sub != "s2" || got[0].email != "boss@x.io" || got[0].role != "admin" { - t.Errorf("surviving row = %+v, want the activated admin membership with a folded address", got[0]) + by := map[string]row{} + for _, r := range got { + by[r.email] = r + } + + // An activated membership outranks a pending invite: the real member is not + // replaced by an invitation nobody has claimed. + if b := by["boss@x.io"]; b.sub != "s2" || b.role != "admin" { + t.Errorf("boss = %+v, want the activated admin membership", b) + } + // Between two pending invites the later one wins: it is what the admin most + // recently asked for, and keeping the earlier would reinstate a role they had + // already replaced. + if c := by["carol@x.io"]; c.sub != "" || c.role != "publisher" { + t.Errorf("carol = %+v, want the later pending invite (publisher)", c) + } + if d := by["dave@x.io"]; d.role != "viewer" { + t.Errorf("dave = %+v, want the trimmed address to survive", d) } // The new index must reject a case variant rather than storing both. From 3ed72505f94df787d27331ef08b458758f551c47 Mon Sep 17 00:00:00 2001 From: Chuck McAndrew <6248903+dcmcand@users.noreply.github.com> Date: Fri, 21 Aug 2026 09:58:57 -0700 Subject: [PATCH 8/8] docs: record the default-role behaviour in AGENTS.md 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. --- AGENTS.md | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index f32f810..b85bb8c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -61,10 +61,19 @@ store.Repository**. the `@gen` alias). All three surfaces (web, CLI, MCP) speak the same RPCs. - `backend/internal/server` wires the mux: unauthenticated `/healthz`, `/readyz`, `/auth/config`, `/config.json` (runtime branding); the FrameService at its generated path behind - `auth.NewInterceptor`; optionally the MCP component; and the embedded SPA at `/`. + `auth.NewInterceptor`; optionally the MCP component; and the embedded SPA at `/`. The + FrameService is injected rather than constructed here, so the Connect and MCP endpoints share one + instance and cannot be configured differently. Request bodies are capped at + `server.MaxRequestBytes`, because a body is read in full before RBAC is consulted. - `backend/internal/auth` resolves identity. `FRAMES_DEV_MODE=true` short-circuits everything with a fixed `dev-user` identity and a permanently-ready `/readyz`. Otherwise a lazy OIDC validator performs discovery from inside the pod, and `/readyz` stays 503 until it succeeds (fail closed). +- `backend/internal/orgs` turns claims into an `rbac.Caller`. Precedence is fixed: a stored + membership, then a pending invite matched by email, then the baseline role from + `FRAMES_DEFAULT_ROLE` (empty means deny, which is the fail-closed default). The baseline + membership is persisted, so those users appear in the members list and are promotable - and it is + written insert-only, because the "no membership" read that leads there is not atomic with the + write and an update would overwrite a role another request had just established. - `backend/internal/rbac` is the single authorization decision point. `Can` evaluates in order: cross-org deny, admin allow, then per-frame grants for the user or the org. Roles are `viewer | publisher | admin`, permissions `read | edit | delete`. A missing read permission is @@ -82,7 +91,10 @@ store.Repository**. slots win last, cycles produce `CycleError`, and an unreadable ancestor propagates `ErrParentUnreadable` rather than silently dropping content. - `backend/internal/store` defines `Repository` plus an in-memory implementation used by tests; - `store/sqlite` is the real one, with goose migrations in `store/sqlite/migrations`. Publishes go + `store/sqlite` is the real one, with goose migrations in `store/sqlite/migrations`. Emails are + canonical at rest (`store.CanonicalEmail`) and unique per org case-insensitively, so an invite + cannot be shadowed by a case variant. The in-memory fake enforces the same unique constraints; + where it cannot, tests reach for real SQLite and say why. Publishes go through `CreateFrameVersion`, which inserts the frame row, version, inheritance edges, and grants atomically. **SQLite is single-writer, so the deployment is pinned to one replica.** - `backend/internal/mcp` is a thin protocol adapter over `frames.Service`, exposing frames as MCP @@ -103,6 +115,10 @@ store.Repository**. ## Conventions - Table-driven Go tests. Tests sit beside the code they cover, including in `web/` (`*.test.tsx`). +- Migrations are only ever applied to a fresh database by the normal suite, which hides anything that + breaks on existing rows. `store/sqlite/migrations/migrate_legacy_test.go` builds an older schema + with the data a migration has to repair and migrates forward; extend it when a migration touches + existing rows. - Never bump `chart/Chart.yaml`'s `version`/`appVersion` by hand: the release job stamps them from the git tag (`version` = tag without `v`, `appVersion` = the literal tag). - The SPA ships inside the image, not the chart. A frontend change reaches a cluster only through a