From 6ed5ca1c6e95af651d36cb0e6149c057d812b732 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Leite?= Date: Wed, 29 Jul 2026 19:33:56 -0300 Subject: [PATCH 1/2] feat: scope Editor app administration by group (#990 slice 1) --- .../ruscker-admin/assets/i18n/en/landing.ftl | 1 + .../ruscker-admin/assets/i18n/es/landing.ftl | 1 + .../ruscker-admin/assets/i18n/fr/landing.ftl | 1 + .../ruscker-admin/assets/i18n/pt/landing.ftl | 1 + .../src/routes/admin/dashboard.rs | 169 +++++-- .../src/routes/admin/spec_form.rs | 202 +++++--- .../ruscker-admin/src/routes/admin/specs.rs | 197 ++++++-- crates/ruscker-admin/src/scope.rs | 10 +- crates/ruscker-admin/tests/rbac.rs | 469 +++++++++++++++++- 9 files changed, 906 insertions(+), 145 deletions(-) diff --git a/crates/ruscker-admin/assets/i18n/en/landing.ftl b/crates/ruscker-admin/assets/i18n/en/landing.ftl index f863feec..52e6303c 100644 --- a/crates/ruscker-admin/assets/i18n/en/landing.ftl +++ b/crates/ruscker-admin/assets/i18n/en/landing.ftl @@ -342,6 +342,7 @@ spec-form-error-cpu = CPU limit must be a positive number (e.g. 0.5). spec-form-error-memory = Memory limit must be a size like 512m or 1.5g. spec-form-error-replica-range = Max replicas must be greater than or equal to min replicas. spec-form-error-stale = Someone else saved this app while you were editing. Review the current values below and submit again. +spec-form-error-editor-scope = A restricted app must keep at least one of your groups and cannot include groups outside your Editor scope. # Admin image library admin-images-title = Media library diff --git a/crates/ruscker-admin/assets/i18n/es/landing.ftl b/crates/ruscker-admin/assets/i18n/es/landing.ftl index 6c5a5e4e..65e26e2e 100644 --- a/crates/ruscker-admin/assets/i18n/es/landing.ftl +++ b/crates/ruscker-admin/assets/i18n/es/landing.ftl @@ -342,6 +342,7 @@ spec-form-error-cpu = El límite de CPU debe ser un número positivo (ej.: 0.5). spec-form-error-memory = El límite de memoria debe ser un tamaño como 512m o 1.5g. spec-form-error-replica-range = Réplicas máx. debe ser mayor o igual que réplicas mín. spec-form-error-stale = Otra persona guardó esta app mientras editabas. Revisa los valores actuales abajo y envía de nuevo. +spec-form-error-editor-scope = Una app restringida debe conservar al menos uno de tus grupos y no puede incluir grupos fuera de tu alcance de Editor. # Admin image library admin-images-title = Biblioteca multimedia diff --git a/crates/ruscker-admin/assets/i18n/fr/landing.ftl b/crates/ruscker-admin/assets/i18n/fr/landing.ftl index d8980833..9a59190a 100644 --- a/crates/ruscker-admin/assets/i18n/fr/landing.ftl +++ b/crates/ruscker-admin/assets/i18n/fr/landing.ftl @@ -342,6 +342,7 @@ spec-form-error-cpu = La limite CPU doit être un nombre positif (ex. 0.5). spec-form-error-memory = La limite mémoire doit être une taille comme 512m ou 1.5g. spec-form-error-replica-range = Le max de réplicas doit être supérieur ou égal au min. spec-form-error-stale = Une autre personne a enregistré cette app pendant votre édition. Vérifiez les valeurs actuelles ci-dessous et soumettez à nouveau. +spec-form-error-editor-scope = Une app restreinte doit conserver au moins un de vos groupes et ne peut pas inclure de groupes hors de votre périmètre d'Éditeur. # Admin image library admin-images-title = Bibliothèque de médias diff --git a/crates/ruscker-admin/assets/i18n/pt/landing.ftl b/crates/ruscker-admin/assets/i18n/pt/landing.ftl index d93dfb2e..e560581f 100644 --- a/crates/ruscker-admin/assets/i18n/pt/landing.ftl +++ b/crates/ruscker-admin/assets/i18n/pt/landing.ftl @@ -346,6 +346,7 @@ spec-form-error-cpu = O limite de CPU deve ser um número positivo (ex.: 0.5). spec-form-error-memory = O limite de memória deve ser um tamanho como 512m ou 1.5g. spec-form-error-replica-range = Réplicas máx. deve ser maior ou igual a réplicas mín. spec-form-error-stale = Outra pessoa salvou este app enquanto você editava. Revise os valores atuais abaixo e envie novamente. +spec-form-error-editor-scope = Um app restrito deve manter pelo menos um dos seus grupos e não pode incluir grupos fora do seu escopo de Editor. # Admin image library admin-images-title = Biblioteca de mídia diff --git a/crates/ruscker-admin/src/routes/admin/dashboard.rs b/crates/ruscker-admin/src/routes/admin/dashboard.rs index 03f05c7d..202bc170 100644 --- a/crates/ruscker-admin/src/routes/admin/dashboard.rs +++ b/crates/ruscker-admin/src/routes/admin/dashboard.rs @@ -39,6 +39,7 @@ use dashmap::DashMap; use crate::auth::{AdminSession, RequireEditor, Role}; use crate::i18n::{Locale, Locales}; +use crate::scope::EditorScope; use crate::theme::Theme; use crate::AppState; @@ -364,7 +365,15 @@ async fn index( if !session.role.can_access_section("dashboard") { return Redirect::to("/").into_response(); } - let snap = build_snapshot(&state, loc).await; + let scope = EditorScope::from_editor( + RequireEditor { + role: session.role, + actor: session.actor, + }, + state.db.as_ref(), + ) + .await; + let snap = build_snapshot(&state, loc, &scope).await; let snapshot_json = json_for_html_script(&snap); let page = DashboardPage { locale: loc, @@ -373,7 +382,7 @@ async fn index( locales_all: &Locale::ALL, base: state.base_path.clone(), nav_section: "dashboard", - role: session.role, + role: scope.role, backend_connected: snap.backend_connected, total_containers: snap.total_containers, total_sessions: snap.total_sessions, @@ -396,18 +405,33 @@ async fn index( /// Snapshot for the dashboard + SSE, memoized per locale for one /// [`SSE_INTERVAL`] (#291) so N concurrent tabs share one build instead /// of each re-querying the DB every tick. -async fn build_snapshot(state: &AppState, locale: Locale) -> DashboardSnapshot { - if let Some(entry) = SNAPSHOT_CACHE.get(&locale) { - if entry.0.elapsed() < SSE_INTERVAL { - return (*entry.1).clone(); +async fn build_snapshot( + state: &AppState, + locale: Locale, + scope: &EditorScope, +) -> DashboardSnapshot { + // The old cache key was locale-only. Reusing it for scoped Editors + // would serve one team's rows to another, so only the truly unscoped + // Admin/break-glass view uses that shared cache. + if scope.unscoped { + if let Some(entry) = SNAPSHOT_CACHE.get(&locale) { + if entry.0.elapsed() < SSE_INTERVAL { + return (*entry.1).clone(); + } } } - let snap = build_snapshot_uncached(state, locale).await; - SNAPSHOT_CACHE.insert(locale, (Instant::now(), Arc::new(snap.clone()))); + let snap = build_snapshot_uncached(state, locale, scope).await; + if scope.unscoped { + SNAPSHOT_CACHE.insert(locale, (Instant::now(), Arc::new(snap.clone()))); + } snap } -async fn build_snapshot_uncached(state: &AppState, locale: Locale) -> DashboardSnapshot { +async fn build_snapshot_uncached( + state: &AppState, + locale: Locale, + scope: &EditorScope, +) -> DashboardSnapshot { let backend_connected = state.backend.is_some(); // Snapshot the registry once. Cloning `Replica` is cheap @@ -419,11 +443,23 @@ async fn build_snapshot_uncached(state: &AppState, locale: Locale) -> DashboardS // #202 / admin-added specs), which aren't in the YAML config. The // old config-keyed walk silently dropped every such replica from // the dashboard even though it was running. - let snap: Vec = { + let mut snap: Vec = { let reg = state.replicas.read().await; reg.all().cloned().collect() }; + let catalog = crate::catalog::effective_specs_cached(state).await; + if !scope.unscoped { + snap.retain(|replica| { + // Replica rows carry only a spec id. Missing catalog metadata is + // never equivalent to an open app: scoped Editors fail closed. + catalog + .iter() + .find(|spec| spec.id == replica.spec_id) + .is_some_and(|spec| scope.may_touch_spec(spec)) + }); + } + let total_containers = snap.len(); let total_sessions: u32 = snap.iter().map(|r| r.sessions_active).sum(); let spec_count: usize = snap @@ -431,14 +467,20 @@ async fn build_snapshot_uncached(state: &AppState, locale: Locale) -> DashboardS .map(|r| r.spec_id.as_str()) .collect::>() .len(); - let tracker_sessions = state.sessions.len(); + let tracker_sessions = if scope.unscoped { + state.sessions.len() + } else { + // SessionStore exposes only a global length. For a scoped dashboard, + // use the visible replicas' committed counts rather than leaking the + // global tracker cardinality through a KPI. + total_sessions as usize + }; // Resolve display names from the effective catalog (DB ∪ YAML, // DB-first) fetched **once** per snapshot — not a `find_spec` DB // round-trip per spec, which on the 5s SSE tick (× every open tab) // hammered the DB (#281). Still DB-first, so a renamed-in-admin spec // shows its current name (#275). - let catalog = crate::catalog::effective_specs_cached(state).await; let name_of: std::collections::HashMap = snap .iter() .map(|r| r.spec_id.clone()) @@ -585,16 +627,11 @@ fn sse_no_buffer_headers() -> [(axum::http::HeaderName, &'static str); 2] { /// `no-store` so the browser always re-polls (freshness comes from the /// server-side cache, not the HTTP cache). async fn snapshot( - session: AdminSession, + scope: EditorScope, State(state): State, loc: Locale, ) -> Response { - // Same gate as the dashboard page (#857): a Viewer never gets the - // live metrics feed. - if !session.role.can_access_section("dashboard") { - return (StatusCode::FORBIDDEN, "forbidden").into_response(); - } - let snap = build_snapshot(&state, loc).await; + let snap = build_snapshot(&state, loc, &scope).await; ( [(axum::http::header::CACHE_CONTROL, "no-store")], axum::Json(snap), @@ -614,7 +651,7 @@ async fn snapshot( async fn logs( // Container logs can carry sensitive data — gate behind Editor+ // (Viewers can see the dashboard, not the logs). #261 - editor: RequireEditor, + scope: EditorScope, State(state): State, loc: Locale, theme: Theme, @@ -625,6 +662,11 @@ async fn logs( }; let rid = ReplicaId(uuid); + let (_, scoped_spec) = match scoped_replica_spec(&state, &scope, &rid).await { + Ok(found) => found, + Err(response) => return response, + }; + let Some(backend) = state.backend.as_ref() else { return ( StatusCode::SERVICE_UNAVAILABLE, @@ -636,18 +678,10 @@ async fn logs( // Resolve display_name + spec_id for the heading. Read the spec id // from the registry under the lock, then drop it before the DB-first // name lookup (#275) so we never hold the registry lock across I/O. - let spec_id_of_replica = { - let reg = state.replicas.read().await; - let found = reg.all().find(|r| r.id == rid).map(|r| r.spec_id.clone()); - found - }; - let (display_name, spec_id) = match spec_id_of_replica { - Some(sid) => { - let dn = crate::routes::proxy::find_spec(&state, &sid) - .await - .and_then(|s| s.display_name.clone()) - .unwrap_or_else(|| sid.clone()); - (dn, sid) + let (display_name, spec_id) = match scoped_spec { + Some(spec) => { + let display_name = spec.display_name.clone().unwrap_or_else(|| spec.id.clone()); + (display_name, spec.id) } // Replica not in registry — still try to fetch logs (it may have // just been dropped from the registry but the container lingers). @@ -674,7 +708,7 @@ async fn logs( locales_all: &Locale::ALL, base: state.base_path.clone(), nav_section: "dashboard", - role: editor.role, + role: scope.role, display_name, spec_id, replica_id, @@ -693,7 +727,7 @@ async fn logs( /// Seeded with the last 100 lines so turning Live on shows /// recent context, not just lines that arrive after connect. async fn logs_stream( - _: RequireEditor, + scope: EditorScope, State(state): State, Path(replica_id): Path, ) -> Response { @@ -701,6 +735,9 @@ async fn logs_stream( Ok(r) => r, Err(resp) => return *resp, }; + if let Err(response) = scoped_replica_spec(&state, &scope, &rid).await { + return response; + } let Some(backend) = state.backend.clone() else { return (StatusCode::SERVICE_UNAVAILABLE, "no backend").into_response(); }; @@ -731,6 +768,48 @@ fn parse_replica_id(s: &str) -> Result> { .map_err(|_| Box::new((StatusCode::BAD_REQUEST, "invalid replica id").into_response())) } +/// Resolve a replica through the effective app catalog and enforce its scope. +/// +/// A scoped Editor gets 404 both when the replica/spec is missing and when +/// the spec belongs to another group. This is intentionally checked before +/// backend access so a guessed UUID cannot reveal a foreign container through +/// a 503/transport error. Admin and break-glass callers remain unrestricted, +/// including the historical best-effort access to a just-orphaned container. +async fn scoped_replica_spec( + state: &AppState, + scope: &EditorScope, + replica_id: &ReplicaId, +) -> Result<(Option, Option), Response> { + let spec_id = { + let registry = state.replicas.read().await; + let found = registry + .all() + .find(|replica| replica.id == *replica_id) + .map(|replica| replica.spec_id.clone()); + found + }; + + let Some(spec_id) = spec_id else { + return if scope.unscoped { + Ok((None, None)) + } else { + Err((StatusCode::NOT_FOUND, "replica not found").into_response()) + }; + }; + let catalog = crate::catalog::effective_specs_cached(state).await; + let spec = catalog.iter().find(|spec| spec.id == spec_id).cloned(); + if !scope.unscoped + && !spec + .as_ref() + .is_some_and(|spec| scope.may_touch_spec(spec)) + { + // Missing effective metadata also fails closed; it does not make the + // replica's app "open". + return Err((StatusCode::NOT_FOUND, "replica not found").into_response()); + } + Ok((Some(spec_id), spec)) +} + /// POST `/admin/dashboard/replicas/{id}/stop` — stop a replica /// and drop it from the registry. The auto-scaler will respawn /// it to `min-replicas` on its next tick if the spec demands a @@ -741,7 +820,7 @@ fn parse_replica_id(s: &str) -> Result> { /// Redirects back to the dashboard so the browser lands on a /// fresh render. async fn stop_replica( - editor: RequireEditor, + scope: EditorScope, State(state): State, Path(replica_id): Path, ) -> Response { @@ -749,6 +828,9 @@ async fn stop_replica( Ok(r) => r, Err(resp) => return *resp, }; + if let Err(response) = scoped_replica_spec(&state, &scope, &rid).await { + return response; + } let Some(backend) = state.backend.as_ref() else { return (StatusCode::SERVICE_UNAVAILABLE, "no backend").into_response(); }; @@ -765,7 +847,7 @@ async fn stop_replica( state.sessions.drop_replica(&rid).await; // Destructive operational action → audit row (#745); config // mutations were audited, replica stop/restart only hit the log. - record_replica_action(&state, editor.actor(), "replica.stop", &replica_id).await; + record_replica_action(&state, scope.actor(), "replica.stop", &replica_id).await; tracing::info!(replica = %replica_id, "replica stopped via dashboard"); Redirect::to("/admin/dashboard").into_response() } @@ -787,7 +869,7 @@ async fn record_replica_action(state: &AppState, actor: &str, action: &str, repl /// respawn ~one tick later), restart brings capacity back /// right away. async fn restart_replica( - editor: RequireEditor, + scope: EditorScope, State(state): State, Path(replica_id): Path, ) -> Response { @@ -795,6 +877,10 @@ async fn restart_replica( Ok(r) => r, Err(resp) => return *resp, }; + let (spec_id, scoped_spec) = match scoped_replica_spec(&state, &scope, &rid).await { + Ok(found) => found, + Err(response) => return response, + }; let Some(backend) = state.backend.as_ref() else { return (StatusCode::SERVICE_UNAVAILABLE, "no backend").into_response(); }; @@ -802,17 +888,12 @@ async fn restart_replica( // Resolve the spec before stopping so a restart of a // since-deleted spec fails cleanly without first killing // the running container. - let spec_id = { - let reg = state.replicas.read().await; - let found = reg.all().find(|r| r.id == rid).map(|r| r.spec_id.clone()); - found - }; let Some(spec_id) = spec_id else { return (StatusCode::NOT_FOUND, "replica not found").into_response(); }; // DB-first (admin edits + showcase seed), not just the YAML config — // otherwise a DB-only spec's replica couldn't be restarted (#257). - let Some(spec) = crate::routes::proxy::find_spec(&state, &spec_id).await else { + let Some(spec) = scoped_spec else { return ( StatusCode::CONFLICT, format!("spec `{spec_id}` no longer exists; cannot restart"), @@ -836,7 +917,7 @@ async fn restart_replica( ) .into_response(); } - record_replica_action(&state, editor.actor(), "replica.restart", &replica_id).await; + record_replica_action(&state, scope.actor(), "replica.restart", &replica_id).await; tracing::info!(replica = %replica_id, spec = %spec.id, "replica restarted via dashboard"); Redirect::to("/admin/dashboard").into_response() } diff --git a/crates/ruscker-admin/src/routes/admin/spec_form.rs b/crates/ruscker-admin/src/routes/admin/spec_form.rs index 066041aa..ca40e9ae 100644 --- a/crates/ruscker-admin/src/routes/admin/spec_form.rs +++ b/crates/ruscker-admin/src/routes/admin/spec_form.rs @@ -28,9 +28,10 @@ use serde::{Deserialize, Serialize}; use serde_yaml_ng::Value as YamlValue; use std::collections::HashMap; -use crate::auth::{RequireEditor, Role}; +use crate::auth::Role; use crate::db; use crate::i18n::{Locale, Locales}; +use crate::scope::EditorScope; use crate::theme::Theme; use crate::view_model::DisplayType; use crate::AppState; @@ -78,7 +79,7 @@ struct ImageCheckResult { /// (#498). Pull-free and Editor-gated; a quick yes/no, no registry round /// trip (that's slice B's explicit Pull button). async fn image_check( - _: RequireEditor, + _scope: EditorScope, State(state): State, Query(q): Query, ) -> Json { @@ -166,7 +167,7 @@ const PULL_JOB_TTL_SECS: u64 = 300; /// editor then opens an EventSource on `…/image-pull/events?job=`. /// Editor-gated; the POST inherits the chrome CSRF (Fetch-Metadata) guard. async fn image_pull_start( - _: RequireEditor, + _scope: EditorScope, State(state): State, Form(q): Form, ) -> Response { @@ -277,7 +278,7 @@ async fn start_pull( /// `{ "job": "" }` for the row to follow over SSE. Editor-gated; /// inherits the chrome CSRF guard. async fn image_repull( - _: RequireEditor, + scope: EditorScope, State(state): State, Path(id): Path, ) -> Response { @@ -292,6 +293,11 @@ async fn image_repull( return (StatusCode::INTERNAL_SERVER_ERROR, "db error").into_response(); } }; + // A direct re-pull URL is an operation on the app, not on the shared + // media library. Hide a foreign app exactly like the edit/delete routes. + if !scope.may_touch_spec(&spec) { + return (StatusCode::NOT_FOUND, "no such app").into_response(); + } let Some(image) = spec .container_image .as_deref() @@ -330,7 +336,7 @@ struct PullEventsQuery { /// already-running pull's progress over SSE (default events = lines, then /// one terminal `done` event), then drops the job. An unknown or /// already-followed token ⇒ 404. Editor-gated (RBAC preserved). -async fn image_pull_events(_: RequireEditor, Query(q): Query) -> Response { +async fn image_pull_events(_scope: EditorScope, Query(q): Query) -> Response { use axum::http::header::{HeaderName, CACHE_CONTROL}; use axum::response::sse::{Event, KeepAlive, Sse}; @@ -393,7 +399,7 @@ async fn resolve_pull_creds( /// Mirror of the form fields. Strings are unconditional so empty /// inputs round-trip as `""` rather than disappearing; conversion /// to [`Spec`] handles "empty means None". -#[derive(Debug, Default, Serialize, Deserialize)] +#[derive(Clone, Debug, Default, Serialize, Deserialize)] #[serde(default)] pub struct SpecForm { pub id: String, @@ -1290,10 +1296,11 @@ impl<'a> SpecFormPage<'a> { /// wired or the query fails — the picker degrades to the text field. /// Distinct, sorted `template-properties.subject` values across the /// effective catalog (#746) — real data instead of a fixed list. -async fn subject_suggestions(state: &AppState) -> Vec { +async fn subject_suggestions(state: &AppState, scope: &EditorScope) -> Vec { let specs = crate::catalog::effective_specs(state.db.as_ref(), &state.config).await; let mut subjects: Vec = specs .iter() + .filter(|spec| scope.may_touch_spec(spec)) .filter_map(|sp| sp.template_properties.get_str("subject")) .map(|t| t.trim().to_string()) .filter(|t| !t.is_empty()) @@ -1332,7 +1339,7 @@ async fn credential_names(state: &AppState) -> Vec { /// every effective spec's `access-groups` and every user's memberships, /// sorted and de-duplicated. Empty when no DB is wired — the picker then /// just offers the "add group" input. -async fn group_names(state: &AppState) -> Vec { +async fn group_names(state: &AppState, scope: &EditorScope) -> Vec { let mut set = std::collections::BTreeSet::new(); for s in crate::catalog::effective_specs(state.db.as_ref(), &state.config).await { if let Some(groups) = s.access_groups.as_ref() { @@ -1346,11 +1353,15 @@ async fn group_names(state: &AppState) -> Vec { } } } - set.into_iter().collect() + let known: Vec = set.into_iter().collect(); + // The picker must not advertise another team's vocabulary. Reuse the + // scope merge primitive with an empty base to retain only assignable + // groups for Editors; Admins receive the full set unchanged. + scope.merge_preserving_out_of_scope(&[], &known) } async fn new_form( - editor: RequireEditor, + scope: EditorScope, State(state): State, loc: Locale, theme: Theme, @@ -1362,7 +1373,7 @@ async fn new_form( locales_all: &Locale::ALL, base: state.base_path.clone(), nav_section: "specs", - role: editor.role, + role: scope.role, mode: FormMode::New, form: SpecForm { // Sensible defaults for a new app @@ -1375,8 +1386,8 @@ async fn new_form( just_created: false, errors: Vec::new(), logo_images: logo_filenames(&state).await, - subject_suggestions: subject_suggestions(&state).await, - available_groups: group_names(&state).await, + subject_suggestions: subject_suggestions(&state, &scope).await, + available_groups: group_names(&state, &scope).await, credential_names: credential_names(&state).await, }; super::render(&page) @@ -1390,7 +1401,7 @@ struct EditFormQuery { } async fn edit_form( - editor: RequireEditor, + scope: EditorScope, State(state): State, loc: Locale, theme: Theme, @@ -1408,6 +1419,11 @@ async fn edit_form( return (StatusCode::INTERNAL_SERVER_ERROR, "db error").into_response(); } }; + // List filtering is not authorization. A foreign app deliberately + // returns 404 so its id is not disclosed through a direct URL (#990). + if !scope.may_touch_spec(&spec) { + return (StatusCode::NOT_FOUND, "spec not found").into_response(); + } let page = SpecFormPage { locale: loc, theme, @@ -1415,10 +1431,15 @@ async fn edit_form( locales_all: &Locale::ALL, base: state.base_path.clone(), nav_section: "specs", - role: editor.role, + role: scope.role, mode: FormMode::Edit, form: { let mut f = SpecForm::from_spec(&spec); + let visible_groups = scope.merge_preserving_out_of_scope( + &[], + spec.access_groups.as_deref().unwrap_or_default(), + ); + f.access_groups = visible_groups.join(", "); // Optimistic-concurrency token (#745): the template emits it // as a hidden field; `update` rejects a stale submit. f.base_version = db::specs::fetch_version(pool, &id) @@ -1432,8 +1453,8 @@ async fn edit_form( just_created: q.created.is_some(), errors: Vec::new(), logo_images: logo_filenames(&state).await, - subject_suggestions: subject_suggestions(&state).await, - available_groups: group_names(&state).await, + subject_suggestions: subject_suggestions(&state, &scope).await, + available_groups: group_names(&state, &scope).await, credential_names: credential_names(&state).await, }; super::render(&page) @@ -1480,7 +1501,7 @@ fn pick_copy_id(base: &str, taken: &std::collections::HashSet) -> String /// a handy way to fork a YAML-defined spec into an editable DB one. The /// registry password is write-only (#260), so it isn't carried over. async fn duplicate_form( - editor: RequireEditor, + scope: EditorScope, State(state): State, loc: Locale, theme: Theme, @@ -1510,7 +1531,18 @@ async fn duplicate_form( return (StatusCode::INTERNAL_SERVER_ERROR, "db error").into_response(); } }; + // Duplicating is still an operation on the source app. Return 404 at + // the boundary so a guessed foreign id is indistinguishable from one + // that does not exist. + if !scope.may_touch_spec(&spec) { + return (StatusCode::NOT_FOUND, "spec not found").into_response(); + } let mut form = SpecForm::from_spec(&spec); + let visible_groups = scope.merge_preserving_out_of_scope( + &[], + spec.access_groups.as_deref().unwrap_or_default(), + ); + form.access_groups = visible_groups.join(", "); form.id = unique_copy_id(&state, &spec.id).await; let page = SpecFormPage { locale: loc, @@ -1519,21 +1551,21 @@ async fn duplicate_form( locales_all: &Locale::ALL, base: state.base_path.clone(), nav_section: "specs", - role: editor.role, + role: scope.role, mode: FormMode::New, form, just_created: false, errors: Vec::new(), logo_images: logo_filenames(&state).await, - subject_suggestions: subject_suggestions(&state).await, - available_groups: group_names(&state).await, + subject_suggestions: subject_suggestions(&state, &scope).await, + available_groups: group_names(&state, &scope).await, credential_names: credential_names(&state).await, }; super::render(&page) } async fn create( - editor: RequireEditor, + scope: EditorScope, State(state): State, loc: Locale, theme: Theme, @@ -1545,6 +1577,20 @@ async fn create( let mut errors = form.validate(FormMode::New); + if errors.is_empty() { + let candidate = match form.clone().into_spec(None, scope.role) { + Ok(spec) => spec, + Err(e) => { + tracing::error!(error = ?e, "form → spec failed during scope validation"); + return (StatusCode::BAD_REQUEST, "invalid form data").into_response(); + } + }; + let requested = candidate.access_groups.as_deref().unwrap_or_default(); + if !scope.may_assign_groups(requested) || !scope.may_touch_spec(&candidate) { + errors.push("spec-form-error-editor-scope"); + } + } + // Uniqueness check if errors.is_empty() { match db::specs::fetch_one(pool, form.id.trim()).await { @@ -1562,7 +1608,7 @@ async fn create( &state, loc, theme, - editor.role, + &scope, FormMode::New, form, errors, @@ -1571,7 +1617,7 @@ async fn create( } let id = form.id.trim().to_string(); - let spec = match form.into_spec(None, editor.role) { + let spec = match form.into_spec(None, scope.role) { Ok(s) => s, Err(e) => { tracing::error!(error = ?e, "form → spec failed"); @@ -1582,7 +1628,7 @@ async fn create( // insert_new fails CLOSED on an existing id (#745) — the friendly // pre-check above is just UX; this is the race-proof gate (the old // upsert silently overwrote the loser of a concurrent create). - match db::specs::insert_new(pool, &spec, Some(editor.actor())).await { + match db::specs::insert_new(pool, &spec, Some(scope.actor())).await { // Land on the new app's edit form with `?created=1` so the page // shows the post-create confirmation (#835): keep editing here, or // jump to the apps list. @@ -1594,7 +1640,7 @@ async fn create( &state, loc, theme, - editor.role, + &scope, FormMode::New, SpecForm::from_spec(&spec), vec!["spec-form-error-id-duplicate"], @@ -1609,7 +1655,7 @@ async fn create( } async fn update( - editor: RequireEditor, + scope: EditorScope, State(state): State, loc: Locale, theme: Theme, @@ -1625,13 +1671,57 @@ async fn update( // log target). Renaming is a separate planned action. form.id = id.clone(); - let errors = form.validate(FormMode::Edit); + // Load the existing spec as the merge base so fields the form + // doesn't model (registry creds, lifetimes, limits, scaling, custom + // template-properties) survive the edit instead of being wiped. + let base = match db::specs::fetch_one(pool, &id).await { + Ok(Some(base)) => base, + Ok(None) => { + return (StatusCode::NOT_FOUND, format!("spec `{id}` not found")).into_response(); + } + Err(e) => { + tracing::error!(error = ?e, id, "load base spec failed"); + return (StatusCode::INTERNAL_SERVER_ERROR, "db error").into_response(); + } + }; + // This is the *edit* path — the spec must already exist (#261). Check + // scope before validation so even a malformed crafted POST cannot use + // response differences to confirm an out-of-scope app id (#990). + if !scope.may_touch_spec(&base) { + return (StatusCode::NOT_FOUND, "spec not found").into_response(); + } + + let mut errors = form.validate(FormMode::Edit); + let mut candidate = if errors.is_empty() { + match form.clone().into_spec(Some(&base), scope.role) { + Ok(spec) => Some(spec), + Err(e) => { + tracing::error!(error = ?e, "form → spec failed during scope validation"); + return (StatusCode::BAD_REQUEST, "invalid form data").into_response(); + } + } + } else { + None + }; + if let Some(spec) = candidate.as_mut() { + let requested = spec.access_groups.clone().unwrap_or_default(); + if !scope.may_assign_groups(&requested) { + errors.push("spec-form-error-editor-scope"); + } else { + let existing = base.access_groups.as_deref().unwrap_or_default(); + let merged = scope.merge_preserving_out_of_scope(existing, &requested); + spec.access_groups = (!merged.is_empty()).then_some(merged); + if !scope.may_touch_spec(spec) { + errors.push("spec-form-error-editor-scope"); + } + } + } if !errors.is_empty() { return render_form_with_errors( &state, loc, theme, - editor.role, + &scope, FormMode::Edit, form, errors, @@ -1639,24 +1729,6 @@ async fn update( .await; } - // Load the existing spec as the merge base so fields the form - // doesn't model (registry creds, lifetimes, limits, scaling, custom - // template-properties) survive the edit instead of being wiped. - let base = match db::specs::fetch_one(pool, &id).await { - Ok(b) => b, - Err(e) => { - tracing::error!(error = ?e, id, "load base spec failed"); - return (StatusCode::INTERNAL_SERVER_ERROR, "db error").into_response(); - } - }; - // This is the *edit* path — the spec must already exist. Without - // this guard `into_spec(None)` + `upsert_one` would silently - // (re)create a spec at this id (e.g. a stale tab POSTing to a - // since-deleted spec). #261 - if base.is_none() { - return (StatusCode::NOT_FOUND, format!("spec `{id}` not found")).into_response(); - } - // Optimistic concurrency (#745): the form carries the version it // was rendered against; if someone saved meanwhile, re-render with // a conflict error instead of silently last-write-winning over @@ -1671,7 +1743,7 @@ async fn update( &state, loc, theme, - editor.role, + &scope, FormMode::Edit, stale, vec!["spec-form-error-stale"], @@ -1680,15 +1752,9 @@ async fn update( } } - let spec = match form.into_spec(base.as_ref(), editor.role) { - Ok(s) => s, - Err(e) => { - tracing::error!(error = ?e, "form → spec failed"); - return (StatusCode::BAD_REQUEST, "invalid form data").into_response(); - } - }; + let spec = candidate.expect("validated form always builds a candidate spec"); - match db::specs::upsert_one(pool, &spec, Some(editor.actor())).await { + match db::specs::upsert_one(pool, &spec, Some(scope.actor())).await { Ok(_) => Redirect::to(&format!("/admin/specs/{}/edit", id)).into_response(), Err(e) => { tracing::error!(error = ?e, "save failed"); @@ -1698,14 +1764,26 @@ async fn update( } async fn delete( - editor: RequireEditor, + scope: EditorScope, State(state): State, Path(id): Path, ) -> Response { let Some(pool) = state.db.as_ref() else { return (StatusCode::SERVICE_UNAVAILABLE, "no db").into_response(); }; - match db::specs::delete_one(pool, &id, Some(editor.actor())).await { + let spec = match db::specs::fetch_one(pool, &id).await { + Ok(Some(spec)) => spec, + Ok(None) => return (StatusCode::NOT_FOUND, "spec not found").into_response(), + Err(e) => { + tracing::error!(error = ?e, id, "load spec for delete failed"); + return (StatusCode::INTERNAL_SERVER_ERROR, "db error").into_response(); + } + }; + if !scope.may_touch_spec(&spec) { + // 404 avoids disclosing that a guessed foreign id exists. + return (StatusCode::NOT_FOUND, "spec not found").into_response(); + } + match db::specs::delete_one(pool, &id, Some(scope.actor())).await { Ok(_) => { // Reap the app's containers so a delete doesn't leave orphans // eating disk (#453). Best-effort and logged inside; the DB row @@ -1724,7 +1802,7 @@ async fn render_form_with_errors( state: &AppState, loc: Locale, theme: Theme, - role: Role, + scope: &EditorScope, mode: FormMode, form: SpecForm, errors: Vec<&'static str>, @@ -1736,14 +1814,14 @@ async fn render_form_with_errors( locales_all: &Locale::ALL, base: state.base_path.clone(), nav_section: "specs", - role, + role: scope.role, mode, form, just_created: false, errors, logo_images: logo_filenames(state).await, - subject_suggestions: subject_suggestions(state).await, - available_groups: group_names(state).await, + subject_suggestions: subject_suggestions(state, scope).await, + available_groups: group_names(state, scope).await, credential_names: credential_names(state).await, }; let body = match page.render() { diff --git a/crates/ruscker-admin/src/routes/admin/specs.rs b/crates/ruscker-admin/src/routes/admin/specs.rs index 4d1690a5..6b179b20 100644 --- a/crates/ruscker-admin/src/routes/admin/specs.rs +++ b/crates/ruscker-admin/src/routes/admin/specs.rs @@ -16,8 +16,9 @@ use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; use sqlx::FromRow; -use crate::auth::{RequireEditor, Role}; +use crate::auth::Role; use crate::i18n::{Locale, Locales}; +use crate::scope::EditorScope; use crate::theme::Theme; use crate::AppState; @@ -51,7 +52,7 @@ struct ToggleResult { /// flag from the Apps table's star (#521), without opening the editor. /// Editor-gated; returns the new state as JSON for the optimistic UI. async fn toggle_featured( - editor: RequireEditor, + scope: EditorScope, State(state): State, Path(id): Path, ) -> Response { @@ -66,10 +67,15 @@ async fn toggle_featured( return (StatusCode::INTERNAL_SERVER_ERROR, "db error").into_response(); } }; + // Out-of-scope specs deliberately look absent: returning 403 here would + // confirm an app id that the Editor is not allowed to discover (#990). + if !scope.may_touch_spec(&spec) { + return (StatusCode::NOT_FOUND, "spec not found").into_response(); + } let now_featured = !spec.is_featured(); // None when off so a normal spec carries no `featured` noise in JSON. spec.featured = now_featured.then_some(true); - if let Err(e) = crate::db::specs::upsert_one(db, &spec, Some(editor.actor())).await { + if let Err(e) = crate::db::specs::upsert_one(db, &spec, Some(scope.actor())).await { tracing::error!(id, error = ?e, "save featured toggle failed"); return (StatusCode::INTERNAL_SERVER_ERROR, "save failed").into_response(); } @@ -95,7 +101,7 @@ struct StateToggleResult { /// toggle (#787 — the original form POST + redirect reloaded the page /// and threw the scroll back to the top). async fn toggle_state( - editor: RequireEditor, + scope: EditorScope, State(state): State, Path(id): Path, ) -> Response { @@ -110,13 +116,18 @@ async fn toggle_state( return (StatusCode::INTERNAL_SERVER_ERROR, "db error").into_response(); } }; + // Hide existence across Editor group boundaries; list filtering is only + // presentation and direct URLs must be protected independently (#990). + if !scope.may_touch_spec(&spec) { + return (StatusCode::NOT_FOUND, "spec not found").into_response(); + } let next_active = !spec.template_properties.is_active(); // `set_state`, not `upsert_one` (#780): the list sorts by // `updated_at` DESC and the upsert path stamps it, so archiving made // the row jump to the top of the table. The dedicated writer leaves // `updated_at` alone (a visibility flip is not a content edit) while // still bumping the version and auditing archive/unarchive. - if let Err(e) = crate::db::specs::set_state(db, &id, next_active, Some(editor.actor())).await { + if let Err(e) = crate::db::specs::set_state(db, &id, next_active, Some(scope.actor())).await { tracing::error!(id, error = ?e, "save state toggle failed"); return (StatusCode::INTERNAL_SERVER_ERROR, "save failed").into_response(); } @@ -261,8 +272,11 @@ struct ImportRow { async fn build_import_rows( pool: &crate::db::ConfigDb, raw: &str, + scope: &EditorScope, + effective_specs: &[ruscker_config::Spec], ) -> Result<(Vec, usize), String> { - let config = ruscker_config::Config::from_yaml(raw).map_err(|e| format!("{e}"))?; + let config = ruscker_config::Config::from_yaml(raw) + .map_err(|e| format!("YAML parse failed: {e}"))?; let report = ruscker_config::validate::run(&config); let warning_count = report.warnings.len() + config.raw_warnings.len(); let existing: std::collections::HashSet = crate::db::specs::list_all(pool) @@ -271,25 +285,101 @@ async fn build_import_rows( .into_iter() .map(|s| s.id) .collect(); - let rows = config - .proxy - .specs - .iter() - .map(|s| ImportRow { - exists: existing.contains(&s.id), - kind: match s.kind() { + let mut rows = Vec::with_capacity(config.proxy.specs.len()); + for spec in &config.proxy.specs { + if existing.contains(&spec.id) { + // A persisted id that disappeared from the effective catalog is + // not an open app. Scoped Editors fail closed because the lean + // existence query carries no ACL metadata (#990). + match effective_specs.iter().find(|current| current.id == spec.id) { + Some(current) if !scope.may_touch_spec(current) => { + return Err(format!("app `{}` is outside your Editor scope", spec.id)); + } + None if !scope.unscoped => { + return Err(format!("app `{}` is outside your Editor scope", spec.id)); + } + _ => {} + } + } else { + let requested = spec.access_groups.as_deref().unwrap_or_default(); + if !scope.may_assign_groups(requested) + || (!spec.is_open() && !scope.may_touch_spec(spec)) + { + return Err(format!( + "restricted app `{}` must use only groups in your Editor scope", + spec.id + )); + } + } + rows.push(ImportRow { + exists: existing.contains(&spec.id), + kind: match spec.kind() { ruscker_config::SpecKind::Shiny => "shiny", ruscker_config::SpecKind::InteractiveApp => "interactive", ruscker_config::SpecKind::Api => "api", ruscker_config::SpecKind::External => "external", }, - display_name: s.display_name.clone().unwrap_or_default(), - id: s.id.clone(), - }) - .collect(); + display_name: spec.display_name.clone().unwrap_or_default(), + id: spec.id.clone(), + }); + } Ok((rows, warning_count)) } +/// Apply Editor scope to the selected YAML specs immediately before import. +/// +/// The preview is only UX; a crafted confirm POST must pass this same +/// server-side gate. Existing foreign memberships are retained through the +/// slice-0 merge primitive, while a new/restricted app must stay reachable +/// to its creator and may name only groups the Editor owns. +fn prepare_scoped_import( + config: &mut ruscker_config::Config, + ids: &[String], + persisted_ids: &std::collections::HashSet, + effective_specs: &[ruscker_config::Spec], + scope: &EditorScope, +) -> Result<(), String> { + use std::collections::HashSet; + + let selected: HashSet<&str> = ids.iter().map(String::as_str).collect(); + for spec in config + .proxy + .specs + .iter_mut() + .filter(|spec| selected.contains(spec.id.as_str())) + { + let current = effective_specs.iter().find(|current| current.id == spec.id); + if persisted_ids.contains(&spec.id) && current.is_none() && !scope.unscoped { + // A DB row without effective ACL metadata is not a new/open app. + // Fail closed before import_selected can overwrite it. + return Err(format!("app `{}` is outside your Editor scope", spec.id)); + } + if current.is_some_and(|current| !scope.may_touch_spec(current)) { + return Err(format!("app `{}` is outside your Editor scope", spec.id)); + } + + let requested = spec.access_groups.clone().unwrap_or_default(); + if !scope.may_assign_groups(&requested) { + return Err(format!( + "restricted app `{}` may only use groups in your Editor scope", + spec.id + )); + } + if let Some(current) = current { + let existing = current.access_groups.as_deref().unwrap_or_default(); + let merged = scope.merge_preserving_out_of_scope(existing, &requested); + spec.access_groups = (!merged.is_empty()).then_some(merged); + } + if !spec.is_open() && !scope.may_touch_spec(spec) { + return Err(format!( + "restricted app `{}` must retain at least one of your groups", + spec.id + )); + } + } + Ok(()) +} + /// JSON shape returned by `POST /admin/specs/import/preview` for the live /// editor: either an error (parse failure) or the parsed rows + warnings. #[derive(Serialize)] @@ -394,7 +484,7 @@ fn build_flash(locales: &Locales, loc: Locale, q: &SpecsQuery) -> Option } async fn index( - editor: RequireEditor, + scope: EditorScope, State(state): State, loc: Locale, theme: Theme, @@ -444,9 +534,9 @@ async fn index( // Card logo + access-groups per spec for the table (#623). The effective // catalog deserializes each spec once — fine here (admin page, not the // proxy/dashboard hot path the lean SELECT of #588 protects). + let catalog = crate::catalog::effective_specs_cached(&state).await; let meta: std::collections::HashMap, Vec, bool)> = - crate::catalog::effective_specs_cached(&state) - .await + catalog .iter() .map(|s| { let logo = s.template_properties.get_str("logo").map(str::to_string); @@ -457,6 +547,19 @@ async fn index( }) .collect(); + if !scope.unscoped { + specs.retain(|row| { + // IMPORTANT: never authorize against the enriched `SpecRow`. + // A lean-SELECT row missing from the effective catalog has empty + // `access_groups` and would look open. A scoped Editor therefore + // fails closed unless the id resolves to the authoritative Spec. + catalog + .iter() + .find(|spec| spec.id == row.id) + .is_some_and(|spec| scope.may_touch_spec(spec)) + }); + } + // Per-row access total + sparkline (#549). `featured` already came from // the lean SELECT (#588), so there's no second config_json deserialize. for row in &mut specs { @@ -481,6 +584,16 @@ async fn index( if db_ids.contains(&s.id) { continue; } + if !scope.unscoped + && !catalog + .iter() + .find(|spec| spec.id == s.id) + .is_some_and(|spec| scope.may_touch_spec(spec)) + { + // Missing effective metadata also fails closed here. Treating + // the row's empty fallback ACL as "open" would leak it. + continue; + } let kind = match s.kind() { ruscker_config::SpecKind::Shiny => "shiny", ruscker_config::SpecKind::InteractiveApp => "interactive", @@ -527,7 +640,7 @@ async fn index( locales_all: &Locale::ALL, base: state.base_path.clone(), nav_section: "specs", - role: editor.role, + role: scope.role, specs, kpi_total, kpi_active, @@ -553,7 +666,7 @@ async fn index( /// with a checkbox — so the operator confirms which to import. Nothing is /// written to the DB here. async fn import( - editor: RequireEditor, + scope: EditorScope, State(state): State, loc: Locale, theme: Theme, @@ -588,10 +701,12 @@ async fn import( }; // Parse + diff via the shared core (#623). Parse failure → error flash. - let (rows, warning_count) = match build_import_rows(pool, &raw).await { - Ok(v) => v, - Err(e) => return redirect_err(&format!("YAML parse failed: {e}")), - }; + let effective_specs = crate::catalog::effective_specs_cached(&state).await; + let (rows, warning_count) = + match build_import_rows(pool, &raw, &scope, &effective_specs).await { + Ok(v) => v, + Err(e) => return redirect_err(&e), + }; let page = ImportPreviewPage { locale: loc, @@ -600,7 +715,7 @@ async fn import( locales_all: &Locale::ALL, base: state.base_path.clone(), nav_section: "specs", - role: editor.role, + role: scope.role, rows, raw_yaml: raw, warning_count, @@ -610,7 +725,7 @@ async fn import( /// `GET /admin/specs/import` — the live 2-pane import editor (#623). async fn import_editor( - editor: RequireEditor, + scope: EditorScope, State(state): State, loc: Locale, theme: Theme, @@ -622,7 +737,7 @@ async fn import_editor( locales_all: &Locale::ALL, base: state.base_path.clone(), nav_section: "specs", - role: editor.role, + role: scope.role, }; super::render(&page) } @@ -637,7 +752,7 @@ struct PreviewForm { } async fn import_preview( - _: RequireEditor, + scope: EditorScope, State(state): State, axum::Form(form): axum::Form, ) -> Response { @@ -655,7 +770,8 @@ async fn import_preview( }) .into_response(); } - match build_import_rows(pool, &form.yaml).await { + let effective_specs = crate::catalog::effective_specs_cached(&state).await; + match build_import_rows(pool, &form.yaml, &scope, &effective_specs).await { Ok((rows, warning_count)) => { let update_count = rows.iter().filter(|r| r.exists).count(); let new_count = rows.len() - update_count; @@ -685,7 +801,7 @@ async fn import_preview( /// preview form re-posts the original YAML (hidden) plus one `ids` field /// per selected spec, as multipart so repeated `ids` collect cleanly. async fn import_confirm( - editor: RequireEditor, + scope: EditorScope, State(state): State, mut multipart: Multipart, ) -> Response { @@ -733,15 +849,28 @@ async fn import_confirm( Ok(c) => c, Err(e) => return redirect_err(&format!("YAML parse failed: {e}")), }; + let effective_specs = crate::catalog::effective_specs_cached(&state).await; + let persisted_ids = match crate::db::specs::list_all(pool).await { + Ok(specs) => specs.into_iter().map(|spec| spec.id).collect(), + Err(error) => { + tracing::error!(error = ?error, "load catalog ids for scoped import failed"); + return redirect_err("could not verify Editor scope"); + } + }; + if let Err(error) = + prepare_scoped_import(&mut config, &ids, &persisted_ids, &effective_specs, &scope) + { + return redirect_err(&error); + } // #560 B: lift inline registry passwords into the encrypted credential // store and rewire the selected specs to reference them by name, so the // plaintext never lands in `config_json`. Runs before the spec upsert // (it mutates the specs that get imported). Best-effort. - let creds = extract_inline_credentials(&state, &mut config, &ids, editor.actor()).await; + let creds = extract_inline_credentials(&state, &mut config, &ids, scope.actor()).await; match crate::db::specs::import_selected(pool, &config, &ids).await { Ok(r) => { // #560 A: bring the specs' --images-dir logos into the Media library. - let logos = import_referenced_logos(&state, &config, &ids, editor.actor()).await; + let logos = import_referenced_logos(&state, &config, &ids, scope.actor()).await; tracing::info!( created = r.created, updated = r.updated, unchanged = r.unchanged, selected = ids.len(), credentials = creds, logos = logos, diff --git a/crates/ruscker-admin/src/scope.rs b/crates/ruscker-admin/src/scope.rs index c268ca9f..5f761aa9 100644 --- a/crates/ruscker-admin/src/scope.rs +++ b/crates/ruscker-admin/src/scope.rs @@ -39,7 +39,7 @@ pub struct EditorScope { } impl EditorScope { - async fn from_editor(editor: RequireEditor, db: Option<&ConfigDb>) -> Self { + pub(crate) async fn from_editor(editor: RequireEditor, db: Option<&ConfigDb>) -> Self { if editor.role == Role::Admin { return Self { role: editor.role, @@ -73,6 +73,14 @@ impl EditorScope { } } + /// Audit actor carried by the authenticated session. + /// + /// The break-glass token has no account username, so it keeps the same + /// stable `token` label used by [`RequireEditor::actor`]. + pub fn actor(&self) -> &str { + self.actor.as_deref().unwrap_or("token") + } + fn has_group(&self, group: &str) -> bool { self.groups.iter().any(|owned| owned == group) } diff --git a/crates/ruscker-admin/tests/rbac.rs b/crates/ruscker-admin/tests/rbac.rs index 216aaad9..413c3b21 100644 --- a/crates/ruscker-admin/tests/rbac.rs +++ b/crates/ruscker-admin/tests/rbac.rs @@ -11,11 +11,15 @@ //! before the handler runs, and an *unauthenticated* request is //! redirected to the login form. We assert on those three shapes. -use axum::body::Body; +use axum::body::{to_bytes, Body}; use axum::http::{Request, StatusCode}; +use axum::response::Response; +use chrono::Utc; use ruscker_admin::auth::{AdminAuth, Role, COOKIE_NAME}; use ruscker_admin::{router, AppState}; -use ruscker_config::Config; +use ruscker_config::{Config, Spec}; +use ruscker_core::{Replica, ReplicaId, ReplicaState}; +use std::net::SocketAddr; use std::sync::Arc; use tower::ServiceExt; @@ -77,13 +81,146 @@ async fn cookie_for(state: &AppState, role: Role) -> String { } async fn send(state: AppState, method: &str, uri: &str, cookie: Option<&str>) -> StatusCode { + send_request(state, method, uri, cookie, Body::empty(), None) + .await + .status() +} + +async fn send_request( + state: AppState, + method: &str, + uri: &str, + cookie: Option<&str>, + body: Body, + content_type: Option<&str>, +) -> Response { let app = router(state); let mut builder = Request::builder().method(method).uri(uri); if let Some(c) = cookie { builder = builder.header("cookie", c); } - let req = builder.body(Body::empty()).unwrap(); - app.oneshot(req).await.unwrap().status() + if let Some(value) = content_type { + builder = builder.header("content-type", value); + } + let req = builder.body(body).unwrap(); + app.oneshot(req).await.unwrap() +} + +async fn response_body(response: Response) -> String { + let bytes = to_bytes(response.into_body(), 2 * 1024 * 1024) + .await + .expect("read response body"); + String::from_utf8(bytes.to_vec()).expect("response body is utf-8") +} + +const TIME_A_REPLICA: &str = "aaaaaaaa-1111-4111-8111-111111111111"; +const TIME_B_REPLICA: &str = "bbbbbbbb-2222-4222-8222-222222222222"; +const OPEN_REPLICA: &str = "cccccccc-3333-4333-8333-333333333333"; + +fn replica(id: &str, spec_id: &str) -> Replica { + Replica { + id: ReplicaId(uuid::Uuid::parse_str(id).expect("valid replica id")), + spec_id: spec_id.to_string(), + container_id: format!("container-{spec_id}"), + upstream: "127.0.0.1:3838" + .parse::() + .expect("valid upstream"), + state: ReplicaState::Ready, + started_at: Utc::now(), + sessions_active: 0, + sessions_max: 1, + host: None, + } +} + +fn app(yaml: &str) -> Spec { + serde_yaml_ng::from_str(yaml).expect("parse scoped test app") +} + +/// Real SQLite catalog + real opaque account session for the #990 scope +/// integration tests. `time-a` deliberately also carries `legacy-ops`: +/// the Editor shares one group and may edit it, but must not erase the +/// foreign membership that is hidden from their form. +async fn scoped_state() -> (AppState, ruscker_admin::db::ConfigDb) { + let mut state = state(); + state.config = Arc::new( + Config::from_yaml("proxy:\n title: Scoped test\n specs: []\n") + .expect("parse empty scoped config"), + ); + let path = + std::env::temp_dir().join(format!("ruscker-rbac-scope-{}.db", uuid::Uuid::new_v4())); + let pool = ruscker_admin::db::open(&path).await.expect("open scoped db"); + let db = ruscker_admin::db::ConfigDb::Sqlite(pool); + // `db::open` seeds the product showcase. This fixture needs a closed, + // exact three-app catalog so row/KPI assertions document scope precisely. + for seeded in ruscker_admin::db::specs::list_all(&db) + .await + .expect("list seeded showcase") + { + ruscker_admin::db::specs::delete_one(&db, &seeded.id, Some("test-reset")) + .await + .expect("remove seeded showcase app"); + } + for spec in [ + app( + "id: time-a\n\ + display-name: Time A\n\ + container-image: org/time-a:1\n\ + access-groups: [time-a, legacy-ops]\n", + ), + app( + "id: time-b\n\ + display-name: Time B\n\ + container-image: org/time-b:1\n\ + access-groups: [time-b]\n", + ), + app( + "id: open-app\n\ + display-name: Open App\n\ + container-image: org/open:1\n", + ), + ] { + ruscker_admin::db::specs::upsert_one(&db, &spec, Some("seed")) + .await + .expect("seed scoped app"); + } + ruscker_admin::db::users::create( + &db, + "editor-a", + "EditorPass9!", + Role::Editor, + false, + &["time-a".to_string()], + Some("seed"), + ) + .await + .expect("create scoped Editor"); + { + let mut registry = state.replicas.write().await; + registry.add(replica(TIME_A_REPLICA, "time-a")); + registry.add(replica(TIME_B_REPLICA, "time-b")); + registry.add(replica(OPEN_REPLICA, "open-app")); + } + state.db = Some(db.clone()); + (state, db) +} + +async fn scoped_cookie(state: &AppState, role: Role, actor: Option<&str>) -> String { + let id = state + .admin_sessions + .create(role, actor.map(str::to_string)) + .await; + format!("{COOKIE_NAME}={id}") +} + +fn metric_values(body: &str) -> Vec<&str> { + const OPEN: &str = "
"; + body.match_indices(OPEN) + .filter_map(|(start, _)| { + let value = &body[start + OPEN.len()..]; + value.find("
").map(|end| value[..end].trim()) + }) + .collect() } // ── Viewer: no panel — portal authenticated-user role (#857) ───────── @@ -296,3 +433,327 @@ async fn following_an_unknown_pull_job_is_404_for_editor() { "unknown pull token ⇒ 404, never a side effect" ); } + +// ── Editor group scope over applications (#990 slice 1) ───────────── + +#[tokio::test] +async fn scoped_editor_lists_only_shared_and_open_apps_with_matching_kpis() { + let (state, _db) = scoped_state().await; + let cookie = scoped_cookie(&state, Role::Editor, Some("editor-a")).await; + + let response = send_request( + state.clone(), + "GET", + "/admin/specs", + Some(&cookie), + Body::empty(), + None, + ) + .await; + assert_eq!(response.status(), StatusCode::OK); + let body = response_body(response).await; + assert!(body.contains("data-id=\"time-a\""), "shared app is listed"); + assert!(body.contains("data-id=\"open-app\""), "open app is global"); + assert!( + !body.contains("data-id=\"time-b\""), + "foreign-team app stays hidden" + ); + let listed = body.matches("2" + ), + "container KPI must equal the filtered replica grid" + ); +} + +#[tokio::test] +async fn scoped_editor_gets_404_on_every_foreign_app_or_replica_id_route() { + let (state, _db) = scoped_state().await; + let cookie = scoped_cookie(&state, Role::Editor, Some("editor-a")).await; + + // Every current app route carrying `{id}` is enumerated here. Filtering + // the list alone is not authorization: a typed/guessed URL must fail. + for (method, uri) in [ + ("GET", "/admin/specs/time-b/edit"), + ("GET", "/admin/specs/time-b/duplicate"), + ("POST", "/admin/specs/time-b"), + ("POST", "/admin/specs/time-b/delete"), + ("POST", "/admin/specs/time-b/featured/toggle"), + ("POST", "/admin/specs/time-b/state/toggle"), + ("POST", "/admin/specs/time-b/repull"), + ] { + let response = send_request( + state.clone(), + method, + uri, + Some(&cookie), + Body::empty(), + (method == "POST").then_some("application/x-www-form-urlencoded"), + ) + .await; + assert_eq!( + response.status(), + StatusCode::NOT_FOUND, + "foreign app route must be 404: {method} {uri}" + ); + } + + // Replica ids are just another path to the owning spec. Logs, live logs, + // stop and restart all resolve the effective Spec before backend access. + for (method, uri) in [ + ( + "GET", + "/admin/dashboard/logs/bbbbbbbb-2222-4222-8222-222222222222", + ), + ( + "GET", + "/admin/dashboard/logs/bbbbbbbb-2222-4222-8222-222222222222/stream", + ), + ( + "POST", + "/admin/dashboard/replicas/bbbbbbbb-2222-4222-8222-222222222222/stop", + ), + ( + "POST", + "/admin/dashboard/replicas/bbbbbbbb-2222-4222-8222-222222222222/restart", + ), + ] { + let status = send(state.clone(), method, uri, Some(&cookie)).await; + assert_eq!( + status, + StatusCode::NOT_FOUND, + "foreign replica route must be 404: {method} {uri}" + ); + } +} + +#[tokio::test] +async fn admin_remains_unscoped_for_foreign_apps_and_replicas() { + let (state, db) = scoped_state().await; + let cookie = scoped_cookie(&state, Role::Admin, Some("admin")).await; + + let response = send_request( + state.clone(), + "GET", + "/admin/specs", + Some(&cookie), + Body::empty(), + None, + ) + .await; + assert_eq!(response.status(), StatusCode::OK); + let body = response_body(response).await; + for id in ["time-a", "time-b", "open-app"] { + assert!(body.contains(&format!("data-id=\"{id}\"")), "Admin sees {id}"); + } + assert_eq!(metric_values(&body).first().copied(), Some("3")); + + assert_eq!( + send( + state.clone(), + "GET", + "/admin/specs/time-b/edit", + Some(&cookie), + ) + .await, + StatusCode::OK + ); + assert_eq!( + send( + state.clone(), + "GET", + "/admin/specs/time-b/duplicate", + Some(&cookie), + ) + .await, + StatusCode::OK + ); + assert_eq!( + send( + state.clone(), + "POST", + "/admin/specs/time-b/featured/toggle", + Some(&cookie), + ) + .await, + StatusCode::OK + ); + assert_eq!( + send( + state.clone(), + "POST", + "/admin/specs/time-b/state/toggle", + Some(&cookie), + ) + .await, + StatusCode::OK + ); + for uri in [ + "/admin/specs/time-b/repull", + "/admin/dashboard/replicas/bbbbbbbb-2222-4222-8222-222222222222/stop", + "/admin/dashboard/replicas/bbbbbbbb-2222-4222-8222-222222222222/restart", + ] { + assert_eq!( + send(state.clone(), "POST", uri, Some(&cookie)).await, + StatusCode::SERVICE_UNAVAILABLE, + "Admin passes scope and reaches the intentionally absent backend: {uri}" + ); + } + + let response = send_request( + state.clone(), + "POST", + "/admin/specs/time-b", + Some(&cookie), + Body::from( + "display_name=Time+B+admin&display_type=app&state=active&\ + container_image=org%2Ftime-b%3A2&inject_base_href=on&access_groups=time-b", + ), + Some("application/x-www-form-urlencoded"), + ) + .await; + assert_eq!(response.status(), StatusCode::SEE_OTHER); + assert_eq!( + ruscker_admin::db::specs::fetch_one(&db, "time-b") + .await + .unwrap() + .unwrap() + .display_name + .as_deref(), + Some("Time B admin") + ); + + assert_eq!( + send( + state, + "POST", + "/admin/specs/time-b/delete", + Some(&cookie), + ) + .await, + StatusCode::SEE_OTHER, + "Admin can delete the foreign-team app" + ); + assert!( + ruscker_admin::db::specs::fetch_one(&db, "time-b") + .await + .unwrap() + .is_none() + ); +} + +#[tokio::test] +async fn scoped_editor_cannot_create_restricted_app_with_foreign_group() { + let (state, db) = scoped_state().await; + let cookie = scoped_cookie(&state, Role::Editor, Some("editor-a")).await; + let response = send_request( + state, + "POST", + "/admin/specs", + Some(&cookie), + Body::from( + "id=foreign-new&display_name=Foreign+new&display_type=app&state=active&\ + container_image=org%2Fforeign%3A1&inject_base_href=on&access_groups=time-b", + ), + Some("application/x-www-form-urlencoded"), + ) + .await; + assert_eq!(response.status(), StatusCode::UNPROCESSABLE_ENTITY); + let body = response_body(response).await; + assert!( + body.contains("fora do seu escopo de Editor"), + "localized scope validation must explain the rejection" + ); + assert!( + ruscker_admin::db::specs::fetch_one(&db, "foreign-new") + .await + .unwrap() + .is_none(), + "rejected app must not be persisted" + ); +} + +#[tokio::test] +async fn scoped_editor_cannot_assign_foreign_group_and_edit_preserves_it() { + let (state, db) = scoped_state().await; + let cookie = scoped_cookie(&state, Role::Editor, Some("editor-a")).await; + + let edit = send_request( + state.clone(), + "GET", + "/admin/specs/time-a/edit", + Some(&cookie), + Body::empty(), + None, + ) + .await; + assert_eq!(edit.status(), StatusCode::OK); + let edit_body = response_body(edit).await; + assert!( + !edit_body.contains("legacy-ops"), + "foreign memberships are preserved server-side, not exposed as editable pills" + ); + + let rejected = send_request( + state.clone(), + "POST", + "/admin/specs/time-a", + Some(&cookie), + Body::from( + "display_name=Time+A&display_type=app&state=active&\ + container_image=org%2Ftime-a%3A1&inject_base_href=on&access_groups=time-b", + ), + Some("application/x-www-form-urlencoded"), + ) + .await; + assert_eq!(rejected.status(), StatusCode::UNPROCESSABLE_ENTITY); + + let saved = send_request( + state, + "POST", + "/admin/specs/time-a", + Some(&cookie), + Body::from( + "display_name=Time+A+edited&display_type=app&state=active&\ + container_image=org%2Ftime-a%3A2&inject_base_href=on&access_groups=time-a", + ), + Some("application/x-www-form-urlencoded"), + ) + .await; + assert_eq!(saved.status(), StatusCode::SEE_OTHER); + let spec = ruscker_admin::db::specs::fetch_one(&db, "time-a") + .await + .unwrap() + .unwrap(); + assert_eq!(spec.display_name.as_deref(), Some("Time A edited")); + assert_eq!( + spec.access_groups.as_deref(), + Some(&["legacy-ops".to_string(), "time-a".to_string()][..]), + "the out-of-scope group survives the Editor's replacement save" + ); +} From e92c2ad898bcca2c4e074cccbf927ba926b81f4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Leite?= Date: Wed, 29 Jul 2026 19:41:35 -0300 Subject: [PATCH 2/2] fix: key the dashboard snapshot cache by scope, not drop it (review of #990 slice 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The slice caught a real leak: the snapshot cache key was locale-only, so a scoped Editor could be served another team's container rows. Its fix was to skip the cache entirely for scoped Editors — correct, but it throws away the reason the cache exists. That cache is #291: building a snapshot reads the registry, queries the effective catalog and assembles the rows, and N dashboard tabs each poll on their own 5 s interval. Without it, every Editor tab rebuilds every tick — the same many-tabs pressure that made the admin hang behind an HTTP/1.1 front end in #1039/#1040. Keyed by `(locale, scope)` instead: the unscoped Admin/break-glass view shares one entry, two Editors with the same group set share theirs, and no one can read another scope's. The scope key sorts and dedups the group list, since `["a","b"]` and `["b","a"]` are the same scope and must not split the cache, and joins on the unit separator so a group literally named `a,b` cannot forge the key of `["a","b"]`. A test pins all of that. The cache map's type is factored into `SnapshotCacheKey` / `CachedSnapshot` — clippy's `type_complexity` fails the gate otherwise. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/routes/admin/dashboard.rs | 105 ++++++++++++++---- 1 file changed, 85 insertions(+), 20 deletions(-) diff --git a/crates/ruscker-admin/src/routes/admin/dashboard.rs b/crates/ruscker-admin/src/routes/admin/dashboard.rs index 202bc170..00afe9cd 100644 --- a/crates/ruscker-admin/src/routes/admin/dashboard.rs +++ b/crates/ruscker-admin/src/routes/admin/dashboard.rs @@ -73,17 +73,49 @@ const SSE_INTERVAL: Duration = Duration::from_secs(5); /// handler. const SSE_KEEPALIVE_INTERVAL: Duration = Duration::from_secs(15); -/// Per-locale memoized dashboard snapshot (#291). Building a snapshot -/// reads the registry, fetches the effective spec catalog (a DB query), -/// and assembles the rows; with N open dashboard tabs each running its -/// own [`SSE_INTERVAL`] loop that was N× the work every tick. The whole -/// snapshot is the same for everyone in a given locale (only the state -/// labels are localized), so cache it for one interval and let every -/// tab/connection in that locale reuse it. Keyed by locale (≤4 entries); -/// a clone of the cached snapshot is cheap next to a fresh DB build. -static SNAPSHOT_CACHE: LazyLock)>> = +/// Memoized dashboard snapshot (#291). Building one reads the registry, +/// fetches the effective spec catalog (a DB query) and assembles the rows; +/// with N open dashboard tabs each running its own [`SSE_INTERVAL`] loop +/// that was N× the work every tick. +/// +/// Keyed by locale **and scope** (#990). The snapshot used to be identical +/// for everyone in a locale, but a scoped Editor sees only their groups' +/// rows, so a locale-only key would hand one team's containers to another. +/// Sharing survives where it matters: every Admin/break-glass view shares +/// one entry, and two Editors with the same group set share theirs — which +/// keeps the #291 guarantee for the multi-tab case that made the admin +/// hang on an HTTP/1.1 front end (#1039/#1040) instead of dropping caching +/// for Editors altogether. +static SNAPSHOT_CACHE: LazyLock> = LazyLock::new(DashMap::new); +/// `(locale, scope)` — see [`SNAPSHOT_CACHE`]. +type SnapshotCacheKey = (Locale, ScopeKey); + +/// When it was built, and the snapshot itself behind an `Arc` so serving a +/// cache hit clones a pointer rather than the whole row set. +type CachedSnapshot = (Instant, Arc); + +/// Cache identity of a viewer's authorization scope: `None` for the +/// unscoped Admin/token view, or the caller's groups sorted and joined so +/// two Editors with the same memberships hit the same entry. Sorting +/// matters — `["a","b"]` and `["b","a"]` are the same scope and must not +/// split the cache. +type ScopeKey = Option; + +fn scope_key(scope: &EditorScope) -> ScopeKey { + if scope.unscoped { + return None; + } + let mut groups: Vec<&str> = scope.groups.iter().map(String::as_str).collect(); + groups.sort_unstable(); + groups.dedup(); + // `\u{1f}` (unit separator) can't appear in a group name, so the join is + // unambiguous where a plain comma would let `["a,b"]` collide with + // `["a","b"]`. + Some(groups.join("\u{1f}")) +} + /// One row of the replicas table — flattened for the template /// and also serialized as JSON over the SSE stream so the /// client-side patcher can update in place. @@ -410,20 +442,17 @@ async fn build_snapshot( locale: Locale, scope: &EditorScope, ) -> DashboardSnapshot { - // The old cache key was locale-only. Reusing it for scoped Editors - // would serve one team's rows to another, so only the truly unscoped - // Admin/break-glass view uses that shared cache. - if scope.unscoped { - if let Some(entry) = SNAPSHOT_CACHE.get(&locale) { - if entry.0.elapsed() < SSE_INTERVAL { - return (*entry.1).clone(); - } + // Keyed by (locale, scope): a scoped Editor must never read an entry + // built for another group set, but two viewers with the SAME scope + // still share one build — that's the #291 guarantee for many tabs. + let key = (locale, scope_key(scope)); + if let Some(entry) = SNAPSHOT_CACHE.get(&key) { + if entry.0.elapsed() < SSE_INTERVAL { + return (*entry.1).clone(); } } let snap = build_snapshot_uncached(state, locale, scope).await; - if scope.unscoped { - SNAPSHOT_CACHE.insert(locale, (Instant::now(), Arc::new(snap.clone()))); - } + SNAPSHOT_CACHE.insert(key, (Instant::now(), Arc::new(snap.clone()))); snap } @@ -1260,6 +1289,42 @@ mod tests { assert!(html.contains(">2/10<"), "summed sessions on the head"); } + /// The snapshot cache is keyed by scope as well as locale (#990): two + /// Editors with the same groups must share an entry (that's the #291 + /// many-tabs guarantee), while a different group set — or the unscoped + /// Admin view — must never read someone else's rows. + #[test] + fn snapshot_cache_key_separates_scopes_but_shares_equal_ones() { + let scoped = |groups: &[&str]| EditorScope { + role: Role::Editor, + actor: Some("editor".into()), + groups: groups.iter().map(|g| (*g).to_string()).collect(), + unscoped: false, + }; + let admin = EditorScope { + role: Role::Admin, + actor: Some("admin".into()), + groups: Vec::new(), + unscoped: true, + }; + + assert_eq!(scope_key(&admin), None, "the unscoped view is one entry"); + assert_ne!(scope_key(&scoped(&["a"])), scope_key(&admin)); + assert_ne!(scope_key(&scoped(&["a"])), scope_key(&scoped(&["b"]))); + // Order and duplicates are not different scopes. + assert_eq!( + scope_key(&scoped(&["a", "b"])), + scope_key(&scoped(&["b", "a"])) + ); + assert_eq!(scope_key(&scoped(&["a", "a"])), scope_key(&scoped(&["a"]))); + // A group name containing the separator we join on can't forge + // another scope's key. + assert_ne!( + scope_key(&scoped(&["a", "b"])), + scope_key(&scoped(&["a,b"])) + ); + } + #[test] fn metric_cards_show_aggregated_counts() { let html = render_with(