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 diff --git a/README.md b/README.md index ff79f0c..e2f7b28 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,49 @@ 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 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 - + 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. + ## 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..c66e61d 100644 --- a/backend/internal/orgs/orgs.go +++ b/backend/internal/orgs/orgs.go @@ -4,10 +4,14 @@ package orgs import ( "context" "errors" + "log/slog" + + "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 +19,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 +68,89 @@ 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 := store.CanonicalEmail(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), + // 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(), + } + // 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 + } + // 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 + } + 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..c07ad24 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,381 @@ 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 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") + 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)) + } +} + +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/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/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..f84f731 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,45 @@ 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 + } + // 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 + } + 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 { + // 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 d5256b2..67bc851 100644 --- a/backend/internal/seed/seed_test.go +++ b/backend/internal/seed/seed_test.go @@ -176,3 +176,144 @@ 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) + } + }) + } +} + +// 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/server/server.go b/backend/internal/server/server.go index af13711..2868d0a 100644 --- a/backend/internal/server/server.go +++ b/backend/internal/server/server.go @@ -8,12 +8,18 @@ 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" ) +// 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 } @@ -30,8 +36,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,8 +58,9 @@ func New( mux.HandleFunc("/config.json", handleBranding(brandingCfg)) interceptor := auth.NewInterceptor(validator, devMode) path, handler := framesv1connect.NewFrameServiceHandler( - frames.NewService(repo), + 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 4343314..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" @@ -11,6 +12,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 +40,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 +86,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 +123,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 +180,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 +209,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 +260,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") @@ -272,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 046aa04..270fd0e 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 @@ -92,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() @@ -108,7 +131,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 == "" && CanonicalEmail(mem.Email) == CanonicalEmail(email) { return mem, nil } } @@ -239,14 +263,16 @@ 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 } } 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 @@ -256,7 +282,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 == "" && CanonicalEmail(e.Email) == CanonicalEmail(email) { e.UserSub = sub 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..3118300 --- /dev/null +++ b/backend/internal/store/sqlite/migrations/005_canonical_membership_email.sql @@ -0,0 +1,53 @@ +-- +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. +-- +-- 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. + +DROP INDEX idx_membership_email; + +-- 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 ( + SELECT rowid FROM ( + SELECT rowid, + ROW_NUMBER() OVER ( + PARTITION BY org_id, lower(trim(email)) + 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 + ) + WHERE rn = 1 + ); + +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..5d6aeac --- /dev/null +++ b/backend/internal/store/sqlite/migrations/migrate_legacy_test.go @@ -0,0 +1,102 @@ +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')`, + // 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 { + 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), and every address is folded. + if len(got) != 3 { + t.Fatalf("got %d rows, want 3: %+v", len(got), got) + } + 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. + 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 9e7c010..0aac913 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,19 @@ 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 + } + 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 } @@ -190,7 +202,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`, store.CanonicalEmail(email)). Scan(&m.OrgId, &m.UserSub, &m.Role, &added, &e) if errors.Is(err, sql.ErrNoRows) { return nil, store.ErrNotFound @@ -436,7 +451,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 } @@ -444,8 +459,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 = ? 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, store.CanonicalEmail(email)) if err != nil { return err } @@ -463,7 +488,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 @@ -482,7 +507,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 bbac0d4..915de8c 100644 --- a/backend/internal/store/sqlite/sqlite_test.go +++ b/backend/internal/store/sqlite/sqlite_test.go @@ -697,3 +697,96 @@ func TestSQLite_PublishAtomicRollback(t *testing.T) { }) } } + +// 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() + + 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) + } + activated := 0 + 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", 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) + } + }) +} diff --git a/backend/internal/store/store.go b/backend/internal/store/store.go index 15e70e6..275ab1a 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" @@ -48,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) 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: "" diff --git a/dev/keycloak/frames-realm.json b/dev/keycloak/frames-realm.json index 808bda7..b84f133 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/*" @@ -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 } + ] } ] }