Skip to content

Feature/auth improvements - #23

Draft
zorro432 wants to merge 27 commits into
VoidNullable:masterfrom
zorro432:feature/auth-improvements
Draft

Feature/auth improvements#23
zorro432 wants to merge 27 commits into
VoidNullable:masterfrom
zorro432:feature/auth-improvements

Conversation

@zorro432

@zorro432 zorro432 commented Aug 2, 2026

Copy link
Copy Markdown

Summary

This PR reworks how Lific identifies who is acting, and then how an operator sets the instance up. It unifies identity resolution across REST, MCP, and CLI, gives AI agents a first-class identity so they act (and are attributed) as themselves, and turns lific init into a real initial-configuration step where the operator picks an auth mode with the security consequences made explicit.

Three phases, one coherent story:

  1. Unify identity resolution: a single resolve_caller module produces one resolved identity every gate reads. Four redundant carrier mechanisms are deleted.
  2. Model agents as first-class identities: agents carry their own token and resolve as themselves. lific connect sets them up, stdio agents carry LIFIC_TOKEN, and transport is a visible menu choice.
  3. Invite the operator at setup: lific init now asks how you want to sign in (login-free vs passwords), persists the choice, and binds login-free mode to loopback so the security decision is made up front — not by editing a TOML and hoping.

Why

Three problems existed before the identity-unification phase:

  1. "Auth off" mode was half-broken. Project reads passed, but admin endpoints returned 403. The operator signal stopped at the authz seam and did not reach handler-level gates.
  2. Four carrier mechanisms carried the same signal. A task-local, a request extension, an MCP global, and a separate flag all tried to say "this is an operator." They could drift apart.
  3. MCP and REST enforced different gates in legacy mode. MCP stayed wide open; REST kept its historic gates. An agent could do more through MCP than through the web UI.

And the agent-identity gap: a connected agent carried no credential, so every stdio session and CLI run resolved to the first admin. You couldn't tell which agent did what, and a first-admin fallback masked a misconfigured agent as an admin.

And the setup gap: lific init only created a user — it never asked how the operator wanted to sign in. The default config was insecure by default (required=true but bind 0.0.0.0, LAN-reachable), and once login-free mode was on, the only warning was a runtime log line at lific start — too late and the security decision lived in a hand-edited TOML.

Phase 1 — Unify identity resolution

A single module (resolve_caller) decides who the caller is. It returns a ResolvedIdentity { user, transport }. There is always a real user. When a credential carries no user (unbound key, legacy OAuth, auth-off request, stdio session), the first admin supplies the identity.

Every gate now reads identity.user.is_admin from this one type. The operator bypass is no longer a separate signal — it is a property of the resolved identity.

Before After
Option<AuthUser> + 4 carrier mechanisms ResolvedIdentity + 1 field
Auth-off: admin endpoints 403 Auth-off: all endpoints work
MCP gates: legacy no-op short-circuit MCP gates: same authz functions as REST

This phase also:

  • lific init creates the first (passwordless) admin: identity is always known from the moment the instance exists; auth-off becomes passwordless mode, not half-broken anonymous.
  • OAuth approval mints per-tool bots: the approval screen shows a Connected Tools pick-list, the credential is bound to the tool's bot, and Disconnect/Delete revoke the bot's OAuth tokens too. Connected state includes a live OAuth token, not just an API key.
  • Remembers each client's tool choice across reconnects: tool is a stable attribute persisted per OAuth client.
  • Single-user session self-heals: connect is idempotent across stdio/remote/OAuth (a re-run repairs a stale or token-less config in place), and the web single-user auto-login now validates an existing session at load — an expired token is cleared and a fresh session minted instead of trapping the operator.

Phase 2 — Agent identity & connect token carrier

