From c0a779b79e374e502db9403685601f17f6a3c0ad Mon Sep 17 00:00:00 2001 From: xavix-yo Date: Fri, 7 Aug 2026 12:37:23 +0100 Subject: [PATCH] fix: let an admin use the connectors they implicitly hold MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Activating a per-user connector as admin failed with "you are not authorized to use this connector — ask an admin to enable it for you". `db::access_defaults` deliberately writes no grant rows for admins, and says why: "they already hold every plugin and connector implicitly, so a row for them would be noise". That implicit hold was only ever implemented for plugins (`plugin_access::effective_access`). The two MCP grant tables had nothing but the raw junction read, so an admin ended up with no row *and* no short-circuit — denied their own connectors, and denied more the more the seeding was trusted to skip them. The reported symptom was the mildest of four: - `activate` refused, while `available` listed the entry (an admin holds `mcp.manage_catalog`) — visible but unusable; - the login-time startup filter dropped an admin's already-activated catalog connectors, so they silently stopped running; - `accessible_global` snapshotted an empty set, so an admin's sessions were offered no shared MCP tools at all — no error, just absence; - the connector report told the agent an admin's own global connector was "not granted to you". `users::is_admin` is now the single predicate behind every "admins hold it implicitly" short-circuit, and `plugin_access` was moved onto it too: three tables open-coding the same role lookup is what let one of them be written without it. Each MCP table grows an `effective_access` beside its `has_access`, and the distinction is the point — `has_access` stays the roster question ("what did the admin tick"), which the access-editing surfaces must keep asking, while the gates ask the authorization one. Nothing widens for anyone else: deny-by-default is untouched for non-admins, an unknown user is nobody, a disabled global stays excluded for admins too, and the `not_granted` report branch survives for a non-admin who was given the catalog-management capability. --- .../skald-core/src/db/mcp_catalog_access.rs | 63 ++++++++++ crates/skald-core/src/db/mcp_global_access.rs | 117 +++++++++++++++++- crates/skald-core/src/db/plugin_access.rs | 11 +- crates/skald-core/src/db/users.rs | 18 +++ .../src/loop_adapters/activation.rs | 4 +- crates/skald-core/src/skald/user_context.rs | 6 +- crates/skald-core/src/tools/mcp_report.rs | 41 +++++- src/frontend/api/mcp.rs | 9 +- 8 files changed, 248 insertions(+), 21 deletions(-) diff --git a/crates/skald-core/src/db/mcp_catalog_access.rs b/crates/skald-core/src/db/mcp_catalog_access.rs index 60e29ef..839e799 100644 --- a/crates/skald-core/src/db/mcp_catalog_access.rs +++ b/crates/skald-core/src/db/mcp_catalog_access.rs @@ -33,6 +33,10 @@ pub async fn users_for_catalog(pool: &SqlitePool, catalog_name: &str) -> Result< Ok(rows.into_iter().map(|(u,)| u).collect()) } +/// The raw junction read: is there a grant row? This is the **roster** question — +/// what an admin ticked on somebody's page — and it is what the access-editing +/// surfaces must show. It is *not* the authorization question; use +/// [`effective_access`] for that. pub async fn has_access(pool: &SqlitePool, catalog_name: &str, user_id: &str) -> Result { let row = sqlx::query_as::<_, (i64,)>( "SELECT 1 FROM mcp_catalog_access WHERE catalog_name = ? AND user_id = ?", @@ -44,6 +48,27 @@ pub async fn has_access(pool: &SqlitePool, catalog_name: &str, user_id: &str) -> Ok(row.is_some()) } +/// The authorization decision: may this user activate/run this connector? +/// +/// The admin role holds every connector implicitly, exactly as it holds every +/// plugin ([`super::plugin_access::effective_access`]) and every capability +/// ([`super::role_capabilities::has`]). That implicit hold is not a convenience — +/// [`super::access_defaults`] *depends* on it: it skips admins when seeding grants +/// ("they already hold every plugin and connector implicitly, so a row for them +/// would be noise"), so without a short-circuit here an admin ends up with no row +/// and no implicit access, and is denied their own connectors. That was the bug: +/// `available` listed a per-user connector to the admin (who holds +/// `mcp.manage_catalog`) while `activate` refused it — visible but unusable. +/// +/// An unknown user id resolves to `false`; errors propagate, so callers fail +/// closed. +pub async fn effective_access(pool: &SqlitePool, catalog_name: &str, user_id: &str) -> Result { + if super::users::is_admin(pool, user_id).await? { + return Ok(true); + } + has_access(pool, catalog_name, user_id).await +} + // ── Writes ─────────────────────────────────────────────────────────────────── /// Grants a user access to a catalog entry. Idempotent on the PK. @@ -122,6 +147,11 @@ mod tests { sqlx::query("INSERT INTO users (id, username, role_id, encrypted) VALUES (?, ?, 'admin', 0)") .bind(id).bind(name).execute(&pool).await.unwrap(); } + // A non-admin, for the effective-access tests: only `admin` is seeded. + sqlx::query("INSERT INTO roles (id, label, permission_group) VALUES ('member', 'Member', 'default')") + .execute(&pool).await.unwrap(); + sqlx::query("INSERT INTO users (id, username, role_id, encrypted) VALUES ('m1', 'mallory', 'member', 0)") + .execute(&pool).await.unwrap(); for cat in ["gmail", "pokemon"] { sqlx::query("INSERT INTO mcp_catalog (name, scope, source) VALUES (?, 'per_user', 'remote')") .bind(cat).execute(&pool).await.unwrap(); @@ -171,4 +201,37 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + + #[tokio::test] + async fn an_admin_is_authorized_without_a_grant_row() { + // The regression this exists for: `access_defaults` deliberately writes no + // grant rows for admins, on the stated grounds that they hold every + // connector implicitly. Nothing implemented that here, so an admin was + // listed a connector (they hold `mcp.manage_catalog`) and then refused when + // they tried to activate it. + let (pool, dir) = registry_pool("admin-implicit").await; + + assert!(!has_access(&pool, "gmail", "u1").await.unwrap(), "no row, by design"); + assert!(effective_access(&pool, "gmail", "u1").await.unwrap(), "but an admin holds it"); + + let _ = std::fs::remove_dir_all(&dir); + } + + #[tokio::test] + async fn a_member_still_needs_the_grant() { + // The other half: the short-circuit must not have widened anything for + // anyone else. Deny-by-default is unchanged for a non-admin. + let (pool, dir) = registry_pool("member-denied").await; + + assert!(!effective_access(&pool, "gmail", "m1").await.unwrap()); + grant(&pool, "gmail", "m1").await.unwrap(); + assert!(effective_access(&pool, "gmail", "m1").await.unwrap()); + // And a connector they were not granted stays denied. + assert!(!effective_access(&pool, "pokemon", "m1").await.unwrap()); + + // An unknown user is nobody, not an admin. + assert!(!effective_access(&pool, "gmail", "ghost").await.unwrap()); + + let _ = std::fs::remove_dir_all(&dir); + } } diff --git a/crates/skald-core/src/db/mcp_global_access.rs b/crates/skald-core/src/db/mcp_global_access.rs index 8d97607..8ea0125 100644 --- a/crates/skald-core/src/db/mcp_global_access.rs +++ b/crates/skald-core/src/db/mcp_global_access.rs @@ -10,8 +10,8 @@ use sqlx::SqlitePool; // ── Reads ──────────────────────────────────────────────────────────────────── -/// The names of the **enabled** global servers a user may use. Feeds the -/// `accessible_global` snapshot captured when the user's context is built. +/// The names of the **enabled** global servers granted to a user by a row. The +/// roster read — for the runtime set, use [`effective_server_names_for_user`]. pub async fn server_names_for_user(pool: &SqlitePool, user_id: &str) -> Result> { let rows = sqlx::query_as::<_, (String,)>( "SELECT s.name @@ -26,6 +26,30 @@ pub async fn server_names_for_user(pool: &SqlitePool, user_id: &str) -> Result Result> { + if !super::users::is_admin(pool, user_id).await? { + return server_names_for_user(pool, user_id).await; + } + let rows = sqlx::query_as::<_, (String,)>( + "SELECT name FROM mcp_global_servers WHERE enabled = 1 ORDER BY name", + ) + .fetch_all(pool) + .await?; + Ok(rows.into_iter().map(|(n,)| n).collect()) +} + /// The ids of the users granted access to a given global server. pub async fn users_for_server(pool: &SqlitePool, server_id: i64) -> Result> { let rows = sqlx::query_as::<_, (String,)>( @@ -37,6 +61,9 @@ pub async fn users_for_server(pool: &SqlitePool, server_id: i64) -> Result Result { let row = sqlx::query_as::<_, (i64,)>( "SELECT 1 FROM mcp_global_access WHERE server_id = ? AND user_id = ?", @@ -48,6 +75,19 @@ pub async fn has_access(pool: &SqlitePool, server_id: i64, user_id: &str) -> Res Ok(row.is_some()) } +/// The authorization decision: may this user use this shared connector? +/// +/// Admins hold every connector implicitly — see +/// [`super::mcp_catalog_access::effective_access`] for why that short-circuit is +/// load-bearing rather than cosmetic (`access_defaults` skips seeding them rows +/// precisely because it is supposed to exist). +pub async fn effective_access(pool: &SqlitePool, server_id: i64, user_id: &str) -> Result { + if super::users::is_admin(pool, user_id).await? { + return Ok(true); + } + has_access(pool, server_id, user_id).await +} + // ── Writes ─────────────────────────────────────────────────────────────────── /// Grants a user access to a global server. Idempotent on the PK. @@ -108,3 +148,76 @@ pub async fn set_for_user(pool: &SqlitePool, user_id: &str, server_ids: &[i64]) tx.commit().await?; Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + use std::path::PathBuf; + + /// A registry-schema database with one admin, one member, and two global + /// servers — one of them disabled, since "enabled" is part of the answer. + async fn registry_pool(tag: &str) -> (SqlitePool, PathBuf, i64) { + use std::sync::atomic::{AtomicU64, Ordering}; + static SEQ: AtomicU64 = AtomicU64::new(0); + let n = SEQ.fetch_add(1, Ordering::Relaxed); + let dir = std::env::temp_dir() + .join(format!("skald-globalaccess-{}-{tag}-{n}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let pool = crate::db::init_system_pool(&dir.join("system.db").to_string_lossy()) + .await + .unwrap(); + + sqlx::query("INSERT INTO users (id, username, role_id, encrypted) VALUES ('adm', 'adm', 'admin', 0)") + .execute(&pool).await.unwrap(); + sqlx::query("INSERT INTO roles (id, label, permission_group) VALUES ('member', 'Member', 'default')") + .execute(&pool).await.unwrap(); + sqlx::query("INSERT INTO users (id, username, role_id, encrypted) VALUES ('mem', 'mem', 'member', 0)") + .execute(&pool).await.unwrap(); + + let sid = sqlx::query("INSERT INTO mcp_global_servers (name, enabled) VALUES ('websearch', 1)") + .execute(&pool).await.unwrap().last_insert_rowid(); + sqlx::query("INSERT INTO mcp_global_servers (name, enabled) VALUES ('offline', 0)") + .execute(&pool).await.unwrap(); + + (pool, dir, sid) + } + + #[tokio::test] + async fn an_admin_holds_every_enabled_global_without_a_row() { + let (pool, dir, sid) = registry_pool("admin-implicit").await; + + assert!(!has_access(&pool, sid, "adm").await.unwrap(), "no row, by design"); + assert!(effective_access(&pool, sid, "adm").await.unwrap()); + // The snapshot that decides which shared MCP tools the session is offered. + // A disabled server is still excluded — implicit access is not a bypass of + // the admin having switched something off. + assert_eq!( + effective_server_names_for_user(&pool, "adm").await.unwrap(), + vec!["websearch"], + ); + + let _ = std::fs::remove_dir_all(&dir); + } + + #[tokio::test] + async fn a_member_still_needs_the_grant() { + let (pool, dir, sid) = registry_pool("member-denied").await; + + assert!(!effective_access(&pool, sid, "mem").await.unwrap()); + assert!(effective_server_names_for_user(&pool, "mem").await.unwrap().is_empty()); + + grant(&pool, sid, "mem").await.unwrap(); + assert!(effective_access(&pool, sid, "mem").await.unwrap()); + assert_eq!( + effective_server_names_for_user(&pool, "mem").await.unwrap(), + vec!["websearch"], + ); + + // An unknown user is nobody, not an admin. + assert!(!effective_access(&pool, sid, "ghost").await.unwrap()); + assert!(effective_server_names_for_user(&pool, "ghost").await.unwrap().is_empty()); + + let _ = std::fs::remove_dir_all(&dir); + } +} diff --git a/crates/skald-core/src/db/plugin_access.rs b/crates/skald-core/src/db/plugin_access.rs index 861ce64..ed1ab61 100644 --- a/crates/skald-core/src/db/plugin_access.rs +++ b/crates/skald-core/src/db/plugin_access.rs @@ -49,15 +49,10 @@ pub async fn has_access(pool: &SqlitePool, plugin_id: &str, user_id: &str) -> Re /// otherwise the user must be granted in `plugin_access`. An unknown user id /// resolves to `false`. Errors propagate — the caller fails closed. pub async fn effective_access(pool: &SqlitePool, plugin_id: &str, user_id: &str) -> Result { - let role = sqlx::query_as::<_, (String,)>("SELECT role_id FROM users WHERE id = ?") - .bind(user_id) - .fetch_optional(pool) - .await?; - match role { - Some((r,)) if r == crate::db::roles::ADMIN_ROLE_ID => Ok(true), - Some(_) => has_access(pool, plugin_id, user_id).await, - None => Ok(false), + if crate::db::users::is_admin(pool, user_id).await? { + return Ok(true); } + has_access(pool, plugin_id, user_id).await } // ── Writes ─────────────────────────────────────────────────────────────────── diff --git a/crates/skald-core/src/db/users.rs b/crates/skald-core/src/db/users.rs index 7106e94..9f9d97d 100644 --- a/crates/skald-core/src/db/users.rs +++ b/crates/skald-core/src/db/users.rs @@ -272,6 +272,24 @@ pub async fn count(pool: &SqlitePool) -> Result { Ok(n) } +/// Whether this user holds the admin role — the one predicate behind every +/// "admins hold it implicitly" short-circuit (`plugin_access`, +/// `mcp_catalog_access`, `mcp_global_access`). +/// +/// It lives here, as one function, because the alternative is what actually +/// happened: each grant table open-coded the role lookup, one of them was written +/// without it, and admins were denied their own connectors while +/// [`super::access_defaults`] skipped seeding them rows on the grounds that the +/// short-circuit existed. An unknown user is not an admin; errors propagate so +/// callers fail closed. +pub async fn is_admin(pool: &SqlitePool, user_id: &str) -> Result { + let role = sqlx::query_as::<_, (String,)>("SELECT role_id FROM users WHERE id = ?") + .bind(user_id) + .fetch_optional(pool) + .await?; + Ok(matches!(role, Some((r,)) if r == super::roles::ADMIN_ROLE_ID)) +} + // ── Writes ──────────────────────────────────────────────────────────────────── /// `id` is supplied by the caller and must be opaque (never the username), so a diff --git a/crates/skald-core/src/loop_adapters/activation.rs b/crates/skald-core/src/loop_adapters/activation.rs index bce5f82..8690548 100644 --- a/crates/skald-core/src/loop_adapters/activation.rs +++ b/crates/skald-core/src/loop_adapters/activation.rs @@ -278,7 +278,7 @@ impl SkaldToolActivator { }; } let granted = self - .lookup(mcp_global_access::has_access(&self.shared_pool, row.id, &self.user_id).await, "mcp_global_access", name) + .lookup(mcp_global_access::effective_access(&self.shared_pool, row.id, &self.user_id).await, "mcp_global_access", name) .unwrap_or(false); if !granted { return GroupReport { @@ -322,7 +322,7 @@ impl SkaldToolActivator { }; } let authorized = self - .lookup(mcp_catalog_access::has_access(&self.shared_pool, name, &self.user_id).await, "mcp_catalog_access", name) + .lookup(mcp_catalog_access::effective_access(&self.shared_pool, name, &self.user_id).await, "mcp_catalog_access", name) .unwrap_or(false); return if authorized { GroupReport { diff --git a/crates/skald-core/src/skald/user_context.rs b/crates/skald-core/src/skald/user_context.rs index 7a0df8e..ff5092e 100644 --- a/crates/skald-core/src/skald/user_context.rs +++ b/crates/skald-core/src/skald/user_context.rs @@ -106,7 +106,7 @@ impl UserContext { /// without a restart (the §7 MCP twin of the §6 fs remount). pub async fn refresh_global_access(&self) -> anyhow::Result<()> { let names: std::collections::HashSet = - crate::db::mcp_global_access::server_names_for_user(&self.registry_pool, &self.user_id) + crate::db::mcp_global_access::effective_server_names_for_user(&self.registry_pool, &self.user_id) .await? .into_iter() .collect(); @@ -286,7 +286,7 @@ impl UserContextFactory { let mut startable = Vec::with_capacity(rows.len()); for r in rows { let allowed = match &r.catalog_name { - Some(cat) => crate::db::mcp_catalog_access::has_access(®istry, cat, &uid) + Some(cat) => crate::db::mcp_catalog_access::effective_access(®istry, cat, &uid) .await .unwrap_or(false), None => true, @@ -319,7 +319,7 @@ impl UserContextFactory { // unioned with their per-user runtime (§7). `accessible_global` is a // snapshot of `mcp_global_access`, captured at build time like fs membership. let accessible_global: std::collections::HashSet = - crate::db::mcp_global_access::server_names_for_user(&self.registry_pool, user_id) + crate::db::mcp_global_access::effective_server_names_for_user(&self.registry_pool, user_id) .await .unwrap_or_default() .into_iter() diff --git a/crates/skald-core/src/tools/mcp_report.rs b/crates/skald-core/src/tools/mcp_report.rs index caec0e5..88ed672 100644 --- a/crates/skald-core/src/tools/mcp_report.rs +++ b/crates/skald-core/src/tools/mcp_report.rs @@ -184,8 +184,10 @@ pub async fn build( } // ── Global connectors ──────────────────────────────────────────────────── + // Effective, not the raw roster: this report tells the agent what the user can + // use, and an admin holds every global connector without ever having a grant row. let granted_globals: HashSet = - mcp_global_access::server_names_for_user(registry, user_id).await? + mcp_global_access::effective_server_names_for_user(registry, user_id).await? .into_iter() .collect(); @@ -448,10 +450,13 @@ mod tests { assert!(!rendered.contains("secret"), "ungranted global leaked: {rendered}"); } - /// A catalogue manager must see what they have not granted themselves, or - /// they cannot reason about the instance they administer. + /// An admin holds every global connector implicitly and is deliberately never + /// given a grant row, so the report must describe one as *theirs* — here + /// "enabled but not connected" — rather than as something granted to somebody + /// else. Reporting `not_granted` was the visible face of the bug that also + /// refused them activation and gave their sessions no shared MCP tools at all. #[tokio::test] - async fn a_catalog_manager_sees_ungranted_globals() { + async fn an_admin_holds_globals_without_a_grant_row() { let dir = temp_dir("admin"); std::fs::create_dir_all(&dir).unwrap(); let registry = crate::db::init_system_pool(dir.join("system.db").to_str().unwrap()) @@ -465,6 +470,34 @@ mod tests { let out = build(®istry, &owner, "a1", 1, None).await.unwrap(); + assert_eq!(out["your_role"]["can_manage_catalog"], true); + assert!(ids(&out["installable"]).is_empty(), "an admin is not missing a grant"); + assert_eq!(state_of(&out["needs_setup"], "tavily"), "not_running"); + } + + /// The `not_granted` branch is still live — for a *non-admin* who was given the + /// catalog-management capability. They must see a connector they have not + /// granted themselves, or they cannot reason about the instance they curate, + /// but they genuinely do not hold it. + #[tokio::test] + async fn a_non_admin_catalog_manager_sees_ungranted_globals() { + let dir = temp_dir("curator"); + std::fs::create_dir_all(&dir).unwrap(); + let registry = crate::db::init_system_pool(dir.join("system.db").to_str().unwrap()) + .await.unwrap(); + let owner = crate::db::create_user_pool(&dir.join("c1.db"), None).await.unwrap(); + + seed_member(®istry).await; + sqlx::query("INSERT INTO role_capabilities (role_id, capability) VALUES ('member', ?)") + .bind(role_capabilities::MANAGE_CATALOG) + .execute(®istry).await.unwrap(); + sqlx::query("INSERT INTO users (id, username, role_id, encrypted) VALUES ('c1', 'c1', 'member', 0)") + .execute(®istry).await.unwrap(); + sqlx::query("INSERT INTO mcp_global_servers (id, name, enabled) VALUES (1, 'tavily', 1)") + .execute(®istry).await.unwrap(); + + let out = build(®istry, &owner, "c1", 1, None).await.unwrap(); + assert_eq!(out["your_role"]["can_manage_catalog"], true); assert_eq!(ids(&out["installable"]), ["tavily"]); assert_eq!(state_of(&out["installable"], "tavily"), "not_granted"); diff --git a/src/frontend/api/mcp.rs b/src/frontend/api/mcp.rs index 095c9aa..961eb3d 100644 --- a/src/frontend/api/mcp.rs +++ b/src/frontend/api/mcp.rs @@ -837,6 +837,11 @@ pub async fn available( .ok_or_else(|| ApiError::unauthorized("unknown user"))?; let manages_catalog = role_capabilities::has(skald.db(), &user.role_id, role_capabilities::MANAGE_CATALOG).await?; + // Admins hold every connector implicitly and are deliberately never given grant + // rows (see `mcp_catalog_access::effective_access`), so every "may I use this" + // answer below has to OR this in — otherwise the page shows an admin their own + // connectors greyed out as unusable. + let is_admin = skald_core::db::users::is_admin(skald.db(), &auth.user_id).await?; let granted_catalog: std::collections::HashSet = mcp_catalog_access::catalog_names_for_user(skald.db(), &auth.user_id).await? @@ -863,7 +868,7 @@ pub async fn available( // entry enabled for someone else becomes invisible and unmanageable. .filter(|r| manages_catalog || granted.contains(&r.name)) .map(|r| GlobalView { - can_use: granted.contains(&r.name), + can_use: is_admin || granted.contains(&r.name), id: r.id, name: r.name, catalog_name: r.catalog_name, @@ -919,7 +924,7 @@ pub async fn activate( // Deny-by-default per-user access: the admin must have granted this user // the connector (`mcp_catalog_access`). This is the real boundary — the // `available` list only hides it in the UI. - if !mcp_catalog_access::has_access(skald.db(), cat_name, &auth.user_id).await? { + if !mcp_catalog_access::effective_access(skald.db(), cat_name, &auth.user_id).await? { return Err(ApiError::forbidden( "you are not authorized to use this connector — ask an admin to enable it for you", ));