feat: api v1 admin api access for orgs #22544
Conversation
WalkthroughA new permission-based system for determining admin access to an organization's admin API was introduced. The Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (07/16/25)1 reviewer was added to this PR based on Keith Williams's automation. |
|
✅ No security or compliance issues detected. Reviewed everything up to 66957f3. Security Overview
Detected Code Changes
Reply to this PR with |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/public/static/locales/en/common.json (1)
718-720: Consider aligning new PBAC keys with existing permission sectionsThe new keys (
pbac_action_admin_api,pbac_desc_access_organization_admin_api) are added much earlier in the file than the rest of the PBAC‐related strings (which start around line 3270).
Keeping all PBAC keys together makes future maintenance easier and avoids “hidden” duplicates further down the file.No functional issue here—just a suggestion to move these two entries next to the other
pbac_action_*/pbac_desc_*keys.apps/api/v1/test/lib/utils/isAdmin.integration-test.ts (1)
160-177: Good test for fallback behavior verification.This test validates the fallback mechanism when PBAC is disabled:
- Fallback scenario: Tests that admin access is granted when permission check returns true (via fallback roles)
- Backward compatibility: Ensures existing role-based access continues to work
However, the test name could be more descriptive of what it's actually testing.
Consider updating the test name to better reflect what it's testing:
- it("Returns admin when PBAC is disabled and user has fallback admin role", async () => { + it("Returns admin when permission check succeeds via fallback roles", async () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/api/v1/lib/utils/isAdmin.ts(2 hunks)apps/api/v1/test/lib/utils/isAdmin.integration-test.ts(2 hunks)apps/web/public/static/locales/en/common.json(1 hunks)packages/features/pbac/domain/types/permission-registry.ts(2 hunks)packages/prisma/migrations/20250716084913_add_organization_admin_api_permission/migration.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
apps/web/public/static/locales/en/common.json (1)
undefined
<retrieved_learning>
Learnt from: bandhan-majumder
PR: #22359
File: packages/lib/server/locales/en/common.json:1336-1339
Timestamp: 2025-07-14T16:31:45.201Z
Learning: When making localization changes for new features, it's often safer to add new strings rather than modify existing ones to avoid breaking existing functionality that depends on the original strings. This approach allows for feature-specific customization while maintaining backward compatibility.
</retrieved_learning>
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Detect changes
- GitHub Check: Security Check
🔇 Additional comments (12)
packages/prisma/migrations/20250716084913_add_organization_admin_api_permission/migration.sql (1)
1-8: Well-structured migration with proper idempotency handling.The migration correctly adds the new
organization.adminApipermission to the admin role. The use ofON CONFLICT DO NOTHINGensures the migration is idempotent and safe to run multiple times.The values (
'admin_role','organization','adminApi') align with the corresponding TypeScript enum and registry definitions, maintaining consistency across the database and application layers.packages/features/pbac/domain/types/permission-registry.ts (3)
28-28: Consistent enum addition follows established patterns.The
AdminApi = "adminApi"addition to theCustomActionenum is well-placed and follows the existing naming convention. The string value matches what's used in the database migration.
232-237: Complete permission registry entry with proper structure.The permission registry entry for
AdminApiis well-structured and includes all required fields:
- Description clearly explains the permission's purpose
- Category "org" is consistent with other Organization resource permissions
- i18n keys follow the established naming convention
The entry aligns with the database migration and maintains consistency across the permission system.
235-236: i18n keys verified in English locale
- Both
pbac_action_admin_apiandpbac_desc_access_organization_admin_apiare defined in
apps/web/public/static/locales/en/common.json.- No missing-translation warnings will occur for the default locale.
If you support additional locales, please add these entries to their respective JSON files.
apps/api/v1/lib/utils/isAdmin.ts (2)
3-3: LGTM: New PBAC service import added correctly.The import of
PermissionCheckServiceis properly added to support the new permission-based admin checks.
36-56: Excellent implementation of PBAC integration with proper fallback handling.The refactored logic successfully integrates Permission-Based Access Control while maintaining backward compatibility. Key strengths:
- Proper permission check: Uses
"organization.adminApi"permission with correct fallback roles- Maintains existing query structure: Still filters for accepted memberships in organizations with admin API enabled
- Fallback mechanism: Specifies
OWNERandADMINroles for backward compatibility when PBAC is disabled- Correct scope assignment: Returns
OrgOwnerOrAdminscope when permission is grantedThe implementation aligns perfectly with the PR objectives of seamless PBAC integration.
apps/api/v1/test/lib/utils/isAdmin.integration-test.ts (6)
4-4: LGTM: Test imports updated correctly.The addition of
viandbeforeEachimports is appropriate for the new test setup requirements.
6-6: LGTM: Permission service import added for testing.The import of
PermissionCheckServiceis correctly added to support mocking in the tests.
12-12: LGTM: Proper mock setup for permission service.The mock setup for
PermissionCheckServiceis correctly implemented using Vitest's mocking capabilities.
18-29: Excellent test setup with proper mock management.The test setup properly:
- Mock service type definition: Correctly types the mock service with
checkPermissionmethod- beforeEach cleanup: Clears mocks between tests to prevent interference
- Mock implementation: Properly mocks the
PermissionCheckServiceconstructor- Mock method setup: Configures the
checkPermissionmethod for testingThis ensures isolated and reliable test execution.
110-133: Comprehensive test coverage for PBAC permission granted scenario.This test case excellently validates:
- Permission check flow: Verifies that the permission service is called when PBAC is enabled
- Correct parameters: Validates that
checkPermissionis called with proper userId, teamId, permission, and fallback roles- Expected behavior: Confirms admin access is granted when permission check returns true
- Proper scope: Ensures
OrgOwnerOrAdminscope is returnedThe test aligns perfectly with the PR objectives of testing PBAC functionality.
135-158: Thorough test coverage for PBAC permission denied scenario.This test case provides excellent validation for the negative case:
- Permission denial: Verifies behavior when
checkPermissionreturns false- Correct response: Confirms admin access is denied and scope is null
- Service interaction: Validates that the permission service is still called with correct parameters
This ensures robust testing of both positive and negative PBAC scenarios.
E2E results are ready! |
| @@ -22,7 +23,6 @@ export const isAdminGuard = async (req: NextApiRequest) => { | |||
| isAdminAPIEnabled: true, | |||
| }, | |||
| }, | |||
| OR: [{ role: MembershipRole.OWNER }, { role: MembershipRole.ADMIN }], | |||
There was a problem hiding this comment.
Remove this check because we just want to pull back their membership for now
apps/api/v1/lib/utils/isAdmin.ts
Outdated
| @@ -33,7 +33,27 @@ export const isAdminGuard = async (req: NextApiRequest) => { | |||
| }, | |||
| }, | |||
| }); | |||
| if (orgOwnerOrAdminMemberships.length > 0) return { isAdmin: true, scope: ScopeOfAdmin.OrgOwnerOrAdmin }; | |||
|
|
|||
| if (orgOwnerOrAdminMemberships.length > 0) { | |||
There was a problem hiding this comment.
If they have a membership for any orgs with admin enabled
apps/api/v1/lib/utils/isAdmin.ts
Outdated
|
|
||
| // Check PBAC permissions for each organization | ||
| // This should only ever be one membership as we only support one org currently. | ||
| for (const membership of orgOwnerOrAdminMemberships) { |
There was a problem hiding this comment.
Should only be one loop iteration due to only one org per user
| const hasAdminPermission = await permissionCheckService.checkPermission({ | ||
| userId, | ||
| teamId, | ||
| permission: "organization.adminApi", | ||
| fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN], | ||
| }); |
There was a problem hiding this comment.
Check if this user has the org adminApi permission if PBAC is enabled. OR fall back to checking if they are a owner or admin of this team
| @@ -228,6 +229,12 @@ export const PERMISSION_REGISTRY: PermissionRegistry = { | |||
| i18nKey: "pbac_action_update", | |||
| descriptionI18nKey: "pbac_desc_edit_organization_settings", | |||
| }, | |||
| [CustomAction.AdminApi]: { | |||
There was a problem hiding this comment.
This auto adds everything to the UI to add this permission to new/existing roles
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
| @@ -12,7 +13,7 @@ export const isAdminGuard = async (req: NextApiRequest) => { | |||
| const { role: userRole } = user; | |||
| if (userRole === UserPermissionRole.ADMIN) return { isAdmin: true, scope: ScopeOfAdmin.SystemWide }; | |||
|
|
|||
| const orgOwnerOrAdminMemberships = await prisma.membership.findMany({ | |||
| const usersOrgMemberships = await prisma.membership.findMany({ | |||
There was a problem hiding this comment.
If we're refactoring this query, can we use a repository here?
| for (const membership of usersOrgMemberships) { | ||
| const teamId = membership.team.id; | ||
|
|
||
| const hasAdminPermission = await permissionCheckService.checkPermission({ |
There was a problem hiding this comment.
@sean-brydon reached out to you about this but I noticed we're making a query to check for a valid membership in this function but we're making that call again in permissionCheckService.checkPermission.
There was a problem hiding this comment.
After some discussion this isn't a blocking comment.
|
This PR is being marked as stale due to inactivity. |
Add PBAC Permission Checking to Admin API Guard
Summary
isAdminGuardto checkorganization.adminApipermission when PBAC is enabled for a teamChanges Made
Permission Flow
UserPermissionRole.ADMINget system-wide accessorganization.adminApipermission via custom rolesOWNER/ADMINrole checkTest Coverage
Added test cases covering:
Testing Setup
SQL Script to Create Developer Role with Admin API Permission
Test Plan