-
-
Notifications
You must be signed in to change notification settings - Fork 658
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
feat: upgrade AdminAlert to PermissionGuard #4074
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
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 helps to explain to the user what type of permission they need, but I'm not sure this is what we want to do. Most services don't provide many details about which permissions you're missing. I think it's a good feature but I'd validate it with others
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 concur with Gaston here. I really like this feature, it might even help people request the correct access from their manager / admin.
Ping @thomasheartman - What do you think?
d052ecb
to
5a07e46
Compare
https://linear.app/unleash/issue/2-1165/improve-adminalert-usage-to-be-more-generic-accept-non-admin
Upgrades our
AdminAlert
to a newPermissionGuard
.Question: We don't need to, but should we be specific about the
ADMIN
permission every time?Technically
PermissionGuard
could havepermissions
as optional and assume[]
by default, which will addADMIN
anyways. However, I feel like we may gain some readability if we're specific about it. WDYT?Single permission:
Multiple permissions: