From 47baa0bcb1bf9a01c3511f3faad9600740134fd9 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Nov 2026 17:19:38 +0000 Subject: [PATCH] Reader role: an administrator made on the user page or through POST /admin/users stays a reader after a demotion An administrator made after the upgrade held `admin` alone: the user page's switch to Admin unassigned `reader`, and `POST /admin/users` with `"role": "admin"` assigned `admin` only. An OIDC or credential-forwarding realm that later took `admin` away (`sync_admin_role(false)`) left the account with no built-in role, unlike the administrators migration 0013 found, who keep `reader` (#792). - `insert_with_role` assigns `reader` next to `admin` for user accounts. A service account never holds `admin` and gets no `reader`. - The user page's Admin level is `admin` plus `reader`: switching to it assigns both and unassigns only writer; switching back to Reader takes `admin` alone. Fixes #812 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012e6DJXgdmBmbEhT2HcZAv5 --- crates/server/src/admin/users.rs | 17 +- crates/server/tests/auth_realms.rs | 150 ++++++++++++++++++ crates/server/tests/oidc_routes.rs | 85 ++++++++++ docs/changes/2.2.0/812.fixed.md | 7 + web/src/routes/admin/users/Edit.tsx | 34 ++-- web/src/routes/admin/users/userRoles.test.tsx | 48 +++++- 6 files changed, 323 insertions(+), 18 deletions(-) create mode 100644 docs/changes/2.2.0/812.fixed.md diff --git a/crates/server/src/admin/users.rs b/crates/server/src/admin/users.rs index d733be94..38d3c68f 100644 --- a/crates/server/src/admin/users.rs +++ b/crates/server/src/admin/users.rs @@ -22,7 +22,9 @@ //! realm without `auto_provision` names only the accounts created here). //! `role` gives the account its built-in role: `reader` (the default; reads //! every repository), `writer` or `admin`, or `roles_only` for none, so that -//! only the roles assigned to it later count. An account of a trusted-header +//! only the roles assigned to it later count. An `admin` holds `reader` too, +//! so that one whose identity provider or upstream takes `admin` away stays a +//! reader (#812). An account of a trusted-header //! realm is never an administrator (**400**, #547): its headers never make it //! one. //! @@ -693,7 +695,11 @@ pub(crate) fn trusted_header_admin_refused() -> Response { } /// Insert the user (with `email` as its address) and assign it `role`'s -/// built-in role in the same transaction. +/// built-in role in the same transaction. An administrator gets `reader` as +/// well (#812): an identity provider or upstream that later takes `admin` +/// away (`authz_dao::sync_admin_role`) removes that role alone, so they stay a +/// reader, as the administrators migration 0013 found do. A service account +/// never holds `admin` and gets no `reader` it was not asked for. async fn insert_with_role( state: &AppState, username: &str, @@ -728,7 +734,12 @@ async fn insert_with_role( .await? } }; - if let Some(name) = role.role_name() { + let also_reader = role == AccessRole::Admin && !matches!(account, NewAccount::Service { .. }); + let names = role + .role_name() + .into_iter() + .chain(also_reader.then_some(authz_dao::READER_ROLE)); + for name in names { let role_id = authz_dao::role_id_by_name(&mut *tx, name) .await? .ok_or_else(|| sqlx::Error::Protocol(format!("the built-in {name} role is missing")))?; diff --git a/crates/server/tests/auth_realms.rs b/crates/server/tests/auth_realms.rs index fd2ed578..365dd561 100644 --- a/crates/server/tests/auth_realms.rs +++ b/crates/server/tests/auth_realms.rs @@ -1565,3 +1565,153 @@ async fn kept_password_hashes_carry_a_realm_past_its_upstream() { assert_eq!(account.auth_source, "local"); } } + +/// #812: an administrator made through the admin API or the user page holds +/// `reader` next to `admin`, so that a credential-forwarding realm whose +/// upstream drops them from its `admin_group` leaves a reader, not an +/// account with no built-in role. A service account is never an admin and +/// stays roles only. +#[tokio::test] +async fn an_admin_demoted_by_the_upstream_stays_a_reader() { + use wiremock::matchers::{method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + // The upstream accepts every password and lists no admin group: every + // login it answers takes `admin` away. + let upstream = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/status")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ "groups": [] }))) + .mount(&upstream) + .await; + + // Its own database: an enabled forwarding realm would join the login + // chains of every other test sharing one. + let srv = common::start_with(common::fresh_pool().await, common::ServerOpts::default()).await; + let (admin, password) = srv.admin_basic(); + let client = srv.client(); + let resp = client + .post(format!("{}/admin/realms", srv.base_url)) + .basic_auth(&admin, Some(&password)) + .json(&json!({ + "kind": "credential_forwarding", + "name": "nexus", + "config": { + "url": format!("{}/status", upstream.uri()), + "key_pepper": "pepper", + "groups_path": "groups", + "admin_group": "nexus-admins", + } + })) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), 201, "{}", resp.text().await.unwrap()); + + let create = |body: Value| { + let req = client + .post(format!("{}/admin/users", srv.base_url)) + .basic_auth(&admin, Some(&password)) + .json(&body); + async move { + let resp = req.send().await.unwrap(); + let status = resp.status(); + let body: Value = resp.json().await.unwrap_or(Value::Null); + (status, body) + } + }; + let roles_of = |id: i64| { + let req = client + .get(format!("{}/admin/users/{id}/roles", srv.base_url)) + .basic_auth(&admin, Some(&password)); + async move { + let held: Vec = req.send().await.unwrap().json().await.unwrap(); + let mut roles: Vec = held + .iter() + .map(|r| r["name"].as_str().unwrap().to_string()) + .collect(); + roles.sort(); + roles + } + }; + let sign_in = |username: String| { + let req = client + .post(format!("{}/auth/login", srv.base_url)) + .json(&json!({ "username": username, "password": "upstream-pw" })); + async move { + let resp = req.send().await.unwrap(); + assert_eq!(resp.status(), 200, "{}", resp.text().await.unwrap()); + } + }; + + // Created with `"role": "admin"`: admin and reader. + let created = format!("created{}@example.com", common::unique()); + let (status, body) = + create(json!({ "username": created, "realm": "nexus", "role": "admin" })).await; + assert_eq!(status, 201, "{body}"); + let created_id = body["id"].as_i64().unwrap(); + assert_eq!(roles_of(created_id).await, ["admin", "reader"]); + + // Switched to Admin on the user page: a writer gets admin and reader + // (`PUT /admin/users/{id}/roles` for each), then loses writer. + let switched = format!("switched{}@example.com", common::unique()); + let (status, body) = + create(json!({ "username": switched, "realm": "nexus", "role": "writer" })).await; + assert_eq!(status, 201, "{body}"); + let switched_id = body["id"].as_i64().unwrap(); + let roles: Vec = client + .get(format!("{}/admin/roles", srv.base_url)) + .basic_auth(&admin, Some(&password)) + .send() + .await + .unwrap() + .json() + .await + .unwrap(); + let role_id = |name: &str| { + roles + .iter() + .find(|r| r["name"] == json!(name)) + .and_then(|r| r["id"].as_i64()) + .unwrap_or_else(|| panic!("no {name} role")) + }; + for name in ["admin", "reader"] { + let resp = client + .put(format!("{}/admin/users/{switched_id}/roles", srv.base_url)) + .basic_auth(&admin, Some(&password)) + .json(&json!({ "role_id": role_id(name) })) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), 204); + } + let resp = client + .delete(format!( + "{}/admin/users/{switched_id}/roles/{}", + srv.base_url, + role_id("writer") + )) + .basic_auth(&admin, Some(&password)) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), 204); + assert_eq!(roles_of(switched_id).await, ["admin", "reader"]); + + // The upstream no longer lists them in its admin group: the next login + // takes `admin` away and leaves a reader. + sign_in(created).await; + sign_in(switched).await; + assert_eq!(roles_of(created_id).await, ["reader"]); + assert_eq!(roles_of(switched_id).await, ["reader"]); + + // A service account is never an admin, and gets no reader it was not + // asked for. + let service = format!("ci{}@example.com", common::unique()); + let (status, _) = + create(json!({ "username": service, "kind": "service", "role": "admin" })).await; + assert_eq!(status, 400); + let (status, body) = create(json!({ "username": service, "kind": "service" })).await; + assert_eq!(status, 201, "{body}"); + assert!(roles_of(body["id"].as_i64().unwrap()).await.is_empty()); +} diff --git a/crates/server/tests/oidc_routes.rs b/crates/server/tests/oidc_routes.rs index 4cf5a11f..734df9b0 100644 --- a/crates/server/tests/oidc_routes.rs +++ b/crates/server/tests/oidc_routes.rs @@ -1395,3 +1395,88 @@ async fn idp_demotion_ends_sessions_opened_as_admin() { let body: Value = me(demoted).await.json().await.unwrap(); assert_eq!(body["is_admin"], json!(false)); } + +/// #812: a user an administrator switched to Admin on the user page holds +/// `reader` next to `admin`, so that the IdP taking `admin` away (the realm +/// maps `admin_groups`) leaves a reader, not an account with no built-in +/// role. +#[tokio::test] +async fn a_user_made_admin_on_the_user_page_stays_a_reader_after_an_idp_demotion() { + let _serial = SERIAL.lock().await; + let idp = MockIdp::start().await; + let srv = start_with( + common::pool().await, + ServerOpts { + bootstrap_admin: Some(( + format!("admin{}@example.com", common::unique()), + "adminpw".into(), + )), + oidc_block: Some(idp.oidc_block()), + ..Default::default() + }, + ) + .await; + let (admin, password) = srv.admin_basic(); + let client = srv.client(); + let n = common::unique(); + let (sub, username) = (format!("sub-{n}"), format!("promoted-{n}@example.com")); + let roles_of = |id: i64| { + let req = client + .get(format!("{}/admin/users/{id}/roles", srv.base_url)) + .basic_auth(&admin, Some(&password)); + async move { + let held: Vec = req.send().await.unwrap().json().await.unwrap(); + let mut roles: Vec = held + .iter() + .map(|r| r["name"].as_str().unwrap().to_string()) + .collect(); + roles.sort(); + roles + } + }; + + // The IdP creates a reader. + let (cookie, _) = login_and_get_cookie(&idp, &srv, &sub, &username, json!([])).await; + let me: Value = no_redirect_client() + .get(format!("{}/auth/me", srv.base_url)) + .header("cookie", format!("forge_session={cookie}")) + .send() + .await + .unwrap() + .json() + .await + .unwrap(); + let user_id = me["user_id"].as_i64().unwrap_or_else(|| panic!("{me}")); + assert_eq!(roles_of(user_id).await, ["reader"]); + + // Switched to Admin on the user page: it assigns `admin` and, holding + // `reader` already, unassigns nothing. + let roles: Vec = client + .get(format!("{}/admin/roles", srv.base_url)) + .basic_auth(&admin, Some(&password)) + .send() + .await + .unwrap() + .json() + .await + .unwrap(); + let admin_role = roles + .iter() + .find(|r| r["name"] == json!("admin")) + .and_then(|r| r["id"].as_i64()) + .expect("the admin role"); + let resp = client + .put(format!("{}/admin/users/{user_id}/roles", srv.base_url)) + .basic_auth(&admin, Some(&password)) + .json(&json!({ "role_id": admin_role })) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), 204); + assert_eq!(roles_of(user_id).await, ["admin", "reader"]); + + // The IdP does not list the admin group: the next login takes `admin` + // away and leaves a reader. + login_and_get_cookie(&idp, &srv, &sub, &username, json!([])).await; + assert_eq!(roles_of(user_id).await, ["reader"]); +} diff --git a/docs/changes/2.2.0/812.fixed.md b/docs/changes/2.2.0/812.fixed.md new file mode 100644 index 00000000..f7b96fd5 --- /dev/null +++ b/docs/changes/2.2.0/812.fixed.md @@ -0,0 +1,7 @@ +An administrator made under **Admin → Users → New user** or through +`POST /admin/users` with `"role": "admin"`, or switched to Admin on the +user's page, now holds the built-in `reader` role as well, as the +administrators the upgrade finds do. One whose identity provider or upstream +later takes `admin` away stays a reader, instead of being left with no +built-in role (Roles only) and losing all read access. Service accounts are +never administrators and are not given `reader`. diff --git a/web/src/routes/admin/users/Edit.tsx b/web/src/routes/admin/users/Edit.tsx index 1a1ad8ca..a1da3073 100644 --- a/web/src/routes/admin/users/Edit.tsx +++ b/web/src/routes/admin/users/Edit.tsx @@ -207,15 +207,26 @@ function UserPage() { ) } +/** + * The built-in roles a user at `level` holds: an admin holds `reader` next to + * `admin` (#812), so that an identity provider or upstream that takes `admin` + * away leaves a reader, as `POST /admin/users` with `role: admin` does. + */ +function levelRoles(level: AccessLevel): readonly string[] { + if (level === 'roles_only') return [] + if (level === 'admin') return [ACCESS_ROLES.admin, ACCESS_ROLES.reader] + return [ACCESS_ROLES[level]] +} + /** * The user's built-in access level: roles only (no built-in role), reader * (the `reader` role), writer (the `writer` role) or admin (the `admin` - * role), switched through `/admin/users/{id}/roles`. A switch assigns the new - * level's role first and then takes the others, so a refused step (the last - * active administrator keeps `admin`: 409, with the server's reason) never - * leaves the user with less access than before. A service account - * (`serviceAccount`) or an account of a trusted-header realm - * (`trustedHeader`) is never an admin. + * role, with `reader` kept next to it), switched through + * `/admin/users/{id}/roles`. A switch assigns the new level's roles first and + * then takes the others, so a refused step (the last active administrator + * keeps `admin`: 409, with the server's reason) never leaves the user with + * less access than before. A service account (`serviceAccount`) or an + * account of a trusted-header realm (`trustedHeader`) is never an admin. */ export function UserAccess({ userId, @@ -241,14 +252,15 @@ export function UserAccess({ setSaving(true) setError(null) try { - const keep = chosen === 'roles_only' ? null : ACCESS_ROLES[chosen] - if (keep) { - const role = (roles.data ?? []).find((r) => r.name === keep) - if (!role) throw new Error(`The built-in role ${keep} is missing.`) + const keep = levelRoles(chosen) + for (const name of keep) { + const role = (roles.data ?? []).find((r) => r.name === name) + if (!role) throw new Error(`The built-in role ${name} is missing.`) + if ((held.data ?? []).some((r) => r.id === role.id)) continue await assign.mutateAsync({ userId, roleId: role.id }) } for (const role of held.data ?? []) { - if (LEVEL_ROLES.includes(role.name) && role.name !== keep) { + if (LEVEL_ROLES.includes(role.name) && !keep.includes(role.name)) { await unassign.mutateAsync({ userId, roleId: role.id }) } } diff --git a/web/src/routes/admin/users/userRoles.test.tsx b/web/src/routes/admin/users/userRoles.test.tsx index fb71a47a..43e5b516 100644 --- a/web/src/routes/admin/users/userRoles.test.tsx +++ b/web/src/routes/admin/users/userRoles.test.tsx @@ -120,25 +120,36 @@ describe('admin user roles', () => { expect(within(form).getByRole('radio', { name: /^Writer/ })).toBeChecked(), ) - // Writer → admin: the admin role first, then writer goes. + // Writer → admin: the admin role and the reader role next to it first + // (#812: a demotion by the identity provider leaves a reader), then + // writer goes. await u.click(within(form).getByRole('radio', { name: /^Admin/ })) await u.click(within(form).getByRole('button', { name: 'Change access' })) await waitFor(() => expect(api.sent('DELETE', /^\/admin\/users\/2\/roles\/4$/)).toHaveLength(1), ) - expect(api.sent('PUT', /^\/admin\/users\/2\/roles$/)[1]?.body).toEqual({ role_id: 1 }) + expect(api.sent('PUT', /^\/admin\/users\/2\/roles$/).map((r) => r.body)).toEqual([ + { role_id: 4 }, + { role_id: 1 }, + { role_id: 3 }, + ]) expect(api.state.assignments).toContainEqual([2, 1]) + expect(api.state.assignments).toContainEqual([2, 3]) expect(api.state.assignments).not.toContainEqual([2, 4]) // The custom role stays. expect(api.state.assignments).toContainEqual([2, 7]) + await waitFor(() => + expect(within(form).getByRole('radio', { name: /^Admin/ })).toBeChecked(), + ) - // Admin → reader: the reader role first, then admin goes. + // Admin → reader: the reader role is held already, so only admin goes. await u.click(within(form).getByRole('radio', { name: /^Reader/ })) await u.click(within(form).getByRole('button', { name: 'Change access' })) await waitFor(() => expect(api.sent('DELETE', /^\/admin\/users\/2\/roles\/1$/)).toHaveLength(1), ) - expect(api.sent('PUT', /^\/admin\/users\/2\/roles$/)[2]?.body).toEqual({ role_id: 3 }) + expect(api.sent('PUT', /^\/admin\/users\/2\/roles$/)).toHaveLength(3) + expect(api.sent('DELETE', /^\/admin\/users\/2\/roles\/3$/)).toHaveLength(0) expect(api.state.assignments).toContainEqual([2, 3]) // Back to roles only takes the reader role and keeps the custom one. @@ -151,6 +162,35 @@ describe('admin user roles', () => { expect(api.state.assignments).toContainEqual([2, 7]) }) + it('keeps the reader role of a reader made an admin, and lists them as admin', async () => { + const u = userEvent.setup() + const router = await renderApp('/manage/users/6') + + const form = await screen.findByRole('form', { name: 'Change access' }) + await waitFor(() => + expect(within(form).getByRole('radio', { name: /^Reader/ })).toBeChecked(), + ) + await u.click(within(form).getByRole('radio', { name: /^Admin/ })) + await u.click(within(form).getByRole('button', { name: 'Change access' })) + await waitFor(() => + expect(within(form).getByRole('radio', { name: /^Admin/ })).toBeChecked(), + ) + expect(api.sent('PUT', /^\/admin\/users\/6\/roles$/).map((r) => r.body)).toEqual([ + { role_id: 1 }, + ]) + expect(api.sent('DELETE', /^\/admin\/users\/6\/roles\//)).toHaveLength(0) + expect(api.state.assignments).toContainEqual([6, 1]) + expect(api.state.assignments).toContainEqual([6, 3]) + + // The list shows the admin level alone: reader is not listed as another + // role. + await router.navigate({ to: '/manage/users' }) + const row = (await screen.findByRole('link', { name: 'bob' })).closest('tr')! + const cells = within(row).getAllByRole('cell') + await waitFor(() => expect(cells[2]).toHaveTextContent('admin')) + expect(cells[3]).toHaveTextContent('—') + }) + it('keeps the last administrator an admin, with the server’s reason', async () => { const u = userEvent.setup() await renderApp('/manage/users/1')