Step What it does
(1) Stable agent dedupe Agents are deduplicated on (owner, tool) instead of the derived username, so renaming the owner never orphans an agent. Adds users.tool_id (migration 037) with legacy backfill; a single find-or-create decision (ensure_bot) is shared across OAuth, web, connect, and import.
(2) Stdio token carrier lific connect --stdio mints a per-tool bot+key and writes it into the client config env as LIFIC_TOKEN (environment for opencode, env for claude-code/codex); the command stays lific --db <path> mcp. The lific mcp stdio entrypoint reads the token and resolves the session as the agent. A missing/invalid token falls back to the operator with a stderr warning — never a hard error.
(3) Transport menu Interactive transport menu (stdio preselected / remote / OAuth). The flag path (--stdio, --oauth, remote default, --url) is unchanged for scripted runs; both funnel through one TransportMode resolver so a menu pick and its flag equivalent can never diverge. The target URL stays a server-config fact, never a connect-time prompt.

Phase 3 — init asks how you want to sign in

lific init becomes an initial-configuration step. On a fresh install (no human operator), it shows an auth-mode menu — Login-free or Passwords — and the operator's choice is persisted to the config file and database, with the admin created in that mode.

  • AuthMode enum: (required, host, web_auto_login, passwordless) bundled into one type so the menu, start, and init can never drift on what a choice means.
  • Login-free: required=false, host=127.0.0.1, web_auto_login=true, a passwordless admin; the shared plain-language caution ("anyone who can reach it can administer it") is shown and confirmed first. Binding to loopback makes the safety check and the actual socket agree.
  • Passwords: required=true, host left unchanged, web_auto_login=false, an admin created with the chosen password.
  • --auth-mode / --password flags are the non-interactive path (a TTY gets the interactive menu) — deterministic for scripts, tests, and JSON.
  • Startup guards now key on the bind, not public_url: login-free mode refuses to start if [server] host is not loopback, so a 0.0.0.0 bind can't silently pass while listening on the network.
  • The config file is edited in place, preserving every other section (via a toml_edit-based merge editor); a missing file gets a fresh config with the chosen values.

This closes the loop with Phase 1: it was never possible to choose an auth mode at setup — now the operator decides explicitly, and the risk of login-free mode is stated in plain language at both init and start.

Behavior Changes

  • Auth-off mode works fully. Admin endpoints, instance settings, and the web UI operate with no password; the first admin is the active identity.
  • Unbound API keys still work. They resolve to the first admin. No existing power-user setup breaks.
  • MCP enforces the same gates as REST. Identical behavior across both transports in legacy and enforced modes.
  • require_admin / require_authenticated read the identity. The operator signal reaches handler-level gates, not only authz.
  • Connected tools are attributed as themselves. OAuth- and stdio-connected agents write to the audit log as the tool, not the operator.
  • Stdio agents resolve as themselves. A connected stdio session is the agent; a no-token session is the operator with a warning.
  • Login-free mode is genuinely local. lific init --auth-mode login-free writes host=127.0.0.1 and the start guard refuses any non-loopback bind in that mode.
  • The security decision is made at setup, not by editing TOML. init asks login-free vs passwords, shows the risk in plain words, and persists the choice.

Commits

Each commit is a safe step; the suite stays green after each one.

Phase 1 — identity unification

  1. Add the resolve_caller module + ResolvedIdentity.
  2. Move all REST gates onto the new type; fix the auth-off bug.
  3. Move all MCP gates onto the new type; delete the legacy mcp_gate no-op.
  4. Delete the four operator carrier mechanisms.
  5. lific init creates the first passwordless admin (+ review fixes).
  6. OAuth per-tool bot minting at approval (+ disconnect/delete token revoke, tool pick-list Option A, widget dedupe).
  7. Remember each client's tool choice across reconnects.

