From 1ed056faeccd85d3a6123f0103d8b8ba0f76f491 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Leite?= Date: Wed, 29 Jul 2026 20:33:45 -0300 Subject: [PATCH 1/2] feat: scope Editor group membership by group (#990 slice 3) --- .../ruscker-admin/assets/i18n/en/landing.ftl | 2 + .../ruscker-admin/assets/i18n/es/landing.ftl | 2 + .../ruscker-admin/assets/i18n/fr/landing.ftl | 2 + .../ruscker-admin/assets/i18n/pt/landing.ftl | 2 + crates/ruscker-admin/src/auth.rs | 36 +- crates/ruscker-admin/src/routes/admin.rs | 2 + .../ruscker-admin/src/routes/admin/groups.rs | 252 ++++++++++--- crates/ruscker-admin/src/scope.rs | 13 + .../ruscker-admin/templates/admin/groups.html | 14 +- crates/ruscker-admin/tests/rbac.rs | 338 ++++++++++++++++++ 10 files changed, 586 insertions(+), 77 deletions(-) diff --git a/crates/ruscker-admin/assets/i18n/en/landing.ftl b/crates/ruscker-admin/assets/i18n/en/landing.ftl index bb5d831..9d4c8a6 100644 --- a/crates/ruscker-admin/assets/i18n/en/landing.ftl +++ b/crates/ruscker-admin/assets/i18n/en/landing.ftl @@ -902,6 +902,7 @@ admin-groups-kpi-total = Groups admin-groups-kpi-members = Unique members admin-groups-kpi-apps = Apps covered admin-groups-subtitle = Create and edit the groups derived from apps and users. +admin-groups-editor-subtitle = Add or remove members in your groups. Creating, renaming, and deleting groups requires an Admin. admin-groups-members = Members admin-groups-apps = Apps admin-groups-public-title = Public apps @@ -917,6 +918,7 @@ admin-groups-pick-user = Choose a user… admin-groups-create = Create group admin-groups-new-name = Group name admin-groups-new-group-title = New group: +admin-groups-flash-created = Group created. admin-groups-flash-renamed = Group renamed. admin-groups-flash-deleted = Group deleted. admin-groups-flash-member-added = Member added. diff --git a/crates/ruscker-admin/assets/i18n/es/landing.ftl b/crates/ruscker-admin/assets/i18n/es/landing.ftl index ee69e3b..0a0bb2e 100644 --- a/crates/ruscker-admin/assets/i18n/es/landing.ftl +++ b/crates/ruscker-admin/assets/i18n/es/landing.ftl @@ -902,6 +902,7 @@ admin-groups-kpi-total = Grupos admin-groups-kpi-members = Miembros únicos admin-groups-kpi-apps = Apps cubiertas admin-groups-subtitle = Creación y edición de los grupos derivados de los apps y usuarios. +admin-groups-editor-subtitle = Añade o quita miembros de tus grupos. Crear, renombrar y eliminar grupos requiere un Admin. admin-groups-members = Miembros admin-groups-apps = Apps admin-groups-public-title = Apps públicas @@ -917,6 +918,7 @@ admin-groups-pick-user = Elegir usuario… admin-groups-create = Crear grupo admin-groups-new-name = Nombre del grupo admin-groups-new-group-title = Nuevo grupo: +admin-groups-flash-created = Grupo creado. admin-groups-flash-renamed = Grupo renombrado. admin-groups-flash-deleted = Grupo eliminado. admin-groups-flash-member-added = Miembro añadido. diff --git a/crates/ruscker-admin/assets/i18n/fr/landing.ftl b/crates/ruscker-admin/assets/i18n/fr/landing.ftl index a135703..c89f047 100644 --- a/crates/ruscker-admin/assets/i18n/fr/landing.ftl +++ b/crates/ruscker-admin/assets/i18n/fr/landing.ftl @@ -902,6 +902,7 @@ admin-groups-kpi-total = Groupes admin-groups-kpi-members = Membres uniques admin-groups-kpi-apps = Applications couvertes admin-groups-subtitle = Création et édition des groupes dérivés des apps et des utilisateurs. +admin-groups-editor-subtitle = Ajoutez ou retirez des membres de vos groupes. La création, le renommage et la suppression nécessitent un Admin. admin-groups-members = Membres admin-groups-apps = Apps admin-groups-public-title = Apps publiques @@ -917,6 +918,7 @@ admin-groups-pick-user = Choisir un utilisateur… admin-groups-create = Créer un groupe admin-groups-new-name = Nom du groupe admin-groups-new-group-title = Nouveau groupe : +admin-groups-flash-created = Groupe créé. admin-groups-flash-renamed = Groupe renommé. admin-groups-flash-deleted = Groupe supprimé. admin-groups-flash-member-added = Membre ajouté. diff --git a/crates/ruscker-admin/assets/i18n/pt/landing.ftl b/crates/ruscker-admin/assets/i18n/pt/landing.ftl index 045c86c..3e632e9 100644 --- a/crates/ruscker-admin/assets/i18n/pt/landing.ftl +++ b/crates/ruscker-admin/assets/i18n/pt/landing.ftl @@ -906,6 +906,7 @@ admin-groups-kpi-total = Grupos admin-groups-kpi-members = Membros únicos admin-groups-kpi-apps = Apps cobertas admin-groups-subtitle = Criação e edição dos grupos derivados dos apps e usuários. +admin-groups-editor-subtitle = Adicione ou remova membros dos seus grupos. Criar, renomear e excluir grupos exige um Admin. admin-groups-members = Membros admin-groups-apps = Apps admin-groups-public-title = Apps públicos @@ -921,6 +922,7 @@ admin-groups-pick-user = Escolher usuário… admin-groups-create = Criar grupo admin-groups-new-name = Nome do grupo admin-groups-new-group-title = Novo grupo: +admin-groups-flash-created = Grupo criado. admin-groups-flash-renamed = Grupo renomeado. admin-groups-flash-deleted = Grupo excluído. admin-groups-flash-member-added = Membro adicionado. diff --git a/crates/ruscker-admin/src/auth.rs b/crates/ruscker-admin/src/auth.rs index a9ef494..dbc7ee2 100644 --- a/crates/ruscker-admin/src/auth.rs +++ b/crates/ruscker-admin/src/auth.rs @@ -60,17 +60,17 @@ pub enum Role { impl Role { /// Whether this role may reach the given admin nav section. /// `section` matches the `nav_section` strings used by the - /// templates (`dashboard`, `specs`, `images`, `credentials`, - /// `landing`, `blocks`, `audit`). + /// templates (`dashboard`, `specs`, `images`, `users`, `groups`, + /// `credentials`, `landing`, `blocks`, `audit`). pub fn can_access_section(&self, section: &str) -> bool { match section { - // Dashboard + apps + media + scoped users: Editor and up. A + // Dashboard + apps + media + scoped users/groups: Editor and up. A // Viewer is NOT an admin-panel operator (#857) — it's the portal's // authenticated-end-user role (group-based card visibility, // #155), so it reaches NO admin section. - "dashboard" | "specs" | "images" | "users" => *self >= Role::Editor, - // Everything else — credentials, landing, groups, logs, disk, - // audit, system, schedules — is admin-only. + "dashboard" | "specs" | "images" | "users" | "groups" => *self >= Role::Editor, + // Everything else — credentials, landing, logs, disk, audit, + // system, schedules — is admin-only. _ => *self == Role::Admin, } } @@ -114,13 +114,19 @@ impl Role { } } - /// Parse the lowercase form back to a [`Role`]. Unknown ⇒ `None`. + /// Parse a wire/DB role case-insensitively. Unknown ⇒ `None`. + /// + /// Writes stay canonical lowercase through [`Self::as_str`], but the + /// column has no CHECK and external identity provisioners may vary casing. pub fn parse(s: &str) -> Option { - match s { - "viewer" => Some(Role::Viewer), - "editor" => Some(Role::Editor), - "admin" => Some(Role::Admin), - _ => None, + if s.eq_ignore_ascii_case("viewer") { + Some(Role::Viewer) + } else if s.eq_ignore_ascii_case("editor") { + Some(Role::Editor) + } else if s.eq_ignore_ascii_case("admin") { + Some(Role::Admin) + } else { + None } } } @@ -741,9 +747,9 @@ mod tests { assert!(Role::Admin > Role::Editor); assert!(Role::Editor > Role::Viewer); - // Dashboard + apps + media + scoped users: Editor and up — a Viewer - // reaches no admin section (#857). - for sec in ["dashboard", "specs", "images", "users"] { + // Dashboard + apps + media + scoped users/groups: Editor and up — a + // Viewer reaches no admin section (#857). + for sec in ["dashboard", "specs", "images", "users", "groups"] { assert!(!Role::Viewer.can_access_section(sec)); assert!(Role::Editor.can_access_section(sec)); assert!(Role::Admin.can_access_section(sec)); diff --git a/crates/ruscker-admin/src/routes/admin.rs b/crates/ruscker-admin/src/routes/admin.rs index 4b0f9ab..f9167a7 100644 --- a/crates/ruscker-admin/src/routes/admin.rs +++ b/crates/ruscker-admin/src/routes/admin.rs @@ -779,6 +779,8 @@ fn section_for_admin_path(path: &str) -> &'static str { "schedules" } else if path.starts_with("/admin/users") { "users" + } else if path.starts_with("/admin/groups") { + "groups" } else { // /admin root and anything unrecognised → dashboard (every // role can reach it). diff --git a/crates/ruscker-admin/src/routes/admin/groups.rs b/crates/ruscker-admin/src/routes/admin/groups.rs index c239660..732c1fa 100644 --- a/crates/ruscker-admin/src/routes/admin/groups.rs +++ b/crates/ruscker-admin/src/routes/admin/groups.rs @@ -1,9 +1,10 @@ -//! Admin > Groups (#503 read-only + #540 CRUD). +//! Admin > Groups (#503 read-only + #540 CRUD + #990 scoped membership). //! //! Groups in Ruscker are **derived**, not a first-class entity: a group is //! any name that appears in a user's memberships or a spec's `access-groups`. //! This page surfaces them — for each group, its member users and the apps it -//! gates — and lets an Admin **rename**, **delete**, and add/remove members. +//! gates. Admins may create, rename, delete, and change membership. Editors +//! may only add/remove non-Admin members of groups they themselves belong to. //! Because there's no `groups` table, every edit rewrites the name across the //! users and specs that reference it; a group exists exactly as long as //! something points at it (adding the first member creates it; removing the @@ -12,6 +13,7 @@ use askama::Template; use axum::{ extract::{Query, State}, + http::StatusCode, response::{IntoResponse, Redirect, Response}, routing::{get, post}, Form, Router, @@ -20,7 +22,9 @@ use serde::Deserialize; use std::collections::{BTreeMap, BTreeSet}; use crate::auth::{RequireAdmin, Role}; +use crate::db::users::UserRow; use crate::i18n::{Locale, Locales}; +use crate::scope::EditorScope; use crate::theme::Theme; use crate::AppState; @@ -29,6 +33,7 @@ use super::KpiMetric; pub fn routes() -> Router { Router::new() .route("/admin/groups", get(index)) + .route("/admin/groups/create", post(create)) .route("/admin/groups/rename", post(rename)) .route("/admin/groups/delete", post(delete)) .route("/admin/groups/members/add", post(add_member)) @@ -73,7 +78,7 @@ struct GroupsPage<'a> { /// Mount prefix for base-path-correct URLs (#294). base: std::sync::Arc, nav_section: &'static str, - /// Current session role (always Admin here) — drives nav gating. + /// Current session role — drives nav and structural-action gating. role: Role, groups: Vec, kpi_groups: i64, @@ -83,7 +88,8 @@ struct GroupsPage<'a> { /// a dedicated "Public apps" rail so the page accounts for every spec, /// not only the gated ones. public_apps: Vec, - /// All usernames, for the "add member" picker. + /// Eligible usernames for the "add member" picker. Editors never receive + /// Admin targets; unscoped Admins receive every account. all_users: Vec, flash: Option, } @@ -113,58 +119,87 @@ fn redirect_flash(flash: &str) -> Response { } async fn index( - _: RequireAdmin, + scope: EditorScope, State(state): State, loc: Locale, theme: Theme, Query(q): Query, ) -> Response { - // group name → (member usernames, apps id→name). BTree everywhere so the + type GroupData = (BTreeSet, BTreeSet); + + // group name → (member usernames, app ids). BTree everywhere so the // page is deterministically ordered (groups, members, apps) with no dups. - let mut map: BTreeMap, BTreeMap)> = BTreeMap::new(); + let mut map: BTreeMap = BTreeMap::new(); let mut all_users: Vec = Vec::new(); - // Users contribute members. + // An Editor's own authoritative row is enough to establish each owned + // group even if no app references it. A failed scope lookup yields an + // empty list, so this remains fail-closed. + if !scope.unscoped { + for group in &scope.groups { + map.entry(group.clone()).or_default(); + } + } + + // Users contribute members and the add-member picker. Editors may add any + // non-Admin account to an owned group, but Admin accounts are neither + // exposed nor offered as mutation targets. if let Some(db) = state.db.as_ref() { - if let Ok(users) = crate::db::users::list_all(db).await { - for u in users { - all_users.push(u.username.clone()); - for g in u.groups { - map.entry(g).or_default().0.insert(u.username.clone()); + match crate::db::users::list_all(db).await { + Ok(users) => { + for u in users { + if scope.unscoped || u.role != Role::Admin { + all_users.push(u.username.clone()); + } + if !scope.may_touch_user(&u) { + continue; + } + for group in &u.groups { + if scope.may_touch_group(group) { + map.entry(group.clone()) + .or_default() + .0 + .insert(u.username.clone()); + } + } } } + Err(error) => { + tracing::warn!(error = ?error, "group member lookup failed; using empty list"); + } } } all_users.sort(); - // Specs contribute apps (by `access-groups`). + // Specs contribute apps (by `access-groups`). Resolve and authorize from + // the effective authoritative catalog before inserting ids into the view; + // a missing row can never be interpreted as an unrestricted app. let specs = crate::catalog::effective_specs_cached(&state).await; - for s in specs.iter() { - if let Some(groups) = s.access_groups.as_ref() { - let name = s.display_name.clone().unwrap_or_else(|| s.id.clone()); - for g in groups { - map.entry(g.clone()) - .or_default() - .1 - .insert(s.id.clone(), name.clone()); + for spec in specs.iter().filter(|spec| scope.may_touch_spec(spec)) { + if let Some(groups) = spec.access_groups.as_ref() { + for group in groups { + if scope.may_touch_group(group) { + map.entry(group.clone()) + .or_default() + .1 + .insert(spec.id.clone()); + } } } } let groups: Vec = map .into_iter() - .map(|(name, (members, apps))| GroupView { + .map(|(name, (members, app_ids))| GroupView { name, members: members.into_iter().collect(), - apps: apps + apps: app_ids .into_iter() - .map(|(id, name)| { - // Resolve back to the spec for logo + type (#809); - // admin page, the linear scan is fine. - match specs.iter().find(|s| s.id == id) { - Some(s) => AppRef::from_spec(s), - None => AppRef { id, name, logo: None, kind: "app" }, - } + .filter_map(|id| { + specs + .iter() + .find(|spec| spec.id == id) + .map(AppRef::from_spec) }) .collect(), }) @@ -185,7 +220,7 @@ async fn index( // access-users empty (#623 audit: a users-only-gated spec is not public). let mut public_apps: Vec = specs .iter() - .filter(|s| s.is_open()) + .filter(|spec| scope.may_touch_spec(spec) && spec.is_open()) .map(AppRef::from_spec) .collect(); public_apps.sort_by(|a, b| a.name.cmp(&b.name)); @@ -197,7 +232,7 @@ async fn index( locales_all: &Locale::ALL, base: state.base_path.clone(), nav_section: "groups", - role: Role::Admin, + role: scope.role, groups, kpi_groups, kpi_members, @@ -219,6 +254,70 @@ fn clean_group(raw: &str) -> Option { Some(g.to_string()) } +fn not_found() -> Response { + StatusCode::NOT_FOUND.into_response() +} + +/// Persist one membership list and invalidate the proxy identity cache only +/// after the authoritative write succeeds. +async fn save_memberships( + state: &AppState, + user: &UserRow, + groups: &[String], + actor: &str, + success: &str, +) -> Response { + let Some(db) = state.db.as_ref() else { + return redirect_flash("bad-input"); + }; + match crate::db::users::set_groups(db, &user.username, groups, Some(actor)).await { + Ok(()) => { + state.invalidate_identity_cache(); + redirect_flash(success) + } + Err(error) => { + tracing::warn!( + user = %user.username, + error = ?error, + "group membership write failed" + ); + redirect_flash("bad-input") + } + } +} + +#[derive(Deserialize)] +struct MemberForm { + group: String, + username: String, +} + +/// Creating a derived group means adding its first member. This remains a +/// distinct Admin-only handler so the membership endpoint opened to Editors +/// cannot grow a new authority boundary through its UI or route contract. +async fn create( + admin: RequireAdmin, + State(state): State, + Form(form): Form, +) -> Response { + let Some(group) = clean_group(&form.group) else { + return redirect_flash("bad-input"); + }; + let Some(db) = state.db.as_ref() else { + return redirect_flash("bad-input"); + }; + match crate::db::users::fetch(db, &form.username).await { + Ok(Some(user)) => { + let mut groups = user.groups.clone(); + if !groups.iter().any(|existing| existing == &group) { + groups.push(group); + } + save_memberships(&state, &user, &groups, admin.actor(), "created").await + } + _ => redirect_flash("bad-input"), + } +} + /// Rewrite group `old` everywhere it's referenced. `new = Some(n)` renames it /// (folding into `n` if a user/spec already has both); `new = None` deletes /// it. Touches both user memberships and spec `access-groups`. @@ -300,49 +399,86 @@ async fn delete(admin: RequireAdmin, State(state): State, Form(f): For redirect_flash("deleted") } -#[derive(Deserialize)] -struct MemberForm { - group: String, - username: String, -} - async fn add_member( - admin: RequireAdmin, + scope: EditorScope, State(state): State, - Form(f): Form, + Form(form): Form, ) -> Response { - let Some(group) = clean_group(&f.group) else { + let Some(group) = clean_group(&form.group) else { return redirect_flash("bad-input"); }; + if !scope.may_touch_group(&group) { + return not_found(); + } let Some(db) = state.db.as_ref() else { return redirect_flash("bad-input"); }; - match crate::db::users::fetch(db, &f.username).await { - Ok(Some(u)) => { - let mut groups = u.groups; - if !groups.iter().any(|g| g == &group) { - groups.push(group); + match crate::db::users::fetch(db, &form.username).await { + Ok(Some(user)) => { + let mut requested: Vec = user + .groups + .iter() + .filter(|existing| scope.may_touch_group(existing)) + .cloned() + .collect(); + if !requested.iter().any(|existing| existing == &group) { + requested.push(group); } - let _ = crate::db::users::set_groups(db, &u.username, &groups, Some(admin.actor())).await; - state.invalidate_identity_cache(); - redirect_flash("member-added") + let groups = scope.merge_preserving_out_of_scope(&user.groups, &requested); + + // `may_touch_user` normally evaluates the current membership. For + // an add, evaluate the authoritative row plus the proposed owned + // group: this permits inducting a non-Admin into the team while + // retaining the same central predicate that excludes Admin. + let mut proposed = user.clone(); + proposed.groups = groups.clone(); + if !scope.may_touch_user(&proposed) { + return not_found(); + } + save_memberships(&state, &user, &groups, scope.actor(), "member-added").await } - _ => redirect_flash("bad-input"), + _ => not_found(), } } async fn remove_member( - admin: RequireAdmin, + scope: EditorScope, State(state): State, - Form(f): Form, + Form(form): Form, ) -> Response { + let Some(group) = clean_group(&form.group) else { + return redirect_flash("bad-input"); + }; + if !scope.may_touch_group(&group) { + return not_found(); + } let Some(db) = state.db.as_ref() else { return redirect_flash("bad-input"); }; - if let Ok(Some(u)) = crate::db::users::fetch(db, &f.username).await { - let groups: Vec = u.groups.into_iter().filter(|g| g != &f.group).collect(); - let _ = crate::db::users::set_groups(db, &u.username, &groups, Some(admin.actor())).await; - state.invalidate_identity_cache(); + match crate::db::users::fetch(db, &form.username).await { + Ok(Some(user)) if scope.may_touch_user(&user) => { + // `set_groups` replaces the entire CSV. Feed only the remaining + // in-scope memberships through the shared merge primitive so an + // Editor removes this group surgically and preserves every + // foreign-team membership. + let requested: Vec = user + .groups + .iter() + .filter(|existing| { + existing.as_str() != group.as_str() && scope.may_touch_group(existing) + }) + .cloned() + .collect(); + let groups = scope.merge_preserving_out_of_scope(&user.groups, &requested); + save_memberships( + &state, + &user, + &groups, + scope.actor(), + "member-removed", + ) + .await + } + _ => not_found(), } - redirect_flash("member-removed") } diff --git a/crates/ruscker-admin/src/scope.rs b/crates/ruscker-admin/src/scope.rs index 17e501c..e3d3522 100644 --- a/crates/ruscker-admin/src/scope.rs +++ b/crates/ruscker-admin/src/scope.rs @@ -85,6 +85,15 @@ impl EditorScope { self.groups.iter().any(|owned| owned == group) } + /// Whether this caller owns the named group boundary. + /// + /// Group names stay case-sensitive, matching [`Spec::access_allows`]. + /// Admin and break-glass sessions are unscoped; an Editor must carry the + /// exact group in the authoritative memberships fetched for this request. + pub fn may_touch_group(&self, group: &str) -> bool { + self.unscoped || self.has_group(group) + } + /// Whether this caller may mutate or operate one spec. /// /// Open specs are shared admin-panel resources, so every Editor may work @@ -272,6 +281,7 @@ mod tests { assert!(scope.groups.is_empty()); assert!(scope.may_touch_spec(&restricted)); assert!(scope.may_touch_user(&admin_target)); + assert!(scope.may_touch_group("foreign")); assert!(scope.may_assign_groups(&["foreign".into()])); assert!(scope.may_assign_role(Role::Admin)); } @@ -283,6 +293,9 @@ mod tests { fn editor_touches_only_open_or_group_shared_specs() { let editor = scope(&["blue"]); + assert!(editor.may_touch_group("blue")); + assert!(!editor.may_touch_group("Blue")); + assert!(!editor.may_touch_group("green")); assert!(editor.may_touch_spec(&spec(&[], &[])), "open spec"); assert!( editor.may_touch_spec(&spec(&["blue", "green"], &[])), diff --git a/crates/ruscker-admin/templates/admin/groups.html b/crates/ruscker-admin/templates/admin/groups.html index 66af072..1e42b95 100644 --- a/crates/ruscker-admin/templates/admin/groups.html +++ b/crates/ruscker-admin/templates/admin/groups.html @@ -7,7 +7,10 @@

{{ self.t("admin-groups-title") }}

-

{{ self.t("admin-groups-subtitle") }}

+

+ {% if role == Role::Admin %}{{ self.t("admin-groups-subtitle") }} + {% else %}{{ self.t("admin-groups-editor-subtitle") }}{% endif %} +

{{ groups.len() }}
@@ -28,7 +31,8 @@

{{ self.t("admin-groups-title") } {% else %}
- {% if f == "renamed" %}{{ self.t("admin-groups-flash-renamed") }} + {% if f == "created" %}{{ self.t("admin-groups-flash-created") }} + {% else if f == "renamed" %}{{ self.t("admin-groups-flash-renamed") }} {% else if f == "deleted" %}{{ self.t("admin-groups-flash-deleted") }} {% else if f == "member-added" %}{{ self.t("admin-groups-flash-member-added") }} {% else %}{{ self.t("admin-groups-flash-member-removed") }}{% endif %} @@ -38,8 +42,8 @@

{{ self.t("admin-groups-title") } {# Create a group: a group exists once a user belongs to it, so "create" = add its first member under a new name. #} -{% if !all_users.is_empty() %} -
+{% if role == Role::Admin && !all_users.is_empty() %} + {{ self.t("admin-groups-new-group-title") }} @@ -66,6 +70,7 @@

{{ self.t("admin-groups-title") }
{{ g.name }}
+ {% if role == Role::Admin %} {# Rename: prompt for a new name (read the old one from the DOM, not interpolated into JS, so a name with a quote can't break it). #} {{ self.t("admin-groups-title") } + {% endif %}
{# Collapse is measured, not assumed: >8 pills that still FIT the preview height (short names / wide card) must not fade visible diff --git a/crates/ruscker-admin/tests/rbac.rs b/crates/ruscker-admin/tests/rbac.rs index 7d4f5c8..fc95f83 100644 --- a/crates/ruscker-admin/tests/rbac.rs +++ b/crates/ruscker-admin/tests/rbac.rs @@ -1358,3 +1358,341 @@ async fn admin_remains_unscoped_for_all_user_operations() { "Admin bulk import remains functional" ); } + +// ── Editor group scope over group membership (#990 slice 3) ───────── + +#[tokio::test] +async fn scoped_editor_lists_only_owned_groups_apps_and_matching_kpis() { + let (state, _db) = scoped_user_state().await; + let cookie = scoped_cookie(&state, Role::Editor, Some("editor-a")).await; + let response = send_request( + state, + "GET", + "/admin/groups", + Some(&cookie), + Body::empty(), + None, + ) + .await; + assert_eq!(response.status(), StatusCode::OK); + let body = response_body(response).await; + + assert!(body.contains(r#"data-group="time-a""#)); + assert!( + !body.contains(r#"data-group="time-b""#), + "a foreign group must not get a card" + ); + assert!(body.contains(r#"href="/admin/specs/time-a/edit""#)); + assert!( + !body.contains(r#"href="/admin/specs/time-b/edit""#), + "a foreign app must not leak through a group card" + ); + assert!( + !body.contains(">admin-a"), + "an Admin is not offered as an Editor membership target" + ); + assert!( + body.contains(r#"href="/admin/groups""#), + "the Groups nav tab is visible to Editors" + ); + assert_eq!( + metric_values(&body), + vec!["1", "3", "1"], + "group, member and app KPIs derive from the scoped cards" + ); + assert!( + !body.contains(r#"action="/admin/groups/create""#), + "Editors do not get the create-group affordance" + ); + assert!(!body.contains(r#"action="/admin/groups/rename""#)); + assert!(!body.contains(r#"action="/admin/groups/delete""#)); +} + +#[tokio::test] +async fn scoped_editor_gets_404_for_foreign_group_membership_routes() { + let (state, _db) = scoped_user_state().await; + let cookie = scoped_cookie(&state, Role::Editor, Some("editor-a")).await; + + for (uri, body) in [ + ( + "/admin/groups/members/add", + "group=time-b&username=viewer-b", + ), + ( + "/admin/groups/members/remove", + "group=time-b&username=viewer-b", + ), + ] { + let response = send_request( + state.clone(), + "POST", + uri, + Some(&cookie), + Body::from(body), + Some("application/x-www-form-urlencoded"), + ) + .await; + assert_eq!( + response.status(), + StatusCode::NOT_FOUND, + "foreign group must be indistinguishable from a missing group: {uri}" + ); + } +} + +#[tokio::test] +async fn scoped_editor_adds_and_removes_only_owned_membership() { + let (state, db) = scoped_user_state().await; + let cookie = scoped_cookie(&state, Role::Editor, Some("editor-a")).await; + + let added = send_request( + state.clone(), + "POST", + "/admin/groups/members/add", + Some(&cookie), + Body::from("group=time-a&username=viewer-b"), + Some("application/x-www-form-urlencoded"), + ) + .await; + assert_eq!(added.status(), StatusCode::SEE_OTHER); + assert_eq!( + ruscker_admin::db::users::fetch(&db, "viewer-b") + .await + .unwrap() + .unwrap() + .groups, + vec!["time-a", "time-b"] + ); + + let removed_shared = send_request( + state.clone(), + "POST", + "/admin/groups/members/remove", + Some(&cookie), + Body::from("group=time-a&username=shared-user"), + Some("application/x-www-form-urlencoded"), + ) + .await; + assert_eq!(removed_shared.status(), StatusCode::SEE_OTHER); + assert_eq!( + ruscker_admin::db::users::fetch(&db, "shared-user") + .await + .unwrap() + .unwrap() + .groups, + vec!["time-b"], + "removing time-a must preserve the foreign time-b membership" + ); + + let removed_added = send_request( + state, + "POST", + "/admin/groups/members/remove", + Some(&cookie), + Body::from("group=time-a&username=viewer-b"), + Some("application/x-www-form-urlencoded"), + ) + .await; + assert_eq!(removed_added.status(), StatusCode::SEE_OTHER); + assert_eq!( + ruscker_admin::db::users::fetch(&db, "viewer-b") + .await + .unwrap() + .unwrap() + .groups, + vec!["time-b"] + ); +} + +#[tokio::test] +async fn scoped_editor_cannot_add_or_remove_an_admin_member() { + let (state, db) = scoped_user_state().await; + let ruscker_admin::db::ConfigDb::Sqlite(pool) = &db else { + panic!("RBAC scope fixture must use SQLite"); + }; + // The role column has no CHECK and the future external-identity writer may + // use different casing. The membership guard must still recognize Admin. + sqlx::query("UPDATE users SET role = 'Admin' WHERE username = 'admin-a'") + .execute(pool) + .await + .unwrap(); + let cookie = scoped_cookie(&state, Role::Editor, Some("editor-a")).await; + + for uri in [ + "/admin/groups/members/add", + "/admin/groups/members/remove", + ] { + let response = send_request( + state.clone(), + "POST", + uri, + Some(&cookie), + Body::from("group=time-a&username=admin-a"), + Some("application/x-www-form-urlencoded"), + ) + .await; + assert_eq!( + response.status(), + StatusCode::NOT_FOUND, + "Admin target must stay outside Editor scope: {uri}" + ); + } + assert_eq!( + ruscker_admin::db::users::fetch(&db, "admin-a") + .await + .unwrap() + .unwrap() + .groups, + vec!["time-a"] + ); +} + +#[tokio::test] +async fn scoped_editor_cannot_create_rename_or_delete_groups() { + let (state, _db) = scoped_user_state().await; + let cookie = scoped_cookie(&state, Role::Editor, Some("editor-a")).await; + + for (uri, body) in [ + ( + "/admin/groups/create", + "group=new-team&username=viewer-a", + ), + ( + "/admin/groups/rename", + "old=time-a&new=renamed-time-a", + ), + ("/admin/groups/delete", "name=time-a"), + ] { + let response = send_request( + state.clone(), + "POST", + uri, + Some(&cookie), + Body::from(body), + Some("application/x-www-form-urlencoded"), + ) + .await; + assert_eq!( + response.status(), + StatusCode::FORBIDDEN, + "structural group operation remains Admin-only: {uri}" + ); + } +} + +#[tokio::test] +async fn admin_remains_unscoped_for_all_group_operations() { + let (state, db) = scoped_user_state().await; + let cookie = scoped_cookie(&state, Role::Admin, Some("admin-op")).await; + + let list = send_request( + state.clone(), + "GET", + "/admin/groups", + Some(&cookie), + Body::empty(), + None, + ) + .await; + assert_eq!(list.status(), StatusCode::OK); + let list = response_body(list).await; + assert!(list.contains(r#"data-group="time-a""#)); + assert!(list.contains(r#"data-group="time-b""#)); + assert!(list.contains(r#"action="/admin/groups/create""#)); + assert!(list.contains(r#"action="/admin/groups/rename""#)); + assert!(list.contains(r#"action="/admin/groups/delete""#)); + + let created = send_request( + state.clone(), + "POST", + "/admin/groups/create", + Some(&cookie), + Body::from("group=new-team&username=viewer-b"), + Some("application/x-www-form-urlencoded"), + ) + .await; + assert_eq!(created.status(), StatusCode::SEE_OTHER); + assert!( + ruscker_admin::db::users::fetch(&db, "viewer-b") + .await + .unwrap() + .unwrap() + .groups + .contains(&"new-team".to_string()) + ); + + let renamed = send_request( + state.clone(), + "POST", + "/admin/groups/rename", + Some(&cookie), + Body::from("old=new-team&new=renamed-team"), + Some("application/x-www-form-urlencoded"), + ) + .await; + assert_eq!(renamed.status(), StatusCode::SEE_OTHER); + let viewer_b = ruscker_admin::db::users::fetch(&db, "viewer-b") + .await + .unwrap() + .unwrap(); + assert!(!viewer_b.groups.contains(&"new-team".to_string())); + assert!(viewer_b.groups.contains(&"renamed-team".to_string())); + + let deleted = send_request( + state.clone(), + "POST", + "/admin/groups/delete", + Some(&cookie), + Body::from("name=renamed-team"), + Some("application/x-www-form-urlencoded"), + ) + .await; + assert_eq!(deleted.status(), StatusCode::SEE_OTHER); + assert!( + !ruscker_admin::db::users::fetch(&db, "viewer-b") + .await + .unwrap() + .unwrap() + .groups + .contains(&"renamed-team".to_string()) + ); + + let add_admin = send_request( + state.clone(), + "POST", + "/admin/groups/members/add", + Some(&cookie), + Body::from("group=time-b&username=admin-a"), + Some("application/x-www-form-urlencoded"), + ) + .await; + assert_eq!(add_admin.status(), StatusCode::SEE_OTHER); + assert!( + ruscker_admin::db::users::fetch(&db, "admin-a") + .await + .unwrap() + .unwrap() + .groups + .contains(&"time-b".to_string()), + "unscoped Admin may still manage Admin memberships" + ); + + let remove_admin = send_request( + state, + "POST", + "/admin/groups/members/remove", + Some(&cookie), + Body::from("group=time-b&username=admin-a"), + Some("application/x-www-form-urlencoded"), + ) + .await; + assert_eq!(remove_admin.status(), StatusCode::SEE_OTHER); + assert!( + !ruscker_admin::db::users::fetch(&db, "admin-a") + .await + .unwrap() + .unwrap() + .groups + .contains(&"time-b".to_string()) + ); +} From 60f6505bdbd7201da76ec4f7f157ddd4fa9b149c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Leite?= Date: Wed, 29 Jul 2026 20:38:05 -0300 Subject: [PATCH 2/2] fix: log instead of swallowing an unparseable stored role (review of #990 slice 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The slice made `Role::parse` case-insensitive, which is the right call for consistency: slice 2's SQL filter already hides `Admin`-cased rows from Editors via `lower(role)`, and having Rust believe the same row is a Viewer would leave the two layers disagreeing about an account's role — how confused-deputy bugs start. Chasing that turned up the actual latent defect, which was never the casing: `Role::parse(&role).unwrap_or(Role::Viewer)` swallowed the failure. Falling back to the LEAST privilege is correct, but doing it silently means an account that suddenly reads as Viewer is an undebuggable mystery from the UI — and `users.role` is plain TEXT with no CHECK, so a hand-edited row or an external provisioner (#934) can put anything there. Now it warns with the username and the stored value. The fallback is unchanged; only the silence is gone. Co-Authored-By: Claude Opus 5 (1M context) --- crates/ruscker-admin/src/db/users.rs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/crates/ruscker-admin/src/db/users.rs b/crates/ruscker-admin/src/db/users.rs index 84c2fcc..f194d7e 100644 --- a/crates/ruscker-admin/src/db/users.rs +++ b/crates/ruscker-admin/src/db/users.rs @@ -213,10 +213,27 @@ fn row_from( // Treat a stored empty string the same as NULL (the form may submit // blanks); keep only non-empty trimmed values. let clean = |s: Option| s.map(|v| v.trim().to_string()).filter(|v| !v.is_empty()); + // An unparseable stored role falls back to the LEAST privilege, which is + // the right default — but it used to do so silently, and a row that + // suddenly reads as Viewer is exactly the "why did my admin lose access?" + // mystery nobody can debug from the UI. `role` is plain TEXT with no + // CHECK, so a hand-edited row or an external provisioner (#934) can put + // anything there. Say so once per read; an operator needs the row's name. + let role = match Role::parse(&role) { + Some(parsed) => parsed, + None => { + tracing::warn!( + username = %username, + stored_role = %role, + "stored role does not parse; treating the account as Viewer" + ); + Role::Viewer + } + }; UserRow { id, username, - role: Role::parse(&role).unwrap_or(Role::Viewer), + role, // `must_change_password` reads as `bool` on both backends: sqlx // decodes SQLite's INTEGER 0/1 and Postgres' native BOOLEAN. must_change_password: must_change,