Repository navigation
Admin users: a PATCH applies all of its fields or none; descriptions refuse bidi overrides - #701
Merged
Merged
Conversation
`PATCH /admin/users/{id}` committed a service account's `description`
and `realm` in one transaction and then applied `active` in another:
`{"active": false, "realm": "lb"}` left the account active in its new
realm for a moment, and a failure in the second step left the first
applied. The request is now checked first (the account is a service
account, the description and realm are accepted) and then applied in
one transaction: the description, the realm, and `active` under the
last-admin guard. `find_user_by_id` takes any executor, so the
unchanged account is read in that transaction too. The test makes the
deactivation fail in the database and checks that nothing else was
applied (on the old code the account kept its new realm).
The service account page sent `realm` with every save, so once the
realm was renamed or deleted after the page loaded, saving a new
description alone failed with 400. It now sends only the fields that
changed; the fake admin API of the web tests refuses an unknown realm
like the server.
`parse_description` refused control characters (Cc) but accepted
format characters (Cf) such as U+202E, which made a description read
otherwise in the admin list than it is. It now refuses them too, all
but the zero-width non-joiner and joiner that emoji and several scripts
need.
Fixes #585
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012e6DJZaXD2mBH9tURsdutM
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.
Fixes #585
The three defects of the user and service-account administration:
PATCH /admin/users/{id}in separate transactionsupdatecommitteddescriptionandrealm(update_service_account), then appliedactivein a second transaction (set_active_guarded). Now the request is checked first (service_account_changes: the account exists and is a service account, the description parses, the realm is a trusted-header realm) and then applied in one transaction (apply_update): description, realm, thenactiveunder the last-admin guard. Whenactiveis absent the account is read in that transaction too, which is whyuser_dao::find_user_by_idnow takes any executor (every caller passes a pool, so nothing else changes). The side effects of a deactivation (sessions, verdicts, invitation) still run after the commit, as before.Test (
service_accounts.rs): a test-only trigger makes the deactivation fail in the database.{"active": false, "description": …, "realm": …}then answers 500 and the account keeps its description and no realm; without the trigger all three apply at once. Onmainthe test fails with the account still active in the new realm.The service account page re-sent the realm
ServiceAccountSettings.savenow sends only the fields that changed, so saving a description after the account's realm was renamed or deleted no longer fails with 400. The web tests' fake admin API now refuses an unknown realm like the server does; a new test saves a description with the realm gone (it fails on the old page), and the existing one now expects{realm: null}alone for a realm change.Descriptions accepted format characters
parse_descriptionrefused Cc but accepted Cf, such as U+202E. It now also refuses Unicode format characters (is_format_character: bidi embeddings, overrides and isolates, zero-width and other invisible characters, tags), except U+200C/U+200D, which emoji and several scripts need. The 400 names the code point. This applies on create and update;docs/deployment.mdstates the rule.Checks run
cargo test -p forge-server --test service_accounts --test admin(4 + 45), the new unit testadmin::users::testspnpm run tsc,pnpm exec vitest run(523 passed)cargo clippy --workspace --all-targets -- -D warnings,cargo fmt --all --check,cargo xtask changelog --checkChangelog:
docs/changes/2.2.0/585.fixed.md.Generated by Claude Code