feat(cli): migrate CLI auth checks to RBAC-aware permissions#1983
Merged
feat(cli): migrate CLI auth checks to RBAC-aware permissions#1983
Conversation
Port CLI PR #571 (Cap-go/CLI) changes into monorepo cli/ workspace. Migrates all CLI auth checks to RBAC-aware backend wrappers (hasCliPermission, filterOrgsByPermission, getOrganizationListWithPermission, resolveUserIdFromApiKey). Makes login, user account, and organization add identity-only (no legacy key-mode gating). Uses org.create_app permission key for app creation and init flow. Legacy fallback semantics preserved for older API keys. Includes build/request.ts refactoring to reduce cognitive complexity and extract helpers (dispatchCustomMsg, WsEntry interface).
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces legacy API-key validation with a CLI permission system: adds Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI Command"
participant Utils as "cli/src/utils"
participant DB as "Supabase RPC / DB"
participant Action as "Command Action"
CLI->>Utils: resolveUserIdFromApiKey(apikey)
Utils->>DB: get_user_id(apikey)
DB-->>Utils: userId
Utils-->>CLI: userId
CLI->>Utils: assertCliPermission(permissionKey, scope)
Utils->>DB: cli_check_permission(permissionKey, scope)
DB-->>Utils: allowed / denied
Utils-->>CLI: return or throw
CLI->>Action: perform operation (uses org/app ids)
Action->>DB: insert/update/delete rows / emit events
DB-->>Action: result
Action-->>CLI: command output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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)
Review rate limit: 1/5 review remaining, refill in 36 minutes and 42 seconds. Comment |
Contributor
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/src/init/command.ts (1)
1285-1297:⚠️ Potential issue | 🔴 CriticalGuard against zero permitted organizations before
allowedOrganizations[0].If
allowedOrganizations.length === 0, Line 1297 dereferencesallowedOrganizations[0].gidand crashes. Please handle the empty-permission case explicitly (friendly message + exit/error).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/init/command.ts` around lines 1285 - 1297, The code assumes allowedOrganizations has at least one element when assigning organizationUidRaw; add an explicit guard for allowedOrganizations.length === 0 before using allowedOrganizations[0] (e.g., check length, display a friendly error message and exit or throw), then keep the existing branch that calls pSelect when length > 1 and uses allowedOrganizations[0].gid when length === 1; ensure the check happens before the current ternary so allowedOrganizations[0].gid is never dereferenced on an empty array.
🧹 Nitpick comments (1)
cli/src/channel/add.ts (1)
37-40: Resolve the API key once and reuse the result.This path looks up the same identity twice. Keep the first return value in
userIdand reuse it forcreated_byto avoid an extra lookup on every channel creation.♻️ Proposed fix
- await resolveUserIdFromApiKey(supabase, options.apikey) + const userId = await resolveUserIdFromApiKey(supabase, options.apikey) await checkAppExistsAndHasPermissionOrgErr(supabase, options.apikey, appId, OrganizationPerm.admin, silent, true) @@ - const userId = await resolveUserIdFromApiKey(supabase, options.apikey) - const res = await createChannel(supabase, {Also applies to: 52-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/channel/add.ts` around lines 37 - 40, The code calls resolveUserIdFromApiKey twice causing an extra lookup; capture its return once (e.g., const userId = await resolveUserIdFromApiKey(supabase, options.apikey)) right after creating the Supabase client and then reuse userId wherever created_by is set instead of calling resolveUserIdFromApiKey again (also update the similar duplicate at the second occurrence around the other block referencing created_by), leaving calls to check2FAComplianceForApp and checkAppExistsAndHasPermissionOrgErr unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/build/request.ts`:
- Around line 1130-1134: Remove the redundant identity validation call
resolveUserIdFromApiKey(...) before asserting permissions; the call's return
value is unused and can block valid non-user API keys, so delete the await
resolveUserIdFromApiKey(supabase, options.apikey, silent) line and leave the
await assertCliPermission(supabase, options.apikey, 'app.build_native', { appId
}, { message: `Insufficient permissions to request a native build for app
${appId}`, silent }) invocation intact so permission checks rely solely on
cli_check_permission as in assertCliPermission.
In `@cli/src/bundle/compatibility.ts`:
- Line 67: Remove the redundant identity validation call to
resolveUserIdFromApiKey(supabase, enrichedOptions.apikey) since
checkAppExistsAndHasPermissionOrgErr(...) already performs API-key-to-user
resolution and enforces permissions; delete that call (and any unused userId
variable it would assign) from the flow where enrichedOptions.apikey and
supabase are passed, leaving checkAppExistsAndHasPermissionOrgErr as the sole
validation step so behavior and RPC-based permission checks remain unchanged.
In `@cli/src/init/command.ts`:
- Line 30: Remove the unused import getInstalledVersion from the import list in
command.ts; locate the import statement that currently brings in
createSupabaseClient, findBuildCommandForProjectType, ..., getInstalledVersion,
validateIosUpdaterSync and delete only the getInstalledVersion identifier (and
any now-incorrect trailing commas) so the file no longer imports an unused
symbol and lint/CI will pass.
---
Outside diff comments:
In `@cli/src/init/command.ts`:
- Around line 1285-1297: The code assumes allowedOrganizations has at least one
element when assigning organizationUidRaw; add an explicit guard for
allowedOrganizations.length === 0 before using allowedOrganizations[0] (e.g.,
check length, display a friendly error message and exit or throw), then keep the
existing branch that calls pSelect when length > 1 and uses
allowedOrganizations[0].gid when length === 1; ensure the check happens before
the current ternary so allowedOrganizations[0].gid is never dereferenced on an
empty array.
---
Nitpick comments:
In `@cli/src/channel/add.ts`:
- Around line 37-40: The code calls resolveUserIdFromApiKey twice causing an
extra lookup; capture its return once (e.g., const userId = await
resolveUserIdFromApiKey(supabase, options.apikey)) right after creating the
Supabase client and then reuse userId wherever created_by is set instead of
calling resolveUserIdFromApiKey again (also update the similar duplicate at the
second occurrence around the other block referencing created_by), leaving calls
to check2FAComplianceForApp and checkAppExistsAndHasPermissionOrgErr unchanged.
🪄 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
Run ID: e9eb6a31-895e-449e-898f-7180a860b5b6
📒 Files selected for processing (25)
cli/src/app/add.tscli/src/app/delete.tscli/src/app/list.tscli/src/app/set.tscli/src/build/request.tscli/src/bundle/cleanup.tscli/src/bundle/compatibility.tscli/src/bundle/delete.tscli/src/bundle/list.tscli/src/bundle/unlink.tscli/src/bundle/upload.tscli/src/channel/add.tscli/src/channel/currentBundle.tscli/src/channel/delete.tscli/src/channel/list.tscli/src/channel/set.tscli/src/init/command.tscli/src/login.tscli/src/organization/add.tscli/src/organization/delete.tscli/src/organization/list.tscli/src/organization/members.tscli/src/organization/set.tscli/src/user/account.tscli/src/utils.ts
… compatibility assertCliPermission and checkAppExistsAndHasPermissionOrgErr already validate API key permissions independently via cli_check_permission RPC. The prior identity gate was redundant and could incorrectly reject scoped or service keys that have valid permissions but don't resolve to a user.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/init/command.ts`:
- Around line 3969-3993: When falling back to a new organization (the branches
that call selectOrganizationForInit and discardResumedState), also clear the
restored onboarding metadata so stale resumed values (resumed.appId,
resumed.packageJson, resumed.channelId, resumed.platform, etc.) cannot leak into
the fresh flow; either extend discardResumedState() to reset the full resumed
object/state restored by tryResumeOnboarding(), or call a new
clearResumedState() immediately after organization = await
selectOrganizationForInit(...) (and in the blocked/permission branches) to wipe
resumed before proceeding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…ng state discardResumedState() now resets all globals that tryResumeOnboarding() may have restored (globalAppId, globalPathToPackageJson, globalChannelName, globalPlatform, globalDelta, globalCurrentVersion, globalOrgId, globalOrgName) and sets resumed to undefined. This prevents stale values from a previous onboarding session leaking into the fresh org flow.
…signment Capture resumed into a const resumedSnapshot at the top of the if-block so TypeScript can narrow the type correctly across await boundaries.
|
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
2 participants
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.
Summary (AI generated)
feat/rbac-cli-permissions) into the monorepocli/workspacehasCliPermission,filterOrgsByPermission,getOrganizationListWithPermission,resolveUserIdFromApiKey)login,user account, andorganization addidentity-only (no legacy key-mode gating)org.create_apppermission key for app creation and init flowbuild/request.tsrefactoring to reduce cognitive complexity and extract helpersMotivation (AI generated)
The CLI repository has been integrated into the capgo monorepo as the
cli/workspace. The RBAC permission changes from CLI PR #571 need to be applied here instead of the standalone CLI repo, so that the CLI permission model stays aligned with the backend RBAC system being developed onrbac-api-keys.Business Impact (AI generated)
This enables fine-grained API key permissions for CLI users, allowing organizations to restrict which operations each API key can perform. This is a prerequisite for the full RBAC API key system, improving security posture for enterprise customers.
Test Plan (AI generated)
bun run cli:build)bun run cli:check)bun test:cli)Generated with AI
Summary by CodeRabbit
Improvements
Behavioral Changes