fix(auth): revoke privileges immediately on role change, deactivation, or deletion - #9360
Open
lstein wants to merge 22 commits into
Open
fix(auth): revoke privileges immediately on role change, deactivation, or deletion#9360lstein wants to merge 22 commits into
lstein wants to merge 22 commits into
Conversation
github-actions
Bot
added
api
python
lstein
force-pushed
the
fix/multiuser-privilege-revocation
branch
from
July 20, 2026 00:14
960d607 to
efae7b4
Compare
lstein
changed the title
lstein
force-pushed
the
fix/multiuser-privilege-revocation
branch
from
July 31, 2026 02:01
7ff3da0 to
90e1f03
Compare
lstein
marked this pull request as ready for review
July 31, 2026 02:01
lstein
requested review from
JPPhoto,
Pfannkuchensack,
blessedcoolant and
dunkeroni
as code owners
July 31, 2026 02:01
lstein
changed the title
lstein
force-pushed
the
fix/multiuser-privilege-revocation
branch
from
July 31, 2026 02:18
90e1f03 to
0c95caa
Compare
…, or deletion Three related gaps let stale credentials outlive database changes in multiuser mode: 1. JWT privileges survived demotion. Authentication verified the user exists and is active but kept trusting the token's is_admin claim, and the sliding-window middleware re-minted new tokens (and the media cookie) from those stale claims — so a demoted administrator kept admin rights indefinitely as long as they kept making requests. All auth dependencies now derive authorization fields from the database record on every request (the token proves identity only), and the middleware refuses to refresh for missing/inactive users and mints refreshed tokens from the database record. A promoted user symmetrically gains admin rights on their next request without re-login. 2. Open sockets retained revoked privileges. Socket room membership was established once at connect from the token's claims. Connect now derives is_admin from the database, and a new server-internal user_access_changed event (emitted by the user-management routes) re-authorizes live sockets: demotion leaves the admin room, promotion joins it, deactivation/deletion disconnects the user's sockets. 3. Deactivated users' queued work kept executing. The session processor now rejects (cancels) dequeued items whose owner is inactive, stops running sessions at the next node boundary, and cancels the current item immediately when its owner is deactivated (which also stops step-callback nodes mid-node via the existing cancel-event machinery). Invocation-context media reads and saves also require an active account. Single-user mode and the "system" user are exempt. Tests cover: demoted-admin 403 with no token/cookie refresh, DB-derived refresh claims, promoted-user semantics, socket reconnect-with-old-token, live socket demotion/promotion/deactivation, dequeue rejection, multi-node mid-session deactivation, and invocation-context read/save denial for inactive accounts, plus positive cases for unchanged admins, active users, and single-user mode. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…dmin test stubs Two CI failures introduced by the privilege-revocation commit: - `UserAccessChangedEvent` is dispatched only between server components, but `EventBase.get_events()` sweeps in every subclass carrying `__event_name__`, and its sole consumer is the OpenAPI generator. The event therefore leaked into `openapi.json`/`schema.ts`, failing openapi-checks and typegen-checks and contradicting the event's own documented contract. Events can now opt out with `__server_internal__ = True`. - The pre-existing "rejects non-admin users" tests in `test_app_info.py` stubbed the user lookup with a bare `Mock(is_active=True)`. Authorization is now derived from the database record on every request, so `TokenData` validation rejected the Mock-valued fields. The stub now carries concrete values. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A JWT is self-contained: nothing in the database can make an already-issued token stop verifying. Authorization fields are re-derived from the user record on every request, so demotion and deactivation take effect immediately — but a password change had no way to invalidate anything. A stolen token therefore outlived the password rotation meant to evict the thief, and because the sliding window renews on every mutating request, it survived indefinitely rather than expiring after a day. Adds `users.token_epoch`, stamped into every minted token and compared against the record on each authenticated request. Rotating the password bumps it in SQL (`token_epoch = token_epoch + 1`, so concurrent bumps can't lose each other), invalidating every token issued before the change. This is the general "revoke everything issued so far" primitive the auth layer was missing. Details worth noting: - The sliding-window middleware also checks the epoch. It runs after the route and refreshes on any 2xx mutation, so an unauthenticated route carrying a stale bearer header would otherwise launder a revoked token into a valid one. - Changing your own password signs out your *other* sessions, not the tab you did it from: the route mints a replacement and returns it in X-Refreshed-Token. The middleware can't do this — it correctly refuses to refresh a stale-epoch token, and leaves an already-set header alone. - Rejection is reported as an ordinary invalid/expired token, so a stolen token's holder isn't told the password was just rotated. - Existing rows and existing tokens both start at 0, so upgrading logs nobody out; only a real bump revokes. Tests cover cross-session revocation, the calling session surviving, admin password reset revoking the target, the refresh-laundering path, non-password updates not revoking, and pre-existing tokens staying valid.
…ecks `queue_owner_is_active` exempted `user_id == "system"` on the stated grounds that the system user "has no database record". That premise is wrong: migration_27 creates a real, active `system` row that owns every board, image, and workflow carried over from before multiuser support. Because the row exists and is active, the exemption changed nothing in normal operation — it only took effect when the row was missing or inactive, which is exactly the case where this gate then disagreed with the save gates in `invocation_context`, which have no such exemption. A system-owned item would pass the gate that decides whether to spend GPU time, load models and denoise, then fail at the first `context.images.save()`. That is the worst possible ordering for two checks that disagree. Drops the exemption so all three checks agree: the system user now passes on its own merits, and if its row is ever gone the item is rejected at dequeue instead of after generating. Also protects the row, since orphaning it is what made the disagreement reachable in the first place. Neither `delete_user` nor `update_user` guarded it — `list_users` merely hides it from the UI, and the last-admin guard does not apply because the system row is deliberately not an admin. Deleting or deactivating it is now rejected; it is not a login account, so there is no legitimate reason to do either.
lstein
force-pushed
the
fix/multiuser-privilege-revocation
branch
from
July 31, 2026 15:13
0c95caa to
d46d3b3
Compare
This was referenced Aug 8, 2026
…transaction
Removing the last active administrator is irreversible from inside the app: authorization
is derived from the database on every request, so no authenticated path back exists. It
also drops `has_admin()` to zero, which makes `GET /auth/status` report `setup_required:
true` and re-opens the *unauthenticated* `POST /auth/setup` to any caller.
The invariant was enforced in one place only — the `delete_user` route — and enforced
there by reading `count_admins()` in its own transaction before writing in another. That
left three gaps:
- `update_user` had no guard at all, so `PATCH /auth/users/{id}` with `{"is_admin": false}`
or `{"is_active": false}` against the sole administrator succeeded, single-threaded.
- The read and the write were separate transactions, so two callers could each observe two
administrators and each remove one. Route handlers now run in a threadpool, which makes
that reachable from two concurrent requests rather than only across processes.
- `invoke-usermod` / `invoke-userdel` construct `UserService` directly and never reach the
route guard, so the CLI could take the instance to zero on its own.
Moves the check into `UserService.update()` / `UserService.delete()`, evaluated on the
cursor of the transaction that performs the write. Those transactions now open with
`BEGIN IMMEDIATE` so the count is read under the write lock: without it the SELECT runs in
autocommit and a second process can still interleave. In-process callers are additionally
serialized by the database's shared RLock.
The guard keys on the *requested* values, so renaming or changing the password of the last
administrator stays allowed, as does removing an administrator who is already inactive —
they are not counted, so removing them cannot reach zero.
`LastAdministratorError` subclasses `ValueError`, which both the routes and the CLIs
already map to a friendly 400 / error message, so no call site needed changing. The
existing route-level check in `delete_user` stays as the friendly-message path; the
service is the backstop.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ation `_handle_user_access_changed` snapshots the affected sids, then re-indexes `self._socket_users[sid]` on later iterations. `AsyncServer.disconnect()` flushes a packet — it yields to the event loop, and then runs the socket's own disconnect handler, which deletes that entry. Any socket that goes away during the yield (its client dropping, engine.io's ping-timeout reaper, or a second access-changed event for the same user) makes the next iteration raise `KeyError`, and every remaining socket in the loop is left holding exactly the privileges this handler exists to revoke. On the demotion path the abandoned socket also stays in the admin room and keeps a cached `is_admin` of True, which `_handle_sub_queue` uses to re-add it on the next subscription. Nothing observes the failure: the dispatcher runs handlers as bare tasks, so the exception surfaces only as an unretrieved-task warning at GC. Look the entry up once per iteration and skip it if it is gone. The existing tests could not catch this — they replace `disconnect` with an `AsyncMock` that neither yields nor mutates `_socket_users`, so they proved the loop iterates the right sids, not that it survives the removal its own call causes. The two new tests use a fake that does what the real one does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ng item `_on_user_access_changed` collected the matching item ids and handed them to a threadpool, which then cancelled them unconditionally. Every other gate in this change re-reads the database at the point of decision — `queue_owner_is_active` at dequeue, and again between nodes — but this one acted on the event's snapshot. Each event is dispatched as its own task, so a deactivate immediately followed by a reactivate can leave the first handler still parked in the threadpool while the second has come and gone (it returns early on `is_active`). The parked handler then kills a running item of an account the database says is active, and nothing undoes a cancellation. Re-check with `queue_owner_is_active`, which is the same gate the other two use, including its fail-to-active policy for a failed lookup: skipping the cancel is safe because the between-node gate re-checks at the very next node, while cancelling a live job on a transient SQLite error is not recoverable. It also short-circuits in single-user mode, where nothing else in this change enforces ownership either. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`test_cuda_device_pin_is_deferred_until_first_claim_and_runs_once` arrived on this branch with the merge from main, after the post-dequeue owner gate was written and after the sibling scenario helper had been given a `configuration` stub. It builds its own `_invoker` SimpleNamespace, which has no `configuration`, so `queue_owner_is_active` raises AttributeError on its first line. The failure is quiet in an unhelpful way: the exception is swallowed by a broad handler in `_process`, so the item is silently dropped rather than treated as active, and the test fails on a missing "run:1" event rather than on the error. Single-user mode, matching the stub the guard-scenario helper already uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The last-admin tests hand-build the `users` table rather than running migrations, mirroring what `test_user_service.py` does. This branch adds a `token_epoch` column and selects it in `get`, `get_by_email`, `authenticate`, and `list_users`, so every test in the file failed with `no such column: token_epoch` once the two changes met. Same column definition the migration adds, and the same fix already applied to `test_user_service.py`'s fixture. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n guard
Both layers of the last-admin guard — the route pre-check and the service backstop
inside the write transaction — enforce "at least one row with is_admin AND
is_active". The property that actually matters is "at least one administrator who
can log in", and the `system` row satisfies the first without the second: it is
active, but migration_27 gives it an empty password hash, so `verify_password`
always fails.
Nothing stopped an admin from promoting it, and neither guard objects to a
promotion — it is not a change that revokes anything:
PATCH /auth/users/system {"is_admin": true} -> 200, count_admins() 1 -> 2
PATCH /auth/users/{admin} {"is_admin": false} -> 200, both guards see 2
POST /auth/login system@system.invokeai -> 401, empty password hash
Zero usable administrators, `has_admin()` still true so `/auth/setup` stays closed,
and no authenticated path back — precisely the outcome the guard exists to prevent.
Setting a password on the system row was the same hole from the other end: it turns
the owner of every pre-multiuser board, image, workflow, and queue item into a
login account.
The system-user protection also lived only in the routes, so `invoke-userdel
--email system@system.invokeai --force` deleted it and reported success, and
`UserService.update` would deactivate it — the CLIs construct the service directly.
That is the same reasoning that moved the last-admin invariant into the service.
So: `SYSTEM_USER_ID` and the two refusal messages move to `users_common` beside
their errors, and `UserService.update`/`delete` refuse to delete, deactivate,
promote, or set a password on that row. Renaming it stays allowed — this is not a
blanket lock. `SystemUserProtectedError` subclasses `ValueError`, so the routes and
both CLIs map it to a friendly message with no call-site changes, exactly like
`LastAdministratorError`.
Two route contract fixes found in the same pass:
- `update_user` returned 400 for an unknown user id, contradicting its own
docstring and both sibling routes, by falling through to the service's
`ValueError("User ... not found")`. It already reads the record; it now 404s.
- The delete route said "Cannot delete the last administrator" while the service
said "Cannot remove the last administrator", so losing the race between the
pre-check and the backstop changed the wording of the same refusal on the same
endpoint. Both now use `LAST_ADMIN_DETAIL`.
- An admin who deactivates themselves in the same request as a password change no
longer receives a replacement token and media cookie for the account they just
disabled.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… re-read fails The re-read added in the previous commit borrowed `queue_owner_is_active`, and with it its fail-to-active policy: a lookup that raises is treated as "still active". That policy is right for the two gates it was written for, which re-run at the next node boundary — but it is wrong here. This handler is the only thing that stops a *single-node* graph. `_run_session_loop` checks the owner before `run_node` and then only tests `session.is_complete()`, so a one-node session is checked exactly once, before it starts. Skipping the cancel on a busy-timeout therefore lets a deactivated account's node run to completion: `worker.cancel_event` is deliberately not set here, so nothing else interrupts it. The event is itself evidence of a committed deactivation. When the re-read cannot contradict it — as opposed to actively reporting the account active again — the event stands. A successful read showing an active account still spares the item, which is the reactivation race the re-read exists for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The service guard blocks future promotions but does nothing about a database where
`PATCH /auth/users/system {"is_admin": true}` already succeeded — which shipped builds
allow. That state is self-perpetuating: `count_admins()` is inflated by one forever, so
the last-administrator guard stays willing to demote the last real administrator, and
`has_admin()` stays true afterwards, which keeps `/auth/setup` closed. There is then no
authenticated way back, since the system row cannot log in.
A dated migration clears `is_admin` on that row. It is a no-op on a healthy database
(the row is seeded non-admin), restores the count's meaning on a poisoned one, and can
reopen `/auth/setup` for an instance already locked out. Nobody is signed out: no token
can carry `user_id="system"`, because authenticating as it is impossible.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The deactivation branch went straight to `disconnect(sid); continue` without indexing `_socket_users`, so a mid-loop removal never raised there — only the active branch (the epoch compare and the `is_admin` write) could. Verified against the pre-fix loop: the deactivation scenario raised nothing and still reached the last socket, failing only on the redundant second `disconnect`. `test_demotion_survives_a_socket_dropping_mid_loop` is the test that reproduces the KeyError. Rename and reword its neighbour so nobody reads it as covering that too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FastAPI publishes route docstrings as the operation description, so widening `update_user`'s documented 400 to mention the protected system account changes both generated artifacts. openapi-checks and typegen-checks diff them against the committed copies. Regenerated with the frontend's locked deps and CI's own commands; the only delta in either file is that sentence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JPPhoto
requested changes
Aug 9, 2026
JPPhoto
left a comment
JPPhoto
left a comment
Collaborator
There was a problem hiding this comment.
I took a look at this PR and the comments made in #9479 to prepare this.
Merge blockers:
-
invokeai/app/services/shared/sqlite_migrator/migrations/migration_2026_08_08_demote_system_user.py:26-28only demotessystem; it does not clear an existing password.auth.py:220-243still permits that account to log in and access legacy assets. #9479 explicitly identified this password hole. Test: seedsystemwith a bcrypt hash, run migration, assert login fails. -
invokeai/app/util/user_management.py:218-248,492-527mutates users directly, withoutemit_user_access_changed. Existing sockets therefore retain admin/user-room access after CLI demotion, deletion, or password reset; this contradicts the immediate-revocation claim. Test: connect a socket, run CLI mutation, emit a private/admin event, assert disconnect/no delivery. -
invokeai/app/services/session_processor/session_processor_default.py:47-82,733-745treats every owner lookup exception as “active.” A deleted/deactivated user can therefore run queued work during a DB/read failure, contradicting the “stop before the next node” policy. Test: makeusers.get()raise after deactivation; assert cancellation and zero node execution.
Other findings/issues:
docs/src/content/docs/features/Multi-User Mode/admin-guide.mdx:285-294still describes tokens as expiry-only. It omits DB-derived role revocation, password epoch invalidation, socket disconnection, and queued-work cancellation. Test: reconcile this section with the new auth and password-change behavior.
Alternative implementation ideas:
-
Instead of only demoting
system, clear its password hash in the migration and reject system authentication at the service boundary; this makes the internal-account invariant hold for damaged legacy rows too. -
Instead of route-only invalidation events, use a shared mutation service plus DB-backed socket revalidation; this covers CLI and future direct-service callers.
-
Instead of treating lookup failures as authorization success, fail closed with a retryable queue state; this prevents unknown DB state from becoming executable work.
-
Instead of documenting JWT expiry alone, document epoch and live-revocation semantics; this prevents unsafe operator assumptions.
The system row owns everything carried over from before multiuser support. It is seeded unable to authenticate, but an administrator could once give it a password through `PATCH /auth/users/system`. Refusing that from now on repairs nothing that already happened, and the hole has three separate ends: - the row itself, which may still carry a usable hash under a fixed, public email — the migration now clears `password_hash` alongside `is_admin`; - a row damaged *after* the migration, by direct SQL or on a database that applied an earlier revision of the same migration id, since migrations run once — `UserService.authenticate` now refuses the account outright, whatever the row holds; - a token *already issued*, which the migration cannot reach at all. The row is deliberately left active and its epoch untouched, so nothing else rejected it and sliding-window refresh would renew it forever — `resolve_authorized_user` now refuses the id, which covers REST, media, sockets and the video-upload gate in one place. Single-user mode, where everything legitimately runs as `system`, never reaches `resolve_authorized_user`: its dependencies synthesize the TokenData and return first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t be read `queue_owner_is_active` treated any lookup exception as "active", which makes unknown database state executable: the account may have been deactivated a moment earlier, and this gate is what stands between that and GPU time spent on its behalf. It now retries the read before refusing, so a transient error — a busy-timeout on the shared SQLite connection under multi-GPU write contention, say — does not cost a valid user their queued work. Only a database that is unreadable across every attempt refuses the item, and that costs a cancellation, which is retryable. Both call sites run on a worker thread, so the wait between attempts blocks nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`invoke-usermod` and `invoke-userdel` write to the database from their own process, so no in-process event can be raised for them. Everything the server derives per request — REST authorization, the dequeue and between-node gates, the media read and save gates — already changes the instant those commands commit. Socket room membership does not: it is established at connect time and refreshed only by `user_access_changed`, so a demoted administrator's socket sat in the admin room, receiving other users' private events, until it happened to reconnect. A periodic sweep now re-derives each connected user from the database and publishes any difference as the same event the routes emit, so sockets re-authorize and the session processor cancels the user's running items through one code path rather than a second copy that drifts from the first. Staleness is judged against every socket of the user, not a representative one — a session that reconnected after a password change holds the current epoch while the superseded session is still connected under the old one. `_handle_user_access_changed` now re-reads the record and applies that, treating the event as a trigger, the same way `_on_user_access_changed` already does. Handlers are dispatched as independent tasks and the sweep's payload is a snapshot taken before an await, so an event can arrive already superseded: applying it would re-grant the admin room to someone just demoted, or disconnect the replacement session a password change had just issued. A read that fails leaves the event standing. The sweep is started and stopped from the app lifespan, in a `finally` so an abnormal exit cannot leave it running against a half-torn-down process. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Session Management section described tokens as expiry-only, which invites the operator assumption that a demotion or deactivation does not take effect until the target's token runs out. Replace it with what actually happens: role changes derived from the database per request, epoch invalidation on password change, socket disconnection, queued-work cancellation, and the bounded staleness of a change made with the CLIs. Both limits are stated plainly too — the token stays cryptographically valid until it expires, and a session in flight is stopped, not rewound. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Thanks — all three blockers were real. Fixed at 8a3d38d, along with the doc gap. 1. The
|
…d_user The middleware carried its own copy of the exists/active/epoch checks, which is exactly how a rule added later reaches every entry point but this one: the refusal of the internal `system` id landed in `resolve_authorized_user`, so REST, media, the socket handshake and the video-upload gate all stopped honoring those tokens while this kept minting fresh ones for them. The `system` row is deliberately active with an untouched epoch, so none of the local checks fired. Nothing accepted the renewed token, so no access followed from it — but an indefinitely renewed session is a hole waiting for one consumer that trusts a token without re-checking the id. Deciding in one place is the point of that function. The lookup still runs off the event loop, for the reason the old comment gave. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Follow-up at 49d00b3, on my own point (1) above. I said No access followed from it — nothing accepts the renewed token — but that is precisely the shape the function's own docstring warns about ("a check added to some copies but not others is indistinguishable from no check at all on the paths that were missed"), and an indefinitely renewed session is a hole waiting for one consumer that trusts a token without re-checking the id. The middleware now decides through |
JPPhoto
requested changes
Aug 10, 2026
JPPhoto
left a comment
JPPhoto
left a comment
Collaborator
There was a problem hiding this comment.
Merge blockers:
-
invokeai/app/api/sockets.py:312-317,339-370preserves cached privileges when DB reads fail. After CLI demotion, a stale admin socket can keep receiving private events.Test:commit demotion, forceusers.getto fail across sweeps, emit another user's event, assert the socket receives nothing. -
invokeai/app/api/sockets.py:393-418,441-447applies stale event authorization when revalidation fails. A superseded promotion event can re-add a demoted user toadmin.Test:send promotion event after demotion, make the reread raise, assertenter_room(..., "admin")is never called.
Other findings/issues:
-
invokeai/app/api/sockets.py:319-324andinvokeai/app/services/session_processor/session_processor_default.py:163-178leave a no-socket user's already-running single-node graph executing after CLI deletion; only save gates stop persistence. Docs atdocs/src/content/docs/features/Multi-User Mode/admin-guide.mdx:298-303overstate cancellation.Test:run a side-effecting one-node item, delete the owner with no socket, assert the node is not invoked. -
invokeai/app/services/users/users_default.py:100-133omitstoken_epochfromget_many(), so users whose passwords were changed receive DTOs reporting epoch0.Test:bump an account epoch, callget_many([user_id]), assert the returned epoch matchesget().
Suggestions:
-
Instead of preserving socket authorization on repeated DB errors, disconnect or remove privileged room membership after bounded failures; this fails closed and limits stale access.
-
Instead of sweeping only users with sockets, track authorization changes in the database or periodically revalidate every running queue owner; this closes the no-socket single-node gap.
-
Try using one shared row mapper/projection instead of manually maintaining each user
SELECT; this prevents future fields such astoken_epochfrom silently becoming stale.
Pfannkuchensack
added a commit
to Pfannkuchensack/InvokeAI
that referenced
this pull request
Aug 11, 2026
Converting the routes from `async def` to `def` removed an implicit guarantee: a handler whose body contains no `await` could not be interleaved with another request, because the event loop had no point at which to switch. Two handlers relied on it. `POST /auth/setup` did has_admin() then create_admin() in separate transactions. Two concurrent requests both saw no admin and both created one, so the loser ended up with a persistent admin account instead of the intended 400. The condition now lives inside the INSERT's own transaction, behind BEGIN IMMEDIATE so a second process (invoke-useradd --admin) cannot slip a write in either. This mirrors what invoke-ai#9360 does for the update/delete last-admin invariant; create_admin was the one path it does not cover. Custom node install, uninstall and reload all mutate the same custom-nodes directory, sys.modules and invocation registry. Interleaved, a failed install's cleanup rmtree'd the directory a concurrent install had just cloned into. A module-level lock restores the exclusion; the install and uninstall bodies moved into helpers so the lock scope is visible rather than an 80-line reindent. Both regression tests fail without their fix: the admin one creates two administrators, the pack one loses the successful install's directory. perf(queue): render the queue list from summaries, with one sanitizer The list fetched full queue items for every visible row, each carrying its session graph and workflow — megabytes per screenful for fields no row draws. The rows now render from SessionQueueItemSummary and the full item is fetched only when a row is expanded, which is what the summary route added in this branch was for; until now nothing consumed it. The per-item summary query provides the same cache tags as getQueueItem, so every existing invalidation path covers the list rows with nothing to wire up, and the optimistic status write is mirrored so a row's status still flips without a round trip. The range hook batches at the backend's 1000-id limit, which a fast fling could otherwise exceed. Both sanitizers are now one generic function over a single redaction table: the summary and the full item are two projections of one row, and a field stripped from the list but left on the detail view is leaked anyway. A test walks the intersection of both models and asserts they redact it identically. `device` is deliberately not redacted in either. It names the instance's GPU rather than anything about the other user's work, and the list has always shown it — redacting it here would have quietly changed what non-admins see. parent_item_id joins the summary because the rows decide from it whether to offer a retry.
Pfannkuchensack
added a commit
to Pfannkuchensack/InvokeAI
that referenced
this pull request
Aug 11, 2026
Two review follow-ups landed together here. Converting the routes from `async def` to `def` removed an implicit guarantee: a handler whose body contains no `await` could not be interleaved with another request, because the event loop had no point at which to switch. `POST /auth/setup` did has_admin() then create_admin() in separate transactions. Two concurrent requests both saw no admin and both created one, so the loser ended up with a persistent admin account instead of the intended 400. The condition now lives inside the INSERT's own transaction, behind BEGIN IMMEDIATE so a second process (invoke-useradd --admin) cannot slip a write in either. This mirrors what invoke-ai#9360 does for the update/delete last-admin invariant; create_admin is the one path it does not cover. Custom node install, uninstall and reload all mutate the same custom-nodes directory, sys.modules and invocation registry. Interleaved, a failed install's cleanup rmtree'd the directory a concurrent install had just cloned into. A module-level lock restores the exclusion; the install and uninstall bodies moved into helpers so the lock scope is visible rather than an 80-line reindent. Both regression tests fail without their fix: the admin one creates two administrators, the pack one loses the successful install's directory. The route added earlier in this branch had none — the list still fetched full queue items for every visible row, each carrying its session graph and workflow, so the claimed saving was not being realised. The rows now render from SessionQueueItemSummary and the full item is fetched only when a row is expanded. Measured against the previous commit, same backend and same 396-item queue, identical scroll (page load, queue tab, scroll to 60%): requests 62 -> 2 payload (gzip) 262 KB -> 1.3 KB (30 items) server time 60ms -> 4ms (30 items) The request count collapses because the old path was self-amplifying: the range hook re-asks which ids are uncached on every range event, and at ~60ms per response the cache had not filled yet, so overlapping fetches piled up. A side effect worth knowing: `items_by_ids` silently skips items it cannot deserialize, so a queue item whose graph references an unregistered node type left its row permanently blank. Summaries never touch the graph, so the row now renders and only the expanded detail is affected. The per-item summary query provides the same cache tags as getQueueItem, so every existing invalidation path covers the list rows with nothing to wire up; the optimistic status write is mirrored so a row still flips without a round trip. The range hook batches at the backend's 1000-id limit, which a fast fling could otherwise exceed. The summary and the full item are two projections of one row, and a field stripped from the list but left on the detail view is leaked anyway. Both now go through one generic function over a single redaction table; a test walks the intersection of the two models and asserts they redact it identically. `device` is deliberately not redacted in either. It names the instance's GPU rather than anything about the other user's work, and the list has always shown it — redacting it would have quietly changed what non-admins see. parent_item_id joins the summary because the rows decide from it whether to offer a retry.
Pfannkuchensack
added a commit
to Pfannkuchensack/InvokeAI
that referenced
this pull request
Aug 11, 2026
Six follow-ups from @lstein's sweep that were left open. `require_admin` and `require_admin_or_default` go back to `async def`. They only read `is_admin` off already-resolved token data, so declaring them `def` bought a threadpool round-trip per admin request and nothing else. The `users.get` that can block lives in `get_current_user`, which stays synchronous — the docstrings now say why the two layers differ. The AST guard now inspects what it claims to. It walked only `tree.body`, so a handler registered from inside a factory function or an `if` block was never seen, and `_awaits_something` used `ast.walk`, which counts `await`s inside nested closures — a handler could have passed by defining an inner async helper it never awaits. Both are fixed and both now have their own tests, so the guard's behaviour is pinned rather than asserted in a comment. `convert_model` takes a lock non-blocking and answers 409 otherwise. Blocking would be wrong: a conversion runs for minutes, and for the same key the second caller reads a record the first is midway through replacing. Two conversions in flight also means two models resident at once, which nothing bounds. The body moved into `_convert_model` so the lock scope is visible. Tested for the 409 and for the lock surviving a failed conversion rather than wedging the endpoint for the process's lifetime. `do_hf_login` and `reset_hf_token` hold a lock across the write and the status read-back, which otherwise could report a status belonging to a different token than the one just written. The blocking-work doc gains the bound it was missing: anyio's thread limiter holds 40 tokens, so past forty concurrent blocking requests the stall moves rather than vanishes — and anything else needing a thread queues behind them, including the synchronous auth dependency that runs before a handler is reached. Noted there too that `test_event_loop_blocking.py` cannot show this, because its probe route has neither auth nor database access. `QueueItemDetail` tells a failed fetch apart from a pending one. A queue item the backend cannot serve — one whose graph references a node type this build no longer registers — previously read as "Loading" forever. Left alone deliberately: the stale `old_is_public` in the workflow-updated event, which is cosmetic and would need `workflow_records.update()` to return the previous row to fix properly; and the `delete_user` / `update_user` last-admin invariants, which belong to invoke-ai#9360.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-on PR 1 from @JPPhoto's review of #9163 (the "Database role changes do not invalidate JWT privileges" / "Open sockets retain revoked account privileges" / "Deactivated users can continue queued execution" findings).
Note
Stacked on #9163 — this branch is based on the WAN video branch because the fixes build on machinery that only exists there (media cookie, video invocation-context authorization). The diff will show #9163's changes until it merges; only the top commit (
960d607c76) is new. I'll rebase/retarget once #9163 lands.1. JWT privileges now derive from the database on every request
get_current_user,get_current_user_or_default, the media-cookie validator) build the returnedTokenDatafrom the database record — the token proves identity only. A demoted administrator's old token gets 403 on admin endpoints immediately; a promoted user gains admin on their next request without re-login (this defines the promotion semantics JPPhoto asked to pin down).X-Refreshed-Token, no media cookie) and mints refreshed tokens from the database record, so a stale admin claim can never be renewed — the media cookie renewal path is closed with it.2. Open sockets are re-authorized live
_handle_connectderivesis_adminfrom the database, so a demoted admin reconnecting with an old token does not rejoin the admin room.user_access_changedevent (not registered withpayload_schema, never emitted to clients — no typegen churn) is emitted by the user-management routes on role/status changes. The socket layer responds: demotion leaves the admin room (and the cachedis_adminis corrected sosubscribe_queuecan't re-add it), promotion joins it, deactivation/deletion disconnects all the user's sockets.3. Deactivated users' queued execution is revoked
Policy: pending items are rejected (canceled) at dequeue; running items are canceled immediately where possible and always stop before the next node.
user_access_changedand cancels the currently running item immediately — this drives the existing cancel-event machinery, so step-callback nodes (e.g. denoising) stop mid-node rather than running to completion.systemuser are exempt, per the review spec.Tests (all per JPPhoto's specs)
tests/app/routers/test_privilege_revocation.py— demoted admin: 403 + noX-Refreshed-Token+ no media cookie; same token denied reading another user's private image (positive pre-demotion, negative post-demotion); promoted user gains admin with old token; unchanged admin refresh carriesis_admin=true; demoted user's allowed mutation refreshes withis_admin=false; event emission on demotion/deactivation/deletion and not on display-name changes.tests/app/api/test_sliding_window_token.py— new multiuser class: demoted/promoted refresh carries DB role; deactivated/deleted users get no refresh;remember_mepreserved. (Existing tests now run under an explicit single-user harness.)tests/app/test_socket_privilege_revocation.py— reconnect-with-old-token after demotion; deactivated reconnect rejected; live demotion leaves admin room and can't re-subscribe into it; deactivation/deletion disconnects; promotion joins; other users' sockets untouched (the positive unchanged-admin case).tests/app/services/session_processor/test_privilege_revocation.py—queue_owner_is_activematrix; dequeue rejection (incl. concurrent-deletion race); immediate cancel of the running item on deactivation; multi-node session stops after node 1 when the owner is deactivated mid-run; positive active-user and single-user/system cases.tests/app/services/shared/test_invocation_context_{images,videos}.py— inactive/deleted queue user denied reads and saves (even uncategorized); active user still saves.370 tests pass across the affected areas (auth routes, multiuser authorization, sockets, session queue/processor, invocation context, videos multiuser); ruff clean.
🤖 Generated with Claude Code