From 243f4dc2aba93c8e05ba55ec92998ab6f0730b3d Mon Sep 17 00:00:00 2001 From: John Coffey Date: Fri, 14 Aug 2026 07:11:18 -0700 Subject: [PATCH] Enforce per-resource dashboard grants (RBAC matrix's own/granted qualifier) api/dashboards' handler previously enforced only tenant-baseline role (RoleEditor+), so any Editor could edit/delete any dashboard in their tenant -- the matrix's "(own/granted)" qualifier was explicitly named as unbuilt in this handler's own doc comment. This closes that gap. New core interface api/dashboards.PermissionStore (nil-safe, same "not wired == no-op" shape as authz.Authorizer) resolves a per-resource dashboard_permissions grant. canEditDashboard now requires the identity be Admin/Owner, the dashboard's creator, or hold a grant of at least Editor; canManageGrants is deliberately stricter (creator or Admin/Owner only, never grant-derived access) so a user who can edit a dashboard only because of a grant can't extend or re-grant that access to themselves or others. Wired handlers: PUT/DELETE /dashboards/{id}/permissions/{userId}, GET .../permissions. Two real bugs found and fixed while wiring this up, before any of it touched a live database: - handleCreate/handleImport never stamped created_by from the authenticated identity, so every dashboard was owned by "anonymous" regardless of who made it -- the ownership check would have been meaningless. Also fixed: ImportDashboard trusted the exported JSON's created_by verbatim, so re-importing someone else's export would leave the actual importer unable to edit their own copy. - metadata/migrations/0024_create_dashboard_permissions.sql's CHECK constraint diverged from /docs/phase-4-rbac-design.md's schema (allowed role='admin', nullable granted_by). Reconciled via 0033_restrict_dashboard_permissions_role.sql: Admin/Owner already have tenant-wide access so a resource-level "admin" grant is meaningless, and every real grant now always has an attributable granter. enterprise/internal/rbacstore gets the storage side: raw CRUD (dashboard_permissions.go) plus DashboardPermissions (dashboards_adapter.go), an adapter implementing api/dashboards.PermissionStore -- same pattern as audit.QueryAPILogger over queryapi.AuditLogger. Wired into enterprise/cmd/enterprise-api only; plain api/cmd/api passes nil (ownership/Admin checks still work via the nil-permissions fallback, just without the "granted" bonus). Verified: the full own/granted/admin/creator matrix, including the granted-editor-cannot-manage-grants regression, passes against a fake PermissionStore (api/dashboards/handler_test.go, all existing tests also still pass unmodified in behavior). Real integration tests exist in enterprise/internal/rbacstore/rbacstore_test.go (skip-gated on RBACSTORE_TEST_POSTGRES_ADDR, same convention as every other Postgres-backed piece this phase) but have not run against a live database in this environment -- disclosed in threat-model.md, phase-4-runbook.md, and enterprise/README.md alongside every other piece carrying the same gap. Also fixed a stale path in phase-4-runbook.md's dashboards-tenant-scoping section (./internal/dashboards/... -> ./dashboards/..., stale since that package moved out of api/internal/ earlier in this phase). --- CLAUDE.md | 12 +- api/cmd/api/main.go | 8 +- api/dashboards/handler.go | 242 +++++++++++++++-- api/dashboards/handler_test.go | 254 +++++++++++++++++- api/dashboards/permissions.go | 53 ++++ docs/phase-4-rbac-design.md | 12 + docs/phase-4-runbook.md | 42 ++- docs/security/threat-model.md | 33 ++- enterprise/README.md | 34 ++- enterprise/cmd/enterprise-api/main.go | 2 +- .../rbacstore/dashboard_permissions.go | 96 +++++++ .../internal/rbacstore/dashboards_adapter.go | 65 +++++ enterprise/internal/rbacstore/rbacstore.go | 20 +- .../internal/rbacstore/rbacstore_test.go | 231 ++++++++++++++++ ...33_restrict_dashboard_permissions_role.sql | 14 + 15 files changed, 1064 insertions(+), 54 deletions(-) create mode 100644 api/dashboards/permissions.go create mode 100644 enterprise/internal/rbacstore/dashboard_permissions.go create mode 100644 enterprise/internal/rbacstore/dashboards_adapter.go create mode 100644 metadata/migrations/0033_restrict_dashboard_permissions_role.sql diff --git a/CLAUDE.md b/CLAUDE.md index 1fc6ac0..77a10d3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -181,7 +181,17 @@ RBAC/audit/SSO, rendering to the same Service name/port either way — a Helm-deployed cluster can't accidentally run the wrong one. `docker-compose.yml` still runs plain `api` unconditionally, though (local/dev parity with the Helm chart's enforcement is real remaining -work). What still keeps this phase from being done: ingest itself has no +work). Per-resource dashboard grants (the RBAC matrix's "(own/granted)" +qualifier) are now enforced too: `api/dashboards.PermissionStore` (core +interface) implemented by `enterprise/internal/rbacstore. +DashboardPermissions`, wired in only by `enterprise-api` — an Editor can +now only edit/delete a dashboard they created or were granted access to, +not every dashboard in their tenant; managing grants themselves is +stricter still (creator/Admin/Owner only, closing a self-escalation +path). Verified against a fake store (`api/dashboards/handler_test.go`); +real integration tests exist but haven't run against a live Postgres, +same disclosed gap as the rest of this phase's Postgres-backed pieces. +What still keeps this phase from being done: ingest itself has no tenant concept for either storage engine (every record lands in the one shared ClickHouse database and Tantivy index no matter what — undesigned, not just unbuilt), and the two diff --git a/api/cmd/api/main.go b/api/cmd/api/main.go index e4add2f..03cfd36 100644 --- a/api/cmd/api/main.go +++ b/api/cmd/api/main.go @@ -101,7 +101,13 @@ func main() { // audit logging is nil (a no-op) until Phase 4 task 5 wires in // enterprise/internal/audit -- see queryapi.AuditLogger's doc comment. queryHandler := queryapi.NewHandler(logger, sqlRunner, search, cfg.QueryTimeout, nil, authorizer) - dashboardsHandler := dashboards.NewHandler(logger, dashboards.NewStore(pgPool), authorizer) + // permissions is nil -- api/cmd/api is single-tenant/core; the + // enterprise-supplied dashboard_permissions store is only wired in + // by enterprise/cmd/enterprise-api (see dashboards.PermissionStore's + // doc comment). Ownership/Admin access still work via + // canEditDashboard's nil-permissions fallback -- only the "granted" + // half of the matrix's "(own/granted)" qualifier is unavailable here. + dashboardsHandler := dashboards.NewHandler(logger, dashboards.NewStore(pgPool), authorizer, nil) // One shared mux, CORS applied once around the whole thing -- see // httpserver's doc comment for why this changed from each diff --git a/api/dashboards/handler.go b/api/dashboards/handler.go index f8996dd..42fe2c5 100644 --- a/api/dashboards/handler.go +++ b/api/dashboards/handler.go @@ -27,26 +27,23 @@ type store interface { } type Handler struct { - logger *slog.Logger - store store - authorizer authz.Authorizer + logger *slog.Logger + store store + authorizer authz.Authorizer + permissions PermissionStore } -func NewHandler(logger *slog.Logger, store store, authorizer authz.Authorizer) *Handler { - return &Handler{logger: logger, store: store, authorizer: authorizer} +func NewHandler(logger *slog.Logger, store store, authorizer authz.Authorizer, permissions PermissionStore) *Handler { + return &Handler{logger: logger, store: store, authorizer: authorizer, permissions: permissions} } // RegisterRoutes wires the RBAC minimum-role bar from -// /docs/phase-4-rbac-design.md's matrix. Note what's NOT enforced here -// yet: the matrix's "(own/granted)" qualifier for Editor create/edit/ -// delete requires the dashboard_permissions/ownership lookup that -// enterprise/internal/rbacstore hasn't been built yet -- until then, -// RoleEditor is necessary but not sufficient per the matrix, and every -// Editor can act on every dashboard *within their own tenant* (tenant -// scoping itself -- a different, more basic property than the -// per-resource "(own/granted)" qualifier -- is enforced, via tenantID -// below and store.go's WHERE tenant_id = ... filtering). Tracked as -// follow-up, not silently dropped. +// /docs/phase-4-rbac-design.md's matrix, plus (as of Phase 4 task 5) the +// matrix's "(own/granted)" qualifier: RoleEditor is necessary but not +// sufficient for editing/deleting a dashboard or its panels -- see +// canEditDashboard. Tenant scoping itself -- a different, more basic +// property than the per-resource qualifier -- is enforced separately, +// via tenantID below and store.go's WHERE tenant_id = ... filtering. func (h *Handler) RegisterRoutes(mux *http.ServeMux) { mux.HandleFunc("POST /dashboards", authz.RequireRole(h.authorizer, authz.RoleEditor, h.handleCreate)) mux.HandleFunc("GET /dashboards", authz.RequireRole(h.authorizer, authz.RoleViewer, h.handleList)) @@ -58,6 +55,60 @@ func (h *Handler) RegisterRoutes(mux *http.ServeMux) { mux.HandleFunc("POST /dashboards/{id}/panels", authz.RequireRole(h.authorizer, authz.RoleEditor, h.handleAddPanel)) mux.HandleFunc("PUT /dashboards/{id}/panels/{panelId}", authz.RequireRole(h.authorizer, authz.RoleEditor, h.handleUpdatePanel)) mux.HandleFunc("DELETE /dashboards/{id}/panels/{panelId}", authz.RequireRole(h.authorizer, authz.RoleEditor, h.handleDeletePanel)) + mux.HandleFunc("GET /dashboards/{id}/permissions", authz.RequireRole(h.authorizer, authz.RoleEditor, h.handleListPermissions)) + mux.HandleFunc("PUT /dashboards/{id}/permissions/{userId}", authz.RequireRole(h.authorizer, authz.RoleEditor, h.handleSetPermission)) + mux.HandleFunc("DELETE /dashboards/{id}/permissions/{userId}", authz.RequireRole(h.authorizer, authz.RoleEditor, h.handleRevokePermission)) +} + +// canEditDashboard reports whether the request's authenticated identity +// may edit/delete d or its panels, per the matrix's Editor "(own/ +// granted)" qualifier: Admin/Owner may act on any dashboard in their +// tenant (RequireRole already gated the RoleEditor floor); a plain +// Editor may act only on a dashboard they created, or one where a +// dashboard_permissions grant raises their effective role to at least +// Editor. A nil authorizer is the same no-op RequireRole already treats +// specially -- a single-tenant deployment has no identity to check +// ownership against, so this must not start rejecting Phase 0-3 +// requests that never carried one. A nil permissions store (RBAC +// enforced, but no enterprise permission service wired -- e.g. plain +// api/cmd/api with ENTERPRISE_AUTH_URL set) still enforces own/Admin, +// just without the "granted" bonus. +func (h *Handler) canEditDashboard(ctx context.Context, d *Dashboard) bool { + if h.authorizer == nil { + return true + } + identity, ok := authz.IdentityFromContext(ctx) + if !ok { + return false + } + if identity.Role.Satisfies(authz.RoleAdmin) { + return true + } + if identity.UserID != "" && identity.UserID == d.CreatedBy { + return true + } + if h.permissions == nil { + return false + } + role, granted, err := h.permissions.GrantedRole(ctx, d.ID, identity.UserID) + return err == nil && granted && role.Satisfies(authz.RoleEditor) +} + +// canManageGrants is deliberately stricter than canEditDashboard: the +// matrix lists "manage a dashboard's per-user grants" as available to +// the creator or Admin/Owner only ("if creator", not "own/granted") -- +// a user who themselves only has access *via* a grant must not be able +// to extend that access to others or re-grant themselves a persistent +// one, which a grant-based bypass here would allow. +func (h *Handler) canManageGrants(ctx context.Context, d *Dashboard) bool { + if h.authorizer == nil { + return true + } + identity, ok := authz.IdentityFromContext(ctx) + if !ok { + return false + } + return identity.Role.Satisfies(authz.RoleAdmin) || (identity.UserID != "" && identity.UserID == d.CreatedBy) } const maxBodyBytes = 1 << 20 // 1 MiB, same cap as queryapi @@ -80,6 +131,18 @@ func (h *Handler) tenantID(r *http.Request) string { return "default" } +// createdBy resolves the authenticated identity's UserID to stamp onto a +// newly-created dashboard, exactly like tenantID resolves TenantID -- +// empty (falling back to store.go's "anonymous" default) when no +// identity is present, never from client-supplied JSON. This is what +// canEditDashboard's ownership check compares against later. +func (h *Handler) createdBy(r *http.Request) string { + if id, ok := authz.IdentityFromContext(r.Context()); ok { + return id.UserID + } + return "" +} + func (h *Handler) handleCreate(w http.ResponseWriter, r *http.Request) { var d Dashboard if !decodeJSON(w, r, &d) { @@ -89,8 +152,11 @@ func (h *Handler) handleCreate(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusBadRequest, "name must not be empty") return } - // Overrides any client-supplied tenant_id -- see tenantID's doc comment. + // Overrides any client-supplied tenant_id/created_by -- see + // tenantID's doc comment; same reasoning applies to created_by, + // which canEditDashboard later trusts for the ownership check. d.TenantID = h.tenantID(r) + d.CreatedBy = h.createdBy(r) if err := h.store.CreateDashboard(r.Context(), &d); err != nil { h.logger.Error("creating dashboard", "error", err) writeError(w, http.StatusInternalServerError, "creating dashboard failed") @@ -135,6 +201,15 @@ func (h *Handler) handleImport(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusBadRequest, "name must not be empty") return } + // Overrides any created_by the exported JSON carries -- store.go's + // ImportDashboard trusts d.CreatedBy verbatim (unlike TenantID, + // which it always overrides from the separate tenantID argument), + // so without this an imported dashboard would keep its *original* + // creator's ID, leaving the actual importer unable to edit their own + // freshly-imported copy once canEditDashboard's ownership check + // applies -- the same reasoning TestImportIgnoresExportedTenantID + // already established for tenant_id extends to created_by. + d.CreatedBy = h.createdBy(r) imported, err := h.store.ImportDashboard(r.Context(), h.tenantID(r), &d) if err != nil { h.logger.Error("importing dashboard", "error", err) @@ -154,6 +229,15 @@ func (h *Handler) handleUpdate(w http.ResponseWriter, r *http.Request) { return } d.ID = r.PathValue("id") + existing, err := h.store.GetDashboard(r.Context(), h.tenantID(r), d.ID) + if err != nil { + h.writeStoreErr(w, err, "fetching dashboard") + return + } + if !h.canEditDashboard(r.Context(), existing) { + writeError(w, http.StatusForbidden, "forbidden") + return + } if err := h.store.UpdateDashboard(r.Context(), h.tenantID(r), &d); err != nil { h.writeStoreErr(w, err, "updating dashboard") return @@ -162,7 +246,17 @@ func (h *Handler) handleUpdate(w http.ResponseWriter, r *http.Request) { } func (h *Handler) handleDelete(w http.ResponseWriter, r *http.Request) { - if err := h.store.DeleteDashboard(r.Context(), h.tenantID(r), r.PathValue("id")); err != nil { + id := r.PathValue("id") + existing, err := h.store.GetDashboard(r.Context(), h.tenantID(r), id) + if err != nil { + h.writeStoreErr(w, err, "fetching dashboard") + return + } + if !h.canEditDashboard(r.Context(), existing) { + writeError(w, http.StatusForbidden, "forbidden") + return + } + if err := h.store.DeleteDashboard(r.Context(), h.tenantID(r), id); err != nil { h.writeStoreErr(w, err, "deleting dashboard") return } @@ -178,7 +272,17 @@ func (h *Handler) handleAddPanel(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusBadRequest, err.Error()) return } - if err := h.store.AddPanel(r.Context(), h.tenantID(r), r.PathValue("id"), &p); err != nil { + dashboardID := r.PathValue("id") + existing, err := h.store.GetDashboard(r.Context(), h.tenantID(r), dashboardID) + if err != nil { + h.writeStoreErr(w, err, "fetching dashboard") + return + } + if !h.canEditDashboard(r.Context(), existing) { + writeError(w, http.StatusForbidden, "forbidden") + return + } + if err := h.store.AddPanel(r.Context(), h.tenantID(r), dashboardID, &p); err != nil { h.writeStoreErr(w, err, "adding panel") return } @@ -196,6 +300,15 @@ func (h *Handler) handleUpdatePanel(w http.ResponseWriter, r *http.Request) { } p.ID = r.PathValue("panelId") p.DashboardID = r.PathValue("id") + existing, err := h.store.GetDashboard(r.Context(), h.tenantID(r), p.DashboardID) + if err != nil { + h.writeStoreErr(w, err, "fetching dashboard") + return + } + if !h.canEditDashboard(r.Context(), existing) { + writeError(w, http.StatusForbidden, "forbidden") + return + } if err := h.store.UpdatePanel(r.Context(), h.tenantID(r), &p); err != nil { h.writeStoreErr(w, err, "updating panel") return @@ -204,13 +317,104 @@ func (h *Handler) handleUpdatePanel(w http.ResponseWriter, r *http.Request) { } func (h *Handler) handleDeletePanel(w http.ResponseWriter, r *http.Request) { - if err := h.store.DeletePanel(r.Context(), h.tenantID(r), r.PathValue("id"), r.PathValue("panelId")); err != nil { + dashboardID := r.PathValue("id") + existing, err := h.store.GetDashboard(r.Context(), h.tenantID(r), dashboardID) + if err != nil { + h.writeStoreErr(w, err, "fetching dashboard") + return + } + if !h.canEditDashboard(r.Context(), existing) { + writeError(w, http.StatusForbidden, "forbidden") + return + } + if err := h.store.DeletePanel(r.Context(), h.tenantID(r), dashboardID, r.PathValue("panelId")); err != nil { h.writeStoreErr(w, err, "deleting panel") return } w.WriteHeader(http.StatusNoContent) } +func (h *Handler) handleListPermissions(w http.ResponseWriter, r *http.Request) { + existing, err := h.store.GetDashboard(r.Context(), h.tenantID(r), r.PathValue("id")) + if err != nil { + h.writeStoreErr(w, err, "fetching dashboard") + return + } + if !h.canManageGrants(r.Context(), existing) { + writeError(w, http.StatusForbidden, "forbidden") + return + } + if h.permissions == nil { + writeError(w, http.StatusNotImplemented, "dashboard permission grants are not available on this deployment") + return + } + perms, err := h.permissions.ListPermissions(r.Context(), existing.ID) + if err != nil { + h.logger.Error("listing dashboard permissions", "error", err) + writeError(w, http.StatusInternalServerError, "listing permissions failed") + return + } + writeJSON(w, http.StatusOK, perms) +} + +type setPermissionRequest struct { + Role string `json:"role"` +} + +func (h *Handler) handleSetPermission(w http.ResponseWriter, r *http.Request) { + existing, err := h.store.GetDashboard(r.Context(), h.tenantID(r), r.PathValue("id")) + if err != nil { + h.writeStoreErr(w, err, "fetching dashboard") + return + } + if !h.canManageGrants(r.Context(), existing) { + writeError(w, http.StatusForbidden, "forbidden") + return + } + if h.permissions == nil { + writeError(w, http.StatusNotImplemented, "dashboard permission grants are not available on this deployment") + return + } + var body setPermissionRequest + if !decodeJSON(w, r, &body) { + return + } + role := authz.Role(body.Role) + if !validGrantRole(role) { + writeError(w, http.StatusBadRequest, "role must be \"viewer\" or \"editor\"") + return + } + identity, _ := authz.IdentityFromContext(r.Context()) + if err := h.permissions.SetPermission(r.Context(), existing.ID, r.PathValue("userId"), role, identity.UserID); err != nil { + h.logger.Error("setting dashboard permission", "error", err) + writeError(w, http.StatusInternalServerError, "granting permission failed") + return + } + w.WriteHeader(http.StatusNoContent) +} + +func (h *Handler) handleRevokePermission(w http.ResponseWriter, r *http.Request) { + existing, err := h.store.GetDashboard(r.Context(), h.tenantID(r), r.PathValue("id")) + if err != nil { + h.writeStoreErr(w, err, "fetching dashboard") + return + } + if !h.canManageGrants(r.Context(), existing) { + writeError(w, http.StatusForbidden, "forbidden") + return + } + if h.permissions == nil { + writeError(w, http.StatusNotImplemented, "dashboard permission grants are not available on this deployment") + return + } + if err := h.permissions.RevokePermission(r.Context(), existing.ID, r.PathValue("userId")); err != nil { + h.logger.Error("revoking dashboard permission", "error", err) + writeError(w, http.StatusInternalServerError, "revoking permission failed") + return + } + w.WriteHeader(http.StatusNoContent) +} + func (h *Handler) writeStoreErr(w http.ResponseWriter, err error, action string) { if errors.Is(err, ErrNotFound) { writeError(w, http.StatusNotFound, "not found") diff --git a/api/dashboards/handler_test.go b/api/dashboards/handler_test.go index 1871e1b..3f64d0c 100644 --- a/api/dashboards/handler_test.go +++ b/api/dashboards/handler_test.go @@ -128,7 +128,7 @@ func (f *fakeStore) ImportDashboard(_ context.Context, tenantID string, d *Dashb } func newTestMux(fs *fakeStore) *http.ServeMux { - h := NewHandler(slog.New(slog.NewTextHandler(io.Discard, nil)), fs, nil) + h := NewHandler(slog.New(slog.NewTextHandler(io.Discard, nil)), fs, nil, nil) mux := http.NewServeMux() h.RegisterRoutes(mux) return mux @@ -146,9 +146,61 @@ func (f *fakeAuthorizer) Authorize(*http.Request) (authz.Identity, error) { return f.identity, nil } +// fakePermissionStore is an in-memory PermissionStore, keyed by +// dashboardID -> userID -> grant, enough to drive canEditDashboard/ +// canManageGrants's "granted" path and the permission-management +// handlers without a real Postgres. +type fakePermissionStore struct { + grants map[string]map[string]Permission +} + +func newFakePermissionStore() *fakePermissionStore { + return &fakePermissionStore{grants: map[string]map[string]Permission{}} +} + +func (f *fakePermissionStore) GrantedRole(_ context.Context, dashboardID, userID string) (authz.Role, bool, error) { + p, ok := f.grants[dashboardID][userID] + if !ok { + return "", false, nil + } + return p.Role, true, nil +} + +func (f *fakePermissionStore) SetPermission(_ context.Context, dashboardID, userID string, role authz.Role, grantedBy string) error { + if f.grants[dashboardID] == nil { + f.grants[dashboardID] = map[string]Permission{} + } + f.grants[dashboardID][userID] = Permission{UserID: userID, Role: role, GrantedBy: grantedBy} + return nil +} + +func (f *fakePermissionStore) RevokePermission(_ context.Context, dashboardID, userID string) error { + delete(f.grants[dashboardID], userID) + return nil +} + +func (f *fakePermissionStore) ListPermissions(_ context.Context, dashboardID string) ([]Permission, error) { + var out []Permission + for _, p := range f.grants[dashboardID] { + out = append(out, p) + } + return out, nil +} + func newTestMuxWithTenant(fs *fakeStore, tenantID string) *http.ServeMux { h := NewHandler(slog.New(slog.NewTextHandler(io.Discard, nil)), fs, - &fakeAuthorizer{identity: authz.Identity{TenantID: tenantID, Role: authz.RoleOwner}}) + &fakeAuthorizer{identity: authz.Identity{TenantID: tenantID, Role: authz.RoleOwner}}, nil) + mux := http.NewServeMux() + h.RegisterRoutes(mux) + return mux +} + +// newTestMuxWithIdentity is newTestMuxWithTenant's more general sibling +// -- lets ownership/grant tests control Role and UserID directly (Owner +// always bypasses canEditDashboard/canManageGrants, so those tests can't +// use newTestMuxWithTenant), and optionally wires a PermissionStore. +func newTestMuxWithIdentity(fs *fakeStore, identity authz.Identity, permissions PermissionStore) *http.ServeMux { + h := NewHandler(slog.New(slog.NewTextHandler(io.Discard, nil)), fs, &fakeAuthorizer{identity: identity}, permissions) mux := http.NewServeMux() h.RegisterRoutes(mux) return mux @@ -444,7 +496,7 @@ func TestServiceIdentityCannotAccessDashboards(t *testing.T) { fs := newFakeStore() fs.dashboards["dash-1"] = &Dashboard{ID: "dash-1", TenantID: "default", Name: "Overview"} h := NewHandler(slog.New(slog.NewTextHandler(io.Discard, nil)), fs, - &fakeAuthorizer{identity: authz.Identity{Role: authz.RoleService}}) + &fakeAuthorizer{identity: authz.Identity{Role: authz.RoleService}}, nil) mux := http.NewServeMux() h.RegisterRoutes(mux) @@ -462,3 +514,199 @@ func TestServiceIdentityCannotAccessDashboards(t *testing.T) { } } } + +// --- "(own/granted)" qualifier tests (Phase 4 task 5) --- + +func TestEditorCannotEditOthersDashboardWithoutGrant(t *testing.T) { + fs := newFakeStore() + fs.dashboards["dash-1"] = &Dashboard{ID: "dash-1", TenantID: "acme", Name: "Overview", CreatedBy: "user-owner"} + mux := newTestMuxWithIdentity(fs, authz.Identity{TenantID: "acme", UserID: "user-editor", Role: authz.RoleEditor}, newFakePermissionStore()) + + rec := doRequest(t, mux, http.MethodPut, "/dashboards/dash-1", `{"name": "Hijacked"}`) + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403; body=%s", rec.Code, rec.Body.String()) + } + if fs.dashboards["dash-1"].Name != "Overview" { + t.Fatalf("must not modify the dashboard, got Name = %q", fs.dashboards["dash-1"].Name) + } +} + +func TestEditorCanEditOwnDashboard(t *testing.T) { + fs := newFakeStore() + fs.dashboards["dash-1"] = &Dashboard{ID: "dash-1", TenantID: "acme", Name: "Overview", CreatedBy: "user-editor"} + mux := newTestMuxWithIdentity(fs, authz.Identity{TenantID: "acme", UserID: "user-editor", Role: authz.RoleEditor}, newFakePermissionStore()) + + rec := doRequest(t, mux, http.MethodPut, "/dashboards/dash-1", `{"name": "Updated"}`) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", rec.Code, rec.Body.String()) + } +} + +func TestEditorCanEditDashboardGrantedEditorRole(t *testing.T) { + fs := newFakeStore() + fs.dashboards["dash-1"] = &Dashboard{ID: "dash-1", TenantID: "acme", Name: "Overview", CreatedBy: "user-owner"} + perms := newFakePermissionStore() + if err := perms.SetPermission(context.Background(), "dash-1", "user-editor", authz.RoleEditor, "user-owner"); err != nil { + t.Fatalf("seeding grant: %v", err) + } + mux := newTestMuxWithIdentity(fs, authz.Identity{TenantID: "acme", UserID: "user-editor", Role: authz.RoleEditor}, perms) + + rec := doRequest(t, mux, http.MethodPut, "/dashboards/dash-1", `{"name": "Updated"}`) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (a granted Editor role must allow editing someone else's dashboard); body=%s", rec.Code, rec.Body.String()) + } +} + +func TestEditorCannotEditDashboardGrantedViewerRoleOnly(t *testing.T) { + fs := newFakeStore() + fs.dashboards["dash-1"] = &Dashboard{ID: "dash-1", TenantID: "acme", Name: "Overview", CreatedBy: "user-owner"} + perms := newFakePermissionStore() + if err := perms.SetPermission(context.Background(), "dash-1", "user-editor", authz.RoleViewer, "user-owner"); err != nil { + t.Fatalf("seeding grant: %v", err) + } + mux := newTestMuxWithIdentity(fs, authz.Identity{TenantID: "acme", UserID: "user-editor", Role: authz.RoleEditor}, perms) + + rec := doRequest(t, mux, http.MethodPut, "/dashboards/dash-1", `{"name": "Updated"}`) + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403 (a Viewer-level grant must not allow editing -- additive-only, not an escalation)", rec.Code) + } +} + +func TestAdminCanEditAnyDashboardWithoutGrant(t *testing.T) { + fs := newFakeStore() + fs.dashboards["dash-1"] = &Dashboard{ID: "dash-1", TenantID: "acme", Name: "Overview", CreatedBy: "user-owner"} + mux := newTestMuxWithIdentity(fs, authz.Identity{TenantID: "acme", UserID: "user-admin", Role: authz.RoleAdmin}, newFakePermissionStore()) + + rec := doRequest(t, mux, http.MethodDelete, "/dashboards/dash-1", "") + if rec.Code != http.StatusNoContent { + t.Fatalf("status = %d, want 204 (Admin may act on any dashboard in its tenant); body=%s", rec.Code, rec.Body.String()) + } +} + +func TestEditorCannotEditPanelsOfOthersDashboardWithoutGrant(t *testing.T) { + fs := newFakeStore() + fs.dashboards["dash-1"] = &Dashboard{ID: "dash-1", TenantID: "acme", Name: "Overview", CreatedBy: "user-owner"} + mux := newTestMuxWithIdentity(fs, authz.Identity{TenantID: "acme", UserID: "user-editor", Role: authz.RoleEditor}, newFakePermissionStore()) + + rec := doRequest(t, mux, http.MethodPost, "/dashboards/dash-1/panels", `{"query": "service=api", "viz_type": "table"}`) + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403; body=%s", rec.Code, rec.Body.String()) + } + if len(fs.dashboards["dash-1"].Panels) != 0 { + t.Fatalf("must not attach a panel without edit access") + } +} + +func TestNoPermissionStoreFallsBackToOwnershipOnly(t *testing.T) { + fs := newFakeStore() + fs.dashboards["dash-1"] = &Dashboard{ID: "dash-1", TenantID: "acme", Name: "Overview", CreatedBy: "user-owner"} + // authorizer is real (RBAC enforced) but permissions is nil -- e.g. + // plain api/cmd/api with ENTERPRISE_AUTH_URL set but no enterprise + // permission service wired. Own/Admin must still work; "granted" + // simply isn't checkable. + mux := newTestMuxWithIdentity(fs, authz.Identity{TenantID: "acme", UserID: "user-editor", Role: authz.RoleEditor}, nil) + + rec := doRequest(t, mux, http.MethodPut, "/dashboards/dash-1", `{"name": "Updated"}`) + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403 (no permission store means no 'granted' bonus, and this user isn't the creator)", rec.Code) + } +} + +// --- grant management endpoint tests --- + +func TestCreatorCanManageGrants(t *testing.T) { + fs := newFakeStore() + fs.dashboards["dash-1"] = &Dashboard{ID: "dash-1", TenantID: "acme", Name: "Overview", CreatedBy: "user-creator"} + perms := newFakePermissionStore() + mux := newTestMuxWithIdentity(fs, authz.Identity{TenantID: "acme", UserID: "user-creator", Role: authz.RoleEditor}, perms) + + rec := doRequest(t, mux, http.MethodPut, "/dashboards/dash-1/permissions/user-2", `{"role": "editor"}`) + if rec.Code != http.StatusNoContent { + t.Fatalf("status = %d, want 204; body=%s", rec.Code, rec.Body.String()) + } + role, ok, err := perms.GrantedRole(context.Background(), "dash-1", "user-2") + if err != nil || !ok || role != authz.RoleEditor { + t.Fatalf("GrantedRole = (%v, %v, %v), want (editor, true, nil)", role, ok, err) + } +} + +// TestGrantedEditorCannotManageGrants is the regression test for the +// matrix's distinction between "(own/granted)" (edit/delete content) and +// "(if creator)" (manage grants) -- a user who can edit only because of +// a grant must not be able to extend or re-grant that access to anyone, +// including themselves. +func TestGrantedEditorCannotManageGrants(t *testing.T) { + fs := newFakeStore() + fs.dashboards["dash-1"] = &Dashboard{ID: "dash-1", TenantID: "acme", Name: "Overview", CreatedBy: "user-owner"} + perms := newFakePermissionStore() + if err := perms.SetPermission(context.Background(), "dash-1", "user-editor", authz.RoleEditor, "user-owner"); err != nil { + t.Fatalf("seeding grant: %v", err) + } + mux := newTestMuxWithIdentity(fs, authz.Identity{TenantID: "acme", UserID: "user-editor", Role: authz.RoleEditor}, perms) + + rec := doRequest(t, mux, http.MethodPut, "/dashboards/dash-1/permissions/user-3", `{"role": "editor"}`) + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403 (a granted Editor must not be able to grant access to others)", rec.Code) + } +} + +func TestSetPermissionRejectsInvalidRole(t *testing.T) { + fs := newFakeStore() + fs.dashboards["dash-1"] = &Dashboard{ID: "dash-1", TenantID: "acme", Name: "Overview", CreatedBy: "user-creator"} + mux := newTestMuxWithIdentity(fs, authz.Identity{TenantID: "acme", UserID: "user-creator", Role: authz.RoleEditor}, newFakePermissionStore()) + + rec := doRequest(t, mux, http.MethodPut, "/dashboards/dash-1/permissions/user-2", `{"role": "admin"}`) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400 (admin is not a grantable resource-level role -- see metadata/migrations/0033)", rec.Code) + } +} + +func TestRevokePermission(t *testing.T) { + fs := newFakeStore() + fs.dashboards["dash-1"] = &Dashboard{ID: "dash-1", TenantID: "acme", Name: "Overview", CreatedBy: "user-creator"} + perms := newFakePermissionStore() + if err := perms.SetPermission(context.Background(), "dash-1", "user-2", authz.RoleEditor, "user-creator"); err != nil { + t.Fatalf("seeding grant: %v", err) + } + mux := newTestMuxWithIdentity(fs, authz.Identity{TenantID: "acme", UserID: "user-creator", Role: authz.RoleEditor}, perms) + + rec := doRequest(t, mux, http.MethodDelete, "/dashboards/dash-1/permissions/user-2", "") + if rec.Code != http.StatusNoContent { + t.Fatalf("status = %d, want 204; body=%s", rec.Code, rec.Body.String()) + } + if _, ok, _ := perms.GrantedRole(context.Background(), "dash-1", "user-2"); ok { + t.Fatalf("expected the grant to be revoked") + } +} + +func TestPermissionEndpointsReturn501WithoutPermissionStore(t *testing.T) { + fs := newFakeStore() + fs.dashboards["dash-1"] = &Dashboard{ID: "dash-1", TenantID: "acme", Name: "Overview", CreatedBy: "user-creator"} + mux := newTestMuxWithIdentity(fs, authz.Identity{TenantID: "acme", UserID: "user-creator", Role: authz.RoleEditor}, nil) + + rec := doRequest(t, mux, http.MethodPut, "/dashboards/dash-1/permissions/user-2", `{"role": "editor"}`) + if rec.Code != http.StatusNotImplemented { + t.Fatalf("status = %d, want 501 (no enterprise permission service wired)", rec.Code) + } +} + +// TestImportStampsImportingUserAsCreator is the regression test for the +// created_by-on-import fix: without it, an imported dashboard would keep +// the *exported* JSON's created_by, leaving the actual importer unable +// to edit their own freshly-imported copy. +func TestImportStampsImportingUserAsCreator(t *testing.T) { + fs := newFakeStore() + mux := newTestMuxWithIdentity(fs, authz.Identity{TenantID: "acme", UserID: "user-importer", Role: authz.RoleEditor}, newFakePermissionStore()) + + rec := doRequest(t, mux, http.MethodPost, "/dashboards/import", `{"name": "Imported", "created_by": "someone-else"}`) + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201; body=%s", rec.Code, rec.Body.String()) + } + var imported Dashboard + if err := json.Unmarshal(rec.Body.Bytes(), &imported); err != nil { + t.Fatalf("decoding response: %v", err) + } + if imported.CreatedBy != "user-importer" { + t.Fatalf("CreatedBy = %q, want %q (the importing identity, not the exported JSON's value)", imported.CreatedBy, "user-importer") + } +} diff --git a/api/dashboards/permissions.go b/api/dashboards/permissions.go new file mode 100644 index 0000000..8061427 --- /dev/null +++ b/api/dashboards/permissions.go @@ -0,0 +1,53 @@ +package dashboards + +import ( + "context" + "time" + + "github.com/sentry/sentry/api/authz" +) + +// Permission is one dashboard_permissions row -- see +// /docs/phase-4-rbac-design.md's "additive-only per-resource grants" +// section. Role is always RoleViewer or RoleEditor: Admin/Owner already +// have tenant-wide access to every dashboard, so a resource-level grant +// only ever needs to raise someone as high as Editor on one dashboard +// (metadata/migrations/0033_restrict_dashboard_permissions_role.sql). +type Permission struct { + UserID string + Role authz.Role + GrantedBy string + CreatedAt time.Time +} + +// PermissionStore resolves and manages per-resource dashboard grants -- +// the RBAC matrix's "(own/granted)" qualifier that a baseline tenant +// role alone can't answer (see RegisterRoutes' doc comment). nil is a +// deliberate no-op, the same shape as a nil authz.Authorizer: a +// single-tenant deployment, or one running plain api/cmd/api with RBAC +// enforcement on but no enterprise permission service wired, simply +// doesn't get the "granted" half of "(own/granted)" -- ownership and +// Admin/Owner access still work (see canEditDashboard), only grants +// beyond that are unavailable. The real implementation +// (enterprise/internal/rbacstore.DashboardPermissions) is enterprise +// code, wired in only by enterprise/cmd/enterprise-api -- core never +// imports it, per the module boundary. +type PermissionStore interface { + // GrantedRole returns the role a specific user has been granted on a + // specific dashboard, ok=false if no grant exists. + GrantedRole(ctx context.Context, dashboardID, userID string) (role authz.Role, ok bool, err error) + // SetPermission creates or updates a grant. grantedBy is the + // authenticated identity performing the grant -- always recorded, + // never empty, so every grant is attributable (see the migration + // above's NOT NULL fix). + SetPermission(ctx context.Context, dashboardID, userID string, role authz.Role, grantedBy string) error + RevokePermission(ctx context.Context, dashboardID, userID string) error + ListPermissions(ctx context.Context, dashboardID string) ([]Permission, error) +} + +// validGrantRole reports whether role is one a dashboard_permissions row +// may actually hold -- see Permission's doc comment for why Admin/Owner +// are excluded. +func validGrantRole(role authz.Role) bool { + return role == authz.RoleViewer || role == authz.RoleEditor +} diff --git a/docs/phase-4-rbac-design.md b/docs/phase-4-rbac-design.md index 55217f3..c3af6cf 100644 --- a/docs/phase-4-rbac-design.md +++ b/docs/phase-4-rbac-design.md @@ -166,6 +166,18 @@ present) is needed for the matrix above — "own vs. any" in the matrix is `created_by = current user` vs. tenant-wide Admin/Owner authority, not a separate grants table for those resource types. +**Implementation note (Phase 4 task 5, added after this design was +signed off):** `dashboard_permissions` is now built -- +`enterprise/internal/rbacstore`'s CRUD plus a `DashboardPermissions` +adapter implementing a new core interface, `api/dashboards. +PermissionStore`, enforced in `api/dashboards`' handler. The applied +migration (`metadata/migrations/0024_create_dashboard_permissions.sql`) +diverged slightly from the schema above -- it allowed `role='admin'` +and left `granted_by` nullable -- and was reconciled to match this +document via `0033_restrict_dashboard_permissions_role.sql`, found +while wiring the enforcement code up. See `enterprise/README.md` and +`/docs/security/threat-model.md` for verification status. + ## Enforcement shape (design only — implementation is task 5) RBAC checks happen server-side, on every `/api`/`/alerting` endpoint diff --git a/docs/phase-4-runbook.md b/docs/phase-4-runbook.md index 3b52b6d..a8966ed 100644 --- a/docs/phase-4-runbook.md +++ b/docs/phase-4-runbook.md @@ -232,15 +232,43 @@ tenant. Verify the real SQL, not just the fake-store unit tests: docker run --rm --network sentry_default -v $(pwd)/api:/src -w /src \ -e DASHBOARDS_TEST_POSTGRES_ADDR=metadata-postgres:5432 \ -e DASHBOARDS_TEST_POSTGRES_PASSWORD=sentry-dev-only \ - golang:1.25-alpine go test ./internal/dashboards/... -run Integration -v + golang:1.25-alpine go test ./dashboards/... -run Integration -v ``` +(Path fixed from an earlier `./internal/dashboards/...` -- stale since +`dashboards` moved from `api/internal/dashboards` to `api/dashboards` +earlier in Phase 4, once `enterprise/cmd/enterprise-api` needed to +import it: Go's compiler-enforced `internal/` visibility rule meant a +separate module like `enterprise/` could never import anything under +`api/internal/...`, regardless of the AGPL/commercial licensing +boundary, which only forbids the reverse direction.) + Expect all `TestIntegration*` tests to pass, including `TestIntegrationDashboardTenantForeignKeyRejectsUnknownTenant` (the `tenant_id` foreign key added in `metadata/migrations/0027_add_dashboards_tenant_fk.sql` rejecting a dashboard for a tenant that doesn't exist). +## 5a. Per-resource dashboard grants (new -- built and unit-tested, not yet run live) + +`enterprise/internal/rbacstore`'s `dashboard_permissions` CRUD and its +`DashboardPermissions` adapter (implementing `api/dashboards. +PermissionStore`) have real integration tests, same skip-gated shape as +§6 below: + +```sh +docker run --rm --network sentry_default -v $(pwd)/enterprise:/src -w /src \ + -e RBACSTORE_TEST_POSTGRES_ADDR=metadata-postgres:5432 \ + -e RBACSTORE_TEST_POSTGRES_PASSWORD=sentry-dev-only \ + golang:1.25-alpine go test ./internal/rbacstore/... -run DashboardPermission -v +``` + +Expect all `TestDashboardPermission*`/`TestSetDashboardPermission*`/ +`TestGetDashboardPermission*`/`TestRevokeDashboardPermission*`/ +`TestListDashboardPermissions` tests to pass. This only takes effect +when `enterprise-api` (not plain `api`) is serving traffic -- see +§8/§10. + ## 6. `enterprise/internal/rbacstore` and `internal/audit` (already verified — reconfirm here) ```sh @@ -422,8 +450,16 @@ 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 -- §3a's manual SQL bootstrap is the only way to grant a logged-in identity access today. -- **No per-resource dashboard grants** (`dashboard_permissions` has a - schema, no handler reads it). +- **Per-resource dashboard grants are now enforced** (`api/dashboards`' + handler reads `dashboard_permissions` via + `enterprise/internal/rbacstore.DashboardPermissions`, only when + `enterprise-api` -- not plain `api` -- serves traffic), but there's + still no UI or `sentryctl` command to create a grant -- `PUT + /dashboards/{id}/permissions/{userId}` has to be called directly. + Verified against a fake store; the real-Postgres integration tests + (`enterprise/internal/rbacstore/rbacstore_test.go`) haven't run + against a live database in this environment, same gap as the rest of + this phase's Postgres-backed pieces. - Three of the four adversarial ClickHouse/Tantivy probes named in `/docs/phase-4-isolation-design.md`'s verification plan are closed (§8, §9); the last (mid-provisioning-race handling) is still stubbed diff --git a/docs/security/threat-model.md b/docs/security/threat-model.md index e270968..5caa5fe 100644 --- a/docs/security/threat-model.md +++ b/docs/security/threat-model.md @@ -219,12 +219,31 @@ means an operator who forgets to set `ENTERPRISE_AUTH_URL` in a multi-tenant deployment gets *no* enforcement at all, silently. Worth a deployment-time check a real rollout should add (not built here). -**Not yet enforced:** the RBAC matrix's `(own/granted)` qualifier for -Editor-level dashboard actions — `dashboard_permissions` (per-resource -grants beyond a user's tenant-baseline role) has a schema -(`metadata/migrations/0024_create_dashboard_permissions.sql`) but no -handler reads it yet. Every Editor in a tenant can act on every -dashboard in that tenant, not just their own/granted ones. +**Now enforced:** the RBAC matrix's `(own/granted)` qualifier for +Editor-level dashboard actions. `dashboard_permissions` +(`metadata/migrations/0024_create_dashboard_permissions.sql`, tightened +by `0033_restrict_dashboard_permissions_role.sql`) is read via +`api/dashboards.PermissionStore` — a core-defined interface, same shape +as `queryapi.AuditLogger` — implemented by +`enterprise/internal/rbacstore.DashboardPermissions` and wired in only +by `enterprise/cmd/enterprise-api`. A plain Editor may now only +edit/delete a dashboard (or its panels) they created, or one where a +grant raises their effective role to Editor; Admin/Owner still act on +any dashboard in their tenant. Managing grants themselves +(`PUT`/`DELETE /dashboards/{id}/permissions/{userId}`) is deliberately +stricter than editing content — only the creator or Admin/Owner may +grant or revoke, never a user who can edit *only* because of a grant +(closes a self-escalation path a looser check would allow). Verified by +`api/dashboards/handler_test.go`'s fake-store tests (the ownership/ +grant/admin matrix, plus the granted-editor-cannot-manage-grants +regression case) — real integration tests against a live Postgres exist +in `enterprise/internal/rbacstore/rbacstore_test.go` but, like the rest +of this phase's rbacstore work, have not been run against one in this +environment. A plain `api/cmd/api` deployment with RBAC enforcement on +but no enterprise permission service wired still enforces ownership/ +Admin — only the "granted" bonus and grant management are unavailable +there (nil `PermissionStore` is a documented no-op, same shape as a nil +`Authorizer`). **Application-layer tenant scoping (dashboards only).** Every `dashboards` store query filters `WHERE tenant_id = $identity.TenantID` @@ -389,7 +408,7 @@ terms: | Human SSO login — OIDC | **Built, verified with a real fake IdP** (not yet tried against a real external IdP) | | Human SSO login — SAML | **Built, verified with a real fake IdP** (not yet tried against a real external IdP) | | Multi-tenant-membership login (tenant picker) | **Not implemented** — refused with a clear error, not guessed | -| Per-resource dashboard grants (`own/granted`) | **Not implemented** | +| Per-resource dashboard grants (`own/granted`) | **Built, unit-tested against a fake store; live-Postgres integration tests written, not run in this environment** (only when `enterprise-api` serves traffic — plain `api` falls back to own/Admin only) | | Query audit logging (routine queries) | **Enforced**, fail-open, and now wired to a real writer via `enterprise-api` (`audit.QueryAPILogger`) | | Audit log tamper detection (hash chain) | **Enforced**, verified live | | Audit log tamper prevention (external anchoring) | **Design only** — `FileSink` is a dev stand-in | diff --git a/enterprise/README.md b/enterprise/README.md index e660530..d576055 100644 --- a/enterprise/README.md +++ b/enterprise/README.md @@ -105,15 +105,34 @@ what `samlidp`'s own default assertion builder does. Neither protocol has been tried against a real external IdP or a running `enterprise-auth` container -- see `/docs/phase-4-runbook.md` §3a/§3b. +`dashboard_permissions` is now wired end to end: `rbacstore/ +dashboard_permissions.go` is the raw CRUD, `rbacstore/ +dashboards_adapter.go`'s `DashboardPermissions` implements +`api/dashboards.PermissionStore` (the interface core defines and +carries as a nil-by-default field, same shape as +`queryapi.AuditLogger`), and `enterprise-api`'s `main.go` wires it in. +`api/dashboards`' handler now enforces the matrix's "(own/granted)" +qualifier: an Editor may edit/delete a dashboard (or its panels) they +created, or one where a grant raises their effective role to Editor; +managing grants themselves is stricter still -- creator or Admin/Owner +only, closing a self-escalation path where a granted-but-not-creator +Editor could otherwise extend their own access. `metadata/migrations/ +0033_restrict_dashboard_permissions_role.sql` fixes a divergence +between 0024's actual CHECK constraint (allowed `role='admin'`, nullable +`granted_by`) and the design doc's schema (viewer/editor only, +`granted_by` required) found while wiring this up. Verified against a +fake `PermissionStore` in `api/dashboards/handler_test.go` (the full +own/granted/admin/creator matrix, including the granted-editor-cannot- +manage-grants regression); real integration tests exist in +`rbacstore_test.go` but haven't run against a live Postgres in this +environment, same disclosed gap as the rest of this package's +Postgres-backed pieces. + **Deliberately deferred, not half-built** -- named explicitly rather than silently left out: - A tenant-picker UI/flow for an identity with more than one `tenant_memberships` row -- `loginhandler` refuses these logins outright rather than guessing (`ErrMultipleMemberships`). -- `dashboard_permissions` CRUD (schema exists, - `metadata/migrations/0024`; no caller reads per-resource grants - yet -- `dashboards`' handler enforces tenant-baseline role only, not - the matrix's "(own/granted)" qualifier). - **Ingest tenant-awareness, for either storage engine** -- `chrunner`/ `searchclient` prove read isolation given tenant-scoped data exists, but nothing writes it: every record `ingest` produces still lands in @@ -139,7 +158,7 @@ internal/saml/ crewjam/saml wiring: SP setup, login redirect, respons internal/session/ issues/validates signed session + RoleService tokens internal/authhandler/ POST /internal/authorize, GET /auth/features internal/loginhandler/ GET /auth/oidc/{login,callback} + GET /auth/saml/login + POST /auth/saml/acs -- the human login flow -internal/rbacstore/ users/tenants/tenant_memberships/data_sources CRUD (pgx against sentry_metadata) +internal/rbacstore/ users/tenants/tenant_memberships/data_sources/dashboard_permissions CRUD (pgx against sentry_metadata) internal/tenantprovision/ real ClickHouse CREATE DATABASE/USER/GRANT internal/chrunner/ tenant-scoped api/querylang/executor.SQLRunner internal/searchclient/ tenant-scoped api/querylang/executor.SearchClient @@ -149,9 +168,8 @@ internal/apiconfig/ enterprise-api's own env-var config internal/config/ enterprise-auth's env-var config ``` -Future additions: `dashboard_permissions` CRUD, ingest tenant-awareness -(undesigned), and real deployment-topology wiring for `enterprise-api` --- see "Status" above. +Future additions: ingest tenant-awareness (undesigned), and real +deployment-topology wiring for `enterprise-api` -- see "Status" above. ## Why OIDC and SAML aren't hand-rolled diff --git a/enterprise/cmd/enterprise-api/main.go b/enterprise/cmd/enterprise-api/main.go index 521b95d..099eecf 100644 --- a/enterprise/cmd/enterprise-api/main.go +++ b/enterprise/cmd/enterprise-api/main.go @@ -153,7 +153,7 @@ func main() { auditLogger := audit.NewQueryAPILogger(audit.NewStore(auditPool), audit.SourceAPI) queryHandler := queryapi.NewHandler(logger, registry, search, cfg.QueryTimeout, auditLogger, authorizer) - dashboardsHandler := dashboards.NewHandler(logger, dashboards.NewStore(pgPool), authorizer) + dashboardsHandler := dashboards.NewHandler(logger, dashboards.NewStore(pgPool), authorizer, rbacstore.NewDashboardPermissions(rbac)) mux := http.NewServeMux() queryHandler.RegisterRoutes(mux) diff --git a/enterprise/internal/rbacstore/dashboard_permissions.go b/enterprise/internal/rbacstore/dashboard_permissions.go new file mode 100644 index 0000000..3192375 --- /dev/null +++ b/enterprise/internal/rbacstore/dashboard_permissions.go @@ -0,0 +1,96 @@ +package rbacstore + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/google/uuid" + "github.com/jackc/pgx/v5" +) + +// DashboardPermission is one dashboard_permissions row -- see +// /docs/phase-4-rbac-design.md's "additive-only per-resource grants" +// section and metadata/migrations/0024/0033. Role is always RoleViewer +// or RoleEditor: metadata/migrations/0033_restrict_dashboard_permissions_role.sql +// narrowed the CHECK constraint to match, since Admin/Owner already have +// tenant-wide access and never need a resource-level grant. +type DashboardPermission struct { + DashboardID string + UserID string + Role Role + GrantedBy string + CreatedAt time.Time +} + +// SetDashboardPermission upserts a grant -- the sole mutation path, +// same "one method, ON CONFLICT DO UPDATE" shape as SetMembership, so a +// future audit-log hook has one call site to wrap. grantedBy is +// required (metadata/migrations/0033 made granted_by NOT NULL): every +// grant must be attributable to the identity that created it. +func (s *Store) SetDashboardPermission(ctx context.Context, dashboardID, userID string, role Role, grantedBy string) error { + if grantedBy == "" { + return fmt.Errorf("rbacstore: grantedBy is required") + } + _, err := s.pool.Exec(ctx, ` + INSERT INTO dashboard_permissions (id, dashboard_id, user_id, role, granted_by) + VALUES ($1, $2, $3, $4, $5) + ON CONFLICT (dashboard_id, user_id) DO UPDATE + SET role = EXCLUDED.role, granted_by = EXCLUDED.granted_by`, + uuid.NewString(), dashboardID, userID, string(role), grantedBy) + if err != nil { + return fmt.Errorf("rbacstore: setting dashboard permission: %w", err) + } + return nil +} + +func (s *Store) RevokeDashboardPermission(ctx context.Context, dashboardID, userID string) error { + _, err := s.pool.Exec(ctx, + `DELETE FROM dashboard_permissions WHERE dashboard_id = $1 AND user_id = $2`, dashboardID, userID) + if err != nil { + return fmt.Errorf("rbacstore: revoking dashboard permission: %w", err) + } + return nil +} + +func (s *Store) GetDashboardPermission(ctx context.Context, dashboardID, userID string) (*DashboardPermission, error) { + var p DashboardPermission + var role string + row := s.pool.QueryRow(ctx, ` + SELECT dashboard_id, user_id, role, granted_by, created_at + FROM dashboard_permissions WHERE dashboard_id = $1 AND user_id = $2`, dashboardID, userID) + if err := row.Scan(&p.DashboardID, &p.UserID, &role, &p.GrantedBy, &p.CreatedAt); err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return nil, ErrNotFound + } + return nil, fmt.Errorf("rbacstore: getting dashboard permission: %w", err) + } + p.Role = Role(role) + return &p, nil +} + +// ListDashboardPermissions supports the "manage a dashboard's per-user +// grants" UI/endpoint -- every grant on one dashboard, for a +// creator/Admin/Owner to review or revoke. +func (s *Store) ListDashboardPermissions(ctx context.Context, dashboardID string) ([]DashboardPermission, error) { + rows, err := s.pool.Query(ctx, ` + SELECT dashboard_id, user_id, role, granted_by, created_at + FROM dashboard_permissions WHERE dashboard_id = $1 ORDER BY created_at`, dashboardID) + if err != nil { + return nil, fmt.Errorf("rbacstore: listing dashboard permissions: %w", err) + } + defer rows.Close() + + var out []DashboardPermission + for rows.Next() { + var p DashboardPermission + var role string + if err := rows.Scan(&p.DashboardID, &p.UserID, &role, &p.GrantedBy, &p.CreatedAt); err != nil { + return nil, fmt.Errorf("rbacstore: scanning dashboard permission: %w", err) + } + p.Role = Role(role) + out = append(out, p) + } + return out, rows.Err() +} diff --git a/enterprise/internal/rbacstore/dashboards_adapter.go b/enterprise/internal/rbacstore/dashboards_adapter.go new file mode 100644 index 0000000..be7ab59 --- /dev/null +++ b/enterprise/internal/rbacstore/dashboards_adapter.go @@ -0,0 +1,65 @@ +// Adapts *Store to api/dashboards.PermissionStore -- the interface core +// defines and has carried as a nil-by-default field +// (api/dashboards.Handler.permissions) since Phase 4 task 5, waiting on +// exactly this: a real implementation, wired in by +// enterprise/cmd/enterprise-api, the one binary allowed to import both +// packages. Same shape as audit.QueryAPILogger's adapter over +// api/queryapi.AuditLogger. +package rbacstore + +import ( + "context" + "errors" + "fmt" + + "github.com/sentry/sentry/api/authz" + "github.com/sentry/sentry/api/dashboards" +) + +// DashboardPermissions implements dashboards.PermissionStore by +// translating between authz.Role (core's type) and this package's Role +// (kept separate rather than importing authz's constants directly -- +// see Role's own doc comment for why). +type DashboardPermissions struct { + store *Store +} + +func NewDashboardPermissions(store *Store) *DashboardPermissions { + return &DashboardPermissions{store: store} +} + +func (d *DashboardPermissions) GrantedRole(ctx context.Context, dashboardID, userID string) (authz.Role, bool, error) { + p, err := d.store.GetDashboardPermission(ctx, dashboardID, userID) + if err != nil { + if errors.Is(err, ErrNotFound) { + return "", false, nil + } + return "", false, err + } + return authz.Role(p.Role), true, nil +} + +func (d *DashboardPermissions) SetPermission(ctx context.Context, dashboardID, userID string, role authz.Role, grantedBy string) error { + if role != authz.RoleViewer && role != authz.RoleEditor { + return fmt.Errorf("rbacstore: dashboard permission role must be viewer or editor, got %q", role) + } + return d.store.SetDashboardPermission(ctx, dashboardID, userID, Role(role), grantedBy) +} + +func (d *DashboardPermissions) RevokePermission(ctx context.Context, dashboardID, userID string) error { + return d.store.RevokeDashboardPermission(ctx, dashboardID, userID) +} + +func (d *DashboardPermissions) ListPermissions(ctx context.Context, dashboardID string) ([]dashboards.Permission, error) { + rows, err := d.store.ListDashboardPermissions(ctx, dashboardID) + if err != nil { + return nil, err + } + out := make([]dashboards.Permission, 0, len(rows)) + for _, r := range rows { + out = append(out, dashboards.Permission{ + UserID: r.UserID, Role: authz.Role(r.Role), GrantedBy: r.GrantedBy, CreatedAt: r.CreatedAt, + }) + } + return out, nil +} diff --git a/enterprise/internal/rbacstore/rbacstore.go b/enterprise/internal/rbacstore/rbacstore.go index c090e23..00f50bd 100644 --- a/enterprise/internal/rbacstore/rbacstore.go +++ b/enterprise/internal/rbacstore/rbacstore.go @@ -8,17 +8,15 @@ // append-only ledger, so it has no analogous reason to restrict its own // write access. // -// This package is the storage building block a future OIDC/SAML login -// HTTP handler would call to resolve "which tenant/role does this SSO -// identity map to" and issue a session (internal/session) accordingly -- -// that handler itself isn't built yet (see cmd/enterprise-auth/main.go's -// doc comment), so today rbacstore's only production caller is -// -mint-service-token's future tenant-aware successor and its own tests. -// dashboard_permissions doesn't have CRUD here yet -- no caller reads -// per-resource grants (see api/dashboards/handler.go's doc -// comment). data_sources CRUD was added once enterprise/internal/ -// chrunner needed a real place to read per-tenant ClickHouse credentials -// from at startup (see that package's doc comment). +// This package is the storage building block internal/loginhandler's +// OIDC/SAML handlers call to resolve "which tenant/role does this SSO +// identity map to" and issue a session (internal/session) accordingly. +// dashboard_permissions CRUD (dashboard_permissions.go) is wrapped by +// DashboardPermissions (dashboards_adapter.go) to implement +// api/dashboards.PermissionStore -- see that adapter's doc comment. +// data_sources CRUD was added once enterprise/internal/chrunner needed a +// real place to read per-tenant ClickHouse credentials from at startup +// (see that package's doc comment). package rbacstore import ( diff --git a/enterprise/internal/rbacstore/rbacstore_test.go b/enterprise/internal/rbacstore/rbacstore_test.go index a4d38a9..74d8d42 100644 --- a/enterprise/internal/rbacstore/rbacstore_test.go +++ b/enterprise/internal/rbacstore/rbacstore_test.go @@ -18,6 +18,8 @@ import ( "github.com/google/uuid" "github.com/jackc/pgx/v5/pgxpool" + + "github.com/sentry/sentry/api/authz" ) func testStore(t *testing.T) *Store { @@ -346,3 +348,232 @@ func TestListProvisionedDataSourcesExcludesUnprovisionedAndInactive(t *testing.T t.Fatal("expected the active, provisioned data source to be in the list") } } + +// createTestDashboard inserts directly into the dashboards table (owned +// by api/dashboards, not this package) -- dashboard_permissions. +// dashboard_id has a real foreign-key constraint +// (metadata/migrations/0024), so a permission row for a dashboard that +// doesn't exist is rejected by Postgres itself. Mirrors +// api/dashboards/store_integration_test.go's createTestTenant, which +// does the same thing in reverse (inserting into tenants, a table that +// package doesn't own either). +func createTestDashboard(t *testing.T, s *Store, tenantID, createdBy string) string { + t.Helper() + id := uuid.NewString() + _, err := s.pool.Exec(context.Background(), ` + INSERT INTO dashboards (id, tenant_id, name, default_earliest, default_latest, created_by) + VALUES ($1, $2, $3, '-1h', 'now', $4)`, id, tenantID, "Test Dashboard "+uniqueSuffix(), createdBy) + if err != nil { + t.Fatalf("inserting test dashboard: %v", err) + } + return id +} + +func TestSetDashboardPermissionThenGet(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) + } + creator, err := s.UpsertUserBySSO(ctx, "sub-creator-"+uniqueSuffix(), "creator-"+uniqueSuffix()+"@example.com", "Creator") + if err != nil { + t.Fatalf("UpsertUserBySSO creator: %v", err) + } + grantee, err := s.UpsertUserBySSO(ctx, "sub-grantee-"+uniqueSuffix(), "grantee-"+uniqueSuffix()+"@example.com", "Grantee") + if err != nil { + t.Fatalf("UpsertUserBySSO grantee: %v", err) + } + dashboardID := createTestDashboard(t, s, tenantID, creator.ID) + + if err := s.SetDashboardPermission(ctx, dashboardID, grantee.ID, RoleEditor, creator.ID); err != nil { + t.Fatalf("SetDashboardPermission: %v", err) + } + got, err := s.GetDashboardPermission(ctx, dashboardID, grantee.ID) + if err != nil { + t.Fatalf("GetDashboardPermission: %v", err) + } + if got.Role != RoleEditor || got.GrantedBy != creator.ID { + t.Fatalf("unexpected permission: %+v", got) + } + + // Re-setting (e.g. a role change from viewer to editor) must update + // in place, not create a duplicate row for the same (dashboard, user). + if err := s.SetDashboardPermission(ctx, dashboardID, grantee.ID, RoleViewer, creator.ID); err != nil { + t.Fatalf("SetDashboardPermission (update): %v", err) + } + got, err = s.GetDashboardPermission(ctx, dashboardID, grantee.ID) + if err != nil { + t.Fatalf("GetDashboardPermission after update: %v", err) + } + if got.Role != RoleViewer { + t.Fatalf("role after update = %q, want viewer", got.Role) + } +} + +func TestSetDashboardPermissionRequiresGrantedBy(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) + } + dashboardID := createTestDashboard(t, s, tenantID, user.ID) + + if err := s.SetDashboardPermission(ctx, dashboardID, user.ID, RoleEditor, ""); err == nil { + t.Fatal("expected an error for an empty grantedBy -- every grant must be attributable") + } +} + +func TestGetDashboardPermissionNotFound(t *testing.T) { + s := testStore(t) + if _, err := s.GetDashboardPermission(context.Background(), uuid.NewString(), uuid.NewString()); err != ErrNotFound { + t.Fatalf("GetDashboardPermission error = %v, want ErrNotFound", err) + } +} + +func TestRevokeDashboardPermission(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) + } + creator, err := s.UpsertUserBySSO(ctx, "sub-creator-"+uniqueSuffix(), "creator-"+uniqueSuffix()+"@example.com", "Creator") + if err != nil { + t.Fatalf("UpsertUserBySSO creator: %v", err) + } + grantee, err := s.UpsertUserBySSO(ctx, "sub-grantee-"+uniqueSuffix(), "grantee-"+uniqueSuffix()+"@example.com", "Grantee") + if err != nil { + t.Fatalf("UpsertUserBySSO grantee: %v", err) + } + dashboardID := createTestDashboard(t, s, tenantID, creator.ID) + if err := s.SetDashboardPermission(ctx, dashboardID, grantee.ID, RoleEditor, creator.ID); err != nil { + t.Fatalf("SetDashboardPermission: %v", err) + } + + if err := s.RevokeDashboardPermission(ctx, dashboardID, grantee.ID); err != nil { + t.Fatalf("RevokeDashboardPermission: %v", err) + } + if _, err := s.GetDashboardPermission(ctx, dashboardID, grantee.ID); err != ErrNotFound { + t.Fatalf("GetDashboardPermission after revoke = %v, want ErrNotFound", err) + } +} + +func TestListDashboardPermissions(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) + } + creator, err := s.UpsertUserBySSO(ctx, "sub-creator-"+uniqueSuffix(), "creator-"+uniqueSuffix()+"@example.com", "Creator") + if err != nil { + t.Fatalf("UpsertUserBySSO creator: %v", err) + } + dashboardID := createTestDashboard(t, s, tenantID, creator.ID) + otherDashboardID := createTestDashboard(t, s, tenantID, creator.ID) + + for i := 0; i < 2; i++ { + grantee, err := s.UpsertUserBySSO(ctx, fmt.Sprintf("sub-grantee-%d-%s", i, uniqueSuffix()), fmt.Sprintf("grantee-%d-%s@example.com", i, uniqueSuffix()), "Grantee") + if err != nil { + t.Fatalf("UpsertUserBySSO grantee %d: %v", i, err) + } + if err := s.SetDashboardPermission(ctx, dashboardID, grantee.ID, RoleEditor, creator.ID); err != nil { + t.Fatalf("SetDashboardPermission %d: %v", i, err) + } + } + // A grant on a different dashboard must not leak into this one's list. + otherGrantee, err := s.UpsertUserBySSO(ctx, "sub-other-"+uniqueSuffix(), "other-"+uniqueSuffix()+"@example.com", "Other") + if err != nil { + t.Fatalf("UpsertUserBySSO otherGrantee: %v", err) + } + if err := s.SetDashboardPermission(ctx, otherDashboardID, otherGrantee.ID, RoleViewer, creator.ID); err != nil { + t.Fatalf("SetDashboardPermission otherDashboard: %v", err) + } + + list, err := s.ListDashboardPermissions(ctx, dashboardID) + if err != nil { + t.Fatalf("ListDashboardPermissions: %v", err) + } + if len(list) != 2 { + t.Fatalf("len(list) = %d, want 2", len(list)) + } +} + +// TestDashboardPermissionsAdapterImplementsPermissionStore drives the +// adapter (dashboards_adapter.go) end to end -- the same interface +// api/dashboards.Handler actually calls -- rather than only testing the +// raw Store methods above, so a mismatch between the two (e.g. a bad +// authz.Role<->Role conversion) would be caught here. +func TestDashboardPermissionsAdapterImplementsPermissionStore(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) + } + creator, err := s.UpsertUserBySSO(ctx, "sub-creator-"+uniqueSuffix(), "creator-"+uniqueSuffix()+"@example.com", "Creator") + if err != nil { + t.Fatalf("UpsertUserBySSO creator: %v", err) + } + grantee, err := s.UpsertUserBySSO(ctx, "sub-grantee-"+uniqueSuffix(), "grantee-"+uniqueSuffix()+"@example.com", "Grantee") + if err != nil { + t.Fatalf("UpsertUserBySSO grantee: %v", err) + } + dashboardID := createTestDashboard(t, s, tenantID, creator.ID) + + adapter := NewDashboardPermissions(s) + + if _, ok, err := adapter.GrantedRole(ctx, dashboardID, grantee.ID); err != nil || ok { + t.Fatalf("GrantedRole before any grant = (_, %v, %v), want (_, false, nil)", ok, err) + } + if err := adapter.SetPermission(ctx, dashboardID, grantee.ID, authz.RoleEditor, creator.ID); err != nil { + t.Fatalf("SetPermission: %v", err) + } + role, ok, err := adapter.GrantedRole(ctx, dashboardID, grantee.ID) + if err != nil || !ok || role != authz.RoleEditor { + t.Fatalf("GrantedRole = (%v, %v, %v), want (editor, true, nil)", role, ok, err) + } + + list, err := adapter.ListPermissions(ctx, dashboardID) + if err != nil || len(list) != 1 || list[0].Role != authz.RoleEditor { + t.Fatalf("ListPermissions = (%+v, %v), want one editor grant", list, err) + } + + if err := adapter.RevokePermission(ctx, dashboardID, grantee.ID); err != nil { + t.Fatalf("RevokePermission: %v", err) + } + if _, ok, _ := adapter.GrantedRole(ctx, dashboardID, grantee.ID); ok { + t.Fatal("expected the grant to be revoked") + } +} + +// TestDashboardPermissionsAdapterRejectsAdminRole is the regression test +// for Permission's doc comment: Admin/Owner already have tenant-wide +// dashboard access, so a resource-level grant of "admin" is meaningless +// under this design and metadata/migrations/0033 tightened the CHECK +// constraint to match -- the adapter must reject it before it ever +// reaches SQL, not rely on the constraint alone. +func TestDashboardPermissionsAdapterRejectsAdminRole(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) + } + creator, err := s.UpsertUserBySSO(ctx, "sub-creator-"+uniqueSuffix(), "creator-"+uniqueSuffix()+"@example.com", "Creator") + if err != nil { + t.Fatalf("UpsertUserBySSO creator: %v", err) + } + dashboardID := createTestDashboard(t, s, tenantID, creator.ID) + + adapter := NewDashboardPermissions(s) + if err := adapter.SetPermission(ctx, dashboardID, uuid.NewString(), authz.RoleAdmin, creator.ID); err == nil { + t.Fatal("expected an error granting role=admin via a dashboard permission") + } +} diff --git a/metadata/migrations/0033_restrict_dashboard_permissions_role.sql b/metadata/migrations/0033_restrict_dashboard_permissions_role.sql new file mode 100644 index 0000000..f55af20 --- /dev/null +++ b/metadata/migrations/0033_restrict_dashboard_permissions_role.sql @@ -0,0 +1,14 @@ +-- 0024_create_dashboard_permissions.sql's CHECK constraint diverged +-- from /docs/phase-4-rbac-design.md's schema: it allowed role='admin' +-- and left granted_by nullable. Neither divergence is meaningful under +-- the enforcement built in Phase 4 task 5 (enterprise/internal/ +-- rbacstore's dashboard-permission adapter): a resource-level grant only +-- ever raises someone to Editor on one dashboard (Admin/Owner already +-- have tenant-wide access, so an "admin" grant would be a no-op at best +-- and a confusing dead code path at worst), and every real grant is +-- created by an authenticated Editor/Admin/Owner, so granted_by should +-- never legitimately be null. Found and fixed before any of this ran +-- against a live database in this environment. +ALTER TABLE dashboard_permissions DROP CONSTRAINT dashboard_permissions_role_check; +ALTER TABLE dashboard_permissions ADD CONSTRAINT dashboard_permissions_role_check CHECK (role IN ('viewer', 'editor')); +ALTER TABLE dashboard_permissions ALTER COLUMN granted_by SET NOT NULL;