Repository navigation
Migrate legacy CAN() permission checks to Sistent useHasPermission hook (Pattern C) - #20860
Conversation
Signed-off-by: Rishi Raj <[email protected]>
📝 WalkthroughWalkthroughThis PR replaces legacy ChangesPermission migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Still draft, @rishiraj38? |
Yep, working on it right now. |
Signed-off-by: Rishi Raj <[email protected]>
|
@coderabbitai review |
|
@coderabbitai help |
✅ Action performedReview finished.
|
This comment was marked as off-topic.
This comment was marked as off-topic.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
ui/components/connections/ConnectionTable.test.tsx (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBlanket
useHasPermission: () => truemocks skip permission-denied coverage. All three test files mock the hook to always returntrue, so none exercise the disabled/hidden UI states when permission is absent (env-assignment select and status select inConnectionTable, "Get Token" menu item inHeaderMenu, "Publish to Catalog" button inLegacyInfoModal).
ui/components/connections/ConnectionTable.test.tsx#L83-83: consider a variant test withuseHasPermission: () => falseto assertMultiSelectWrapper/ConnectionStatusSelectrenderdisabled.ui/components/layout/Header/HeaderMenu.test.tsx#L69-69: consider asserting the "Get Token" item is hidden whenhasDownloadTokenPermissionisfalse.ui/components/shared/Modal/Information/InfoModal.test.tsx#L207-207: consider asserting the "Publish to Catalog" button is disabled whencanPublishDesignisfalse.🤖 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 `@ui/components/connections/ConnectionTable.test.tsx` at line 1, The tests only cover permitted states because useHasPermission is always mocked to return true. Add denied-permission variants in ConnectionTable tests to verify MultiSelectWrapper and ConnectionStatusSelect are disabled, in HeaderMenu tests to verify “Get Token” is hidden when hasDownloadTokenPermission is false, and in InfoModal tests to verify “Publish to Catalog” is disabled when canPublishDesign is false.ui/components/dashboard/charts/ConnectionCharts.test.tsx (1)
82-82: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake the permission mocks key-aware.
These mocks control permission outcomes but do not validate which permission key production requests, leaving authorization wiring regressions undetected.
ui/components/dashboard/charts/ConnectionCharts.test.tsx#L82-L82: forwarduseHasPermissiontocanSpyand assertKeys.WorkspaceManagementViewConnections.ui/components/dashboard/charts/DashboardMeshModelGraph.test.tsx#L67-L67: replace the unconditionaltruemock with a spy and assertKeys.MesherySystemViewRegistry.ui/components/settings/MesherySettings.test.tsx#L67-L67: replace the shared boolean with a key-aware mapping or spy covering the settings, infrastructure, and registry keys.🤖 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 `@ui/components/dashboard/charts/ConnectionCharts.test.tsx` at line 82, Make all permission mocks key-aware: in ui/components/dashboard/charts/ConnectionCharts.test.tsx at lines 82-82, forward the requested key from useHasPermission to canSpy and assert Keys.WorkspaceManagementViewConnections; in ui/components/dashboard/charts/DashboardMeshModelGraph.test.tsx at lines 67-67, replace the unconditional true mock with a spy and assert Keys.MesherySystemViewRegistry; in ui/components/settings/MesherySettings.test.tsx at lines 67-67, replace the shared boolean with a key-aware mapping or spy covering the settings, infrastructure, and registry permission keys.
🤖 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 `@ui/components/connections/wizard/genericSteps.tsx`:
- Around line 86-90: The selectStep canProceed guard must enforce kind
permissions for preset/deep-linked connections, not only check that kindConfig
exists. Update selectStep.canProceed to require
kindPermission(ctx.data.kindConfig) alongside the existing presence check,
preserving normal selection behavior while rejecting unauthorized kinds.
In `@ui/components/designs/patterns/MesheryPatterns.tsx`:
- Around line 72-83: Update the permission gate in MesheryPatterns so
catalog-only users authorized by Keys.CatalogManagementViewCatalog are not
rejected by the separate Keys.CatalogManagementViewDesigns check. Pass the
page-specific permission into the shared component or condition the gate on the
active route, while preserving the existing design permission behavior for
design routes.
In `@ui/components/settings/MesherySettings.tsx`:
- Line 255: Gate the registry content rendering, not only its tab control:
update the REGISTRY branch identified by tabVal and MeshModelComponent to
require canViewRegistry, or enforce the equivalent check inside
MeshModelComponent, so direct Registry routes cannot render without permission.
In `@ui/components/shared/Modal/Information/LegacyInfoModal.tsx`:
- Around line 496-501: Update the disabled expression for the “Publish to
Catalog” button in LegacyInfoModal so unpublished designs are enabled only when
canPublishDesign is true and the current user owns the selected resource, while
published designs remain disabled. Remove the swapped ternary branches and
redundant isPublished condition so the permission and ownership checks affect
the result.
In `@ui/components/workspaces/WorkspaceFormModalNav.tsx`:
- Line 50: Update WorkspaceContentWrapper so its getNavItem call passes the
required canViewViews argument, propagating the existing permission value or
retrieving it within the wrapper before building navigation items. Ensure all
getNavItem invocations satisfy its updated signature.
---
Nitpick comments:
In `@ui/components/connections/ConnectionTable.test.tsx`:
- Line 1: The tests only cover permitted states because useHasPermission is
always mocked to return true. Add denied-permission variants in ConnectionTable
tests to verify MultiSelectWrapper and ConnectionStatusSelect are disabled, in
HeaderMenu tests to verify “Get Token” is hidden when hasDownloadTokenPermission
is false, and in InfoModal tests to verify “Publish to Catalog” is disabled when
canPublishDesign is false.
In `@ui/components/dashboard/charts/ConnectionCharts.test.tsx`:
- Line 82: Make all permission mocks key-aware: in
ui/components/dashboard/charts/ConnectionCharts.test.tsx at lines 82-82, forward
the requested key from useHasPermission to canSpy and assert
Keys.WorkspaceManagementViewConnections; in
ui/components/dashboard/charts/DashboardMeshModelGraph.test.tsx at lines 67-67,
replace the unconditional true mock with a spy and assert
Keys.MesherySystemViewRegistry; in
ui/components/settings/MesherySettings.test.tsx at lines 67-67, replace the
shared boolean with a key-aware mapping or spy covering the settings,
infrastructure, and registry permission keys.
🪄 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: 47e9eb4a-e213-4516-9b6a-0af37cc90375
📒 Files selected for processing (55)
ui/components/connections/ConnectionTable.columns.tsxui/components/connections/ConnectionTable.test.tsxui/components/connections/ConnectionWizardLauncher.tsxui/components/connections/index.test.tsxui/components/connections/index.tsxui/components/connections/wizard/genericSteps.tsxui/components/dashboard/charts/ConnectionCharts.test.tsxui/components/dashboard/charts/ConnectionCharts.tsxui/components/dashboard/charts/DashboardMeshModelGraph.test.tsxui/components/dashboard/charts/DashboardMeshModelGraph.tsxui/components/dashboard/charts/KubernetesConnectionChart.test.tsxui/components/dashboard/charts/KubernetesConnectionChart.tsxui/components/dashboard/charts/MesheryConfigurationCharts.test.tsxui/components/dashboard/charts/MesheryConfigurationCharts.tsxui/components/dashboard/charts/WorkloadChart.test.tsxui/components/dashboard/charts/WorkloadChart.tsxui/components/dashboard/widgets/getting-started/index.test.tsxui/components/dashboard/widgets/getting-started/index.tsxui/components/designs/lifecycle/DeployConfirmationModal.tsxui/components/designs/patterns/MesheryPatternCard.tsxui/components/designs/patterns/MesheryPatterns.columns.test.tsxui/components/designs/patterns/MesheryPatterns.columns.tsxui/components/designs/patterns/MesheryPatterns.tsxui/components/designs/patterns/design-permissions.test.tsui/components/designs/patterns/design-permissions.tsui/components/environments/environment-card.tsxui/components/environments/index.tsxui/components/filters/Filters.tsxui/components/layout/AppShell/AppComponents.test.tsxui/components/layout/AppShell/AppComponents.tsxui/components/layout/Header/HeaderMenu.test.tsxui/components/layout/Header/HeaderMenu.tsxui/components/layout/Navigator/NavigatorExtension.tsxui/components/performance/index.tsxui/components/settings/MesherySettings.test.tsxui/components/settings/MesherySettings.tsxui/components/shared/Modal/Information/InfoModal.test.tsxui/components/shared/Modal/Information/LegacyInfoModal.tsxui/components/workspaces/MesheryWorkspaceCard.tsxui/components/workspaces/SpacesSwitcher/MainDesignsContent.tsxui/components/workspaces/SpacesSwitcher/MainViewsContent.tsxui/components/workspaces/SpacesSwitcher/RecentContent.tsxui/components/workspaces/SpacesSwitcher/SharedContent.tsxui/components/workspaces/SpacesSwitcher/WorkspaceContent.tsxui/components/workspaces/SpacesSwitcher/WorkspaceSwitcher.tsxui/components/workspaces/SpacesSwitcher/components.tsxui/components/workspaces/WorkspaceActionList.test.tsxui/components/workspaces/WorkspaceActionList.tsxui/components/workspaces/WorkspaceDataTable.tsxui/components/workspaces/WorkspaceFormModalNav.tsxui/components/workspaces/WorkspaceGridView.test.tsxui/components/workspaces/WorkspaceGridView.tsxui/components/workspaces/index.tsxui/pages/configuration/catalog.tsxui/pages/extensions.tsx
Signed-off-by: Rishi Raj <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@ui/components/connections/wizard/useConnectionWizard.tsx`:
- Around line 145-150: Update the preset-handling effect around
appliedPresetRef, data.kindConfig, and kindPermission so permission is confirmed
before consuming the preset or changing wizard state. When permission is denied,
leave the preset unapplied and do not select or advance it; when permission
becomes available later, allow the same preset to be processed and advance to
the kind-details step. Add coverage for denied and delayed-permission scenarios.
In `@ui/components/workspaces/WorkspaceFormModalNav.tsx`:
- Line 100: Update the canViewViews permission checks in WorkspaceContentWrapper
and the related Navigation/getNavItem callers to consistently use
Keys.KanvasViewViews, matching the existing navigation authorization contract
and eliminating use of Keys.CatalogManagementViewViews for My-Views.
🪄 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: 7115e567-cdf0-4565-9410-6d2b55c8c629
📒 Files selected for processing (5)
ui/components/connections/wizard/useConnectionWizard.tsxui/components/designs/patterns/MesheryPatterns.tsxui/components/settings/MesherySettings.tsxui/components/shared/Modal/Information/LegacyInfoModal.tsxui/components/workspaces/WorkspaceFormModalNav.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/components/settings/MesherySettings.tsx
- ui/components/designs/patterns/MesheryPatterns.tsx
|
@rishiraj38 conflict 👁️ |
Signed-off-by: Rishi Raj <[email protected]>
…ration Addresses the outstanding review feedback on the CAN() -> useHasPermission migration. WorkspaceFormModalNav: the two `getNavItem` callers read different keys for the same "My Views" item - the drawer used `KanvasViewViews` (as master did) while the new content wrapper used `CatalogManagementViewViews`, so navigation and content could authorize differently. Replaced the helper with a single `useNavConfig` hook that reads `KanvasViewViews` once, which removes the class of defect rather than the one instance, and made the wrapper honour `enabled !== false` so a stale selection cannot render a gated item. useConnectionWizard: the preset deep-link recorded `appliedPresetRef` before evaluating `kindPermission`. Permissions resolve asynchronously, so a preset seen while access was still denied latched in and the effect short-circuited once permission arrived, never advancing an authorized user. Permission is now resolved first; a denied kind is left unapplied so the kind chooser (which already disables unusable kinds) stays in charge. Also hardened the params-sync effects, which wrote a fresh state object on every run. A caller passing a non-memoized `availableKinds` would re-render, mint new params, and re-run the effect without end; they now compare by value and return the current state unchanged so React bails out. Extracted `useKindPermission` into its own module - the wizard hook was importing the entire step UI just to read one permission rule. Tests: new `useConnectionWizard` coverage for the granted, denied and delayed-permission preset cases plus the selected-kind guard; new `ConnectionTable.columns` coverage asserting each row control is gated on its own key in both directions; and the blanket `useHasPermission: () => true` mocks in the charts, settings, header and info-modal suites are now key-aware so a cross-wired key fails the suite instead of passing silently. Signed-off-by: Arjun Mehta <[email protected]>
|
Picking up the two review nitpicks, both accepted and addressed in cfce4dc. Key-aware permission mocks. A blanket Permission-denied coverage. Added for all three sites, with one correction: for The Also removed the now-dead Locally: |
|
@yi-nuo426 the conflict was resolved by the master merge in 66f4ba5 - the branch reports |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@ui/components/workspaces/WorkspaceFormModalNav.tsx`:
- Line 151: Update the navigation state around navConfig and selectedId so that
when the currently selected item is no longer present because its permission is
denied, selectedId is reset to the allowed default view (such as Recents).
Preserve the existing selection when the selected navigation item remains
enabled, and ensure the header and displayed content use the reconciled
selection.
🪄 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: 38398844-08c6-4702-b0aa-4613340ae649
📒 Files selected for processing (11)
ui/components/connections/ConnectionTable.columns.test.tsxui/components/connections/wizard/genericSteps.tsxui/components/connections/wizard/useConnectionWizard.test.tsxui/components/connections/wizard/useConnectionWizard.tsxui/components/connections/wizard/useKindPermission.tsui/components/dashboard/charts/ConnectionCharts.test.tsxui/components/dashboard/charts/DashboardMeshModelGraph.test.tsxui/components/layout/Header/HeaderMenu.test.tsxui/components/settings/MesherySettings.test.tsxui/components/shared/Modal/Information/InfoModal.test.tsxui/components/workspaces/WorkspaceFormModalNav.tsx
…ow tests `ui/components/workspaces/index.test.tsx` and `ui/components/environments/index.test.tsx` landed on master after this branch's last merge. Both stub `@sistent/sistent` with an explicit export list that predates this PR's migration of the components they render, so once `Workspaces` and `Environments` read `useHasPermission` the mock resolves it to undefined and the suites fail on render. A semantic conflict rather than a textual one: neither side is wrong on its own, so it only surfaces once the branch is merged with current master. Both suites exercise the create flow rather than authorization, so the mock grants every capability and the permission gates stay out of the way of the behaviour under test. Signed-off-by: Arjun Mehta <[email protected]>
|
CI's
Merged current master and added Full local run on the merged tree: 2650 passed / 18 skipped, |
Neither InfoModal nor LegacyInfoModal reads `@/utils/can` after the migration, so the stub is inert. The equivalent stubs were already removed from the charts, settings and header suites in this branch; this leaves no odd one out. Signed-off-by: Arjun Mehta <[email protected]>
When My Views is selected and its permission is later denied, the drawer drops the item and the content wrapper falls back to Recents, but `selectedId` kept its value - so `updateHeaderInfo` found no matching item, never updated, and the header went on naming a view whose content was no longer rendered. Reconcile the selection to the default when the selected nav item loses its gate. The guard is a boolean derived from the unfiltered item list, so it cannot loop and never touches a selected workspace id (those are not nav ids). Adds the first test coverage for this component: the permission-gated item is listed or omitted as expected, the selected item's content renders, and the revocation path is asserted end to end (content falls back *and* the header follows). Verified the last case fails without the fix. Signed-off-by: Arjun Mehta <[email protected]>
|
Commit SHA: END-TO-END TESTS
📦 Test Result Summary
⌛ Duration: 3 minutes and 8 seconds Overall Result: 👍 All tests passed. [Show/Hide] Test Result Details
🔗 Relationship Tests [Show/Hide] Relationship Test Details (18 tests)
|
|
Thanks, @arjunmehta-git. |
Description
This PR completes the Pattern C migration across Meshery UI components. Legacy
CAN()evaluation functions from@/utils/canhave been replaced with Sistent's reactiveuseHasPermissionhook for layout/section gating and conditional variable evaluation.Key Changes
Connections & Wizards:
ui/components/connections/ConnectionTable.columns.tsx: Migrated permission checks in column definitions touseHasPermission.ui/components/connections/ConnectionWizardLauncher.tsx: Replaced directCAN()evaluations withuseHasPermissionhook calls forKeys.LifecycleManagementAddClusterandKeys.MesherySystemConnectMetrics.ui/components/connections/index.tsx: Updated workspace view permissions check to useuseHasPermission.ui/components/connections/wizard/genericSteps.tsx: IntroduceduseKindPermissionhook replacing legacykindPermissionutility function.Workspaces:
ui/components/workspaces/WorkspaceActionList.tsx: Replaced inlineCAN()calls withcanEditWorkspaceandcanDeleteWorkspacehooks.ui/components/workspaces/WorkspaceFormModalNav.tsx: RefactoredgetNavItemhelper to accept resolved permission props from the parent component.ui/components/workspaces/index.tsx: Replaced 7 inlineCAN()calls with top-level permission hooks.Environments:
ui/components/environments/index.tsx: Migrated 7 inlineCAN()calls to top-level permission hook variables.ui/components/environments/environment-card.tsx: Cleaned up legacy commentedCAN()code.Designs:
ui/components/designs/lifecycle/DeployConfirmationModal.tsx: Migrated design validate/deploy/undeploy checks touseHasPermission.ui/components/designs/patterns/design-permissions.ts: RefactoredcanEditDesignto a pure function accepting the permission boolean to adhere to React Rule of Hooks.ui/components/designs/patterns/MesheryPatterns.tsx: Migrated design catalog permission checks to top-level hooks.ui/components/designs/patterns/MesheryPatterns.columns.tsx: RefactoredbuildPatternActionsandbuildPatternColumnsto accept apermissionsobject.ui/components/designs/patterns/MesheryPatternCard.tsx: UpdatedcanEditDesigncall signature withhasEditPermission.Filters & Settings:
ui/components/filters/Filters.tsx: Migrated filter catalog view, import, publish, and details checks touseHasPermission.ui/components/settings/MesherySettings.tsx: Migrated system settings and adapters view checks touseHasPermission.Layout, Extensions & Shared Components:
ui/components/layout/AppShell/AppComponents.tsx: MigratedKubernetesSubscriptionpermissions touseHasPermission.ui/components/layout/Navigator/NavigatorExtension.tsx: ExposeduseHasPermissionhook underhooksinside remote component injection props.ui/components/shared/Modal/Information/LegacyInfoModal.tsx: ReplacedCAN()withcanPublishDesignhook.ui/components/dashboard/widgets/getting-started/index.tsx: UpdatedcanAssignUserRolescheck.Test Suite Updates:
useHasPermissionand preserve dynamic permission assertions.Verification
npm run test) pass cleanly (380 test files, 2608 tests).make ui-build) compiles and exports static pages with zero errors.Signed commits
Summary by CodeRabbit
Refactor / Authorization
Tests
useHasPermission, plus added/adjusted coverage for permission-driven rendering and link/button enablement/disablement.