feat(authz): enforce disabled org state on the authorization path - #1860
feat(authz): enforce disabled org state on the authorization path#1860AmanGIT07 wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change enforces active-organization checks for authorized resources and personal access tokens. It adds organization resolution, PAT endpoint validation, invitation authorization fallback handling, generated mocks, and table-driven tests. ChangesAuthorization enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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: 1
🧹 Nitpick comments (2)
internal/api/v1beta1connect/org_state.go (1)
78-157: 🚀 Performance & Scalability | 🔵 TrivialConsider caching organization state for the authorization hot path.
resolveObjectOrgruns after every successful permission check. For a project target it adds one project read plus one organization read. For a custom resource it adds a resource read, a project read, and an organization read. This applies to every authorized RPC, so read load on those tables increases with total request volume.A short-lived cache of organization state, or a cached project-to-organization mapping, would remove most of this overhead. Track the added latency before and after rollout.
internal/api/v1beta1connect/authorize_test.go (1)
91-334: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
Internalbranches of organization resolution.The table covers the not-found and invalid-ID branches of
resolveObjectOrg. It does not cover the branches that wrap an unexpected service error intoconnect.CodeInternal. Those areinternal/api/v1beta1connect/org_state.golines 89, 101, 118, 128, 147, and 154. A regression that swallows such an error would let a request through the gate without any test failing.One case per resolution path is enough. For example, make
projectSvc.Getreturn a generic error and assert the code isconnect.CodeInternal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 287f32f5-f6ae-46cc-bf98-453d77482eec
📒 Files selected for processing (6)
internal/api/v1beta1connect/authorize.gointernal/api/v1beta1connect/authorize_test.gointernal/api/v1beta1connect/interfaces.gointernal/api/v1beta1connect/mocks/user_pat_service.gointernal/api/v1beta1connect/org_state.gopkg/server/connect_interceptors/authorization.go
Coverage Report for CI Build 31157939344Coverage increased (+0.2%) to 48.154%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
dd838bd to
a6afcc2
Compare
Resolve every authorized object to its owning org and reject requests with FailedPrecondition when that org is disabled. Superusers bypass the gate so they can inspect and re-enable orgs. PAT self-management RPCs now authorize through the same gate instead of skipping authorization. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
a6afcc2 to
c06d96a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/api/v1beta1connect/authorize_test.go (2)
111-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a generic
orgService.Getfailure.The table covers
organization.ErrDisabledandorganization.ErrNotExist. It does not cover a generic error fromorgService.Get. That path decides whether an infrastructure failure surfaces asCodeInternaland whether it fails closed. Add a case that returns a plain error and asserts the resulting code.🧪 Proposed additional table case
+ { + name: "org lookup failure surfaces as internal", + object: relation.Object{Namespace: schema.OrganizationNamespace, ID: testAuthzOrgID}, + setup: func(m authorizeMocks, object relation.Object) { + expectUserPrincipal(m) + expectCheckAuthz(m, object, testAuthzUserID, schema.GetPermission, true, nil) + m.orgSvc.EXPECT().Get(mock.Anything, testAuthzOrgID).Return(organization.Organization{}, errors.New("db down")) + }, + wantErr: /* expected connect error from ensureOrgEnabled */, + },
346-374: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the namespace coverage list exhaustive.
resolveObjectOrgalso short-circuits constants such asschema.SuperUserPrincipalandschema.GroupPrincipal, butTestResolveObjectOrgNamespacesonly covers namespaces it can exercise through theresolvedmap andorglessslice. Add a test that the covered constants match the schema constants, or include the uncovered constants so the documented checklist actually requires them.internal/api/v1beta1connect/authorize.go (1)
84-85: 🚀 Performance & Scalability | 🔵 TrivialReduce the lookup cost added on the authorization path.
ensureObjectOrgActivecallsresolveObjectOrgfor each authorized request. A custom resource path performsresourceService.Getfollowed byprojectService.Getbefore checking organization state, and neither service implements cache-backedGetlookups in the checked code. Add a short TTL cache for organization/project metadata, or provide a cached path for these authorization lookups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a308f39-1a33-490b-bae3-57431c0c16f9
📒 Files selected for processing (10)
.mockery.yamlcore/resource/mocks/authn_service.gocore/resource/mocks/org_service.gocore/resource/mocks/policy_service.gocore/resource/service.gointernal/api/v1beta1connect/authorize.gointernal/api/v1beta1connect/authorize_test.gointernal/api/v1beta1connect/mocks/user_pat_service.gointernal/api/v1beta1connect/org_state.gopkg/server/connect_interceptors/authorization.go
💤 Files with no reviewable changes (1)
- core/resource/mocks/authn_service.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/server/connect_interceptors/authorization.go
- internal/api/v1beta1connect/org_state.go
Manual verificationVerified against a local server built from this branch, exercised over ConnectRPC. Codes below are the ConnectRPC error codes returned to the caller. Platform admins bypass the gate, except on group and domain RPCs, whose handlers keep their own org-state check (handler cleanup is a TODO). Enabled org — baseline
Disabled org — core resource RPCs
The invitee (the user the invitation was addressed to) also gets FailedPrecondition on GetOrganizationInvitation while the org is disabled. Object types the gate resolves to an orgEach row was reached through a real RPC. Member on a disabled org gets FailedPrecondition; non-member gets PermissionDenied.
Personal access token RPCs — disabled org
Another user's token id returns NotFound even when that token's own org is disabled (same result as an unknown id). CheckCurrentUserPATTitle:
Billing RPCs — disabled org
GetBillingAccount, GetBillingBalance, and GetCheckout were not exercised (they need a Stripe customer, which this environment does not have). They resolve the billing/checkout id to the org and authorize against the org, so they run through the same gate. Lifecycle
OtherCheckResourcePermission for an org referenced by name, while disabled, returns a normal true/false status rather than an error. Not covered here
|
2 participants
Summary
Disabling an org only flips its state in Postgres while SpiceDB tuples stay in place, so RPCs that never read the org row kept working for members of a disabled org. This adds one gate on the authorization path: after a permission check passes, the target's org is resolved and the request fails with
FailedPreconditionwhen that org is disabled.Changes
IsAuthorized; denied callers getPermissionDeniedbefore any org lookupGetRaw) inside the permission check, so a caller without access learns nothing about org state whether it uses the id or the name; the gate enforces state only after the check passesCheckCurrentUserPATTitlerequires the same org access asCreateCurrentUserPAT; the id-based RPCs check the token's org state through an owner-scoped lookup, so an unknown and a foreign token id behave the samecore/resourceto the mockery config (its mocks existed but were not regenerable)Technical Details
AuthTokenpath is on the skip list and unaffectedAuthToken's optional project claim is dropped, not failed, when the project's org is disabled — matching how other authorization failures already behave on that pathTest Plan