fix(auth/rbac): scope-aware ActorRole revoke (A-4)

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.
This commit is contained in:
shankar0123
2026-05-11 10:50:34 +00:00
parent a123263498
commit 0152bdf567
13 changed files with 715 additions and 32 deletions
+70 -3
View File
@@ -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=<id>` /
// `scope_type=issuer&scope_id=<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())