Phase 2 — agent identity
8. Connected state includes a live OAuth token.
9. Stable agent dedupe on (owner, tool).
10. Stdio agents carry LIFIC_TOKEN identity.
11. Interactive transport menu with stdio preselected.
12. Pin stdio session identity at the entrypoint seam (test) + code-review fixes (shared validate_api_key, stdio graceful-degrade, test-isolation).
13. Connect idempotency tests across all transports (reconnect heals stale/token-less configs).
14. Web: single-user auto-login self-heals expired stored sessions.

Phase 3 — auth-mode selection at init (LIFIC-21 epic)
15. Shared plain-language login-free caution (LIFIC-22).
16. Merge-preserving config editor set auth.required + server.host (LIFIC-23).
17. Startup guard checks the bind for login-free mode + rewrites the warning (LIFIC-24).
18. init auth-mode menu with login-free safety (LIFIC-25).
19. Review fixes: dedupe the first-admin insert, guard empty operator password.

Testing

  • Full suite green (1318 tests passing).
  • cargo clippy --all-targets -- -D warnings clean.
  • Follows the repo's CI contract (CONTRIBUTING.md): in-memory SQLite, behavior-named tests, MCP tool tests via Parameters, REST tests via tower::ServiceExt::oneshot.
  • The MCP test suite was the most sensitive to the identity changes; it runs clean across repeated runs.

Expand step of the auth unification (parent LIFIC-7). Adds the
resolve_caller module producing ResolvedIdentity { user, transport } —
a resolved identity with a real user (no anonymous). When a credential
carries no user (unbound key, legacy OAuth, auth-off), the first-admin
passwordless fallback supplies one, consolidating the four previously
scattered first_admin call sites (auto-login, authless-MCP,
comment-create, comment-edit) into a single decision.

The auth middleware now inserts Option<ResolvedIdentity> into the
request extension ALONGSIDE the existing Option<AuthUser>, so nothing
breaks during the migration window — downstream tickets LIFIC-10/11
migrate the gates onto it. A resolve failure is logged and degrades to
None rather than failing the request.

14 new tests (7 module, 7 middleware via identity_echo_app); full suite
green (1243 passing); clippy clean.

Reviewed by /code-review (standards + spec axes). Test names now
describe behavior; the duplicated identity-stamp across five middleware
branches is extracted into insert_resolved_identity, which also logs
(rather than silently swallows) resolve errors at the seam.
…IFIC-10)

All REST gates and handler-level checks migrate from Option<AuthUser> to
ResolvedIdentity. require_admin becomes identity.user.is_admin;
require_authenticated passes in passwordless mode (identity always
resolves). The operator bypass is no longer a separate signal — an
unbound API key now resolves to first_admin via resolve_caller, so
identity.user.is_admin catches it. This fixes the auth-off bug where
/api/instance/settings returned 403 instead of 200.

authz::* gates (require_role, require_structure_role, require_project_delete_role,
require_workspace_admin, visible_project_ids) consume &Option<ResolvedIdentity>.
operator_context() is now unread (marked dead-code-pending; LIFIC-14 deletes it
along with the other carrier mechanisms).

MCP bridge: current_identity() added to mcp/mod.rs, wrapping the legacy
Option<AuthUser> + operator flag into ResolvedIdentity for the 5 require_*_mcp
wrappers. Full MCP migration is LIFIC-11.

21 files changed, 1242 tests passing, clippy clean.
@zorro432
zorro432 marked this pull request as draft August 2, 2026 04:01
zorro432 added 23 commits August 3, 2026 21:19
Fresh installs now create the first human operator at `lific init` time,
so identity is always known from the moment the instance exists — 'auth
off' becomes passwordless mode, not half-broken anonymous. The operator
name is prompted interactively, or supplied non-interactively with
`--name`; existing instances with users skip creation entirely.

`create_passwordless_admin` is the deep seam: it derives a unique
username, fills the NOT NULL email/password columns with an unusable
placeholder hash, and marks the user an admin (not a bot). It is the
passwordless fallback target for resolve_caller and browser auto-login.

