auth: make deactivation and group revocation actually revoke
Nightly Build / build (push) Successful in 6m58s

Two variants of the same defect: an admin took away access and the running
system kept granting it.

Deactivating or deleting a user only stopped the *next* login. `login`
checks the active flag, but `require_auth` maps token -> id without
re-reading the row, so an already-open session kept working over a pool
whose key was still in RAM. There was no way to stop one user either: the
per-user cron, hub and MCP loops all observed the *instance* shutdown
token. They now take a per-user child token stored on UserContext, and
Skald::revoke_user_runtime tears a single user down in a load-bearing
order — revoke every session, evict and cancel the context, then lock the
database, so nothing is left querying a pool we are about to close.

Revoking a security group had a durable version of the same problem. The
group is validated when selected and then persisted on
chat_sessions.run_context, which was replayed verbatim on every later
load — so a group removed from a role stayed in force on sessions that
already had it, across restarts. get_or_create_handler now runs the stored
value through run_context::reconcile_group_for_user, making it advisory:
every load re-checks it, whether or not anyone announced the change.

The degrade target is the role's default group, never None: a context with
no group resolves to the catch-all `default`, whose rules are the fallback
tier under every other group, so clearing widens rather than narrows. The
reconcile touches only security_group, so a project session's server-built
project_root and system_prompt survive a permissions edit, and it leaves
the stored group alone when the role cannot be resolved — guessing on a
transient error could only widen. role_default_run_context moves into the
core seam so the group a session starts on and the group it falls back to
cannot drift apart.

Both fixes run synchronously in their handlers. Only the container half of
deactivation rides the bus, as the new UserActiveChanged event: a lossy
64-slot broadcast whose contract is "settles at the next login" is the
wrong transport for taking access away.

Tests: revoke_user drops all of one user's sessions and nobody else's, and
is a no-op when nothing is live; the reconcile degrades a revoked group to
the role default, keeps an allowed one, preserves project fields in both
directions, never touches an admin, and stays put when the role is
unresolvable.

Not exercised at runtime: no Docker/live-server run, so the end-to-end
paths (deactivating a logged-in user, editing a role with sessions open)
are covered by unit tests only.
This commit is contained in:
2026-07-26 22:17:30 +01:00
parent c50a0d84da
commit 0ba140186f
11 changed files with 501 additions and 19 deletions
+89
View File
@@ -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);
}
}