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

Conditionally render User edit/delete actions in the web UI #49645

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

avatus
Copy link
Contributor

@avatus avatus commented Dec 2, 2024

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).
Screenshot 2024-12-02 at 10 18 14 AM
Screenshot 2024-12-02 at 10 18 17 AM
Screenshot 2024-12-02 at 10 36 22 AM

Closes #6885

@avatus avatus changed the title Conditionally render User edit actions in the web UI Conditionally render User edit/delete actions in the web UI Dec 2, 2024
tipContent={
canEditUsers
? ''
: 'You do not have permission to edit users.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be

Suggested change
: 'You do not have permission to edit users.'
: 'You do not have permission to create users.'

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Contributor

@ryanclark ryanclark Dec 2, 2024

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:

image

image

with a different message if they're able to edit roles themselves (maybe not needed)

image

cc @roraback

Copy link
Contributor Author

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

Copy link
Collaborator

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.

Copy link
Contributor

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)

Copy link
Contributor Author

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.

Screenshot 2024-12-02 at 12 07 20 PM

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"

@avatus avatus force-pushed the avatus/hide_edit branch 2 times, most recently from 681639f to 0ae5758 Compare December 3, 2024 01:09
@avatus avatus requested a review from ryanclark December 3, 2024 14:26
@avatus avatus enabled auto-merge December 3, 2024 14:52
@avatus avatus added the no-changelog Indicates that a PR does not require a changelog entry label Dec 3, 2024
@avatus avatus added this pull request to the merge queue Dec 3, 2024
Merged via the queue into master with commit f91c7d2 Dec 3, 2024
40 of 42 checks passed
@avatus avatus deleted the avatus/hide_edit branch December 3, 2024 16:24
@public-teleport-github-review-bot

@avatus See the table below for backport results.

Branch Result
branch/v16 Failed
branch/v17 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Users Options box still present when no privileges
3 participants