Skip to content

Consider required permission of ancestors in MasterMenuRoutes #3816

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

johnnyomair
Copy link
Collaborator

@johnnyomair johnnyomair commented Apr 29, 2025

Description

It's possible to provide a required permission for a collapsible or group in MasterMenuData, but the parent permissions wasn't considered in MasterMenuRoutes. This PR adds a check for the required permission of the ancestors.

Acceptance criteria

Screenshots/screencasts

Error

Master.menu.routes.error.mov

Fix

Master.menu.routes.fix.mov

Further information

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR ensures that the required permission for a parent group or collapsible item is properly considered in the routing. Key changes include adding a parent permission check in MasterMenuRoutes, updating access control logic for additional permissions and scopes, and incorporating a new content scope configuration hook in NewsPage.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
packages/admin/cms-admin/src/common/MasterMenuRoutes.tsx Added parent permission check and updated recursive routing logic; note potential bug in collapsible branch.
demo/api/src/auth/access-control.service.ts Expanded denied permissions and updated content scopes for non-admin users.
demo/admin/src/news/NewsPage.tsx Added a call to useContentScopeConfig to set the redirect path after scope change.
.changeset/little-houses-wink.md Updated changeset file for patch-level versioning.

@johnnyomair johnnyomair requested a review from Copilot April 29, 2025 15:53
Copilot

This comment was marked as duplicate.

@nsams
Copy link
Member

nsams commented Apr 30, 2025

master menu can have more tan 2 levels - the data structure is an unlimited tree, styling in MainNavigation ends at level 3 or 4 tough.

@johnnyomair
Copy link
Collaborator Author

master menu can have more tan 2 levels - the data structure is an unlimited tree, styling in MainNavigation ends at level 3 or 4 tough.

But this is out of scope of this PR, right?

@nsams
Copy link
Member

nsams commented May 5, 2025

But this is out of scope of this PR, right?

hm? if MainNavigation supports 3 levels, MasterMenuRoutes must also. Yes you could do it in 2 steps, but why?

@johnnyomair
Copy link
Collaborator Author

hm? if MainNavigation supports 3 levels, MasterMenuRoutes must also. Yes you could do it in 2 steps, but why?

Sorry, I misunderstood your comment. Fixed: 357112f

@johnnyomair johnnyomair changed the title Consider required permission of parent group or collapsible in MasterMenuRoutes Consider required permission of ancestors in MasterMenuRoutes May 13, 2025
thomasdax98
thomasdax98 previously approved these changes May 13, 2025
fraxachun
fraxachun previously approved these changes May 15, 2025
nsams
nsams previously approved these changes May 20, 2025
@johnnyomair johnnyomair marked this pull request as draft May 21, 2025 16:34
@johnnyomair
Copy link
Collaborator Author

We need to decide if we want the redirect or show a not found page instead. @manuelblum will discuss this with UX tomorrow.

@johnnyomair johnnyomair dismissed stale reviews from nsams, fraxachun, and thomasdax98 via 4502b5e May 27, 2025 12:44
@johnnyomair johnnyomair force-pushed the master-menu-routes-parent-permission branch from 709aea4 to 4502b5e Compare May 27, 2025 12:44
@johnnyomair
Copy link
Collaborator Author

We discussed this with UX and decided to a) redirect for routes that exist but can't be accessed and b) show a not found page for non-existing routes. Changed here: 4502b5e.

The changes introduced in 5bec677 didn't consider that collapsible items can have both a route and items. Change the return to an assignment to reflect that.
@johnnyomair johnnyomair requested a review from Copilot May 27, 2025 12:51
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances MasterMenuRoutes to respect required permissions of ancestor menu items and adds a fallback route for unauthorized or unknown paths.

  • Check ancestor permissions in the route-flattening logic
  • Introduce a wildcard route to redirect unauthorized access to the first allowed route
  • Add unit tests for ancestor permission filtering and update Jest config for ESM stubs

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/admin/cms-admin/src/testing/stub-file.ts Add Jest stub for ESM modules
packages/admin/cms-admin/src/common/MasterMenuRoutes.tsx Extend permission check to include ancestors; add catch-all route
packages/admin/cms-admin/src/common/MasterMenuRoutes.test.tsx Add tests for ancestor permission filtering
packages/admin/cms-admin/jest.config.ts Configure moduleNameMapper to use the stub file
demo/api/src/auth/access-control.service.ts Adjust default permissions and content scopes
demo/admin/src/news/NewsPage.tsx Wire up useContentScopeConfig and import new hook
.changeset/little-houses-wink.md Record patch-level changeset
Comments suppressed due to low confidence (4)

packages/admin/cms-admin/src/common/MasterMenuRoutes.tsx:26

  • [nitpick] The helper function name flat is ambiguous; consider renaming it to something more descriptive, such as flattenRoutes.
const flat = (

packages/admin/cms-admin/src/common/MasterMenuRoutes.tsx:66

  • The new catch-all route logic for unauthorized or unknown paths isn't covered by existing tests; consider adding a test to verify redirects or 404 handling.
<RouteWithErrorBoundary

demo/admin/src/news/NewsPage.tsx:34

  • [nitpick] The scope variable is retrieved but never used; consider removing the destructuring or using scope to avoid an unused-variable warning.
const { scope } = useContentScope();

demo/api/src/auth/access-control.service.ts:10

  • [nitpick] Excluding news in deniedPermissions and then immediately re-adding it with specific contentScopes can be confusing; consider refactoring this into a clearer, two-step process or adding a comment to explain the intent.
const deniedPermissions = ["userPermissions", "news"];

Co-authored-by: Copilot <[email protected]>
@johnnyomair johnnyomair marked this pull request as ready for review May 27, 2025 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants