Reader role: an administrator made on the user page or through POST /admin/users stays a reader after a demotion

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")))?;

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<Value> = req.send().await.unwrap().json().await.unwrap();
+            let mut roles: Vec<String> = 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<Value> = 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());
+}

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<Value> = req.send().await.unwrap().json().await.unwrap();
+            let mut roles: Vec<String> = 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<Value> = 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"]);
+}

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`.

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 })
         }
       }