-
Notifications
You must be signed in to change notification settings - Fork 1
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
Mrc-5367- Initial setup of manage access #76
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #76 +/- ##
==========================================
+ Coverage 94.10% 94.47% +0.36%
==========================================
Files 71 75 +4
Lines 577 597 +20
Branches 145 151 +6
==========================================
+ Hits 543 564 +21
+ Misses 32 31 -1
Partials 2 2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Good to extract our the generic sidebar.
I think the visible breadcrumb should be "human readable" i.e. "Manager Users" rather than "manage-users" - as is displayed in the sidebar.
Someone unauthorized could hack their local storage into making the app show the link to the page, but they'd get nothing back from the API so there isn't a security issue with that..(?)
But I do wonder if there's an issue with keeping roles and permissions together in the same "authorities" array. It's unlikely, but an unhinged admin could create a role with the same name as a permission (but which doesn't actually have that permission), which could lead to confusion in the UI.... and actually would that not potentially wrongly grant that permission to any user with the badly named role, since aren't the token 'authorities' used by Spring to apply authorization?
We still don't have e2e tests in packit, right? Getting some in to test e2e auth might not be a bad idea!
return ( | ||
<div className="container h-[800px] flex items-center justify-center m-auto"> | ||
<div className="flex flex-col space-y-2 text-center"> | ||
<h1 className="text-2xl font-semibold tracking-tight text-red-500">401 Unauthorized</h1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting the 401 here might confuse us, since you haven't necessarily had a 401 back from the API to make this show! And probably wouldn't make much sense to non-tech users. 😆 But don't mind!
|
As discussed, let's take roles out of the token authorities (since we only use permissions as authorities). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - I think it would also be good to capitalise "Manage Users" (and roles) - to in the breadcrumb, to match the casing on the sidebar and the breadcrumb for "Home". Not a showstopper though..
This is the initial work manage access pages. It includes the following :
Dev testing:
figma: https://www.figma.com/design/NMR5XdmWYfphrrRYgIV2US/Packit-User-manage?node-id=22-285&t=llDh1pAzXzdMAunV-0