EDM-3608: Check permissions for catalog/item list#602
Merged
rawagner merged 2 commits intoflightctl:mainfrom Mar 31, 2026
Merged
EDM-3608: Check permissions for catalog/item list#602rawagner merged 2 commits intoflightctl:mainfrom
rawagner merged 2 commits intoflightctl:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughSeveral catalog-related UI components now perform RBAC checks via the permissions context and render their content inside a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Component
participant PermissionsCtx
participant PageWithPerms
participant UI
User->>Component: navigate / open page or wizard
Component->>PermissionsCtx: checkPermissions(permissionSet)
PermissionsCtx-->>Component: { canX, canY, loading }
Component->>PageWithPerms: render(allowed=(canX && canY), loading=loading)
PageWithPerms-->>Component: show gated content OR show denied/loading
Component->>UI: render inner UI (or denied/loading state)
UI-->>User: display result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
🧹 Nitpick comments (1)
libs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsx (1)
26-29: Permission gating implementation looks correct.The permission check setup and
PageWithPermissionswrapper correctly gate access based oncanListItems && canListCatalogs. Per learnings,checkPermissionsis a pure calculation on cached permission arrays, so the loading state handling here is appropriate.Note: This file defines its own
catalogPagePermissionsconstant (lines 26-29) that duplicates the one inCatalogPage.tsx(lines 428-429). Consider extracting this to a shared location to avoid drift.Also applies to: 41-42, 59-59, 75-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsx` around lines 26 - 29, The constant catalogPagePermissions is duplicated (defined in ResourceCatalogPage as catalogPagePermissions and also in CatalogPage) — extract this shared permission array into a single exported symbol (e.g., export const catalogPagePermissions from a shared permissions module or from CatalogPage) and update ResourceCatalogPage to import and use that single exported catalogPagePermissions instead of redefining it; update all usages (e.g., where PageWithPermissions consumes catalogPagePermissions and any local references to canListItems/canListCatalogs) to reference the shared symbol so the permission definition is centralized and avoids drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@libs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsx`:
- Around line 26-29: The constant catalogPagePermissions is duplicated (defined
in ResourceCatalogPage as catalogPagePermissions and also in CatalogPage) —
extract this shared permission array into a single exported symbol (e.g., export
const catalogPagePermissions from a shared permissions module or from
CatalogPage) and update ResourceCatalogPage to import and use that single
exported catalogPagePermissions instead of redefining it; update all usages
(e.g., where PageWithPermissions consumes catalogPagePermissions and any local
references to canListItems/canListCatalogs) to reference the shared symbol so
the permission definition is centralized and avoids drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2842fb4e-f522-4d34-815e-9ec077ec897c
📒 Files selected for processing (2)
libs/ui-components/src/components/Catalog/CatalogPage.tsxlibs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsx
celdrake
reviewed
Mar 30, 2026
| onInstall: (installItem: { item: CatalogItem; channel: string; version: string }) => void; | ||
| }; | ||
|
|
||
| const catalogPagePermissions = [ |
Collaborator
There was a problem hiding this comment.
This is good for CatalogPage, but I believe other sections in the UI also need additional checks.
Quick review from Cursor:
The add/edit item wizard’s General Info step, install wizard, edit-from-catalog wizard, edit item wizard, and asset picker modal can still issue catalog/catalog-item requests without a matching client-side permission check first;
Collaborator
Author
There was a problem hiding this comment.
added those additional locations as well
rawagner
force-pushed
the
catalog_permissions
branch
from
b596848 to
4793899
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/ui-components/src/components/Catalog/EditWizard/EditWizard.tsx (1)
252-253: Avoid per-component permission loading gates for cached checks.Line 252 and Line 280 already use
checkPermissions, which is synchronous against cached permissions. PassingpermissionsLoadingintoPageWithPermissionshere adds unnecessary spinner gating; prefer gating only byallowed.♻️ Suggested simplification
- const { checkPermissions, loading: permissionsLoading } = usePermissionsContext(); + const { checkPermissions } = usePermissionsContext(); const [canGetItem] = checkPermissions(editWizardPermissions); @@ - <PageWithPermissions allowed={canGetItem} loading={permissionsLoading}> + <PageWithPermissions allowed={canGetItem} loading={false}> @@ - const { checkPermissions, loading: permissionsLoading } = usePermissionsContext(); + const { checkPermissions } = usePermissionsContext(); const [canGetItem] = checkPermissions(editWizardPermissions); @@ - <PageWithPermissions allowed={canGetItem} loading={permissionsLoading}> + <PageWithPermissions allowed={canGetItem} loading={false}>Based on learnings: In PR
#376,checkPermissionsinusePermissionsContextis a pure calculation on cached permissions and loading state handling is generally not required at individual component call sites.Also applies to: 259-260, 280-281, 287-288
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/EditWizard/EditWizard.tsx` around lines 252 - 253, The component is passing the synchronous cached-loading flag permissionsLoading from usePermissionsContext into PageWithPermissions, causing unnecessary spinner gating; instead call checkPermissions (as already done to get canGetItem) and remove permissionsLoading from PageWithPermissions props so gating is driven only by the allowed boolean (e.g., canGetItem). Update all occurrences where permissionsLoading is forwarded (including the other checkPermissions usages around the EditWizard component such as the canGetItem/canEdit checks) to stop passing permissionsLoading and rely solely on the allowed flags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@libs/ui-components/src/components/Catalog/AddCatalogItemWizard/AddCatalogItemWizard.tsx`:
- Around line 256-260: The edit-mode permission gate
(addCatalogItemWizardPermissions) currently requires VERB.PATCH for
RESOURCE.CATALOG_ITEM but not VERB.GET, causing fetch errors when the component
(which calls the GET for the item at load) lacks read permission; update
addCatalogItemWizardPermissions to include { kind: RESOURCE.CATALOG_ITEM, verb:
VERB.GET } alongside the existing PATCH/CREATE entries so users who can edit
also have permission to fetch the item (refer to
addCatalogItemWizardPermissions, RESOURCE.CATALOG_ITEM, VERB.GET, VERB.PATCH).
---
Nitpick comments:
In `@libs/ui-components/src/components/Catalog/EditWizard/EditWizard.tsx`:
- Around line 252-253: The component is passing the synchronous cached-loading
flag permissionsLoading from usePermissionsContext into PageWithPermissions,
causing unnecessary spinner gating; instead call checkPermissions (as already
done to get canGetItem) and remove permissionsLoading from PageWithPermissions
props so gating is driven only by the allowed boolean (e.g., canGetItem). Update
all occurrences where permissionsLoading is forwarded (including the other
checkPermissions usages around the EditWizard component such as the
canGetItem/canEdit checks) to stop passing permissionsLoading and rely solely on
the allowed flags.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf0c6f8c-6b22-4d13-819a-293ac62c06ca
📒 Files selected for processing (6)
libs/ui-components/src/components/Catalog/AddCatalogItemWizard/AddCatalogItemWizard.tsxlibs/ui-components/src/components/Catalog/CatalogPage.tsxlibs/ui-components/src/components/Catalog/EditWizard/EditWizard.tsxlibs/ui-components/src/components/Catalog/InstallWizard/InstallWizard.tsxlibs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsxlibs/ui-components/src/components/DynamicForm/VolumeImageField.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/ui-components/src/components/Catalog/CatalogPage.tsx
libs/ui-components/src/components/Catalog/AddCatalogItemWizard/AddCatalogItemWizard.tsx
Show resolved
Hide resolved
rawagner
force-pushed
the
catalog_permissions
branch
from
4793899 to
e8a66ef
Compare
rawagner
added a commit
to rawagner/flightctl-ui
that referenced
this pull request
Mar 31, 2026
rawagner
added a commit
that referenced
this pull request
Mar 31, 2026
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
3 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 by CodeRabbit