2 Commits

Author	SHA1	Message	Date
shankar0123	0152bdf567	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.	2026-05-11 10:50:34 +00:00
shankar0123	cbb47aaf5d	auth-bundle-1 Phase 11 + 12: RBAC MCP tools + negative-test coverage gate ... # Phase 11 — RBAC MCP tools 12 new tools in internal/mcp/tools_auth.go mirroring the Phase-4 + Phase-7 HTTP surface so operators driving certctl from Claude / VS Code / any MCP client get the same management capability the GUI + CLI already expose: certctl_auth_me GET /v1/auth/me certctl_auth_list_roles GET /v1/auth/roles certctl_auth_get_role GET /v1/auth/roles/{id} certctl_auth_create_role POST /v1/auth/roles certctl_auth_update_role PUT /v1/auth/roles/{id} certctl_auth_delete_role DELETE /v1/auth/roles/{id} certctl_auth_list_permissions GET /v1/auth/permissions certctl_auth_add_permission_to_role POST /v1/auth/roles/{id}/permissions certctl_auth_remove_permission_from_role DELETE /v1/auth/roles/{id}/permissions/{perm} certctl_auth_list_keys GET /v1/auth/keys certctl_auth_assign_role_to_key POST /v1/auth/keys/{id}/roles certctl_auth_revoke_role_from_key DELETE /v1/auth/keys/{id}/roles/{role_id} Each tool routes through the existing HTTP client (no parallel business logic), so permission gates fire server-side: a non-admin caller's MCP tool invocation returns whatever 403 the underlying HTTP handler emits, fenced via errorResult for LLM- prompt-injection defense. Input types in internal/mcp/types.go (AuthRoleIDInput, AuthCreateRoleInput, AuthUpdateRoleInput, AuthRolePermissionGrantInput, AuthRolePermissionRevokeInput, AuthAssignKeyRoleInput, AuthRevokeKeyRoleInput) carry jsonschema descriptions so the MCP consumer's tool catalogue shows operator-friendly hints. internal/mcp/tools_auth_test.go ships 14 tests: - TestAuthMCP_AllToolsRegister (registration must not panic) - TestAuthMCP_PathsAndMethods (table-driven, 12 rows pinning each tool's HTTP method + URL) - TestAuthMCP_ForbiddenSurfacesFencedError (12 tools × 403 mock → error surface) internal/mcp/tools_per_tool_test.go's allHappyPathCases extended with the 12 new rows so the in-memory dispatch coverage gate (TestMCP_RegisterTools_DispatchableToolCount) stays green at the new total of 139 registered tools. Re-derived total via 'grep -cE "gomcp\.AddTool\(" internal/mcp/tools*.go': 133 (121 in tools.go + 12 in tools_auth.go). # Phase 12 — negative-test coverage gate Audit of the prompt's 12 negative-test paths against existing coverage: 1. Missing actor → 401 ✓ TestRequirePermission_NoActorReturns401, TestRBACGate_NoActorReturns401 2. No roles → 403 ✓ TestRequirePermission_DeniedActorReturns403, TestRBACGate_AuditorRole_403sOnAdminRoutes 3. Role lacks specific perm → 403 ✓ same suite 4. Wrong scope → 403 ✓ TestAuthorizer_SpecificScopeMatchesExactID (wrongID arm) 5. Self-grant w/o auth.role.assign → 403 ✓ TestActorRoleService_GrantRequiresAuthRoleAssign 6. Bootstrap token wrong → 401 ✓ TestEnvTokenStrategy_WrongTokenReturnsInvalidToken, TestBootstrapHandler_Mint_WrongToken_401 7. Bootstrap used twice → 410 ✓ TestEnvTokenStrategy_OneShotConsumption, TestBootstrapHandler_Mint_TwiceReturns410 8. Bootstrap when admin exists → 410 ✓ TestEnvTokenStrategy_AdminExistsClosesPath, TestBootstrapHandler_Mint_AdminExists410 9. Role delete with assignees → 409 NEW: TestRoleService_DeleteWithActorsAssignedReturns409 10. Profile-edit loophole → gated ✓ TestProfileEdit_RequiresApprovalLoopholeClosed 11. Permission not in catalog → 400 ✓ TestRoleService_AddPermissionRejectsNonCanonical 12. Scope ID for nonexistent resource → 404 (validation deferred — no FK constraint between role_permissions.scope_id and the resource tables; documented for a future bundle) Filled the gap at #9 with TestRoleService_DeleteWithActorsAssignedReturns409 which pins the repository sentinel pass-through (postgres FK ON DELETE RESTRICT → repository.ErrAuthRoleInUse → service returns the sentinel verbatim → handler maps to HTTP 409). # Coverage gates .github/coverage-thresholds.yml gains 2 entries: - internal/auth: floor 85 - internal/service/auth: floor 85 .github/workflows/ci.yml's coverage test command extended with ./internal/auth/... and ./internal/api/router/... so the threshold check has data to evaluate. # Protocol-endpoint not-gated test (Category F) internal/api/router/phase12_protocol_allowlist_test.go (new) adds 3 router-level invariant tests: - TestPhase12_ProtocolEndpointsNotGated: AST-walks router.go, asserts no rbacGate(...) call references a path under any protocol-endpoint prefix (/acme, /scep, /.well-known/est, /.well-known/pki/ocsp, /.well-known/pki/crl). - TestPhase12_IsProtocolEndpoint_CoversCanonicalPrefixes: pins auth.IsProtocolEndpoint against the canonical prefix set; if a future protocol lands without lockstep allowlist update, this fails. - TestPhase12_RBACGateRoutesAreUnderAPIv1: belt-and-braces — every rbacGate-wrapped route MUST start with /api/v1/. Catches accidental cross-prefix wraps. Complements the existing TestRequirePermission_ProtocolEndpointBypassesGate (middleware-level) + TestRouter_AuthExemptAllowlist_PinsActualRegistrations (allowlist drift) so the Category F invariant is pinned at all three layers (middleware + router + dispatch). # Verifications * gofmt clean repo-wide. * go vet ./... clean. * staticcheck across internal/auth + handler + router + cli + service + repository + cmd + domain + mcp: clean. * go test -short -count=1 green across internal/auth (incl. bootstrap), internal/api/handler, internal/api/router, internal/cli, internal/service (incl. auth), internal/domain/auth, internal/mcp, cmd/server, cmd/cli.	2026-05-09 23:46:01 +00:00