Skip to content

Conversation

@simonLeary42
Copy link
Collaborator

@simonLeary42 simonLeary42 commented Dec 16, 2025

before:

  • duplicate and invalid keys are silently ignored
  • when github API returns no keys, nothing happens and the user is left scratching their head
  • private keys are accepted and written to world-readable LDAP
  • lots of strange key types and key formats are accepted (ex: DSA, JWT)
  • when writing multiple keys, there is only one LDAP write (fast)

after:

  • added message when key is added
  • added message when key is removed
  • added message when duplicate key is skipped
  • added message when key is invalid
  • added message when github API returns empty array
  • only configured [ldap][allowed_ssh_public_key_types] key types are accepted
  • only authorized_keys-formatted keys are accepted
  • private keys are not accepted
  • when adding multiple keys, there is one LDAP write per key (slow)
    • this happens only when importing from github and even then it should only typically be a small number of keys
image
Screen.Recording.2025-12-18.at.12.40.14.PM.mov

TODO:

  • decide what to do with currently unused UnityUser->sshKeyExists() function
  • enforce phpseclib-compatible authorized_keys-style public key format with regex
  • switch from load to loadPublicKey
  • display key invalid message in modal for paste key

This comment was marked as outdated.

@simonLeary42 simonLeary42 changed the title simplify ssh key adding behavior rewrite ssh key adding Dec 16, 2025
@simonLeary42 simonLeary42 marked this pull request as draft December 16, 2025 20:57
@simonLeary42 simonLeary42 force-pushed the ssh-keys-all-or-none branch 3 times, most recently from 12d2314 to feb793b Compare December 17, 2025 02:14
@simonLeary42 simonLeary42 changed the title rewrite ssh key adding rewrite ssh key logic Dec 17, 2025
@simonLeary42 simonLeary42 requested a review from Copilot December 17, 2025 14:03
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@simonLeary42 simonLeary42 marked this pull request as ready for review December 17, 2025 14:22
@simonLeary42 simonLeary42 force-pushed the ssh-keys-all-or-none branch 2 times, most recently from 537bad1 to 18b1f54 Compare December 17, 2025 20:59
@simonLeary42 simonLeary42 marked this pull request as draft December 17, 2025 20:59
@simonLeary42 simonLeary42 force-pushed the ssh-keys-all-or-none branch 4 times, most recently from 32ebd66 to 2e52486 Compare December 18, 2025 16:03
@simonLeary42 simonLeary42 marked this pull request as ready for review December 18, 2025 17:44
@simonLeary42 simonLeary42 requested a review from Copilot December 18, 2025 17:44
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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.

2 participants