diff --git a/CLAUDE.md b/CLAUDE.md index fe8d66a..f50ce0a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -28,12 +28,14 @@ Three global broadcast buses — **never add a fourth without checking these fir | Bus | Cap | Events | File | |-----|-----|--------|------| | `ChatEventBus` | 256 | user message, assistant response, compaction done | `core-api/src/bus.rs` | -| `SystemEventBus` | 64 | provider (un)registered, config key updated, job completed, session cancelled, **user created/deleted/mounts-changed** | `core-api/src/system_bus.rs` | +| `SystemEventBus` | 64 | provider (un)registered, config key updated, job completed, session cancelled, **user created/deleted/active-changed/mounts-changed** | `core-api/src/system_bus.rs` | | `GlobalEvent` (per-user) | 512 | all `ServerEvent` variants → WS clients + inbox lifecycle | `core-api/src/events.rs` | Plus internal `mpsc` queues: per-source `SourceInbox` (message serialization) and a central `notify` queue (background agents → user). -**The user-lifecycle reconciler** is the worked example of the rule. Creating a user, deleting one, or changing a shared-folder/project membership all need Docker work (provision, tear down, recreate with new bind mounts). None of the endpoints that make those changes touches `ContainerManager`: each announces `SystemEvent::User{Created,Deleted,MountsChanged}` **after** its DB write, and one subscriber — `skald::wiring::spawn_user_lifecycle`, spawned post-construction because it reacts through `Skald`'s own accessors, holding only a `Weak` — does the reacting, sequentially and best-effort. The payoff is that a *future* endpoint granting membership cannot forget to remount, because remounting was never its job. Reactions never block the HTTP response, and a failure settles at the user's next login or at boot reconciliation. +**The user-lifecycle reconciler** is the worked example of the rule. Creating a user, deleting one, deactivating one, or changing a shared-folder/project membership all need Docker work (provision, tear down, stop, recreate with new bind mounts). None of the endpoints that make those changes touches `ContainerManager`: each announces `SystemEvent::User{Created,Deleted,ActiveChanged,MountsChanged}` **after** its DB write, and one subscriber — `skald::wiring::spawn_user_lifecycle`, spawned post-construction because it reacts through `Skald`'s own accessors, holding only a `Weak` — does the reacting, sequentially and best-effort. The payoff is that a *future* endpoint granting membership cannot forget to remount, because remounting was never its job. Reactions never block the HTTP response, and a failure settles at the user's next login or at boot reconciliation. + +**Where the bus stops: reconciliation rides it, authorization does not.** `SystemEventBus` is a lossy 64-slot broadcast whose contract is *"best-effort, settles at the next login"* — right for a stale mount, wrong for a revocation, where "settles later" *is* the failure. So deactivating or deleting a user splits in two: `Skald::revoke_user_runtime` runs **synchronously in the handler, before it responds** (revoke every session → evict + cancel the `UserContext` → `UserManager::lock`, in that order, so nothing is left querying a pool we then close and the DEK leaves RAM per §9), while only the container half — stop or remove — rides the bus. Before this, `active = 0` blocked the *next* login but left live sessions working: `login` checks the flag, `require_auth` only maps token → id. **Never put an access revocation on a bus.** **Before you add a direct function call or a new import between two components, stop and ask:** is one component producing data another needs? If yes, add a variant to an existing bus and spawn a subscriber. Don't call `some_manager.log_thing(...)` from the producer — emit a `ThingHappened` event on `SystemEventBus` and let the manager subscribe. @@ -55,7 +57,7 @@ Domain words are allowed only in seed data, preset labels, UI copy and positioni ### Current state -`UserManager` (§11) is now **consumed**. Login exists (`crates/skald-core/src/auth/`: `SessionStore` + the `guard.rs` deny-by-default middleware; first admin created by `skald-setup`), and the per-user owner-bound runtime is `UserContext` (`crates/skald-core/src/skald/user_context.rs`) — resolved by `Skald::user_context` / the frontend's `require_context`, keyed off `UserManager::pool_of`. The frontend owner call-sites (WS, sessions, inbox, approval-pending, projects, uploads, run-context, **cron**) route through the per-user pool; dev/stats read `llm_requests` — a *registry* table — from `system.db`, which is correct. The "owner-without-a-user" question resolved to **there isn't one**: every owner content belongs to a logged-in user (the admin included). The global owner-bound bundles (`Conversation`/`Tasks`: the "ownerless" `ChatSessionManager`, `ChatHub`, cron `TaskManager`, `TicManager`) are still constructed but **inert** — their loops never spawn and nothing consumes their accessors; removing them is pending follow-on work (kept for now because `RunContextManager` shares the `Conversation` bundle and *is* used, being registry-backed). See blueprint §19. +`UserManager` (§11) is now **consumed**. Login exists (`crates/skald-core/src/auth/mod.rs`: `SessionStore` — `login`/`user_of`/`logout` plus `revoke_user`, the admin-side "drop every session of this user" used by `Skald::revoke_user_runtime`; the deny-by-default middleware is `src/frontend/api/guard.rs`, whose `require_auth` maps token → id and does **not** re-read the row, which is exactly why revocation must be pushed rather than polled; first admin created by `skald-setup`), and the per-user owner-bound runtime is `UserContext` (`crates/skald-core/src/skald/user_context.rs`) — resolved by `Skald::user_context` / the frontend's `require_context`, keyed off `UserManager::pool_of`, and carrying its **own `CancellationToken`** (a child of the instance one) so a single user's cron/hub/MCP loops can be stopped without touching anyone else's. The frontend owner call-sites (WS, sessions, inbox, approval-pending, projects, uploads, run-context, **cron**) route through the per-user pool; dev/stats read `llm_requests` — a *registry* table — from `system.db`, which is correct. The "owner-without-a-user" question resolved to **there isn't one**: every owner content belongs to a logged-in user (the admin included). The global owner-bound bundles (`Conversation`/`Tasks`: the "ownerless" `ChatSessionManager`, `ChatHub`, cron `TaskManager`, `TicManager`) are still constructed but **inert** — their loops never spawn and nothing consumes their accessors; removing them is pending follow-on work (kept for now because `RunContextManager` shares the `Conversation` bundle and *is* used, being registry-backed). See blueprint §19. Direction of travel, decided but not yet executed: strip the **power-user surface** (self-rewriting, arbitrary shell, dev-agent suite, ticket system) and move to a **binary-first** layout — the app is built once and run from a compiled binary, not executed from its own source tree. @@ -347,7 +349,11 @@ All extend `LightElement` from `web/lib/base.js` (Lit). `ChatSession` (`web/lib/ **Role-driven interface** (§0.1 — data, not enums): `roles.attrs` JSON may carry `"ui_mode": "simple"`. `/api/auth/me` resolves it via `RoleAttrs` (`admin` is always `full`) and the sidebar renders chat + inbox only for simple-mode members; the role editor exposes it as an "Interface" select. Hiding links is never access control — routes stay capability-gated server-side. `MeResponse` also carries `locale`, `default_locale` and `encrypted`. -**Security-group picker (per-session, runtime, role-gated).** A **security-group** is a *permission bundle only* — a `tool_permission_groups` id, driving tool visibility/approval — **not** a "mode" (no system-context injection; the `RunContext.system_prompt` substrate exists but is unused by the picker). The role carries the user's **allowed set** (default `permission_group` + `attrs.permission_groups`, §0.1); a new non-project session inherits the role's default group (`sessions.rs::create` → `role_default_run_context`). The chat surface switches it **at runtime like the model pill**: `copilot.js` renders a shield pill (hidden when ≤1 group) fed by `GET /api/my/security-groups` (the caller's role set, joined with group names; `admin` → all); selecting one sends the WS control message `{type:"select_security_group", group}` (`chat-session.js::_selectGroup`, twin of `select_client`). The server (`ws.rs::handle_select_security_group_msg`) validates against the role, persists it on `chat_sessions.run_context`, updates the live handler, and **broadcasts `ServerEvent::SecurityGroupSelected`** so every open tab re-syncs (the initial state is sent on WS connect). **Enforcement is server-side** via the shared `run_context::validate_run_context_for_role` (used by both the WS path and the REST `set_session_run_context`): a non-admin may only pick a group in its role's effective set (else 403), and **every other `RunContext` field** (`system_prompt`, `allow_fs_writes`/`allow_fs_reads`, `working_directory`) is **discarded** — closing an fs-escalation hole; `admin` passes through unchanged. The role editor (`roles-page.js`) sets the default group + an allowed-groups checklist (→ `attrs.permission_groups`) + a **default-assistant** select (→ `attrs.chat_agent`) fed by `GET /api/agents` filtered to `type:chat` minus `project-coordinator` (source-driven); the same exclusion is enforced server-side in the roles API (`validate_chat_agent`). +**Security-group picker (per-session, runtime, role-gated).** A **security-group** is a *permission bundle only* — a `tool_permission_groups` id, driving tool visibility/approval — **not** a "mode" (no system-context injection; the `RunContext.system_prompt` substrate exists but is unused by the picker). The role carries the user's **allowed set** (default `permission_group` + `attrs.permission_groups`, §0.1); a new non-project session inherits the role's default group (`sessions.rs::create` → `role_default_run_context`). The chat surface switches it **at runtime like the model pill**: `copilot.js` renders a shield pill (hidden when ≤1 group) fed by `GET /api/my/security-groups` (the caller's role set, joined with group names; `admin` → all); selecting one sends the WS control message `{type:"select_security_group", group}` (`chat-session.js::_selectGroup`, twin of `select_client`). The server (`ws.rs::handle_select_security_group_msg`) validates against the role, persists it on `chat_sessions.run_context`, updates the live handler, and **broadcasts `ServerEvent::SecurityGroupSelected`** so every open tab re-syncs (the initial state is sent on WS connect). **Enforcement is server-side** via the shared `run_context::validate_run_context_for_role` (used by both the WS path and the REST `set_session_run_context`): a non-admin may only pick a group in its role's effective set (else 403), and **every other `RunContext` field** (`system_prompt`, `allow_fs_writes`/`allow_fs_reads`, `working_directory`) is **discarded** — closing an fs-escalation hole; `admin` passes through unchanged. + +**Selection is gated once; the persisted group is re-checked on every load.** `validate_run_context_for_role` runs at *selection* time, and the result is persisted on `chat_sessions.run_context` — so on its own it let a group survive the role that granted it, indefinitely and across restarts (revoke `ops` from a role, and every session that had already picked it kept running on it). The fix is a second, narrower seam: `run_context::reconcile_group_for_user`, run by `ChatSessionManager::get_or_create_handler` on **every** handler build, which treats the stored group as *advisory* and degrades it when the owner's current role no longer allows it. Three properties are load-bearing: (a) it degrades to the **role's default group** (`role_default_group`, the same seam `sessions.rs` uses for a new session, so start-group and fallback-group cannot drift) — **never to `None`**, because a missing group means the catch-all `default`, whose rules are the fallback tier under every other group, so clearing *widens*; (b) it touches **only** `security_group`, unlike the selection path, so a project session's server-built `project_root`/`system_prompt` survive a permissions edit; (c) on uncertainty (unknown user, unreadable role, DB error) it leaves the stored group alone — guessing could only widen. The liveness half is `Skald::revalidate_security_groups_for_{user,role}`, called **synchronously** from the roles API (`update`) and the users API (role reassignment), which reconciles already-open handlers, persists, and emits `SecurityGroupSelected` so the pill re-syncs. Same rule as revocation: authorization is pushed, never left to the bus. + +The role editor (`roles-page.js`) sets the default group + an allowed-groups checklist (→ `attrs.permission_groups`) + a **default-assistant** select (→ `attrs.chat_agent`) fed by `GET /api/agents` filtered to `type:chat` minus `project-coordinator` (source-driven); the same exclusion is enforced server-side in the roles API (`validate_chat_agent`). | File | Element | Notes | | ---- | ------- | ----- | diff --git a/crates/core-api/src/system_bus.rs b/crates/core-api/src/system_bus.rs index ed7979b..b65c1a5 100644 --- a/crates/core-api/src/system_bus.rs +++ b/crates/core-api/src/system_bus.rs @@ -68,6 +68,18 @@ pub enum SystemEvent { UserDeleted { user_id: String, }, + /// A user was deactivated (`false`) or reactivated (`true`). Their sandbox is + /// stopped or started to match — boot reconciliation keeps a container only for + /// *active* users, so this is the running-server equivalent. + /// + /// Revoking the live runtime (sessions, loops, database key) is **not** on this + /// event: it is an authorization invariant and runs synchronously in the handler + /// (`Skald::revoke_user_runtime`), because a lossy broadcast is the wrong + /// transport for "this person must stop being logged in". + UserActiveChanged { + user_id: String, + active: bool, + }, /// A user's **mount topology** changed — a shared-folder or project membership /// was granted, revoked or re-graded (RO ⇄ RW). Their container must be /// recreated against the new mount set, and a live session's filesystem view diff --git a/crates/skald-core/src/auth/mod.rs b/crates/skald-core/src/auth/mod.rs index 26e74fc..0233d2b 100644 --- a/crates/skald-core/src/auth/mod.rs +++ b/crates/skald-core/src/auth/mod.rs @@ -71,6 +71,39 @@ impl SessionStore { .cloned() } + /// Removes **every** session of one user, returning how many were dropped. + /// + /// The admin half of [`logout`](Self::logout): a deactivated or deleted user + /// must stop being authenticated *now*, not at their next request. `login` + /// already refuses an inactive user (`verify_credentials` / `open_db` check the + /// flag), but `require_auth` only maps token → id and would happily keep serving + /// a token minted before the flag flipped. + /// + /// Sessions only, deliberately: this leaves the pool open, so the caller must + /// follow with `UserManager::lock` to get the key out of RAM (§9). Both run + /// synchronously on the admin's request — revocation is an invariant, not + /// something to reconcile later on a lossy bus. + pub fn revoke_user(&self, user_id: &str) -> usize { + let mut map = self.sessions.write().expect("sessions map poisoned"); + let before = map.len(); + map.retain(|_, id| id != user_id); + let removed = before - map.len(); + if removed > 0 { + info!(user = %user_id, sessions = removed, "sessions revoked"); + } + removed + } + + /// Records a session without authenticating — tests only, so the revocation + /// semantics can be exercised without paying an Argon2id derivation per login. + #[cfg(test)] + fn insert_session(&self, token: &str, user_id: &str) { + self.sessions + .write() + .expect("sessions map poisoned") + .insert(token.to_string(), user_id.to_string()); + } + /// Removes a single session. The database pool stays open (§9). pub fn logout(&self, token: &str) { if let Some(user_id) = self @@ -83,3 +116,59 @@ impl SessionStore { } } } + +#[cfg(test)] +mod tests { + use super::*; + + fn temp_db_path(tag: &str) -> String { + let mut p = std::env::temp_dir(); + let nanos = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH).unwrap().as_nanos(); + p.push(format!("skald-test-{tag}-{}-{nanos}.db", std::process::id())); + p.to_string_lossy().into_owned() + } + + fn cleanup(path: &str) { + for suffix in ["", "-wal", "-shm"] { + let _ = std::fs::remove_file(format!("{path}{suffix}")); + } + } + + async fn store(tag: &str) -> (SessionStore, String) { + let path = temp_db_path(tag); + let pool = Arc::new(crate::db::init_system_pool(&path).await.unwrap()); + (SessionStore::new(Arc::new(UserManager::new(pool))), path) + } + + /// Deactivating or deleting a user must drop **every** session they hold — one + /// browser left logged in is the whole bug — and **only** theirs. + #[tokio::test] + async fn revoke_user_drops_all_of_one_users_sessions_and_nobody_elses() { + let (store, path) = store("revoke").await; + store.insert_session("t-laptop", "u-1"); + store.insert_session("t-phone", "u-1"); + store.insert_session("t-other", "u-2"); + + assert_eq!(store.revoke_user("u-1"), 2); + assert_eq!(store.user_of("t-laptop"), None); + assert_eq!(store.user_of("t-phone"), None); + assert_eq!(store.user_of("t-other").as_deref(), Some("u-2"), + "revoking one user must not log out the rest of the household"); + + cleanup(&path); + } + + /// Idempotent: revoking a user with nothing live is a no-op, not an error — the + /// admin path calls it unconditionally. + #[tokio::test] + async fn revoke_user_is_a_no_op_when_nothing_is_live() { + let (store, path) = store("revoke-empty").await; + store.insert_session("t-other", "u-2"); + + assert_eq!(store.revoke_user("u-1"), 0); + assert_eq!(store.user_of("t-other").as_deref(), Some("u-2")); + + cleanup(&path); + } +} diff --git a/crates/skald-core/src/run_context/mod.rs b/crates/skald-core/src/run_context/mod.rs index 0aaf6c3..f23392e 100644 --- a/crates/skald-core/src/run_context/mod.rs +++ b/crates/skald-core/src/run_context/mod.rs @@ -140,6 +140,88 @@ pub async fn validate_run_context_for_role( } } +/// The catch-all permission group. A `RunContext` with no `security_group` resolves +/// here, and its rules are the fallback tier under *every* other group — so clearing +/// a group **widens** what a session may do, and is never the safe direction. +pub const DEFAULT_GROUP_ID: &str = "default"; + +/// The security group a session gets from its owner's role: `roles.permission_group`, +/// or `None` when that is unset or already the catch-all (nothing to pin). +pub async fn role_default_group(registry_pool: &SqlitePool, user_id: &str) -> Option { + let user = crate::db::users::get(registry_pool, user_id).await.ok()??; + let role = crate::db::roles::get(registry_pool, &user.role_id).await.ok()??; + let group = role.permission_group; + (!group.is_empty() && group != DEFAULT_GROUP_ID).then_some(group) +} + +/// A run-context for a **new** session carrying nothing but the role's default group, +/// so a restricted role starts scoped instead of on the catch-all. +pub async fn role_default_run_context( + registry_pool: &SqlitePool, + user_id: &str, +) -> Option { + role_default_group(registry_pool, user_id) + .await + .map(|g| RunContext::with_security_group(Some(g))) +} + +/// Re-checks a **persisted** run-context's security group against the owner's +/// *current* role, degrading it to the role default when the role no longer allows it. +/// +/// This is the counterpart of [`validate_run_context_for_role`], which gates a group +/// at *selection* time. The selected group is then persisted on `chat_sessions. +/// run_context` and was replayed verbatim on every later load — so revoking a group +/// from a role, or moving a user to a stricter role, left every session that already +/// had it running with it, indefinitely and across restarts. Running every load +/// through here makes the persisted value advisory rather than authoritative. +/// +/// Deliberately **narrow**: only `security_group` is touched. A project session's +/// server-built context (`project_root`, `system_prompt`, fs grants) must survive +/// intact — unlike the selection path, which discards those because they came from +/// a client. +/// +/// Conservative on uncertainty: with no group, an `admin` owner, or a role that +/// cannot be read, the context is returned unchanged. Guessing on a transient DB +/// error could only widen the session, which is the one outcome worth avoiding. +pub async fn reconcile_group_for_user( + registry_pool: &SqlitePool, + user_id: &str, + rc: Option, +) -> Option { + let mut rc = rc?; + let Some(group) = rc.tool_group_id().map(str::to_string) else { return Some(rc) }; + + let role_id = match crate::db::users::get(registry_pool, user_id).await { + Ok(Some(u)) => u.role_id, + other => { + tracing::warn!(user = %user_id, group = %group, missing = other.is_ok(), + "run_context: cannot resolve role, leaving the persisted security group in place"); + return Some(rc); + } + }; + if role_id == crate::db::roles::ADMIN_ROLE_ID { + return Some(rc); + } + match crate::db::roles::role_allows_group(registry_pool, &role_id, &group).await { + Ok(true) => return Some(rc), + Ok(false) => {} + Err(e) => { + tracing::warn!(user = %user_id, group = %group, error = %e, + "run_context: group check failed, leaving the persisted security group in place"); + return Some(rc); + } + } + + let replacement = role_default_group(registry_pool, user_id).await; + info!( + user = %user_id, role = %role_id, revoked = %group, + now = replacement.as_deref().unwrap_or(DEFAULT_GROUP_ID), + "run_context: security group no longer allowed by the role, degraded to the role default" + ); + rc.security_group = replacement; + Some(rc) +} + pub struct RunContextManager { db: Arc, approval: Arc, @@ -370,6 +452,100 @@ mod tests { std::fs::remove_dir_all(&wd).ok(); } + /// A registry with one role and one member of it. No crypto: the reconcile path + /// only reads the directory, never a credential. + async fn registry_with(role: &str, default_group: &str, extra_groups: &str) -> SqlitePool { + let path = unique_tmp().join("system.db"); + let pool = crate::db::init_system_pool(path.to_str().unwrap()).await.unwrap(); + crate::db::roles::insert(&pool, role, "Role", default_group, Some(extra_groups)) + .await + .unwrap(); + crate::db::users::insert( + &pool, "u-1", "ada", None, role, + &crate::db::users::Credentials::Cleartext(None), + ) + .await + .unwrap(); + pool + } + + /// The regression: a group selected while the role allowed it was replayed from + /// `chat_sessions.run_context` forever, so revoking it from the role changed + /// nothing for sessions that already had it. + #[tokio::test] + async fn reconcile_degrades_a_group_the_role_no_longer_allows() { + // The role's default is `kid`; `ops` is NOT in its set (it was, once). + let pool = registry_with("member", "kid", r#"{"permission_groups":[]}"#).await; + + let rc = RunContext { security_group: Some("ops".into()), ..Default::default() }; + let got = reconcile_group_for_user(&pool, "u-1", Some(rc)).await.unwrap(); + assert_eq!(got.tool_group_id(), Some("kid"), + "a revoked group must degrade to the role default, never to the catch-all"); + } + + /// Degrading must not silently widen: clearing to `None` would put a restricted + /// user on the catch-all `default` group, whose rules are the fallback tier under + /// every other group. + #[tokio::test] + async fn reconcile_keeps_an_allowed_group_and_preserves_the_rest_of_the_context() { + let pool = registry_with("member", "kid", r#"{"permission_groups":["ops"]}"#).await; + + // Still allowed → untouched, including a project session's server-built fields, + // which the *selection* path would have stripped. + let rc = RunContext { + security_group: Some("ops".into()), + project_root: Some("projects/ada/site".into()), + system_prompt: vec!["project brief".into()], + ..Default::default() + }; + let got = reconcile_group_for_user(&pool, "u-1", Some(rc)).await.unwrap(); + assert_eq!(got.tool_group_id(), Some("ops")); + assert_eq!(got.project_root.as_deref(), Some("projects/ada/site")); + assert_eq!(got.system_prompt, vec!["project brief".to_string()]); + } + + /// A degrade must keep the rest of the context too — losing `project_root` would + /// break a project chat as a side effect of a permissions edit. + #[tokio::test] + async fn reconcile_degrade_preserves_project_fields() { + let pool = registry_with("member", "kid", r#"{"permission_groups":[]}"#).await; + + let rc = RunContext { + security_group: Some("ops".into()), + project_root: Some("projects/ada/site".into()), + ..Default::default() + }; + let got = reconcile_group_for_user(&pool, "u-1", Some(rc)).await.unwrap(); + assert_eq!(got.tool_group_id(), Some("kid")); + assert_eq!(got.project_root.as_deref(), Some("projects/ada/site")); + } + + /// Uncertainty must never widen: an unknown user leaves the persisted group alone + /// rather than falling back to the catch-all. + #[tokio::test] + async fn reconcile_leaves_the_group_alone_when_the_role_cannot_be_resolved() { + let pool = registry_with("member", "kid", r#"{"permission_groups":[]}"#).await; + + let rc = RunContext { security_group: Some("ops".into()), ..Default::default() }; + let got = reconcile_group_for_user(&pool, "ghost", Some(rc)).await.unwrap(); + assert_eq!(got.tool_group_id(), Some("ops")); + } + + /// `admin` holds every group by construction, so nothing is ever degraded for it. + #[tokio::test] + async fn reconcile_never_touches_an_admin() { + let path = unique_tmp().join("system.db"); + let pool = crate::db::init_system_pool(path.to_str().unwrap()).await.unwrap(); + crate::db::users::insert( + &pool, "u-admin", "root", None, crate::db::roles::ADMIN_ROLE_ID, + &crate::db::users::Credentials::Cleartext(None), + ).await.unwrap(); + + let rc = RunContext { security_group: Some("anything".into()), ..Default::default() }; + let got = reconcile_group_for_user(&pool, "u-admin", Some(rc)).await.unwrap(); + assert_eq!(got.tool_group_id(), Some("anything")); + } + #[tokio::test] async fn validate_admin_passes_through_untouched() { let path = unique_tmp().join("system.db"); diff --git a/crates/skald-core/src/session/manager.rs b/crates/skald-core/src/session/manager.rs index 7257575..70d009e 100644 --- a/crates/skald-core/src/session/manager.rs +++ b/crates/skald-core/src/session/manager.rs @@ -191,7 +191,16 @@ impl ChatSessionManager { .await? .ok_or_else(|| anyhow::anyhow!("session {session_id} not found"))?; - let run_context = session.run_context.as_deref().and_then(RunContext::from_db); + // The persisted group is **advisory**: re-check it against the owner's current + // role, so a group revoked since the session last ran cannot be replayed from + // the row. Every load goes through here — restart, re-login, new handler — so + // correctness does not depend on anyone having pushed a notification. + let run_context = crate::run_context::reconcile_group_for_user( + &self.shared_pool, + &self.user_id, + session.run_context.as_deref().and_then(RunContext::from_db), + ) + .await; let handler = Arc::new(ChatSessionHandler::new( session_id, @@ -228,4 +237,49 @@ impl ChatSessionManager { pub fn refresh_fs(&self, fs: UserFs) { self.user_fs.store(fs); } + + /// Re-checks every **live** handler's security group against the owner's current + /// role, degrading any the role no longer allows (see + /// [`crate::run_context::reconcile_group_for_user`]). + /// + /// [`get_or_create_handler`](Self::get_or_create_handler) already reconciles on + /// load, which covers every future session; this covers the sessions that are + /// *already* open, whose handler holds its run-context in RAM and would otherwise + /// keep the revoked group until the process restarts. Both the row and the live + /// handler are updated, so the change survives and the UI reads the truth. + /// + /// Returns `(source, effective group)` for each session that actually changed — + /// the caller broadcasts `SecurityGroupSelected` so open tabs re-sync their pill. + pub async fn revalidate_security_groups(&self) -> Vec<(String, String)> { + let handlers: Vec<_> = self.active.lock().await + .iter().map(|(id, h)| (*id, Arc::clone(h))).collect(); + + let mut changed = Vec::new(); + for (session_id, handler) in handlers { + let before = handler.run_context.read().await.clone(); + let before_group = before.as_ref().and_then(|rc| rc.tool_group_id().map(str::to_string)); + let after = crate::run_context::reconcile_group_for_user( + &self.shared_pool, &self.user_id, before, + ).await; + let after_group = after.as_ref().and_then(|rc| rc.tool_group_id().map(str::to_string)); + if before_group == after_group { + continue; + } + + if let Err(e) = chat_sessions::set_run_context( + &self.db, session_id, after.as_ref().map(|rc| rc.to_db()).as_deref(), + ).await { + // The in-RAM update below still takes effect for this process; the + // reconcile on next load would redo the degrade anyway. + tracing::warn!(session = session_id, error = %e, + "failed to persist a degraded security group"); + } + handler.set_run_context(after).await; + changed.push(( + handler.source.clone(), + after_group.unwrap_or_else(|| crate::run_context::DEFAULT_GROUP_ID.to_string()), + )); + } + changed + } } diff --git a/crates/skald-core/src/skald/accessors.rs b/crates/skald-core/src/skald/accessors.rs index 2dfaaad..8a4ee72 100644 --- a/crates/skald-core/src/skald/accessors.rs +++ b/crates/skald-core/src/skald/accessors.rs @@ -70,6 +70,71 @@ impl Skald { self.rt_user_contexts().peek(user_id).await } + /// Revokes a user's live runtime: sessions, owner-bound loops, database key. + /// + /// Called when a user is **deactivated or deleted**. Writing `active = 0` (or + /// deleting the row) only stops the *next* login: `login` checks the flag, but + /// `require_auth` maps token → id without re-reading the row, so a session minted + /// before the change would keep working, over a pool whose key is still in RAM. + /// + /// The order is load-bearing: + /// + /// 1. **Revoke the sessions** — the moment this returns, no token authenticates + /// as this user. + /// 2. **Evict the context** — cancels their cron loop, hub and per-user MCP + /// runtime, so nothing is left to query the pool we are about to close. + /// 3. **Lock the database** — `close()`s the pool, which invalidates every + /// surviving clone and drops the DEK (§9). The user is opaque again. + /// + /// Synchronous by design: this is an authorization invariant, not reconciliation, + /// so it must not ride the lossy system bus. The Docker half (stop or remove the + /// container) *is* reconciliation and does ride it. + /// + /// Idempotent — a user with no live session and a locked database is a no-op. + pub async fn revoke_user_runtime(&self, user_id: &str) { + self.sessions().revoke_user(user_id); + self.rt_user_contexts().evict(user_id).await; + self.rt.users.lock(user_id).await; + } + + /// Re-checks a live user's open sessions against their current role, degrading any + /// security group the role no longer allows, and tells their open tabs about it. + /// + /// The durable half of this is in `ChatSessionManager::get_or_create_handler`, + /// which reconciles on every load; this is the liveness half, for sessions already + /// in RAM. Synchronous, like [`Self::revoke_user_runtime`] and for the same reason: + /// narrowing someone's permissions is an authorization change, not reconciliation. + /// + /// No-op for a user who is not logged in — their next login loads through the + /// reconcile anyway. + pub async fn revalidate_security_groups_for_user(&self, user_id: &str) { + let Some(ctx) = self.user_context_if_live(user_id).await else { return }; + for (source, group) in ctx.sessions.revalidate_security_groups().await { + ctx.chat_hub.emit(core_api::events::GlobalEvent { + source: Some(source), + session_id: None, + event: core_api::events::ServerEvent::SecurityGroupSelected { group }, + }); + } + } + + /// [`Self::revalidate_security_groups_for_user`] for every member of a role — + /// called when the role's own group set changes, which can narrow many users at + /// once. Members who are not logged in need nothing. + pub async fn revalidate_security_groups_for_role(&self, role_id: &str) { + let users = match crate::db::users::list(self.db()).await { + Ok(u) => u, + Err(e) => { + tracing::warn!(role = %role_id, error = %e, + "cannot list users to revalidate security groups"); + return; + } + }; + for user in users.into_iter().filter(|u| u.role_id == role_id) { + self.revalidate_security_groups_for_user(&user.id).await; + } + } + /// Applies a shared-folder membership change to a user (blueprint §6 remount). /// /// A container's bind mounts are fixed at `docker create` time, so the mount set diff --git a/crates/skald-core/src/skald/user_context.rs b/crates/skald-core/src/skald/user_context.rs index 03affbe..f1f8b5c 100644 --- a/crates/skald-core/src/skald/user_context.rs +++ b/crates/skald-core/src/skald/user_context.rs @@ -65,6 +65,11 @@ use super::runtime::Runtime; pub struct UserContext { pub user_id: String, pub pool: Arc, + /// This user's stop signal — a child of the instance shutdown token. Every + /// owner-bound loop (cron, hub, per-user MCP) observes it, so cancelling it + /// tears down exactly one user's runtime without touching anyone else's. + /// Cancelled by [`UserContextRegistry::evict`] on deactivation/deletion. + pub shutdown: CancellationToken, /// The owner's filesystem view (home + shared folders + container, §6), /// threaded into every `ToolContext` this user's sessions produce. A shared /// swappable cell so a shared-folder membership change is applied in place @@ -181,6 +186,13 @@ impl UserContextFactory { async fn build(&self, user_id: &str, pool: SqlitePool) -> Result> { let pool = Arc::new(pool); + // This user's own stop signal: a **child** of the instance token, so a global + // shutdown still stops every user's loops, while cancelling it alone tears + // down exactly one user's runtime (deactivation / deletion — see + // `UserContextRegistry::evict`). Every owner-bound loop below takes this + // token, never the instance one, or a revoked user's cron would keep polling + // a closed pool. + let user_shutdown = self.shutdown_token.child_token(); // The owner's filesystem view: private home + shared folders + container. // A shared swappable cell — a shared-folder membership change is applied in // place while the user is live (§6 remount), not deferred to next login. @@ -227,7 +239,7 @@ impl UserContextFactory { } let user_mcp = Arc::new(McpManager::new( Arc::clone(&pool), - self.shutdown_token.clone(), + user_shutdown.clone(), "data", )); // NOTE: per-user MCP elicitation (interactive connector login, §15) is @@ -335,7 +347,7 @@ impl UserContextFactory { Arc::clone(&manager), Arc::clone(&approval), global_tx.clone(), - self.shutdown_token.clone(), + user_shutdown.clone(), default_agent, ); chat_hub.register("web").await; @@ -350,14 +362,16 @@ impl UserContextFactory { // durable cron job (blueprint §7.2) instead of an interface-tool call. manager.loop_runtime().set_task_manager(Arc::clone(&cron)); - // Per-user cron loop. `start()` observes the shutdown token, so it stops on - // shutdown; adopting it lets the supervisor also join it. The name is leaked - // to satisfy the `&'static str` label — bounded by the (small) user count. + // Per-user cron loop. `start()` observes the token, so it stops on a global + // shutdown *or* on this user's own revocation; adopting it lets the supervisor + // also join it. The name is leaked to satisfy the `&'static str` label — + // bounded by the (small) user count. let name: &'static str = Box::leak(format!("cron:{user_id}").into_boxed_str()); - self.supervisor.adopt(name, Arc::clone(&cron).start(self.shutdown_token.clone())); + self.supervisor.adopt(name, Arc::clone(&cron).start(user_shutdown.clone())); Ok(Arc::new(UserContext { user_id: user_id.to_string(), + shutdown: user_shutdown, pool, fs, event_bus, @@ -413,6 +427,23 @@ impl UserContextRegistry { pub(super) async fn all_live(&self) -> Vec> { self.contexts.lock().await.values().cloned().collect() } + + /// Removes a user's context and cancels it — the runtime half of revoking a + /// user (deactivation or deletion). + /// + /// Cancelling the context's own token stops its cron loop, hub and per-user MCP + /// runtime; dropping the registry's `Arc` lets the context die once the last + /// in-flight borrow releases it, at which point the `docker exec -i` children of + /// its MCP servers are reaped by `kill_on_drop`. Locking the pool is **not** done + /// here — that is `UserManager`'s job (§11 boundary) and the caller sequences it + /// after this, so no loop is left querying a closed pool. + /// + /// Returns the evicted context, or `None` if the user was not live. + pub(super) async fn evict(&self, user_id: &str) -> Option> { + let ctx = self.contexts.lock().await.remove(user_id)?; + ctx.shutdown.cancel(); + Some(ctx) + } } // ── UserChannelHandle impl ──────────────────────────────────────────────────── diff --git a/crates/skald-core/src/skald/wiring.rs b/crates/skald-core/src/skald/wiring.rs index 9ae7cbd..51cefb6 100644 --- a/crates/skald-core/src/skald/wiring.rs +++ b/crates/skald-core/src/skald/wiring.rs @@ -134,6 +134,20 @@ pub(super) fn spawn_user_lifecycle(skald: &Arc) { "user-lifecycle: failed to remove container"); } } + // Match boot reconciliation, which keeps a container only for active + // users. The user's live runtime is already gone by now — the handler + // revoked it synchronously before emitting. + SystemEvent::UserActiveChanged { user_id, active } => { + let result = if active { + skald.container().ensure(&user_id).await + } else { + skald.container().stop(&user_id).await + }; + if let Err(e) = result { + warn!(user = %user_id, active, error = %e, + "user-lifecycle: failed to apply active-state change to container"); + } + } SystemEvent::UserMountsChanged { user_id } => { if let Err(e) = skald.refresh_user_mounts(&user_id).await { warn!(user = %user_id, error = %e, diff --git a/src/frontend/api/roles.rs b/src/frontend/api/roles.rs index 1221c10..99ecb30 100644 --- a/src/frontend/api/roles.rs +++ b/src/frontend/api/roles.rs @@ -96,6 +96,11 @@ pub async fn update( return Err(ApiError::not_found("role not found")); } let role = roles::get(skald.db(), &id).await?.ok_or_else(|| ApiError::not_found("role not found after update"))?; + // The edit may have narrowed the role's group set. Push that onto the members who + // are logged in right now — their open sessions hold the previously-selected group + // in RAM and would otherwise keep using it. (`delete` needs no equivalent: it + // refuses while any user is still assigned.) + skald.revalidate_security_groups_for_role(&id).await; Ok(Json(role)) } diff --git a/src/frontend/api/sessions.rs b/src/frontend/api/sessions.rs index f568b7d..83bed84 100644 --- a/src/frontend/api/sessions.rs +++ b/src/frontend/api/sessions.rs @@ -10,7 +10,7 @@ use serde::{Deserialize, Serialize}; use serde_json::{Value, json}; use sqlx::SqlitePool; -use skald_core::db::{chat_history, chat_llm_tools, chat_sessions, chat_sessions_stack, roles, sources}; +use skald_core::db::{chat_history, chat_llm_tools, chat_sessions, chat_sessions_stack, sources}; use skald_core::db::chat_sessions_stack::SessionStack; use skald_core::run_context::RunContext; use std::sync::Arc; @@ -54,17 +54,16 @@ pub async fn create( /// The default security-group a new session gets from the owner's role, or `None` /// when the role points at the catch-all `default` group (nothing to pin). +/// +/// Thin wrapper over the core seam, which is also what +/// [`reconcile_group_for_user`](skald_core::run_context::reconcile_group_for_user) +/// degrades *to* — so the group a session starts on and the group it falls back to +/// after a revocation can never drift apart. async fn role_default_run_context( skald: &Skald, user_id: &str, ) -> Result, ApiError> { - let Some(user) = skald.users().get(user_id).await? else { return Ok(None) }; - let Some(role) = roles::get(skald.db(), &user.role_id).await? else { return Ok(None) }; - let group = role.permission_group; - if group.is_empty() || group == "default" { - return Ok(None); - } - Ok(Some(RunContext::with_security_group(Some(group)))) + Ok(skald_core::run_context::role_default_run_context(skald.db(), user_id).await) } // ── GET /api/web/messages ───────────────────────────────────────────────────── diff --git a/src/frontend/api/users_mgmt.rs b/src/frontend/api/users_mgmt.rs index 2f0282d..63fc276 100644 --- a/src/frontend/api/users_mgmt.rs +++ b/src/frontend/api/users_mgmt.rs @@ -142,6 +142,14 @@ pub async fn update( body.sex.as_deref(), body.notes.as_deref(), )?; + // Read the current row first: only a real transition should revoke a runtime, + // touch a container or re-check permissions, so an ordinary profile edit stays free. + let before = skald + .users() + .get(&id) + .await? + .ok_or_else(|| ApiError::not_found("no such user"))?; + let (was_active, was_role) = (before.active, before.role_id); skald_core::db::users::update_profile( skald.db(), &id, @@ -164,6 +172,26 @@ pub async fn update( ) .await?; + // A move to a different role can narrow what this user may select. Their open + // sessions hold the old group in RAM and the row holds it on disk, so re-check + // both before responding — same reason deactivation is synchronous. + if was_role != body.role_id { + skald.revalidate_security_groups_for_user(&id).await; + } + + if was_active != body.active { + // Deactivation has to bite now, not at the user's next request: revoke their + // sessions, stop their loops and drop their database key before responding. + if !body.active { + skald.revoke_user_runtime(&id).await; + } + // The container half is reconciliation — it rides the bus. + skald.system_bus().send(SystemEvent::UserActiveChanged { + user_id: id.clone(), + active: body.active, + }); + } + Ok(Json(serde_json::json!({ "ok": true }))) } @@ -173,6 +201,9 @@ pub async fn delete( State(skald): State>, Path(id): Path, ) -> Result, ApiError> { + // Revoke *before* the row goes: a token outliving its user would otherwise + // authenticate as an id that no longer exists. + skald.revoke_user_runtime(&id).await; skald.users().delete_user(&id).await?; // The row is gone; the reconciler tears the container down (a missing one is fine). skald.system_bus().send(SystemEvent::UserDeleted { user_id: id });