agentic-community / agentic-community/mcp-gateway-registry

Replace inferred admin status with an explicit is_admin scope flag, gated by registry-admins membership

Ouverte
#1,315 1 commentaire 0 réactions 0 personnes assignées Voir sur GitHub
architecture enhancement security
Langage dominant
Python
Étoiles
911
Forks
234
Merge moyen
1 j 11 h
PR mergées (30 j)
62

Description

# Replace inferred admin status with an explicit `is_admin` scope flag, gated by `registry-admins` membership

## Summary

Today a user's admin status is **inferred** from their UI permissions rather than **declared**. `_user_is_admin()` returns `True` if any *mutating* UI action (`register_`, `modify_`, `toggle_`, `delete_`, `publish_`, `create_`) has the literal value `"all"`. This is fragile and causes accidental privilege escalation.

This issue proposes making admin status an **explicit boolean flag** on the scope, and restricting **who can set that flag** to members of the existing bootstrap `registry-admins` group.

**Decisions locked in:**

- Admin status is modeled as an `is_admin` **boolean field** on the scope document (not a separate `admin_scope` type).
- The **only** identity allowed to set `is_admin: true` is a member of `registry-admins`. `mcp-registry-admin` (the human SSO admin group) can manage ordinary groups but cannot mint new admin-conferring groups.

The two existing admin groups stay exactly as they are. Nothing about their membership, origin, or day-to-day behavior changes, except that both seed scope files gain `"is_admin": true`.

## Motivation

While building a non-admin "read-write" group (can list + register servers + run health checks, but cannot toggle / modify / delete), the group was accidentally promoted to full admin. The cause: it set `register_service: ["all"]`, and `register_` is a mutating prefix, so `_user_is_admin()` flagged it as admin. The symptom was the settings gear, delete buttons, toggle switches, and the "Admin Access" badge all appearing in the UI.

The only reason the workaround (`register_service: ["*"]`) works is that the admin heuristic keys on the literal string `"all"` and ignores `"*"`, even though `"*"` and `"all"` are treated as equivalent wildcards almost everywhere else. That is a sharp edge that the next person will trip over.

## Current behavior (for reference)

- `_user_is_admin()` and `_ADMIN_ACTION_PREFIXES`: `registry/auth/dependencies.py` (~lines 343-381)
- Admin status is then consumed all over the UI (`user.is_admin` in `frontend/src/pages/Dashboard.tsx`, `Layout.tsx`, `Sidebar.tsx`, `ServerCard.tsx`) and in many backend route guards.
- The two admin groups are seeded from disk at DB init by `scripts/init-mongodb-ce.py` -> `_load_default_scopes()`, which loads `scripts/registry-admins.json` and `scripts/mcp-registry-admin.json` into the `mcp_scopes` collection.
- `registry-admins` is NOT a Keycloak realm group (it is the bootstrap / M2M identity, seeded by `cli/bootstrap_user_and_m2m_setup.sh`). `mcp-registry-admin` IS a Keycloak realm group (`keycloak/import/realm-config.json`) used by human SSO admins.

## Proposed change

### 1. Scope schema: add `is_admin`

Add an optional `is_admin: bool = False` field to the scope/group schema (`registry/schemas/management.py` group models) and to the `import_group` path (`registry/services/scope_service.py`, `registry/api/server_routes.py` import-group endpoint, scope repository `import_group`).

### 2. Seed files: set the flag

Add `"is_admin": true` to both:

- `scripts/registry-admins.json`
- `scripts/mcp-registry-admin.json`

No other change to these files. The existing DB-init loader carries the flag in for free on fresh installs; a one-time migration stamps it onto existing deployments (see Backwards compatibility below).

### 3. Admin determination becomes trivial

`_user_is_admin()` no longer inspects UI permission patterns. Instead, a user is admin if **any of their resolved scopes has `is_admin: true`**. The mutating-prefix heuristic and `_ADMIN_ACTION_PREFIXES` are removed.

Net effect: a group can grant `register_service: ["all"]` (or any wildcard permission) WITHOUT becoming admin. Admin is now opt-in and explicit.

### 4. Only `registry-admins` can set `is_admin: true` (escalation guard)

When the import-group / create-group / group-update endpoints receive a scope with `is_admin: true` (or a change to that flag), the API verifies the **calling identity** (`user_context["groups"]`) is a member of `registry-admins`. If not, return 403.

- `registry-admins` is the bootstrap group, seeded only from disk and not creatable through the API, so it is a tamper-resistant root of trust.
- A normal operational admin can still manage ordinary (non-admin) groups, but cannot mint new admin-conferring groups.

The building block already exists: `user_can_modify_servers()` does a hardcoded membership check against `registry-admins` / `mcp-registry-admin` (`registry/auth/dependencies.py` ~lines 420-423). A new `_caller_can_grant_admin(user_context)` helper would follow the same pattern.

## Acceptance criteria

- [ ] Scope schema supports an `is_admin` boolean (default `false`).
- [ ] `scripts/registry-admins.json` and `scripts/mcp-registry-admin.json` set `"is_admin": true`.
- [ ] `_user_is_admin()` returns admin status based solely on the `is_admin` flag of the user's resolved scopes; the mutating-prefix heuristic is removed.
- [ ] A non-admin group with `register_service: ["all"]` does NOT resolve to admin.
- [ ] import-group / create-group / group-update reject a scope carrying `is_admin: true` unless the caller is in `registry-admins`.
- [ ] A migration stamps `is_admin: true` onto the two existing admin scopes in DocumentDB for already-deployed installs.
- [ ] Existing admin users (human SSO via `mcp-registry-admin`, and M2M via `registry-admins`) retain full admin after migration.
- [ ] Tests: non-admin-with-wildcard stays non-admin; admin-flag scope is admin; non-`registry-admins` caller cannot create an admin group; migration idempotency.
- [ ] Docs updated (`docs/scopes.md`, `docs/scopes-mgmt.md`, `docs/read-write-non-admin-groups.md`) to describe the explicit flag and the escalation rule.

## Backwards compatibility

- Any deployment relying on the old inference (a custom group that used a mutating `"all"` permission to gain admin) will lose admin after this change unless its scope gets `is_admin: true`. This is intended (it is the bug being fixed) but must be called out in release notes.
- The migration must be idempotent and safe to run on installs that already have the flag.

## Open questions

1. Do we also guard **modifying** an existing admin scope (not just creating one) and **clearing** the escalation flag? (Recommended: yes, guard all writes that set or clear `is_admin`, so a non-`registry-admins` caller can neither grant nor strip admin.)
2. Should the `registry-admins` membership check read from the caller's token `groups` claim, their resolved scope `group_mappings`, or both? (Recommended: accept either, mirroring `user_can_modify_servers()` which checks groups and scopes.)

## Out of scope

- Removing or merging `registry-admins` vs `mcp-registry-admin`. They serve distinct roles (M2M/bootstrap root-of-trust vs Keycloak human-admin) and several endpoints gate on one or the other. They stay as-is.

Guide de contribution

Ouvrir le guide de contribution

Évaluation

Cette issue n'a pas encore été évaluée.

Recevez les nouvelles issues par e-mail

Un résumé court des issues GitHub adaptées aux débutants.