starred/onyx
1
0 Fork 0
mirror of https://github.com/onyx-dot-app/onyx.git synced 2026-10-19 01:50:51 -05:00
Code Issues Packages Projects Releases 10 Wiki Activity

feat(permissions):new group based permission system (#10172)

Browse Source 
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
SubashMohanandClaude Opus 4.7 authored and GitHub committed 2026-08-17 16:32:51 +00:00
1 parent ab070335d6
commit be8cfa6144
349 files changed
+22929 -6582
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

docs/group-manager-scoped-permissions/00-index.md
+53
View File
@@ -0,0 +1,53 @@
> Status: draft → active · Task: group-manager-scoped-permissions
# §8 Scoped Permissions (Group Manager) — Spec Index
Implementation spec for **§8** of the Group-Based Permissions System V2. The base system (§1–7) is built on
`new-permission-system`; §8 is the one remaining, fully-greenfield section.
Source of truth (wiki): `Engineering Projects/Group-Based Permissions System V2/solution-design.md` §8.
| # | Doc | Contents | Status |
|---|---|---|---|
| 01 | [01-research.md](01-research.md) | Requirement · codebase verification (§8 absent / §1–7 built) · industry backing · locked wiki §8 approach + carried-in must-fixes · migration trap | ✅ |
| 02 | [02-high-level-design.md](02-high-level-design.md) | Plain-language whole-approach: two-gate model, live scope, data flow, scenario, decisions that matter | ✅ |
| 03 | [03-detailed-design.md](03-detailed-design.md) | DB design (+rationale) · auth primitives · ~4 filter rewrites (+skill, +token-limit; §11) · write-path gate insertions · PAT · API · FE · file tree · pre-impl notes · open decisions | ✅ |
| 04 | [04-implementation-plan.md](04-implementation-plan.md) | CLAUDE.md-format plan + plan-challenge results (passed all 6) | ✅ |
| 05 | [05-pr-roadmap.md](05-pr-roadmap.md) | 6 ordered PRs (~2.3k LOC) w/ drift checkpoints + lands-together safety invariant | ✅ |
| 07 | [07-ui-capability-model-design.md](07-ui-capability-model-design.md) | **UI redesign** (server-driven capability model): kills client-side `visibilityPermissions()`; `/me.admin_capabilities` (coarse) + per-resource `permissions{}` map (per-item) stamped by a shared `can_<action>` helper the write-guard also calls (drift impossible by construction); `<Can>` primitive; 49/109 affordance audit + complete mapping + real contract test + 5-PR roadmap. **v2** — revised after a 34-finding adversarial pressure test | 🆕 design (v2) |
| 10 | [10-branch-audit-anon-and-service-accounts.md](10-branch-audit-anon-and-service-accounts.md) | Independent branch audit: anonymous-access regression check (none found) · groupless-service-account behavior spec · **CRITICAL live-confirmed escalation** (GLOBAL `manage:user_groups` holder can self-add to the Admin default group) · 31 more ranked findings | ✅ audit |
**Decisions locked at GATE 2:** D1 cache `is_group_manager` boolean (route gate zero-query; managed-list stays
live) · D2 admins-only create groups · D3 admin-or-manager-of-that-group assigns managers.
**Decisions locked at the 2026-06-29 regression + GO/NO-GO reviews:** D4 actions = `manage:actions`
**stays in the bundle** (GATE 1 reach), scoped via the agents that reference them at GATE 2 · D5 skills = a 7th scoped resource under a **new dedicated
`manage:skills` permission** (grantable in the groups UI + in the bundle; no DB migration) · D6 managers may do
everything **except delete** · **D7 attaching an agent to a group is controlled by `manage:agents`** (standard GATE 2 keyed on
`MANAGE_AGENTS` — admins/global holders self-share to their groups, scoped managers to managed groups;
`add:agents`-only users can't group-share). The reviews confirmed PAT,
chat-runtime, and document/Vespa ACL are untouched, and refuted the backfill data-loss concern. Full
case-by-case coverage + the boot-bug prerequisite are in **[03 §11](03-detailed-design.md)** — the
authoritative implementation checklist.
**Decision locked 2026-08-03:** **D8 action + MCP-server _management_ (edit / toggle / OAuth-auth) is
owner-or-admin, NOT agent-mediated.** D4 still holds for GATE 1 reach + create (a scoped manager reaches
the action/MCP routes and may create), but the agent-mediated GATE 2 that once let a manager edit any
action whose referencing agents were all in their managed groups was **dropped** — it was the source of
most of the review's P1/P2 findings, and the simpler owner-or-creator gate (`can_manage_own_tool` /
`_ensure_mcp_server_owner_or_admin`, mirrored 1:1 by the UI projection) is the intended final design. Delete
follows D9 below. This **supersedes** the "agent-mediated GATE 2 replaces owner-or-admin" language
still in [03 §11](03-detailed-design.md), [05](05-pr-roadmap.md), and [07](07-ui-capability-model-design.md).
**Decision locked 2026-08-04:** **D9 delete/publish follow ownership — D6's "except delete" scopes
managed-group resources, not ones the manager created.** A scoped manager may delete a custom action or
MCP server *they created*, and publish an agent *they own*, exactly as any other owner may: the gate is
owner-or-admin (`can_manage_own_tool`, `_ensure_mcp_server_owner_or_admin`, `can_delete_persona`), and
being a manager never subtracts a right they'd hold as an ordinary user. D6 still holds where it was
aimed — a manager may not delete a connector, document set, or agent that merely sits in a group they
manage. Pinned by `test_permission_projection_contract.py` ("the creator fully controls the action they
made"); `assert_within_scope` in `_assert_persona_update_within_managed_scope` therefore does **not**
gate `is_public`, and the EE group-share gate exempts an owner whose group shares are unchanged —
**shares, not just group ids**: re-leveling a group from VIEWER to EDITOR is a change and gets no
exemption. What ownership never buys is *widening* — sharing a public agent into a managed group still
rejects, because that would capture it into scope.
docs/group-manager-scoped-permissions/01-research.md
+158
View File
@@ -0,0 +1,158 @@
> Status: active · Task: group-manager-scoped-permissions
# §8 Scoped Permissions (Group Manager) — Research
> **Historical research.** Final primitive names/shapes differ — see [03 §2](03-detailed-design.md):
> `has_permission` returns `PermissionAuthority` (the one classifier; the proposed `has_permission_or_scope`
> was folded into it), and the write gate is `assert_within_scope` / `assert_global` (not
> `assert_group_set_within_scope`). The names below reflect the original investigation, not the shipped API.
## Requirement
Implement **§8 of the Group-Based Permissions System V2** solution design — the **Group Manager**
(scoped-permissions) layer — on the `new-permission-system` branch. A Group Manager is a user given
admin-like control over a **single group's** resources and members, and nothing outside it.
Source of truth: wiki `Engineering Projects/Group-Based Permissions System V2/solution-design.md` §8.
## Clarifications
| Question | Answer |
|---|---|
| How should this run treat §8's design? | **Adopt the wiki §8 model as-is** (chosen approach is locked — no approach generation). The 2026-06-23 46-agent adversarial review already settled the design; §8 was rewritten to the FINAL model on 2026-06-24. |
| Scope | All of §8 (§8.1–§8.5): the `is_manager` flag + migration, the scoped-permission bundle, live scope resolution, two-gate enforcement, filter re-keying, PAT intersection, and the Group-Manager assignment UI. |
## Current status & reuse (from codebase scan — `OnyxFolder/onyx`, branch `new-permission-system`)
### §8 is entirely greenfield — every artifact ABSENT
| §8 artifact | Status | Evidence |
|---|---|---|
| `is_manager` column on `User__UserGroup` | ABSENT at research time → **BUILT (PR1)** | now on `User__UserGroup`; `User.is_group_manager` cached flag added too |
| Alembic migration (`is_manager` add/backfill) | ABSENT at research time → **BUILT (PR1) as `c71a18ea7d07`** | down_revision `c8e316473aaa`, now head; placeholder `4fa09af6ca14` never used |
| `SCOPED_MANAGER_PERMISSIONS` bundle | ABSENT | no match in backend |
| `get_scoped_groups()` resolver | ABSENT | no match |
| `assert_group_set_within_scope` / `can_act_on_resource` write-side gate | ABSENT | no match |
| `make_group_manager` / `revoke_group_manager` | ABSENT | no match |
| `has_permission_or_scope` route-gate variant | ABSENT | only plain `has_permission` — `backend/onyx/auth/permissions.py:252` |
| `is_manager` boolean inside `effective_permissions` | ABSENT | `effective_permissions` is `Mapped[list[str]]` tokens only — `models.py:375` |
| Group-Manager assignment UI (web) | ABSENT | no manager UI under `web/src/app/(ee/)admin/groups*` |
| PAT scope-intersection w/ manager scope | ABSENT | `backend/onyx/db/pat.py` scopes are flat permission tokens |
> **⚠ Status (updated):** at research time §8 was entirely unbuilt and the wiki's *"Implemented as revision
> `4fa09af6ca14`"* was wrong. Since then **PR0+PR1 shipped** (migration `c71a18ea7d07`); scoped enforcement
> (PR2+) is still not-yet-built.
### Base system (§1–7) — FULLY BUILT (the foundation §8 extends)
| Building block | Path |
|---|---|
| `AccountType` enum (STANDARD/BOT/EXT_PERM_USER/SERVICE_ACCOUNT/ANONYMOUS) | `backend/onyx/db/enums.py:7-28` |
| `account_type` column on `User` | `backend/onyx/db/models.py:324-329` |
| `Permission` enum (token set) | `backend/onyx/db/enums.py:490-549` |
| `PermissionGrant` model (`(group_id, permission)` unique) | `backend/onyx/db/models.py:4371-4391` |
| `require_permission(...)` FastAPI dep | `backend/onyx/auth/permissions.py:257-289` |
| `has_permission(...)` (non-FastAPI) | `backend/onyx/auth/permissions.py:252` |
| `resolve_effective_permissions()` + `IMPLIED_PERMISSIONS` | `backend/onyx/auth/permissions.py:214-231`, `:32-71` |
| `get_effective_permissions()` (reads `User.effective_permissions`) | `backend/onyx/auth/permissions.py:234-249` |
| 6× `_add_user_filters` (token-based, **no** `role`/`is_curator`) | `connector_credential_pair.py:50`, `persona.py:77`, `document_set.py:41`, `credentials.py:41`, `feedback.py:46`, EE `token_limit.py` |
| `User__UserGroup` membership model (where `is_manager` lands) | `backend/onyx/db/models.py:~4361` |
| PAT model | `backend/onyx/db/pat.py` |
### Residual tombstones (kept by design; §8 reuses / must not break)
- `role` column + `UserRole` enum — `models.py:320-323` (nullable, "Legacy tombstone").
- `is_curator` column on `User__UserGroup` — `models.py:4361` (to be **repurposed** as `is_manager`).
- 3 surviving `user.role == UserRole.ADMIN` readers: `persona_sharing.py:53`, `build_session.py:638`,
`search/api.py:104`. These block dropping `role`; **out of scope** for §8 (deferred cleanup release).
**Reuse posture:** §8 adds one column, one code-defined permission bundle, a handful of resolver/gate helpers,
and re-keys ~4 editable filters (connector, document_set, persona, skill — credentials + feedback unchanged;
see [03 §11.7](03-detailed-design.md)) from "membership" to "managed groups." It does **not** add tables, does
**not** touch `permission_grant` (stays global-only), and does **not** add a second auth round-trip.
## Industry best practices (backing for the locked design)
The chosen model maps to established scoped-RBAC patterns (validated in the prior adversarial review):
- **Role binding, not a permission row** — Group Manager = a binding of a *role* to a (user, group) edge, like
Kubernetes `RoleBinding` (namespace-scoped) vs `ClusterRoleBinding` (global). A role ≠ an atomic permission
(NIST RBAC, k8s, Google Zanzibar all keep permissions atomic and bind roles separately). → §8 keeps the
manager bundle out of `permission_grant`.
- **Permission boundaries narrow, never widen** — AWS IAM permission boundaries / SCPs cap effective access; a
scoped principal can only intersect. → §8.5 PAT intersection: a scoped token can only narrow, never widen.
- **Resolve scope live; don't materialize it** — Azure scoped RBAC and Zanzibar resolve the scope set at check
time from the relationship graph rather than caching a denormalized list, avoiding stale-after-move bugs. →
§8.1 caches only an `is_manager` boolean; the managed-group **list** is resolved live per request.
- **Authorization of record at the write, not the route** — defense-in-depth / "don't trust the client's
object list": the mutating layer must re-read the resource's current owners and re-check, because the route
filter only hides things from the UI. → §8.2 per-resource gate runs inside the DB write.
## Chosen approach — the locked wiki §8 model
**One-line:** a Group Manager is a single boolean `is_manager` on the membership row; their abilities are a
code-defined bundle applied **only** to the groups they manage; scope is resolved **live**; and every manager
action passes **two gates** — a coarse route gate (cached) and a per-resource write-side gate (authoritative).
### The five pillars
1. **Who** — `is_manager` boolean on `user__user_group` (repurposes the dead `is_curator` column). No new
table, no new row. Semantically a role binding on the membership edge.
2. **What** — `SCOPED_MANAGER_PERMISSIONS = {manage:connectors, manage:document_sets, manage:agents,
add:agents, manage:user_groups}`, expanded live at resolve time, applied only to managed groups.
**Never** merged into `effective_permissions.global`. (Per the 2026-06-29 review — D4 — `manage:actions`
**stays in the bundle** so GATE 1 admits managers; scope is resolved at GATE 2 via the agents that
reference the action. Skills are added as a 7th scoped resource under a new dedicated `manage:skills`
token (D5). See [03 §11](03-detailed-design.md).)
3. **Cached vs live** — `User.effective_permissions` carries global tokens **plus an `is_manager` boolean**
(so the route gate needs no extra query). The managed-group **list** is read live by
`get_scoped_groups(user)` (one indexed read on `user_id WHERE is_manager=true`) — never cached, so never
stale.
4. **Two-gate enforcement:**
- **Route gate** `has_permission_or_scope` — coarse, cached; lets a manager *reach* the endpoint. Can only
reject; never authorizes.
- **Per-resource gate** `assert_group_set_within_scope` — the authorization of record; runs **inside the
write**, re-reads the resource's current groups in-transaction, allows only if the resource ends up in
**≥1 managed group, none outside, and PRIVATE**.
5. **What a manager can/can't do** — create/edit/attach/detach resources fully inside managed groups; add/remove
members of managed groups. **Cannot** edit a group's permissions, act on out-of-scope resources, or make
anything PUBLIC/SYNC. Admins bypass all of it.
### Carried-in must-fixes / confirmed risks (from the 46-agent review — design inputs, not open questions)
These are already folded into the locked design; the implementation must honor each:
1. **Write-side gate is mandatory & centralized.** Put `assert_group_set_within_scope` inside *every*
group-mutating DB fn (`add_users`, `update_group`/agents, resource group-attach). Route gate is only a
pre-filter. `set_group_permissions` stays **admin-only** (a manager cannot grant tokens).
2. **PUBLIC/SYNC is an orthogonal axis** — scoped managers are **PRIVATE-only**; reject any create/edit that
sets or keeps `access_type` PUBLIC or SYNC.
3. **Filters key on managed groups, not membership** — `get_scoped_groups`, not "all of the user's groups."
Filters are heterogeneous (e.g. document_set editable filter is `sa_false()` today and must be built).
4. **Fail closed on empty scope** — empty managed-group set ⇒ no access, never "no filter"; guard against
`IN ()` / dropped `WHERE` matching everything.
5. **Don't trust "they can't see it"** — a direct API call by resource ID bypasses the listing filter; the
write must load the resource's **current** groups, not just the client-supplied new set (capture-by-reassign
attack, e.g. `PUT /connector/<Finance id> {groups:[Engineering]}`).
6. **Bulk/list endpoints check every item**, not the first or the aggregate.
7. **Live resolution kills the staleness class** — no pre-expansion / materialization of scoped perms; resolve
at check time (deletes pre-expansion staleness + `@validates` divergence + write-time fan-out).
### Migration (the disappearing-tombstone trap — §6.1.1 / §6.1.2)
- Backfill `is_manager` is **not a rename** — `is_curator` alone misses GLOBAL_CURATOR (they have no
per-group `is_curator` rows). Compute it:
- `is_manager=true` where `is_curator=true` **AND** `user.role='CURATOR'`;
- `is_manager=true` on **every** membership where `user.role='GLOBAL_CURATOR'`.
- Must run **before** `role` is dropped, or GLOBAL_CURATOR is lost permanently (the `account_type` backfill
already collapsed CURATOR/GLOBAL_CURATOR→STANDARD, so `role`+`is_curator` are the only surviving signal).
- Migration goes in `backend/alembic/versions/` (tenant schema), **not** `alembic_tenants/`.
- Ship **additive only** with the feature (add column + backfill + reuse existing `ix_user__user_group_user_id`);
**defer** dropping `is_curator`/`role` to a later cleanup release (rollback-safe; not GA).
- Emit a per-user migration report flagging any CURATOR/GLOBAL_CURATOR mapping to **zero** managed groups
(snapshot caveat — GLOBAL_CURATOR was dynamic; migrated set does not auto-extend to groups joined later).
## Chosen approach
**Locked: the wiki §8 settled model above** (user selected "Adopt wiki §8 as-is"). No competing approaches
generated — the design was already settled by the 2026-06-23 adversarial review. Proceed to high-level design.
docs/group-manager-scoped-permissions/02-high-level-design.md
+211
View File
@@ -0,0 +1,211 @@
> Status: active · Task: group-manager-scoped-permissions
# §8 Scoped Permissions (Group Manager) — High-Level Design
> **Primitives superseded by [03 §2](03-detailed-design.md) (single-classifier model).** `has_permission`
> now returns `PermissionAuthority` (GLOBAL/SCOPED/NONE) — it is the one classifier; the separate `has_permission_or_scope`
> is gone. GATE 1 is `require_permission(..., allow_scope=True)` (a threshold on `has_permission`); GATE 2 is
> `assert_within_scope` / `assert_global`. Names below are updated to match; see 03 §2 for signatures.
> **Revised by the 2026-06-29 regression review.** Bundle/coverage decisions here are updated by
> **D4** (`manage:actions` stays in the bundle — GATE 1 reach + create; its agent-mediated GATE 2 was later
> dropped, see **D8**), **D5** (skills = a 7th scoped resource under a
> new `manage:skills` token), **D6** (managers do everything **except delete a resource that merely sits in a
> managed group** — deleting something they *created* is ownership, see **D9**) and **D7** (attaching an agent
> to a group is controlled by `manage:agents`). The authoritative, complete case list lives in
> [03 §11](03-detailed-design.md). Also a hard prerequisite: the broken `current_curator_or_admin_user`
> import (03 §11.0) must be fixed or the API server won't boot.
## What we're building, in one paragraph
The base system (§1–7) grants permission tokens to a whole **group** — every member gets them, **everywhere**.
A **Group Manager** is the controlled exception: one user given admin-like control over a **single group's**
resources and members, and *nothing* outside it. We add this with **one boolean** (`is_manager` on the
membership row), **one code-defined permission bundle** (`SCOPED_MANAGER_PERMISSIONS`), **live** scope
resolution (never cached, never stale), and a **two-gate** enforcement model whose authoritative check runs
*inside the database write*, not at the route. No new tables, no new rows, no second auth round-trip.
## The core idea: scope lives on the membership edge, abilities live in code
```
user__user_group (the membership EDGE)
┌───────┐ ┌───────────────────────────────────┐ ┌────────────┐
│ Alice │──▶│ user_id=Alice group_id=Engineering│──▶│ Engineering│
└───────┘ │ is_manager = TRUE ◀── the only new │ │ (group) │
│ bit of state │ └────────────┘
└───────────────────────────────────┘
│
is_manager=TRUE ⇒ apply SCOPED_MANAGER_PERMISSIONS (in code)
│ {manage:connectors, manage:document_sets,
▼ manage:agents, add:agents, manage:user_groups,
but ONLY to manage:skills, manage:actions}
Engineering's
resources — resolved LIVE
EXCEPT manage:actions — a custom action or MCP server belongs to no group, so there is
nothing to scope it by. The bundle grants GATE 1 reach + create only; managing an
existing one is plain owner-or-admin (D8). Agent-derived scope survives solely for
*viewing* an MCP server connected to a managed group. See 03 §11.1.
```
Two things never live in the database as data:
- **The manager's abilities** are a constant set in code (`SCOPED_MANAGER_PERMISSIONS`), *not* rows in
`permission_grant`. `permission_grant` stays **global-only**. (A role binding ≠ a permission — the same split
k8s, AWS IAM, and Zanzibar make.)
- **The manager's scope** (which groups) is *not* cached — it is resolved live from `is_manager` on every
request that needs it (`get_scoped_groups`). One indexed read. Because a manager is always a member, and
membership is the source, the scope can never go stale after a group is renamed, moved, or deleted.
## The two gates — the heart of the design
Every manager action passes **two** independent checks. The first lets them *reach* the code; the second is the
*authorization of record*.
```
Manager calls PUT /document-set {id: 7, groups:[Engineering]}
│
▼
┌─────────────────────────── GATE 1: ROUTE GATE ───────────────────────────┐
│ require_permission(...) → has_permission(MANAGE_DOCUMENT_SETS) │
│ passes if: holds the token GLOBALLY OR manages ANY group. │
│ COARSE. Can only reject. Does NOT authorize the action. │
└──────────────────────────────────────────────────────────────────────────┘
│ (reached the handler)
▼
┌──────────────────── GATE 2: PER-RESOURCE WRITE-SIDE GATE ─────────────────┐
│ assert_within_scope(user, resource, new_groups, access_type) │
│ runs INSIDE the DB write, in the same transaction. Re-reads the │
│ resource's CURRENT groups from the DB (not the client's list). Allows │
│ only if the resource ends up: │
│ • in ≥1 managed group, │
│ • with NO group outside the managed set (current ∪ new ⊆ managed), │
│ • non-PUBLIC (PRIVATE or SYNC; never PUBLIC). │
│ Admin override (admin token) skips this entirely. │
└──────────────────────────────────────────────────────────────────────────┘
│
▼
write commits (or OnyxError FORBIDDEN)
```
**Why two gates and not one?** The route gate is cheap and coarse — it answers "could this user *ever* do this?"
without knowing the specific resource. It exists so a manager isn't 403'd before the handler can even load the
resource. But it is *not* safe on its own: a manager holds `manage:document_sets` "scopedly," so a global-style
route check would let them edit *any* document set. The real decision needs the resource's actual group
membership, which only the handler/DB layer has — so the authoritative check lives there, re-reading current
state in-transaction. **The route gate can only ever reject; it must never be the thing that authorizes.**
## Why the write-side gate must re-read current groups (the escalation it closes)
The listing filters (`_add_user_filters`) only hide out-of-scope resources from the **UI**. A direct API call by
resource ID bypasses them entirely. And a group-reassign request carries only the *new* groups. So without
re-reading current state, this works:
```
Finance doc set #7 currently belongs to {Finance}. Alice manages {Engineering} only.
Alice calls: PUT /document-set {id: 7, groups:[Engineering]}
Naive check (new groups ⊆ managed): [Engineering] ⊆ {Engineering} ✓ → Alice captures Finance's doc set.
Correct check (CURRENT ∪ new ⊆ managed): {Finance, Engineering} ⊄ {Engineering} ✗ → rejected.
```
The gate therefore loads the resource's **current** groups in the same transaction and requires
`current_groups ∪ requested_groups ⊆ managed_groups`. Capture-by-reassignment is closed.
## End-to-end data flow
### Read / list path (which connectors does Alice see in the admin UI?)
1. `current_user` loads Alice with her cached `effective_permissions` (global tokens only).
2. The list endpoint calls `_add_user_filters(stmt, user, get_editable=True)`.
3. Filter logic: `admin` → no filter; else if she holds the token **globally** → all; else if she **manages any
group** (`get_scoped_groups` non-empty) → resources whose group set ⊆ her managed groups; else → empty
(`get_editable`) / public+member (viewing). **Fail-closed:** empty managed set ⇒ empty result, never "no
filter."
### Write path (Alice edits an Engineering connector)
1. Route gate `has_permission(MANAGE_CONNECTORS)` → passes (she manages a group).
2. Handler calls the connector update DB fn.
3. **Inside the DB write**, `assert_within_scope` re-reads the cc_pair's current groups, checks
`current ∪ requested ⊆ get_scoped_groups(Alice)`, checks `access_type != PUBLIC` (PRIVATE or SYNC).
Pass → write; fail →
`OnyxError(INSUFFICIENT_PERMISSIONS)`, transaction rolls back.
### Membership path (Alice adds Bob to Engineering)
1. Route gate `has_permission(MANAGE_USER_GROUPS)` → passes.
2. `add_users_to_user_group(... group_id=Engineering ...)`.
3. Write-side gate: `Engineering ∈ get_scoped_groups(Alice)`? Yes → add. (If Alice targeted Marketing → reject.)
`set_group_permissions` is **untouched** — it stays `FULL_ADMIN_PANEL_ACCESS`-only, so a manager can never
change *what tokens* a group grants (no privilege manufacturing).
## Component interaction
```
┌────────────────────────── auth/permissions.py ──────────────────────────┐
│ SCOPED_MANAGER_PERMISSIONS (code-defined bundle) │
│ get_scoped_groups(user, perm) → live indexed read of is_manager │
│ has_permission(user, perm) → GATE 1 (route) │
│ assert_within_scope(...) → GATE 2 (write-side) │
└───────▲───────────────▲────────────────────────────▲─────────────────────┘
│ │ │
route deps │ 6× _add_user_filters group + resource DB write fns
(require_perm) │ (connector, persona, (add_users_to_user_group,
│ document_set, credentials, update_user_group,
EE/CE │ feedback, token_limit) add_credential_to_connector,
API routers │ re-keyed onto update_document_set,
│ get_scoped_groups create_update_persona, …)
each calls GATE 2 before commit
┌──────────────── db/models.py ────────────────┐ ┌─── alembic/versions ───┐
│ User__UserGroup.is_manager : bool (NEW) │ │ add is_manager + backfill│
└───────────────────────────────────────────────┘ └──────────────────────────┘
┌──────── make_group_manager / revoke_group_manager ────────┐
│ one-row flip · used by migration + Group-Manager UI │
└────────────────────────────────────────────────────────────┘
```
## End-to-end scenario
> Admin makes **Alice** a manager of **Engineering** (`make_group_manager(Alice, Engineering)` → one row flip,
> `is_manager=true`).
- Alice opens the Connectors page → sees only Engineering's connectors (filter re-keyed on managed groups).
- She creates a connector into Engineering, PRIVATE → GATE 2: `{Engineering} ⊆ {Engineering}` ✓, PRIVATE ✓ →
created.
- She tries to set it PUBLIC → GATE 2 rejects (managers can't make anything PUBLIC; PRIVATE or SYNC only).
- She tries `PUT /connector/<Finance id> {groups:[Engineering]}` → GATE 2 re-reads current `{Finance}`,
`{Finance,Engineering} ⊄ {Engineering}` ✗ → 403.
- She adds Bob to Engineering → allowed. She tries to add Bob to Marketing → 403.
- She opens a group's permission editor → cannot change its tokens (`set_group_permissions` stays admin-only).
- She mints a PAT scoped to `manage:connectors` → it works only on Engineering connectors (token caps the
permission set; `is_manager` still bounds the groups, live). She cannot mint a PAT that widens her to
Marketing — the group bound comes from her live `is_manager`, which the token cannot touch.
## The decisions that mattered (resolved at GATE 2)
1. **`is_manager` at the route gate → CACHE THE BOOLEAN (D1).** A dedicated cached `user.is_group_manager`
(recomputed on membership change and on a manager flip; loaded with the user at auth) lets GATE 1 decide
reachability with **zero queries**, per the wiki §8.1 intent. It is a *sibling* field to
`effective_permissions` (which stays global-tokens-only) — not a sentinel inside the token list. Crucially,
only the **boolean** is cached; the managed-group **list** is still resolved live, so the scope set itself
can never go stale.
2. **GATE 2 lives inside each group/resource DB-write function** (re-reading current groups in-txn), exposed as
one shared helper `assert_within_scope`. *Not* a second FastAPI dependency (a dependency can't see
the resource's current groups or run in the write transaction). The route dependency `has_permission`
is only GATE 1.
3. **PAT composition** — a PAT stays a flat permission cap (`request.state.token_scopes`, already implemented). A
manager's *group* scope is **not** encoded in the token — it always comes from live `is_manager`. So a PAT
can only ever **narrow** the manager's permission set and never widen group reach. No new PAT schema; §8.5
"intersect" = (manager bundle ∩ token scopes) for permissions, AND (live managed groups) for scope, both
enforced independently.
4. **Group create → admins only (D2).** Managers manage assigned groups; they cannot create top-level groups.
**Manager assignment → admin or manager-of-that-group (D3)**, enabling in-group delegation.
## What is explicitly out of scope
- Dropping `role` / `UserRole` / `is_curator` and migrating the 3 residual `user.role==ADMIN` readers
(`persona_sharing.py:53`, `build_session.py:638`, `search/api.py:104`) — deferred cleanup release.
- Any change to document-level Vespa ACL (`get_acl_for_user`) — unchanged; managers affect entity-level access
only.
- CE behavior — Group Manager is an EE capability (custom groups are EE); CE has only Basic + Admins.
docs/group-manager-scoped-permissions/03-detailed-design.md
+684
View File
@@ -0,0 +1,684 @@
> Status: active · Task: group-manager-scoped-permissions
# §8 Scoped Permissions (Group Manager) — Detailed Design
Granular spec for implementing §8 on `new-permission-system`. All paths relative to repo root.
---
## 1. Database design
### 1.1 New column: `user__user_group.is_manager`
Reuses the dead `is_curator` slot semantically (we **add** `is_manager` and later drop `is_curator`; we do
**not** rename in-place — the backfill needs both columns to coexist during the transition).
```python
# backend/onyx/db/models.py — class User__UserGroup (currently lines 4356-4368)
is_manager: Mapped[bool] = mapped_column(
Boolean, nullable=False, default=False, server_default=text("false")
)
```
| Attribute | Choice | Rationale |
|---|---|---|
| Name | `is_manager` | Distinct from the tombstone `is_curator`; "manager" matches the §8 vocabulary and the new single-resolver meaning. |
| Type | `Boolean` | A manager binding is binary per (user, group) edge. The *abilities* are code-defined, not stored — so no need for a richer type. |
| `nullable=False` | yes | Every membership row has a definite manager-or-not state; avoids tri-state ambiguity in the `WHERE is_manager` filter. |
| `default=False` / `server_default='false'` | yes | New memberships are non-managers; existing rows backfill to `false` before the targeted UPDATE sets the real managers. Server default lets the `ADD COLUMN` be non-blocking. |
| Placement | on the **edge** `user__user_group`, not on `user` or `permission_grant` | Scope is per-(user, group). `permission_grant` stays **global-only** (`group_id NOT NULL`, no `user_id`). A role binding is not a permission. |
| Index | **none new** — reuse `ix_user__user_group_user_id` (models.py:4359) | The live LIST lookup is `WHERE user_id = ? AND is_manager = true`; the existing `(user_id)` index serves it, and a manager has few memberships so the residual `is_manager` filter is cheap. A partial `(user_id) WHERE is_manager` is an optional later optimization, not needed at launch. |
### 1.1b Cached route-gate flag: `user.is_group_manager` (D1 — cache the boolean)
GATE-2 review chose **cache the boolean** so the route gate needs **zero queries**. Rather than reshape the
`effective_permissions` JSONB (`list[str]` — a sentinel would break `Permission(p)` validation in
`get_effective_permissions`), add a dedicated cached boolean on `User`, recomputed alongside the permission
cache. `effective_permissions` therefore stays **global-tokens-only**; the manager flag is a sibling field.
```python
# backend/onyx/db/models.py — class User
is_group_manager: Mapped[bool] = mapped_column(
Boolean, nullable=False, default=False, server_default=text("false")
)
```
| Attribute | Choice | Rationale |
|---|---|---|
| What it caches | "does this user manage **any** group?" (the boolean only) | The route gate (GATE 1) needs only reachability; it's loaded with the user at auth → no query. |
| What it does **not** cache | the managed-group **list** | The list stays live (`scoped_group_ids_subquery`) so filters + GATE 2 never go stale after a rename/move/delete. |
| Recompute trigger | `recompute_user_permissions__no_commit` (extend) **and** `make/revoke_group_manager` | Flag = `EXISTS(is_manager=true for user)`. Membership changes already recompute; manager flips must recompute the affected user too. |
| Staleness window | bounded to a single user, flipped in the same txn as the membership/manager change | Acceptable — the cost the live-list avoids was the *scope set* going stale, which this doesn't touch. |
### 1.2 Migration — additive only (ship with the feature)
`backend/alembic/versions/c71a18ea7d07_add_is_manager_and_is_group_manager.py`
(revision `c71a18ea7d07`, **down_revision `c8e316473aaa`**).
**Tenant schema** (`alembic/versions/`), NOT `alembic_tenants/`.
```python
def upgrade() -> None:
op.add_column(
"user__user_group",
sa.Column("is_manager", sa.Boolean(), nullable=False, server_default=sa.false()),
)
op.add_column(
"user",
sa.Column("is_group_manager", sa.Boolean(), nullable=False, server_default=sa.false()),
)
# Backfill is_manager — NOT a rename. is_curator alone misses GLOBAL_CURATOR (no per-group rows).
op.execute("""
UPDATE user__user_group ug SET is_manager = true
FROM "user" u
WHERE ug.user_id = u.id AND u.role = 'CURATOR' AND ug.is_curator = true
""")
op.execute("""
UPDATE user__user_group ug SET is_manager = true
FROM "user" u
WHERE ug.user_id = u.id AND u.role = 'GLOBAL_CURATOR'
""")
# Backfill the cached flag from the rows just set.
op.execute("""
UPDATE "user" u SET is_group_manager = true
WHERE EXISTS (
SELECT 1 FROM user__user_group ug
WHERE ug.user_id = u.id AND ug.is_manager = true
)
""")
def downgrade() -> None:
op.drop_column("user", "is_group_manager")
op.drop_column("user__user_group", "is_manager")
```
- **Ordering invariant:** must run **before** any later migration drops `role` / `is_curator`. Captured in
`01-research.md` (disappearing-tombstone trap).
- **Fresh installs / new tenants:** backfill UPDATEs match nothing; `is_manager` starts all-false. ✓
- **Migration report (operational, not a migration step):** a one-off script flags any CURATOR/GLOBAL_CURATOR
whose backfill mapped to **zero** managed groups (snapshot caveat — GLOBAL_CURATOR was dynamic). Out of the
migration transaction; can be a logged query or admin CSV.
### 1.3 Deferred (NOT in this feature)
Dropping `is_curator`, `role`, `UserRole`; migrating the 3 `user.role==UserRole.ADMIN` readers. Separate cleanup
release — keeps a code rollback possible while §8 is unproven.
---
## 2. Code design — new authorization primitives
**One classifier decides authority everywhere — no `allow_scope` fork, no separate `has_permission_or_scope`.**
`has_permission` itself returns the authority. The classifier + bundle live in the pure `permissions.py`; the
DB-querying scope logic (scope resolution, write gates, read clause) lives in
**`backend/onyx/auth/scoped_permissions.py`** (imports `User`, `User__UserGroup`, the bundle — one direction).
### 2.1 The manager ability bundle (in `permissions.py`)
```python
SCOPED_MANAGER_PERMISSIONS: frozenset[Permission] = frozenset({
Permission.MANAGE_CONNECTORS,
Permission.MANAGE_DOCUMENT_SETS,
Permission.MANAGE_AGENTS,
Permission.ADD_AGENTS,
Permission.MANAGE_USER_GROUPS, # membership + resource sharing of the managed group only
Permission.MANAGE_SKILLS, # NEW dedicated token (D5) — also grantable globally in the groups UI
Permission.MANAGE_ACTIONS, # tools/MCP — GATE 1 reach + create only (D4); manage is owner-or-admin (D8)
})
```
Code-defined, never written to `permission_grant`, never merged into `effective_permissions` (which stays
global-only). Lives in `permissions.py` (not `scoped_permissions.py`) so `has_permission` can read it to
classify SCOPED authority without an import cycle.
> **`MANAGE_ACTIONS` is in the bundle (D4).** GATE 1 would 403 a scoped manager on every
> action endpoint otherwise. The route gate lets the manager *reach* the tool/MCP endpoints and create their
> own; managing an existing action or server is owner-or-admin — the agent-mediated GATE 2 once planned here
> was built and then dropped (**D8**). See §11.1.
### 2.2 `has_permission` — the single 3-state classifier
A scoped grant is **group-qualified** ("manage:connectors for groups {3,7}") — it has no flat token
representation, so it can never live in `effective_permissions`, and `has_permission` can never answer a plain
`True` for a manager: 7 existing call sites treat `True` as global "do anything" (see-all filters, set
featured/listed/public, fetch-any — call-site audit), so a bool returning `True` for a manager would leak
org-wide powers. So `has_permission` returns **which** authority the user holds:
```python
class PermissionAuthority(Enum):
GLOBAL # holds the token outright / admin → unrestricted
SCOPED # group manager → only within managed groups
NONE # not authorized
def has_permission(user: User, permission: Permission) -> PermissionAuthority:
if permission in get_effective_permissions(user): # global token / admin override
return PermissionAuthority.GLOBAL
if permission in SCOPED_MANAGER_PERMISSIONS and user.is_group_manager:
return PermissionAuthority.SCOPED # cached flag → zero query
return PermissionAuthority.NONE
def has_global_permission(user: User, permission: Permission) -> bool:
"""GLOBAL-only convenience for the many checks that must exclude scoped managers."""
return has_permission(user, permission) is PermissionAuthority.GLOBAL
```
Existing callers migrate by a behavior-preserving rule — a `SCOPED` manager reproduces the old `False` at
those sites, so nothing leaks and the feature stays inert until each filter opts into `SCOPED` (§3):
```
has_permission(user, X) → has_global_permission(user, X)
not has_permission(user, X) → not has_global_permission(user, X)
```
### 2.3 Live scope resolution (in `scoped_permissions.py`)
```python
def scoped_group_ids_subquery(user: User) -> Select:
"""Composable subquery of the user's managed group ids — embed in _add_user_filters
so the scope predicate stays in SQL (no extra round-trip)."""
return select(User__UserGroup.user_group_id).where(
User__UserGroup.user_id == user.id,
User__UserGroup.is_manager.is_(True),
)
def get_scoped_groups(user: User, db_session: Session,
permission: Permission | None = None) -> set[int]:
"""Imperative form for the write-side gate. Empty if permission given but not scopable."""
if permission is not None and permission not in SCOPED_MANAGER_PERMISSIONS:
return set()
return set(db_session.scalars(scoped_group_ids_subquery(user)).all())
```
### 2.4 GATE 1 — route gate (one classifier, per-endpoint threshold)
`require_permission` keeps the single `has_permission` classifier; `allow_scope` only sets the **acceptance
threshold** — it is NOT a second resolution path, and `has_permission_or_scope` is gone:
```python
# permissions.py — require_permission(required, *, allow_anonymous=False, allow_scope=False)
permitted_by_user = (has_permission(user, required) is GLOBAL) if not allow_scope \
else (has_permission(user, required) is not NONE)
# pass: permitted_by_user AND permitted_by_token (token cap unchanged)
```
- **default (`allow_scope=False`)**: `is GLOBAL` → identical to today; a manager is rejected. The safe default
for every endpoint whose GATE 2 has not landed.
- **`allow_scope=True`**: `is not NONE` → a manager *reaches* the endpoint. Set this **only when the endpoint's
GATE 2 exists** (PR3/PR4). This per-endpoint opt-in **is** the lands-together / no-escalation invariant:
~101 bundle-token endpoints exist today with no GATE 2, and managers already exist (PR1 backfills
`is_manager` for ex-curators), so admitting `SCOPED` universally would let an ex-curator create resources in
arbitrary groups. No DB session at the route (reads the cached flag).
Admin-only endpoints on a **non-bundle** token (`set_group_permissions` → `FULL_ADMIN_PANEL_ACCESS`) classify a
manager as NONE regardless of `allow_scope` → reject automatically.
### 2.5 GATE 2 — per-resource write gates (authorization of record)
Two gates, both built on the one classifier:
```python
def assert_within_scope(
user: User, db_session: Session, *,
permission: Permission, # the manage:* token this write needs
current_group_ids: Collection[int], # re-read from DB, in this txn
requested_group_ids: Collection[int], # client-supplied target groups
is_non_public: bool, # access_type!=PUBLIC (cc_pair) / not is_public (doc set)
) -> None:
authority = has_permission(user, permission)
if authority is PermissionAuthority.GLOBAL:
return # base-system rules govern
if authority is PermissionAuthority.SCOPED:
managed = get_scoped_groups(user, db_session, permission)
final = set(current_group_ids) | set(requested_group_ids)
if managed and final and final.issubset(managed) and is_non_public:
return
raise OnyxError(
OnyxErrorCode.INSUFFICIENT_PERMISSIONS,
"Group managers can only act on private resources within the groups they manage.",
) # NONE, or out-of-scope
def assert_global(user: User, *, permission: Permission) -> None: # delete / admin-only on a bundle token
if has_permission(user, permission) is not PermissionAuthority.GLOBAL:
raise OnyxError(OnyxErrorCode.INSUFFICIENT_PERMISSIONS, "Admin only.") # SCOPED manager rejected
```
`assert_within_scope` invariants: `final ⊆ managed` (closes capture-by-reassign), `final` non-empty (stays in
≥1 group — covers detach), `is_non_public` (PRIVATE or SYNC), **fail-closed** (empty `managed` ⇒ reject).
`assert_global` (D6, **rule A**) keeps delete/admin-only ops admin-only even though they share a bundle token
with scoped create/update: the route admits the manager (SCOPED ≠ NONE), the handler's `assert_global` rejects
them.
> **GATE 2 attaches to the group-share / access-change step, not to owner self-edits.** An owner editing their
> own private persona holds `NONE` for `MANAGE_AGENTS` and must still pass — `assert_within_scope` guards the
> `update_persona_access` (group_ids / is_public) write specifically, not every mutation (§11.5).
### 2.6 Manager assignment helpers (EE)
```python
# backend/ee/onyx/db/user_group.py
def make_group_manager(db_session: Session, user_id: UUID, group_id: int) -> None:
"""Flip is_manager=true on the (user, group) row. Row must exist (a manager is a member).
Idempotent. Used by the migration backfill helper and the assignment UI."""
def revoke_group_manager(db_session: Session, user_id: UUID, group_id: int) -> None:
"""Flip is_manager=false. Idempotent."""
```
### 2.7 Cached-flag recompute (D1 plumbing)
`backend/onyx/db/permissions.py:43` `recompute_user_permissions__no_commit` — **extend** to also set
`user.is_group_manager = EXISTS(is_manager=true for that user)` in the same write. This already fires on
membership add/remove (`update_user_group:570`). Additionally, `make_group_manager` / `revoke_group_manager`
must call `recompute_user_permissions__no_commit([user_id], db_session)` for the affected user so a pure
manager flip (no membership change) refreshes the cached flag. `effective_permissions` content is unchanged (global tokens only).
---
## 3. Filter rewrites — the `_add_user_filters` set
> **Reconciled with §11.4/§11.7** (regression review): the real set is **4 re-keyed filters** (connector,
> document_set, persona, **skill**) + the `token_limit` write-path. Credentials AND **feedback** are
> unchanged (no feedback permission in the bundle). The table below now carries all four, with the two
> no-ops marked — read it row-by-row. Skill is the odd one out: a new admin-list path, not a filter edit.
**The scope clause is OR-ed *into* each existing editable predicate — it does NOT replace the filter.** Each
`_add_user_filters` keeps its full structure; only two things change: (1) the global short-circuit flips to
`has_permission(user, MANAGE_X) is GLOBAL`; (2) inside the **`get_editable=True`** branch, when
`has_permission(user, MANAGE_X) is SCOPED`, OR in `within_managed_scope_clause(...)`.
```python
def _add_user_filters(stmt, user, get_editable=True):
if has_permission(user, MANAGE_X) is GLOBAL:
return stmt # global → all
if not get_editable and has_permission(user, READ_X) is GLOBAL:
return stmt # global read → all (persona/doc-set)
... existing joins ...
if get_editable:
where_clause = <existing owner/share/member/creator predicate> # PRESERVED
if has_permission(user, MANAGE_X) is SCOPED: # NEW, additive
where_clause |= within_managed_scope_clause(...)
else:
where_clause = <existing read predicate> # PRESERVED — manager reads as a member
return stmt.where(where_clause)
```
Why additive, not replacement — the regressions this prevents:
- **`get_editable` must be respected.** The read path keeps its own visibility (public/SYNC for cc_pair,
public+member for doc-set, owner/share/READ for persona); a manager reads there as a normal member. Only the
editable branch gains the scope term.
- **A `NONE` user keeps their access.** persona owners, cc_pair creators, EDITOR-shares hold no manage token;
their existing predicate stays. A blanket `NONE → sa_false()` would strip every normal editor.
- **A `SCOPED` manager is also a normal user.** They may own a persona / have created a cc_pair *outside* their
managed groups; OR-ing (not replacing) preserves that on top of the scoped-editable set.
The editable read-side predicate mirrors GATE 2: editable-by-manager iff every group is managed, in ≥1 group,
and private — exactly `within_managed_scope_clause` (new helper in `scoped_permissions.py`):
```python
def within_managed_scope_clause(
resource_id_col, junction_resource_col, junction_group_col,
non_public_clause: ColumnElement[bool], # a predicate, NOT a bool column — cc_pair passes
managed_subq: Select, # access_type != AccessType.PUBLIC; doc-set/persona is_public.is_(False)
) -> ColumnElement[bool]:
# NOT EXISTS(group not in managed) AND EXISTS(group in managed) AND non_public_clause
```
| File | Existing editable fallback | Integration |
|---|---|---|
| `backend/onyx/db/document_set.py:41` | `sa_false()` (no owner/share concept) | `|=` scope clause over `DocumentSet__UserGroup` (`is_public.is_(False)`). The one resource where a standalone scope filter *happens* to match (`false() OR clause == clause`); still expressed as `|=` for uniformity. |
| `backend/onyx/db/connector_credential_pair.py:50` | member-of-owning-group (all groups ⊆ mine) ∨ creator | `|=` scope clause over `UserGroup__ConnectorCredentialPair` (`access_type == PRIVATE`). |
| `backend/onyx/db/persona.py:77` | owner ∨ owning-group member ∨ EDITOR direct/group share ∨ org-edit | `|=` scope clause over `Persona__UserGroup` (`is_public.is_(False)`); `add:agents` ownership tier unchanged. |
| `backend/onyx/db/skill.py:260` (`list_skills_for_admin`) | none — unfiltered `select(Skill)` | **NEW scoped admin-list path**, not an `_add_user_filters` edit: scope clause over `Skill__UserGroup` (`is_public.is_(False)`), fail-closed, taken only when the caller is `SCOPED`. **Do NOT touch `_add_user_visibility_filter` (`skill.py:85`)** — it has no editable/viewing split and feeds the agent RUNTIME injection path. (§11.2) |
| `backend/onyx/db/feedback.py:46` | admin-only (`FULL_ADMIN_PANEL_ACCESS`) | **NO CHANGE** — not in the bundle. (§11.7) |
| `backend/onyx/db/credentials.py:41` | owner-keyed (`Credential.user_id==user.id`) | **NO CHANGE** — credentials stay owner-scoped; document the deliberate no-op. |
| `backend/ee/onyx/db/token_limit.py` | no `_add_user_filters`; direct group query | enforce managed-scope in the group-token-limit **write/endpoint** path. Minor. |
**Fail-closed:** `scoped_group_ids_subquery` yields no rows for a non-manager, so `within_managed_scope_clause`
contributes nothing — never an unfiltered statement.
---
## 4. Write-path gate insertions (where GATE 2 is called)
> **Superseded by §11.4** (regression review): the table below under-enumerates the manager-reachable
> writes. §11.4 is the complete list (cc_pair status/name/property/prune, persona `/share`, group rename,
> `/agents` attach, skills) with the **delete = admin-only (D6)** rule and the persona-gate fix (§11.5)
> and cc_pair-reattach fix (§11.6). Use §11.4 as the source of truth.
Each DB-write fn loads the resource's **current** groups in-txn, then calls
`assert_within_scope(...)` before mutating (delete/admin-only ops call `assert_global(...)` instead — rule A).
Endpoints switch to `allow_scope=True`.
| Resource / action | DB fn (insert gate here) | Endpoint → new dep |
|---|---|---|
| Connector create | `add_credential_to_connector` (`connector_credential_pair.py:496`, groups via `_relate_groups_to_cc_pair__no_commit:480`) | `connector.py:1603` POST → `require_permission(MANAGE_CONNECTORS, allow_scope=True)` |
| Connector update (groups/access) | cc_pair update path (`server/documents/cc_pair.py` group/access setter) | same dep |
| Document set create | `insert_document_set` (`document_set.py:220`) | `document_set/api.py:33` POST → `MANAGE_DOCUMENT_SETS, allow_scope=True` |
| Document set update | `update_document_set` (`document_set.py:296`) | `document_set/api.py:59` PATCH → same |
| Persona create/update | `create_update_persona` (`persona.py:325`) → `update_persona_access` (`ee/persona.py:68`, `group_ids`+is_public) | persona create/update endpoint → `MANAGE_AGENTS, allow_scope=True` |
| Group create | `insert_user_group` (`ee/user_group.py:413`) | `user_group/api.py:144` POST → **UNCHANGED** `MANAGE_USER_GROUPS` (no `allow_scope`). **D2: admins only create top-level groups** — no self-grant path. |
| Group update / members | `update_user_group` (`ee/user_group.py:504`), `add_users_to_user_group` (`:462`) | `user_group/api.py:194` PATCH, `:215` add-users → `MANAGE_USER_GROUPS, allow_scope=True`; gate: `group_id ∈ managed` |
| Group **permissions** | `set_group_permission(s)__no_commit` (`ee/user_group.py:705/748`) | `user_group/api.py:115` PUT → **UNCHANGED** `FULL_ADMIN_PANEL_ACCESS` (managers cannot grant tokens) |
For group membership/update, GATE 2 degenerates to "is the *target group* in `managed`?" (the resource *is* the
group). For resource writes it is the full `current ∪ requested ⊆ managed` + private check.
**Bulk/list endpoints:** any endpoint accepting multiple resource ids must run GATE 2 **per item**, not on the
first/aggregate (e.g. batch cc_pair group edits).
> **D2 (decided): managers cannot create top-level groups.** Only admins create groups; managers manage the
> groups assigned to them. Group *create* keeps the plain global `MANAGE_USER_GROUPS` dependency (no
> `allow_scope`); `allow_scope=True` applies only to group *update / members*. No manager-creates-manager
> self-grant path to reason about.
---
## 5. PAT composition (§8.5) — minimal change
PAT scopes already cap permissions (`request.state.token_scopes` → `require_permission`, `permissions.py:278`;
model `db/pat.py`). A manager's **group** scope is never encoded in the token — it always comes from live
`is_manager`. Therefore:
- Permissions: effective = (manager bundle ∩ token_scopes) — the existing token cap already does the
intersection; no change needed.
- Groups: GATE 2 runs regardless of PAT and bounds to live managed groups — a token cannot widen it.
**No PAT schema change.** Add only tests proving a scoped PAT cannot widen group reach.
---
## 6. API surface changes
- **`GET /users/me/permissions`** (`permissions.py` API) — add `is_manager: bool` (true if the user manages any
group) so the frontend can reveal manager-relevant nav. Optionally `managed_group_ids: list[int]`. Keeps the
endpoint the single "what can I do?" source.
- **New EE endpoint** — `PUT /manage/admin/user-group/{group_id}/manager` `{user_id, is_manager}` →
`make_group_manager`/`revoke_group_manager`. Dep: `require_permission(MANAGE_USER_GROUPS, allow_scope=True)`.
**D3 (decided): admin or manager-of-that-group may assign** — GATE 2 on this endpoint = admin **or**
`group_id ∈ get_scoped_groups(actor)`. A manager can thus delegate management within their own group;
assignment of a manager outside the actor's managed groups is rejected. The target `user_id` must already be a
member of `group_id` (a manager is always a member) — else 400.
---
## 7. Frontend
- `web/src/lib/.../usePermissions` (and `hasPermission`) — consume the new `is_manager` flag; treat a manager as
holding the scoped `manage:*` tokens **for nav/visibility only** (real enforcement is backend GATE 2).
- Sidebar (`Connectors`/`Document Sets`/`Groups`) shows for managers.
- **Group detail page** — per-member "Make Manager" / "Revoke Manager" toggle (mirrors the old "Make Curator"
affordance), calling the new endpoint. Files under `web/src/app/ee/admin/groups/[groupId]/`.
- The resource list pages (connectors / doc-sets / agents / skills) call backend list endpoints whose
filters now return the manager-scoped set — no client-side scoping logic needed (and must not be relied on
for security). **Exception: the GROUP list is NOT auto-scoped** — `fetch_user_groups` returns all groups
and needs a manager-scoped variant (§11.9) before the Groups page / assign-toggle UI work for managers.
---
## 8. New files & file tree
```
backend/
onyx/
auth/
scoped_permissions.py ← NEW: scoped_group_ids_subquery, get_scoped_groups,
assert_within_scope, assert_global, within_managed_scope_clause
db/enums.py ← MOD: PermissionAuthority enum (GLOBAL/SCOPED/NONE)
auth/permissions.py ← MOD: SCOPED_MANAGER_PERMISSIONS; has_permission → PermissionAuthority;
require_permission(..., allow_scope=False) threshold
db/models.py ← MOD: User__UserGroup.is_manager + User.is_group_manager
db/permissions.py ← MOD: recompute_user_permissions__no_commit sets is_group_manager (§11.7)
db/document_set.py ← MOD: _add_user_filters editable manager branch (was sa_false)
db/connector_credential_pair.py ← MOD: filter + add_credential_to_connector gate
db/persona.py ← MOD: filter + update_persona_access gate (MIT twin, §11.5)
db/skill.py ← MOD: scoped admin-list path + replace_skill_grants GATE 2 (§11.2)
db/feedback.py ← (no change — admin-only, not in bundle; §11.7)
db/credentials.py ← (no change — documented no-op)
server/features/skill/api.py ← MOD: re-point off curator dep; allow_scope by verb (§11.2)
server/features/tool/api.py ← MOD: allow_scope=True (reach+create); manage stays owner-or-admin — D8 dropped agent-mediated GATE 2 (§11.1)
server/features/mcp/api.py ← MOD: allow_scope=True (reach+create); manage stays owner-or-admin — D8 dropped agent-mediated GATE 2 (§11.1)
db/tools.py ← MOD: owner-or-admin manage gates (D8); agent-mediated scope kept only for view (§11.1)
server/.../{document_set,connector,cc_pair,persona}/api.py ← MOD: allow_scope=True deps
server/.../permissions api ← MOD: /users/me/permissions adds is_manager
ee/onyx/
db/user_group.py ← MOD: make/revoke_group_manager + gates in update/add_users
db/persona.py ← MOD: update_persona_access gate
db/token_limit.py ← MOD: managed-scope enforcement on group token-limit writes
server/user_group/api.py ← MOD: allow_scope deps + NEW manager-assign endpoint
alembic/versions/
c71a18ea7d07_add_is_manager_and_is_group_manager.py ← NEW migration (down_revision c8e316473aaa)
web/
src/app/ee/admin/groups/[groupId]/ ← MOD: manager toggle UI
src/lib/.../usePermissions(.ts) ← MOD: is_manager flag
src/.../hasPermission(.ts) ← MOD: manager nav visibility
```
---
## 9. Pre-implementation notes (must honor)
1. **GATE 2 is the authorization of record.** Route gate (`allow_scope`) only widens *reachability*; never let
it authorize. Every scoped write path must call `assert_within_scope` (or `assert_global` for admin-only ops).
2. **Re-read current groups in-txn.** Never trust the client's group list alone (capture-by-reassign).
3. **Non-PUBLIC only.** Reject any manager create/edit that sets/keeps PUBLIC; PRIVATE and SYNC are in scope
(a manager manages their group's private *and* auto-sync connectors). Doc sets have no SYNC → PRIVATE only.
4. **Fail closed.** Empty managed set ⇒ no access; guard every filter against an unfiltered fallback.
5. **`set_group_permissions` stays admin-only.** Managers manage membership + resource sharing, never the
group's token grants.
6. **Bulk endpoints check every item.**
7. **Keep the bundle out of `effective_permissions.global`.** Never persist scoped tokens; resolve live.
8. **Migration before drops.** Backfill `is_manager` before any later release drops `role`/`is_curator`.
9. **Credentials unchanged.** Owner-scoped; deliberately no manager inheritance.
10. **Tracing / OnyxError / typing conventions** per CLAUDE.md (raise `OnyxError`, strict typing, no
`response_model`).
11. **Extend the existing permission tests; don't reflexively add new test files.** The new permission
system already has good coverage — verify the relevant suite is sound and add to it when the behavior is
one it already drives. New files only for genuinely new homes (e.g. migration backfill). See
[04 Tests](04-implementation-plan.md#tests) for the known homes + the PR1 worked example.
## 10. Decisions resolved at GATE 2
- **D1 → cache the boolean.** New cached `user.is_group_manager` (sibling to `effective_permissions`, not inside
it), recomputed on membership change and on manager flip; route gate reads it with zero queries. The managed
**list** stays live. (§1.1b, §2.3, §2.6.)
- **D2 → admins only create groups.** Group create keeps global `MANAGE_USER_GROUPS`; `allow_scope` only on
group update/members. (§4 table, note.)
- **D3 → admin or manager-of-that-group assigns managers.** Manager-assign endpoint gates on
`admin ∨ group_id ∈ managed`. (§6.)
---
## 11. Regression-review resolutions — full case coverage (verified 2026-06-29)
A 5-dimension adversarial review against `new-permission-system` (PAT · admin-retrieval · chat ·
junction-only · completeness, 18 agents) confirmed the core design is sound — PAT cap preserved, chat
runtime untouched, purely junction-based, and the feared backfill data-loss does **not** occur — but found
that §1–10 under-specify several manager-reachable paths. **This section is the authoritative coverage
checklist; implement every row.** New decisions locked with the owner: **D4 actions = `manage:actions` in the
bundle** (its agent-mediated GATE 2 later dropped — **D8**) · **D5 skills = in scope (7th resource)** ·
**D6 managers may do everything EXCEPT delete** (narrowed by **D9**: a creator may delete what they made).
### 11.0 PREREQUISITE — independent boot bug (fix before/with PR1)
`current_curator_or_admin_user` was removed from `onyx/auth/users.py` by §1–7 but is still imported by
`server/features/skill/api.py:16` and `server/documents/targeted_reindex.py:22` → `import onyx.main` raises
`ImportError` and **the API server cannot boot on this branch** (verified at runtime). Re-point both off the
dead dep: skills → see §11.2; `targeted_reindex.py:80/163` → `require_permission(MANAGE_CONNECTORS)` (matches
its connector/indexing peers). This is a merge-integration break, not a §8 feature change, but it blocks
everything. Add an `import onyx.main` smoke test to CI so a deleted auth dep fails fast.
### 11.1 Actions (D4 — `MANAGE_ACTIONS` in the bundle; manage is owner-or-admin per D8)
> **⚠️ SUPERSEDED by D8 (2026-08-03) for the _management_ verbs.** The agent-mediated GATE 2 below
> (edit/toggle/OAuth-auth resolving an action's groups through its referencing agents) was **built, then
> dropped** — it caused most of the review's P1/P2 findings. Manage is now plain **owner-or-admin**
> (`can_manage_own_tool` / `_ensure_mcp_server_owner_or_admin`), mirrored 1:1 by the UI projection; the
> *view* path keeps agent-mediated scope. GATE 1 reach + create (allow_scope=True) is unchanged; **delete
> now follows the same owner-or-admin gate (D9), not admin-only.** Read the rest of this section as
> historical design, not the current gate.
- **`MANAGE_ACTIONS` stays in `SCOPED_MANAGER_PERMISSIONS`** (§2.1). Bundle membership is what GATE 1
(`has_permission` → SCOPED, admitted when `allow_scope=True`) checks — drop it and a scoped manager 403s at
the route on every action endpoint. "Agent-mediated" is GATE 2's scope resolution, not a reason to omit it.
- **Reaching the endpoints (GATE 1):** switch the standalone tool/MCP admin endpoints (`tool/api.py`
POST/PUT/DELETE, `mcp/api.py`) to `require_permission(MANAGE_ACTIONS, allow_scope=True)` so managers can reach
them, including DELETE — a creator may delete what they made (D9).
- **Scope check (GATE 2):** actions have no direct group; derive an action's groups from the agents that
reference it — `Tool → Persona__Tool → Persona__UserGroup` — and require them ⊆ managed (+ PRIVATE). This
**replaces** the current owner-or-admin per-resource check (`_get_editable_custom_tool` /
`_ensure_mcp_server_owner_or_admin`); keep owner/admin as a bypass.
- **Edge cases:** a tool used by agents spanning a group the manager doesn't manage → reject; a tool referenced
by **no** agent → no group context → owner/admin only.
- Attaching existing catalog tools to an agent still flows through the persona path (`tool_ids` on
`create_update_persona`, gated by `MANAGE_AGENTS` + the persona GATE 2) — unchanged.
### 11.2 Skills (D5 — in scope, 7th scoped resource) — REVISED per the GO/NO-GO review
Skills (`Skill__UserGroup` `models.py:4460`; `db/skill.py`) are group-shareable but **do NOT mirror personas
structurally** — the original "re-key the editable filter like personas" instruction was wrong. Five
corrections (verified 2026-06-29):
- **Dedicated `MANAGE_SKILLS` permission (owner decision — replaces the "reuse MANAGE_AGENTS" idea).** Add
`Permission.MANAGE_SKILLS = "manage:skills"` to the enum + the permission **registry** (so it shows in the
**groups permission UI** and is grantable to a group globally, like the other manage tokens) and to
`SCOPED_MANAGER_PERMISSIONS` (so managers get it scoped). **No DB migration needed** —
`permission_grant.permission` is `Enum(native_enum=False)` (stored as a string), so a new enum value is a
pure code change. Admins get it automatically via the `FULL_ADMIN_PANEL_ACCESS` override. Don't put it in
`NON_TOGGLEABLE_PERMISSIONS`. This removes the over/under-grant of reusing `MANAGE_AGENTS`.
- **Do NOT touch `_add_user_visibility_filter` (`skill.py:85`).** It has no editable/viewing split AND it feeds
the agent RUNTIME — `list_skills_for_sandbox_injection` (`skill.py:250`) → `skills/push.py` → build-session
sandbox hydration (`session/manager.py:386`, `session/api.py:438`). OR-ing a manager branch into it would
widen which skills get injected into a manager's agent context at runtime — the exact leak the design
deliberately closes (admins get NO bypass here on purpose, `skill.py:91-94`). **The manager predicate must
NEVER enter this filter or any function reaching `push.py`/`session/*`.**
- **Read side = a NEW scoped ADMIN-LIST path.** `list_skills_for_admin` (`skill.py:260`) is `select(Skill)`
with no filter — a re-pointed manager would see every skill in the tenant. Add a manager-scoped variant
(`within_managed_scope_clause` over `Skill__UserGroup`, PRIVATE-only, fail-closed) used only when the caller
is a non-admin scoped manager; leave the runtime/visibility filter byte-identical.
- **Write side = GATE 2 on the GRANTS seam, not create/update.** `create_skill__no_commit` takes no group_ids
and `patch_skill` only toggles is_public/enabled; the only group↔skill writer is `replace_skill_grants`
(`skill.py:430`, reached via PUT `/admin/skills/custom/{id}/grants`, body `GrantsReplace.group_ids`). Put
GATE 2 there (current grants via `get_group_ids_for_skill` ∪ requested ⊆ managed, `is_non_public = not
skill.is_public`, re-read in-txn). Also gate the `is_public` toggle in `patch_custom_skill` so a manager
can't publish a private skill out of scope.
- **Endpoint re-point BY VERB.** list/get/create/update/share → `require_permission(MANAGE_SKILLS,
allow_scope=True)`; **skill DELETE stays admin-only (D6).** NOTE: the admin skill router this assumed is
gone — `31d5a492e4` unified the skill API onto `user_router` + `SkillManagementPolicy`, so the only delete
is `delete_current_user_skill` (`skill/api.py:937`, `BASIC_ACCESS` + `SkillManagementPolicy.EDIT`). There is
no admin route left to exclude from the re-point, so D6 has to be enforced inside that handler/policy
instead. `Skill.is_public` + `Skill__UserGroup` exist, so GATE 2 is expressible. (PR0 parks the
re-point on `FULL_ADMIN_PANEL_ACCESS` to unbreak boot; PR4 adds `MANAGE_SKILLS` + `allow_scope` + the GATE 2
and admin-list together, then narrows the deps.)
### 11.3 Manager power = everything EXCEPT deleting a managed-group resource (D6, narrowed by D9)
Managers may create / edit / attach / detach / pause / rename / share within managed groups (non-PUBLIC only —
PRIVATE or SYNC; GATE 2). **Delete is admin-only for a resource that merely sits in a managed group.** These
stay on the plain global dep (no `allow_scope`): connector/cc_pair delete (`administrative.py:141`),
document-set delete (`document_set/api.py:93`), skill delete (`skill/api.py:937` — see §11.2: no admin route
remains, so this one is enforced in the handler, not by a global dep); plus group create
(D2) + group delete + `set_group_permissions` (admin-only).
**D9 carve-out:** deleting something the manager *created* is ownership, not scope, so it follows the same
owner-or-admin gate as every other owner: custom actions / MCP servers (`can_manage_own_tool`,
`_ensure_mcp_server_owner_or_admin`), personas (`can_delete_persona`), personal skills
(`_ensure_owned_personal`). Being a manager never subtracts a right an ordinary user holds.
### 11.4 Complete write-path enumeration (supersedes/extends the §4 table — gate EVERY row)
All `allow_scope=True` + GATE 2 EXCEPT where marked admin-only (D6/D2):
| Endpoint(s) | DB fn (GATE 2 site) | Treatment |
|---|---|---|
| cc_pair status `cc_pair.py:427` · name `:512` · property `:542` · prune `:604` | re-keyed editable read-filter authorizes (no group/access change) | allow_scope=True |
| associate-credential `cc_pair.py:716` · connector create mock-cred `connector.py:1568` · bare create `:1538` | `add_credential_to_connector` (`:496`) | allow_scope=True on **all**; the `connector.py:1603` anchor was imprecise (it's the mock-cred path) |
| ee `sync_cc_pair_groups` (sync-groups) · `sync_cc_pair` (sync-permissions) | re-keyed editable read-filter authorizes — NOTE: these only *enqueue* an external sync task (they do NOT rewrite the group junction; that's PR5's `update_user_group`) | allow_scope=True; load `get_editable=True` (they target SYNC connectors, which a manager manages) |
| connector/cc_pair **DELETE** `administrative.py:141` | — | **admin-only (D6)** |
| doc-set create `document_set/api.py:36` · patch `:62` | `insert/update_document_set` (`:220/:296`) | allow_scope=True |
| doc-set **DELETE** `:93` | — | **admin-only (D6)** |
| persona create `persona/api.py:310` · update `:181` · **share `:443`** | `update_persona_access` (§11.5) | allow_scope=True; **GATE 2 keyed on `MANAGE_AGENTS` (D7)** — admin/global bypass; scoped managers ⊆ managed; ADD_AGENTS-only can't group-share |
| skill create/update `skill/api.py:186/...` | skill write fn (§11.2) | allow_scope=True; GATE 2 on `replace_skill_grants` |
| tool/MCP create/update/**delete** `tool/api.py` · `mcp/api.py` | owner-or-admin per resource (`can_manage_own_tool` / `_ensure_mcp_server_owner_or_admin`) — the agent-mediated GATE 2 was dropped (**D8**) | allow_scope=True on every verb; **delete included (D9)** |
| group update/members `user_group/api.py:194/215` · **rename `:164`** | `update_user_group` / `add_users_to_user_group` | allow_scope=True; GATE 2 = group ∈ managed; **cc_pair_ids per §11.6** |
| group `/agents` persona attach `user_group/api.py:256` | `update_persona_access` | allow_scope=True; **manager-scope GATE 2 = target group ∈ managed** (the roster surface) |
| group create `:144` · delete · permissions `:115` | — | **admin-only (D2)** |
### 11.5 Persona group-share = MANAGE_AGENTS-controlled (D7, owner decision 2026-06-29) + gate plumbing
**D7 — who may attach an agent to a group:** self member-share (sharing your own agent to a group) is
controlled by **`MANAGE_AGENTS`** — not `ADD_AGENTS`, not bare membership. So the group-share write is the
**standard GATE 2 keyed on `MANAGE_AGENTS`**, no special carve-out (the earlier membership framing is dropped):
- `has_permission(user, MANAGE_AGENTS)` (admin or a **global** `MANAGE_AGENTS` holder) → bypass; keeps today's
self-share to their groups.
- else a **scoped manager** → allow only if target groups ⊆ `get_scoped_groups(user, MANAGE_AGENTS)` (managed),
private. Managers hold `MANAGE_AGENTS` via the bundle.
- else (`ADD_AGENTS`-only) → reject the group-share. Such a user can still create/edit a private, **no-group**
personal agent — they just can't attach it to a group.
**The gate authorizes the *diff*, not the state.** Unchanged group shares are not a group-share mutation,
so they're exempt for a non-SCOPED actor (the editor round-trips the current groups on every save — without
this a plain owner couldn't edit an agent someone else group-shared) and for an **owner**, who sets their
own agent's privacy per **D9**. "Unchanged" compares the whole `{group_id: permission}` map — re-leveling
a group from VIEWER to EDITOR is a change and gets no exemption. A scoped manager who isn't the owner is
still checked even when nothing changed: holding the share while flipping a public agent private is a
capture. So is the reverse — sharing a public agent *into* a managed group — which no ownership exempts.
> **Current-code nuance to enforce (PR4):** today the persona create/`/share` route gates on `ADD_AGENTS`
> (`persona/api.py:310/340/447`) and the share write authorizes via the **editable fetch** (owner/EDITOR/admin,
> `update_persona_shared:434`), **not** `MANAGE_AGENTS` — so group-share isn't `MANAGE_AGENTS`-gated yet. PR4
> adds the `MANAGE_AGENTS` requirement on the group-share write via GATE 2 (`permission=MANAGE_AGENTS`). The
> route stays `ADD_AGENTS` (so users can still make personal agents) + `allow_scope=True` so scoped managers
> reach it. This is a small, intended tightening of who can put an agent into a group.
Plumbing (review-F2/F4/BR-3): thread the acting `user: User` into `update_persona_access` from all callers
(`create_update_persona` `persona.py:384`, `update_persona_shared` `persona.py:472`, `update_group_agents`
`user_group/api.py:269/281`); apply to BOTH `onyx/db/persona.py:273` (MIT) and `ee/onyx/db/persona.py:68` (EE);
re-read current groups + `is_public` in-txn (don't trust caller flags). `is_public` stays owner/admin-gated
(unchanged — `update_persona_shared`'s `is_owner_or_admin`). The group `/agents` roster surface uses the same
manager-scope GATE 2 (target group ∈ managed) — §11.4.
### 11.6 Group-update cc_pair re-attach is an escalation vector (closes review-F4)
`update_user_group` (`ee/user_group.py:551-562`) rewrites the group↔cc_pair junction from a client
`cc_pair_ids` list, bypassing the gated `add_credential_to_connector`. "group ∈ managed" does **not**
validate those cc_pairs → a manager could attach a public/out-of-scope connector to their group.
**Resolution:** when a scoped manager edits a managed group, run GATE 2 **per added cc_pair** (each within
managed scope + PRIVATE); reject otherwise. Admins unaffected (global bypass).
### 11.7 Filter & recompute corrections
- **Feedback `db/feedback.py:46` → NO CHANGE.** No feedback permission exists in the bundle and its editable
gate is `FULL_ADMIN_PANEL_ACCESS` (admin-only). Leave it like `credentials.py`; the §3 "mirror connector"
label was wrong. Real re-keyed filters are **4** (connector, document_set, persona, **skill**) + the
`token_limit` write-path; credentials + feedback unchanged. ("6 filters" was a miscount.)
- **`recompute_user_permissions__no_commit` (`db/permissions.py:43`) signature is `(user_ids, db_session)`** —
the §2.6 one-arg call is wrong. Extend the fn to also set `user.is_group_manager = EXISTS(is_manager)` in
the same txn; `make/revoke_group_manager` call it as `([user_id], db_session)`.
### 11.8 Confirmed SAFE — no action (the three core worries)
- **PAT:** token cap (`permissions.py:278`) preserved verbatim; `allow_scope` only swaps the user-side
conjunct and only on opt-in routes; GATE 2 never reads the token → a scoped PAT can only narrow, never
widen group reach. Default `allow_scope=False` path is byte-identical to today. Implementation invariants:
keep `dependency._is_require_permission = True` unconditional (`permissions.py:288`); keep the default
path calling `has_permission` directly.
- **Chat:** answer hot path reads `chat_session.persona` (ORM, `process_message.py:635`); all runtime reads
use `get_editable=False`; GATE 2 never on the send path; `get_acl_for_user` untouched. Invariant: keep all
filter edits inside the `if get_editable:` block as correlated subqueries — do not perturb the shared joins
above the split.
- **Junction-only:** scope is purely `User__UserGroup.is_manager` + resource↔group junctions;
`permission_grant` stays global-only; Vespa ACL untouched. Backfill is safe — no upgrade migration nulls
`role`, and `native_enum=False` stores `'CURATOR'`/`'GLOBAL_CURATOR'` literally. Ship the
zero-managed-group migration report for the GLOBAL_CURATOR snapshot caveat.
### 11.9 Group LIST must be scoped (closes the group-list gap)
`fetch_user_groups` (`ee/user_group.py:188`) returns ALL groups with no user/`is_manager` filter, and
`list_user_groups` (`user_group/api.py:46`) is the list the Groups admin page AND the §6 manager-assign
toggle UI depend on. So the §7 claim "list pages already return the manager-scoped set" is **false for
groups** — `MANAGE_USER_GROUPS` is in the bundle but the group list is not one of the re-keyed filters.
Resolution: add a manager-scoped group-list variant filtered by `User__UserGroup.is_manager` (NOT membership),
switch `list_user_groups` (and the single-group/member-list reads the toggle UI needs) to `allow_scope=True`
returning only managed groups for a scoped manager; admins keep the full list. Without this a scoped manager
either 403s on the Groups page (feature unusable) or sees every org group (leak). Correct the §7 wording.
### 11.10 Doc-consistency reconciliations (mechanical)
- **§8 file tree + PR4 file table:** flip `db/feedback.py` to no-op; ADD `db/skill.py` (filter + grants GATE 2)
and `server/features/skill/api.py` (re-point); ADD `User.is_group_manager` to the models.py line and
`db/permissions.py` (recompute) — these are the artifacts implementers slice from.
- **§11.0 tokens:** `targeted_reindex.py:80/163` → `require_permission(MANAGE_CONNECTORS)` (matches connector
peers; independent of the skills token). Skills token per §11.2.
- **D6 delete invariant:** a managed-group resource stays undeletable **only** because its route gate keeps
`allow_scope=False` — the re-keyed editable filter would otherwise authorize a manager. Never add
`allow_scope=True` to a DELETE route whose only authorization is that filter (connector/cc_pair, doc set,
admin skill); add an escalation test asserting each 403s a scoped-only manager. Routes with a per-resource
ownership gate are the D9 exception: action/MCP/persona delete run `allow_scope=True` and authorize on
owner-or-admin instead, so the filter never decides.
- Stale prose to fix: "6 filters" → 4 re-keyed + skill (`00:14`, `01:52/64`, `02` ASCII); `§2.6` call →
`([user_id], db_session)`; `§3` feedback row → NO CHANGE.
docs/group-manager-scoped-permissions/04-implementation-plan.md
+211
View File
@@ -0,0 +1,211 @@
> Status: active · Task: group-manager-scoped-permissions
# §8 Scoped Permissions (Group Manager) — Implementation Plan
> **Primitives per [03 §2](03-detailed-design.md) (single-classifier model).** `has_permission` returns
> `PermissionAuthority` (one classifier; the separate `has_permission_or_scope` removed) and lives in `permissions.py` with
> the bundle; `has_global_permission` is the GLOBAL-only bool helper. GATE 1 = `require_permission(...,
> allow_scope=True)` (threshold); GATE 2 = `assert_within_scope` / `assert_global`. Names below updated to match.
## Issues to Address
The base group-permission system (§1–7) grants tokens to a whole group — every member gets them everywhere.
There is **no way to delegate admin-like control over a single group** without granting it globally. §8 adds the
**Group Manager**: a user with `manage:*` powers confined to the group(s) they manage, and nothing else. The
change must deliver that delegation **without re-introducing the old dual-path fragility** and **without opening
privilege-escalation paths** (capturing out-of-scope resources, publishing org-wide, or self-granting tokens).
Outcome: an EE admin (or an existing manager) can make a user a manager of a group; that user can create/edit/
share/attach the group's connectors, document sets, agents, actions, and manage its membership — strictly
PRIVATE and strictly within their managed groups — enforced authoritatively at the database write.
## Important Notes
- **§8 is greenfield on §1–7; PR0+PR1 now built.** Base system complete (`01-research.md`). PR1 shipped the
schema migration as `c71a18ea7d07` (down_revision `c8e316473aaa`, now head) — the earlier placeholder
`4fa09af6ca14` was never used. Scoped artifacts (PR2+) remain absent.
- **Two-gate model is non-negotiable.** Route gate (`has_permission`, cached flag) only grants
*reachability*; the **authorization of record** is `assert_within_scope`, run **inside the DB write**,
re-reading the resource's **current** groups (`02/03`). The route gate must never authorize.
- **D1 (cache the boolean):** new cached `user.is_group_manager` (sibling to `effective_permissions`, which stays
global-only), recomputed via `recompute_user_permissions__no_commit` (`db/permissions.py:43`) on membership
change and on manager flip. Managed-group **list** stays live (`scoped_group_ids_subquery`).
- **D2:** admins only create top-level groups. **D3:** admin or manager-of-that-group assigns managers.
- **Reuse, don't rebuild:** extend `require_permission` (`auth/permissions.py:257`) with `allow_scope`; reuse the
existing `(user_id)` index; PAT token-cap (`permissions.py:278`, `db/pat.py`) already intersects permissions —
no PAT schema change.
- **Migration ordering trap:** `is_manager` backfill is role-gated (captures GLOBAL_CURATOR, which has no
`is_curator` rows) and **must run before any later release drops `role`/`is_curator`**.
- **`document_set` editable filter is `sa_false()`** today (`db/document_set.py:47`) — the manager branch is a
full build, not a tweak. `credentials` stays owner-scoped (deliberate no-op).
- **Escalation points that gate only on global `manage:*` today:** group membership writes
(`ee/user_group.py:462/504`), connector create (`connector_credential_pair.py:496`), persona group-share
(`ee/persona.py:68`). These are where GATE 2 must be inserted.
- **Conventions (CLAUDE.md):** raise `OnyxError`, strict typing, no `response_model`, DB ops only under
`*/db/`, EE code under `ee/`.
- **Regression-review additions (2026-06-29) — see [03 §11](03-detailed-design.md) for the full checklist:**
- **PREREQUISITE (boot bug, §11.0):** `current_curator_or_admin_user` is gone but still imported by
`skill/api.py:16` + `targeted_reindex.py:22` → the API server won't boot. Fix first (Step 0).
- **D4 actions (§11.1), superseded in part by D8:** keep `MANAGE_ACTIONS` **in the bundle** (GATE 1 reach);
switch the tool/MCP admin endpoints to `allow_scope=True`. The agent-mediated GATE 2 was built and then
**dropped** — managing an action/server is plain **owner-or-admin** (`can_manage_tool` /
`can_manage_mcp_server`), delete included (D9). An MCP-discovered tool has no `user_id`, so
`can_manage_tool` routes it to its **server's** owner; global `MANAGE_ACTIONS` is full-admin-equivalent
here and is not narrowed to `FULL_ADMIN`. Agent-derived scope survives only for *viewing* an MCP server
connected to a managed group.
- **D5 skills (§11.2):** add a **dedicated `MANAGE_SKILLS` permission** (groups UI + bundle; no migration).
Skills do NOT mirror personas — add a NEW scoped admin-list path (don't touch the runtime visibility
filter), GATE 2 on `replace_skill_grants` (the `/grants` seam), re-point `skill/api.py` by verb to
`MANAGE_SKILLS, allow_scope=True` (DELETE stays admin-only).
- **D7 agent→group = `MANAGE_AGENTS`-controlled (§11.5):** group-share is the standard GATE 2 keyed on
`MANAGE_AGENTS` (admin/global bypass; scoped managers ⊆ managed; `ADD_AGENTS`-only can't group-share).
Today's route is `ADD_AGENTS` + editable-fetch, so PR4 adds the `MANAGE_AGENTS` requirement on the
group-share write (a small intended tightening).
- **D6 delete (§11.3), narrowed by D9:** managers do everything *except delete a resource that merely sits
in a managed group* (connector/cc_pair, doc set, admin skill). Deleting something they **created** —
action, MCP server, agent — is ownership, not scope, and stays owner-or-admin.
- **D8 (§11.1, supersedes part of D4):** managing an existing action / MCP server is owner-or-admin
(`can_manage_tool` / `can_manage_mcp_server`); the bundle grants reach + create only.
- **D9 (§11.3, narrows D6):** deleting a resource the manager *created* is owner-or-admin, not admin-only.
- **Persona GATE 2 (§11.5):** `update_persona_access` lacks the actor `User`+`permission`; thread the
acting user into it from all 3 callers (create / share / `/agents`) and gate the shared chokepoint.
- **cc_pair re-attach (§11.6):** `update_user_group` rewrites group↔cc_pair from client `cc_pair_ids` —
run GATE 2 per added cc_pair (else a manager attaches out-of-scope connectors).
- **Corrections (§11.7):** feedback `db/feedback.py` = **no change** (admin-only; not in bundle);
`recompute_user_permissions__no_commit` takes `(user_ids, db_session)` and must be extended to set
`is_group_manager`.
- **Confirmed SAFE (§11.8):** PAT cap, chat runtime, and document/Vespa ACL are untouched — keep them so.
## Implementation Strategy
**Step 0 — Prerequisite boot fix (independent of §8).** Re-point `skill/api.py` (`:16` + deps at
`:173/186/223/259/297/322`) and `targeted_reindex.py:22` off the deleted `current_curator_or_admin_user`
onto `require_permission(...)`. Until this lands, `import onyx.main` raises `ImportError` and nothing runs.
Lands as its own small commit ahead of (or at the head of) PR1.
**Step 1 — Schema + cached flag + migration.** Add `User__UserGroup.is_manager` and `User.is_group_manager`
(`db/models.py`). Migration `c71a18ea7d07` (down_revision `c8e316473aaa`, `alembic/versions/`) ships this in
PR1 — verify, don't author: both columns, role-gated `is_manager` backfill (CURATOR + GLOBAL_CURATOR), and
`is_group_manager` backfill from the result. Extend `recompute_user_permissions__no_commit`
(`db/permissions.py:43`) to recompute `is_group_manager`.
**Step 2 — Auth primitives.** New `auth/scoped_permissions.py`: `SCOPED_MANAGER_PERMISSIONS`,
`scoped_group_ids_subquery`, `get_scoped_groups`, `has_permission` (reads cached flag),
`within_managed_scope_clause`, `assert_within_scope`. Extend `require_permission` with
`allow_scope: bool` (`auth/permissions.py`). Unit-coverable, no endpoints wired yet.
**Step 3 — Manager assignment.** `make_group_manager` / `revoke_group_manager` (`ee/onyx/db/user_group.py`) with
a recompute trigger for the affected user. New EE endpoint `PUT …/user-group/{group_id}/manager`
(`ee/onyx/server/user_group/api.py`) gated `admin ∨ group_id ∈ managed` (D3); reject non-member targets.
**Step 4 — Write-side gates (the security core).** Insert `assert_within_scope` into each scoped write
fn, re-reading current groups in-txn: connector create/update (`db/connector_credential_pair.py:496` +
cc_pair update), document set create/update (`db/document_set.py:220/296`), persona
(`db/persona.py:325`→`ee/persona.py:68`), group update/add-users (`ee/user_group.py:504/462`). Switch those
endpoints to `require_permission(<token>, allow_scope=True)`. Leave **group create** and **set_group_permissions**
on the plain global dependency (D2 + admin-only grants). Ensure bulk endpoints check **every** item.
**Step 5 — Listing/edit filters.** Re-key the editable branch of the 4 filters onto `within_managed_scope_clause`:
`document_set.py` (build from `sa_false()`), `connector_credential_pair.py`, `persona.py`, and **`skill.py`**
(`_add_user_visibility_filter`, the new 7th resource — D5). Enforce managed-scope in the EE `token_limit.py` group
write path. `credentials.py` **and `feedback.py`** unchanged (documented no-ops — feedback is admin-only,
`FULL_ADMIN_PANEL_ACCESS`, and not in the bundle; §11.7). Every branch fails closed on an empty managed set.
**Step 6 — API + frontend.** Add `is_manager` (and optionally `managed_group_ids`) to `GET /users/me/permissions`.
Frontend: `usePermissions` / `hasPermission` consume the flag for nav visibility; group-detail page gets a
per-member "Make/Revoke Manager" toggle (`web/src/app/ee/admin/groups/[groupId]/`) calling the Step-3 endpoint.
**Step 7 — PAT composition verification.** No schema change; add tests proving a scoped PAT narrows permissions
and can never widen group reach (live `is_manager` bounds groups regardless of token).
## Tests
Primary type = **integration** (per CLAUDE.md: real deployment, hardest to fake; this is a security boundary).
Use `UserGroupManager` / resource managers in `tests/integration/common_utils`; prefer fixtures.
> **Find the existing home before writing a new test file.** The new permission system already has
> strong coverage — don't reflexively `git add` a fresh test file. First locate the suite that already
> exercises the behavior, read it, confirm it's sound, and **extend it** when the new assertion belongs to
> a flow it already drives. Only create a new file when no existing suite covers the behavior. Known homes:
> - recompute / `effective_permissions` → `tests/integration/tests/usergroup/test_group_membership_updates_user_permissions.py`
> - grant / revoke (bulk) + implied-expansion → `tests/integration/tests/usergroup/test_group_permission_toggle.py`
> - registration / default-group propagation + fixtures → `tests/integration/tests/permissions/` (`test_auth_permission_propagation.py`, `conftest.py`)
> - read-time permission expansion (pure logic) → `tests/unit/onyx/auth/test_permissions.py`
>
> Worked example (PR1): `is_group_manager` is the *second* column `recompute_user_permissions__no_commit`
> writes, so its coverage was folded into the existing recompute test above — **not** a standalone file. An
> earlier standalone `test_is_manager_recompute.py` was deleted because it duplicated that home, flipped the
> flag on an incidental default-group membership, and re-ran the migration's copied SQL as a self-referential
> oracle. The migration-backfill case is the one genuinely new home (own file under
> `tests/integration/tests/migrations/`, running the real alembic migration — never a copied-SQL oracle).
- **Escalation suite (integration)** — for a manager of group X: (a) capture-by-reassign rejected
(`PUT resource{groups:[X]}` on a resource currently in Y → 403); (b) PUBLIC/SYNC create+edit rejected;
(c) cross-group membership add rejected (add to Y); (d) **fail-closed** — a user with `is_manager` on zero
groups gets empty editable lists, not everything; (e) `set_group_permissions` rejected for a manager;
(f) bulk edit rejects the batch if any item is out of scope; (g) happy paths (create/edit/attach/detach/member
add within X) succeed.
- **PAT (integration)** — a manager's PAT scoped to `manage:connectors` edits only X's connectors and cannot
reach Y; a PAT cannot widen group reach.
- **Migration backfill (DEFERRED by owner; revisit before GA) — integration, real alembic** — a new test under
`tests/integration/tests/migrations/` that seeds CURATOR(+is_curator) and a zero-`is_curator` GLOBAL_CURATOR,
runs the actual migration (down→up), and asserts `is_manager` (GLOBAL_CURATOR captured on all memberships) +
`is_group_manager` mirror; fresh-install leaves all false. NOT a copied-SQL oracle.
- **Filter SQL (external-dependency unit)** — `within_managed_scope_clause` returns exactly the resources whose
every group ⊆ managed and ≥1 group and private (the `document_set` rebuild especially).
- **Manager toggle UI (playwright, 1 test)** — admin assigns a manager via the group page; that user then sees
the scoped admin pages and only their group's resources.
---
## Plan Challenge Results
### 1. Extendability & Scalability: PASS (one refinement)
Adding a new manageable resource = add token to `SCOPED_MANAGER_PERMISSIONS` + re-key its filter + insert the
gate in its write fn; the `within_managed_scope_clause` helper is reused, no hardcoded limits. **Refinement
(folded in):** in bulk endpoints, resolve `get_scoped_groups(user)` **once per request** and pass the set to the
per-item gate — avoids N indexed reads on batch edits. The route-gate cost is O(1) (cached `is_group_manager`).
### 2. Fragility: CONCERN → hardened
The model's load-bearing assumption is *every* scoped write path calls `assert_within_scope`; a future
write path that forgets it is an escalation. Hardenings added to the plan:
- **Minimize insertion sites** — route resource→group attaches through the existing single junction writers
(e.g. `_relate_groups_to_cc_pair__no_commit`) so the gate has few, obvious homes, not scattered call sites.
- **Defense-in-depth test** — the escalation integration suite asserts each scoped endpoint rejects out-of-scope
input, so a missing gate fails CI.
- **Cache-staleness is security-safe by construction** — the cached `is_group_manager` is only a *reachability*
hint for GATE 1 (can only reject earlier). The authorization of record (GATE 2) resolves the managed-group
**list live**, so a stale `true` still hits a live `managed={} → 403` (fail-closed); a stale `false` is an
availability bug, never an escalation. Recompute must fire on every `is_manager` flip (already specified).
### 3. Industry Standard: VERIFIED
Searched scoped-RBAC / delegated-admin best practices (2025), k8s RoleBinding delegation, and Zanzibar
live-vs-materialized. Findings confirm the design: scoped (not global) roles with a **User→Role→Scope** model are
the recommended pattern; access control should be **centralized at the data/policy layer**, not the gateway
(Oso/OpenFGA/Cedar). k8s binds a *role* to a *namespace* (our bundle→group) and **prevents privilege escalation
by only allowing you to bind a role you already hold at that scope** — exactly what D3 in-group delegation does
(a manager grants the same bundle it holds, within a group it manages). The two-gate + write-side enforcement is
industry-standard.
### 4. Fact Check: PASS (and D1 strengthened)
- *k8s RoleBinding namespace-scoping + escalation-prevention bind rule* — **verified**.
- *Zanzibar resolves live to avoid the "new-enemy problem" (Alice removes Bob, adds docs, Bob must not see them)*
— **verified**, and it nuances D1 favorably: Zanzibar serves ~99% of checks from **bounded-stale** local
replicas and only forces freshness for critical ops (zookies). So **caching the `is_manager` boolean (D1) is
consistent with industry practice** *because* the security-critical decision (GATE 2) stays live — the original
"must resolve everything live" framing was stricter than the standard requires.
- *AWS IAM permission boundaries / Azure scoped RBAC (narrow-only)* — widely documented; carried from the prior
46-agent review (not re-searched this round). Confidence high.
### 5. Maintainability: PASS
Two gates, one code bundle, one cached flag — graspable in <15 min. Extends existing seams (`require_permission`,
`_add_user_filters`, the recompute hooks) rather than inventing a parallel system. Watch item: the
`within_managed_scope_clause` SQL and the `document_set` rebuild (from `sa_false()`) need clear docstrings so the
"every group ⊆ managed ∧ ≥1 ∧ private" predicate stays consistent across the four filters.
### 6. Patch vs. Fix: PROPER FIX
Root cause (no scoped delegation) is solved with a first-class model; no error suppression, no timeouts, no
workarounds. Reusing the `is_curator` column is a deliberate, documented single-resolver decision (not a
tombstone hack) — the old dual-path *meaning* is gone. **No patch/fix decision needed from the user.**
**Verdict: plan passes all six checks. No blocking concerns; the fragility hardenings are folded into Steps 4–5
and the test plan.**
docs/group-manager-scoped-permissions/05-pr-roadmap.md
+229
View File
@@ -0,0 +1,229 @@
> Status: active · Task: group-manager-scoped-permissions · Source plan: 04-implementation-plan.md
# §8 Scoped Permissions (Group Manager) — PR Roadmap
> **Primitives per [03 §2](03-detailed-design.md) (single-classifier model).** `has_permission` returns
> `PermissionAuthority` — one classifier replacing the old boolean form, since a scoped grant is
> group-qualified and can't be a flat bool; `has_global_permission` is the GLOBAL-only bool helper.
> GATE 1 = `require_permission(..., allow_scope=True)` threshold; GATE 2 =
> `assert_within_scope` / `assert_global`. Names below updated to match.
Six PRs, dependency-ordered. **Safety invariant:** every enforcement PR lands its read-filter + write-side gate
+ endpoint `allow_scope` switch **together** — a manager can never reach an endpoint before its GATE 2 exists, so
there is no escalation window at any merge boundary. (Pre-GA branch: existing curators were already collapsed to
STANDARD by the `account_type` backfill, so no live capability is regressed while enforcement lands incrementally.)
> **Revised by the 2026-06-29 regression review** — see [03 §11](03-detailed-design.md) for the full case
> checklist. Net changes to this roadmap: a new **PR0** (boot-fix prerequisite); **PR4** adds **skills** (D5)
> and **actions** scoping (D4 — `manage:actions` stays in the bundle for GATE 1 reach + create; the
> agent-mediated GATE 2 was later dropped, see **D8**);
> **delete stays admin-only** across all PRs (D6, narrowed by **D9** — a creator may delete their own
> action/MCP server); PR3/PR5 enumerate
> the previously-missed write endpoints (cc_pair status/name/property/prune, persona `/share`, group rename,
> `/agents`) and the persona-gate signature fix (§11.5) + cc_pair-reattach fix (§11.6).
## Overview
| PR | Title | Est. LOC | Depends on | Key deliverable |
|----|-------|----------|------------|-----------------|
| 0 | `fix(perms): re-point skill/targeted-reindex off removed curator dep` | ~30 | — | **Unblocks boot** — `import onyx.main` currently fails (§11.0); independent of §8 |
| 1 | `feat(perms): add is_manager + is_group_manager columns and backfill` | ~220 | 0 | Schema + role-gated migration + cached-flag recompute |
| 2 | `feat(perms): scoped-manager authorization primitives` | ~330 | 1 | `scoped_permissions.py` (bundle w/o actions, gates, scope helpers) + `require_permission(allow_scope)` — inert |
| 3 | `feat(perms): scope connectors & document sets to group managers` | ~560 | 2 | Two-gate enforcement on cc_pairs + doc sets incl. status/name/property/prune (delete admin-only) |
| 4 | `feat(perms): scope agents, skills & token limits to group managers` | ~520 | 2 | Persona (+ `/share`, `/agents`, gate-signature fix §11.5) + **skills** + token-limit enforcement + scoped-PAT tests |
| 5 | `feat(perms): group-manager assignment & membership scoping` | ~430 | 3,4 | make/revoke + assign endpoint (D3) + membership gates + cc_pair-reattach gate (§11.6) + group rename + `/me/permissions.is_manager` |
| 6 | `feat(perms): group-manager assignment UI & scoped nav` | ~370 | 5 | Group-detail Make/Revoke toggle + manager nav visibility + Playwright |
## Sequence
```
PR0 (boot-fix: curator dep) ─▶ PR1 (schema+migration+recompute)
└─▶ PR2 (auth primitives — inert)
├─▶ PR3 (connectors + doc sets enforcement) ┐
└─▶ PR4 (agents + skills + token limits) ┘ both build on PR2 only; mergeable in either order
▼
PR5 (assignment + membership scoping)
▼
PR6 (frontend)
```
Walking skeleton = PR1→PR2→PR3: after PR3 the full two-gate model is provably working on the two
highest-value resource types, exercised by the escalation integration suite (managers seeded via fixture).
---
## PR 0 — Boot-fix prerequisite: re-point off the removed curator dep
- **Goal:** restore a bootable app — independent of §8, but blocks every later PR.
- **Scope (in):** `current_curator_or_admin_user` was deleted from `onyx/auth/users.py` by §1–7 but is still
imported by `server/features/skill/api.py:16` (deps `:173/186/223/259/297/322`) and
`server/documents/targeted_reindex.py:22`, so `import onyx.main` raises `ImportError`. Re-point both onto the
correct `require_permission(...)` dep: `targeted_reindex.py:80/163` → `MANAGE_CONNECTORS` (its connector
peers); skills → `FULL_ADMIN_PANEL_ACCESS` in PR0 (safe, unbreaks boot), narrowed to the new
`MANAGE_SKILLS, allow_scope=True` when the skill GATE 2 lands in PR4 (§11.2). DELETE endpoints excluded
(stay admin-only).
- **Out of scope:** any manager scoping (that's PR1+).
- **Est. size:** ~30 LOC.
- **Depends on:** —
- **Tests on merge:** `python -c "import onyx.main"` succeeds; skill admin + targeted-reindex endpoints import.
- **Drift checkpoint:** confirm whether §1–7 already intends a specific replacement dep for these endpoints
(align with how the other ex-curator endpoints were migrated) before picking the token.
## PR 1 — Schema foundation: `is_manager` + cached flag + backfill
- **Goal:** add the only new state §8 needs and preserve the curator signal before it can be lost.
- **Scope (in):** `User__UserGroup.is_manager`; `User.is_group_manager` (cached route-gate flag); migration
`c71a18ea7d07` (down_revision `c8e316473aaa`, role-gated backfill capturing CURATOR + GLOBAL_CURATOR, then
`is_group_manager` backfill); extend `recompute_user_permissions__no_commit` to recompute the cached flag.
- **Out of scope:** any reader of the new columns (they're inert this PR); dropping `is_curator`/`role`.
- **Files:**
| File | New/Modified | This PR's slice |
|------|--------------|-----------------|
| `backend/onyx/db/models.py` | modified | 2 boolean columns (`User__UserGroup`, `User`) |
| `backend/alembic/versions/c71a18ea7d07_*.py` | new | add columns + role-gated backfill |
| `backend/onyx/db/permissions.py` | modified | recompute sets `is_group_manager` |
| `backend/tests/integration/tests/usergroup/test_group_membership_updates_user_permissions.py` | modified | recompute sets `is_group_manager` (folded into the existing recompute test) |
| `tests/integration/tests/migrations/test_is_manager_backfill_migration.py` | new (DEFERRED) | backfill correctness via real alembic |
- **Est. size:** ~220 LOC
- **Depends on:** —
- **Feature-flag state:** N/A — additive, columns unread until PR3+.
- **Tests on merge:** integration — `is_group_manager` set/cleared by `recompute` on a managed edge (folded
into the existing recompute test). Backfill correctness (CURATOR / zero-`is_curator` GLOBAL_CURATOR /
fresh-install) is DEFERRED — revisit as a real-alembic migration test before GA.
- **Drift checkpoint:** confirm `c8e316473aaa` is still head; confirm the pre-GA assumption (curators already
collapsed to STANDARD) still holds so migrated `is_manager` bits are dormant, not a silent re-grant. If §6.1.1
snapshot caveat matters operationally, decide whether the zero-managed-group report ships now or later.
## PR 2 — Authorization primitives (inert core)
- **Goal:** land the reusable scope logic and the route-gate extension with no behavior change.
- **Scope (in):** new `backend/onyx/auth/scoped_permissions.py` — `SCOPED_MANAGER_PERMISSIONS`,
`scoped_group_ids_subquery`, `get_scoped_groups`, `has_permission` (reads cached flag),
`within_managed_scope_clause`, `assert_within_scope`; extend `require_permission(allow_scope=False)`.
- **Out of scope:** wiring any endpoint/filter to them (PR3+).
- **Files:**
| File | New/Modified | This PR's slice |
|------|--------------|-----------------|
| `backend/onyx/auth/scoped_permissions.py` | new | bundle + scope helpers + both gates |
| `backend/onyx/auth/permissions.py` | modified | `require_permission(..., allow_scope)` |
| `backend/tests/external_dependency_unit/.../test_scoped_permissions.py` | new | gate logic + clause SQL |
- **Est. size:** ~330 LOC
- **Depends on:** PR 1
- **Feature-flag state:** N/A — nothing calls these yet.
- **Tests on merge:** unit/external-dependency unit — `assert_within_scope` invariants (⊆ managed,
non-empty, private, fail-closed, admin/global bypass); `within_managed_scope_clause` selects the right rows.
- **Drift checkpoint:** confirm the bundle is the 7-token set — includes `manage:actions` (D4 keeps it in the
bundle for reach + create; manage itself is owner-or-admin per **D8**) and the new `manage:skills` (D5);
confirm `require_permission`'s token-cap branch is unchanged since `03`.
## PR 3 — Enforce scope on connectors & document sets (walking skeleton)
- **Goal:** prove the full two-gate model end-to-end on the two highest-value resources.
- **Scope (in):** rewrite editable filters in `connector_credential_pair.py` and `document_set.py` (the latter
built from today's `sa_false()`) onto `within_managed_scope_clause`; insert `assert_within_scope` in
cc_pair create/update + doc-set create/update DB fns (re-reading current groups in-txn); switch the **full set**
of manager-reachable endpoints (§11.4) to `require_permission(<token>, allow_scope=True)` — connector create
(mock-cred `connector.py:1568` **and** bare `:1538`), associate-credential `cc_pair.py:716`, cc_pair status
`:427`/name `:512`/property `:542`/prune `:604`, ee `sync_cc_pair_groups`, doc-set create/patch — **but leave
connector/cc_pair and doc-set DELETE on the global admin dep (D6)**; resolve `get_scoped_groups` once per
request on bulk paths.
- **Out of scope:** agents/feedback/token-limit (PR4); membership/assignment (PR5); UI (PR6).
- **Files:**
| File | New/Modified | This PR's slice |
|------|--------------|-----------------|
| `backend/onyx/db/connector_credential_pair.py` | modified | filter re-key + create/update gate |
| `backend/onyx/db/document_set.py` | modified | filter rebuild + create/update gate |
| `backend/onyx/server/documents/{connector,cc_pair}.py` | modified | `allow_scope=True` deps |
| `backend/onyx/server/features/document_set/api.py` | modified | `allow_scope=True` deps |
| `backend/tests/integration/.../test_group_manager_resources.py` | new | escalation suite (these two) |
- **Est. size:** ~520 LOC
- **Depends on:** PR 2
- **Feature-flag state:** N/A — safe by the lands-together invariant; managers seeded via fixture for tests.
- **Tests on merge:** integration — capture-by-reassign rejected; PUBLIC/SYNC rejected; fail-closed empty scope;
bulk per-item; happy paths (create/edit/attach/detach within managed groups) succeed.
- **Drift checkpoint:** confirm the cc_pair update path that sets groups/access (file may differ from `03`'s
guess `server/documents/cc_pair.py`) — locate the actual group/access setter before coding. Confirm doc sets
still use `is_public` (not an `access_type`) for the private check.
## PR 4 — Enforce scope on agents, skills, actions & token limits
- **Goal:** extend the proven model to the remaining manager-scoped resources; verify PAT composition.
- **Scope (in):** persona filter + thread the acting `user: User` into `update_persona_access` (both MIT+EE
twins); persona group-share is **`MANAGE_AGENTS`-gated via GATE 2 (D7, §11.5)** — admin/global bypass,
scoped managers ⊆ managed, `ADD_AGENTS`-only can't group-share (today's route is `ADD_AGENTS` + editable
fetch, so PR4 adds the `MANAGE_AGENTS` check on the group-share write); `is_public` stays owner/admin-gated. **Skills (D5):** new dedicated **`MANAGE_SKILLS`** permission (enum + registry/groups UI +
bundle; no migration), a NEW scoped admin-list path (do NOT touch the runtime visibility filter), GATE 2 on
`replace_skill_grants`, re-point `skill/api.py` by verb to `MANAGE_SKILLS, allow_scope=True` (§11.2);
managed-scope enforcement in EE `token_limit.py` group write path; `credentials.py` **and `feedback.py`** left
unchanged (documented no-ops); **admin skill delete stays admin-only (D6); persona delete is owner-or-admin
(D9)**; scoped-PAT tests. **Actions
(D4 + D8):** `MANAGE_ACTIONS` is in the bundle; switch the tool/MCP admin endpoints to `allow_scope=True`
so a manager can reach them and create their own. Managing an existing action/server stays
**owner-or-admin** (`can_manage_own_tool` / `_ensure_mcp_server_owner_or_admin`) — the agent-mediated GATE 2
once planned here was built and then dropped (**D8**); agent-derived scope survives only for *viewing* an
MCP server connected to a managed group. A creator may delete what they made (**D9**).
- **Out of scope:** membership/assignment (PR5); UI (PR6).
- **Files:**
| File | New/Modified | This PR's slice |
|------|--------------|-----------------|
| `backend/onyx/db/persona.py` | modified | filter + `update_persona_access` MIT twin gate + owner carve-out (§11.5) |
| `backend/ee/onyx/db/persona.py` | modified | `update_persona_access` EE gate (lockstep signature) |
| `backend/onyx/db/skill.py` | modified | scoped admin-list path + `replace_skill_grants` GATE 2 + is_public toggle gate (§11.2) |
| `backend/onyx/server/features/skill/api.py` | modified | re-point off curator dep; `allow_scope=True` by verb (DELETE stays admin-only) |
| `backend/onyx/server/features/{tool,mcp}/api.py` | modified | `allow_scope=True` (reach + create); manage (edit/toggle/auth) stays **owner-or-admin** — agent-mediated GATE 2 dropped per **D8** |
| `backend/onyx/db/tools.py` | modified | `can_manage_own_tool` / `can_manage_mcp_server` owner-or-admin gates (D8); agent-mediated scope kept only for *view* |
| `backend/ee/onyx/db/token_limit.py` | modified | managed-scope on group token-limit writes |
| `backend/onyx/server/.../persona api` | modified | `ADD_AGENTS, allow_scope=True` deps |
| `backend/tests/integration/.../test_group_manager_agents.py` | new | agent + skill + action escalation + ADD_AGENTS-owner no-regression + PAT narrowing |
| `backend/onyx/db/feedback.py` | unchanged | NO CHANGE — admin-only, not in bundle (§11.7) |
- **Est. size:** ~620 LOC (agents + skills + actions + token limits)
- **Depends on:** PR 2 (independent of PR 3)
- **Feature-flag state:** N/A — lands-together invariant.
- **Tests on merge:** integration — agent create/share scoped; manager can't widen a PAT's group reach; `add:agents`
ownership tier still keyed on `Persona.user_id`; credentials remain owner-only for a manager.
- **Drift checkpoint:** confirm the persona create/update endpoint path + that `manage:agents` is the right
scoped token. (The actions-via-agents assumption no longer gates anything — D8 made action/MCP manage
owner-or-admin, so there is nothing to derive from the agent join.)
## PR 5 — Manager assignment & group-membership scoping (backend complete)
- **Goal:** let admins/in-group managers create managers, and scope membership edits; expose the flag to clients.
- **Scope (in):** `make_group_manager`/`revoke_group_manager` (`ee/onyx/db/user_group.py`) + recompute trigger
(extend `recompute_user_permissions__no_commit(user_ids, db_session)` to set `is_group_manager` — §11.7);
gates in `update_user_group`/`add_users_to_user_group` (`group_id ∈ managed`) **plus a per-cc_pair GATE 2 on
`update_user_group`'s `cc_pair_ids` re-attach (§11.6 — else a manager attaches out-of-scope connectors)**;
`allow_scope=True` on group update/add-users **and rename (`:164`)** endpoints; **scoped group-LIST**
(`fetch_user_groups` manager variant filtered by `is_manager`, `list_user_groups`+detail/member reads to
`allow_scope=True` returning only managed groups — §11.9, or the Groups page/assign-UI breaks); **new**
`PUT …/user-group/{group_id}/manager` gated `admin ∨ group_id ∈ managed` (D3); group **create**, **delete**,
and `set_group_permissions` left admin-only (D2/D6); add `is_manager` (+`managed_group_ids`) to
`GET /users/me/permissions`.
- **Out of scope:** frontend (PR6).
- **Files:**
| File | New/Modified | This PR's slice |
|------|--------------|-----------------|
| `backend/ee/onyx/db/user_group.py` | modified | make/revoke + membership gates |
| `backend/ee/onyx/server/user_group/api.py` | modified | allow_scope deps + manager-assign endpoint |
| `backend/onyx/server/.../permissions api` | modified | `/me/permissions` adds `is_manager` |
| `backend/tests/integration/.../test_group_manager_membership.py` | new | assign authz + membership scope |
- **Est. size:** ~400 LOC
- **Depends on:** PR 3, PR 4
- **Feature-flag state:** N/A — this is the backend "on switch" for creating new managers.
- **Tests on merge:** integration — cross-group member add rejected; `set_group_permissions` rejected for
managers; assign endpoint allows admin + manager-of-that-group, rejects others; non-member target → 400.
- **Drift checkpoint:** re-confirm D2 (admins-only create) and D3 (admin-or-manager assign) still hold; confirm
the `/me/permissions` response shape the frontend (PR6) will consume.
## PR 6 — Frontend: assignment UI & scoped nav
- **Goal:** make the capability usable and visible to humans.
- **Scope (in):** `usePermissions`/`hasPermission` consume `is_manager` for nav visibility; group-detail
per-member "Make/Revoke Manager" toggle calling the PR5 endpoint; Playwright happy-path.
- **Out of scope:** any backend change (all landed PR1–5).
- **Files:**
| File | New/Modified | This PR's slice |
|------|--------------|-----------------|
| `web/src/lib/.../usePermissions.ts` | modified | `is_manager` flag |
| `web/src/.../hasPermission.ts` | modified | manager nav visibility |
| `web/src/app/ee/admin/groups/[groupId]/...` | modified | Make/Revoke Manager toggle |
| `web/tests/e2e/.../group-manager.spec.ts` | new | assign → scoped pages visible |
- **Est. size:** ~370 LOC
- **Depends on:** PR 5
- **Feature-flag state:** N/A — final wiring; feature fully live after merge.
- **Tests on merge:** Playwright — admin assigns a manager on the group page; that user sees the scoped admin
pages and only their group's resources; non-managed pages/resources hidden.
- **Drift checkpoint:** confirm the group-detail page location under `web/src/app/ee/admin/groups/[groupId]/` and
whether an old "Make Curator" affordance still exists to replace rather than add beside.
docs/group-manager-scoped-permissions/07-ui-capability-model-design.md
+1251
View File
File diff suppressed because it is too large. Load diff
docs/group-manager-scoped-permissions/10-branch-audit-anon-and-service-accounts.md
+286
View File
@@ -0,0 +1,286 @@
# §10 Branch Audit — Anonymous Access, Groupless Service Accounts, and a Live Admin Escalation
Audit of `new-permission-system` vs `origin/main` (merge-base `6376385614`), requested to (1) understand
the branch, (2) chase a reported anonymous-access regression, and (3) define groupless-service-account
behavior. Method: route/actor matrix built from the live FastAPI dependency graph on both branches (507
routes each), live probes against a running deployment, plus a 4-hunter / adversarial-verify pass (60
agents, 32 findings survived verification, 16 refuted, 2 unverified on a session-limit cutoff).
**Headline: no anonymous-access regression was found.** The audit instead surfaced a **live,
reproduced CRITICAL privilege escalation** unrelated to anonymous access — see §3.
> **Status (2026-08-17, updated):** The CRITICAL finding (§3.3) is **fixed** — `0ad792f85e`
> (`_assert_no_privilege_amplification`), committed by a parallel session that read this doc while it was
> still being written. Independently re-verified against the current worktree by calling
> `add_users_to_user_group` directly (bypassing the HTTP layer): raises `OnyxError(INSUFFICIENT_PERMISSIONS)`
> and leaves the attacker's `effective_permissions` unchanged. The original live HTTP repro in §3.3 succeeded
> only because it ran against a stale `api_server` process (started before the fix commit, no `--reload`) —
> **restart the api_server before re-testing any of these findings against a live server.**
>
> Also fixed by the same parallel session: **`690129858c`** (connector-write surface for scoped managers —
> ingestion/run-once/targeted-reindex regained `allow_scope`; `get_llm_for_persona` aligned to `MANAGE_LLMS`;
> agent-editor tool visibility) and **`28063d2784`** (dead `UserRole`/curator residue removed, including
> `CURATORS_CANNOT_VIEW_OR_EDIT_NON_OWNED_ASSISTANTS` — resolved by deletion rather than restoration, since
> the migration to token-based auth left it unreachable either way).
>
> Additionally fixed **in this session**: `GET /admin/index-attempt/{id}/stage-metrics` was missing
> `allow_scope=True` at GATE 1 despite its own GATE 2 already scoping correctly
> (`backend/onyx/server/documents/cc_pair.py`) — a scoped manager who could read the sibling
> `index-attempts` list was wrongly 403'd here. Fixed + covered by two new integration tests in
> `test_group_manager_resources.py` (`test_manager_reads_stage_metrics_of_managed_cc_pair`,
> `test_manager_cannot_read_stage_metrics_of_unmanaged_cc_pair`). **Left unstaged, not committed** — the
> integration test needs a restarted `api_server` to actually run (see above); verified independently via
> direct route-dependency inspection instead.
>
> Two findings below are **reclassified**, not fixed, after cross-checking the locked design decisions in
> [00-index.md](00-index.md): "group manager can no longer delete a shared cc-pair/doc-set" matches **D6**
> ("managers may do everything except delete") — not a bug. "`DELETE /admin/credential/{id}` moved to
> `MANAGE_CONNECTORS` with no ownership scoping" is GLOBAL-only (no `allow_scope`, so no scoped-manager
> leak) and looks like the same accepted broadening as `MANAGE_SERVICE_ACCOUNT_API_KEYS` (doc 08 finding
> #1, "admin-equivalent, accepted, won't fix") — flagged as an open question for the owner, not fixed.
>
> Remaining open items needing an owner decision before a backend fix makes sense: EE non-admins losing
> `add:agents` (no migration ever grants it — granting it broadly is a capability decision, not a pure bug
> fix), Community Edition having no path to promote/demote an admin, and whether `GET /manage/users`'s
> org-wide visibility should be narrowed for a `MANAGE_LLMS`-only delegate (it also serves as the
> group-member-picker for `MANAGE_USER_GROUPS` delegates, which have a real need to search the whole
> directory — narrowing it is a design call, not a mechanical fix).
---
## 1. What the branch changes
Main already had the permission-token core (`require_permission`, `IMPLIED_PERMISSIONS`,
`User.effective_permissions`). This branch adds the **scoped group manager** layer and removes `UserRole`
entirely:
- `User__UserGroup.is_manager` (per-membership edge) + cached `User.is_group_manager`
(`backend/onyx/db/models.py`), written by `recompute_user_permissions__no_commit`
(`backend/onyx/db/permissions.py:52-150`).
- `has_permission(user, perm) -> GLOBAL | SCOPED | NONE` (`backend/onyx/auth/permissions.py:296-310`).
Two-gate model: GATE 1 (`allow_scope=True` on `require_permission`) lets a SCOPED manager *reach* a
route; GATE 2 (`assert_within_scope` / `assert_manages_group` / a managed-scope read clause) must
confine them, or every resource leaks. `backend/onyx/auth/scoped_permissions.py` is the primitive layer.
- Read-side capability projection: `permissions{}` maps stamped on DTOs
(`backend/onyx/auth/permission_projection.py`), affordance-only, never an authz input.
- `UserRole` is now a nullable tombstone (`backend/onyx/db/models.py:333`, migration
`c8e316473aaa_make_user_role_nullable.py`) and is read nowhere in `backend/onyx` or `backend/ee`.
`GET /get-user-role` and `PATCH /manage/set-user-role` are deleted (both 404 live).
- Only two migrations: `is_manager`/`is_group_manager` (with a CURATOR/GLOBAL_CURATOR → `is_manager`
backfill) and the role-nullable migration. **No migration touches `permission_grant` rows.**
| Old (`UserRole`) | New equivalent |
|---|---|
| `ADMIN` | `FULL_ADMIN_PANEL_ACCESS` via the seeded Admin group (`admin` grant) |
| `BASIC` | `BASIC_ACCESS` via the seeded Basic group |
| `GLOBAL_CURATOR` / `CURATOR` | `is_manager` edge on a `User__UserGroup` row → `is_group_manager` |
| `LIMITED` (service account, no group) | `SERVICE_ACCOUNT` + empty `effective_permissions` → `account_derived_permissions` gives `write:chat` only |
| `SLACK_USER` / `EXT_PERM_USER` | unchanged: `AccountType.BOT` / `AccountType.EXT_PERM_USER`, no group system |
---
## 2. Anonymous user access — no regression confirmed
Built the actor×route matrix from the live dependency graph on **both** `main` and HEAD. The
anonymous-capable route set is **identical** — same 13 routes on both branches:
```
GET /chat/get-chat-session/{session_id} POST /chat/create-chat-session
POST /chat/send-chat-message POST /chat/create-chat-message-feedback
DELETE /chat/remove-chat-message-feedback GET /chat/available-context-tokens/{id}
GET /chat/chat-session/{id}/resume-stream GET /manage/connector-status
GET /persona GET /agents
GET /settings GET /llm/provider
GET /llm/persona/{persona_id}/providers
```
Live-verified end to end: flipped `anonymous_user_enabled` on, sent an anonymous chat message, got a real
streamed model response, confirmed the write-surface routes still 401 (`/chat/file`, `/tool`,
`/chat/get-user-chat-sessions`), flipped the setting back off. No route gained or lost `allow_anonymous`
between branches; `/chat/file` has required an authenticated user since `f3ff4b57bd` on main, long before
this branch.
Three real anonymous-adjacent findings did survive verification — all low/info, none change what an
anonymous visitor can read or write:
- **LOW — Anonymous visitors are stamped `edit: true` / `share: true` on every public listed agent.**
`backend/onyx/db/persona.py:116-121`'s anonymous carve-out sits *above* the `get_editable` split at
`:130`, so it returns the same `is_public AND is_listed` clause for both edit and view queries.
`stamp_persona_permissions` (`persona.py:539-588`) then marks every such agent editable/shareable in
the `GET /persona` and `GET /agents` responses, and `AgentCard.tsx:103,115` renders Edit/Share buttons
for signed-out visitors — including on built-in agents, which main's `checkUserCanEditAgent` explicitly
excluded. **Live-reproduced**: with anonymity on, `GET /api/persona` returned
`"permissions":{"edit":true,"share":true,...}` on the default public agent. No write lands — the
single-agent GET and the share PATCH both reject anonymous — so this is a UI dead-end, not a data leak.
Fix: move the anonymous return inside the `get_editable` branch in `_add_user_filters`.
- **LOW — `GET /me` for the anonymous user under-reports its own permissions.**
`fetch_anonymous_user_info` (`backend/onyx/auth/anonymous_user.py:50-61`) builds `UserInfo` without
passing `effective_permissions`/`admin_capabilities`, so the anonymous `/me` response reports `[]` for
both while the in-process `User` object actually resolves `["basic"]` → the full BASIC bundle. Nothing
client-side gates on those fields for anonymous today, so this is currently cosmetic — but it is a
latent trap for the next feature that checks `permissions.includes(...)` before rendering a chat
affordance. Fix: pass `effective_permissions=["basic", ...]` (or the resolved set) into the `UserInfo`
constructor.
- **INFO — `document_set._add_user_filters` and `ee/onyx/db/token_limit.py`'s anonymous carve-outs were
deleted without replacement**, and now fail closed (empty result) for anonymous on the editable path.
Currently unreachable (no anonymous-facing route calls the editable path), recorded for completeness.
---
## 3. Groupless service account — definitive behavior, plus a live escalation found nearby
### 3.1 The requested spec
Creation with `group_ids: []` succeeds. `account_derived_permissions()`
(`backend/onyx/db/permissions.py:40-49`) is the single mechanism that keeps a groupless service account
usable at all — it hands it `write:chat` (which implies `read:chat`) purely because it's a
`SERVICE_ACCOUNT` with zero group memberships. `is_limited_user` (`backend/onyx/db/users.py:44-60`) is
**not** triggered, so the key authenticates normally.
Live-probed against a running deployment:
| Route class | Result |
|---|---|
| `GET /me`, `POST /chat/create-chat-session`, `GET /chat/get-user-chat-sessions` | **200** |
| `/settings`, `/tool`, `/manage/document-set`, `/manage/users`, `/onyx-api/ingestion`, `POST /user/pats` | **403** `INSUFFICIENT_PERMISSIONS` |
This mirrors main's `role=LIMITED` service account (`role_derived_permissions` keyed on `UserRole.LIMITED`
→ now keyed on "service account in no group") — same chat-only capability, same shape, different
mechanism. **Not a regression** for the groupless case itself.
### 3.2 Sharp edges around it (verified)
- **MEDIUM — A rename-only `PATCH /admin/api-key/{id}` silently strips every group.**
`APIKeyArgs.group_ids` defaults to `[]` (`backend/onyx/server/api_key/models.py`); `update_api_key`
(`backend/onyx/db/api_key.py:172-205`) always calls `set_user_groups__no_commit` with whatever
`group_ids` the client sent. A client that omits the field to do a pure name-change (main's contract,
where `role` had no bearing on a rename) now demotes the key from admin/basic to chat-only. The web UI
always resends current groups, so only direct API callers hit this — but the blast radius is worse than
main's analogous footgun (main defaulted to `BASIC`, not chat-only). **Live-reproduced**: posting
`{"name":"x","role":"admin"}` (the old-client shape) against the new endpoint silently created a
chat-only key — the unknown `role` field is dropped by pydantic with no error.
- **MEDIUM — Community Edition can only ever create chat-only service accounts.** `ApiKeyFormModal`'s
group picker is fed by `useGroups(true)`, which is hard-empty in CE (no group-permission UI exists
there). Every CE-created key is therefore permanently groupless → chat-only. On main, CE defaulted new
keys to the `BASIC` role, which included `read:search`, `generate:image`, `use:llm_gateway`. This is a
genuine CE regression, not an artifact of the groupless case being intentionally narrow.
- **LOW — `is_limited_user`'s `SERVICE_ACCOUNT` branch is now dead code.** Every group (including any
admin-created one) auto-grants `BASIC_ACCESS` on creation (`ee/onyx/db/user_group.py:509`,
`ee/onyx/db/scim.py:998`), and `basic` is non-toggleable — so a service account with ≥1 group can never
have empty `effective_permissions`. The `not user.effective_permissions` check only ever fires for the
zero-group case, which is already handled by `account_derived_permissions`. Docstring says "no group
membership" as the trigger; code says "and permissions ended up empty," which given the auto-grant is
the same thing today but is fragile to a future group ever *not* granting `basic`.
- **LOW/design — a service account in any group can never be scoped below the full Basic bundle**
(`read:search`, `read:chat`, `write:chat`, `generate:image`, `use:llm_gateway`) — there's no
intermediate tier between "groupless → chat-only" and "any group → full basic". Worth a product
decision, not a bug.
- **LOW — `set_user_groups__no_commit` deletes and re-inserts `User__UserGroup` rows wholesale**
(`backend/onyx/db/users.py:796-798`), which strips any `is_manager` edge. A service account made a
group manager loses that status silently the next time its key is edited (rename, group change) via
`update_api_key`. SCIM's group-sync path already diffs instead of delete-all for this reason
(`ee/onyx/db/scim.py`); `set_user_groups__no_commit` should do the same.
### 3.3 CRITICAL — unrelated to either question, found and live-confirmed during the sweep
**A user holding only the delegatable `MANAGE_USER_GROUPS` permission (not full admin) can add themself
to the seeded Admin group and become a full admin.**
`assert_manages_group` → `manages_group` (`backend/onyx/auth/scoped_permissions.py:108-127`)
short-circuits `True` for **any** GLOBAL `MANAGE_USER_GROUPS` holder against **any** `group_id`, including
the seeded Admin default group — no `is_default` guard, no `FULL_ADMIN_PANEL_ACCESS` requirement. This is
the GATE 2 for `POST /admin/user-group/{id}/add-users` (`backend/ee/onyx/server/user_group/api.py:304-323`
→ `add_users_to_user_group`, `backend/ee/onyx/db/user_group.py:561-604`) and for
`PATCH /admin/user-group/{id}` (`api.py:280-301` → `update_user_group`, `user_group.py:696`). Sibling
routes (`rename`, `delete`) *do* carry an explicit `is_default` check (`api.py:236,334`) — this one
doesn't.
**Live-reproduced** on the running deployment: created a non-admin user holding only
`["basic", "manage:user_groups"]`, POSTed `add-users` against the seeded Admin group (id 1) with their own
user id, got **HTTP 200**, confirmed the `user__user_group` row, and their `/me` came back with the full
27-token admin permission set. A user who is only a SCOPED manager (a manager edge, no global token)
correctly got 403 on the identical call — the hole is exclusively in the GLOBAL branch.
The prior review (`08-security-review-must-fix.md`, finding #4) fixed the equivalent hole for *scoped*
managers by excluding default groups from `scoped_group_ids_subquery`
(`backend/onyx/db/scoped_permissions.py:20-34`). That fix never touches `manages_group`'s GLOBAL
short-circuit, so a GLOBAL `manage:user_groups` delegate — someone an admin intended to hand only "Manage
Groups," per the registry description "Add and update user groups" — bypasses it completely.
**Fix direction:** add the same `is_default` guard `manages_group` uses on its SCOPED path to its GLOBAL
path too, or require `FULL_ADMIN_PANEL_ACCESS` (not just `MANAGE_USER_GROUPS`) for any membership/manager
edit that touches a default group.
---
## 4. Other confirmed findings, ranked
**HIGH**
- **EE: every non-admin loses agent creation and deletion of their own agents.** `POST /persona` /
`DELETE /persona/{id}` moved `basic` → `add:agents` (`backend/onyx/server/features/persona/api.py`), but
no migration ever grants `add:agents` — the only two migrations that write `permission_grant` seed
`admin`/`basic` only (`977e834c1427_seed_default_groups.py:22-25`,
`b4b7e1028dfd_grant_basic_to_existing_groups.py:47`). `CE_UNGATED_PERMISSIONS` covers Community Edition
(`backend/onyx/auth/permissions.py:105-109`), but EE has no equivalent — every existing EE deployment's
non-admins lose agent creation on upgrade until an admin manually grants "Create Agents" per group.
- **Community Edition: no remaining mechanism to promote or demote an admin.** `PATCH /manage/set-user-role`
is deleted; the only replacement (adding a user to the Admin group via
`POST /manage/admin/user-group/{id}/add-users`) is registered only by `ee/onyx/main.py` — absent in CE
builds. The lone CE admin is permanently whoever registered first.
- **`MANAGE_LLMS` now implies `READ_USERS`, and `GET /manage/users` has `allow_scope=True` with no GATE 2**
(`backend/onyx/server/manage/users.py:357-371` — the handler calls `get_all_users()` unfiltered and
discards the user as `_`). An "LLM manager" delegate — a much narrower grant than "manage groups" —
gets the entire org user directory plus the pending-invite list as a side effect. Same missing-GATE-2
route was independently flagged by two hunters.
**MEDIUM** (6 more survived, spanning: CE service-account chat-only default (§3.2); the rename-strips-groups
footgun (§3.2); the org-directory leak above stated twice; group managers 403'd on the whole
connector-write surface — file upload, PATCH, DELETE, run-once, OAuth callbacks — despite holding
`MANAGE_CONNECTORS` in scope; credential group-sharing being write-only for a manager (can create, can't
view); `CURATORS_CANNOT_VIEW_OR_EDIT_NON_OWNED_ASSISTANTS` becoming silently inert since it still checks
the deleted `UserRole`; a `stage-metrics` route with a working GATE 2 but missing `allow_scope` at GATE 1
(reachable-but-403 for a manager who should pass); a manager losing the ability to delete a cc-pair or
doc-set shared into their own managed group; `DELETE /admin/credential/{id}` moved from full-admin to
`MANAGE_CONNECTORS` with no per-credential ownership scoping).
**LOW / INFO** (13 more survived): affordance-only anonymous-agent stamping and `/me` under-report (§2);
migration silently demoting `GLOBAL_CURATOR`/`CURATOR` users whose group membership doesn't map cleanly;
an agent owner losing analytics on their own agent (route needs `READ_AGENT_ANALYTICS` with no
`allow_scope`); a connector detail page rendering admin-only buttons to a manager; agent-create UI gating
on the wrong permission set; the four `/onyx-api/ingestion` routes losing curator-level access entirely;
managers losing the ability to create PUBLIC (not just private) resources that CURATOR/GLOBAL_CURATOR
could; a `get_llm_for_persona` call site checking `FULL_ADMIN_PANEL_ACCESS` where every sibling checks
`MANAGE_LLMS`; confirmation that leaving `request.state.token_scopes` unset for API-key auth is
intentional (not a bug).
**16 findings were raised and refuted** on review — mostly claims where a caller-side guard, a different
dependency, or a DB constraint already covered the scenario. Full detail (including refutation reasoning)
is in the audit transcript, not reproduced here to keep this doc load-bearing rather than a changelog.
**2 findings could not be verified** — the verify pass hit a session token limit before reaching them:
"deleted test coverage: no replacement pins the manager's connector-creation flow" and "ordinary users
lose the `/admin/credential` update paths that were `BASIC_ACCESS` on main." Both were raised at
medium/low severity by the hunt pass; re-run if a definitive answer is needed.
---
## 5. Open questions for the author
- Is the CE-only capability loss (agent creation via `add:agents`, PAT creation via
`create:user_api_keys`, admin promotion, service-account scope) intended tightening, or an oversight of
`CE_UNGATED_PERMISSIONS` not covering the full new permission surface?
- Should `manages_group`'s GLOBAL branch require `FULL_ADMIN_PANEL_ACCESS` for default-group edits, matching
the `is_default` guard already present on rename/delete? (§3.3 — recommend fixing before any release
that delegates `MANAGE_USER_GROUPS` to a non-admin.)
- Is CE's forced chat-only service-account tier (§3.2) acceptable, or should CE keep a "full basic" option
the way main's `role=BASIC` default gave it?
---
*Method: route/actor matrix diffed against main via the live FastAPI dependency graph (507 routes, both
branches); live probes against a running deployment (anonymous chat send/receive, groupless
service-account creation and route sweep, the Admin-group escalation); 5 mapping agents → 4 adversarial
hunters (50 raw findings) → per-finding adversarial verification (32 survived, 16 refuted, 2 unverified on
a session cutoff). The Admin-group escalation (§3.3) was independently re-derived and confirmed by hand
against the code after the automated pass, not taken on the sub-agent's word alone.*