Repository navigation
feat: separate deactivating a user from deleting them, and manage administrators - #19
Merged
Merged
Conversation
…inistrators Closes #18. `DELETE /scim2/Users/{id}` cleared every grant the principal held and then only marked the account DISABLED: advertised as deletion, behaving as deactivation, and destroying the grants on the way. The row survived, so the email stayed taken and could never be registered again. The two are now separate operations: - Deactivate leaves grants and owned objects untouched, so reactivating gives the account back as it was. Access stops on the next request, since the state is re-read per request. - Purge is the irreversible one, and the only thing that frees the email. It requires the principal typed back, refuses an account that is still active, refuses your own, and requires an heir when the user owns securables -- those objects change hands. Metastore administrators can be appointed and stood down from the Users page, under the invariant that at least one *enabled* administrator always remains. Fixed along the way: - `PATCH /scim2/Users/{id}` carried no authorization annotation (from upstream #500), and the access decorator lets a method without one through, so any signed-in account could deactivate any other -- an administrator included. It now requires metastore OWNER. - The administrator password sign-in never consulted the user table: a deactivated `admin` still received a session, whose every request was then refused. It now checks that the account exists and is enabled, and says which is wrong. - The bootstrap administrator could be deactivated by three separate routes, and stripped of its administrator status, either of which closes the password sign-in -- the way back in when the identity provider is unavailable, and something no restart repairs. All of them are refused. - The PatchOp schema typed `value` as a boolean, so the UI sent a scalar where RFC 7644 3.5.2.1 requires an object; the SCIM library rejected it while parsing and the user saw a JSON parse failure instead of a deactivated account. Both forms the RFC allows are now accepted. - Test servers raced for their port: it was picked, released, and only bound once Hibernate was up, failing whole test classes with `Address already in use` on a loaded runner. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `purge` / `reassign_to` / `confirm_principal` parameters changed `UsersApi.deleteUser`, and the CLI still called the one-argument form, so the `cli` module did not compile. Passing nulls keeps what `uc user delete` has always done -- deactivate. It went unnoticed because the GitLab pipeline builds and tests `server` alone; GitHub builds every module, which is where it surfaced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The port retry added alongside this series builds a new server and binds it, but a start that failed partway was left as it was. Anything it had already bound would keep listening, and keep answering requests meant for the server the test actually ends up using. Stop it before retrying. Nothing in CI has taken this path yet -- the retry has not fired since it landed -- which is the point: it should not be the first thing to go wrong when it does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #18.
What this does
Purge is guarded in the service rather than trusted to the caller: the principal has to be typed
back (the user list mixes people with instance principals, where the wrong row takes a whole
compute instance offline), the account must already be deactivated, you cannot purge your own or
the bootstrap administrator, and a user who owns securables cannot be purged without naming an
heir — those objects change hands as part of the operation.
Fixed along the way
PATCH /scim2/Users/{id}carried no authorization annotation, andUnityAccessDecoratorlets a method without one through — so any signed-in account could deactivate any other, an administrator includedadminstill got a session whose every request was then refused — a sign-in that looked like it worked and did nothingvalueas a boolean, so the UI sent a scalar where RFC 7644 §3.5.2.1 requires an object; the SCIM library rejected it while parsing and the user saw a JSON parse failure instead of a deactivated accountAddress already in useon a loaded runnerThe last one is why
test (Spark 4.1.0)went red onmainright after #17 merged:AuthServiceIdentityResolutionTestfailed 1 of 224 insetUp, for a reason that had nothing to dowith that class.
Tests
server231 → 252,ui100 → 109. New:Scim2UserPatchTest,Scim2UserPurgeTest,MetastoreAdminsTest,BootstrapAdminProtectionTest,users.test.tsx,UsersList.test.tsx, plusadditions to
AuthServiceTokenLoginTestandUserActionsDropdown.test.tsx.Every fix here has a test that was verified in reverse: reverting the fix makes the corresponding
case fail.