The unbound 'default' API key is no longer auto-minted once a human
exists — `should_mint_initial_key` gates both `init` and `start` on
'no humans AND no keys'. Keys remain available on demand via
`lific key create`. This removes the auto-minted operator key shadowing
the new human identity.

Tests: create_passwordless_admin (5), should_mint_initial_key (3),
prompt_text (3), and end-to-end cmd_init fresh/existing install (3).
Full suite green (1253 passing); clippy clean.
… (LIFIC-9)

Split the multi-assertion first-admin test; stop asserting the '-N' dedup
suffix (an implementation detail) and assert distinctness instead.
- drop unnecessary #[allow(dead_code)] from prompt_text (both are used)
- make the start key-mint log branches mutually exclusive so 'passwordless
  mode' only prints when no keys exist, not whenever a human is present
- extract shared unusable_password_hash used by both create_bot_user and
  create_passwordless_admin (removes duplicated random-hash core)
- document the open_memory() exception: cmd_init e2e tests must use a real
  on-disk DB in a temp dir because init writes a config file from a path
At OAuth approval Lific now mints (or reuses) a per-tool bot and binds the
issued credential to it, so the audit log attributes agent requests to the
tool instead of the human operator. The approval screen (auth-code and
device flows) shows a pick-list sourced from the same Connected Tools
registry `lific connect` writes, plus a sanitized free-text field for
unrecognized tools (lowercase, non-alphanumerics collapsed, reserved ids
admin/system rejected).

Deep seams added:
- `users::ensure_bot` — the single find-or-create decision for a
  {tool}-{owner} bot, consolidating the previously duplicated logic in
  `lific connect` and the web `create_bot` onto one query.
- `oauth::resolve_tool` — validate/resolve the form choice (registry id
  or sanitized free text) into (tool_id, display_name).
- `oauth::resolve_approval_bot` — the shared tool-resolution + bot-mint
  step used by both the auth-code and device approval doors, returning a
  small (status, message) error the page renders.
- `oauth::tool_options_html` — shared Connected Tools option rendering.

The bot's owner is the approving human, so authz's existing
bot→owner resolution (`effective_user`) grants it the human's perms, and
the token's user_id (now the bot) flows through token exchange unchanged.

Tests (TDD, red-green): ensure_bot create/reuse/owner-isolation;
resolve_tool registry/sanitize/reserved/empty; end-to-end auth-code and
device approval binding the token to the bot; re-approval reuse;
tool-required and reserved-id rejection; middleware resolving an OAuth
token bound to a bot. Full suite green (1265 passing); clippy clean.
… follow-up)

After LIFIC-13 OAuth-connected tools get a per-tool bot row, so they appear
in the Connected Tools UI. But Disconnect/Delete only touched the bot's API
keys — an OAuth-connected agent has no API key, so its access token stayed
live and the bot kept acting after 'Disconnect'. This closes that gap.

- disconnect_bot now revokes the bot's OAuth tokens too (rows kept, so the
  bot identity remains reconnectable)
- delete_bot now deletes the bot's OAuth token rows (identity gone — no
  dangling rows pointing at a removed user)
- extract the duplicated ownership guard into verify_bot_owner, shared by
  both

Tests (red-green): disconnect revokes an OAuth token but keeps the bot and
its token row (reconnectable); delete removes the token rows outright. Full
suite green (1268 passing); clippy clean.
…LIFIC-13)

Rework the OAuth approval tool pick-list so a dropdown and a free-text input
are never both live at once (interaction-flow discipline). The dropdown lists
the Connected Tools registry plus a single 'Custom tool…' entry; selecting it
reveals a free-text tool-name field via a small inline script.

- tool_options_html() gains the 'Custom tool…' option (value __custom__),
  replacing the old always-visible custom field
- authorize_page and device_page hide the custom field by default, revealed
  when __custom__ is chosen
- resolve_approval_bot treats __custom__ as 'read tool_custom', known ids
  as themselves
