From 0152bdf567097a18574475e5937a3a14a653eca9 Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Mon, 11 May 2026 10:50:34 +0000 Subject: [PATCH] fix(auth/rbac): scope-aware ActorRole revoke (A-4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HIGH-10's UNIQUE (actor, role, scope_type, scope_id, tenant) uniqueness extension lets an operator grant the same role to the same actor at multiple scopes (e.g. r-operator on profile=p-acme AND profile=p-globex). But ActorRoleRepository.Revoke's WHERE clause omitted (scope_type, scope_id) — a single call deleted every variant. Selective revoke was unrepresentable; operators had to drop all and re-grant N-1, opening a race window where the actor's access was briefly different. Closure across all layers (handler → service → repo → MCP → GUI client), preserving the legacy "revoke all variants" contract for unmodified callers: internal/repository/auth.go - New ActorRoleRevokeOptions struct. Zero value = legacy semantic; non-empty ScopeType narrows to one variant. - New ErrActorRoleNotFound sentinel for scoped no-match (HTTP 404). internal/repository/postgres/auth.go - Revoke signature extended with opts. Empty opts.ScopeType uses the legacy SQL (no scope WHERE), zero-row delete = no error. - Non-empty narrows with `scope_type = $5 AND scope_id IS NOT DISTINCT FROM $6` — the IS-NOT-DISTINCT-FROM is load-bearing, vanilla `=` would silently miss the (global, NULL) case because NULL ≠ NULL in standard SQL. - Selective revoke with zero matching rows returns ErrActorRoleNotFound; operators get feedback on typos. internal/service/auth/actor_role_service.go - Revoke takes opts. Audit row's details map records the scope so SIEMs can distinguish wide-vs-selective revokes: `scope: "all_variants"` for the legacy path, or `scope_type` + `scope_id` for selective. Privilege check (auth.role.assign) and reserved-actor guard unchanged. internal/api/handler/auth.go - RevokeRoleFromKey parses optional `?scope_type=` / `?scope_id=` query params via new parseRevokeScope helper. - Validation mirrors AssignRoleToKey: scope_id forbidden with scope_type=global, required with profile/issuer, invalid scope_type → 400. scope_id without scope_type also → 400. - writeAuthError maps ErrActorRoleNotFound to 404. internal/mcp/tools_auth.go + types.go - AuthRevokeKeyRoleInput gains optional ScopeType + ScopeID with jsonschema descriptions explaining the dual-mode contract. - Tool call site appends URL-encoded query params when ScopeType is set; legacy callers (no scope_type) emit the bare DELETE path unchanged. web/src/api/client.ts - authRevokeKeyRole signature: optional 3rd argument `{ scope_type?, scope_id? }`. Pre-A-4 call sites (no opts arg) keep firing the bare DELETE — fully backward compatible. The GUI KeysPage's per-row revoke button (still one row per role, pre-Fix-12) continues to use the legacy shape; future GUI work can pass scope params for per-variant rows. docs/operator/rbac.md - New "Revoke: legacy 'all variants' vs scope-selective" subsection under "From the HTTP API" with curl examples for both modes plus the audit-row payload shape that lets SOC/SIEM tell them apart. Regression coverage: Repository (testcontainers, skipped under -short — 6 tests in internal/repository/postgres/auth_revoke_scope_test.go): TestRevokeActorRole_NoOpts_RemovesAllVariants TestRevokeActorRole_WithScope_RemovesOnlyMatching TestRevokeActorRole_WithGlobalScope_RemovesOnlyGlobal — pins the IS-NOT-DISTINCT-FROM branch (global, NULL) TestRevokeActorRole_NoMatch_ReturnsNotFound — pins the new sentinel TestRevokeActorRole_NoOpts_NoMatch_IsNoOp — pins the legacy idempotence contract TestRevokeActorRole_IssuerScope_RemovesOnlyMatching — pin the issuer-scope half (profile + issuer are symmetric scope types) Handler (7 new tests in auth_test.go): TestAuthHandler_RevokeRoleFromKey — extended to assert no scope filter is forwarded when query string is empty (legacy behaviour) TestAuthHandler_RevokeRoleFromKey_A4_ScopedProfile TestAuthHandler_RevokeRoleFromKey_A4_ScopedGlobal TestAuthHandler_RevokeRoleFromKey_A4_RejectsScopeIDWithGlobal TestAuthHandler_RevokeRoleFromKey_A4_RejectsMissingScopeID TestAuthHandler_RevokeRoleFromKey_A4_RejectsScopeIDWithoutScopeType TestAuthHandler_RevokeRoleFromKey_A4_RejectsInvalidScopeType TestAuthHandler_RevokeRoleFromKey_A4_ScopedNotFoundReturns404 MCP (2 new table rows in tools_per_tool_test.go): Scoped revoke with scope_type=profile + scope_id=p-acme → `?scope_type=profile&scope_id=p-acme` Scoped revoke with scope_type=global (no scope_id) → `?scope_type=global` Service-layer test plumbing (service_test.go) updated for new opts arg: 4 existing call sites pass repository.ActorRoleRevokeOptions{} to keep their pre-A-4 semantics; the fakeActorRoleRepo.Revoke implementation now mirrors the postgres scope-aware behaviour (legacy zero-value vs scoped narrowing + ErrActorRoleNotFound on no-match). Verify gate green: gofmt clean, go vet clean, go test -short across repository/postgres, service/auth, api/handler, and mcp. The pre-existing KeysPage.test.tsx failure observed on the baseline commit (reproduced via `git stash` earlier in Fix 03) is unrelated; my client.ts change adds an optional third argument and is fully backward-compatible. Spec at cowork/auth-bundles-fixes-2026-05-11/04-high-actor-role-revoke-scope.md. Audit doc updated: new row A-4 (2026-05-11) CLOSED appended to the status table at the bottom of cowork/auth-bundles-audit-2026-05-10.md. Operator-visible advisory in CHANGELOG.md v2.1.0 release notes under Security (non-BREAKING — legacy callers are unchanged). Depends on Fix 01 (the scope-aware EffectivePermissions read path on branch fix/audit-2026-05-11/crit-actor-role-scope-reads). This fix makes the inverse op selectively reversible; without Fix 01 the read side would mis-evaluate scoped grants anyway, making selective revoke moot at runtime. --- CHANGELOG.md | 22 ++ docs/operator/rbac.md | 39 ++- internal/api/handler/auth.go | 73 +++++- internal/api/handler/auth_test.go | 145 ++++++++++- internal/mcp/tools_auth.go | 16 +- internal/mcp/tools_per_tool_test.go | 4 + internal/mcp/types.go | 13 +- internal/repository/auth.go | 51 +++- internal/repository/postgres/auth.go | 53 +++- .../postgres/auth_revoke_scope_test.go | 231 ++++++++++++++++++ internal/service/auth/actor_role_service.go | 25 +- internal/service/auth/service_test.go | 50 +++- web/src/api/client.ts | 25 +- 13 files changed, 715 insertions(+), 32 deletions(-) create mode 100644 internal/repository/postgres/auth_revoke_scope_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b6c0cc..f9d3123 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,28 @@ ## Unreleased +### Security + +- **Scope-aware actor-role revoke (Audit 2026-05-11 A-4).** + HIGH-10 made it possible to grant the same role to the same actor at + multiple scopes (e.g. `r-operator` on `profile=p-acme` AND `profile=p-globex`) + via the unique constraint extension on `actor_roles`, but + `ActorRoleRepository.Revoke` ignored `(scope_type, scope_id)` and + unconditionally deleted every variant. Operators who wanted to drop + one scoped grant had to nuke them all and re-grant the remainder — + a race window where the actor's access was briefly different. The + `DELETE /v1/auth/keys/{id}/roles/{role_id}` endpoint now accepts + optional `?scope_type=` / `?scope_id=` query params that narrow the + revoke to a single variant; no-match returns 404. The legacy "revoke + every variant" semantic is preserved when the query params are + absent, so existing CLI / GUI buttons keep working unchanged. The + audit row's `details` payload records which mode fired so SOC / SIEM + can distinguish wide cleanups from targeted demotions. MCP tool + `certctl_auth_revoke_role_from_key` gains optional `scope_type` + + `scope_id` input fields with matching semantics. Documented in + `docs/operator/rbac.md` under "Revoke: legacy 'all variants' vs + scope-selective." + ### Security (BREAKING — silent-elevation closure) - **HIGH-10 actor-role scope is now enforced (Audit 2026-05-11 A-1).** diff --git a/docs/operator/rbac.md b/docs/operator/rbac.md index c8d832b..ef33be4 100644 --- a/docs/operator/rbac.md +++ b/docs/operator/rbac.md @@ -209,10 +209,47 @@ tag. Quick reference: | `DELETE /v1/auth/roles/{id}/permissions/{perm}` | `auth.role.edit` | | `GET /v1/auth/keys` | `auth.role.list` | | `POST /v1/auth/keys/{id}/roles` | `auth.role.assign` | -| `DELETE /v1/auth/keys/{id}/roles/{role_id}` | `auth.role.assign` | +| `DELETE /v1/auth/keys/{id}/roles/{role_id}` (+ optional `?scope_type=` / `?scope_id=`) | `auth.role.assign` | | `GET /v1/auth/check` | (authenticated; surfaces effective perms) | | `GET /v1/auth/bootstrap` + `POST /v1/auth/bootstrap` | (auth-exempt; gated by env-var token) | +#### Revoke: legacy "all variants" vs scope-selective (Audit 2026-05-11 A-4) + +`DELETE /v1/auth/keys/{id}/roles/{role_id}` runs in one of two modes, +selected by presence of the optional query parameters: + +- **No query params (legacy "revoke all variants")** — every scoped grant of + this role held by this actor is dropped. Idempotent: zero-row deletes + return 204 (no error). This is the pre-A-4 behaviour and remains the + default for the CLI / GUI buttons that don't know about scope. + + ```bash + # Drop EVERY variant of r-operator from alice (global, profile-scoped, + # issuer-scoped — all gone). + curl -X DELETE https://certctl.example.com/api/v1/auth/keys/alice/roles/r-operator + ``` + +- **`?scope_type=` (+ optional `?scope_id=`)** — drop ONE variant. Used + when an actor holds the same role at multiple scopes (HIGH-10 made + that representable; A-4 makes it selectively revocable). + `scope_type=global` requires `scope_id` to be absent; `scope_type=profile` + / `issuer` require `scope_id`. No match returns 404 so operators get + feedback when they target a scope variant the actor doesn't hold. + + ```bash + # Alice holds r-operator scoped to p-acme AND p-globex. + # Drop ONLY the p-acme grant; the p-globex grant stays. + curl -X DELETE 'https://certctl.example.com/api/v1/auth/keys/alice/roles/r-operator?scope_type=profile&scope_id=p-acme' + + # Drop ONLY the global grant of r-operator (keeps any profile / issuer variants): + curl -X DELETE 'https://certctl.example.com/api/v1/auth/keys/alice/roles/r-operator?scope_type=global' + ``` + +The audit row's `details` payload records which mode fired — +`scope: "all_variants"` for the legacy path, or the explicit +`scope_type` + `scope_id` for selective revoke — so SOC / SIEM can +distinguish wide cleanups from targeted demotions in the access log. + ### From the MCP server Bundle 1 Phase 11 ships 12 RBAC tools: diff --git a/internal/api/handler/auth.go b/internal/api/handler/auth.go index 4697ae6..6d92a80 100644 --- a/internal/api/handler/auth.go +++ b/internal/api/handler/auth.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "net/http" "strings" "time" @@ -72,7 +73,11 @@ type AuthPermissionService interface { // effective-permissions query the GUI's /v1/auth/me handler uses. type AuthActorRoleService interface { Grant(ctx context.Context, caller *authsvc.Caller, ar *authdomain.ActorRole) error - Revoke(ctx context.Context, caller *authsvc.Caller, actorID string, actorType domain.ActorType, roleID string) error + // Audit 2026-05-11 A-4 — Revoke takes optional scope filtering so + // callers that hold multiple scoped variants of the same role can + // drop one variant selectively. opts.ScopeType == "" preserves the + // legacy "revoke all" semantic. + Revoke(ctx context.Context, caller *authsvc.Caller, actorID string, actorType domain.ActorType, roleID string, opts repository.ActorRoleRevokeOptions) error ListForActor(ctx context.Context, caller *authsvc.Caller, actorID string, actorType domain.ActorType) ([]*authdomain.ActorRole, error) EffectivePermissions(ctx context.Context, caller *authsvc.Caller, actorID string, actorType domain.ActorType) ([]repository.EffectivePermission, error) // ListKeys (Bundle 1 Phase 7) returns every actor in the tenant @@ -496,6 +501,22 @@ func (h AuthHandler) AssignRoleToKey(w http.ResponseWriter, r *http.Request) { } // RevokeRoleFromKey handles DELETE /api/v1/auth/keys/{id}/roles/{role_id}. +// +// Audit 2026-05-11 A-4 — two operating modes selected by presence of +// the optional `?scope_type=` / `?scope_id=` query parameters: +// +// - No query params: legacy "revoke every scope variant of this role +// from this actor" semantic. Preserves pre-A-4 GUI behaviour +// (KeysPage before Fix 12 fires plain DELETE with no scope; one +// button per role row). +// +// - `scope_type=global` (no scope_id) or +// `scope_type=profile&scope_id=` / +// `scope_type=issuer&scope_id=`: drop ONLY the matching variant. +// Returns HTTP 404 when no row matches the scope (operator +// feedback for typos). Validation mirrors AssignRoleToKey: +// `scope_id` MUST be empty with `scope_type=global`, MUST be +// present with `profile` / `issuer`, anything else → 400. func (h AuthHandler) RevokeRoleFromKey(w http.ResponseWriter, r *http.Request) { caller, err := callerFromRequest(r) if err != nil { @@ -504,7 +525,19 @@ func (h AuthHandler) RevokeRoleFromKey(w http.ResponseWriter, r *http.Request) { } keyID := r.PathValue("id") roleID := r.PathValue("role_id") - if err := h.actors.Revoke(r.Context(), caller, keyID, domain.ActorTypeAPIKey, roleID); err != nil { + + // Parse + validate optional scope filter. Empty query string is + // the legacy path; mismatched filter is rejected before the call + // reaches the service. + scopeTypeRaw := r.URL.Query().Get("scope_type") + scopeIDRaw := r.URL.Query().Get("scope_id") + opts, derr := parseRevokeScope(scopeTypeRaw, scopeIDRaw) + if derr != nil { + Error(w, http.StatusBadRequest, derr.Error()) + return + } + + if err := h.actors.Revoke(r.Context(), caller, keyID, domain.ActorTypeAPIKey, roleID, opts); err != nil { writeAuthError(w, err) return } @@ -515,6 +548,40 @@ func (h AuthHandler) RevokeRoleFromKey(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNoContent) } +// parseRevokeScope translates the (scope_type, scope_id) query string +// into an ActorRoleRevokeOptions. Empty inputs → legacy "revoke all" +// option (zero value); any combination missing required halves → +// validation error. Audit 2026-05-11 A-4 — mirrors AssignRoleToKey's +// scope validation so the assign / revoke pair stays symmetric. +func parseRevokeScope(scopeType, scopeID string) (repository.ActorRoleRevokeOptions, error) { + scopeType = strings.TrimSpace(scopeType) + scopeID = strings.TrimSpace(scopeID) + if scopeType == "" { + if scopeID != "" { + return repository.ActorRoleRevokeOptions{}, fmt.Errorf("scope_id requires scope_type") + } + return repository.ActorRoleRevokeOptions{}, nil + } + switch authdomain.ScopeType(scopeType) { + case authdomain.ScopeTypeGlobal: + if scopeID != "" { + return repository.ActorRoleRevokeOptions{}, fmt.Errorf("scope_id must be empty when scope_type=global") + } + return repository.ActorRoleRevokeOptions{ScopeType: authdomain.ScopeTypeGlobal}, nil + case authdomain.ScopeTypeProfile, authdomain.ScopeTypeIssuer: + if scopeID == "" { + return repository.ActorRoleRevokeOptions{}, fmt.Errorf("scope_id is required when scope_type is profile or issuer") + } + sid := scopeID + return repository.ActorRoleRevokeOptions{ + ScopeType: authdomain.ScopeType(scopeType), + ScopeID: &sid, + }, nil + default: + return repository.ActorRoleRevokeOptions{}, fmt.Errorf("invalid scope_type — must be global, profile, or issuer") + } +} + // Me handles GET /api/v1/auth/me. Returns the current actor's effective // permissions plus admin flag (back-compat with /v1/auth/check). No // permission required: every authenticated caller can read their own. @@ -596,7 +663,7 @@ func writeAuthError(w http.ResponseWriter, err error) { Error(w, http.StatusForbidden, err.Error()) case errors.Is(err, authsvc.ErrInvalidPermission): Error(w, http.StatusBadRequest, err.Error()) - case errors.Is(err, repository.ErrAuthNotFound): + case errors.Is(err, repository.ErrAuthNotFound), errors.Is(err, repository.ErrActorRoleNotFound): Error(w, http.StatusNotFound, "Not found") case errors.Is(err, repository.ErrAuthDuplicateName), errors.Is(err, repository.ErrAuthRoleInUse), errors.Is(err, repository.ErrAuthReservedActor): Error(w, http.StatusConflict, err.Error()) diff --git a/internal/api/handler/auth_test.go b/internal/api/handler/auth_test.go index b9aabbf..8f8da8e 100644 --- a/internal/api/handler/auth_test.go +++ b/internal/api/handler/auth_test.go @@ -122,6 +122,13 @@ type fakeAuthActorSvc struct { revokeErr error roles []*authdomain.ActorRole effective []repository.EffectivePermission + // Audit 2026-05-11 A-4 — capture Revoke opts so tests can assert + // that the handler forwards scope_type / scope_id correctly. + revokeOpts repository.ActorRoleRevokeOptions + revokeCall struct { + actorID, roleID string + called bool + } } func newFakeAuthActorSvc() *fakeAuthActorSvc { @@ -134,7 +141,11 @@ func (f *fakeAuthActorSvc) Grant(_ context.Context, _ *authsvc.Caller, ar *authd f.roles = append(f.roles, ar) return nil } -func (f *fakeAuthActorSvc) Revoke(_ context.Context, _ *authsvc.Caller, _ string, _ domain.ActorType, _ string) error { +func (f *fakeAuthActorSvc) Revoke(_ context.Context, _ *authsvc.Caller, actorID string, _ domain.ActorType, roleID string, opts repository.ActorRoleRevokeOptions) error { + f.revokeCall.called = true + f.revokeCall.actorID = actorID + f.revokeCall.roleID = roleID + f.revokeOpts = opts return f.revokeErr } func (f *fakeAuthActorSvc) ListForActor(_ context.Context, _ *authsvc.Caller, _ string, _ domain.ActorType) ([]*authdomain.ActorRole, error) { @@ -440,7 +451,7 @@ func TestAuthHandler_AssignRoleSelfRoleAssignReturns403(t *testing.T) { } func TestAuthHandler_RevokeRoleFromKey(t *testing.T) { - h, _, _, _ := newAuthHandlerWithFakes() + h, _, _, actorSvc := newAuthHandlerWithFakes() req := withAuthCtx(httptest.NewRequest(http.MethodDelete, "/api/v1/auth/keys/alice/roles/r-viewer", nil), "admin", auth.ActorTypeAPIKey) req.SetPathValue("id", "alice") req.SetPathValue("role_id", "r-viewer") @@ -449,6 +460,136 @@ func TestAuthHandler_RevokeRoleFromKey(t *testing.T) { if rec.Code != http.StatusNoContent { t.Errorf("revoke should be 204; got %d", rec.Code) } + // Audit 2026-05-11 A-4 — no scope params → legacy "revoke all + // variants" semantic propagates as the zero-value + // ActorRoleRevokeOptions to the service layer. + if actorSvc.revokeOpts.ScopeType != "" { + t.Errorf("legacy DELETE forwarded a scope filter: ScopeType=%q", actorSvc.revokeOpts.ScopeType) + } + if actorSvc.revokeOpts.ScopeID != nil { + t.Errorf("legacy DELETE forwarded a scope_id: %v", actorSvc.revokeOpts.ScopeID) + } +} + +// ============================================================================= +// Audit 2026-05-11 A-4 — scope-aware revoke handler tests. +// ============================================================================= + +func TestAuthHandler_RevokeRoleFromKey_A4_ScopedProfile(t *testing.T) { + h, _, _, actorSvc := newAuthHandlerWithFakes() + req := withAuthCtx(httptest.NewRequest(http.MethodDelete, + "/api/v1/auth/keys/alice/roles/r-operator?scope_type=profile&scope_id=p-acme", nil), + "admin", auth.ActorTypeAPIKey) + req.SetPathValue("id", "alice") + req.SetPathValue("role_id", "r-operator") + rec := httptest.NewRecorder() + h.RevokeRoleFromKey(rec, req) + if rec.Code != http.StatusNoContent { + t.Fatalf("scoped revoke should be 204; got %d body=%s", rec.Code, rec.Body.String()) + } + if actorSvc.revokeOpts.ScopeType != authdomain.ScopeTypeProfile { + t.Errorf("ScopeType = %q; want profile", actorSvc.revokeOpts.ScopeType) + } + if actorSvc.revokeOpts.ScopeID == nil || *actorSvc.revokeOpts.ScopeID != "p-acme" { + t.Errorf("ScopeID = %v; want p-acme", actorSvc.revokeOpts.ScopeID) + } +} + +func TestAuthHandler_RevokeRoleFromKey_A4_ScopedGlobal(t *testing.T) { + h, _, _, actorSvc := newAuthHandlerWithFakes() + req := withAuthCtx(httptest.NewRequest(http.MethodDelete, + "/api/v1/auth/keys/alice/roles/r-operator?scope_type=global", nil), + "admin", auth.ActorTypeAPIKey) + req.SetPathValue("id", "alice") + req.SetPathValue("role_id", "r-operator") + rec := httptest.NewRecorder() + h.RevokeRoleFromKey(rec, req) + if rec.Code != http.StatusNoContent { + t.Fatalf("scoped revoke (global) should be 204; got %d body=%s", rec.Code, rec.Body.String()) + } + if actorSvc.revokeOpts.ScopeType != authdomain.ScopeTypeGlobal { + t.Errorf("ScopeType = %q; want global", actorSvc.revokeOpts.ScopeType) + } + if actorSvc.revokeOpts.ScopeID != nil { + t.Errorf("ScopeID must be nil for scope_type=global; got %v", actorSvc.revokeOpts.ScopeID) + } +} + +func TestAuthHandler_RevokeRoleFromKey_A4_RejectsScopeIDWithGlobal(t *testing.T) { + h, _, _, actorSvc := newAuthHandlerWithFakes() + req := withAuthCtx(httptest.NewRequest(http.MethodDelete, + "/api/v1/auth/keys/alice/roles/r-operator?scope_type=global&scope_id=p-acme", nil), + "admin", auth.ActorTypeAPIKey) + req.SetPathValue("id", "alice") + req.SetPathValue("role_id", "r-operator") + rec := httptest.NewRecorder() + h.RevokeRoleFromKey(rec, req) + if rec.Code != http.StatusBadRequest { + t.Errorf("global+scope_id should be 400; got %d body=%s", rec.Code, rec.Body.String()) + } + if actorSvc.revokeCall.called { + t.Error("service should NOT have been called on validation error") + } +} + +func TestAuthHandler_RevokeRoleFromKey_A4_RejectsMissingScopeID(t *testing.T) { + h, _, _, actorSvc := newAuthHandlerWithFakes() + req := withAuthCtx(httptest.NewRequest(http.MethodDelete, + "/api/v1/auth/keys/alice/roles/r-operator?scope_type=profile", nil), + "admin", auth.ActorTypeAPIKey) + req.SetPathValue("id", "alice") + req.SetPathValue("role_id", "r-operator") + rec := httptest.NewRecorder() + h.RevokeRoleFromKey(rec, req) + if rec.Code != http.StatusBadRequest { + t.Errorf("profile-without-scope_id should be 400; got %d body=%s", rec.Code, rec.Body.String()) + } + if actorSvc.revokeCall.called { + t.Error("service should NOT have been called on validation error") + } +} + +func TestAuthHandler_RevokeRoleFromKey_A4_RejectsScopeIDWithoutScopeType(t *testing.T) { + h, _, _, _ := newAuthHandlerWithFakes() + req := withAuthCtx(httptest.NewRequest(http.MethodDelete, + "/api/v1/auth/keys/alice/roles/r-operator?scope_id=p-acme", nil), + "admin", auth.ActorTypeAPIKey) + req.SetPathValue("id", "alice") + req.SetPathValue("role_id", "r-operator") + rec := httptest.NewRecorder() + h.RevokeRoleFromKey(rec, req) + if rec.Code != http.StatusBadRequest { + t.Errorf("scope_id-without-scope_type should be 400; got %d body=%s", rec.Code, rec.Body.String()) + } +} + +func TestAuthHandler_RevokeRoleFromKey_A4_RejectsInvalidScopeType(t *testing.T) { + h, _, _, _ := newAuthHandlerWithFakes() + req := withAuthCtx(httptest.NewRequest(http.MethodDelete, + "/api/v1/auth/keys/alice/roles/r-operator?scope_type=bogus", nil), + "admin", auth.ActorTypeAPIKey) + req.SetPathValue("id", "alice") + req.SetPathValue("role_id", "r-operator") + rec := httptest.NewRecorder() + h.RevokeRoleFromKey(rec, req) + if rec.Code != http.StatusBadRequest { + t.Errorf("bogus scope_type should be 400; got %d", rec.Code) + } +} + +func TestAuthHandler_RevokeRoleFromKey_A4_ScopedNotFoundReturns404(t *testing.T) { + h, _, _, actorSvc := newAuthHandlerWithFakes() + actorSvc.revokeErr = repository.ErrActorRoleNotFound + req := withAuthCtx(httptest.NewRequest(http.MethodDelete, + "/api/v1/auth/keys/alice/roles/r-operator?scope_type=profile&scope_id=p-globex", nil), + "admin", auth.ActorTypeAPIKey) + req.SetPathValue("id", "alice") + req.SetPathValue("role_id", "r-operator") + rec := httptest.NewRecorder() + h.RevokeRoleFromKey(rec, req) + if rec.Code != http.StatusNotFound { + t.Errorf("ErrActorRoleNotFound should be 404; got %d", rec.Code) + } } func TestAuthHandler_RevokeReservedActorReturns409(t *testing.T) { diff --git a/internal/mcp/tools_auth.go b/internal/mcp/tools_auth.go index 6abf2d0..24c0aa0 100644 --- a/internal/mcp/tools_auth.go +++ b/internal/mcp/tools_auth.go @@ -190,9 +190,21 @@ func registerAuthTools(s *gomcp.Server, c *Client) { gomcp.AddTool(s, &gomcp.Tool{ Name: "certctl_auth_revoke_role_from_key", - Description: "Revoke a role from an API key actor (DELETE /v1/auth/keys/{id}/roles/{role_id}). Rejects revocations against the reserved actor-demo-anon (HTTP 409). Permission: auth.role.assign.", + Description: "Revoke a role from an API key actor (DELETE /v1/auth/keys/{id}/roles/{role_id}). Rejects revocations against the reserved actor-demo-anon (HTTP 409). Audit 2026-05-11 A-4: pass scope_type=global / profile / issuer (with scope_id for the latter two) to selectively revoke ONE variant when the actor holds the same role at multiple scopes; omit both for the legacy 'revoke every variant' behaviour. Permission: auth.role.assign.", }, func(ctx context.Context, req *gomcp.CallToolRequest, input AuthRevokeKeyRoleInput) (*gomcp.CallToolResult, any, error) { - data, err := c.Delete("/api/v1/auth/keys/" + input.KeyID + "/roles/" + input.RoleID) + // Audit 2026-05-11 A-4 — append the optional scope filter when + // the caller supplied scope_type. The handler validates the + // pair shape (scope_id required vs forbidden) so we don't + // duplicate that here. + path := "/api/v1/auth/keys/" + input.KeyID + "/roles/" + input.RoleID + if input.ScopeType != "" { + q := "?scope_type=" + url.QueryEscape(input.ScopeType) + if input.ScopeID != "" { + q += "&scope_id=" + url.QueryEscape(input.ScopeID) + } + path += q + } + data, err := c.Delete(path) if err != nil { return errorResult(err) } diff --git a/internal/mcp/tools_per_tool_test.go b/internal/mcp/tools_per_tool_test.go index 7442bb4..8498c33 100644 --- a/internal/mcp/tools_per_tool_test.go +++ b/internal/mcp/tools_per_tool_test.go @@ -439,6 +439,10 @@ var allHappyPathCases = []toolCase{ {"certctl_auth_list_keys", map[string]any{}, http.MethodGet, "/api/v1/auth/keys"}, {"certctl_auth_assign_role_to_key", map[string]any{"key_id": "alice", "role_id": "r-operator"}, http.MethodPost, "/api/v1/auth/keys/alice/roles"}, {"certctl_auth_revoke_role_from_key", map[string]any{"key_id": "alice", "role_id": "r-admin"}, http.MethodDelete, "/api/v1/auth/keys/alice/roles/r-admin"}, + // Audit 2026-05-11 A-4 — scoped revoke append both query params. + {"certctl_auth_revoke_role_from_key", map[string]any{"key_id": "alice", "role_id": "r-operator", "scope_type": "profile", "scope_id": "p-acme"}, http.MethodDelete, "/api/v1/auth/keys/alice/roles/r-operator?scope_type=profile&scope_id=p-acme"}, + // Audit 2026-05-11 A-4 — scoped revoke with global emits only scope_type. + {"certctl_auth_revoke_role_from_key", map[string]any{"key_id": "alice", "role_id": "r-operator", "scope_type": "global"}, http.MethodDelete, "/api/v1/auth/keys/alice/roles/r-operator?scope_type=global"}, // Bundle 2 Phase 9 — OIDC + session tools (11 tools). {"certctl_auth_list_oidc_providers", map[string]any{}, http.MethodGet, "/api/v1/auth/oidc/providers"}, diff --git a/internal/mcp/types.go b/internal/mcp/types.go index fb30779..e186a30 100644 --- a/internal/mcp/types.go +++ b/internal/mcp/types.go @@ -602,9 +602,18 @@ type AuthAssignKeyRoleInput struct { // AuthRevokeKeyRoleInput is the input for // certctl_auth_revoke_role_from_key. +// +// Audit 2026-05-11 A-4 — optional scope_type / scope_id fields narrow +// the revoke to a single (scope_type, scope_id) variant when multiple +// scoped grants of the same role exist. Empty scope_type fires the +// legacy "revoke every variant" semantic. The handler enforces: +// `scope_id` empty when scope_type=global; non-empty when +// scope_type=profile / issuer. type AuthRevokeKeyRoleInput struct { - KeyID string `json:"key_id" jsonschema:"API-key actor ID. Reserved actor-demo-anon is rejected server-side"` - RoleID string `json:"role_id" jsonschema:"Role ID to revoke"` + KeyID string `json:"key_id" jsonschema:"API-key actor ID. Reserved actor-demo-anon is rejected server-side"` + RoleID string `json:"role_id" jsonschema:"Role ID to revoke"` + ScopeType string `json:"scope_type,omitempty" jsonschema:"Optional scope filter: global / profile / issuer. Empty = revoke every scope variant of this role (legacy behaviour)"` + ScopeID string `json:"scope_id,omitempty" jsonschema:"Required when scope_type=profile or issuer; must be empty when scope_type=global"` } // ============================================================================= diff --git a/internal/repository/auth.go b/internal/repository/auth.go index 3c5e789..33e1401 100644 --- a/internal/repository/auth.go +++ b/internal/repository/auth.go @@ -32,8 +32,46 @@ var ( // references a permission name not in the canonical catalog. // Maps to HTTP 400. ErrAuthUnknownPermission = errors.New("auth: permission not in canonical catalog") + + // ErrActorRoleNotFound is returned by ActorRoleRepository.Revoke + // when the caller passes a non-empty `RevokeOptions.ScopeType` that + // doesn't match any persisted (actor, role, scope_type, scope_id) + // tuple. The legacy no-opts "revoke all variants" call never + // returns this — pre-A-4 callers cannot start seeing the error. + // Maps to HTTP 404. Audit 2026-05-11 A-4. + ErrActorRoleNotFound = errors.New("auth: no actor_role row matches the requested scope") ) +// ActorRoleRevokeOptions narrows ActorRoleRepository.Revoke to a +// specific (scope_type, scope_id) variant when set. Audit 2026-05-11 +// A-4 — HIGH-10's UNIQUE (actor, role, scope_type, scope_id, tenant) +// uniqueness extension allows multiple scoped grants of the same role +// to the same actor; without scope plumbing on Revoke an operator who +// granted Alice `r-operator` against both `profile=p-acme` and +// `profile=p-globex` cannot selectively revoke one. +// +// Semantics: +// +// - Zero value (ScopeType="") preserves the legacy "revoke all +// variants" behaviour. Every actor_roles row matching +// (actor_id, actor_type, role_id, tenant_id) is deleted regardless +// of scope. Pre-A-4 callers that don't pass options stay correct. +// +// - Non-empty ScopeType filters to that one variant. `global` +// requires ScopeID==nil; `profile` / `issuer` require +// ScopeID!=nil. The SQL uses `scope_type = $5 AND scope_id IS NOT +// DISTINCT FROM $6` so the NULL case matches cleanly. +// +// - If the filter doesn't match any row, the repository returns +// ErrActorRoleNotFound — the caller (service / handler) maps it +// to HTTP 404. The legacy "revoke all" semantic stays best-effort +// (deleting zero rows is not an error) because the GUI used to +// fire it as a clean-up and operators rely on the idempotence. +type ActorRoleRevokeOptions struct { + ScopeType authdomain.ScopeType + ScopeID *string +} + // TenantRepository wraps the tenants table. Bundle 1 ships single-tenant // (one seeded `t-default`); the future managed-service offering activates // multi-tenant by inserting additional tenants. @@ -89,10 +127,15 @@ type ActorRoleRepository interface { // only if the operator explicitly wires that, which the API // layer rejects. Grant(ctx context.Context, ar *authdomain.ActorRole) error - // Revoke deletes an actor_roles row by (actor_id, actor_type, - // role_id, tenant_id). The API layer must reject revocations - // targeting `actor-demo-anon` to preserve the demo path. - Revoke(ctx context.Context, actorID string, actorType authdomain.ActorTypeValue, roleID, tenantID string) error + // Revoke deletes actor_roles row(s). Without opts (the legacy + // no-options call shape) every variant matching + // (actor_id, actor_type, role_id, tenant_id) is deleted regardless + // of (scope_type, scope_id). With opts.ScopeType set, only the + // matching variant is deleted; no-match returns + // ErrActorRoleNotFound. The API layer must reject revocations + // targeting `actor-demo-anon` to preserve the demo path. Audit + // 2026-05-11 A-4 — see ActorRoleRevokeOptions for semantics. + Revoke(ctx context.Context, actorID string, actorType authdomain.ActorTypeValue, roleID, tenantID string, opts ActorRoleRevokeOptions) error // EffectivePermissions returns the deduplicated set of // (permission_name, scope_type, scope_id) triples granted to the diff --git a/internal/repository/postgres/auth.go b/internal/repository/postgres/auth.go index 52049bd..699611a 100644 --- a/internal/repository/postgres/auth.go +++ b/internal/repository/postgres/auth.go @@ -406,14 +406,59 @@ func (r *ActorRoleRepository) Grant(ctx context.Context, ar *authdomain.ActorRol return nil } -func (r *ActorRoleRepository) Revoke(ctx context.Context, actorID string, actorType authdomain.ActorTypeValue, roleID, tenantID string) error { - _, err := r.db.ExecContext(ctx, ` +// Audit 2026-05-11 A-4 — scope-aware revoke. The pre-fix SQL omitted +// (scope_type, scope_id) from the WHERE clause; combined with HIGH-10's +// UNIQUE (actor_id, actor_type, role_id, scope_type, scope_id, tenant_id) +// uniqueness extension, an operator who granted the same role to the +// same actor at two different scopes had no selective-revoke path — +// every Revoke call nuked both rows. The new behaviour: +// +// - opts.ScopeType == "" (legacy call shape): drop the scope from the +// WHERE clause; delete every variant. Zero-row delete is NOT an +// error (preserves the GUI's pre-A-4 idempotence contract). +// +// - opts.ScopeType != "": narrow WHERE with +// `scope_type = $5 AND scope_id IS NOT DISTINCT FROM $6` (the +// IS-NOT-DISTINCT-FROM handles the `global → scope_id IS NULL` +// case cleanly — Postgres `= NULL` would silently match nothing). +// Zero-row delete IS an error (ErrActorRoleNotFound, mapped to +// HTTP 404 upstream) so operators get feedback when they target a +// scope variant that doesn't exist. +func (r *ActorRoleRepository) Revoke(ctx context.Context, actorID string, actorType authdomain.ActorTypeValue, roleID, tenantID string, opts repository.ActorRoleRevokeOptions) error { + if opts.ScopeType == "" { + // Legacy "revoke all variants" path. Zero-row delete = no-op. + _, err := r.db.ExecContext(ctx, ` + DELETE FROM actor_roles + WHERE actor_id = $1 AND actor_type = $2 AND role_id = $3 AND tenant_id = $4 + `, actorID, string(actorType), roleID, tenantID) + if err != nil { + return fmt.Errorf("actorRole.revoke: %w", err) + } + return nil + } + // Scoped path. `scope_id IS NOT DISTINCT FROM $6` makes + // (global, NULL) match (global, NULL) cleanly — vanilla `=` would + // drop on NULL ≠ NULL. + var scopeID interface{} + if opts.ScopeID != nil && *opts.ScopeID != "" { + scopeID = *opts.ScopeID + } + res, err := r.db.ExecContext(ctx, ` DELETE FROM actor_roles - WHERE actor_id = $1 AND actor_type = $2 AND role_id = $3 AND tenant_id = $4 - `, actorID, string(actorType), roleID, tenantID) + WHERE actor_id = $1 + AND actor_type = $2 + AND role_id = $3 + AND tenant_id = $4 + AND scope_type = $5 + AND scope_id IS NOT DISTINCT FROM $6 + `, actorID, string(actorType), roleID, tenantID, string(opts.ScopeType), scopeID) if err != nil { return fmt.Errorf("actorRole.revoke: %w", err) } + n, _ := res.RowsAffected() + if n == 0 { + return repository.ErrActorRoleNotFound + } return nil } diff --git a/internal/repository/postgres/auth_revoke_scope_test.go b/internal/repository/postgres/auth_revoke_scope_test.go new file mode 100644 index 0000000..8a96287 --- /dev/null +++ b/internal/repository/postgres/auth_revoke_scope_test.go @@ -0,0 +1,231 @@ +package postgres_test + +// Audit 2026-05-11 A-4 closure — scope-aware ActorRoleRepository.Revoke +// regression matrix. Pre-fix, Revoke deleted every (actor,role,tenant) +// row regardless of (scope_type, scope_id), so an operator who held +// the same role at two different profile scopes could only nuke them +// both — selective revoke was unrepresentable. The new semantic: +// +// opts.ScopeType == "" → legacy "revoke all variants" +// opts.ScopeType == global → drop only the (global, NULL) row +// opts.ScopeType == profile|issuer + scope_id → drop only that variant +// no match for a scoped revoke → ErrActorRoleNotFound (HTTP 404) +// +// Helpers (seedRoleWithPerm, grantActorRoleAtScope, ptrStr) live in +// auth_scope_test.go in this same _test package. + +import ( + "context" + "errors" + "testing" + + authdomain "github.com/certctl-io/certctl/internal/domain/auth" + "github.com/certctl-io/certctl/internal/repository" + "github.com/certctl-io/certctl/internal/repository/postgres" +) + +// listActorRoleScopes returns every (scope_type, scope_id-or-empty) +// tuple held by the actor for the role. Lets tests assert which +// variants are still standing after a Revoke. +func listActorRoleScopes(t *testing.T, ctx context.Context, repo *postgres.ActorRoleRepository, actorID, roleID string) []string { + t.Helper() + rows, err := repo.ListByActor(ctx, actorID, authdomain.ActorTypeValue("APIKey"), authdomain.DefaultTenantID) + if err != nil { + t.Fatalf("ListByActor: %v", err) + } + var out []string + for _, r := range rows { + if r.RoleID != roleID { + continue + } + st := string(r.ScopeType) + sid := "" + if r.ScopeID != nil { + sid = *r.ScopeID + } + out = append(out, st+"|"+sid) + } + return out +} + +// TestRevokeActorRole_NoOpts_RemovesAllVariants — pre-A-4 legacy +// semantic. Actor holds the same role at two profile scopes; Revoke +// with zero-value opts must wipe both. +func TestRevokeActorRole_NoOpts_RemovesAllVariants(t *testing.T) { + if testing.Short() { + t.Skip("integration test in short mode") + } + db := getTestDB(t).freshSchema(t) + ctx := context.Background() + roleRepo := postgres.NewRoleRepository(db) + permRepo := postgres.NewPermissionRepository(db) + actorRepo := postgres.NewActorRoleRepository(db) + + roleID := seedRoleWithPerm(t, ctx, roleRepo, permRepo, "rev-noopts", "cert.read", authdomain.ScopeTypeGlobal, nil) + grantActorRoleAtScope(t, ctx, actorRepo, "alice", roleID, authdomain.ScopeTypeProfile, ptrStr("p-acme")) + grantActorRoleAtScope(t, ctx, actorRepo, "alice", roleID, authdomain.ScopeTypeProfile, ptrStr("p-globex")) + + got := listActorRoleScopes(t, ctx, actorRepo, "alice", roleID) + if len(got) != 2 { + t.Fatalf("pre-revoke variants = %v; want 2", got) + } + + if err := actorRepo.Revoke(ctx, "alice", authdomain.ActorTypeValue("APIKey"), roleID, authdomain.DefaultTenantID, repository.ActorRoleRevokeOptions{}); err != nil { + t.Fatalf("Revoke (no-opts): %v", err) + } + got = listActorRoleScopes(t, ctx, actorRepo, "alice", roleID) + if len(got) != 0 { + t.Errorf("legacy revoke left variants: %v; want []", got) + } +} + +// TestRevokeActorRole_WithScope_RemovesOnlyMatching — the A-4 +// selective-revoke happy path. Two scoped grants; revoke one by +// (profile=p-acme); the other (profile=p-globex) stays. +func TestRevokeActorRole_WithScope_RemovesOnlyMatching(t *testing.T) { + if testing.Short() { + t.Skip("integration test in short mode") + } + db := getTestDB(t).freshSchema(t) + ctx := context.Background() + roleRepo := postgres.NewRoleRepository(db) + permRepo := postgres.NewPermissionRepository(db) + actorRepo := postgres.NewActorRoleRepository(db) + + roleID := seedRoleWithPerm(t, ctx, roleRepo, permRepo, "rev-scoped", "cert.read", authdomain.ScopeTypeGlobal, nil) + grantActorRoleAtScope(t, ctx, actorRepo, "alice", roleID, authdomain.ScopeTypeProfile, ptrStr("p-acme")) + grantActorRoleAtScope(t, ctx, actorRepo, "alice", roleID, authdomain.ScopeTypeProfile, ptrStr("p-globex")) + + err := actorRepo.Revoke(ctx, "alice", authdomain.ActorTypeValue("APIKey"), roleID, authdomain.DefaultTenantID, repository.ActorRoleRevokeOptions{ + ScopeType: authdomain.ScopeTypeProfile, + ScopeID: ptrStr("p-acme"), + }) + if err != nil { + t.Fatalf("scoped Revoke: %v", err) + } + got := listActorRoleScopes(t, ctx, actorRepo, "alice", roleID) + if len(got) != 1 || got[0] != "profile|p-globex" { + t.Errorf("post-revoke variants = %v; want [profile|p-globex]", got) + } +} + +// TestRevokeActorRole_WithGlobalScope_RemovesOnlyGlobal — pin the +// `scope_id IS NOT DISTINCT FROM $6` branch. Actor holds global + +// profile-scoped grants of the same role; revoke `scope_type=global` +// drops only the global row. +func TestRevokeActorRole_WithGlobalScope_RemovesOnlyGlobal(t *testing.T) { + if testing.Short() { + t.Skip("integration test in short mode") + } + db := getTestDB(t).freshSchema(t) + ctx := context.Background() + roleRepo := postgres.NewRoleRepository(db) + permRepo := postgres.NewPermissionRepository(db) + actorRepo := postgres.NewActorRoleRepository(db) + + roleID := seedRoleWithPerm(t, ctx, roleRepo, permRepo, "rev-global", "cert.read", authdomain.ScopeTypeGlobal, nil) + grantActorRoleAtScope(t, ctx, actorRepo, "alice", roleID, authdomain.ScopeTypeGlobal, nil) + grantActorRoleAtScope(t, ctx, actorRepo, "alice", roleID, authdomain.ScopeTypeProfile, ptrStr("p-acme")) + + err := actorRepo.Revoke(ctx, "alice", authdomain.ActorTypeValue("APIKey"), roleID, authdomain.DefaultTenantID, repository.ActorRoleRevokeOptions{ + ScopeType: authdomain.ScopeTypeGlobal, + ScopeID: nil, + }) + if err != nil { + t.Fatalf("global-scope Revoke: %v", err) + } + got := listActorRoleScopes(t, ctx, actorRepo, "alice", roleID) + if len(got) != 1 || got[0] != "profile|p-acme" { + t.Errorf("post-revoke variants = %v; want [profile|p-acme]", got) + } +} + +// TestRevokeActorRole_NoMatch_ReturnsNotFound — operator passes a +// (scope_type, scope_id) the actor doesn't actually hold for this +// role. Selective revoke must return ErrActorRoleNotFound; existing +// rows must be untouched. +func TestRevokeActorRole_NoMatch_ReturnsNotFound(t *testing.T) { + if testing.Short() { + t.Skip("integration test in short mode") + } + db := getTestDB(t).freshSchema(t) + ctx := context.Background() + roleRepo := postgres.NewRoleRepository(db) + permRepo := postgres.NewPermissionRepository(db) + actorRepo := postgres.NewActorRoleRepository(db) + + roleID := seedRoleWithPerm(t, ctx, roleRepo, permRepo, "rev-nomatch", "cert.read", authdomain.ScopeTypeGlobal, nil) + grantActorRoleAtScope(t, ctx, actorRepo, "alice", roleID, authdomain.ScopeTypeProfile, ptrStr("p-acme")) + + err := actorRepo.Revoke(ctx, "alice", authdomain.ActorTypeValue("APIKey"), roleID, authdomain.DefaultTenantID, repository.ActorRoleRevokeOptions{ + ScopeType: authdomain.ScopeTypeProfile, + ScopeID: ptrStr("p-globex"), + }) + if !errors.Is(err, repository.ErrActorRoleNotFound) { + t.Errorf("err = %v; want ErrActorRoleNotFound", err) + } + got := listActorRoleScopes(t, ctx, actorRepo, "alice", roleID) + if len(got) != 1 || got[0] != "profile|p-acme" { + t.Errorf("untargeted revoke mutated existing rows: %v", got) + } +} + +// TestRevokeActorRole_NoOpts_NoMatch_IsNoOp — pin the legacy +// idempotence contract. Empty opts + zero matching rows must NOT +// return an error (the GUI's pre-A-4 revoke button fires this when +// the operator clicks "remove" on a stale row). +func TestRevokeActorRole_NoOpts_NoMatch_IsNoOp(t *testing.T) { + if testing.Short() { + t.Skip("integration test in short mode") + } + db := getTestDB(t).freshSchema(t) + ctx := context.Background() + roleRepo := postgres.NewRoleRepository(db) + permRepo := postgres.NewPermissionRepository(db) + actorRepo := postgres.NewActorRoleRepository(db) + + // Seed an unrelated role + grant so the table isn't completely + // empty (more realistic baseline). + otherRole := seedRoleWithPerm(t, ctx, roleRepo, permRepo, "rev-noop-other", "cert.read", authdomain.ScopeTypeGlobal, nil) + grantActorRoleAtScope(t, ctx, actorRepo, "bob", otherRole, authdomain.ScopeTypeGlobal, nil) + + if err := actorRepo.Revoke(ctx, "alice", authdomain.ActorTypeValue("APIKey"), "r-nonexistent", authdomain.DefaultTenantID, repository.ActorRoleRevokeOptions{}); err != nil { + t.Errorf("no-match no-opts Revoke should be nil; got %v", err) + } + // Sanity: the unrelated grant is untouched. + got := listActorRoleScopes(t, ctx, actorRepo, "bob", otherRole) + if len(got) != 1 || got[0] != "global|" { + t.Errorf("unrelated grant mutated: %v", got) + } +} + +// TestRevokeActorRole_IssuerScope_RemovesOnlyMatching — pin the +// issuer-scope variant (the other half of the {profile, issuer} +// scope-type pair). Actor holds the same role across profile + issuer +// scopes; revoke `issuer=iss-x` drops only the issuer row. +func TestRevokeActorRole_IssuerScope_RemovesOnlyMatching(t *testing.T) { + if testing.Short() { + t.Skip("integration test in short mode") + } + db := getTestDB(t).freshSchema(t) + ctx := context.Background() + roleRepo := postgres.NewRoleRepository(db) + permRepo := postgres.NewPermissionRepository(db) + actorRepo := postgres.NewActorRoleRepository(db) + + roleID := seedRoleWithPerm(t, ctx, roleRepo, permRepo, "rev-issuer", "cert.read", authdomain.ScopeTypeGlobal, nil) + grantActorRoleAtScope(t, ctx, actorRepo, "alice", roleID, authdomain.ScopeTypeProfile, ptrStr("p-acme")) + grantActorRoleAtScope(t, ctx, actorRepo, "alice", roleID, authdomain.ScopeTypeIssuer, ptrStr("iss-x")) + + err := actorRepo.Revoke(ctx, "alice", authdomain.ActorTypeValue("APIKey"), roleID, authdomain.DefaultTenantID, repository.ActorRoleRevokeOptions{ + ScopeType: authdomain.ScopeTypeIssuer, + ScopeID: ptrStr("iss-x"), + }) + if err != nil { + t.Fatalf("issuer-scope Revoke: %v", err) + } + got := listActorRoleScopes(t, ctx, actorRepo, "alice", roleID) + if len(got) != 1 || got[0] != "profile|p-acme" { + t.Errorf("post-revoke variants = %v; want [profile|p-acme]", got) + } +} diff --git a/internal/service/auth/actor_role_service.go b/internal/service/auth/actor_role_service.go index a483528..7b9d7c7 100644 --- a/internal/service/auth/actor_role_service.go +++ b/internal/service/auth/actor_role_service.go @@ -72,7 +72,15 @@ func (s *ActorRoleService) Grant(ctx context.Context, caller *Caller, ar *authdo // privilege guard as Grant: caller needs `auth.role.assign` to mutate // role membership. Reserved actor `actor-demo-anon` is rejected so the // demo path stays alive even after a misclick. -func (s *ActorRoleService) Revoke(ctx context.Context, caller *Caller, actorID string, actorType domain.ActorType, roleID string) error { +// +// Audit 2026-05-11 A-4 — opts narrows the revoke to a specific +// (scope_type, scope_id) variant. Zero value preserves the legacy +// "revoke all variants" behaviour. When opts.ScopeType is set the +// repository returns repository.ErrActorRoleNotFound if no row matches; +// the handler maps it to HTTP 404. The audit row records the scope so +// operators can distinguish "wide revoke" from "selective revoke" in +// the SIEM. +func (s *ActorRoleService) Revoke(ctx context.Context, caller *Caller, actorID string, actorType domain.ActorType, roleID string, opts repository.ActorRoleRevokeOptions) error { if caller == nil { return ErrUnauthenticated } @@ -89,14 +97,23 @@ func (s *ActorRoleService) Revoke(ctx context.Context, caller *Caller, actorID s return fmt.Errorf("%w: actor-demo-anon is reserved", repository.ErrAuthReservedActor) } tenantID := s.tenantOf(caller) - if err := s.repo.Revoke(ctx, actorID, authdomain.ActorTypeValue(actorType), roleID, tenantID); err != nil { + if err := s.repo.Revoke(ctx, actorID, authdomain.ActorTypeValue(actorType), roleID, tenantID, opts); err != nil { return err } - s.recordAudit(ctx, caller, "actor_role.revoke", "actor_role", roleID, map[string]interface{}{ + details := map[string]interface{}{ "actor_id": actorID, "actor_type": string(actorType), "role_id": roleID, - }) + } + if opts.ScopeType != "" { + details["scope_type"] = string(opts.ScopeType) + if opts.ScopeID != nil { + details["scope_id"] = *opts.ScopeID + } + } else { + details["scope"] = "all_variants" + } + s.recordAudit(ctx, caller, "actor_role.revoke", "actor_role", roleID, details) return nil } diff --git a/internal/service/auth/service_test.go b/internal/service/auth/service_test.go index 4f5535e..908e0f8 100644 --- a/internal/service/auth/service_test.go +++ b/internal/service/auth/service_test.go @@ -159,15 +159,53 @@ func (f *fakeActorRoleRepo) Grant(_ context.Context, ar *authdomain.ActorRole) e f.grants = append(f.grants, ar) return nil } -func (f *fakeActorRoleRepo) Revoke(_ context.Context, actorID string, actorType authdomain.ActorTypeValue, roleID, _ string) error { +func (f *fakeActorRoleRepo) Revoke(_ context.Context, actorID string, actorType authdomain.ActorTypeValue, roleID, _ string, opts repository.ActorRoleRevokeOptions) error { + // Audit 2026-05-11 A-4 — mirror the postgres semantics: when + // opts.ScopeType is empty, remove every (actor,type,role) match + // regardless of scope (legacy). When set, narrow to the single + // matching scope variant; zero-match returns ErrActorRoleNotFound. + wantScopedSpecific := opts.ScopeType != "" + matched := 0 out := f.grants[:0] for _, g := range f.grants { if g.ActorID == actorID && g.ActorType == actorType && g.RoleID == roleID { + if wantScopedSpecific { + // Match by (scope_type, scope_id). + gScope := g.ScopeType + if gScope == "" { + gScope = authdomain.ScopeTypeGlobal + } + if gScope != opts.ScopeType { + out = append(out, g) + continue + } + // scope_id IS NOT DISTINCT FROM: + gSID := "" + if g.ScopeID != nil { + gSID = *g.ScopeID + } + wSID := "" + if opts.ScopeID != nil { + wSID = *opts.ScopeID + } + if gSID != wSID { + out = append(out, g) + continue + } + // Drop this row. + matched++ + continue + } + // Legacy "revoke all variants" path. + matched++ continue } out = append(out, g) } f.grants = out + if wantScopedSpecific && matched == 0 { + return repository.ErrActorRoleNotFound + } return nil } func (f *fakeActorRoleRepo) AdminExists(_ context.Context, _ string) (bool, error) { @@ -439,7 +477,7 @@ func TestActorRoleService_GrantRejectsReservedDemoActor(t *testing.T) { func TestActorRoleService_RevokeRejectsReservedDemoActor(t *testing.T) { svc, _, _ := newActorRoleServiceWithFakes() - err := svc.Revoke(context.Background(), AsSystemCaller(), authdomain.DemoAnonActorID, domain.ActorTypeAnonymous, "r-admin") + err := svc.Revoke(context.Background(), AsSystemCaller(), authdomain.DemoAnonActorID, domain.ActorTypeAnonymous, "r-admin", repository.ActorRoleRevokeOptions{}) if !errors.Is(err, repository.ErrAuthReservedActor) { t.Errorf("Revoke against actor-demo-anon should be rejected; got %v", err) } @@ -848,19 +886,19 @@ func TestActorRoleService_ListKeysGates(t *testing.T) { func TestActorRoleService_RevokeGatesAndSucceeds(t *testing.T) { svc, repo, audit := newActorRoleServiceWithFakes() // nil caller - if err := svc.Revoke(context.Background(), nil, "alice", domain.ActorTypeAPIKey, "r-admin"); !errors.Is(err, ErrUnauthenticated) { + if err := svc.Revoke(context.Background(), nil, "alice", domain.ActorTypeAPIKey, "r-admin", repository.ActorRoleRevokeOptions{}); !errors.Is(err, ErrUnauthenticated) { t.Errorf("Revoke(nil) err = %v; want ErrUnauthenticated", err) } // caller without auth.role.assign caller := &Caller{ActorID: "bob", ActorType: domain.ActorTypeAPIKey} - if err := svc.Revoke(context.Background(), caller, "alice", domain.ActorTypeAPIKey, "r-admin"); !errors.Is(err, ErrSelfRoleAssignment) { + if err := svc.Revoke(context.Background(), caller, "alice", domain.ActorTypeAPIKey, "r-admin", repository.ActorRoleRevokeOptions{}); !errors.Is(err, ErrSelfRoleAssignment) { t.Errorf("Revoke(no perm) err = %v; want ErrSelfRoleAssignment", err) } // system caller success repo.grants = []*authdomain.ActorRole{ {ID: "ar-1", ActorID: "alice", ActorType: authdomain.ActorTypeValue(domain.ActorTypeAPIKey), RoleID: "r-admin", TenantID: authdomain.DefaultTenantID}, } - if err := svc.Revoke(context.Background(), AsSystemCaller(), "alice", domain.ActorTypeAPIKey, "r-admin"); err != nil { + if err := svc.Revoke(context.Background(), AsSystemCaller(), "alice", domain.ActorTypeAPIKey, "r-admin", repository.ActorRoleRevokeOptions{}); err != nil { t.Fatalf("Revoke(system) err: %v", err) } if len(audit.calls) != 1 || audit.calls[0].Action != "actor_role.revoke" { @@ -878,7 +916,7 @@ func TestActorRoleService_RevokeSucceedsWithAuthRoleAssign(t *testing.T) { {ID: "ar-1", ActorID: "carol", ActorType: authdomain.ActorTypeValue(domain.ActorTypeAPIKey), RoleID: "r-viewer", TenantID: authdomain.DefaultTenantID}, } caller := &Caller{ActorID: "alice", ActorType: domain.ActorTypeAPIKey} - if err := svc.Revoke(context.Background(), caller, "carol", domain.ActorTypeAPIKey, "r-viewer"); err != nil { + if err := svc.Revoke(context.Background(), caller, "carol", domain.ActorTypeAPIKey, "r-viewer", repository.ActorRoleRevokeOptions{}); err != nil { t.Fatalf("Revoke(perm) err: %v", err) } if len(audit.calls) != 1 || audit.calls[0].Action != "actor_role.revoke" { diff --git a/web/src/api/client.ts b/web/src/api/client.ts index e574573..f944bf0 100644 --- a/web/src/api/client.ts +++ b/web/src/api/client.ts @@ -383,10 +383,27 @@ export const authOIDCTestProvider = (body: { body: JSON.stringify(body), }); -export const authRevokeKeyRole = (keyId: string, roleId: string) => - fetchJSON(`${BASE}/auth/keys/${keyId}/roles/${roleId}`, { - method: 'DELETE', - }); +// Audit 2026-05-11 A-4 — optional scope filter. When opts is omitted +// or scope_type is empty, the server runs the legacy "revoke every +// scope variant of this role" semantic (preserves pre-A-4 GUI +// behaviour). When scope_type is set, only the matching variant is +// dropped; server enforces scope_id presence vs absence per +// scope_type. Useful when one actor holds the same role scoped to +// multiple profiles / issuers and the operator wants to drop one +// without touching the others. +export const authRevokeKeyRole = ( + keyId: string, + roleId: string, + opts?: { scope_type?: string; scope_id?: string }, +) => { + let path = `${BASE}/auth/keys/${keyId}/roles/${roleId}`; + if (opts?.scope_type) { + const params = new URLSearchParams({ scope_type: opts.scope_type }); + if (opts.scope_id) params.set('scope_id', opts.scope_id); + path += `?${params.toString()}`; + } + return fetchJSON(path, { method: 'DELETE' }); +}; export interface BootstrapAvailability { available: boolean;