feat: SSH UI#1033
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
dcoric
left a comment
dcoric
left a comment
There was a problem hiding this comment.
Review Note:
The implementation looks good overall, but we need to address the SSH port handling in clone URLs.
Requested Changes:
- Currently when a non-standard SSH port is configured, the clone URL format needs to be adjusted to:
git clone ssh://git@${host}:${SSHport}/${path}
dcoric
left a comment
dcoric
left a comment
There was a problem hiding this comment.
Looks good now! This is a bit larger PR since it's built on top of the SSH functionality PR. I've reviewed the UI portion (since the SSH changes are in the other PR).
One suggestion: it might make more sense to target this PR against the original SSH branch rather than main. That way, both features could be reviewed and merged together as a complete feature set. What do you think?
jescalada
left a comment
jescalada
left a comment
There was a problem hiding this comment.
Looks good! I tested the various SSH flows and they work great. 👍🏼
I suggested a few UX and validation improvements. Let me know if anything doesn't make sense! Chances are my errors are due to some setup/environment problem.
| axios | ||
| .get(`${API_BASE}/api/v1/config/ssh`, { withCredentials: true }) | ||
| .then((res) => { | ||
| const { enabled = true, port = 22 } = res.data || {}; |
There was a problem hiding this comment.
Is it necessary to add default values here if the API already sets the same default values (/src/config/index.ts)? 🤔 (Maybe it does help in the rare scenario someone pulls these changes without reloading their backend, since the frontend gets updated dynamically).
| <Grid container alignItems='center' justifyContent='space-between'> | ||
| <Grid item xs> | ||
| <Typography variant='body2' color='textSecondary'> | ||
| {key.hash} |
There was a problem hiding this comment.
Might be good to add a title/text to show that the "hash" here is just the SHA-256 fingerprint rather than the actual key.
I feel users would still have a pretty tough time identifying/managing keys, since right now they only add the key itself (which gets hashed). I'm not too knowledgeable on the topic, so I used AI to generate some recommendations:
The SHA-256 fingerprint can be useful, but by itself, it’s not always helpful from a user experience standpoint, especially when you’re trying to manage or recognize SSH keys at a glance.
The fingerprint (like SHA256:abcd...) is a cryptographic hash of the actual public key. It's primarily used to verify that a public key you received (or stored) is the same one the server has, without exposing the key directly. So yes, it is good from a security perspective, because it allows identification without revealing the key... but! It’s not very user-friendly.
Users often need more context when managing SSH keys. Just a name and a fingerprint is like labeling keys in your drawer with barcodes... sure, it's unique, but what does it mean?
More helpful identifiers could include:
Comment field: Most SSH keys are generated with a comment at the end (like user@host). This is often included in the .pub file and can be a great visual hint. You can extract and show it if available.
Key type and length: Something like ed25519 256-bit or rsa 4096-bit helps users understand what kind of key they’re dealing with. This also helps flag weak keys.
Creation time or added date: Useful when people rotate keys but keep old ones for comparison.
Partial public key: Showing the first ~10 and last ~10 characters of the base64-encoded public key (like GitHub does) can give a recognizable pattern without revealing the whole key.
Associated user/device name (if provided at registration): E.g. “MacBook Pro key”, “CI deploy key”, etc. This is probably what the "name" field in your PR is meant to be, right? Just make sure it’s user-set and not auto-generated.
I guess we could add a few extra optional fields to help UX for key management (even if key details are not actually used internally).
Signed-off-by: Fabio Vincenzi <93596376+fabiovincenzi@users.noreply.github.com>
4 participants
This PR adds an SSH-key management UI. Based on #987
To ease review, a separate PR has been opened against the SSH integration branch to highlight only the UI-related changes: G-Research#52
Frontend
Backend – Two endpoints make it work:
GET /:username/ssh-keys→ returns all fingerprints for the user.DELETE /:username/ssh-keys/fingerprint→ accepts { fingerprint } in the body and removes the matching key.