fix: let an admin use the connectors they implicitly hold
Nightly Build / build (push) Successful in 7m50s
Nightly Build / build (push) Successful in 7m50s
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.
This commit is contained in:
@@ -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<bool> {
|
||||
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<bool> {
|
||||
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);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<Vec<String>> {
|
||||
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<V
|
||||
Ok(rows.into_iter().map(|(n,)| n).collect())
|
||||
}
|
||||
|
||||
/// The enabled global servers a user may actually use. Feeds the
|
||||
/// `accessible_global` snapshot captured when the user's context is built, and so
|
||||
/// decides which shared MCP tools their agent is offered at all.
|
||||
///
|
||||
/// An admin gets every enabled server, because they are never given grant rows
|
||||
/// (see [`effective_access`]). Without this an admin's session snapshotted an
|
||||
/// empty set and simply had no shared connectors — the same root cause as being
|
||||
/// refused activation, one layer down and much quieter, since nothing errors: the
|
||||
/// tools are just absent.
|
||||
pub async fn effective_server_names_for_user(
|
||||
pool: &SqlitePool,
|
||||
user_id: &str,
|
||||
) -> Result<Vec<String>> {
|
||||
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<Vec<String>> {
|
||||
let rows = sqlx::query_as::<_, (String,)>(
|
||||
@@ -37,6 +61,9 @@ pub async fn users_for_server(pool: &SqlitePool, server_id: i64) -> Result<Vec<S
|
||||
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. For "may this user use it", use [`effective_access`].
|
||||
pub async fn has_access(pool: &SqlitePool, server_id: i64, user_id: &str) -> Result<bool> {
|
||||
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<bool> {
|
||||
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);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<bool> {
|
||||
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 ───────────────────────────────────────────────────────────────────
|
||||
|
||||
@@ -272,6 +272,24 @@ pub async fn count(pool: &SqlitePool) -> Result<i64> {
|
||||
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<bool> {
|
||||
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
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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<String> =
|
||||
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<String> =
|
||||
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()
|
||||
|
||||
@@ -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<String> =
|
||||
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");
|
||||
|
||||
Reference in New Issue
Block a user