fix(authz): require permission checks on GetPolicy, ListOrganizations, ListUsers#1622
Conversation
…, ListUsers These three endpoints previously allowed any authenticated user to read sensitive data without resource-level authz, leaking policy details and the platform-wide org/user inventory. - GetPolicy: fetch the policy, then check policymanage (org/project), group admin, or delete on the underlying resource, mirroring DeletePolicy. - ListOrganizations: keep the platform-pref gate, allow self-introspection when user_id matches the caller, otherwise require platform superuser. - ListUsers: keep the platform-pref gate, require get on the org when org_id is set, get on the group when group_id is set, otherwise require platform superuser. Adjust three TestUserAPI subtests that called ListUsers as a non-member regular user to use the existing superuser context, since they verify filter/keyword behaviour rather than authz. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAuthorization handlers for three Frontier service procedures are strengthened with preference gates and request-aware permission checks: ListUsers routes authorization based on org/group context, ListOrganizations permits self-introspection, and GetPolicy applies namespace-specific rules. Integration tests are updated to use org admin context for the new requirements. ChangesAuthorization strengthening for listing and policy operations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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.
🧹 Nitpick comments (3)
pkg/server/connect_interceptors/authorization.go (1)
639-655: ⚡ Quick winDeduplicate the policy-resource authorization rule.
This is now the same org/project/group/default permission switch used by
CreatePolicyandDeletePolicy. Extract it once so future auth changes stay consistent across all three endpoints.♻️ Suggested extraction
+func authorizePolicyResource(ctx context.Context, handler *v1beta1connect.ConnectHandler, resource string, req connect.AnyRequest) error { + ns, id, err := schema.SplitNamespaceAndResourceID(resource) + if err != nil { + return err + } + + switch ns { + case schema.OrganizationNamespace, schema.ProjectNamespace: + return handler.IsAuthorized(ctx, relation.Object{Namespace: ns, ID: id}, schema.PolicyManagePermission, req) + case schema.GroupNamespace: + return handler.IsAuthorized(ctx, relation.Object{Namespace: ns, ID: id}, group.AdminPermission, req) + default: + return handler.IsAuthorized(ctx, relation.Object{Namespace: ns, ID: id}, schema.DeletePermission, req) + } +} + "/raystack.frontier.v1beta1.FrontierService/GetPolicy": func(ctx context.Context, handler *v1beta1connect.ConnectHandler, req connect.AnyRequest) error { pbreq := req.(*connect.Request[frontierv1beta1.GetPolicyRequest]) policyResp, err := handler.GetPolicy(ctx, connect.NewRequest(&frontierv1beta1.GetPolicyRequest{Id: pbreq.Msg.GetId()})) if err != nil { return err } - ns, id, err := schema.SplitNamespaceAndResourceID(policyResp.Msg.GetPolicy().GetResource()) - if err != nil { - return err - } - - switch ns { - case schema.OrganizationNamespace, schema.ProjectNamespace: - return handler.IsAuthorized(ctx, relation.Object{Namespace: ns, ID: id}, schema.PolicyManagePermission, req) - case schema.GroupNamespace: - return handler.IsAuthorized(ctx, relation.Object{Namespace: ns, ID: id}, group.AdminPermission, req) - } - return handler.IsAuthorized(ctx, relation.Object{Namespace: ns, ID: id}, schema.DeletePermission, req) + return authorizePolicyResource(ctx, handler, policyResp.Msg.GetPolicy().GetResource(), req) },test/e2e/regression/api_test.go (2)
1439-1441: ⚡ Quick winKeep the org-filter coverage off the suite admin context.
This context is also used for admin-only API calls in this suite, so these assertions still pass if
ListUsersregresses back to superuser-only access. Prefer authenticating a real admin/member ofexistingOrghere.Also applies to: 1454-1456
1470-1472: ⚡ Quick winAdd the negative case for global
ListUsers.The changed auth rule here is that an unscoped user search now requires platform-superuser. Please add a
PermissionDeniedassertion withctxCurrentUserso that regression stays covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eb77cbaa-eea7-41f6-b5f5-3471edd6ee22
📒 Files selected for processing (2)
pkg/server/connect_interceptors/authorization.gotest/e2e/regression/api_test.go
Coverage Report for CI Build 26084048440Coverage decreased (-0.03%) to 42.306%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
ListUsers now requires `get` permission on the org_id passed in. After the org is deleted, that check fails with permission_denied — which is consistent with how the same subtest already asserts errors for GetOrganization, GetProject, and GetProjectResource on the deleted resource. Returning an empty user list instead would leak information about deleted orgs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Manual test run on local Frontier (this branch): 31/31 pass. Setup: fresh Org A (alice=owner, bob=viewer, dave=manager), fresh Org B (charlie=owner), Project A in Org A, group reused from prior fixtures; fresh policies on each namespace.
|
| # | caller | request | expected | result |
|---|---|---|---|---|
| 1 | regular user | no filter | permission_denied |
✅ |
| 2 | platform SU | no filter | OK | ✅ |
| 3 | org owner | org_id (own) |
OK | ✅ |
| 4 | org viewer | org_id (own) |
OK | ✅ |
| 5 | non-member | org_id |
permission_denied |
✅ |
| 6 | other-org owner | org_id (own org) |
OK | ✅ |
| 7 | platform SU | org_id (any) |
OK | ✅ |
| 8 | group owner | group_id |
OK | ✅ |
| 9 | non-member | group_id |
permission_denied |
✅ |
| 10 | regular user | org_id = bogus UUID |
permission_denied (no leak) |
✅ |
ListOrganizations
| # | caller | request | expected | result |
|---|---|---|---|---|
| 1 | regular user | no filter | permission_denied |
✅ |
| 2 | platform SU | no filter | OK | ✅ |
| 3 | regular user | user_id=self |
OK | ✅ |
| 4 | regular user | user_id=other |
permission_denied |
✅ |
| 5 | platform SU | user_id=other |
OK | ✅ |
GetPolicy
| # | caller | policy scope | expected | result |
|---|---|---|---|---|
| 1 | org owner | org | OK (policymanage) | ✅ |
| 2 | org viewer | org | permission_denied |
✅ |
| 3 | org manager | org | permission_denied (manager ≠ policymanage) |
✅ |
| 4 | non-member | org | permission_denied |
✅ |
| 5 | platform SU | org | OK | ✅ |
| 6 | org owner (parent) | project | OK (org->project_policymanage) |
✅ |
| 7 | project viewer | project | permission_denied |
✅ |
| 8 | other-org owner | project | permission_denied |
✅ |
| 9 | group owner | group | OK (delete-perm) | ✅ |
| 10 | non-member | group | permission_denied |
✅ |
| 11 | any | bogus policy id | not_found |
✅ |
Sibling paths still work
ListOrganizationsByCurrentUser(in skip list) — still OK for regular user ✅disable_users_listing=true— returnsunavailablefor regular user AND platform SU (pref before authz) ✅disable_orgs_listing=true— blocks self-introspection too ✅
Service-user path (Basic auth)
- Service user with org
viewerrole:ListUsers(org_id)OK,ListUsers()denied,ListOrganizations(user_id=self)OK,ListOrganizations(user_id=other)denied ✅
No regressions observed in the skipped/sibling endpoints.
3 participants
Summary
GetPolicy,ListOrganizations,ListUsers) accepted any authenticated user without any resource-level authorization, leaking policy details and the platform-wide org/user inventory. This patch adds authz in the existing interceptor map so the gaps are closed without proto/handler/service changes and without introducing new permissions.GetPolicynow fetches the policy and checkspolicymanage(org/project), groupadmin, ordeleteon the underlying resource — mirroring theDeletePolicyroute.ListOrganizationskeeps the platform-pref gate, allows self-introspection whenuser_idmatches the caller, otherwise requires platform superuser. Regular users wanting their own orgs already haveListOrganizationsByCurrentUser(in the skip list).ListUserskeeps the platform-pref gate, requiresgeton the org whenorg_idis set,geton the group whengroup_idis set, otherwise requires platform superuser.SDK / consumer impact
web/sdk/,web/apps/admin/,web/apps/client-demo/. The admin UI usesAdminService.SearchUsers/SearchOrganizations/ListOrganizationsByCurrentUserinstead.frontier organization listandfrontier user list(global) will now require platform superuser credentials, which is the correct behaviour for those admin paths.appConfig.App.Admin.Users), so they continue to pass. ThreeTestUserAPIsubtests that exercised filter/keyword behaviour as a non-member user were switched to the existing superuser context.Test plan
gofmtclean on changed filesgo vet ./pkg/server/connect_interceptors/...go vet ./test/e2e/...go build ./...TestUserAPI,TestOrganizationAPI, bootstrap helpers, and any policy tests still pass🤖 Generated with Claude Code