diff --git a/docs/phase-4-runbook.md b/docs/phase-4-runbook.md index 26a06ae..3e48e26 100644 --- a/docs/phase-4-runbook.md +++ b/docs/phase-4-runbook.md @@ -557,11 +557,14 @@ Full accounting: `/docs/security/threat-model.md`. Headline items: identity either (refused outright) for either protocol. - No admin UI to create a `tenant_memberships` row, but §3a/§3b's manual SQL bootstrap is gone -- `enterprise-auth -create-tenant`/ - `-grant-membership-*` (offline operator flags, same shape as - `-mint-service-token`) replace it. Nothing yet for revoking a - membership, listing a tenant's members, or changing a role after the - fact (SetMembership's upsert supports it at the storage layer; there's - just no flag exposing it). + `-grant-membership-*`/`-revoke-membership-*`/`-list-memberships-tenant` + (offline operator flags, same shape as `-mint-service-token`) cover + create/grant/revoke/list. Changing a role after the fact is just + re-running `-grant-membership-*` with a different `-grant-membership- + role` (`SetMembership`'s upsert already supports it). `RevokeMembership` + refuses a tenant's current Owner (would leave `tenants.owner_user_id` + dangling) -- transferring ownership first has no flag yet, only + `rbacstore.SetOwner` at the storage layer. - **Per-resource dashboard grants are now enforced** (`api/dashboards`' handler reads `dashboard_permissions` via `enterprise/internal/rbacstore.DashboardPermissions`, only when diff --git a/enterprise/README.md b/enterprise/README.md index 9f35239..f48806d 100644 --- a/enterprise/README.md +++ b/enterprise/README.md @@ -156,7 +156,7 @@ silently left out: ## Package layout ``` -cmd/enterprise-auth/ config loading, OIDC discovery at startup, health/authorize/features endpoints, -mint-service-token, -create-tenant, -grant-membership-* +cmd/enterprise-auth/ config loading, OIDC discovery at startup, health/authorize/features endpoints, -mint-service-token, -create-tenant, -grant-membership-*, -revoke-membership-*, -list-memberships-tenant cmd/enterprise-api/ multi-tenant-aware alternative to api/cmd/api -- see its own doc comment internal/tenant/ the ID type -- see its package doc comment before touching it internal/oidc/ coreos/go-oidc wiring: discovery, login redirect, code exchange + ID token verification @@ -271,15 +271,24 @@ docker compose run --rm enterprise-auth -create-tenant=acme -display-name="Acme # users row by then, which -grant-membership-user-email needs. docker compose run --rm enterprise-auth \ -grant-membership-tenant=acme -grant-membership-user-email=you@example.com -grant-membership-role=owner + +# See who's actually in a tenant, and take access away again: +docker compose run --rm enterprise-auth -list-memberships-tenant=acme +docker compose run --rm enterprise-auth \ + -revoke-membership-tenant=acme -revoke-membership-user-email=someone-else@example.com ``` `-create-tenant` only touches `rbacstore` -- pair with `enterprise-api -provision-tenant` (below) for a tenant to actually be able to run queries, not just log in. `role=owner` also calls `SetOwner`, since a tenant's Owner is a dedicated `tenants.owner_user_id` column, not just -the highest `tenant_memberships` role. Not yet built: revoking a -membership, listing a tenant's members, or a flag for -`dashboard_permissions` grants (those go through the HTTP endpoints +the highest `tenant_memberships` role -- `-revoke-membership-*` refuses +to revoke a tenant's current Owner for the same reason (transferring +ownership first has no flag yet, only `rbacstore.SetOwner` at the +storage layer). Changing a role is just re-running +`-grant-membership-*` with a different `-grant-membership-role` +(`SetMembership`'s upsert already supports it). Not yet built: a flag +for `dashboard_permissions` grants (those go through the HTTP endpoints `api/dashboards`' handler now exposes -- `PUT`/`DELETE /dashboards/{id}/permissions/{userId}`, `GET .../permissions`). diff --git a/enterprise/cmd/enterprise-auth/main.go b/enterprise/cmd/enterprise-auth/main.go index e01822b..47c722a 100644 --- a/enterprise/cmd/enterprise-auth/main.go +++ b/enterprise/cmd/enterprise-auth/main.go @@ -13,11 +13,12 @@ // (crewjam/saml's samlsp.FetchMetadata; a trusted operator-supplied URL, // same trust level as OIDC_ISSUER_URL's discovery fetch, not // end-user-controlled input). Also fully wired: -mint-service-token -// (the RoleService credential /alerting presents), and -// -create-tenant/-grant-membership-* -- the operator actions that -// replace phase-4-runbook.md's old "log in once so UpsertUserBySSO -// creates a users row, then hand-write a psql INSERT into -// tenant_memberships" bootstrap dance with a real command. +// (the RoleService credential /alerting presents), -create-tenant/ +// -grant-membership-* -- the operator actions that replace +// phase-4-runbook.md's old "log in once so UpsertUserBySSO creates a +// users row, then hand-write a psql INSERT into tenant_memberships" +// bootstrap dance with a real command -- and their counterparts +// -revoke-membership-*/-list-memberships-tenant. package main import ( @@ -75,6 +76,9 @@ func main() { grantTenant := flag.String("grant-membership-tenant", "", "tenant id to grant a membership in -- all three -grant-membership-* flags are required together") grantUserEmail := flag.String("grant-membership-user-email", "", "email of an existing user to grant a tenant_memberships row to -- the user must have attempted an SSO login at least once already (UpsertUserBySSO creates the users row on first login, even one that then fails with \"no tenant membership\")") grantRole := flag.String("grant-membership-role", "", "role to grant: viewer, editor, admin, or owner") + revokeTenant := flag.String("revoke-membership-tenant", "", "tenant id to revoke a membership from -- both -revoke-membership-* flags are required together") + revokeUserEmail := flag.String("revoke-membership-user-email", "", "email of the user whose tenant_memberships row to delete") + listMembershipsTenant := flag.String("list-memberships-tenant", "", "print every user with a membership in this tenant (id, email, display name, role) and exit") // -healthcheck: same self-check mode as api/-healthcheck (see that // binary's doc comment) -- enterprise-auth's image is distroless too. healthcheck := flag.Bool("healthcheck", false, "self-check mode for Docker's HEALTHCHECK") @@ -122,6 +126,12 @@ func main() { if *grantTenant != "" || *grantUserEmail != "" || *grantRole != "" { os.Exit(runGrantMembership(ctx, logger, rbac, *grantTenant, *grantUserEmail, *grantRole)) } + if *revokeTenant != "" || *revokeUserEmail != "" { + os.Exit(runRevokeMembership(ctx, logger, rbac, *revokeTenant, *revokeUserEmail)) + } + if *listMembershipsTenant != "" { + os.Exit(runListMemberships(ctx, logger, rbac, *listMembershipsTenant)) + } // oidcProvider stays nil (loginhandler.RegisterRoutes then registers // nothing) unless OIDC is actually configured -- matches every other @@ -288,6 +298,63 @@ func runGrantMembership(ctx context.Context, logger *slog.Logger, rbac *rbacstor return 0 } +// runRevokeMembership is -grant-membership's inverse -- looks the user +// up by email (same reasoning as runGrantMembership: an operator knows +// an email, not a generated UUID) and deletes their tenant_memberships +// row. RevokeMembership itself refuses a tenant's current Owner (see +// that method's doc comment); this function doesn't duplicate that +// check, it just surfaces whatever error comes back. +func runRevokeMembership(ctx context.Context, logger *slog.Logger, rbac *rbacstore.Store, tenantID, userEmail string) int { + if tenantID == "" || userEmail == "" { + logger.Error("-revoke-membership-tenant and -revoke-membership-user-email are both required together") + return 1 + } + user, err := rbac.GetUserByEmail(ctx, userEmail) + if err != nil { + if err == rbacstore.ErrNotFound { + logger.Error("no user with this email exists", "email", userEmail) + } else { + logger.Error("looking up user by email", "email", userEmail, "error", err) + } + return 1 + } + if err := rbac.RevokeMembership(ctx, tenantID, user.ID); err != nil { + if err == rbacstore.ErrNotFound { + logger.Error("user has no membership in this tenant", "tenant_id", tenantID, "email", userEmail) + } else { + logger.Error("revoking membership", "error", err) + } + return 1 + } + logger.Info("revoked membership", "tenant_id", tenantID, "user_id", user.ID, "email", userEmail) + return 0 +} + +// runListMemberships prints every member of a tenant to stdout (plain +// text, not JSON -- an operator convenience for deciding who to +// -grant-membership-role= or -revoke-membership-*, not a machine- +// readable API; enterprise-auth has no admin HTTP surface for this at +// all, per this binary's doc comment on why that's deliberate). +func runListMemberships(ctx context.Context, logger *slog.Logger, rbac *rbacstore.Store, tenantID string) int { + if _, err := rbac.GetTenant(ctx, tenantID); err != nil { + logger.Error("looking up tenant", "tenant_id", tenantID, "error", err) + return 1 + } + members, err := rbac.ListMembershipsForTenant(ctx, tenantID) + if err != nil { + logger.Error("listing memberships", "error", err) + return 1 + } + if len(members) == 0 { + fmt.Println("(no members)") + return 0 + } + for _, m := range members { + fmt.Printf("%s\t%s\t%s\t%s\n", m.UserID, m.Email, m.DisplayName, m.Role) + } + return 0 +} + // runHealthcheck mirrors api/cmd/api/main.go's runHealthcheck exactly -- // see that function's doc comment for why this execs the binary against // itself rather than using an external tool. diff --git a/enterprise/internal/rbacstore/rbacstore.go b/enterprise/internal/rbacstore/rbacstore.go index 4e70fd3..eecf49e 100644 --- a/enterprise/internal/rbacstore/rbacstore.go +++ b/enterprise/internal/rbacstore/rbacstore.go @@ -266,6 +266,75 @@ func (s *Store) GetMembership(ctx context.Context, tenantID, userID string) (*Me return &m, nil } +// RevokeMembership deletes a user's membership in a tenant -- the +// counterpart to SetMembership's upsert. Refuses to revoke a tenant's +// current Owner: unlike every other role, Owner is also a dedicated +// tenants.owner_user_id column (see SetOwner's doc comment), so +// revoking that membership without first transferring ownership would +// leave owner_user_id pointing at a user with no membership in the +// tenant at all -- an inconsistent state, not something this method +// silently allows. Ownership transfer is a deliberate, separate action +// (the RBAC matrix's "Transfer tenant Owner -- Owner only"), not a +// side effect of revoking a membership. +func (s *Store) RevokeMembership(ctx context.Context, tenantID, userID string) error { + tenant, err := s.GetTenant(ctx, tenantID) + if err != nil { + return fmt.Errorf("rbacstore: getting tenant to check ownership: %w", err) + } + if tenant.OwnerUserID == userID { + return fmt.Errorf("rbacstore: refusing to revoke tenant %q's current Owner (%s) -- transfer ownership first", tenantID, userID) + } + + tag, err := s.pool.Exec(ctx, `DELETE FROM tenant_memberships WHERE tenant_id = $1 AND user_id = $2`, tenantID, userID) + if err != nil { + return fmt.Errorf("rbacstore: revoking membership: %w", err) + } + if tag.RowsAffected() == 0 { + return ErrNotFound + } + return nil +} + +// TenantMember is one row of ListMembershipsForTenant's result -- joined +// with users so a caller (e.g. enterprise-auth's -list-memberships-tenant +// operator flag) can show something more useful than a bare user ID. +type TenantMember struct { + UserID string + Email string + DisplayName string + Role Role +} + +// ListMembershipsForTenant is ListMembershipsForUser's inverse -- "who +// is in this tenant, and at what role," the shape an admin reviewing or +// revoking access needs. Joined with users (INNER, not LEFT: a +// tenant_memberships row's user_id is NOT NULL and FK-constrained, so +// every membership has a real user). +func (s *Store) ListMembershipsForTenant(ctx context.Context, tenantID string) ([]TenantMember, error) { + rows, err := s.pool.Query(ctx, ` + SELECT u.id, u.email, u.display_name, m.role + FROM tenant_memberships m + JOIN users u ON u.id = m.user_id + WHERE m.tenant_id = $1 + ORDER BY u.email`, tenantID) + if err != nil { + return nil, fmt.Errorf("rbacstore: listing tenant members: %w", err) + } + defer rows.Close() + + var out []TenantMember + for rows.Next() { + var m TenantMember + var role string + if err := rows.Scan(&m.UserID, &m.Email, &m.DisplayName, &role); err != nil { + return nil, fmt.Errorf("rbacstore: scanning tenant member: %w", err) + } + m.Role = Role(role) + out = append(out, m) + } + return out, rows.Err() +} + // ListMembershipsForUser supports "which tenants can this user act in, // and at what role" -- the shape a login/session-issuance handler needs // when a user belongs to more than one tenant and must pick (or be diff --git a/enterprise/internal/rbacstore/rbacstore_test.go b/enterprise/internal/rbacstore/rbacstore_test.go index b1872c8..e166d4b 100644 --- a/enterprise/internal/rbacstore/rbacstore_test.go +++ b/enterprise/internal/rbacstore/rbacstore_test.go @@ -652,3 +652,122 @@ func TestTenantIsActiveNonexistentTenant(t *testing.T) { t.Fatal("a nonexistent tenant must not be reported active") } } + +func TestRevokeMembership(t *testing.T) { + s := testStore(t) + ctx := context.Background() + tenantID := "test-tenant-" + uniqueSuffix() + if _, err := s.CreateTenant(ctx, tenantID, "Test Tenant"); err != nil { + t.Fatalf("CreateTenant: %v", err) + } + user, err := s.UpsertUserBySSO(ctx, "sub-"+uniqueSuffix(), "user-"+uniqueSuffix()+"@example.com", "User") + if err != nil { + t.Fatalf("UpsertUserBySSO: %v", err) + } + if err := s.SetMembership(ctx, tenantID, user.ID, RoleEditor); err != nil { + t.Fatalf("SetMembership: %v", err) + } + + if err := s.RevokeMembership(ctx, tenantID, user.ID); err != nil { + t.Fatalf("RevokeMembership: %v", err) + } + if _, err := s.GetMembership(ctx, tenantID, user.ID); err != ErrNotFound { + t.Fatalf("GetMembership after revoke = %v, want ErrNotFound", err) + } +} + +func TestRevokeMembershipNotFound(t *testing.T) { + s := testStore(t) + ctx := context.Background() + tenantID := "test-tenant-" + uniqueSuffix() + if _, err := s.CreateTenant(ctx, tenantID, "Test Tenant"); err != nil { + t.Fatalf("CreateTenant: %v", err) + } + if err := s.RevokeMembership(ctx, tenantID, uuid.NewString()); err != ErrNotFound { + t.Fatalf("RevokeMembership error = %v, want ErrNotFound", err) + } +} + +// TestRevokeMembershipRefusesCurrentOwner is the regression test for +// RevokeMembership's doc comment: deleting the Owner's membership +// without transferring ownership first would leave tenants.owner_user_id +// pointing at a user with no membership in the tenant at all. +func TestRevokeMembershipRefusesCurrentOwner(t *testing.T) { + s := testStore(t) + ctx := context.Background() + tenantID := "test-tenant-" + uniqueSuffix() + if _, err := s.CreateTenant(ctx, tenantID, "Test Tenant"); err != nil { + t.Fatalf("CreateTenant: %v", err) + } + owner, err := s.UpsertUserBySSO(ctx, "sub-owner-"+uniqueSuffix(), "owner-"+uniqueSuffix()+"@example.com", "Owner") + if err != nil { + t.Fatalf("UpsertUserBySSO: %v", err) + } + if err := s.SetMembership(ctx, tenantID, owner.ID, RoleOwner); err != nil { + t.Fatalf("SetMembership: %v", err) + } + if err := s.SetOwner(ctx, tenantID, owner.ID); err != nil { + t.Fatalf("SetOwner: %v", err) + } + + if err := s.RevokeMembership(ctx, tenantID, owner.ID); err == nil { + t.Fatal("expected RevokeMembership to refuse revoking the tenant's current Owner") + } + if _, err := s.GetMembership(ctx, tenantID, owner.ID); err != nil { + t.Fatalf("owner's membership must still exist after the refused revoke, GetMembership: %v", err) + } +} + +func TestListMembershipsForTenant(t *testing.T) { + s := testStore(t) + ctx := context.Background() + tenantID := "test-tenant-" + uniqueSuffix() + otherTenantID := "test-tenant-" + uniqueSuffix() + if _, err := s.CreateTenant(ctx, tenantID, "Test Tenant"); err != nil { + t.Fatalf("CreateTenant: %v", err) + } + if _, err := s.CreateTenant(ctx, otherTenantID, "Other Tenant"); err != nil { + t.Fatalf("CreateTenant other: %v", err) + } + + viewer, err := s.UpsertUserBySSO(ctx, "sub-viewer-"+uniqueSuffix(), "viewer-"+uniqueSuffix()+"@example.com", "Viewer") + if err != nil { + t.Fatalf("UpsertUserBySSO viewer: %v", err) + } + if err := s.SetMembership(ctx, tenantID, viewer.ID, RoleViewer); err != nil { + t.Fatalf("SetMembership viewer: %v", err) + } + editor, err := s.UpsertUserBySSO(ctx, "sub-editor-"+uniqueSuffix(), "editor-"+uniqueSuffix()+"@example.com", "Editor") + if err != nil { + t.Fatalf("UpsertUserBySSO editor: %v", err) + } + if err := s.SetMembership(ctx, tenantID, editor.ID, RoleEditor); err != nil { + t.Fatalf("SetMembership editor: %v", err) + } + // A membership in a different tenant must not leak into this list. + elsewhere, err := s.UpsertUserBySSO(ctx, "sub-elsewhere-"+uniqueSuffix(), "elsewhere-"+uniqueSuffix()+"@example.com", "Elsewhere") + if err != nil { + t.Fatalf("UpsertUserBySSO elsewhere: %v", err) + } + if err := s.SetMembership(ctx, otherTenantID, elsewhere.ID, RoleAdmin); err != nil { + t.Fatalf("SetMembership elsewhere: %v", err) + } + + members, err := s.ListMembershipsForTenant(ctx, tenantID) + if err != nil { + t.Fatalf("ListMembershipsForTenant: %v", err) + } + if len(members) != 2 { + t.Fatalf("len(members) = %d, want 2", len(members)) + } + byEmail := map[string]TenantMember{} + for _, m := range members { + byEmail[m.Email] = m + } + if got := byEmail[viewer.Email]; got.Role != RoleViewer || got.UserID != viewer.ID { + t.Fatalf("unexpected viewer entry: %+v", got) + } + if got := byEmail[editor.Email]; got.Role != RoleEditor || got.UserID != editor.ID { + t.Fatalf("unexpected editor entry: %+v", got) + } +}