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

View users with their key fingerprint who should be granted permissions #235

Merged
merged 8 commits into from
Nov 10, 2023

Conversation

SailReal
Copy link
Member

@SailReal SailReal commented Nov 6, 2023

image

@SailReal SailReal added this to the 1.3.0 milestone Nov 6, 2023
Copy link
Member

@tobihagemann tobihagemann left a comment

Choose a reason for hiding this comment

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

I'm totally fine with this PR. But we should add some way for vault owners to verify these fingerprints with the users if they choose to.

As discussed, a user should be able to check their fingerprint. I believe the user profile would make most sense?

@overheadhunter
Copy link
Member

I'm totally fine with this PR. But we should add some way for vault owners to verify these fingerprints with the users if they choose to.

See #190 and #191

As discussed, a user should be able to check their fingerprint. I believe the user profile would make most sense?

I guess this can be part of this PR.

@SailReal
Copy link
Member Author

SailReal commented Nov 7, 2023

User Profile
image

The first block matches the value displayed in the Grant Permission dialogue


It would be nice to be able to see the full fingerprint in the Grant Permission dialogue when hovering over it, but this does not look like it would be easy to implement.

@SailReal
Copy link
Member Author

SailReal commented Nov 7, 2023

With the power of @JaniruTEC we now show the full fingerprint on hover:
image

@JaniruTEC
Copy link
Contributor

JaniruTEC commented Nov 9, 2023

Since I am now officially part of this PR (😁), I have some minor ideas/suggestions:

  • Add a small fingerprint icon (maybe from Font Awesome?) next to the fingerprints in the "Grant Permission" dialog
  • Divide the full fingerprint shown on hover into groups similar to how it's done on the user profile page
  • Find some place to spell out the words "User Fingerprint"

@SailReal
Copy link
Member Author

SailReal commented Nov 9, 2023

This is just a very basic way to verify user fingerprint and will completely replaced by #191 in the next minor version so I would invest the time to enhance it into the replacing implementation.

Add a small fingerprint icon (maybe from Font Awesome?) next to the fingerprints in the "Grant Permission" dialog

I decided against it because there is not much place in this dialog especially because usernames often contains more characters like test1 if we have e.g. julian.raufelder@foo.bar.baz there isn't that much space left in this dialog.

Divide the full fingerprint shown on hover into groups similar to how it's done on the user profile page

This would require another variable that I wanted to avoid 😅

Find some place to spell out the words "User Fingerprint"

Where would you expect those words in this dialog?

@JaniruTEC
Copy link
Contributor

Where would you expect those words in this dialog?

I'd expect them probably on hover, either over the aforementioned icon or over the short fingerprint. Something like:

4242424242 with the hover:

baz' user fingerprint:
42424242424242424242424242424242424242

IMO many users will have no idea what those cryptic numbers mean -- unless they make the connection to their own user fingerprint, which is something I wouldn't count on.

@SailReal SailReal merged commit a215961 into develop Nov 10, 2023
5 checks passed
@SailReal SailReal deleted the feature/show-users-during-grant-permission branch November 10, 2023 10:38
@infeo
Copy link
Member

infeo commented Nov 10, 2023

Because of #190 and #191 we will rework this anyway. For now it is important, that the user is able to view and check the fingerprints in a way.

IMO many users will have no idea what those cryptic numbers mean -- unless they make the connection to their own user fingerprint, which is something I wouldn't count on.

We should mention it in the docs at least.

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

Successfully merging this pull request may close these issues.

None yet

5 participants