feat: migrate CLI auth checks to RBAC-aware permissions#571
Open
feat: migrate CLI auth checks to RBAC-aware permissions#571
Conversation
📝 WalkthroughWalkthroughThis is a major refactoring of the CLI's authentication and authorization system, replacing the legacy Changes
Possibly related PRs
Poem
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/channel/set.ts (1)
167-181:⚠️ Potential issue | 🟠 MajorRemove
user_idfilters from app_versions queries; rely on org-level permission check instead.The
user_idfilters at lines 173 and 223 are inconsistent with the permission model.checkAppExistsAndHasPermissionOrgErrvalidates org-level access (which should grant all org members equal visibility), but subsequent queries restrict results to versions owned by the specific API key holder. Other app_versions queries across the codebase (getActiveAppVersions, getVersionData, deleteSpecificVersion) do not filter by user_id, suggesting the intended access model is org-scoped, not user-scoped.Either remove the
.eq('user_id', userId)filters to align with the org permission check, or clarify whether versions should be intentionally scoped to individual users despite org access.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/channel/set.ts` around lines 167 - 181, The supabase query that selects from 'app_versions' (the block using resolvedBundleVersion and variables resolvedBundleVersion, userId) incorrectly restricts results by .eq('user_id', userId); remove the user_id filter so the query only filters by app_id, name, and deleted to honor org-level permission already enforced by checkAppExistsAndHasPermissionOrgErr; update both places in this file where .eq('user_id', userId) is applied (the resolvedBundleVersion lookup and the later version lookup around line 223) to match other functions like getActiveAppVersions/getVersionData which are org-scoped.
🧹 Nitpick comments (6)
src/channel/add.ts (1)
39-53: DuplicateresolveUserIdFromApiKeycall.
resolveUserIdFromApiKeyis called twice: once at line 39 (discarded) and again at line 53 to retrieveuserId. Consider capturing the result from the first call and reusing it to avoid a redundant RPC roundtrip.♻️ Proposed fix to eliminate duplicate call
- await resolveUserIdFromApiKey(supabase, options.apikey) + const userId = await resolveUserIdFromApiKey(supabase, options.apikey) await checkAppExistsAndHasPermissionOrgErr(supabase, options.apikey, appId, OrganizationPerm.admin, silent, true) if (!silent) log.info(`Creating channel ${appId}#${channelId} to Capgo`) const data = await findUnknownVersion(supabase, appId) if (!data) { if (!silent) log.error('Cannot find default version for channel creation, please contact Capgo support 🤨') throw new Error('Cannot find default version for channel creation') } const orgId = await getOrganizationId(supabase, appId) - const userId = await resolveUserIdFromApiKey(supabase, options.apikey)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/channel/add.ts` around lines 39 - 53, The code calls resolveUserIdFromApiKey twice; capture its returned userId once (call resolveUserIdFromApiKey(supabase, options.apikey) into a variable) and reuse that variable later instead of calling the function again. Replace the first discarded call with an assignment (userId) and remove the second call, ensuring any subsequent logic that needs userId (e.g., the existing userId usage after getOrganizationId) uses the captured variable.src/app/add.ts (1)
94-106: Consider removing unnecessary non-null assertions.SonarCloud flags the
!assertions onoptions.apikeyat lines 94, 99, and 103 as unnecessary. SinceensureOptions(line 91) throws ifapikeyis falsy, the value is guaranteed to be defined at this point.If the type narrowing isn't recognized by TypeScript, consider either:
- Extracting
apikeyto a localconstafter validation- Using a type guard in
ensureOptionsthat narrows the type♻️ Proposed fix to eliminate non-null assertions
ensureOptions(appId, options, silent) + const apikey = options.apikey! // Guaranteed by ensureOptions const supabase = await createSupabaseClient(options.apikey!, options.supaHost, options.supaAnon) - const userId = await resolveUserIdFromApiKey(supabase, options.apikey!) + const userId = await resolveUserIdFromApiKey(supabase, apikey) await ensureAppDoesNotExist(supabase, appId, silent) if (!organization) - organization = await getOrganizationWithPermission(supabase, options.apikey!, 'org.create_app') + organization = await getOrganizationWithPermission(supabase, apikey, 'org.create_app') const organizationUid = organization.gid - await assertCliPermission(supabase, options.apikey!, 'org.create_app', { orgId: organizationUid }, { + await assertCliPermission(supabase, apikey, 'org.create_app', { orgId: organizationUid }, { message: `Insufficient permissions to create an app in organization ${organizationUid}`, silent, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/add.ts` around lines 94 - 106, Remove the unnecessary non-null assertions on options.apikey by extracting a local const apikey immediately after ensureOptions returns (e.g., const apikey = options.apikey) and use that const in resolveUserIdFromApiKey, getOrganizationWithPermission, and assertCliPermission calls; alternatively make ensureOptions a type guard that narrows options so apikey is known defined—update the references to use the narrowed/const apikey instead of options.apikey! (affects resolveUserIdFromApiKey, getOrganizationWithPermission, assertCliPermission).src/app/list.ts (1)
35-35: Unused return value fromresolveUserIdFromApiKey.The
userIdreturned fromresolveUserIdFromApiKeyis discarded. If this is only for API key validation, consider adding a comment to clarify intent, or evaluate if downstream code could benefit from the resolved user ID.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/list.ts` at line 35, The call to resolveUserIdFromApiKey(supabase, options.apikey) discards its return value; either capture and use the returned userId (e.g., const userId = await resolveUserIdFromApiKey(...)) where downstream logic needs it, or explicitly document intent by assigning to an underscore with a comment (e.g., const _userId = await resolveUserIdFromApiKey(...); // validate API key only) so the purpose is clear and linters won’t flag an unused value; update any downstream logic to use userId if needed.src/init/command.ts (1)
942-953: Consider batching permission checks for organizations.The current implementation calls
hasCliPermissionsequentially for each organization viaPromise.all. While this works correctly, users with many organizations may experience noticeable latency. Consider whether a batched permission check RPC could improve performance.For most users this is likely acceptable, but worth noting for future optimization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/init/command.ts` around lines 942 - 953, The permission checks loop (permissionChecks using hasCliPermission over allOrganizations) can cause latency for many orgs; replace the per-org RPC calls with a batched permission check: either implement a new server-side RPC (e.g., hasCliPermissionBatch) that accepts an array of org.gid and returns permission booleans, then call that from this code and map results to allowedOrganizations, or apply client-side batching/concurrency control (chunk allOrganizations into N-sized batches and call hasCliPermission in parallel per chunk then await each batch) to avoid running hundreds of RPCs at once; update the variables permissionChecks and allowedOrganizations accordingly and ensure the error branch and pLog.error message remain unchanged.src/utils.ts (2)
1488-1493: Consider batching permission checks to reduce RPC overhead.This makes N parallel RPC calls (one
cli_check_permissionper organization). For users with many organizations, this could be slow and put unnecessary load on the backend. Consider adding a backend endpoint that returns organizations filtered by permission in a single call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils.ts` around lines 1488 - 1493, Current code builds permissionChecks by mapping allOrganizations and calling hasCliPermission (which triggers cli_check_permission per org) in parallel, causing N RPCs; replace this with a single batched permission API on the backend that accepts an array of org IDs and returns only the allowed orgs, then call that new endpoint (instead of hasCliPermission per org) to get filtered organizations in one request; update the logic around permissionChecks/allOrganizations to use the backend response and remove the Promise.all-per-org pattern to eliminate per-org RPC overhead.
1534-1536: Include error details in log message.When
userIdErroris present, the underlying error reason is lost. Consider including it in the log for easier debugging.Proposed fix
if (!userId || userIdError) { - log.error(`Cannot auth user with apikey`) + log.error(`Cannot auth user with apikey${userIdError ? `: ${formatError(userIdError)}` : ''}`) throw new Error('Cannot authenticate user with provided API key') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils.ts` around lines 1534 - 1536, The current auth failure log loses the underlying error; update the block that checks userId and userIdError to include the error details from userIdError in the log (e.g., log.error message for the userId/auth attempt should append or interpolate userIdError.stack/message) while keeping the thrown Error message unchanged or augmenting it with limited context; locate the conditional that references userId, userIdError and log.error and change the log call to include the userIdError details.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/channel/set.ts`:
- Around line 167-181: The supabase query that selects from 'app_versions' (the
block using resolvedBundleVersion and variables resolvedBundleVersion, userId)
incorrectly restricts results by .eq('user_id', userId); remove the user_id
filter so the query only filters by app_id, name, and deleted to honor org-level
permission already enforced by checkAppExistsAndHasPermissionOrgErr; update both
places in this file where .eq('user_id', userId) is applied (the
resolvedBundleVersion lookup and the later version lookup around line 223) to
match other functions like getActiveAppVersions/getVersionData which are
org-scoped.
---
Nitpick comments:
In `@src/app/add.ts`:
- Around line 94-106: Remove the unnecessary non-null assertions on
options.apikey by extracting a local const apikey immediately after
ensureOptions returns (e.g., const apikey = options.apikey) and use that const
in resolveUserIdFromApiKey, getOrganizationWithPermission, and
assertCliPermission calls; alternatively make ensureOptions a type guard that
narrows options so apikey is known defined—update the references to use the
narrowed/const apikey instead of options.apikey! (affects
resolveUserIdFromApiKey, getOrganizationWithPermission, assertCliPermission).
In `@src/app/list.ts`:
- Line 35: The call to resolveUserIdFromApiKey(supabase, options.apikey)
discards its return value; either capture and use the returned userId (e.g.,
const userId = await resolveUserIdFromApiKey(...)) where downstream logic needs
it, or explicitly document intent by assigning to an underscore with a comment
(e.g., const _userId = await resolveUserIdFromApiKey(...); // validate API key
only) so the purpose is clear and linters won’t flag an unused value; update any
downstream logic to use userId if needed.
In `@src/channel/add.ts`:
- Around line 39-53: The code calls resolveUserIdFromApiKey twice; capture its
returned userId once (call resolveUserIdFromApiKey(supabase, options.apikey)
into a variable) and reuse that variable later instead of calling the function
again. Replace the first discarded call with an assignment (userId) and remove
the second call, ensuring any subsequent logic that needs userId (e.g., the
existing userId usage after getOrganizationId) uses the captured variable.
In `@src/init/command.ts`:
- Around line 942-953: The permission checks loop (permissionChecks using
hasCliPermission over allOrganizations) can cause latency for many orgs; replace
the per-org RPC calls with a batched permission check: either implement a new
server-side RPC (e.g., hasCliPermissionBatch) that accepts an array of org.gid
and returns permission booleans, then call that from this code and map results
to allowedOrganizations, or apply client-side batching/concurrency control
(chunk allOrganizations into N-sized batches and call hasCliPermission in
parallel per chunk then await each batch) to avoid running hundreds of RPCs at
once; update the variables permissionChecks and allowedOrganizations accordingly
and ensure the error branch and pLog.error message remain unchanged.
In `@src/utils.ts`:
- Around line 1488-1493: Current code builds permissionChecks by mapping
allOrganizations and calling hasCliPermission (which triggers
cli_check_permission per org) in parallel, causing N RPCs; replace this with a
single batched permission API on the backend that accepts an array of org IDs
and returns only the allowed orgs, then call that new endpoint (instead of
hasCliPermission per org) to get filtered organizations in one request; update
the logic around permissionChecks/allOrganizations to use the backend response
and remove the Promise.all-per-org pattern to eliminate per-org RPC overhead.
- Around line 1534-1536: The current auth failure log loses the underlying
error; update the block that checks userId and userIdError to include the error
details from userIdError in the log (e.g., log.error message for the userId/auth
attempt should append or interpolate userIdError.stack/message) while keeping
the thrown Error message unchanged or augmenting it with limited context; locate
the conditional that references userId, userIdError and log.error and change the
log call to include the userIdError details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec62f042-6e5b-4394-9ba5-4a4b0ff59103
📒 Files selected for processing (25)
src/app/add.tssrc/app/delete.tssrc/app/list.tssrc/app/set.tssrc/build/request.tssrc/bundle/cleanup.tssrc/bundle/compatibility.tssrc/bundle/delete.tssrc/bundle/list.tssrc/bundle/unlink.tssrc/bundle/upload.tssrc/channel/add.tssrc/channel/currentBundle.tssrc/channel/delete.tssrc/channel/list.tssrc/channel/set.tssrc/init/command.tssrc/login.tssrc/organization/add.tssrc/organization/delete.tssrc/organization/list.tssrc/organization/members.tssrc/organization/set.tssrc/user/account.tssrc/utils.ts
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
1 participant
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)
login,user account, andorganization addidentity-only instead of gating them on legacy key modes.org.create_appfor app creation and init organization selection.Motivation (AI generated)
The CLI still contained legacy
mode-based checks that could conflict with or bypass the RBAC behavior now implemented in the backend. This change makes the CLI defer permission decisions to backend RBAC-aware checks while preserving the current fallback behavior for legacy keys.Business Impact (AI generated)
This brings the CLI in line with the console and public API authorization model, reducing surprising permission mismatches for customers using API Keys v2 and improving confidence in RBAC-based automation flows.
Test Plan (AI generated)
bunx eslint "src/**/*.ts"bun run typecheckapp addworks withorg.create_appbuild requestusesapp.build_nativebundle uploadchannel updates use RBAC-aware permission checksinitonly offers organizations withorg.create_applogin,user account, andorganization addstill work with a valid API keyGenerated with AI
Summary by CodeRabbit
Release Notes