- use the ASCII-safe &hellip; entity so the ellipsis can't mojibake under
  a missing charset

Tests: custom free-text choice mints a sanitized bot; the reserved-word
rejection now exercises the real custom path. Suite green (1268); clippy
clean.
Extract the 'Which tool is connecting?' widget (Connected Tools pick-list +
hidden free-text field + reveal script) into a single tool_pick_list_html()
used by both the auth-code and device approval pages. The reveal script now
compares against CUSTOM_TOOL_OPTION interpolated from the Rust constant, so
the '__custom__' sentinel value lives in one place instead of being
hard-coded in the inline JS of both templates.

Addresses code-review: removes the duplicated inline JS widget/template and
the Rust->JS string bridge. Suite green (1268); clippy clean.
…IC-15)

A registered OAuth client is a persistent DCR object; which tool it is is a
stable attribute of it, not something to re-derive per visit. Persist the
tool on the client at first approval and pre-fill the pick-list on reconnect
instead of re-asking.

- migrations/036: add oauth_clients.tool_id
- resolve_approval_bot persists the resolved tool_id onto the client (same
  write conn as ensure_bot; auth-code flow passes client_id, device passes
  None since ephemeral device codes key no persistent client)
- authorize_page reads the client's remembered tool and pre-selects it; a
  free-text tool is pre-selected as Custom and its field revealed+prefilled
- tool_pick_list_html takes a preset tool id; the placeholder is deselected
  when a tool is remembered so the remembered option wins in the browser

Code-reviewed: fixed the placeholder/selected tree-order bug (pre-fill
wouldn't have rendered), the duplicated is-custom predicate, and a dead
display lookup. Tests (TDD): persistence on approve + known/custom pre-fill
on reconnect, asserting the placeholder isn't also selected. Full suite
green (1271); clippy clean.
…low-up)

A connected bot is one with any live credential (an active API key OR a
non-revoked OAuth token), not only an API key. Renames bot_has_active_key ->
bot_is_connected, makes list_bots and the web UI reflect OAuth-connected
tools, and refuses re-connecting a tool that's already connected via either
door.
An agent is deduplicated on the stable (owner_id, tool_id) pair instead of the
derived '{tool}-{owner.username}' username string, so renaming the owner no
longer orphans the agent. Adds a users.tool_id column (migration 037); ensure_bot
looks up by (owner, tool), falls back to a tool-prefix match for pre-migration
bots and backfills them, and stamps tool_id on new bots. Import routes through
the same find-or-reuse decision as OAuth, web, and connect.
A stdio-connected agent now resolves as itself, not the operator.

- connect --stdio mints a per-tool bot+key (as remote does) and writes the
  key into the client config's env field as LIFIC_TOKEN: `environment` for
  opencode, `env` for claude-code and codex. Command stays lific --db mcp.
  The TOML writer gains nested-object (env) support.
- The `lific mcp` stdio entrypoint reads LIFIC_TOKEN at startup, validates
  it as an API key, and binds the resolved agent as the session identity.
  A missing/invalid/unbound token runs as the operator with a stderr
  warning and the session still starts (no hard error).

Acceptance: connect writes token into env; merge-preserving; valid token resolves agent; missing/invalid falls back to operator with warning.
`lific connect` now shows the transport as a visible choice in an
interactive terminal: Local stdio (preselected), Remote (API key), or
OAuth. The flag path (`--stdio`, `--oauth`, the remote default, `--url`)
is unchanged for scripted runs, and both funnel through one TransportMode
resolver so a menu pick and its flag equivalent can never produce
different configs. The target URL stays a server-config fact — never a
connect-time prompt. The resolved transport is echoed in the run output
(str) and reported in JSON (`"transport": ...`).
Covers seam two of the LIFIC-16 spec — the `lific mcp` entrypoint identity
resolution — at the public boundary `set_stdio_user` → `current_identity`:
with a bound agent the session resolves as that agent (not the operator);
without one it falls back to the first admin. Previously this behavior was
only indirectly covered via resolve_stdio_token.
Extracts the API-key authentication logic (checksum, key_id lookup,
pre-migration backfill, hash verify) that was duplicated between the HTTP
require_api_key middleware and the stdio resolver into one shared
validate_api_key, with a typed ApiKeyReject both callers map to their own
error channel. Behaviour is preserved (Db→500, checksum/notfound/hash→401).

