mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-07 22:31:36 +00:00
0152bdf567
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.
232 lines
9.6 KiB
Go
232 lines
9.6 KiB
Go
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)
|
|
}
|
|
}
|