diff --git a/crates/db/src/user_dao.rs b/crates/db/src/user_dao.rs index 4de70e30..e3b894d6 100644 --- a/crates/db/src/user_dao.rs +++ b/crates/db/src/user_dao.rs @@ -399,7 +399,10 @@ pub async fn find_user_by_username(pool: &PgPool, username: &str) -> sqlx::Resul } /// Find a user by id. -pub async fn find_user_by_id(pool: &PgPool, id: i64) -> sqlx::Result> { +pub async fn find_user_by_id<'e, E>(executor: E, id: i64) -> sqlx::Result> +where + E: sqlx::PgExecutor<'e>, +{ sqlx::query_as!( AppUser, r#" @@ -411,7 +414,7 @@ pub async fn find_user_by_id(pool: &PgPool, id: i64) -> sqlx::Result) -> Result, Response> { let Some(description) = description.map(str::trim).filter(|d| !d.is_empty()) else { return Ok(None); @@ -242,9 +245,52 @@ fn parse_description(description: Option<&str>) -> Result, Respon ) .into_response()); } + if let Some(c) = description.chars().find(|&c| is_format_character(c)) { + return Err(( + StatusCode::BAD_REQUEST, + format!( + "description must not contain invisible formatting characters such as U+{:04X}", + c as u32 + ), + ) + .into_response()); + } Ok(Some(description.to_string())) } +/// A Unicode format character (general category `Cf`): the bidirectional +/// embeddings, overrides and isolates (U+202E reverses what follows it), +/// zero-width and other invisible characters, and tags. Not the zero-width +/// non-joiner and joiner (U+200C, U+200D), which emoji and several scripts +/// need. +fn is_format_character(c: char) -> bool { + matches!( + c as u32, + 0x00AD + | 0x0600..=0x0605 + | 0x061C + | 0x06DD + | 0x070F + | 0x0890..=0x0891 + | 0x08E2 + | 0x180E + | 0x200B + | 0x200E..=0x200F + | 0x202A..=0x202E + | 0x2060..=0x2064 + | 0x2066..=0x206F + | 0xFEFF + | 0xFFF9..=0xFFFB + | 0x110BD + | 0x110CD + | 0x13430..=0x1343F + | 0x1BCA0..=0x1BCA3 + | 0x1D173..=0x1D17A + | 0xE0001 + | 0xE0020..=0xE007F + ) +} + /// The id of the trusted-header realm named `name`, which a service account /// may belong to, or the **400** that refuses the name. async fn service_account_realm(state: &AppState, name: &str) -> Result { @@ -788,9 +834,10 @@ pub async fn revoke_invitation(State(state): State, Path(id): Path, Path(id): Path, @@ -804,28 +851,27 @@ pub async fn update( ) .into_response(); } - if body.description.is_some() || body.realm.is_some() { - if let Err(resp) = update_service_account(&state, id, &body).await { - return resp; + let changes = if body.description.is_some() || body.realm.is_some() { + match service_account_changes(&state, id, &body).await { + Ok(changes) => Some(changes), + Err(resp) => return resp, } - } - let Some(active) = body.active else { - forge_core::authz::cache::invalidate_user(&state.cache, id).await; - return match user_dao::find_user_by_id(&state.pool, id).await { - Ok(Some(user)) => match UserView::load(&state, &user).await { - Ok(view) => Json(view).into_response(), - Err(resp) => resp, - }, - Ok(None) => (StatusCode::NOT_FOUND, "user not found").into_response(), - Err(e) => db_error(e), - }; + } else { + None }; - let user = match set_active_guarded(&state, id, active).await { + let user = match apply_update(&state, id, changes.as_ref(), body.active).await { Ok(Some(u)) => u, Ok(None) => return (StatusCode::NOT_FOUND, "user not found").into_response(), Err(resp) => return resp, }; - if !active { + if changes.is_some() { + tracing::info!( + user_id = user.id, + username = %user.username, + "service account updated" + ); + } + if body.active == Some(false) { if let Err(resp) = revoke_sessions_and_verdicts(&state, &user).await { return resp; } @@ -843,14 +889,21 @@ pub async fn update( } } -/// Set the `description` and `realm` a PATCH carries on the service account -/// `id`, in one transaction: **404** for no account, **409** for a user's, -/// **400** for a description or realm that is refused. -async fn update_service_account( +/// The `description` and `realm` a PATCH sets on a service account, each +/// `None` when the request leaves it as it is. +struct ServiceAccountChanges { + description: Option>, + realm_id: Option>, +} + +/// The `description` and `realm` a PATCH carries for the service account +/// `id`, checked: **404** for no account, **409** for a user's, **400** for +/// a description or realm that is refused. +async fn service_account_changes( state: &AppState, id: i64, body: &UpdateUser, -) -> Result<(), Response> { +) -> Result { let user = match user_dao::find_user_by_id(&state.pool, id).await { Ok(Some(u)) => u, Ok(None) => return Err((StatusCode::NOT_FOUND, "user not found").into_response()), @@ -873,24 +926,54 @@ async fn update_service_account( Some(None) => Some(None), None => None, }; + Ok(ServiceAccountChanges { + description, + realm_id, + }) +} + +/// Apply a PATCH to the account `id` in one transaction: a service +/// account's `changes`, then `active` under the last-admin guard (only a +/// deactivation can remove an admin). `Ok(None)` when no such account +/// exists. +async fn apply_update( + state: &AppState, + id: i64, + changes: Option<&ServiceAccountChanges>, + active: Option, +) -> Result, Response> { let mut tx = forge_db::begin(&state.pool).await.map_err(db_error)?; - if let Some(description) = &description { - user_dao::set_service_account_description(&mut *tx, id, description.as_deref()) - .await - .map_err(db_error)?; + if let Some(changes) = changes { + if let Some(description) = &changes.description { + user_dao::set_service_account_description(&mut *tx, id, description.as_deref()) + .await + .map_err(db_error)?; + } + if let Some(realm_id) = changes.realm_id { + user_dao::set_service_account_realm(&mut *tx, id, realm_id) + .await + .map_err(db_error)?; + } } - if let Some(realm_id) = realm_id { - user_dao::set_service_account_realm(&mut *tx, id, realm_id) + let user = match active { + Some(active) => { + if !active + && authz_dao::would_leave_no_admin(&mut tx, id) + .await + .map_err(db_error)? + { + return Err(last_admin_conflict()); + } + user_dao::set_active(&mut *tx, id, active) + .await + .map_err(db_error)? + } + None => user_dao::find_user_by_id(&mut *tx, id) .await - .map_err(db_error)?; - } + .map_err(db_error)?, + }; tx.commit().await.map_err(db_error)?; - tracing::info!( - user_id = id, - username = %user.username, - "service account updated" - ); - Ok(()) + Ok(user) } /// `DELETE /admin/users/{id}` — delete the user (sessions, tokens, role @@ -1034,28 +1117,6 @@ pub(crate) async fn purge_converted( Ok(()) } -/// Set `active` in a transaction under the last-admin guard (only a -/// deactivation can remove an admin). `Ok(None)` when no such user exists. -async fn set_active_guarded( - state: &AppState, - id: i64, - active: bool, -) -> Result, Response> { - let mut tx = forge_db::begin(&state.pool).await.map_err(db_error)?; - if !active - && authz_dao::would_leave_no_admin(&mut tx, id) - .await - .map_err(db_error)? - { - return Err(last_admin_conflict()); - } - let user = user_dao::set_active(&mut *tx, id, active) - .await - .map_err(db_error)?; - tx.commit().await.map_err(db_error)?; - Ok(user) -} - /// Delete the user in a transaction under the last-admin guard. `Ok(false)` /// when no such user exists. async fn delete_guarded(state: &AppState, id: i64) -> Result { @@ -1094,3 +1155,48 @@ async fn revoke_sessions_and_verdicts(state: &AppState, user: &AppUser) -> Resul fn is_unique_violation(e: &sqlx::Error) -> bool { matches!(e, sqlx::Error::Database(db) if db.code().as_deref() == Some("23505")) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn descriptions_refuse_what_reads_otherwise_than_it_is() { + let parsed = |d: &str| parse_description(Some(d)).ok().flatten(); + assert_eq!( + parsed(" CI of the web team ").as_deref(), + Some("CI of the web team") + ); + assert_eq!( + parsed("owner:\n\tplatform").as_deref(), + Some("owner:\n\tplatform") + ); + // Emoji and scripts keep their joiners. + assert!(parsed("runners \u{1F468}\u{200D}\u{1F4BB}").is_some()); + assert!( + parsed("\u{0645}\u{06CC}\u{200C}\u{062E}\u{0648}\u{0627}\u{0647}\u{0645}").is_some() + ); + assert_eq!(parse_description(Some(" ")).ok(), Some(None)); + for c in [ + '\u{7}', + '\u{202E}', + '\u{202D}', + '\u{2066}', + '\u{2069}', + '\u{200E}', + '\u{200F}', + '\u{061C}', + '\u{200B}', + '\u{2060}', + '\u{FEFF}', + '\u{00AD}', + '\u{E0041}', + ] { + assert!( + parse_description(Some(&format!("build {c}agent"))).is_err(), + "U+{:04X}", + c as u32 + ); + } + } +} diff --git a/crates/server/tests/service_accounts.rs b/crates/server/tests/service_accounts.rs index 55d8c015..c052a1bf 100644 --- a/crates/server/tests/service_accounts.rs +++ b/crates/server/tests/service_accounts.rs @@ -300,6 +300,85 @@ async fn the_admin_api_creates_lists_and_updates_service_accounts() { assert_eq!(view["roles"], json!(["writer"])); } +/// A PATCH applies its fields in one transaction (#585): when deactivating +/// fails, the description and realm it carries are not applied either; when +/// it succeeds, the account is inactive in its new realm at once. A +/// description that would read otherwise than it is (a right-to-left +/// override, a zero-width space) is refused, on create and on update. +#[tokio::test] +async fn a_patch_applies_all_of_its_fields_or_none() { + let srv = server().await; + let realm = create_realm(&srv, false).await; + let name = format!("lb{}@example.com", common::unique()); + let id = create_service_account(&srv, &name, None).await["id"] + .as_i64() + .unwrap(); + + // The database refuses the deactivation, after the realm and the + // description were written in the same request. + sqlx::raw_sql(sqlx::AssertSqlSafe(format!( + "CREATE FUNCTION refuse_deactivation() RETURNS trigger LANGUAGE plpgsql AS $$ \ + BEGIN IF NEW.id = {id} AND NOT NEW.active THEN RAISE EXCEPTION 'refused'; END IF; \ + RETURN NEW; END $$; \ + CREATE TRIGGER refuse_deactivation BEFORE UPDATE ON app_user \ + FOR EACH ROW EXECUTE FUNCTION refuse_deactivation();" + ))) + .execute(&srv.pool) + .await + .unwrap(); + let (status, _) = patch( + &srv, + id, + json!({"active": false, "description": "the load balancer", "realm": realm.name}), + ) + .await; + assert_eq!(status, StatusCode::INTERNAL_SERVER_ERROR); + let view = |users: Vec| users.into_iter().find(|u| u["id"] == json!(id)).unwrap(); + let account = view(list(&srv, "?kind=service").await); + assert_eq!(account["active"], json!(true), "{account}"); + assert_eq!(account["realm"], Value::Null, "{account}"); + assert_eq!( + account["description"], + json!("The build agents; owner: platform"), + "{account}" + ); + sqlx::raw_sql( + "DROP TRIGGER refuse_deactivation ON app_user; DROP FUNCTION refuse_deactivation();", + ) + .execute(&srv.pool) + .await + .unwrap(); + + let (status, body) = patch( + &srv, + id, + json!({"active": false, "description": "the load balancer", "realm": realm.name}), + ) + .await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(body["active"], json!(false)); + assert_eq!(body["realm"], json!(realm.name)); + assert_eq!(body["description"], json!("the load balancer")); + + for description in ["admin \u{202E}lanigiro", "build\u{200B}agents"] { + let (status, body) = patch(&srv, id, json!({"description": description})).await; + assert_eq!(status, StatusCode::BAD_REQUEST, "{body}"); + assert!(body.as_str().unwrap().contains("formatting"), "{body}"); + let (status, body) = create( + &srv, + json!({ + "username": format!("svc{}@example.com", common::unique()), + "kind": "service", + "description": description, + }), + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST, "{body}"); + } + let account = view(list(&srv, "?kind=service").await); + assert_eq!(account["description"], json!("the load balancer")); +} + #[tokio::test] async fn nobody_signs_in_to_a_service_account_but_its_tokens_work() { let srv = server().await; diff --git a/docs/changes/2.2.0/585.fixed.md b/docs/changes/2.2.0/585.fixed.md new file mode 100644 index 00000000..913006de --- /dev/null +++ b/docs/changes/2.2.0/585.fixed.md @@ -0,0 +1,11 @@ +`PATCH /admin/users/{id}` applied a service account's `description` and +`realm` in one transaction and `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. All of a request's +fields are now applied in one transaction, or none. The service account +page sent the realm with every save, so once the realm had been renamed or +deleted since the page loaded, saving a new description alone failed with +400; it now sends only what was changed. A description is also refused +when it holds an invisible formatting character, such as a right-to-left +override (U+202E) that made it read otherwise in the admin list than it +is; zero-width joiners, which emoji and some scripts need, stay allowed. diff --git a/docs/deployment.md b/docs/deployment.md index e3861109..cb4c0a51 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -507,8 +507,12 @@ or `writer`), and the optional `realm` (the name of a trusted-header realm). `GET /admin/users?kind=service` lists the service accounts; every user view carries `kind`, `description`, `realm` and `last_used_at`. `PATCH /admin/users/{id}` sets a service account's `description` and `realm` -(`null` clears either) besides `active`. Tokens are -`POST /admin/users/{id}/tokens`, roles `PUT /admin/users/{id}/roles`. +(`null` clears either) besides `active`, all of them in one transaction. +A description holds at most 1,000 characters, and no control character but +a newline or a tab, nor an invisible formatting character such as a +right-to-left override (U+202E), with which it would read otherwise than it +is. Tokens are `POST /admin/users/{id}/tokens`, roles `PUT +/admin/users/{id}/roles`. ### Usernames diff --git a/web/src/routes/admin/service-accounts/Edit.tsx b/web/src/routes/admin/service-accounts/Edit.tsx index b7d710f6..1f722a90 100644 --- a/web/src/routes/admin/service-accounts/Edit.tsx +++ b/web/src/routes/admin/service-accounts/Edit.tsx @@ -102,7 +102,7 @@ function ServiceAccountPage() { /** * The account's description and trusted-header realm (`PATCH - * /admin/users/{id}`), and its state: deactivating stops its tokens and its + * /admin/users/{id}`, with the fields that changed), and its state: deactivating stops its tokens and its * realm from authenticating it (the tokens are suspended, not revoked); * reactivating lets both work again. Each after a confirmation. */ @@ -116,18 +116,21 @@ function ServiceAccountSettings({ account }: { account: UserView }) { const [realm, setRealm] = useState(account.realm ?? '') const [confirming, setConfirming] = useState<'deactivate' | 'reactivate' | null>(null) const [done, setDone] = useState(null) - const changed = - description.trim() !== (account.description ?? '') || realm !== (account.realm ?? '') + const descriptionChanged = description.trim() !== (account.description ?? '') + const realmChanged = realm !== (account.realm ?? '') + const changed = descriptionChanged || realmChanged const liveTokens = tokens.data?.filter((t) => tokenStatus(t) === 'active').length + // Only what was changed is sent: a realm renamed or deleted since the page + // loaded would refuse the save of a new description. function save(e: React.FormEvent) { e.preventDefault() setDone(null) update.mutate( { id: account.id, - description: description.trim() || null, - realm: realm || null, + ...(descriptionChanged && { description: description.trim() || null }), + ...(realmChanged && { realm: realm || null }), }, { onSuccess: () => setDone(`Saved ${account.username}.`) }, ) diff --git a/web/src/routes/admin/service-accounts/serviceAccounts.test.tsx b/web/src/routes/admin/service-accounts/serviceAccounts.test.tsx index b67b8751..bf197539 100644 --- a/web/src/routes/admin/service-accounts/serviceAccounts.test.tsx +++ b/web/src/routes/admin/service-accounts/serviceAccounts.test.tsx @@ -14,7 +14,8 @@ import { renderApp } from '~/test/renderApp' * them out; * - the create page POSTs `kind: 'service'` with the description, realm * and access level, offers no admin level and takes no email; - * - the account page saves its description and realm, never offers the + * - the account page saves what changed of its description and realm (a + * realm deleted since is not sent again), never offers the * admin role or level, issues a token whose secret shows once, and * deactivates it after a confirmation. */ @@ -159,14 +160,11 @@ describe('admin service accounts', () => { expect(await within(settings).findByText('Saved ci@example.com.')).toBeInTheDocument() await waitFor(() => expect(screen.getByText(/^Realm:/)).toHaveTextContent('edge-lb')) - // Back to no realm: the realm is sent as null. + // Back to no realm: the realm alone is sent, as null. await u.selectOptions(within(settings).getByLabelText('Trusted-header realm'), '') await u.click(within(settings).getByRole('button', { name: 'Save' })) await waitFor(() => - expect(api.sent('PATCH', /^\/admin\/users\/4$/)[1]?.body).toEqual({ - description: 'CI of the web team', - realm: null, - }), + expect(api.sent('PATCH', /^\/admin\/users\/4$/)[1]?.body).toEqual({ realm: null }), ) // The access level has no admin, and neither do the roles to assign. @@ -182,6 +180,25 @@ describe('admin service accounts', () => { expect(within(assign).queryByRole('option', { name: 'admin' })).toBeNull() }) + it('saves a description alone once its realm is gone, without re-sending the realm', async () => { + // The account's realm was deleted (or renamed) since it was assigned. + api.state.realms = [] + const u = userEvent.setup() + await renderApp('/manage/service-accounts/3') + + const settings = await screen.findByRole('form', { name: 'Service account' }) + const field = within(settings).getByLabelText(/Description/) + await u.clear(field) + await u.type(field, 'The office network') + await u.click(within(settings).getByRole('button', { name: 'Save' })) + await waitFor(() => + expect(api.sent('PATCH', /^\/admin\/users\/3$/)[0]?.body).toEqual({ + description: 'The office network', + }), + ) + expect(await within(settings).findByText('Saved office-lan@example.com.')).toBeInTheDocument() + }) + it('issues a token for a service account, showing the secret once', async () => { const u = userEvent.setup() await renderApp('/manage/service-accounts/4') diff --git a/web/src/test/fakeRbacApi.ts b/web/src/test/fakeRbacApi.ts index 4ee42c78..b8dcf4ab 100644 --- a/web/src/test/fakeRbacApi.ts +++ b/web/src/test/fakeRbacApi.ts @@ -367,6 +367,14 @@ export class FakeRbacApi { u.active = body.active u.status = body.active ? 'active' : 'inactive' } + if ( + typeof body.realm === 'string' && + !(s.realms as { kind?: string; name?: string }[]).some( + (r) => r.kind === 'trusted_header' && r.name === body.realm, + ) + ) { + return text(`no trusted-header realm is named "${body.realm}"`, 400) + } if (body.description !== undefined) u.description = body.description if (body.realm !== undefined) u.realm = body.realm return json(this.userView(u))