Also:
- stdio no longer hard-fails when the agent owner can't be resolved on a
  multi-user box without --user; it degrades to a plain operator config
  (LIFIC-18's documented no-token fallback), restoring the pre-LIFIC-18
  --stdio non-interactive behaviour that LIFIC-19 promised to keep.
- pins the codex TOML merge-into-existing path (token env table lands
  without destroying unrelated config).
- makes MCP tests that mutate the process-wide MCP_REQUEST_USER share the
  existing TEST_MCP_SERIALIZATION_LOCK, and stdio-token tests share one
  ENV_LOCK, removing cross-test races.
Pins that re-running `lific connect --stdio` repairs a pre-existing stdio
entry that lacks LIFIC_TOKEN: it mints a token into the env field, reuses
the same agent bot (no duplication, one active key), and preserves sibling
entries. Also asserts stability across consecutive reruns.
Expands the idempotency pins from stdio to the full transport matrix. A
re-run of `lific connect` over an existing lific entry repairs it in
place and keeps credential state stable:

- remote (API key): stale URL corrected and a fresh minted key written
- OAuth: stale URL corrected and a leftover Authorization header dropped
  (header-less config), with nothing minted in the DB
- stdio: token-less entry gains LIFIC_TOKEN env (committed earlier)

Deltas the earlier duplicate 'one bot / one active key on rerun' claim
(already covered by rerun_rotates_both_tool_keys_without_error).
`hasSession()` only checks localStorage, so an expired token (server
rejects with 401) looked valid on load: it skipped auto-login and left the
user stuck with a dead session after every API call 401'd. On mount the
app now probes /auth/me; on 401 it clears the token, then single-user
mode (web_auto_login) mints a fresh admin session — self-healing instead
of trapping the operator. Non-single-user instances fall through to the
login screen as before.
Add a single login_free_caution() source of truth that the init auth-mode
menu and the start startup warning both consume, so the two surfaces can
never diverge. Rewrites the all-caps 'AUTH IS DISABLED' wording to plain
language naming the risk, the safe condition, and the recovery path.
Add Config::apply_auth_mode which sets [auth] required and optionally
[server] host in an existing config document while preserving every other
section, setting, and comment (via toml_edit). Returns Err on an
unparseable existing file rather than destroying it, mirroring the connect
writers. Fresh/absent input produces a default config with the choice.
…FIC-24)

Login-free mode ([auth] required = false) now refuses to start when
[server] host is not loopback, so the safety check agrees with the actual
listening socket. Previously the guard checked server.public_url (the
advertised address), so a 0.0.0.0 bind with an unset/loopback public_url
passed while still listening on the network.

Replaces the URL-based is_localhost_url with is_localhost_host (bind
semantics), removes the now-dead is_localhost_url, and keeps the startup
warning on the shared plain-language login_free_caution wording.
lific init now asks the operator to choose an auth mode on a fresh
install (no human operator), persisting the choice to config + database
and creating the first admin in that mode.

- Add config::AuthMode (LoginFree | Passwords) bundling every consequence:
  [auth] required, [server] host, web_auto_login, passwordless admin.
- Add --auth-mode <login-free|passwords> and --password to init; a TTY
  gets an interactive Select menu (login-free shows the shared caution +
  confirmation first).
- Login-free writes required=false, host=127.0.0.1, web_auto_login=true,
  a passwordless admin. Passwords writes required=true, leaves host
  unchanged, web_auto_login=false, an admin with the chosen password.
