rewrite ssh key logic #401

simonLeary42 · 2025-12-16T19:49:10Z

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)
ssh keys are deleted by index despite that LDAP doesn't not garuntee order

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 ssh-keygen-style public keys are 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
ssh keys are deleted by value

Screen.Recording.2025-12-18.at.12.40.14.PM.mov

future work:

display SSH key fingerprint in account.php #470

Copilot

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.

webroot/panel/account.php

resources/lib/UnityUser.php

webroot/panel/account.php

Copilot

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.

resources/lib/utils.php

webroot/panel/account.php

webroot/panel/modal/new_key.php

resources/lib/UnityUser.php

webroot/panel/account.php

resources/lib/utils.php

bryank-cs · 2025-12-31T14:55:07Z

resources/lib/UnityUser.php

-        $keys = array_values($keys);
-        $this->setSSHKeys($keys, $operator, $send_mail);
-        return $key;
+        $this->setSSHKeys($keys_after, $operator, $send_mail);


Should this only be called if the array actually changed?

I can't find what this was referencing but I don't think this is an issue anymore. Both addSSHKey and removeSSHKey have conditions to break out early before doing an LDAP operation.

resources/lib/UnityUser.php

simplify ssh key adding behavior restructure weird structure wip handle 0 github keys fix bug, return less information less code, better output, slower fix test include key in message shorten key before adding to message message for key removed log + message fix bug fix bug cant rely on error handler for testing add badRequest default case use short key in message consistent operator move logic inside UnityUser expect exception show more characters in shortened key sshkeyexists 1st draft dry multi word comment allow leading whitespace tweak regex, comment simpler regex, valueerror rewrite testValidSSHKey return array fix bug add allowed ssh key types config update test case remove responsibility from addSSHKey update account.php for new testValidSSHKey update ssh_validate for new testValidSSHKey handle ajax failure add tests fix bug fix bugs make error message less promising update test fix require in ajax close other session if exists fix bug only single space use SSHKeyExists() move function tweak comment display invalid key explanation in modal invalid encoding Update webroot/panel/account.php Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> remove unused import remove console.log add tests for shortenString add tests for removing ssh key comment don't log invalid keys update error handler delete key by value instead of by index fixup phpstan fix tests remove old arguments fix tests remove old args remove old arg remove old args

simonLeary42 requested a review from Copilot December 16, 2025 19:49

Copilot started reviewing on behalf of simonLeary42 December 16, 2025 19:49 View session

This comment was marked as outdated.

Sign in to view

simonLeary42 force-pushed the ssh-keys-all-or-none branch from fcc616a to 9402c0d Compare December 16, 2025 20:56

simonLeary42 changed the title ~~simplify ssh key adding behavior~~ rewrite ssh key adding Dec 16, 2025

simonLeary42 marked this pull request as draft December 16, 2025 20:57

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 changed the title ~~rewrite ssh key adding~~ rewrite ssh key logic Dec 17, 2025

simonLeary42 force-pushed the ssh-keys-all-or-none branch from 53e0bc1 to affd118 Compare December 17, 2025 02:44

simonLeary42 requested a review from Copilot December 17, 2025 14:03

Copilot started reviewing on behalf of simonLeary42 December 17, 2025 14:04 View session

Copilot AI reviewed Dec 17, 2025

View reviewed changes

simonLeary42 force-pushed the ssh-keys-all-or-none branch from 2238ad5 to fa2e42f Compare December 17, 2025 14:22

simonLeary42 marked this pull request as ready for review December 17, 2025 14:22

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 marked this pull request as draft December 17, 2025 20:59

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 marked this pull request as ready for review December 18, 2025 17:44

simonLeary42 requested a review from Copilot December 18, 2025 17:44

Copilot started reviewing on behalf of simonLeary42 December 18, 2025 17:44 View session

Copilot AI reviewed Dec 18, 2025

View reviewed changes

simonLeary42 requested a review from bryank-cs December 18, 2025 18:40

simonLeary42 force-pushed the ssh-keys-all-or-none branch from a93c25e to 8aee5ea Compare December 18, 2025 19:29

simonLeary42 mentioned this pull request Dec 19, 2025

add error handlers to ajax calls #419

Merged

simonLeary42 force-pushed the ssh-keys-all-or-none branch 5 times, most recently from 35d7f90 to 481a6e1 Compare December 31, 2025 14:24

bryank-cs reviewed Dec 31, 2025

View reviewed changes

simonLeary42 force-pushed the ssh-keys-all-or-none branch 3 times, most recently from e22a230 to fce31e3 Compare January 6, 2026 12:52

bryank-cs reviewed Jan 6, 2026

View reviewed changes

resources/lib/UnityUser.php Show resolved Hide resolved

bryank-cs previously approved these changes Jan 7, 2026

View reviewed changes

simonLeary42 added 4 commits January 7, 2026 09:31

private

76d4ae1

use tregx

bdf0ede

ensure index contiguity

c666bf4

simonLeary42 dismissed bryank-cs’s stale review via c666bf4 January 7, 2026 14:31

simonLeary42 force-pushed the ssh-keys-all-or-none branch from 1fd233a to c666bf4 Compare January 7, 2026 14:31

simonLeary42 merged commit 0bba8df into main Jan 7, 2026
3 checks passed

simonLeary42 deleted the ssh-keys-all-or-none branch January 7, 2026 14:32

rewrite ssh key logic #401

rewrite ssh key logic #401

Uh oh!

Conversation

simonLeary42 commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bryank-cs Dec 31, 2025

Choose a reason for hiding this comment

Uh oh!

simonLeary42 Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

simonLeary42 commented Dec 16, 2025 •

edited

Loading