-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add role-based feature visibility #1658
Conversation
@@ -391,33 +391,6 @@ def download_crash_id(crash_id): | |||
return jsonify(message=url) | |||
|
|||
|
|||
def isValidUser(user_dict): |
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.
per our discussion in slack and this issue comment. we're no longer validating these fields which existed on the Auth0 idToken
.
@@ -458,20 +427,18 @@ def user_test(): | |||
def user_list_users(): | |||
user_dict = current_user._get_current_object() | |||
page = request.args.get("page") | |||
per_page = request.args.get("per_page") | |||
if isValidUser(user_dict) and hasUserRole(ADMIN_ROLE_NAME, user_dict): |
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.
i removed the hasUserRole
check for /list_users
and /get_user
— thereby enabling any authenticated user to access these methods. see this slack thread.
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.
im not super convinced read only users need to be able to see user details like IP addresses and such but 🤷 feels more like a feature for admins. but i dont feel strongly about this
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.
This is a good point. i can't remember why we added the IP in the first place other than because it was available in the Auth0 data. could be something that I'm overlooking.
there is this old comment about seeing where people are logging in from which maybe was more useful when we were all mostly working from the same IPs in-office. 🤷
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.
hmm—yes I wonder if we should altogether scrap the IPs from the app. if we need to do that kind of forensics on an ad-hoc basis we can grab all the info we need and more from the Auth0 dashboard
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.
I created cityofaustin/atd-data-tech#20751 to remove IP addresses from the API and VZE
allowedRoles, | ||
}: PermissionsRequiredProps) { | ||
const { user } = useAuth0(); | ||
if (!allowedRoles || (user && hasRole(allowedRoles, user))) { |
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.
it might be worth memoizing the result of this test but i wasn't sure 🤷
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.
i think this is okay since i don't imagine this firing off more than once per render of this component when the page first loads since the roles and user aren't changing in-app. I think that I'm thinking about that right. 😅
app/app/crashes/page.tsx
Outdated
import TableWrapper from "@/components/TableWrapper"; | ||
const localStorageKey = "crashesListViewQueryConfig"; | ||
|
||
const allowedRoles = ["vz-admin", "editor"]; |
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.
can you update this var to something like createCrashAllowedRoles
so its apparent from the start that this is specifically about roles that are allowed to create, not just view the page
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.
agree here too!
app/app/users/[user_id]/page.tsx
Outdated
import { useToken, formatRoleName } from "@/utils/auth"; | ||
import { useUser } from "@/utils/users"; | ||
import { User } from "@/types/users"; | ||
import { formatDateTime } from "@/utils/formatters"; | ||
|
||
const allowedRoles = ["vz-admin"]; |
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.
maybe editUserRoles
or editUserAllowedRoles
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.
I agree with Rose!
|
||
/** | ||
* Component wrapper that renders `null` if the user does not have one of the | ||
* allwed roles. If the allowedRoles array is undefined then the permission |
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.
wittle typo "allwed"
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.
this is a cool component, i dont think i would have thought about bringing this logic out into its own component and would have conditionally rendered all the role based features in their components. but this way there is no logic being repeated again and again. super cool 🚀
label="Users" | ||
href="/users" | ||
/> | ||
{routes.map((route) => ( |
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.
super snazzy use of mapping thru the routes i like it
app/configs/routes.ts
Outdated
} from "react-icons/fa6"; | ||
import { IconType } from "react-icons"; | ||
|
||
type Route = { |
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.
still trying to figure out when to use types vs interfaces. from these typescript docs:
"For the most part, you can choose based on personal preference, and TypeScript will tell you if it needs something to be the other kind of declaration. If you would like a heuristic, use interface until you need to use features from type."
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.
yes, i think that's a good enough rule. i changed it to an interface🙏
app/utils/auth.ts
Outdated
/** | ||
* Check if a user has any of the provided role names | ||
* @param {string[]} roles - an array of roles to check for | ||
* @param {CustomUser} user - the user object |
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.
i read that for typescript u can leave out type declarations in the docstrings bc it can be repetitive since they are also listed in the param declarations below but nbd
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.
removed 👍
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.
works great!! i really like the choices you made in how to handle this, like creating a PermissionsRequired
component to conditionally render. ✨
I am running through the test steps and when I log out of vze locally, it redirects me to localhost:3000 and 404 not found because it should be localhost:3002. its a setting in auth0 dashboard right, the sign out redirect? |
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.
I agree with Rose's comments about the variable names.
test steps check out 🚢
wrt the logout issues, i tested this some more and see now that we need to pass the logout url to the |
<Dropdown.Item | ||
eventKey="2" | ||
onClick={() => | ||
logout({ logoutParams: { returnTo: window.location.origin } }) |
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.
here's the fix for the busted logout url 👍
✅ Deploy Preview for vision-zero-nextjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
I tested this with all of the user roles, and I see the appropriate UI pieces hidden. Code looks great too. 🙏 🚢 🚀
@@ -458,20 +427,18 @@ def user_test(): | |||
def user_list_users(): | |||
user_dict = current_user._get_current_object() | |||
page = request.args.get("page") | |||
per_page = request.args.get("per_page") | |||
if isValidUser(user_dict) and hasUserRole(ADMIN_ROLE_NAME, user_dict): |
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.
This is a good point. i can't remember why we added the IP in the first place other than because it was available in the Auth0 data. could be something that I'm overlooking.
there is this old comment about seeing where people are logging in from which maybe was more useful when we were all mostly working from the same IPs in-office. 🤷
import TableWrapper from "@/components/TableWrapper"; | ||
const localStorageKey = "crashesListViewQueryConfig"; | ||
|
||
const allowedCreateCrashRecordRoles = ["vz-admin", "editor"]; |
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.
i like these being on the page-level - feels like a nice way to quickly see what permissions apply to things in a specific view. 🚀
allowedRoles, | ||
}: PermissionsRequiredProps) { | ||
const { user } = useAuth0(); | ||
if (!allowedRoles || (user && hasRole(allowedRoles, user))) { |
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.
i think this is okay since i don't imagine this firing off more than once per render of this component when the page first loads since the roles and user aren't changing in-app. I think that I'm thinking about that right. 😅
Thank you for your reviews! |
Associated issues
Here's the first iteration of restricting access to VZE features based on user role. In the interest of avoiding code conflicts I have excluded the crash details page from this work.
Testing
URL to test: Local
Start your local VZE, and also start the user API following the readme in
/api
. Once you have your env set up you should be able to rundocker compose up
and be on your way. Let me know if you need help.You're going to test the VZE with three user accounts whose 1pass entries are named as follows:
Please test all of the pages/features listed below and verify that the app matches this matrix 👇
To summarize these rules: Read-only users can edit nothing. Editors can edit everything except users. Admins can edit everything.
That's it! nice!
Ship list
main
branch