-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Conditionally render User edit/delete actions in the web UI #49645
Conversation
tipContent={ | ||
canEditUsers | ||
? '' | ||
: 'You do not have permission to edit users.' |
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.
Should this be
: 'You do not have permission to edit users.' | |
: 'You do not have permission to create users.' |
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.
techincally they can create, but the edit is required in the second step. I'm not sure which is better as the "edit" version is more correct, but its on the 'create' button.
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.
and i don't want to hide the button, because we don't want to hide features anymore, only display permission errors
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 it should make sense from the user's perspective, i.e. I'm trying to create a user and don't have permissions, so I don't have permission to create users. Maybe this could be more vague, "You do not have enough permissions" or something better worded
IMO, we should indicate the missing permissions, I do this in TAG:
with a different message if they're able to edit roles themselves (maybe not needed)
cc @roraback
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.
Oh that's wonderful. I'll do something like that!! great idea
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.
That popover looks great (and I agree we should show what permissions are missing), but it would be nice if TAG stuck to our design system so that the user's Teleport experience was consistent. We don't to my knowledge have anything in our design system that uses these colors in this way.
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'll make a note to update the colours (I think that grey is just a default I haven't changed)
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.
We currently have a pretty simple 'tooltip' component that won't let me get anywhere near styling like above. However, I've added the missing permissions to the text, as well as adding in a TODO to abstract some of this logic out to add to a new 'missing permissions tooltip' AND to refactor our current hover tooltip to allow much more grand components.
Thoughts on this as a placeholder until then? I'm also ok with "this feature doesn't need to go in right now so lets hold off on merging this until you create that new component and refactor"
681639f
to
0ae5758
Compare
0ae5758
to
66a20ff
Compare
This will only show the Edit and Reset options in the web UI if the viewing user has permissions to edit. This also hides the Create New User button because, although they can add a user, the action still fails as it tries to "reset" the user to get their first time login token. I thought about trying to decouple this but being able to create a user and not receive a link seems unproductive.
Delete is hidden if the user cannot delete, and the entire option box is removed if a users can't edit or delete
This does not affect the invite collaborators button/menu which emails the users their login info (as this is add only).
Closes #6885