feat(authz): central authorization layer — PDP interface, manifest, builtin driver (BCH-1313)#426
feat(authz): central authorization layer — PDP interface, manifest, builtin driver (BCH-1313)#426gusfcarvalho wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThis PR introduces a pluggable authorization system. A new ChangesPluggable Authorization System
Sequence Diagram(s)sequenceDiagram
participant Client
participant EchoRouter
participant Authorizer
participant builtinPDP
participant DB
Client->>EchoRouter: POST /evidence
EchoRouter->>Authorizer: Authorize("evidence", "create", WithPublicAccess(...))
Authorizer->>Authorizer: subjectFromContext → Subject{user/agent/anon}
Authorizer->>builtinPDP: Evaluate(ctx, Subject, "create", Resource{evidence}, {allow_public: bool})
alt Subject is authenticated
builtinPDP-->>Authorizer: Decision{Allow: true}
else Subject is anonymous and allow_public=true
builtinPDP-->>Authorizer: Decision{Allow: true}
else Subject is anonymous and allow_public=false
builtinPDP-->>Authorizer: Decision{Allow: false}
Authorizer-->>Client: 403 forbidden
end
Authorizer->>EchoRouter: next(c)
EchoRouter-->>Client: 200 OK
Client->>EchoRouter: GET /admin (protected)
EchoRouter->>Authorizer: Authorize("admin", "manage") [via RequireAdminGroups]
Authorizer->>builtinPDP: Evaluate(ctx, Subject, "manage", Resource{admin}, {})
builtinPDP->>DB: load user by email
builtinPDP->>DB: load SSOUserLink
builtinPDP->>DB: resolve provider config + required groups
alt required groups satisfied
builtinPDP-->>Authorizer: Decision{Allow: true}
Authorizer->>EchoRouter: next(c)
else groups missing
builtinPDP-->>Authorizer: Decision{Allow: false}
Authorizer-->>Client: 403 forbidden
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/middleware/admin.go`:
- Around line 19-20: The RequireAdminGroups function creates a new Authorizer
instance on each invocation through NewAuthorizerFromConfig, causing redundant
manifest parsing and driver instantiation across 6+ call sites in api.go.
Refactor RequireAdminGroups to accept an *Authorizer parameter instead of
creating a new one, then update all call sites in api.go to pass a shared
authorizer instance. This eliminates the redundant work while maintaining the
helper function's convenience during the migration phase.
In `@internal/authz/builtin_test.go`:
- Around line 93-180: Add a new test case within the TestBuiltin_Admin function
that verifies the behavior when a database operational error occurs during SSO
link lookup. Create a test scenario using t.Run that mocks or forces the link
lookup query to fail with an operational error (not ErrRecordNotFound), then
call pdp.Evaluate with an SSO-configured user and assert that it returns an
error rather than silently denying access with Allow set to false. This ensures
the authorization decision fails safely when encountering database errors
instead of treating all failures the same way.
In `@internal/authz/builtin.go`:
- Around line 119-125: The error handling in the database query for fetching the
SSO link currently treats all query errors identically by returning a deny
decision, which incorrectly maps operational database failures to 403 responses
instead of surfacing them as 500 internal errors. You need to differentiate
between a missing record error (which legitimately means no SSO link exists for
the user) and other database errors (which are operational failures). Check the
error returned from the First call to determine if it is specifically a
record-not-found error versus other error types, then handle each case
appropriately: for record-not-found return the current deny decision, and for
other errors return an error that will trigger a fail-closed response instead of
allowing the deny decision.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d1e8b512-06f1-4c71-bc91-ddc0f6269fad
📒 Files selected for processing (14)
internal/api/handler/api.gointernal/api/middleware/admin.gointernal/api/middleware/authorize.gointernal/api/middleware/authorize_test.gointernal/authz/builtin.gointernal/authz/builtin_test.gointernal/authz/manifest.gointernal/authz/manifest.yamlinternal/authz/manifest_test.gointernal/authz/pdp.gointernal/authz/registry.gointernal/authz/registry_test.gointernal/config/authz.gointernal/config/config.go
| if err := p.db.WithContext(ctx). | ||
| Where("user_id = ? AND deleted_at IS NULL", user.ID.String()). | ||
| Order("last_sync DESC"). | ||
| First(&link).Error; err != nil { | ||
| p.logger.Warnw("Missing SSO link for admin enforcement", "userID", user.ID.String(), "error", err) | ||
| return Decision{Allow: false, Reason: "missing SSO link for user"}, nil | ||
| } |
There was a problem hiding this comment.
Differentiate missing SSO-link from DB failures in admin evaluation.
Line 122 currently maps all query errors to a deny decision. That collapses operational DB failures into 403s instead of surfacing them as internal errors (500 in fail-closed), which breaks the intended error contract.
Proposed fix
if err := p.db.WithContext(ctx).
Where("user_id = ? AND deleted_at IS NULL", user.ID.String()).
Order("last_sync DESC").
First(&link).Error; err != nil {
- p.logger.Warnw("Missing SSO link for admin enforcement", "userID", user.ID.String(), "error", err)
- return Decision{Allow: false, Reason: "missing SSO link for user"}, nil
+ if errors.Is(err, gorm.ErrRecordNotFound) {
+ p.logger.Warnw("Missing SSO link for admin enforcement", "userID", user.ID.String(), "error", err)
+ return Decision{Allow: false, Reason: "missing SSO link for user"}, nil
+ }
+ p.logger.Errorw("Failed to load SSO link for admin enforcement", "userID", user.ID.String(), "error", err)
+ return Decision{}, fmt.Errorf("load sso link for admin enforcement: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := p.db.WithContext(ctx). | |
| Where("user_id = ? AND deleted_at IS NULL", user.ID.String()). | |
| Order("last_sync DESC"). | |
| First(&link).Error; err != nil { | |
| p.logger.Warnw("Missing SSO link for admin enforcement", "userID", user.ID.String(), "error", err) | |
| return Decision{Allow: false, Reason: "missing SSO link for user"}, nil | |
| } | |
| if err := p.db.WithContext(ctx). | |
| Where("user_id = ? AND deleted_at IS NULL", user.ID.String()). | |
| Order("last_sync DESC"). | |
| First(&link).Error; err != nil { | |
| if errors.Is(err, gorm.ErrRecordNotFound) { | |
| p.logger.Warnw("Missing SSO link for admin enforcement", "userID", user.ID.String(), "error", err) | |
| return Decision{Allow: false, Reason: "missing SSO link for user"}, nil | |
| } | |
| p.logger.Errorw("Failed to load SSO link for admin enforcement", "userID", user.ID.String(), "error", err) | |
| return Decision{}, fmt.Errorf("load sso link for admin enforcement: %w", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/authz/builtin.go` around lines 119 - 125, The error handling in the
database query for fetching the SSO link currently treats all query errors
identically by returning a deny decision, which incorrectly maps operational
database failures to 403 responses instead of surfacing them as 500 internal
errors. You need to differentiate between a missing record error (which
legitimately means no SSO link exists for the user) and other database errors
(which are operational failures). Check the error returned from the First call
to determine if it is specifically a record-not-found error versus other error
types, then handle each case appropriately: for record-not-found return the
current deny decision, and for other errors return an error that will trigger a
fail-closed response instead of allowing the deny decision.
There was a problem hiding this comment.
reviewer conflict detected — pausing until resolved.
This contradicts a deliberate, documented design decision for BCH-1313 (git notes / lisa-design): the acceptance bar is ZERO behavior change, replicating middleware.RequireAdminGroups exactly. The original RequireAdminGroups returned 403 on ANY error from the SSO-link lookup (not only RecordNotFound); builtin.evaluateAdmin preserves that on purpose, so returning 500 on operational DB errors here would deviate from the original behavior. The point has merit: the user-load path DOES distinguish RecordNotFound(403) from DB-error(500), so the SSO-link path is internally inconsistent with it, and the access-control outcome is identical either way (request rejected, fail-closed) -- this is an error-contract refinement, not an access-rule change. Pausing for a human decision: relax strict zero-behavior-change so the SSO-link DB-error path returns 500 (consistent with user-load), or keep exact parity with the original 403-on-any-error for Phase 1? If we adopt 500, I will also add the regression test requested in the builtin_test.go thread.
Please resolve the conflicting feedback and re-request review.
There was a problem hiding this comment.
@gusfcarvalho, thank you for the detailed conflict write-up — this is exactly the kind of context that's useful to preserve.
My position, given the full picture:
You are correct that the original RequireAdminGroups returned 403 on any SSO-link error, and the current SSO-link path faithfully preserves that. Under a strict zero-behavior-change bar, there's nothing wrong here.
However, the user-load path in the same function already departs from strict zero-behavior-change:
| Condition | Original RequireAdminGroups | New evaluateAdmin |
|---|---|---|
| User not found | 403 | 403 ✅ |
| DB error on user load | (likely 500 via unhandled error) | 500 ✅ |
| SSO link not found | 403 | 403 ✅ |
| DB error on SSO link load | 403 | 403 ← the inconsistency |
The user-load path already sets the precedent that "operational DB errors → propagated error → 500" is acceptable in evaluateAdmin. The SSO-link path deviates from that internal contract established within this PR, not from the original middleware.
Recommendation: Since the user-load path already relaxes the zero-behavior-change bar (returning 500 on DB errors), aligning the SSO-link path with it is a code-quality improvement that does not change access-control semantics (both paths reject the request). I'd recommend adopting the fix and adding the regression test — especially since the pattern is already established in the same function.
That said, if the maintainer decides to defer this to Phase 2 to keep the Phase 1 diff minimal, I can open a follow-up issue to track it. Would you like me to do that?
🧠 Learnings used
Learnt from: gusfcarvalho
Repo: compliance-framework/api PR: 404
File: internal/service/relational/workflows/workflow_step_definition_service.go:53-53
Timestamp: 2026-05-26T14:35:41.915Z
Learning: When reviewing Go code that uses GORM (gorm.io/gorm v1.x), be aware that `gorm.Config` embeds the `Dialector` interface and `gorm.DB` embeds `*Config`, so Go method promotion makes `Dialector.Name()` available as an idiomatic `db.Name()` call. Therefore, do not treat `db.Name()` as a compilation error on `*gorm.DB`, and linters/review guidance may prefer `db.Name()` over `db.Dialector.Name()` when both are equivalent.
1 participant
BCH-1313 — AuthZ Phase 1
Introduces CCF's central authorization layer behind one PEP middleware + a pluggable PDP driver interface, with zero behavior change. Existing access rules are unchanged; they are now expressed through the new layer.
Jira: https://container-solutions.atlassian.net/browse/BCH-1313
Design: CCF Pluggable Authorization — Design Plan
What's in this PR
internal/authzpackage:PDPinterface (Evaluate+ batchEvaluations),Subject/Resource/Decision/EvalRequesttypes, and a driver registry (authz.Register/Open) mirroringdatabase/sql.internal/authz/manifest.yaml— engine-neutral vocabulary covering the migrated routes (evidence, admin). Attributes are intentionally minimal/provisional (the authoritative per-resource attribute surface is gated on BCH-1319).builtindriver reproducing today's rules exactly: authenticated ⇒ allowed; admin routes require the SSO admin groups previously enforced byRequireAdminGroups. As the in-process default it self-fetches the admin facts it needs (user row, SSO link, provider config).mw.Authorizer.Authorize(resource, action)PEP middleware: builds the subject from user/agent claims, calls the PDP, enforces the result. Deny → 403 (reason logged, never echoed); evaluator error honoursauthz.fail_mode(default closed → 500 for internal failures, preserving prior behavior).RequireAdminGroupsis now a thin wrapper over the PEP (Authorize("admin","manage")); the evidence-create route (evidence_auth.go/OptionalUserOrAgentJWTMiddleware) gainsAuthorize("evidence","create")while keeping its authentication semantics (401 for missing creds, 403 for bad agent keys) intact.authz_driver(CCF_AUTHZ_DRIVER, defaultbuiltin) andauthz_fail_mode(CCF_AUTHZ_FAIL_MODE, defaultclosed).Notable decisions (see
git notes show/lisa-design)evidence_auth.gostays the authn layer.RequireAdminGroupsbranch-for-branch; only the user-load path distinguishes not-found (deny/403) from a DB error (500).Testing
go build ./...,go vet ./..., full unit suite, and the evidence, templates (admin) and workflow-import (admin) integration suites all pass — confirming zero behavior change.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Refactor