Skip to content
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

Dashboard permissions #2131

Merged
merged 13 commits into from
Dec 19, 2023
Merged

Dashboard permissions #2131

merged 13 commits into from
Dec 19, 2023

Conversation

TheSlimvReal
Copy link
Collaborator

@TheSlimvReal TheSlimvReal commented Dec 14, 2023

closes: #2122, closes #1552

  • Permissions for shortcut widget
  • Permissions for entity widgets

Copy link

Deployed to https://pr-2131.aam-digital.net/

@TheSlimvReal TheSlimvReal changed the base branch from master to guards_fix December 14, 2023 20:57
@TheSlimvReal TheSlimvReal marked this pull request as ready for review December 14, 2023 20:58
@TheSlimvReal TheSlimvReal requested a review from sleidig December 14, 2023 20:58
Copy link
Member

@sleidig sleidig left a comment

Choose a reason for hiding this comment

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

Seems to work well already 👍

Shall we also add the option similar to views, to assign "permittedUserRoles": ["custom_role"] to any dashboard widget?

@@ -53,6 +53,7 @@ export class EntityPermissionGuard implements CanActivate {
path = path.replace(/^\//, "");

function isPathMatch(genericPath: string, path: string) {
// TODO this does not seem to work with children routes (admin module)
Copy link
Member

Choose a reason for hiding this comment

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

yes, I was a bit lazy implementing this ... 😅

I think the logic could actually be shared between all guards, so we can make the code from UserRoleGuard reusable:

private getRouteConfig(path: string): ViewConfig {

(side note: router.config already contains all the data from config, so parsing the config there is actually unnecessary)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure what you mean here. What is the shared logic? And also regarding the router.config I am not fully following. Do you want to extend it here if you have a improvement in mind?

Copy link
Member

Choose a reason for hiding this comment

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

generalized the code. Please review my commit :-)

src/app/core/dashboard/dashboard/dashboard.component.ts Outdated Show resolved Hide resolved
Base automatically changed from guards_fix to master December 18, 2023 11:01
# Conflicts:
#	src/app/core/permissions/permission-guard/entity-permission.guard.ts
#	src/app/core/ui/navigation/navigation/navigation.component.spec.ts
#	src/app/core/ui/navigation/navigation/navigation.component.ts
@TheSlimvReal TheSlimvReal requested a review from sleidig December 18, 2023 17:10
Copy link
Member

@sleidig sleidig left a comment

Choose a reason for hiding this comment

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

Really nice generalizations and great cleanup of the interfaces! 😃

@TheSlimvReal , please review my refactoring of the guards that is part of this PR as well, before merging this.

}

/**
* Extract the relevant route from Router, to get a merged route that contains the full trail of `permittedRoles`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what do you mean by "full trail of permittedRoles"?

Copy link
Member

Choose a reason for hiding this comment

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

this includes the "permittedRoles" from parent routes of nested routing. Not perfect merge of route data, however - for more complex use cases this might potentially break, I think.

@TheSlimvReal TheSlimvReal linked an issue Dec 19, 2023 that may be closed by this pull request
@TheSlimvReal TheSlimvReal merged commit 4f0daf9 into master Dec 19, 2023
6 of 7 checks passed
@TheSlimvReal TheSlimvReal deleted the permission_dashboard branch December 19, 2023 11:41
@aam-digital-ci
Copy link
Collaborator

🎉 This PR is included in version 3.28.0-master.5 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@aam-digital-ci aam-digital-ci added the released on @master managed by CI (semantic-release) label Dec 19, 2023
@aam-digital-ci
Copy link
Collaborator

🎉 This PR is included in version 3.28.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@aam-digital-ci aam-digital-ci added the released managed by CI (semantic-release) label Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @master managed by CI (semantic-release) released managed by CI (semantic-release)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add permissions to dashboard Custom Dashboard for individual user accounts
3 participants