From 3fb5ff809b36ab8597a52a3301a7390af3922433 Mon Sep 17 00:00:00 2001 From: FlorianJeandenans Date: Fri, 7 Aug 2026 15:50:11 +0200 Subject: [PATCH] feat(userauth,dashboard): accept ADR-016 and implement its slice 1 (schema + grant path) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Accepts docs/control-plane/dashboard-rbac-and-tenant-isolation-proposal.md as ADR-016 (moved to docs/adr/016-dashboard-rbac-and-tenant-isolation.md, Status: Accepted) and implements its first sequencing slice: the role schema, the grant path, and the authorization middleware -- with zero routes wrapped in it yet, exactly as the ADR's own sequencing specifies. Slice 2 (tenant workload views) still needs ADR-016 §7 questions 1 and 2 answered before it can ship. Accepting ADR-016 claimed ADR-016 from ADR-012 §6's gate-reservation table (previously "replicated off-chain data plane," #33). Rather than cascade-renumber the whole table again (the same mistake corrected once already this session), only that one gate moved -- to ADR-024, one past the table's current ceiling. ADR-012 documents the corrected policy this establishes: an unplanned ADR always takes the next integer; a collision moves only the one colliding gate to the ceiling, not the whole table. ## Schema (migrations/000012_user_roles.sql) `users.role text NOT NULL DEFAULT 'tenant' CHECK (role IN ('tenant', 'operator'))`. DEFAULT 'tenant' is a fail-closed default: every existing user (and every future wallet-auto-provisioned one, unchanged) becomes the least-privileged role; nobody is silently upgraded to operator by this migration. ## userauth - `User.Role`, `RoleTenant`/`RoleOperator` constants, `ValidRole`. - `RoleSatisfies(actual, required)`: a ranked comparison (operator satisfies a tenant-tier requirement too) that explicitly fails closed for an unrecognized `actual` role rather than relying on a map's zero-value behavior. - `Repository.SetRole`, implemented on `PostgresRepository` (`ErrUserNotFound` for an unknown user_id; the CHECK constraint, not duplicated Go-side validation, is the authoritative guard against an invalid role value). - `CreateUser`/`Authenticate` now read/return `Role`. ## cmd/controlplane-admin `grant-role ` -- the only way a user becomes (or stops being) an operator, mirroring create-user/issue-key's existing break-glass, no-self-service pattern. ## internal/dashboard `requireRole(minRole, next)`: 401 for no credential, 403 for a valid credential with an insufficient role (kept distinct so a caller can tell "log in" from "you're logged in but not allowed"). Refactored `authenticatedUserID` into a thin wrapper over a new `authenticatedUser` (returns the full `userauth.User`, not just the ID) rather than duplicating the bearer-token-parsing logic. ## Tested - `userauth`: role default, SetRole grant/revoke round trip (verified against a real re-Authenticate, not just the SetRole call's own return value), ErrUserNotFound, CHECK-constraint rejection of an invalid role, RoleSatisfies ordering including the fail-closed unrecognized-role case. - `dashboard`: requireRole against every combination (unauthenticated, tenant-at-tenant-gate, tenant-at-operator-gate, operator-at-either-gate, revoked key) -- all against the real PostgresRepository, not a fake. - `grant-role` smoke-tested end to end against the running local dev stack (create-user, grant operator, verify via a direct SQL read, reject an invalid role, reject an unknown user_id), then cleaned up. - gofmt, go vet, go build, and the full control-plane test suite (including every OPENINFRA_TEST_*-gated live Postgres/Redis/chain test) all ran clean. Leaves #76 open: slices 2-6 (tenant workload views, the /api/v1/overview breaking change, operator queue/worker views, the audit log, and E2E tests) are all still outstanding. Co-Authored-By: Claude Sonnet 5 --- AGENTS.md | 2 +- ROADMAP.md | 2 +- control-plane/cmd/controlplane-admin/main.go | 40 ++++- control-plane/internal/dashboard/auth.go | 22 ++- control-plane/internal/dashboard/rbac.go | 48 +++++ control-plane/internal/dashboard/rbac_test.go | 164 ++++++++++++++++++ .../internal/userauth/interceptor_test.go | 3 +- control-plane/internal/userauth/postgres.go | 28 ++- .../internal/userauth/postgres_test.go | 101 +++++++++++ control-plane/internal/userauth/role_test.go | 44 +++++ control-plane/internal/userauth/userauth.go | 63 +++++++ .../migrations/000012_user_roles.sql | 13 ++ ...ralization-roadmap-and-trust-boundaries.md | 18 +- ...16-dashboard-rbac-and-tenant-isolation.md} | 21 ++- 14 files changed, 542 insertions(+), 27 deletions(-) create mode 100644 control-plane/internal/dashboard/rbac.go create mode 100644 control-plane/internal/dashboard/rbac_test.go create mode 100644 control-plane/internal/userauth/role_test.go create mode 100644 control-plane/migrations/000012_user_roles.sql rename docs/{control-plane/dashboard-rbac-and-tenant-isolation-proposal.md => adr/016-dashboard-rbac-and-tenant-isolation.md} (94%) diff --git a/AGENTS.md b/AGENTS.md index 0caecc6..1a1032a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -40,4 +40,4 @@ Every production feature requires tests and every architecture change requires a **Permanent — no ADR lifts these.** Never hard-code secrets. Never change a contract without consumer analysis. Never put detailed metrics on-chain. Never put tenant payloads, logs, secrets, or any personal data on-chain: consensus state cannot be erased, so only hashes and commitments may cross that line (ADR-012 §3). -**Prohibited until the ADR gate named in ADR-012 §6 is accepted.** Do not introduce another database (ADR-016, ADR-021), direct Agent-to-chain access (ADR-020), runtime orchestration (ADR-019), decentralized storage (ADR-021), a TEE trust root (ADR-022), or a replacement for `EnsureRoot` governance (ADR-023). Kubernetes remains prohibited under ADR-006, which fixes Docker as the runtime; adopting it needs its own accepted ADR. +**Prohibited until the ADR gate named in ADR-012 §6 is accepted.** Do not introduce another database (ADR-024, ADR-021), direct Agent-to-chain access (ADR-020), runtime orchestration (ADR-019), decentralized storage (ADR-021), a TEE trust root (ADR-022), or a replacement for `EnsureRoot` governance (ADR-023). Kubernetes remains prohibited under ADR-006, which fixes Docker as the runtime; adopting it needs its own accepted ADR. diff --git a/ROADMAP.md b/ROADMAP.md index c7763c7..1de34e4 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -52,7 +52,7 @@ Progressively remove centralized frontend, database, scheduler, and operational | Issue | Gate | |---|---| | #32 — decentralization roadmap and trust boundaries | ADR-012 (accepted) | -| #33 — replicated off-chain data plane | ADR-016 | +| #33 — replicated off-chain data plane | ADR-024 | | #34 — multiple Control Planes and scheduling relays | ADR-017 | | #35 — content-addressed frontend distribution | ADR-021 | | #36 — decentralized identity, governance, validator operations | ADR-023 | diff --git a/control-plane/cmd/controlplane-admin/main.go b/control-plane/cmd/controlplane-admin/main.go index 6147336..f3f228a 100644 --- a/control-plane/cmd/controlplane-admin/main.go +++ b/control-plane/cmd/controlplane-admin/main.go @@ -1,6 +1,6 @@ -// Command controlplane-admin is operator tooling for two unrelated -// privileged actions that both need the Control Plane's own credentials -// rather than an ordinary user's or validator's: +// Command controlplane-admin is operator tooling for privileged actions +// that need the Control Plane's own credentials rather than an ordinary +// user's or validator's: // // - User/API-key management (issue #12): there is deliberately no // self-service user registration RPC (that would be a much larger @@ -10,6 +10,10 @@ // exactly once, to stdout, and never persisted anywhere (only its // SHA-256 hash lives in Postgres) -- copy it immediately; there is no // way to recover it later, only to revoke it and issue a new one. +// - grant-role (ADR-016 §4, issue #76): the only way a user becomes a +// dashboard operator (or is demoted back to tenant). Same trust +// boundary as create-user/issue-key -- there is no self-service way +// for a user to grant themselves elevated dashboard access. // - resolve-dispute (ADR-013 slice 5, issue #78): pallet-network- // validator's resolve_dispute extrinsic is SuspensionOrigin-gated // (EnsureRoot in this runtime) -- only the Control Plane's own @@ -20,8 +24,8 @@ // account. // // Each subcommand only connects to the credential store it actually -// needs -- user/API-key commands never touch the chain, resolve-dispute -// never touches Postgres. +// needs -- user/API-key/role commands never touch the chain, +// resolve-dispute never touches Postgres. package main import ( @@ -53,7 +57,7 @@ func run(args []string) error { ctx := context.Background() switch args[0] { - case "create-user", "issue-key", "revoke-key": + case "create-user", "issue-key", "revoke-key", "grant-role": return runUserCommand(ctx, args) case "resolve-dispute": return runResolveDispute(ctx, args) @@ -97,6 +101,11 @@ func runUserCommand(ctx context.Context, args []string) error { } fmt.Println("revoked") return nil + case "grant-role": + if len(args) != 3 { + return errors.New("usage: controlplane-admin grant-role ") + } + return grantRole(ctx, repository, args[1], args[2]) } return usageError() } @@ -120,6 +129,23 @@ func issueKey(ctx context.Context, repository *userauth.PostgresRepository, user return nil } +// grantRole is ADR-016 §4's break-glass grant path: the only way a user +// becomes (or stops being) a dashboard operator. Deliberately the same +// operational shape as create-user/issue-key/revoke-key -- an offline +// Postgres write requiring this binary's own DATABASE_URL access, not a +// self-service RPC a logged-in user could call on themselves or anyone +// else. +func grantRole(ctx context.Context, repository *userauth.PostgresRepository, userID, role string) error { + if !userauth.ValidRole(role) { + return fmt.Errorf("%q must be exactly %q or %q", role, userauth.RoleTenant, userauth.RoleOperator) + } + if err := repository.SetRole(ctx, userID, role); err != nil { + return fmt.Errorf("grant role: %w", err) + } + fmt.Printf("user %s is now role=%s\n", userID, role) + return nil +} + // runResolveDispute settles a dispute pallet-network-validator's // resolve_dispute -- see the package doc comment for why this, uniquely // among this session's Network Validator tooling, must run with the @@ -197,5 +223,5 @@ func parseUpholdOrReject(value string) (bool, error) { } func usageError() error { - return errors.New("usage: controlplane-admin | issue-key | revoke-key | resolve-dispute >") + return errors.New("usage: controlplane-admin | issue-key | revoke-key | grant-role | resolve-dispute >") } diff --git a/control-plane/internal/dashboard/auth.go b/control-plane/internal/dashboard/auth.go index 65ecdc9..be83219 100644 --- a/control-plane/internal/dashboard/auth.go +++ b/control-plane/internal/dashboard/auth.go @@ -128,20 +128,34 @@ func (s *Server) authIssueAPIKey(w http.ResponseWriter, r *http.Request) { } func (s *Server) authenticatedUserID(ctx context.Context, r *http.Request) (string, bool) { + user, ok := s.authenticatedUser(ctx, r) + if !ok { + return "", false + } + return user.UserID, true +} + +// authenticatedUser is authenticatedUserID's superset: the full +// userauth.User (including Role, ADR-016), for callers that need more +// than just the ID -- currently only requireRole (rbac.go), which needs +// Role. Kept as the one bearer-token-parsing implementation rather than +// duplicating it, with authenticatedUserID as a thin wrapper so its +// existing callers/behavior are unchanged. +func (s *Server) authenticatedUser(ctx context.Context, r *http.Request) (userauth.User, bool) { const prefix = "Bearer " header := r.Header.Get("Authorization") if !strings.HasPrefix(header, prefix) { - return "", false + return userauth.User{}, false } raw := strings.TrimPrefix(header, prefix) if raw == "" { - return "", false + return userauth.User{}, false } user, err := s.users.Authenticate(ctx, userauth.HashAPIKey(raw)) if err != nil { - return "", false + return userauth.User{}, false } - return user.UserID, true + return user, true } // allowRate applies the per-endpoint abuse rate limit, keyed by caller diff --git a/control-plane/internal/dashboard/rbac.go b/control-plane/internal/dashboard/rbac.go new file mode 100644 index 0000000..7680a12 --- /dev/null +++ b/control-plane/internal/dashboard/rbac.go @@ -0,0 +1,48 @@ +package dashboard + +import ( + "net/http" + + "github.com/openinfra/network/internal/userauth" +) + +// requireRole is ADR-016 slice 1's authorization enforcement point: wrap +// any handler that should only run for a caller holding a valid +// session/API key whose users.role is at least as privileged as minRole +// (userauth.RoleSatisfies -- an operator satisfies a "tenant" +// requirement too, see that function's doc comment). Intended to be +// applied once per route at registration time (Server.Handler in +// dashboard.go), the same place every route is already declared, so the +// entire dashboard authorization surface is auditable by reading one +// function rather than hunting for ad hoc checks inside individual +// handlers. +// +// Two distinct failure responses, not one generic "forbidden": 401 when +// there is no valid credential at all (the caller should log in), 403 +// once a real identity is established but under-privileged for this +// route (the caller is who they say they are, just not allowed here) -- +// collapsing these into one response would make "you're not logged in" +// indistinguishable from "you're logged in as the wrong role," which is +// a worse debugging experience for a legitimate caller and gives an +// attacker no useful signal either way (both already require a valid +// credential to get past the first check). +// +// Slice 1 introduces this function with zero routes wrapped in it yet -- +// see ADR-016's Sequencing section. It is still real, tested code: a +// later slice's diff is "wrap the new route in requireRole," not "invent +// and test this function for the first time under deadline pressure." +func (s *Server) requireRole(minRole string, next http.HandlerFunc) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + user, ok := s.authenticatedUser(ctx, r) + if !ok { + writeJSON(w, http.StatusUnauthorized, map[string]string{"error": "authentication required"}) + return + } + if !userauth.RoleSatisfies(user.Role, minRole) { + writeJSON(w, http.StatusForbidden, map[string]string{"error": "insufficient role"}) + return + } + next(w, r) + } +} diff --git a/control-plane/internal/dashboard/rbac_test.go b/control-plane/internal/dashboard/rbac_test.go new file mode 100644 index 0000000..3b86682 --- /dev/null +++ b/control-plane/internal/dashboard/rbac_test.go @@ -0,0 +1,164 @@ +package dashboard + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/openinfra/network/internal/userauth" +) + +// issueSessionKey is a small test helper: creates a user with the given +// role and returns a raw API key that authenticates as them -- +// deliberately going through the real userauth.PostgresRepository (via +// newAuthTestServer), not a fake, so these tests exercise requireRole +// against the actual Authenticate/Role round trip a real request would. +func issueSessionKey(t *testing.T, server *Server, role string) string { + t.Helper() + user, err := server.users.CreateUser(t.Context(), "rbac-test-user") + if err != nil { + t.Fatal(err) + } + if role != userauth.RoleTenant { + if err := server.users.SetRole(t.Context(), user.UserID, role); err != nil { + t.Fatal(err) + } + } + key, err := server.users.CreateAPIKey(t.Context(), user.UserID) + if err != nil { + t.Fatal(err) + } + return key.Raw +} + +func TestRequireRoleRejectsAnUnauthenticatedCaller(t *testing.T) { + _, server, _ := newAuthTestServer(t) + handler := server.requireRole(userauth.RoleTenant, func(w http.ResponseWriter, r *http.Request) { + t.Fatal("the wrapped handler must not run for an unauthenticated caller") + }) + + request := httptest.NewRequest(http.MethodGet, "/protected", nil) + recorder := httptest.NewRecorder() + handler.ServeHTTP(recorder, request) + + if recorder.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want 401", recorder.Code) + } +} + +func TestRequireRoleAllowsATenantThroughATenantGate(t *testing.T) { + _, server, _ := newAuthTestServer(t) + rawKey := issueSessionKey(t, server, userauth.RoleTenant) + + ran := false + handler := server.requireRole(userauth.RoleTenant, func(w http.ResponseWriter, r *http.Request) { + ran = true + w.WriteHeader(http.StatusOK) + }) + + request := httptest.NewRequest(http.MethodGet, "/protected", nil) + request.Header.Set("Authorization", "Bearer "+rawKey) + recorder := httptest.NewRecorder() + handler.ServeHTTP(recorder, request) + + if !ran { + t.Fatal("expected the wrapped handler to run for a tenant at a tenant gate") + } + if recorder.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", recorder.Code) + } +} + +func TestRequireRoleRejectsATenantAtAnOperatorGate(t *testing.T) { + _, server, _ := newAuthTestServer(t) + rawKey := issueSessionKey(t, server, userauth.RoleTenant) + + handler := server.requireRole(userauth.RoleOperator, func(w http.ResponseWriter, r *http.Request) { + t.Fatal("the wrapped handler must not run for an under-privileged caller") + }) + + request := httptest.NewRequest(http.MethodGet, "/protected", nil) + request.Header.Set("Authorization", "Bearer "+rawKey) + recorder := httptest.NewRecorder() + handler.ServeHTTP(recorder, request) + + // 403, not 401 -- the caller has a real, valid credential; they are + // simply the wrong role. See requireRole's doc comment for why this + // distinction matters. + if recorder.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403", recorder.Code) + } +} + +func TestRequireRoleAllowsAnOperatorThroughATenantGate(t *testing.T) { + _, server, _ := newAuthTestServer(t) + rawKey := issueSessionKey(t, server, userauth.RoleOperator) + + ran := false + handler := server.requireRole(userauth.RoleTenant, func(w http.ResponseWriter, r *http.Request) { + ran = true + w.WriteHeader(http.StatusOK) + }) + + request := httptest.NewRequest(http.MethodGet, "/protected", nil) + request.Header.Set("Authorization", "Bearer "+rawKey) + recorder := httptest.NewRecorder() + handler.ServeHTTP(recorder, request) + + if !ran { + t.Fatal("expected an operator to satisfy a tenant-tier gate too (ADR-016 §1's ranked roles)") + } + if recorder.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", recorder.Code) + } +} + +func TestRequireRoleAllowsAnOperatorThroughAnOperatorGate(t *testing.T) { + _, server, _ := newAuthTestServer(t) + rawKey := issueSessionKey(t, server, userauth.RoleOperator) + + ran := false + handler := server.requireRole(userauth.RoleOperator, func(w http.ResponseWriter, r *http.Request) { + ran = true + w.WriteHeader(http.StatusOK) + }) + + request := httptest.NewRequest(http.MethodGet, "/protected", nil) + request.Header.Set("Authorization", "Bearer "+rawKey) + recorder := httptest.NewRecorder() + handler.ServeHTTP(recorder, request) + + if !ran { + t.Fatal("expected an operator to satisfy an operator-tier gate") + } + if recorder.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", recorder.Code) + } +} + +func TestRequireRoleRejectsARevokedKey(t *testing.T) { + _, server, _ := newAuthTestServer(t) + user, err := server.users.CreateUser(t.Context(), "rbac-test-user") + if err != nil { + t.Fatal(err) + } + key, err := server.users.CreateAPIKey(t.Context(), user.UserID) + if err != nil { + t.Fatal(err) + } + if err := server.users.RevokeAPIKey(t.Context(), key.KeyID); err != nil { + t.Fatal(err) + } + + handler := server.requireRole(userauth.RoleTenant, func(w http.ResponseWriter, r *http.Request) { + t.Fatal("the wrapped handler must not run for a revoked key") + }) + request := httptest.NewRequest(http.MethodGet, "/protected", nil) + request.Header.Set("Authorization", "Bearer "+key.Raw) + recorder := httptest.NewRecorder() + handler.ServeHTTP(recorder, request) + + if recorder.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want 401", recorder.Code) + } +} diff --git a/control-plane/internal/userauth/interceptor_test.go b/control-plane/internal/userauth/interceptor_test.go index baf593c..82014a4 100644 --- a/control-plane/internal/userauth/interceptor_test.go +++ b/control-plane/internal/userauth/interceptor_test.go @@ -25,7 +25,8 @@ func (r fakeRepository) CreateAPIKey(context.Context, string) (userauth.APIKey, func (r fakeRepository) CreateAPIKeyWithExpiry(context.Context, string, *time.Time) (userauth.APIKey, error) { panic("unused") } -func (r fakeRepository) RevokeAPIKey(context.Context, string) error { panic("unused") } +func (r fakeRepository) RevokeAPIKey(context.Context, string) error { panic("unused") } +func (r fakeRepository) SetRole(context.Context, string, string) error { panic("unused") } func (r fakeRepository) Authenticate(_ context.Context, hash [32]byte) (userauth.User, error) { if r.err != nil { return userauth.User{}, r.err diff --git a/control-plane/internal/userauth/postgres.go b/control-plane/internal/userauth/postgres.go index a064306..99d4991 100644 --- a/control-plane/internal/userauth/postgres.go +++ b/control-plane/internal/userauth/postgres.go @@ -17,7 +17,11 @@ func NewPostgresRepository(pool *pgxpool.Pool) *PostgresRepository { } func (r *PostgresRepository) CreateUser(ctx context.Context, displayName string) (User, error) { - user := User{UserID: uuid.NewString(), DisplayName: displayName, CreatedAt: time.Now().UTC()} + // Role is not in the INSERT column list -- the migration's + // DEFAULT 'tenant' applies, and Role is set explicitly here to match + // it, rather than left as Go's zero-value "" (which is not a valid + // role per ValidRole). + user := User{UserID: uuid.NewString(), DisplayName: displayName, CreatedAt: time.Now().UTC(), Role: RoleTenant} if _, err := r.pool.Exec(ctx, `INSERT INTO users (user_id, display_name, created_at) VALUES ($1,$2,$3)`, user.UserID, user.DisplayName, user.CreatedAt); err != nil { return User{}, err } @@ -54,8 +58,8 @@ func (r *PostgresRepository) Authenticate(ctx context.Context, hash [32]byte) (U AND api_keys.revoked_at IS NULL AND (api_keys.expires_at IS NULL OR api_keys.expires_at > now()) AND users.user_id = api_keys.user_id - RETURNING users.user_id, users.display_name, users.created_at - `, hash[:]).Scan(&user.UserID, &user.DisplayName, &user.CreatedAt) + RETURNING users.user_id, users.display_name, users.created_at, users.role + `, hash[:]).Scan(&user.UserID, &user.DisplayName, &user.CreatedAt, &user.Role) if errors.Is(err, pgx.ErrNoRows) { return User{}, ErrInvalidKey } @@ -75,3 +79,21 @@ func (r *PostgresRepository) RevokeAPIKey(ctx context.Context, keyID string) err } return nil } + +// SetRole does not itself validate role against ValidRole -- the CHECK +// constraint on users.role (migrations/000012_user_roles.sql) is the +// actual, authoritative enforcement, so an invalid value fails this +// query rather than silently writing something the constraint would +// have rejected anyway. Callers still validate up front (see +// cmd/controlplane-admin's grant-role) purely to give an operator a +// clear CLI error instead of a raw Postgres constraint-violation message. +func (r *PostgresRepository) SetRole(ctx context.Context, userID string, role string) error { + command, err := r.pool.Exec(ctx, `UPDATE users SET role = $1 WHERE user_id = $2`, role, userID) + if err != nil { + return err + } + if command.RowsAffected() != 1 { + return ErrUserNotFound + } + return nil +} diff --git a/control-plane/internal/userauth/postgres_test.go b/control-plane/internal/userauth/postgres_test.go index f88b9ff..7495b37 100644 --- a/control-plane/internal/userauth/postgres_test.go +++ b/control-plane/internal/userauth/postgres_test.go @@ -164,3 +164,104 @@ func TestAuthenticateIsUnaffectedByAnotherKeysHash(t *testing.T) { t.Fatalf("Authenticate(keyB) = %+v, %v, want user %q", gotB, err, userB.UserID) } } + +// TestCreateUserDefaultsToTenantRole pins ADR-016's fail-closed default: +// a brand new user (created via controlplane-admin, or -- though not +// exercised by this test directly -- auto-provisioned by wallet login, +// which shares the same DEFAULT 'tenant' at the schema level) never +// starts as an operator. +func TestCreateUserDefaultsToTenantRole(t *testing.T) { + ctx, pool := newTestPool(t) + repository := userauth.NewPostgresRepository(pool) + + user, err := repository.CreateUser(ctx, "alice") + if err != nil { + t.Fatal(err) + } + if user.Role != userauth.RoleTenant { + t.Fatalf("CreateUser().Role = %q, want %q", user.Role, userauth.RoleTenant) + } + + // Confirm the persisted row agrees with CreateUser's own return value + // -- not just that the Go struct says "tenant", but that Authenticate + // (a separate query, going through the actual users.role column) also + // reports it. + key, err := repository.CreateAPIKey(ctx, user.UserID) + if err != nil { + t.Fatal(err) + } + authenticated, err := repository.Authenticate(ctx, userauth.HashAPIKey(key.Raw)) + if err != nil { + t.Fatal(err) + } + if authenticated.Role != userauth.RoleTenant { + t.Fatalf("Authenticate().Role = %q, want %q", authenticated.Role, userauth.RoleTenant) + } +} + +func TestSetRoleGrantsAndRevokesOperator(t *testing.T) { + ctx, pool := newTestPool(t) + repository := userauth.NewPostgresRepository(pool) + + user, err := repository.CreateUser(ctx, "alice") + if err != nil { + t.Fatal(err) + } + key, err := repository.CreateAPIKey(ctx, user.UserID) + if err != nil { + t.Fatal(err) + } + + if err := repository.SetRole(ctx, user.UserID, userauth.RoleOperator); err != nil { + t.Fatalf("SetRole(operator): %v", err) + } + promoted, err := repository.Authenticate(ctx, userauth.HashAPIKey(key.Raw)) + if err != nil { + t.Fatal(err) + } + if promoted.Role != userauth.RoleOperator { + t.Fatalf("Role after grant = %q, want %q", promoted.Role, userauth.RoleOperator) + } + + // The grant path is also the revoke path -- setting back to tenant + // is not a separate method, matching ADR-016 §4's + // `grant-role tenant` CLI usage. + if err := repository.SetRole(ctx, user.UserID, userauth.RoleTenant); err != nil { + t.Fatalf("SetRole(tenant): %v", err) + } + demoted, err := repository.Authenticate(ctx, userauth.HashAPIKey(key.Raw)) + if err != nil { + t.Fatal(err) + } + if demoted.Role != userauth.RoleTenant { + t.Fatalf("Role after revoke = %q, want %q", demoted.Role, userauth.RoleTenant) + } +} + +func TestSetRoleReportsUserNotFoundForAnUnknownUserID(t *testing.T) { + ctx, pool := newTestPool(t) + repository := userauth.NewPostgresRepository(pool) + + err := repository.SetRole(ctx, uuid.NewString(), userauth.RoleOperator) + if err != userauth.ErrUserNotFound { + t.Fatalf("SetRole() for an unknown user = %v, want ErrUserNotFound", err) + } +} + +// TestSetRoleRejectsAnInvalidRoleAtTheDatabaseConstraint proves the CHECK +// constraint is the real backstop (SetRole's own doc comment says it +// deliberately does not duplicate ValidRole's check) -- an invalid value +// must fail loudly, not silently write something the schema doesn't +// allow. +func TestSetRoleRejectsAnInvalidRoleAtTheDatabaseConstraint(t *testing.T) { + ctx, pool := newTestPool(t) + repository := userauth.NewPostgresRepository(pool) + + user, err := repository.CreateUser(ctx, "alice") + if err != nil { + t.Fatal(err) + } + if err := repository.SetRole(ctx, user.UserID, "admin"); err == nil { + t.Fatal("expected SetRole with an invalid role to fail against the CHECK constraint") + } +} diff --git a/control-plane/internal/userauth/role_test.go b/control-plane/internal/userauth/role_test.go new file mode 100644 index 0000000..ab43bfa --- /dev/null +++ b/control-plane/internal/userauth/role_test.go @@ -0,0 +1,44 @@ +package userauth_test + +import ( + "testing" + + "github.com/openinfra/network/internal/userauth" +) + +func TestValidRoleAcceptsExactlyTenantAndOperator(t *testing.T) { + if !userauth.ValidRole(userauth.RoleTenant) { + t.Fatal("expected RoleTenant to be valid") + } + if !userauth.ValidRole(userauth.RoleOperator) { + t.Fatal("expected RoleOperator to be valid") + } + for _, bad := range []string{"", "admin", "Tenant", "operator "} { + if userauth.ValidRole(bad) { + t.Fatalf("expected %q to be invalid", bad) + } + } +} + +func TestRoleSatisfiesOrdering(t *testing.T) { + cases := []struct { + actual, required string + want bool + }{ + {userauth.RoleTenant, userauth.RoleTenant, true}, + {userauth.RoleOperator, userauth.RoleTenant, true}, + {userauth.RoleOperator, userauth.RoleOperator, true}, + {userauth.RoleTenant, userauth.RoleOperator, false}, + // An unrecognized role (should never happen once ValidRole is + // enforced at every write path, but RoleSatisfies must still + // fail closed rather than panic on a corrupt/future value) ranks + // below every real role. + {"unknown", userauth.RoleTenant, false}, + {"unknown", "unknown", false}, + } + for _, c := range cases { + if got := userauth.RoleSatisfies(c.actual, c.required); got != c.want { + t.Errorf("RoleSatisfies(%q, %q) = %v, want %v", c.actual, c.required, got, c.want) + } + } +} diff --git a/control-plane/internal/userauth/userauth.go b/control-plane/internal/userauth/userauth.go index 3e6950f..e0b2a41 100644 --- a/control-plane/internal/userauth/userauth.go +++ b/control-plane/internal/userauth/userauth.go @@ -27,6 +27,13 @@ import ( // the raw key. var ErrInvalidKey = errors.New("invalid or expired API key") +// ErrUserNotFound is SetRole's failure for a user_id that does not exist +// -- deliberately a distinct error from ErrInvalidKey, since granting a +// role is an operator (cmd/controlplane-admin) action against a known +// user_id, not a credential check with the same "don't leak which part +// was wrong" concern ErrInvalidKey exists for. +var ErrUserNotFound = errors.New("user not found") + // keyPrefix marks OpenInfra user API keys recognizably in logs/tooling // output, the same spirit as GitHub's "ghp_" or Stripe's "sk_" prefixes. const keyPrefix = "oiu_" @@ -43,6 +50,54 @@ type User struct { UserID string DisplayName string CreatedAt time.Time + // Role is ADR-016's dashboard-authorization tier: RoleTenant (the + // default for every user, existing or new) or RoleOperator (only + // reachable via an explicit controlplane-admin grant-role). Distinct + // from anything about API-key scoping -- this field governs what a + // browser dashboard session may see, not what the gRPC user-facing + // API already scopes by owner_id. + Role string +} + +// RoleTenant and RoleOperator are the only two values users.role's CHECK +// constraint (migrations/000012_user_roles.sql) allows -- ADR-016 §1 +// deliberately keeps this to one column, two values, rather than a +// many-to-many roles table the MVP doesn't need yet. +const ( + RoleTenant = "tenant" + RoleOperator = "operator" +) + +// ValidRole reports whether role is one of the two roles this system +// recognizes -- used by both SetRole implementations and +// cmd/controlplane-admin's grant-role argument parsing, so "reject an +// unknown role" is defined exactly once. +func ValidRole(role string) bool { + return role == RoleTenant || role == RoleOperator +} + +// roleRank orders roles for internal/dashboard's requireRole check: +// higher ranks satisfy lower-or-equal requirements (an operator may reach +// a tenant-tier endpoint; a tenant may never reach an operator-tier one). +// Unexported here deliberately -- RoleSatisfies below is the only +// intended way to compare roles, so the ranking itself can be +// restructured later (e.g. a third tier) without every caller needing to +// know it's backed by integers. +var roleRank = map[string]int{RoleTenant: 1, RoleOperator: 2} + +// RoleSatisfies reports whether actual is at least as privileged as +// required. An unrecognized actual role (should not happen once +// ValidRole is enforced at every write path, but this must still fail +// closed rather than panic or vacuously succeed on a corrupt/future +// value) never satisfies anything -- explicitly checked via ValidRole +// rather than relying on roleRank's zero-value-for-a-missing-key +// behavior, which would otherwise let two equally-unrecognized values +// compare as satisfying each other. +func RoleSatisfies(actual, required string) bool { + if !ValidRole(actual) { + return false + } + return roleRank[actual] >= roleRank[required] } // APIKey is a credential a User authenticates with. Raw is populated only @@ -100,6 +155,14 @@ type Repository interface { // must not fail authentication itself. Authenticate(ctx context.Context, hash [32]byte) (User, error) RevokeAPIKey(ctx context.Context, keyID string) error + // SetRole is ADR-016's grant path (cmd/controlplane-admin's + // grant-role): an operator-only, offline, break-glass action, the + // same trust boundary create-user/issue-key already require -- there + // is deliberately no self-service way for a user to grant themselves + // (or anyone else) the operator role. Returns ErrUserNotFound for an + // unknown userID; callers are expected to have already validated + // role via ValidRole before calling. + SetRole(ctx context.Context, userID string, role string) error } type contextKey int diff --git a/control-plane/migrations/000012_user_roles.sql b/control-plane/migrations/000012_user_roles.sql new file mode 100644 index 0000000..beea64a --- /dev/null +++ b/control-plane/migrations/000012_user_roles.sql @@ -0,0 +1,13 @@ +-- ADR-016 slice 1: dashboard RBAC's schema. A single role column, not a +-- many-to-many table -- every real actor today (tenant, operator) has +-- exactly one job; see ADR-016 §1 for why a join table is deliberately +-- deferred rather than built speculatively. +-- +-- DEFAULT 'tenant' means every existing user (created before this +-- migration, via controlplane-admin create-user or wallet auto- +-- provisioning) becomes a tenant, the least-privileged role, rather than +-- an operator -- a fail-closed default: nobody is silently upgraded to +-- operator by this migration, an explicit `controlplane-admin grant-role` +-- is required for that. +ALTER TABLE users ADD COLUMN IF NOT EXISTS role text NOT NULL DEFAULT 'tenant' + CHECK (role IN ('tenant', 'operator')); diff --git a/docs/adr/012-decentralization-roadmap-and-trust-boundaries.md b/docs/adr/012-decentralization-roadmap-and-trust-boundaries.md index 8e241d5..c5e06a6 100644 --- a/docs/adr/012-decentralization-roadmap-and-trust-boundaries.md +++ b/docs/adr/012-decentralization-roadmap-and-trust-boundaries.md @@ -205,7 +205,6 @@ named in the last column. | Gate | Unblocks | Prohibition it must lift, and what it must settle | |---|---|---| -| **ADR-016** — replicated off-chain data plane | #33 | "another database". Must settle: event log vs CRDT, deterministic IDs, ordering, snapshots, pruning, and the PostgreSQL deprecation criteria | | **ADR-017** — multi-Control-Plane and relay protocol | #34 | single-Control-Plane component boundary. Must settle: leader/leaderless rules, idempotency, peer admission, and how an Agent refuses an unauthenticated relay | | **ADR-018** — slashing and economic penalties | #52 | none (new mechanism). Already demanded by ADR-011 §5, which ships rewards but explicitly defers slashing economics. Must settle: false-positive protection, appeals, and interaction with `dispute_round` | | **ADR-019** — on-chain orchestration | #50, #62 | "runtime orchestration". Must settle: what scheduling logic is deterministic enough for the runtime, and what stays off-chain | @@ -213,6 +212,7 @@ named in the last column. | **ADR-021** — content-addressed distribution and decentralized storage | #35, #58, #59 | "another database". Must settle: pinning, retention proofs, erasure, and gateway trust | | **ADR-022** — TEE and distributed attestation | #60, #61 | none (new trust root). Must settle: which vendor roots are trusted, revocation, and what an unattested provider may still do | | **ADR-023** — decentralized identity, key rotation, and governance | #36 | `EnsureRoot` as governance (`blockchain/runtime/src/lib.rs:316`). Must settle: rotation and recovery per role, stake/delegation, timelocks, and emergency constraints | +| **ADR-024** — replicated off-chain data plane | #33 | "another database". Must settle: event log vs CRDT, deterministic IDs, ordering, snapshots, pruning, and the PostgreSQL deprecation criteria. (Moved here from its original `ADR-016` reservation — see "Consequences" below.) | Three issues need **no new gate**: @@ -297,3 +297,19 @@ It does not change any code, origin, or storage item today. from whatever is next free at the time; this table is a *reservation of intent*, not a claim on the number, and the next accepted ADR of any kind (gate or not) takes the next integer regardless of what this table says. +- **§6 was corrected a second time, more narrowly.** `ADR-016` (dashboard RBAC + and tenant isolation, `docs/adr/016-dashboard-rbac-and-tenant-isolation.md`) + was accepted next, colliding with this table's reservation of `ADR-016` for + "replicated off-chain data plane" (#33). Cascading the whole table down by + one again (as the first correction above did) would only guarantee a third + collision the next time an unplanned ADR lands — this project's actual + velocity, four times running now (`ADR-013` through `ADR-016`), is + "unplanned work claims the next number," not "gates are claimed in this + table's order." The policy is revised accordingly: an unplanned ADR always + takes the next integer; if that collides with a reserved gate, **only that + one gate** moves to one past this table's current highest reserved number, + the rest of the table is left untouched. Concretely here: `#33`'s gate moved + from `ADR-016` to `ADR-024` (§6's table above reflects this; no other gate + number changed). This is expected to be the stable policy going forward — + no further cascading renumbering, just individual gates relocating to the + ceiling as they collide, one at a time. diff --git a/docs/control-plane/dashboard-rbac-and-tenant-isolation-proposal.md b/docs/adr/016-dashboard-rbac-and-tenant-isolation.md similarity index 94% rename from docs/control-plane/dashboard-rbac-and-tenant-isolation-proposal.md rename to docs/adr/016-dashboard-rbac-and-tenant-isolation.md index d1df53c..f09642d 100644 --- a/docs/control-plane/dashboard-rbac-and-tenant-isolation-proposal.md +++ b/docs/adr/016-dashboard-rbac-and-tenant-isolation.md @@ -1,15 +1,18 @@ -# Proposal: dashboard RBAC, tenant isolation, and operator views +# ADR-016: Dashboard RBAC, tenant isolation, and operator views ## Status -**Proposed — not accepted.** Unlike ADR-013/014/015 this session, this one is deliberately left -for explicit human review before implementation starts: it changes who can see which tenant's -data, which is a real security boundary, not a narrower technical decision. If accepted, it -becomes an ADR under `docs/adr/` at whatever number is next free at acceptance time (see -ADR-012's "Consequences" section on why this repository assigns ADR numbers at acceptance, not -in advance). Written to unblock issue #76's largest remaining item: "RBAC and tenant isolation on -the dashboard itself (today: no auth at all)," plus the user- and operator-view items that depend -on it. +Accepted (by the repository owner, explicitly — this proposal was deliberately left unaccepted by +Claude Code when first written, unlike ADR-013/014/015 this session, since it decides who can see +which tenant's data: a real security boundary, not a narrower technical decision). + +**Implementation note:** §7's three open questions are **not** all resolved by acceptance — +acceptance authorizes slice 1 (§ Sequencing: schema + grant path, no tenant-private data exposed +yet) to proceed immediately. Slice 2 (tenant workload views, which first exposes +`workload.definition`) still needs §7 questions 1 and 2 answered before it ships, exactly as +§ Consequences already said. Written to unblock issue #76's largest remaining item: "RBAC and +tenant isolation on the dashboard itself (today: no auth at all)," plus the user- and +operator-view items that depend on it. ## Context