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).
This commit is contained in:
@@ -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-%[email protected]", 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")
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user