- Add db::queries::users::create_first_admin_with_password.
- Closes the spec gap: login-free now actually binds loopback, making the
  LIFIC-24 guard and fresh config mutually consistent.
Extract a shared insert_first_admin used by both create_passwordless_admin
and create_first_admin_with_password, so username/email derivation and the
constraint handling live in one place (DRY). Reject an empty operator
password in create_first_admin_with_password. Fold cmd_init's duplicated
db-override reload into load_config_for_init (LIFIC-25).

@Void-n-Null Void-n-Null left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First: this is excellent work. The ResolvedIdentity unification is the right architecture, the commit discipline made this reviewable, and the test coverage claim holds up: I reproduced 1305 passing tests and clean clippy locally. I also audited every REST gate swap (all ~96 call sites are semantically equivalent for bound users, nothing lost a gate) and the MCP parity story (no existing setup loses capability). The shape of this is going in.

Requesting changes because the review turned up one critical and two high findings, and they all trace to a single design decision: the first-admin fallback applies to every unresolved identity, including credentials that named a user that no longer resolves. That conflates two different situations.

The fix that resolves most of this: a credential that carries a user binding which fails to resolve must be rejected (401), never fall back. The fallback should apply only to credentials that never carried a user at all (auth-off, pre-binding API keys, token-less stdio).

Critical: deleted bot resurrects as first admin

delete_bot (src/db/queries/users.rs) deletes API keys and oauth_tokens but not oauth_codes or oauth_device_codes. An unconsumed authorization code for the deleted bot still exchanges into a token whose user_id points at the deleted row. The auth middleware's get_user_by_id(...).ok() (src/auth.rs, OAuth branch) turns the failed lookup into None, and resolve_caller promotes None to the first admin. Net effect: deleting a bot can convert its leftover grant into an admin credential.

Fix: shred pending codes/device grants in delete_bot, and (per the rule above) treat a token with a non-NULL user_id that doesn't resolve as an auth failure rather than an unbound credential.

High: disconnect leaves outstanding grants live

disconnect_bot revokes current API keys and OAuth tokens, but authorization codes and approved device codes issued before the disconnect can still mint fresh tokens afterward. Revoke those in the same transaction.

High: legacy unbound OAuth tokens are silently promoted to admin

On master, an OAuth token with no user_id resolves to None and is denied at authenticated/admin gates. On this branch it becomes the first admin. I understand this is intentional and consistent with the unbound-API-key story, but it's a strict loosening for a credential class that previously could not pass those gates, on instances exposed to the internet. At minimum this deserves a callout in the PR description and changelog; my preference is to exclude legacy OAuth from the fallback and let those tokens re-approve.

Medium

  1. Backfill prefix collision: the legacy dedupe uses GLOB '{tool}-*', so a tool named claude can claim bots belonging to claude-code. Backfill needs an unambiguous mapping.
  2. Bots can approve OAuth: a bot's token can drive the approval flow and become the owner of nested bots, which breaks the "approving human" semantics. Resolve to the effective human owner or reject bot approvers.
  3. Migration 037 invariant not enforced: the (owner_id, tool_id) dedupe has no unique index backing it. A partial unique index on bot rows would make the invariant a database fact instead of a code convention.

Minor

A few MCP resource mutations are stricter in legacy mode than enforced mode (Lead vs Maintainer for module/label/folder mutations, and project update). Harmless today since fallback identities are admin, but the asymmetry reads inverted; worth normalizing or a comment explaining why.

Happy to re-review quickly once these land. The identity model itself needs no rework; every finding above is fixable inside the seams this PR already built, which is a good sign for the design.


This review was performed by an AI agent on Blake's behalf: full local CI run, a mechanical audit of every gate call site, and a line-by-line pass over the auth core, with the critical finding's escalation chain traced and verified by hand.

