From 35d926b73efb86d9709dcf1635ce78d0625a5688 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Leite?= Date: Wed, 29 Jul 2026 20:55:43 -0300 Subject: [PATCH] feat: add scoped Editor migration safety (#990 slice 4) --- Cargo.lock | 1 + book/src/admin.md | 70 ++++++++++---- crates/ruscker-admin/Cargo.toml | 1 + crates/ruscker-admin/src/db/users.rs | 22 +++++ crates/ruscker-admin/src/lib.rs | 137 +++++++++++++++++++++++++++ docs/ROADMAP.md | 8 +- docs/SECURITY.md | 44 ++++++--- 7 files changed, 250 insertions(+), 33 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a0b1d13..48f0b10 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3081,6 +3081,7 @@ dependencies = [ "tower-cookies", "tower-http", "tracing", + "tracing-subscriber", "unic-langid", "url", "urlencoding", diff --git a/book/src/admin.md b/book/src/admin.md index 2781dbb..f9366e6 100644 --- a/book/src/admin.md +++ b/book/src/admin.md @@ -33,17 +33,18 @@ portal, plus your current **access level** and account actions. ## Users and access levels (roles) -Each person gets their own account (username + password). Admins manage -accounts under **Users** (`/admin/users`): create one, or open a row's -**Edit** button for the consolidated page — role, groups and profile in -one form with a single save, plus the password reset. Each row shows a -coloured avatar with the user's initials and their groups as coloured -badges (a group keeps the same colour on the Groups and Apps pages). -The table is paginated on the server at 50 users per page. Its -server-side, case-insensitive search (accented letters included — `GESTÃO` -matches `Gestão`, though it does not strip diacritics, so `Joao` won't find -`João`) covers username, groups, department, email and phone on both SQLite -and Postgres. +Each person gets their own account (username + password). Admins, and +Editors within the group boundary described below, manage accounts under +**Users** (`/admin/users`): create one, or open a row's **Edit** button for +the consolidated page — role, groups and profile in one form with a single +save, plus the password reset. Each row shows a coloured avatar with the +user's initials and their groups as coloured badges (a group keeps the same +colour on the Groups and Apps pages). The table is paginated on the server +at 50 users per page. Its server-side, case-insensitive search (accented +letters included — `GESTÃO` matches `Gestão`, though it does not strip +diacritics, so `Joao` won't find `João`) covers username, groups, +department, email and phone on both SQLite and Postgres. An Editor's totals, +search and pages all cover only accounts in that Editor's scope. Passwords follow a **policy**: at least 8 characters, with at least one uppercase letter, one lowercase letter, one digit and one special @@ -61,16 +62,53 @@ read a password as you type it. | Role | Can do | |---|---| | **Viewer** | a **portal** account, not a panel operator: signs in to unlock group-restricted cards on the landing; reaches no admin section | -| **Editor** | view + manage Apps and Media; view Containers and stop/restart replicas | -| **Admin** | everything, including managing users, credentials, the landing editor, custom blocks and the audit log | +| **Editor** | manage Apps, Containers, Users and Group membership within their own groups; open apps and the Media library remain shared across Editors | +| **Admin** | unrestricted panel access, including every app, user and group plus credentials, the landing editor, custom blocks and the audit log | The panel shows only the sections your role can reach. Enforcement is server-side — hiding a nav link is just UX; the routes themselves -return `403` for a role that isn't allowed. The audit log records the -acting username. A **last-admin guard** stops you deleting or demoting -the only remaining admin (so the portal can't be locked out); the +return `403` for a role that isn't allowed. When an Editor is allowed into +a section but asks for an app, user, group or replica outside their scope, +the handler returns `404`, deliberately making a forbidden target +indistinguishable from a missing one. The audit log records the acting +username. A **last-admin guard** stops you deleting or demoting the only +remaining admin (so the portal can't be locked out); the `RUSCKER_ADMIN_TOKEN` break-glass login is the other safety net. +An Editor's scope is the exact, case-sensitive set of groups on their own +account, read from the database on every request: + +- **Applications and Containers.** An Editor can see and operate every + app whose `access-groups` overlaps their groups. Apps with neither + `access-groups` nor `access-users` are **open** and remain global to every + Editor. An app restricted only by `access-users` has no group boundary and + is therefore Admin-only. The same rule protects app lists, direct app + actions, YAML app imports, container details, logs, stop and restart. + When editing a shared app, groups outside the Editor's scope stay attached + but are not exposed as editable choices. +- **Users.** The list, search, pagination and KPIs include only non-Admin + accounts sharing at least one group. An Editor may create Viewer or Editor + accounts, but must assign at least one of their own groups; they may edit + in-scope role, profile and membership and reset an in-scope user's + password. They cannot target an Admin, assign the Admin role, edit + themselves through the Users page, delete an account, import users from + CSV or reset MFA. Memberships belonging to other groups survive an + Editor's edit. +- **Groups.** An Editor sees only their groups and may add or remove + non-Admin users from those memberships. Creating, renaming and deleting a + group remain Admin-only. + +Admin account sessions and the `RUSCKER_ADMIN_TOKEN` break-glass session are +unrestricted; group membership never narrows either one. + +> **Before upgrading:** give every Editor account at least one group in the +> current release's **Users** page before starting the upgraded Ruscker. +> Previously, an Editor with no groups operated the whole catalog; under the +> scoped model that account can operate only open apps and sees no users or +> groups. Startup emits a warning on the Logs stream with the count and +> usernames of every Editor still missing a group, but it does not block +> startup. + ### 2FA / MFA for selected apps In an app's **Access & scale** settings, enable **Require 2FA** diff --git a/crates/ruscker-admin/Cargo.toml b/crates/ruscker-admin/Cargo.toml index 54027ef..ab4da70 100644 --- a/crates/ruscker-admin/Cargo.toml +++ b/crates/ruscker-admin/Cargo.toml @@ -118,6 +118,7 @@ pretty_assertions = { workspace = true } insta = { workspace = true } tower = { workspace = true, features = ["util"] } tokio = { workspace = true } +tracing-subscriber = { workspace = true } # WS client + mock upstream for the proxy upgrade tests (#730) tokio-tungstenite = { workspace = true } # Real Docker backend for the ws-e2e feature (#929) — dev-only; the diff --git a/crates/ruscker-admin/src/db/users.rs b/crates/ruscker-admin/src/db/users.rs index f194d7e..eff10f7 100644 --- a/crates/ruscker-admin/src/db/users.rs +++ b/crates/ruscker-admin/src/db/users.rs @@ -274,6 +274,28 @@ pub async fn list_all(db: &ConfigDb) -> Result> { .collect()) } +/// Editor usernames whose authoritative membership list is empty. +/// +/// This is a startup migration safety check for scoped Editor delegation +/// (#990). Read only Editor rows and apply [`parse_groups`] in Rust so a +/// hand-edited value containing only commas/whitespace is treated exactly +/// like the request-time scope resolver would treat it. +pub async fn editors_without_groups(db: &ConfigDb) -> Result> { + let sql = "SELECT username, groups + FROM users + WHERE lower(role) = 'editor' + ORDER BY username ASC"; + let rows: Vec<(String, String)> = match db { + ConfigDb::Sqlite(pool) => sqlx::query_as(sql).fetch_all(pool).await, + ConfigDb::Postgres(pool) => sqlx::query_as(sql).fetch_all(pool).await, + } + .context("list Editor accounts without groups")?; + Ok(rows + .into_iter() + .filter_map(|(username, groups)| parse_groups(&groups).is_empty().then_some(username)) + .collect()) +} + /// Escape `%`, `_` and `\` in a user-typed search term so each matches /// literally under SQL `LIKE`, then wrap it in `%…%` for a substring /// match. Both dialects use `\` as the escape character (Postgres by diff --git a/crates/ruscker-admin/src/lib.rs b/crates/ruscker-admin/src/lib.rs index 2d5afb7..503bb81 100644 --- a/crates/ruscker-admin/src/lib.rs +++ b/crates/ruscker-admin/src/lib.rs @@ -64,6 +64,36 @@ pub const APP_VERSION: &str = env!("CARGO_PKG_VERSION"); /// in `ruscker-cli`'s `init_tracing`. pub const STARTUP_LOG_TARGET: &str = "ruscker_startup"; +/// Warn about the one access-loss edge introduced by scoped Editors (#990). +/// +/// Before group scope, every Editor operated the whole catalog. An upgraded +/// Editor account with no memberships now fails closed and reaches only open +/// apps, so silently continuing would leave the operator with no clue which +/// accounts need migration. This remains a warning: startup must not fail +/// because an account is intentionally ungrouped or because the check itself +/// cannot read the database. +async fn warn_editors_without_groups(db: Option<&db::ConfigDb>) { + let Some(db) = db else { return }; + match db::users::editors_without_groups(db).await { + Ok(editors) if editors.is_empty() => {} + Ok(editors) => { + tracing::warn!( + target: STARTUP_LOG_TARGET, + editors_without_groups = editors.len(), + editors = %editors.join(", "), + "Editor accounts without groups can manage only open apps; assign their groups" + ); + } + Err(error) => { + tracing::warn!( + target: STARTUP_LOG_TARGET, + error = ?error, + "could not check for Editor accounts without groups" + ); + } + } +} + /// Identity attributes cached together for the proxy hot path (#1001). /// `celular`, roles, and authentication tokens are intentionally absent: /// they are not supported upstream identity claims. @@ -482,6 +512,8 @@ impl AdminServer { /// Start listening. Blocks until the process is shut down. pub async fn run(self) -> Result<()> { + warn_editors_without_groups(self.state.db.as_ref()).await; + // Reconcile the in-memory replica registry with whatever // the backend reports as already running. This lets // Ruscker survive its own restart without losing track @@ -1410,3 +1442,108 @@ mod reconcile_tests { assert_eq!(replicas[0].sessions_max, 7, "untouched when spec is absent"); } } + +#[cfg(test)] +mod startup_warning_tests { + use super::warn_editors_without_groups; + use crate::db::{ConfigDb, MIGRATIONS}; + use sqlx::sqlite::SqlitePoolOptions; + use std::io::Write; + use std::sync::{Arc, Mutex}; + use tracing::instrument::WithSubscriber; + use tracing_subscriber::fmt::MakeWriter; + + #[derive(Clone, Default)] + struct CapturedWriter(Arc>>); + + impl Write for CapturedWriter { + fn write(&mut self, bytes: &[u8]) -> std::io::Result { + self.0 + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .write(bytes) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } + } + + impl<'a> MakeWriter<'a> for CapturedWriter { + type Writer = Self; + + fn make_writer(&'a self) -> Self::Writer { + self.clone() + } + } + + async fn user_db(editors: &[(&str, &str)]) -> ConfigDb { + let pool = SqlitePoolOptions::new() + .max_connections(1) + .connect("sqlite::memory:") + .await + .unwrap(); + MIGRATIONS.run(&pool).await.unwrap(); + let now = chrono::Utc::now(); + for (username, groups) in editors { + sqlx::query( + "INSERT INTO users + (id, username, password_hash, role, must_change_password, + groups, created_at, updated_at, created_by) + VALUES (?, ?, 'unused-test-hash', 'editor', 0, ?, ?, ?, NULL)", + ) + .bind(uuid::Uuid::new_v4().to_string()) + .bind(username) + .bind(groups) + .bind(now) + .bind(now) + .execute(&pool) + .await + .unwrap(); + } + ConfigDb::Sqlite(pool) + } + + async fn captured_warning(db: &ConfigDb) -> String { + let writer = CapturedWriter::default(); + let subscriber = tracing_subscriber::fmt() + .without_time() + .with_ansi(false) + .with_target(true) + .with_max_level(tracing::Level::TRACE) + .with_writer(writer.clone()) + .finish(); + warn_editors_without_groups(Some(db)) + .with_subscriber(subscriber) + .await; + let bytes = writer + .0 + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .clone(); + String::from_utf8(bytes).unwrap() + } + + #[tokio::test] + async fn startup_warning_names_every_editor_without_groups_and_stays_silent_otherwise() { + let mixed = user_db(&[ + ("zoe", ""), + ("alice", " , "), + ("grouped-editor", "operations"), + ]) + .await; + let warning = captured_warning(&mixed).await; + assert!(warning.contains(" WARN ")); + assert!(warning.contains("ruscker_startup")); + assert!(warning.contains("editors_without_groups=2")); + assert!(warning.contains("editors=alice, zoe")); + assert!(warning.contains("can manage only open apps")); + assert!(!warning.contains("grouped-editor")); + + let grouped = user_db(&[("alice", "operations"), ("zoe", "analytics")]).await; + assert!( + captured_warning(&grouped).await.is_empty(), + "all grouped Editors must produce no startup warning" + ); + } +} diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index dfa7fcc..ec9b763 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -152,7 +152,9 @@ drill into any container. - [ ] OIDC integration (Keycloak, Auth0, Google) - [ ] SAML for enterprise - [ ] LDAP directory integration -- [x] Role-based access control (Viewer / Editor / Admin) — shipped +- [x] Role-based access control (Viewer / Editor / Admin), including + group-scoped Editor delegation across Apps, Users and Groups; Admin + accounts and the break-glass token remain unrestricted - [x] Per-spec access lists (only group X can use this app) — shipped in Phase 6 - [x] Per-app step-up MFA — one user-owned TOTP factor, recovery codes, trusted-device grants, and per-spec proof-freshness policy @@ -160,8 +162,8 @@ drill into any container. ShinyProxy-compatible user/group headers plus selected profile claims > Only the external identity-provider items (OIDC / SAML / LDAP) remain; the -> database-backed role model, per-spec access lists, step-up MFA, and -> identity forwarding already ship. +> database-backed role model (including group-scoped Editors), per-spec access +> lists, step-up MFA, and identity forwarding already ship. ## Continuing shipped work diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 9bcde57..f177c64 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -138,20 +138,36 @@ Status: living document. Tracks the Phase 5 security audit a cooldown-deduplicated `mfa.break_glass_bypass` audit row. Reserved `__ruscker_mfa_*` cookies are consumed by Ruscker and stripped before HTTP or WebSocket proxying, so device and enrollment bearers never reach an app. -- **[implemented]** Role-based access control (#101/#107) — three - roles (**Viewer** = signed-in portal user with no admin-panel section; - **Editor** = apps + media + dashboard incl. stop/restart; **Admin** = - everything, incl. user - management). Enforcement is **server-side** via the `AdminSession` / - `RequireEditor` / `RequireAdmin` extractors on each route group — - the permission matrix lives in `Role::can_access_section` / - `can_manage`, and the nav only *hides* links it can't reach (UX, not - the boundary). Denied → `403`. Admins manage accounts at - `/admin/users` (create/role/reset-password/delete) with a - **last-admin guard** that refuses to delete or demote the only - remaining admin. Audit entries record the acting username (or - `token` for a break-glass session). Per-app ACLs and external IdPs - (OIDC/SAML/LDAP) remain Phase 8. +- **[implemented]** Role-based access control (#101/#107/#990). The nav + reflects the matrix below, but hiding a link is only UX. Coarse section + access is enforced server-side by `AdminSession` / `RequireEditor` / + `RequireAdmin`; handlers for group-scoped Apps, Containers, Users and + Groups then apply `EditorScope` to the authoritative database row. + + | Session | Admin-panel trust boundary | + |---------|----------------------------| + | **Viewer** | Portal user only; no admin-panel section. | + | **Editor** | App-config-trusted but **group-scoped**. May operate open apps and apps/replicas sharing one of the Editor's exact, case-sensitive groups; may administer non-Admin users and membership within those groups. Media remains a shared Editor resource. | + | **Admin account** | Unrestricted across the panel and all catalog rows. | + | **`RUSCKER_ADMIN_TOKEN` break-glass** | Unrestricted Admin session even when the account database is absent or damaged. | + + A role denied at the section guard receives `403`. Once an Editor is + admitted to Apps, Containers, Users or Groups, a missing **or + out-of-scope** target receives the same `404`; this is deliberate + non-disclosure, so guessed identifiers cannot reveal that another team's + app, account, group or replica exists. Scope is fetched on every request + rather than copied into the session, so membership removal takes effect on + the next request and lookup failure fails closed. + + The #990 integration tests lock down the privilege-escalation invariants: + an Editor cannot grant a group they do not have, cannot view or mutate an + Admin account, cannot assign the Admin role, and cannot change their own + role or groups through delegated user management. An Editor update replaces + only memberships inside their scope; out-of-scope groups survive unchanged. + Account deletion, CSV user import, MFA reset, and group + create/rename/delete remain Admin-only. The **last-admin guard** still + refuses to delete or demote the only remaining Admin. Audit entries record + the acting username (or `token` for break-glass). - **[implemented]** Identifier charset validation (#429 / #423) — a username (`db::users::is_valid_username`) and a stored-credential name (`routes::admin::credentials::is_valid_credential_name`) must be