Merge Fix 04 (HIGH A-4): scope-aware ActorRole revoke

2026-06-07 13:51:36 +00:00 · 2026-05-11 11:16:24 +00:00
parent df53b80cb6 0152bdf567
commit 68af18d081
13 changed files with 715 additions and 32 deletions
@@ -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
@@ -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
 }

@@ -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)
+	}
+}