[Permissions] Capability-based authz refactor. gate org secrets, deleteUser, and ReadMe JWT on MANAGE_ORG#528
Open
juanmrad wants to merge 2 commits into
Open
Conversation
…teUser, and ReadMe JWT on MANAGE_ORG
juanmrad
requested review from
cassidyjames,
dom-notion,
julietshen and
vinaysrao1
as code owners
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughMigrate authorization from role-based checks to permission-based checks: add MANAGE_ROLES, expose me.permissions in GraphQL types, replace hasPermission(role) usage with user.getPermissions().includes(...), and add permission gates and tests across resolvers and client UI. ChangesPermission-based Authorization Refactor
Sequence Diagram: User.readMeJWT flowsequenceDiagram
participant Client
participant Resolver as User.readMeJWT
participant Auth as Auth (ctx.getUser)
participant OrgAPI
participant JWT as JWT Signer
Client->>Resolver: request readMeJWT(parentUserId)
Resolver->>Auth: ctx.getUser() -> user
Resolver->>Auth: user.getPermissions().includes('MANAGE_ORG')?
alt Has MANAGE_ORG
Resolver->>OrgAPI: getActivatedApiKey(user.orgId)
Resolver->>OrgAPI: getPublicSigningKey(user.orgId)
OrgAPI-->>Resolver: apiKey, publicSigningKey
Resolver->>JWT: sign payload with apiKey/publicSigningKey
else Lacks MANAGE_ORG
Resolver->>JWT: sign payload with null secrets
end
JWT-->>Resolver: token
Resolver-->>Client: token
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
server/graphql/modules/apiKey.resolver.test.ts (1)
18-36: ⚡ Quick winAdd unauthenticated-path coverage for
Query.apiKey.Line 21 only verifies forbidden/authorized flows. Since the resolver now distinguishes unauthenticated callers, add a case where
getUser()is null (or missingorgId) and assert'Authenticated user required'plus no service invocation.Proposed test addition
describe('Query.apiKey', () => { + it('throws unauthenticatedError when caller is not authenticated', async () => { + const { ctx, getActiveApiKeyForOrg } = makeCtx([UserPermission.MANAGE_ORG]); + const unauthenticatedCtx = { ...ctx, getUser: () => null }; + await expect(Query.apiKey({}, {}, unauthenticatedCtx)).rejects.toThrow( + 'Authenticated user required', + ); + expect(getActiveApiKeyForOrg).not.toHaveBeenCalled(); + }); + it('throws forbiddenError when caller lacks MANAGE_ORG', async () => {🤖 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 `@server/graphql/modules/apiKey.resolver.test.ts` around lines 18 - 36, Add a new unit test for Query.apiKey that covers the unauthenticated path: create a ctx from makeCtx where getUser() returns null (or a user object without orgId), call Query.apiKey({}, {}, ctx) and assert it rejects with 'Authenticated user required', and also assert getActiveApiKeyForOrg was not called; reference Query.apiKey, getUser, and getActiveApiKeyForOrg to locate where to add the test.server/graphql/modules/user.resolver.test.ts (1)
102-133: ⚡ Quick winAdd coverage for the
readMeJWTidentity guard.The new JWT tests validate MANAGE_ORG gating, but they don’t exercise the
user.id !== authedUser.idcheck. Add a case assertingnulltoken and zero orgAPI calls when parent user differs from authenticated user.Proposed test addition
describe('User.readMeJWT does not leak org secrets to non-admins', () => { @@ it('embeds org secrets in the JWT for MANAGE_ORG callers', async () => { @@ expect(getPublicSigningKeyPem).toHaveBeenCalledWith('org-1'); }); + + it('returns null when authenticated user does not match parent user', async () => { + const { ctx, getActivatedApiKeyForOrg, getPublicSigningKeyPem } = + makeReadMeCtx([UserPermission.MANAGE_ORG]); + const mismatchedParent = { ...userParent, id: 'other-user' }; + await expect(User.readMeJWT(mismatchedParent, {}, ctx)).resolves.toBeNull(); + expect(getActivatedApiKeyForOrg).not.toHaveBeenCalled(); + expect(getPublicSigningKeyPem).not.toHaveBeenCalled(); + }); });🤖 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 `@server/graphql/modules/user.resolver.test.ts` around lines 102 - 133, Add a new test exercising the identity guard in User.readMeJWT: use makeReadMeCtx to create a ctx whose authenticated user id is different from userParent.id (i.e., not the same user), call User.readMeJWT(userParent, {}, ctx) and assert the returned token is null and that getActivatedApiKeyForOrg and getPublicSigningKeyPem were not called; this ensures the user.id !== authedUser.id branch is covered and prevents org-secret lookups for mismatched identities.client/src/webpages/dashboard/mrt/ManualReviewAppealSettings.tsx (1)
17-17: ⚡ Quick winUse
@/alias for the new permissions import.Switch this newly-added relative import to the configured absolute alias for consistency with project standards.
💡 Suggested change
-import { userHasPermissions } from '../../../routing/permissions'; +import { userHasPermissions } from '`@/routing/permissions`';As per coding guidelines,
**/*.{ts,tsx}: Use absolute imports with@/prefix over relative paths where configured intsconfig.json.🤖 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 `@client/src/webpages/dashboard/mrt/ManualReviewAppealSettings.tsx` at line 17, Replace the relative import of userHasPermissions with the project's absolute alias: update the import statement that currently references "../../../routing/permissions" to use the "`@/routing/permissions`" path so ManualReviewAppealSettings.tsx imports userHasPermissions via the configured tsconfig alias.client/src/webpages/dashboard/mrt/manual_review_job/ManualReviewJobReview.tsx (1)
47-47: ⚡ Quick winUse
@/alias for the new permissions utility import.Please convert this new relative import to the configured absolute alias style.
💡 Suggested change
-import { userHasPermissions } from '../../../../routing/permissions'; +import { userHasPermissions } from '`@/routing/permissions`';As per coding guidelines,
**/*.{ts,tsx}: Use absolute imports with@/prefix over relative paths where configured intsconfig.json.🤖 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 `@client/src/webpages/dashboard/mrt/manual_review_job/ManualReviewJobReview.tsx` at line 47, The import in ManualReviewJobReview.tsx uses a relative path for the permissions utility; change the import of userHasPermissions from '../../../../routing/permissions' to use the configured absolute alias (start with '`@/`') so it resolves via the tsconfig paths (e.g., import { userHasPermissions } from '`@/routing/permissions`'). Ensure the symbol name userHasPermissions remains unchanged and update any IDE imports to match the alias.
🤖 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 `@server/services/userManagementService/permissioning.ts`:
- Around line 160-170: getPermissionsForRole currently returns the shared array
from UserPermissionsForRole which allows callers to mutate the canonical
permission set; change getPermissionsForRole to return a defensive copy of the
permission array (e.g., Array.from(...) or spread into a new array) and ensure
when UserPermissionsForRole.get(role) is missing you still return a new empty
array instead of the shared literal so callers cannot mutate the internal map.
---
Nitpick comments:
In
`@client/src/webpages/dashboard/mrt/manual_review_job/ManualReviewJobReview.tsx`:
- Line 47: The import in ManualReviewJobReview.tsx uses a relative path for the
permissions utility; change the import of userHasPermissions from
'../../../../routing/permissions' to use the configured absolute alias (start
with '`@/`') so it resolves via the tsconfig paths (e.g., import {
userHasPermissions } from '`@/routing/permissions`'). Ensure the symbol name
userHasPermissions remains unchanged and update any IDE imports to match the
alias.
In `@client/src/webpages/dashboard/mrt/ManualReviewAppealSettings.tsx`:
- Line 17: Replace the relative import of userHasPermissions with the project's
absolute alias: update the import statement that currently references
"../../../routing/permissions" to use the "`@/routing/permissions`" path so
ManualReviewAppealSettings.tsx imports userHasPermissions via the configured
tsconfig alias.
In `@server/graphql/modules/apiKey.resolver.test.ts`:
- Around line 18-36: Add a new unit test for Query.apiKey that covers the
unauthenticated path: create a ctx from makeCtx where getUser() returns null (or
a user object without orgId), call Query.apiKey({}, {}, ctx) and assert it
rejects with 'Authenticated user required', and also assert
getActiveApiKeyForOrg was not called; reference Query.apiKey, getUser, and
getActiveApiKeyForOrg to locate where to add the test.
In `@server/graphql/modules/user.resolver.test.ts`:
- Around line 102-133: Add a new test exercising the identity guard in
User.readMeJWT: use makeReadMeCtx to create a ctx whose authenticated user id is
different from userParent.id (i.e., not the same user), call
User.readMeJWT(userParent, {}, ctx) and assert the returned token is null and
that getActivatedApiKeyForOrg and getPublicSigningKeyPem were not called; this
ensures the user.id !== authedUser.id branch is covered and prevents org-secret
lookups for mismatched identities.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 648a7844-8e91-48e9-a381-56448ed7aa1a
📒 Files selected for processing (21)
client/src/graphql/generated.tsclient/src/webpages/dashboard/mrt/ManualReviewAppealSettings.tsxclient/src/webpages/dashboard/mrt/manual_review_job/ManualReviewJobReview.tsxserver/graphql/generated.tsserver/graphql/modules/apiKey.resolver.test.tsserver/graphql/modules/apiKey.tsserver/graphql/modules/backtest.resolver.test.tsserver/graphql/modules/backtest.tsserver/graphql/modules/integration.resolver.test.tsserver/graphql/modules/integration.tsserver/graphql/modules/manualReviewTool.tsserver/graphql/modules/org.resolver.test.tsserver/graphql/modules/org.tsserver/graphql/modules/retroaction.resolver.test.tsserver/graphql/modules/retroaction.tsserver/graphql/modules/routingRule.tsserver/graphql/modules/user.resolver.test.tsserver/graphql/modules/user.tsserver/services/userManagementService/index.tsserver/services/userManagementService/permissioning.test.tsserver/services/userManagementService/permissioning.ts
💤 Files with no reviewable changes (1)
- server/services/userManagementService/index.ts
There was a problem hiding this comment.
Pull request overview
Refactors authorization checks away from role-string branching toward capability checks via user.getPermissions().includes(...), introduces a dedicated MANAGE_ROLES capability, and closes several authorization gaps where org-level secrets could be accessed by any authenticated org user.
Changes:
- Add
MANAGE_ROLESpermission and document the capability-driven authz approach inpermissioning.ts; remove the legacyhasPermissionhelper export. - Gate sensitive org secret surfaces (API key, webhook signing key, integration configs) plus
deleteUserand ReadMe JWT secret embedding behindMANAGE_ORG. - Update/extend resolver tests and client UI queries/guards to use
me.permissionsinstead ofme.rolewhere applicable.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/services/userManagementService/permissioning.ts | Adds MANAGE_ROLES and clarifies the permission-seed map as the canonical source for capabilities. |
| server/services/userManagementService/permissioning.test.ts | Adds tests for MANAGE_ROLES defaults and seed-map invariants. |
| server/services/userManagementService/index.ts | Stops re-exporting the removed hasPermission helper. |
| server/graphql/modules/user.ts | Adds MANAGE_ROLES to the GraphQL enum; gates deleteUser and ReadMe JWT secret embedding on MANAGE_ORG. |
| server/graphql/modules/user.resolver.test.ts | Adds coverage for deleteUser MANAGE_ORG gating and ReadMe JWT secret non-leakage. |
| server/graphql/modules/routingRule.ts | Replaces role-based permission check with user.getPermissions() capability check. |
| server/graphql/modules/retroaction.ts | Replaces role-based permission check with capability check for RUN_RETROACTION. |
| server/graphql/modules/retroaction.resolver.test.ts | Updates resolver test user mocks to provide getPermissions(). |
| server/graphql/modules/org.ts | Adds MANAGE_ORG gates to multiple sensitive Org field/mutation resolvers (api key, integration configs, webhook key, safety/SSO settings). |
| server/graphql/modules/org.resolver.test.ts | Adds tests ensuring sensitive Org resolvers require MANAGE_ORG and don’t hit datasources when forbidden. |
| server/graphql/modules/manualReviewTool.ts | Passes user.getPermissions() into service calls instead of resolving via role at callsite. |
| server/graphql/modules/integration.ts | Gates integration config read/write operations behind MANAGE_ORG. |
| server/graphql/modules/integration.resolver.test.ts | Adds forbidden-path tests ensuring MANAGE_ORG is required before datasource/registry access. |
| server/graphql/modules/backtest.ts | Replaces role-based permission check with capability check for RUN_BACKTEST. |
| server/graphql/modules/backtest.resolver.test.ts | Updates resolver test user mocks to provide getPermissions(). |
| server/graphql/modules/apiKey.ts | Gates API key existence query behind MANAGE_ORG; retains MANAGE_ORG checks for rotate operations. |
| server/graphql/modules/apiKey.resolver.test.ts | Adds tests verifying Query.apiKey is MANAGE_ORG-gated and doesn’t call the service when forbidden. |
| server/graphql/generated.ts | Regenerates server GraphQL types to include MANAGE_ROLES. |
| client/src/webpages/dashboard/mrt/ManualReviewAppealSettings.tsx | Switches gating UI from me.role to me.permissions (MANAGE_ORG). |
| client/src/webpages/dashboard/mrt/manual_review_job/ManualReviewJobReview.tsx | Switches “admin bypass” logic to permission-based gating and updates queries to request me.permissions. |
| client/src/graphql/generated.ts | Regenerates client GraphQL types/ops reflecting me.permissions and MANAGE_ROLES. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
|
|
||
| describe('getPermissionsForRole', () => { | ||
| it('returns an empty array for an unknown role rather than throwing', () => { | ||
| expect(getPermissionsForRole('NOT_A_ROLE')).toEqual([]); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context & Requests for Reviewers
Foundation work for #406 (granular permissions).
Replace the remaining role-string authz callsites with
user.getPermissions().includes(...)so server-side checks no longer branch onuser.role. After this PR there are zero resolver-level role-string checks.Add a new
MANAGE_ROLEScapability granted only toADMINtoday — the seam future PRs will gate the role-editing API on so admins can delegate role editing without granting fullMANAGE_ORG.Close several server-side authz gaps where any authenticated user in an org could read or modify org-level secrets. All now require
MANAGE_ORG.The refactor is intentionally a no-op for every role today (verified by
permissioning.test.ts); only the security fixes change observable behavior, and only for non-admins.Summary by CodeRabbit
Security Improvements
UI Changes
Bug Fixes