@Void-n-Null

Copy link
Copy Markdown
Collaborator

Follow-up to the review above, reorganized so it's easier to act on. Noted the five new commits (auth-mode menu, login-free guards, config editor); they don't touch any of the finding sites, so everything below is still open. Split into two audiences.

For the human

The review has one load-bearing disagreement and everything else is mechanical. To save you reading time:

The one decision: the PR treats "credential that never carried a user" and "credential that named a user which no longer resolves" as the same case, and both fall back to the first admin. The first case is a deliberate compat choice and the PR description owns it clearly. The second case is where all the security weight sits: it's how a deleted bot's leftover authorization code becomes an admin credential.

The proposal: split them. Absent binding falls back (auth-off, pre-binding keys, token-less stdio all keep working, nothing you promised breaks). Present-but-unresolvable binding rejects with a 401. That single rule resolves the critical finding and the legacy-OAuth promotion without touching any compat story. Going further than that (failing loud on absent credentials too) is real breakage and belongs behind a major version with a migration path, not in this PR.

Everything else in the original review (grant revocation on disconnect/delete, the backfill GLOB, bot approvers, the 037 index) is uncontroversial fix work, specced below.

On scope, briefly: every finding maps to a commit in this branch. The critical one is about adce27f (revocation completeness) interacting with the fallback introduced in the first four commits. Where findings mention older machinery (oauth_codes is from migration 004), that machinery is the path, not the subject; the behavior change is this PR's.

For the implementation agent

Five work items, ordered by severity. The repo's test conventions apply (in-memory SQLite via crate::db::open_memory(), behavior-named tests). Done means cargo clippy --all-targets -- -D warnings and cargo test --all-targets both pass.

1. Reject bound-but-unresolvable credentials (critical).

  • In the auth middleware OAuth branch (src/auth.rs, lific_at_ path): when oauth_token_user_id returns Some(uid) but get_user_by_id fails, return 401. Do not pass None onward to the fallback. Same rule anywhere else a credential carries a non-NULL user binding (API keys bound to a user, session tokens).
  • resolve_caller itself needs no change; the contract becomes: credential_user = None means "credential never named a user", never "lookup failed".
  • Tests: (a) token whose bound user was deleted gets 401, not first-admin identity; (b) legacy token with NULL user_id still falls back (compat preserved); (c) same pair through the MCP path.

2. Shred outstanding grants on disconnect and delete (high).

  • disconnect_bot (src/db/queries/users.rs): also expire/delete rows in oauth_codes and oauth_device_codes whose user_id is the bot, in the same transaction as the token revocation.
  • delete_bot: same, plus these deletions must precede the user row deletion.
  • Tests: unconsumed auth code for a disconnected bot cannot be exchanged; same for device codes; same after delete.

3. Exact-match backfill (medium).

  • Migration 037 backfill and any runtime legacy-claim path: replace GLOB '{tool}-*' matching with an exact mapping (username = '{owner}-{tool}' or equivalent), so tool claude cannot claim claude-code bots.
  • Test: two tools where one's name prefixes the other's; each claims only its own bots.

4. Human-only OAuth approval (medium).

  • In the approval flow (src/oauth.rs): if the approving identity is a bot, either reject the approval or resolve ownership to the bot's owner before minting. Pick one and state it in the code comment; a bot must never become owner_id of another bot.
  • Test: approval driven by a bot token does not create a bot owned by a bot.

5. Enforce the dedupe invariant (medium).

  • New migration: partial unique index on users(owner_id, tool_id) WHERE is_bot = 1 AND tool_id IS NOT NULL. If existing duplicates would violate it, the migration must dedupe first (keep the row with live credentials, else newest).
  • Test: ensure_bot racing two inserts for the same (owner, tool) yields one row.

The minor legacy/enforced asymmetry in MCP resource gates can be a comment explaining the intent, or a normalization; reviewer has no strong preference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants