From 6693e419baaa8c090bcf767c72a21efc064cbce2 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Thu, 16 Jul 2026 21:26:36 -0400 Subject: [PATCH 01/19] fix(auth): revoke privileges immediately on role change, deactivation, or deletion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- invokeai/app/api/auth_dependencies.py | 28 +- invokeai/app/api/routers/auth.py | 36 +- invokeai/app/api/sockets.py | 47 ++- invokeai/app/api_app.py | 6 + invokeai/app/services/events/events_base.py | 9 + invokeai/app/services/events/events_common.py | 22 + .../session_processor_default.py | 108 +++++ .../app/services/shared/invocation_context.py | 50 ++- invokeai/frontend/web/openapi.json | 2 +- .../frontend/web/src/services/api/schema.ts | 3 +- tests/app/api/test_sliding_window_token.py | 122 ++++++ .../app/routers/test_privilege_revocation.py | 381 ++++++++++++++++++ .../services/session_processor/__init__.py | 0 .../test_privilege_revocation.py | 247 ++++++++++++ .../test_session_processor_cancel_guard.py | 9 +- .../shared/test_invocation_context_images.py | 42 ++ .../shared/test_invocation_context_videos.py | 32 ++ tests/app/test_socket_privilege_revocation.py | 195 +++++++++ tests/app/test_workflow_socketio.py | 4 +- 19 files changed, 1310 insertions(+), 33 deletions(-) create mode 100644 tests/app/routers/test_privilege_revocation.py create mode 100644 tests/app/services/session_processor/__init__.py create mode 100644 tests/app/services/session_processor/test_privilege_revocation.py create mode 100644 tests/app/test_socket_privilege_revocation.py diff --git a/invokeai/app/api/auth_dependencies.py b/invokeai/app/api/auth_dependencies.py index 1ba768a94cd..b5bf67d0606 100644 --- a/invokeai/app/api/auth_dependencies.py +++ b/invokeai/app/api/auth_dependencies.py @@ -1,6 +1,6 @@ """FastAPI dependencies for authentication.""" -from typing import Annotated +from typing import TYPE_CHECKING, Annotated from fastapi import Cookie, Depends, HTTPException, status from fastapi.security import HTTPAuthorizationCredentials, HTTPBearer @@ -9,6 +9,9 @@ from invokeai.app.services.auth.token_service import TokenData, verify_token from invokeai.backend.util.logging import logging +if TYPE_CHECKING: + from invokeai.app.services.users.users_common import UserDTO + logger = logging.getLogger(__name__) # HTTP Bearer token security scheme @@ -24,7 +27,24 @@ def _validate_token(token: str, invalid_detail: str) -> TokenData: user = ApiDependencies.invoker.services.users.get(token_data.user_id) if user is None or not user.is_active: raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="User not found or inactive") - return token_data + return _db_derived_token_data(token_data, user) + + +def _db_derived_token_data(token_data: TokenData, user: "UserDTO") -> TokenData: + """Build TokenData whose authorization fields come from the database record. + + The JWT proves *identity* only. Authorization (``is_admin``) must reflect the + current database state on every request; otherwise a demoted administrator + keeps admin rights until their token expires — and sliding-window refresh + would renew that stale claim indefinitely. A promoted user symmetrically + gains admin rights on their next request without re-login. + """ + return TokenData( + user_id=user.user_id, + email=user.email, + is_admin=user.is_admin, + remember_me=token_data.remember_me, + ) async def get_current_user( @@ -73,7 +93,7 @@ async def get_current_user( headers={"WWW-Authenticate": "Bearer"}, ) - return token_data + return _db_derived_token_data(token_data, user) async def get_current_user_or_default( @@ -125,7 +145,7 @@ async def get_current_user_or_default( # User doesn't exist or is inactive in multiuser mode - reject raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="User not found or inactive") - return token_data + return _db_derived_token_data(token_data, user) async def get_current_media_user_or_default( diff --git a/invokeai/app/api/routers/auth.py b/invokeai/app/api/routers/auth.py index f6a767c7c8d..0c85862f155 100644 --- a/invokeai/app/api/routers/auth.py +++ b/invokeai/app/api/routers/auth.py @@ -528,11 +528,31 @@ async def update_user( The updated user Raises: - HTTPException: 400 if password is weak + HTTPException: 400 if password is weak, or if the change would remove the + last administrator HTTPException: 404 if user not found """ user_service = ApiDependencies.invoker.services.users config = ApiDependencies.invoker.services.configuration + before = user_service.get(user_id) + + # Demoting or deactivating the last administrator is irreversible: authorization is + # derived from the database on every request, so the caller loses admin access + # immediately and no authenticated path back exists. It would also drop `has_admin()` + # to zero, which re-opens the unauthenticated `/auth/setup` endpoint to any caller. + # `delete_user` guards the same invariant. + if ( + before is not None + and before.is_admin + and before.is_active + and (request.is_admin is False or request.is_active is False) + and user_service.count_admins() <= 1 + ): + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail="Cannot remove the last administrator", + ) + try: changes = UserUpdateRequest( display_name=request.display_name, @@ -540,10 +560,19 @@ async def update_user( is_admin=request.is_admin, is_active=request.is_active, ) - return user_service.update(user_id, changes, strict_password_checking=config.strict_password_checking) + updated = user_service.update(user_id, changes, strict_password_checking=config.strict_password_checking) except ValueError as e: raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) from e + # Authorization state changed — notify live connections (open sockets, the + # session processor) so demotion/deactivation takes effect immediately + # instead of persisting until reconnect or token expiry. + if before is not None and (before.is_admin != updated.is_admin or before.is_active != updated.is_active): + ApiDependencies.invoker.services.events.emit_user_access_changed( + user_id=updated.user_id, is_admin=updated.is_admin, is_active=updated.is_active + ) + return updated + @auth_router.delete("/users/{user_id}", status_code=status.HTTP_204_NO_CONTENT) async def delete_user( @@ -579,6 +608,9 @@ async def delete_user( except ValueError as e: raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) from e + # A deleted user must lose live access just like a deactivated one. + ApiDependencies.invoker.services.events.emit_user_access_changed(user_id=user_id, is_admin=False, is_active=False) + @auth_router.patch("/me", response_model=UserDTO) async def update_current_user( diff --git a/invokeai/app/api/sockets.py b/invokeai/app/api/sockets.py index c1cc753c88a..753ffa93c22 100644 --- a/invokeai/app/api/sockets.py +++ b/invokeai/app/api/sockets.py @@ -45,6 +45,7 @@ QueueItemsRetriedEvent, QueueItemStatusChangedEvent, RecallParametersUpdatedEvent, + UserAccessChangedEvent, WorkflowAccessRevokedEvent, WorkflowCreatedEvent, WorkflowDeletedEvent, @@ -102,6 +103,7 @@ class BulkDownloadSubscriptionEvent(BaseModel): BULK_DOWNLOAD_EVENTS = {BulkDownloadStartedEvent, BulkDownloadCompleteEvent, BulkDownloadErrorEvent} WORKFLOW_EVENTS = {WorkflowCreatedEvent, WorkflowUpdatedEvent, WorkflowDeletedEvent} +USER_EVENTS = {UserAccessChangedEvent} LLM_TASK_EVENTS = {LLMTaskProgressEvent, LLMTaskCompleteEvent, LLMTaskErrorEvent} @@ -142,6 +144,7 @@ def __init__(self, app: FastAPI): register_events(BULK_DOWNLOAD_EVENTS, self._handle_bulk_image_download_event) register_events(LLM_TASK_EVENTS, self._handle_llm_task_event) register_events(WORKFLOW_EVENTS, self._handle_workflow_event) + register_events(USER_EVENTS, self._handle_user_access_changed) async def _handle_connect(self, sid: str, environ: dict, auth: dict | None) -> bool: """Handle socket connection and authenticate the user. @@ -169,6 +172,7 @@ async def _handle_connect(self, sid: str, environ: dict, auth: dict | None) -> b if token: token_data = verify_token(token) if token_data: + is_admin = token_data.is_admin # In multiuser mode, also verify the backing user record still # exists and is active — mirrors the REST auth check in # auth_dependencies.py. A deleted or deactivated user whose @@ -181,6 +185,11 @@ async def _handle_connect(self, sid: str, environ: dict, auth: dict | None) -> b if user is None or not user.is_active: logger.warning(f"Rejecting socket {sid}: user {token_data.user_id} not found or inactive") return False + # The token proves identity only — authorization comes from the + # database. A demoted admin reconnecting with an old token must + # not rejoin the admin room; a promoted user gets admin rooms + # without re-login. + is_admin = user.is_admin except Exception: # If user service is unavailable, fail closed logger.warning(f"Rejecting socket {sid}: unable to verify user record") @@ -189,14 +198,12 @@ async def _handle_connect(self, sid: str, environ: dict, auth: dict | None) -> b # Store user_id and is_admin in socket users dict self._socket_users[sid] = { "user_id": token_data.user_id, - "is_admin": token_data.is_admin, + "is_admin": is_admin, } - logger.info( - f"Socket {sid} connected with user_id: {token_data.user_id}, is_admin: {token_data.is_admin}" - ) + logger.info(f"Socket {sid} connected with user_id: {token_data.user_id}, is_admin: {is_admin}") await self._sio.enter_room(sid, f"user:{token_data.user_id}") await self._sio.enter_room(sid, "workflows:shared") - if token_data.is_admin: + if is_admin: await self._sio.enter_room(sid, "admin") return True @@ -240,6 +247,36 @@ async def _handle_disconnect(self, sid: str) -> None: del self._socket_users[sid] logger.debug(f"Socket {sid} disconnected and cleaned up") + async def _handle_user_access_changed(self, event: FastAPIEvent[UserAccessChangedEvent]) -> None: + """Re-authorize a user's open sockets when their role or active status changes. + + Socket room membership is established at connect time; without this handler a + demoted administrator's sockets would remain in the admin room (receiving other + users' private events) and a deactivated or deleted user's sockets would keep + receiving their own private events indefinitely. + + - Deactivated/deleted: disconnect every socket belonging to the user. A + reconnect attempt with the old token is rejected by ``_handle_connect``'s + database check. + - Demoted: leave the admin room and update the cached ``is_admin`` so + ``_handle_sub_queue`` cannot re-add it. + - Promoted: join the admin room, matching the DB-derived REST behavior. + """ + _, event_data = event + affected_sids = [sid for sid, info in self._socket_users.items() if info.get("user_id") == event_data.user_id] + for sid in affected_sids: + if not event_data.is_active: + logger.info(f"Disconnecting socket {sid}: user {event_data.user_id} deactivated or deleted") + await self._sio.disconnect(sid) + continue + self._socket_users[sid]["is_admin"] = event_data.is_admin + if event_data.is_admin: + await self._sio.enter_room(sid, "admin") + logger.info(f"Socket {sid} joined admin room: user {event_data.user_id} promoted") + else: + await self._sio.leave_room(sid, "admin") + logger.info(f"Socket {sid} left admin room: user {event_data.user_id} demoted") + async def _handle_sub_queue(self, sid: str, data: Any) -> None: """Handle queue subscription and add socket to both queue and user-specific rooms.""" queue_id = QueueSubscriptionEvent(**data).queue_id diff --git a/invokeai/app/api_app.py b/invokeai/app/api_app.py index c0722c4bd1c..f6e247e6f8b 100644 --- a/invokeai/app/api_app.py +++ b/invokeai/app/api_app.py @@ -134,6 +134,12 @@ async def dispatch(self, request: Request, call_next: RequestResponseEndpoint): # process-wide lock; run it off the event loop so a contended # lock (e.g. generation-result writes) can't stall every # concurrent request from inside this per-mutation middleware. + # Refresh only for a user that still exists and is active, and mint + # the new token from the *database* record — not the old token's + # claims. Otherwise a demoted administrator's stale is_admin claim + # (and the media cookie carrying it) would be renewed indefinitely + # by their own mutations, and a deactivated user could keep an + # active session alive. user = await run_in_threadpool(ApiDependencies.invoker.services.users.get, token_data.user_id) if user is None or not user.is_active: return response diff --git a/invokeai/app/services/events/events_base.py b/invokeai/app/services/events/events_base.py index 5999258f6cd..5ad7de3a871 100644 --- a/invokeai/app/services/events/events_base.py +++ b/invokeai/app/services/events/events_base.py @@ -36,6 +36,7 @@ QueueItemsRetriedEvent, QueueItemStatusChangedEvent, RecallParametersUpdatedEvent, + UserAccessChangedEvent, WorkflowCreatedEvent, WorkflowDeletedEvent, WorkflowUpdatedEvent, @@ -164,6 +165,14 @@ def emit_workflow_deleted(self, workflow_id: str, user_id: str, is_public: bool) # endregion + # region User accounts + + def emit_user_access_changed(self, user_id: str, is_admin: bool, is_active: bool) -> None: + """Emitted when a user's role or active status changes (server-internal; never sent to clients).""" + self.dispatch(UserAccessChangedEvent.build(user_id=user_id, is_admin=is_admin, is_active=is_active)) + + # endregion + # region Download def emit_download_started(self, job: "DownloadJob") -> None: diff --git a/invokeai/app/services/events/events_common.py b/invokeai/app/services/events/events_common.py index c831f32e602..33065525cf1 100644 --- a/invokeai/app/services/events/events_common.py +++ b/invokeai/app/services/events/events_common.py @@ -869,3 +869,25 @@ class RecallParametersUpdatedEvent(QueueEventBase): @classmethod def build(cls, queue_id: str, user_id: str, parameters: dict[str, Any]) -> "RecallParametersUpdatedEvent": return cls(queue_id=queue_id, user_id=user_id, parameters=parameters) + + +class UserAccessChangedEvent(EventBase): + """Event model for user_access_changed. + + Emitted when a user's authorization state changes (role change, deactivation, + or deletion) so that live connections — e.g. open sockets — can be re-authorized + immediately instead of trusting connect-time claims until reconnect. + + This event is server-internal: it is deliberately NOT registered with + `payload_schema` and is never emitted to clients. + """ + + __event_name__ = "user_access_changed" + + user_id: str = Field(description="The ID of the affected user") + is_admin: bool = Field(description="Whether the user currently has admin privileges") + is_active: bool = Field(description="Whether the user account is currently active (False for deleted users)") + + @classmethod + def build(cls, user_id: str, is_admin: bool, is_active: bool) -> "UserAccessChangedEvent": + return cls(user_id=user_id, is_admin=is_admin, is_active=is_active) diff --git a/invokeai/app/services/session_processor/session_processor_default.py b/invokeai/app/services/session_processor/session_processor_default.py index 1a9a7be9cff..a7b79eeae90 100644 --- a/invokeai/app/services/session_processor/session_processor_default.py +++ b/invokeai/app/services/session_processor/session_processor_default.py @@ -6,6 +6,7 @@ from typing import Iterator, Optional import torch +from starlette.concurrency import run_in_threadpool from invokeai.app.invocations.baseinvocation import BaseInvocation, BaseInvocationOutput from invokeai.app.invocations.call_saved_workflow import CallSavedWorkflowInvocation @@ -14,6 +15,7 @@ FastAPIEvent, QueueClearedEvent, QueueItemStatusChangedEvent, + UserAccessChangedEvent, register_events, ) from invokeai.app.services.invocation_stats.invocation_stats_common import GESStatsNotFoundError @@ -42,6 +44,40 @@ from invokeai.backend.util.devices import TorchDevice +def queue_owner_is_active(services: InvocationServices, queue_item: SessionQueueItem) -> bool: + """Whether the queue item's owner is still permitted to execute work. + + Deactivating (or deleting) an account must also revoke its queued execution: + a queued graph consumes GPU time, reads media, and writes outputs on behalf of + its owner. Pending items are rejected at dequeue; running items are stopped at + the next node boundary (and immediately mid-node for nodes with step callbacks, + via the cancel event set when the item is canceled). + + The ``system`` user represents single-user mode and has no database record, so + it is always considered active. The check is skipped entirely when multiuser + mode is disabled. + + A failed lookup is treated as active. This runs between nodes on a path with no + exception handling of its own, so letting a transient error (e.g. a busy-timeout + on the shared SQLite connection under multi-GPU write contention) escape would + abandon the session without its normal teardown. Denying execution on a failed + read would also revoke privileges the database never actually revoked; the next + node boundary re-checks, and the dequeue gate catches the item on its next run. + """ + if not services.configuration.multiuser: + return True + if queue_item.user_id == "system": + return True + try: + user = services.users.get(queue_item.user_id) + except Exception: + services.logger.warning( + f"Could not verify owner {queue_item.user_id} of queue item {queue_item.item_id}; allowing execution" + ) + return True + return user is not None and user.is_active + + class DefaultSessionRunner(SessionRunnerBase): """Processes a single session's invocations.""" @@ -102,6 +138,17 @@ def _run_session_loop(self, queue_item: SessionQueueItem) -> None: if invocation is None or self._is_canceled(): break + # Revalidate the owner between nodes: an account deactivated mid-session + # must not execute further nodes. Cancel the item so its status reflects + # why execution stopped. + if not queue_owner_is_active(self._services, queue_item): + self._services.logger.warning( + f"Canceling queue item {queue_item.item_id}: owner {queue_item.user_id} is deactivated or deleted" + ) + with suppress(SessionQueueItemNotFoundError): + self._services.session_queue.cancel_queue_item(queue_item.item_id) + break + self.run_node(invocation, queue_item) # The session is complete if all invocations have been run or there is an error on the session. @@ -470,6 +517,7 @@ def start(self, invoker: Invoker) -> None: register_events(QueueClearedEvent, self._on_queue_cleared) register_events(BatchEnqueuedEvent, self._on_batch_enqueued) register_events(QueueItemStatusChangedEvent, self._on_queue_item_status_changed) + register_events(UserAccessChangedEvent, self._on_user_access_changed) devices = self._resolve_devices() @@ -567,6 +615,46 @@ async def _on_queue_cleared(self, event: FastAPIEvent[QueueClearedEvent]) -> Non async def _on_batch_enqueued(self, event: FastAPIEvent[BatchEnqueuedEvent]) -> None: self._poll_now() + async def _on_user_access_changed(self, event: FastAPIEvent[UserAccessChangedEvent]) -> None: + # If the owner of the currently running queue item was deactivated or deleted, + # cancel the item immediately. Canceling emits a QueueItemStatusChangedEvent, + # which sets the cancel event (see `_on_queue_item_status_changed`), stopping + # long-running nodes at their next step callback rather than waiting for the + # node to finish. Pending items are handled at dequeue. + event_data = event[1] + if event_data.is_active: + return + # A single user may have items running on several workers concurrently, so + # cancel every match rather than stopping at the first. + item_ids: list[int] = [] + for worker in self._workers: + queue_item = worker.queue_item + if queue_item is not None and queue_item.user_id == event_data.user_id: + self._invoker.services.logger.warning( + f"Canceling queue item {queue_item.item_id}: owner {queue_item.user_id} was deactivated or deleted" + ) + item_ids.append(queue_item.item_id) + if not item_ids: + return + + # Run the cancellations in a thread. `cancel_queue_item` walks the workflow-call + # chain and issues a transaction per item, all behind the process-wide SQLite lock; + # doing that inline would block the event loop — and with it every HTTP response, + # socket emission, and event dispatch, including the QueueItemStatusChangedEvents + # this cancellation depends on to reach the workers. + # + # The workers' cancel events are deliberately NOT set here. `cancel_queue_item` + # writes the row terminal before emitting, and `_process` relies on that ordering: + # a cancel event set while the row is still non-terminal is treated as a stale + # signal from a previous item and cleared (see the guard after dequeue), which + # would discard this cancellation. + def _cancel_all() -> None: + for item_id in item_ids: + with suppress(SessionQueueItemNotFoundError): + self._invoker.services.session_queue.cancel_queue_item(item_id) + + await run_in_threadpool(_cancel_all) + async def _on_queue_item_status_changed(self, event: FastAPIEvent[QueueItemStatusChangedEvent]) -> None: # Find the worker (if any) currently running the item whose status changed. for worker in self._workers: @@ -614,6 +702,20 @@ def _is_image_move_maintenance_active(self) -> bool: image_moves = getattr(self._invoker.services, "image_moves", None) return image_moves is not None and image_moves.is_maintenance_active() + def _cancel_queue_item_if_owner_inactive(self, queue_item: SessionQueueItem) -> bool: + """Cancel a dequeued item whose owner is deactivated or deleted. + + Returns True if the item was rejected (canceled) and must not be executed. + """ + if queue_owner_is_active(self._invoker.services, queue_item): + return False + self._invoker.services.logger.warning( + f"Canceling queue item {queue_item.item_id}: owner {queue_item.user_id} is deactivated or deleted" + ) + with suppress(SessionQueueItemNotFoundError): + self._invoker.services.session_queue.cancel_queue_item(queue_item.item_id) + return True + def _process( self, worker: _SessionWorker, @@ -705,6 +807,12 @@ def _process( ) continue + # Reject items whose owner was deactivated or deleted while the item + # was pending — no invocation may run and no output may be saved on + # behalf of a revoked account. + if self._cancel_queue_item_if_owner_inactive(worker.queue_item): + continue + # GC-ing here can reduce peak memory usage of the invoke process by freeing allocated memory blocks. # Most queue items take seconds to execute, so the relative cost of a GC is very small. # Python will never cede allocated memory back to the OS, so anything we can do to reduce the peak diff --git a/invokeai/app/services/shared/invocation_context.py b/invokeai/app/services/shared/invocation_context.py index 94a2eb86bf1..e261fc89393 100644 --- a/invokeai/app/services/shared/invocation_context.py +++ b/invokeai/app/services/shared/invocation_context.py @@ -175,7 +175,10 @@ def _assert_read_access(self, image_name: str) -> None: return user_id = self._data.queue_item.user_id user = self._services.users.get(user_id) - if user is None: + # A deactivated or deleted account keeps no queue-time privileges: its + # queued graphs must not read media even if the item slipped past the + # processor's owner checks. + if user is None or not user.is_active: raise PermissionError("Queue user is not authorized to access this image") if user.is_admin or self._services.image_records.get_user_id(image_name) == user_id: return @@ -228,15 +231,20 @@ def save( elif isinstance(self._data.invocation, WithBoard) and self._data.invocation.board: board_id_ = self._data.invocation.board.board_id - if board_id_ is not None and self._services.configuration.multiuser: - board = self._services.boards.get_dto(board_id_) + if self._services.configuration.multiuser: user = self._services.users.get(self._data.queue_item.user_id) - if user is None or ( - not user.is_admin - and board.user_id != self._data.queue_item.user_id - and board.board_visibility != BoardVisibility.Public - ): - raise PermissionError("Queue user is not authorized to save images to this board") + # A deactivated or deleted account must not save outputs, even + # uncategorized ones — deactivation revokes queue-time privileges. + if user is None or not user.is_active: + raise PermissionError("Queue user is not authorized to save images") + if board_id_ is not None: + board = self._services.boards.get_dto(board_id_) + if ( + not user.is_admin + and board.user_id != self._data.queue_item.user_id + and board.board_visibility != BoardVisibility.Public + ): + raise PermissionError("Queue user is not authorized to save images to this board") workflow_ = None if self._data.queue_item.workflow: @@ -341,7 +349,9 @@ def _assert_read_access(self, video_name: str) -> None: return user_id = self._data.queue_item.user_id user = self._services.users.get(user_id) - if user is None: + # See ImagesInterface._assert_read_access: deactivated accounts keep no + # queue-time privileges. + if user is None or not user.is_active: raise PermissionError("Queue user is not authorized to access this video") if user.is_admin or self._services.video_records.get_user_id(video_name) == user_id: return @@ -387,15 +397,19 @@ def save( elif isinstance(self._data.invocation, WithBoard) and self._data.invocation.board: board_id_ = self._data.invocation.board.board_id - if board_id_ is not None and self._services.configuration.multiuser: - board = self._services.boards.get_dto(board_id_) + if self._services.configuration.multiuser: user = self._services.users.get(self._data.queue_item.user_id) - if user is None or ( - not user.is_admin - and board.user_id != self._data.queue_item.user_id - and board.board_visibility != BoardVisibility.Public - ): - raise PermissionError("Queue user is not authorized to save videos to this board") + # See ImagesInterface.save: deactivated accounts must not save outputs. + if user is None or not user.is_active: + raise PermissionError("Queue user is not authorized to save videos") + if board_id_ is not None: + board = self._services.boards.get_dto(board_id_) + if ( + not user.is_admin + and board.user_id != self._data.queue_item.user_id + and board.board_visibility != BoardVisibility.Public + ): + raise PermissionError("Queue user is not authorized to save videos to this board") workflow_ = None if self._data.queue_item.workflow: diff --git a/invokeai/frontend/web/openapi.json b/invokeai/frontend/web/openapi.json index add72e7ac55..6fe1fdf79d7 100644 --- a/invokeai/frontend/web/openapi.json +++ b/invokeai/frontend/web/openapi.json @@ -375,7 +375,7 @@ "patch": { "tags": ["authentication"], "summary": "Update User", - "description": "Update a user. Requires admin privileges.\n\nArgs:\n user_id: The user ID\n request: Fields to update\n\nReturns:\n The updated user\n\nRaises:\n HTTPException: 400 if password is weak\n HTTPException: 404 if user not found", + "description": "Update a user. Requires admin privileges.\n\nArgs:\n user_id: The user ID\n request: Fields to update\n\nReturns:\n The updated user\n\nRaises:\n HTTPException: 400 if password is weak, or if the change would remove the\n last administrator\n HTTPException: 404 if user not found", "operationId": "update_user_api_v1_auth_users__user_id__patch", "security": [ { diff --git a/invokeai/frontend/web/src/services/api/schema.ts b/invokeai/frontend/web/src/services/api/schema.ts index 001aa2233cd..c3f71173ea5 100644 --- a/invokeai/frontend/web/src/services/api/schema.ts +++ b/invokeai/frontend/web/src/services/api/schema.ts @@ -318,7 +318,8 @@ export type paths = { * The updated user * * Raises: - * HTTPException: 400 if password is weak + * HTTPException: 400 if password is weak, or if the change would remove the + * last administrator * HTTPException: 404 if user not found */ patch: operations["update_user_api_v1_auth_users__user_id__patch"]; diff --git a/tests/app/api/test_sliding_window_token.py b/tests/app/api/test_sliding_window_token.py index 409992a65d2..8cfdc711cac 100644 --- a/tests/app/api/test_sliding_window_token.py +++ b/tests/app/api/test_sliding_window_token.py @@ -32,6 +32,29 @@ def _setup_jwt_secret(monkeypatch: pytest.MonkeyPatch): return user +def _patch_user_record(monkeypatch: pytest.MonkeyPatch, user) -> None: + """Point the middleware's user lookup at `user` (or None for a deleted account). + + Overrides the record installed by `_setup_jwt_secret`. The middleware revalidates + against the database on every refresh regardless of multiuser mode, so this is all + the harness needs to vary. + """ + from invokeai.app.api.dependencies import ApiDependencies + + monkeypatch.setattr( + ApiDependencies, + "invoker", + SimpleNamespace( + services=SimpleNamespace( + users=SimpleNamespace( + get=lambda user_id: user if user is not None and user.user_id == user_id else None + ) + ) + ), + raising=False, + ) + + def _create_test_app() -> FastAPI: """Create a minimal FastAPI app with the SlidingWindowTokenMiddleware.""" from invokeai.app.api_app import SlidingWindowTokenMiddleware @@ -310,3 +333,102 @@ def test_auth_route_exclusions_match_under_prefix(self, preserve: bool, path: st assert response.status_code == 200 assert "X-Refreshed-Token" not in response.headers + + +class TestSlidingWindowMultiuserRevalidation: + """In multiuser mode, refresh must derive authorization from the database and + refuse to renew tokens for missing or deactivated users.""" + + def test_demoted_admin_refresh_carries_db_role(self, monkeypatch: pytest.MonkeyPatch): + """A stale is_admin=True claim is NOT renewed — the refreshed token carries the + database's is_admin=False.""" + from types import SimpleNamespace + + from invokeai.app.services.auth.token_service import verify_token + + _patch_user_record( + monkeypatch, + SimpleNamespace(user_id="test-user", email="test@test.com", is_admin=False, is_active=True), + ) + app = _create_test_app() + client = TestClient(app) + stale_admin_token = create_access_token( + TokenData(user_id="test-user", email="test@test.com", is_admin=True, remember_me=False) + ) + + response = client.post("/test", headers={"Authorization": f"Bearer {stale_admin_token}"}) + + assert response.status_code == 200 + refreshed = verify_token(response.headers["X-Refreshed-Token"]) + assert refreshed is not None + assert refreshed.is_admin is False + + def test_promoted_user_refresh_carries_db_role(self, monkeypatch: pytest.MonkeyPatch): + from types import SimpleNamespace + + from invokeai.app.services.auth.token_service import verify_token + + _patch_user_record( + monkeypatch, + SimpleNamespace(user_id="test-user", email="test@test.com", is_admin=True, is_active=True), + ) + app = _create_test_app() + client = TestClient(app) + stale_token = create_access_token( + TokenData(user_id="test-user", email="test@test.com", is_admin=False, remember_me=False) + ) + + response = client.post("/test", headers={"Authorization": f"Bearer {stale_token}"}) + + refreshed = verify_token(response.headers["X-Refreshed-Token"]) + assert refreshed is not None + assert refreshed.is_admin is True + + def test_deactivated_user_gets_no_refresh(self, monkeypatch: pytest.MonkeyPatch): + """No X-Refreshed-Token and no media cookie for a deactivated account.""" + from types import SimpleNamespace + + _patch_user_record( + monkeypatch, + SimpleNamespace(user_id="test-user", email="test@test.com", is_admin=False, is_active=False), + ) + app = _create_test_app() + client = TestClient(app) + token = _make_token() + + response = client.post("/test", headers={"Authorization": f"Bearer {token}"}) + + assert response.status_code == 200 + assert "X-Refreshed-Token" not in response.headers + assert response.cookies.get("invokeai_media_token") is None + + def test_deleted_user_gets_no_refresh(self, monkeypatch: pytest.MonkeyPatch): + _patch_user_record(monkeypatch, None) + app = _create_test_app() + client = TestClient(app) + token = _make_token() + + response = client.post("/test", headers={"Authorization": f"Bearer {token}"}) + + assert response.status_code == 200 + assert "X-Refreshed-Token" not in response.headers + assert response.cookies.get("invokeai_media_token") is None + + def test_active_user_refresh_preserves_remember_me(self, monkeypatch: pytest.MonkeyPatch): + from types import SimpleNamespace + + from invokeai.app.services.auth.token_service import verify_token + + _patch_user_record( + monkeypatch, + SimpleNamespace(user_id="test-user", email="test@test.com", is_admin=False, is_active=True), + ) + app = _create_test_app() + client = TestClient(app) + token = _make_token(remember_me=True) + + response = client.post("/test", headers={"Authorization": f"Bearer {token}"}) + + refreshed = verify_token(response.headers["X-Refreshed-Token"]) + assert refreshed is not None + assert refreshed.remember_me is True diff --git a/tests/app/routers/test_privilege_revocation.py b/tests/app/routers/test_privilege_revocation.py new file mode 100644 index 00000000000..4107371429d --- /dev/null +++ b/tests/app/routers/test_privilege_revocation.py @@ -0,0 +1,381 @@ +"""Tests that database role/status changes take effect immediately for existing JWTs. + +The JWT proves identity only. Authorization (`is_admin`, `is_active`) is derived from +the database on every authenticated request, and sliding-window refresh mints the new +token from the database record. Without this, a demoted administrator could keep admin +rights until token expiry — renewing the stale claim (and media cookie) indefinitely +with every mutation. +""" + +import logging +from typing import Any +from unittest.mock import MagicMock + +import pytest +from fastapi import status +from fastapi.testclient import TestClient + +from invokeai.app.api.dependencies import ApiDependencies +from invokeai.app.api_app import app +from invokeai.app.services.auth.token_service import verify_token +from invokeai.app.services.config.config_default import InvokeAIAppConfig +from invokeai.app.services.events.events_common import UserAccessChangedEvent +from invokeai.app.services.invocation_services import InvocationServices +from invokeai.app.services.invoker import Invoker +from invokeai.app.services.users.users_common import UserCreateRequest +from invokeai.app.services.workflow_records.workflow_records_sqlite import SqliteWorkflowRecordsStorage +from invokeai.backend.util.logging import InvokeAILogger +from tests.fixtures.sqlite_database import create_mock_sqlite_database + + +class MockApiDependencies(ApiDependencies): + invoker: Invoker + + def __init__(self, invoker: Invoker) -> None: + self.invoker = invoker + + +@pytest.fixture +def setup_jwt_secret(): + from invokeai.app.services.auth.token_service import set_jwt_secret + + set_jwt_secret("test-secret-key-for-unit-tests-only-do-not-use-in-production") + + +@pytest.fixture +def client(): + return TestClient(app) + + +@pytest.fixture +def mock_services() -> InvocationServices: + from invokeai.app.services.board_image_records.board_image_records_sqlite import SqliteBoardImageRecordStorage + from invokeai.app.services.board_records.board_records_sqlite import SqliteBoardRecordStorage + from invokeai.app.services.board_video_records.board_video_records_sqlite import SqliteBoardVideoRecordStorage + from invokeai.app.services.boards.boards_default import BoardService + from invokeai.app.services.bulk_download.bulk_download_default import BulkDownloadService + from invokeai.app.services.client_state_persistence.client_state_persistence_sqlite import ( + ClientStatePersistenceSqlite, + ) + from invokeai.app.services.image_records.image_records_sqlite import SqliteImageRecordStorage + from invokeai.app.services.images.images_default import ImageService + from invokeai.app.services.invocation_cache.invocation_cache_memory import MemoryInvocationCache + from invokeai.app.services.invocation_stats.invocation_stats_default import InvocationStatsService + from invokeai.app.services.system_prompt_records.system_prompt_records_sqlite import ( + SqliteSystemPromptRecordsStorage, + ) + from invokeai.app.services.users.users_default import UserService + from invokeai.app.services.video_records.video_records_sqlite import SqliteVideoRecordStorage + from tests.test_nodes import TestEventService + + configuration = InvokeAIAppConfig(use_memory_db=True, node_cache_size=0) + logger = InvokeAILogger.get_logger() + db = create_mock_sqlite_database(configuration, logger) + + return InvocationServices( + board_image_records=SqliteBoardImageRecordStorage(db=db), + board_images=None, # type: ignore + board_records=SqliteBoardRecordStorage(db=db), + boards=BoardService(), + bulk_download=BulkDownloadService(), + configuration=configuration, + events=TestEventService(), + image_files=None, # type: ignore + image_records=SqliteImageRecordStorage(db=db), + images=ImageService(), + invocation_cache=MemoryInvocationCache(max_cache_size=0), + logger=logging, # type: ignore + model_images=None, # type: ignore + model_manager=None, # type: ignore + download_queue=None, # type: ignore + names=None, # type: ignore + performance_statistics=InvocationStatsService(), + session_processor=None, # type: ignore + session_queue=None, # type: ignore + urls=None, # type: ignore + workflow_records=SqliteWorkflowRecordsStorage(db=db), + tensors=None, # type: ignore + conditioning=None, # type: ignore + style_preset_records=None, # type: ignore + style_preset_image_files=None, # type: ignore + system_prompt_records=SqliteSystemPromptRecordsStorage(db=db), + workflow_thumbnails=None, # type: ignore + model_relationship_records=None, # type: ignore + model_relationships=None, # type: ignore + client_state_persistence=ClientStatePersistenceSqlite(db=db), + users=UserService(db), + external_generation=None, # type: ignore + videos=None, # type: ignore + video_files=None, # type: ignore + video_records=SqliteVideoRecordStorage(db=db), + board_video_records=SqliteBoardVideoRecordStorage(db=db), + gallery=None, # type: ignore + ) + + +@pytest.fixture() +def mock_invoker(mock_services: InvocationServices) -> Invoker: + return Invoker(services=mock_services) + + +def _save_image(mock_invoker: Invoker, image_name: str, user_id: str) -> None: + from invokeai.app.services.image_records.image_records_common import ImageCategory, ResourceOrigin + + mock_invoker.services.image_records.save( + image_name=image_name, + image_origin=ResourceOrigin.INTERNAL, + image_category=ImageCategory.GENERAL, + width=100, + height=100, + has_workflow=False, + user_id=user_id, + ) + + +def _create_user(mock_invoker: Invoker, email: str, display_name: str, is_admin: bool = False) -> str: + user = mock_invoker.services.users.create( + UserCreateRequest(email=email, display_name=display_name, password="TestPass123", is_admin=is_admin) + ) + return user.user_id + + +def _login(client: TestClient, email: str) -> str: + r = client.post("/api/v1/auth/login", json={"email": email, "password": "TestPass123", "remember_me": False}) + assert r.status_code == 200 + return r.json()["token"] + + +def _auth(token: str) -> dict[str, str]: + return {"Authorization": f"Bearer {token}"} + + +@pytest.fixture +def enable_multiuser(monkeypatch: Any, mock_invoker: Invoker, setup_jwt_secret: None): + mock_invoker.services.configuration.multiuser = True + + mock_board_images = MagicMock() + mock_board_images.get_all_board_image_names_for_board.return_value = [] + mock_invoker.services.board_images = mock_board_images + + mock_deps = MockApiDependencies(mock_invoker) + monkeypatch.setattr("invokeai.app.api.routers.auth.ApiDependencies", mock_deps) + monkeypatch.setattr("invokeai.app.api.auth_dependencies.ApiDependencies", mock_deps) + monkeypatch.setattr("invokeai.app.api.routers.images.ApiDependencies", mock_deps) + monkeypatch.setattr("invokeai.app.api.routers._access.ApiDependencies", mock_deps) + # The sliding-window middleware binds ApiDependencies at import time in api_app, so + # patching the defining module alone would not reach it. Patch both so refresh + # assertions exercise the real logic. + monkeypatch.setattr("invokeai.app.api.dependencies.ApiDependencies", mock_deps) + monkeypatch.setattr("invokeai.app.api_app.ApiDependencies", mock_deps) + yield + + +@pytest.fixture +def admin_token(enable_multiuser: Any, mock_invoker: Invoker, client: TestClient): + _create_user(mock_invoker, "admin@test.com", "Admin", is_admin=True) + return _login(client, "admin@test.com") + + +def _demote(client: TestClient, admin_token: str, mock_invoker: Invoker, email: str) -> None: + user = mock_invoker.services.users.get_by_email(email) + assert user is not None + r = client.patch(f"/api/v1/auth/users/{user.user_id}", json={"is_admin": False}, headers=_auth(admin_token)) + assert r.status_code == 200 + + +class TestLastAdminGuard: + """Because authorization is now DB-derived and takes effect immediately, removing the + last administrator would be unrecoverable — and would re-open unauthenticated setup.""" + + def _patch_self(self, client: TestClient, mock_invoker: Invoker, token: str, body: dict) -> Any: + user = mock_invoker.services.users.get_by_email("admin@test.com") + assert user is not None + return client.patch(f"/api/v1/auth/users/{user.user_id}", json=body, headers=_auth(token)) + + def test_demoting_last_admin_is_rejected(self, client: TestClient, mock_invoker: Invoker, admin_token: str) -> None: + r = self._patch_self(client, mock_invoker, admin_token, {"is_admin": False}) + + assert r.status_code == status.HTTP_400_BAD_REQUEST + assert mock_invoker.services.users.has_admin() is True + + def test_deactivating_last_admin_is_rejected( + self, client: TestClient, mock_invoker: Invoker, admin_token: str + ) -> None: + r = self._patch_self(client, mock_invoker, admin_token, {"is_active": False}) + + assert r.status_code == status.HTTP_400_BAD_REQUEST + assert mock_invoker.services.users.has_admin() is True + + def test_setup_endpoint_stays_closed_after_rejected_demotion( + self, client: TestClient, mock_invoker: Invoker, admin_token: str + ) -> None: + """The unauthenticated setup endpoint must not become reachable again.""" + self._patch_self(client, mock_invoker, admin_token, {"is_admin": False}) + + r = client.post( + "/api/v1/auth/setup", + json={"email": "attacker@test.com", "display_name": "Attacker", "password": "TestPass123"}, + ) + + assert r.status_code == status.HTTP_400_BAD_REQUEST + + def test_demoting_admin_is_allowed_when_another_admin_remains( + self, client: TestClient, mock_invoker: Invoker, admin_token: str + ) -> None: + _create_user(mock_invoker, "admin2@test.com", "Admin Two", is_admin=True) + + r = self._patch_self(client, mock_invoker, admin_token, {"is_admin": False}) + + assert r.status_code == 200 + assert r.json()["is_admin"] is False + + def test_unrelated_update_to_last_admin_is_allowed( + self, client: TestClient, mock_invoker: Invoker, admin_token: str + ) -> None: + """The guard must only block the two fields that can remove admin access.""" + r = self._patch_self(client, mock_invoker, admin_token, {"display_name": "Renamed"}) + + assert r.status_code == 200 + assert r.json()["display_name"] == "Renamed" + + +class TestDbDerivedAuthorization: + """Authorization fields come from the database record, not the token's claims.""" + + def test_demoted_admin_old_token_gets_403_and_no_refresh( + self, client: TestClient, mock_invoker: Invoker, admin_token: str + ) -> None: + _create_user(mock_invoker, "admin2@test.com", "Admin Two", is_admin=True) + admin2_token = _login(client, "admin2@test.com") + _demote(client, admin_token, mock_invoker, "admin2@test.com") + + # Admin-only mutation with the pre-demotion token: rejected, and the stale + # admin claim is not renewed (no refreshed bearer token, no media cookie). + r = client.post( + "/api/v1/auth/users", + json={"email": "new@test.com", "display_name": "New", "password": "TestPass123", "is_admin": False}, + headers=_auth(admin2_token), + ) + assert r.status_code == status.HTTP_403_FORBIDDEN + assert "X-Refreshed-Token" not in r.headers + assert "set-cookie" not in r.headers + + def test_demoted_admin_old_token_cannot_read_other_users_private_image( + self, client: TestClient, mock_invoker: Invoker, admin_token: str + ) -> None: + _create_user(mock_invoker, "admin2@test.com", "Admin Two", is_admin=True) + _create_user(mock_invoker, "user1@test.com", "User One") + admin2_token = _login(client, "admin2@test.com") + user1 = mock_invoker.services.users.get_by_email("user1@test.com") + assert user1 is not None + _save_image(mock_invoker, "user1-private-img", user1.user_id) + # The DTO route resolves URLs through the urls service, which is None in + # the test harness. + mock_urls = MagicMock() + mock_urls.get_image_url.return_value = "http://test/image.png" + mock_invoker.services.urls = mock_urls + + # Pre-demotion the token works (admin may read any image)... + r = client.get("/api/v1/images/i/user1-private-img", headers=_auth(admin2_token)) + assert r.status_code == status.HTTP_200_OK + + _demote(client, admin_token, mock_invoker, "admin2@test.com") + + # ...post-demotion the same token is denied. + r = client.get("/api/v1/images/i/user1-private-img", headers=_auth(admin2_token)) + assert r.status_code == status.HTTP_403_FORBIDDEN + + def test_promoted_user_old_token_gains_admin( + self, client: TestClient, mock_invoker: Invoker, admin_token: str + ) -> None: + _create_user(mock_invoker, "user1@test.com", "User One") + user1_token = _login(client, "user1@test.com") + user1 = mock_invoker.services.users.get_by_email("user1@test.com") + assert user1 is not None + + r = client.get("/api/v1/auth/users", headers=_auth(user1_token)) + assert r.status_code == status.HTTP_403_FORBIDDEN + + r = client.patch(f"/api/v1/auth/users/{user1.user_id}", json={"is_admin": True}, headers=_auth(admin_token)) + assert r.status_code == 200 + + # The pre-promotion token now carries admin rights (derived from the DB). + r = client.get("/api/v1/auth/users", headers=_auth(user1_token)) + assert r.status_code == status.HTTP_200_OK + + def test_unchanged_admin_mutation_refreshes_with_admin_claim( + self, client: TestClient, mock_invoker: Invoker, admin_token: str + ) -> None: + r = client.post( + "/api/v1/auth/users", + json={"email": "new@test.com", "display_name": "New", "password": "TestPass123", "is_admin": False}, + headers=_auth(admin_token), + ) + assert r.status_code == status.HTTP_201_CREATED + refreshed = verify_token(r.headers["X-Refreshed-Token"]) + assert refreshed is not None + assert refreshed.is_admin is True + + def test_demoted_admin_allowed_mutation_refreshes_with_demoted_claim( + self, client: TestClient, mock_invoker: Invoker, admin_token: str + ) -> None: + """A successful non-admin mutation by a demoted user renews the token with the + database's is_admin=False — the stale admin claim does not survive refresh.""" + _create_user(mock_invoker, "admin2@test.com", "Admin Two", is_admin=True) + admin2_token = _login(client, "admin2@test.com") + _demote(client, admin_token, mock_invoker, "admin2@test.com") + + r = client.patch("/api/v1/auth/me", json={"display_name": "Renamed"}, headers=_auth(admin2_token)) + assert r.status_code == 200 + refreshed = verify_token(r.headers["X-Refreshed-Token"]) + assert refreshed is not None + assert refreshed.is_admin is False + + +class TestUserAccessChangedEmission: + """Role/status changes emit the internal event that re-authorizes live connections.""" + + def _access_events(self, mock_invoker: Invoker) -> list[UserAccessChangedEvent]: + return [e for e in mock_invoker.services.events.events if isinstance(e, UserAccessChangedEvent)] + + def test_demotion_emits_event(self, client: TestClient, mock_invoker: Invoker, admin_token: str) -> None: + _create_user(mock_invoker, "admin2@test.com", "Admin Two", is_admin=True) + _demote(client, admin_token, mock_invoker, "admin2@test.com") + + events = self._access_events(mock_invoker) + assert len(events) == 1 + assert events[0].is_admin is False + assert events[0].is_active is True + + def test_deactivation_emits_event(self, client: TestClient, mock_invoker: Invoker, admin_token: str) -> None: + user_id = _create_user(mock_invoker, "user1@test.com", "User One") + + r = client.patch(f"/api/v1/auth/users/{user_id}", json={"is_active": False}, headers=_auth(admin_token)) + assert r.status_code == 200 + + events = self._access_events(mock_invoker) + assert len(events) == 1 + assert events[0].user_id == user_id + assert events[0].is_active is False + + def test_display_name_change_does_not_emit_event( + self, client: TestClient, mock_invoker: Invoker, admin_token: str + ) -> None: + user_id = _create_user(mock_invoker, "user1@test.com", "User One") + + r = client.patch(f"/api/v1/auth/users/{user_id}", json={"display_name": "Renamed"}, headers=_auth(admin_token)) + assert r.status_code == 200 + + assert self._access_events(mock_invoker) == [] + + def test_deletion_emits_inactive_event(self, client: TestClient, mock_invoker: Invoker, admin_token: str) -> None: + user_id = _create_user(mock_invoker, "user1@test.com", "User One") + + r = client.delete(f"/api/v1/auth/users/{user_id}", headers=_auth(admin_token)) + assert r.status_code == status.HTTP_204_NO_CONTENT + + events = self._access_events(mock_invoker) + assert len(events) == 1 + assert events[0].user_id == user_id + assert events[0].is_active is False + assert events[0].is_admin is False diff --git a/tests/app/services/session_processor/__init__.py b/tests/app/services/session_processor/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/app/services/session_processor/test_privilege_revocation.py b/tests/app/services/session_processor/test_privilege_revocation.py new file mode 100644 index 00000000000..88605480bdd --- /dev/null +++ b/tests/app/services/session_processor/test_privilege_revocation.py @@ -0,0 +1,247 @@ +"""Tests that queued execution is revoked when the owning account is deactivated +or deleted. + +Policy (see queue_owner_is_active): +- Pending items are rejected (canceled) at dequeue, before any invocation runs. +- Running items are stopped at the next node boundary; canceling also sets the + processor's cancel event, which stops step-callback nodes mid-node. +- Single-user mode and the ``system`` user are exempt. +""" + +from threading import Event as ThreadEvent +from types import SimpleNamespace +from unittest.mock import MagicMock + +import pytest + +from invokeai.app.services.session_processor.session_processor_default import ( + DefaultSessionProcessor, + DefaultSessionRunner, + queue_owner_is_active, +) +from invokeai.app.services.session_queue.session_queue_common import SessionQueueItemNotFoundError + + +def _services(multiuser: bool = True, users_by_id: dict | None = None) -> SimpleNamespace: + users_by_id = users_by_id or {} + return SimpleNamespace( + configuration=SimpleNamespace(multiuser=multiuser), + users=SimpleNamespace(get=lambda user_id: users_by_id.get(user_id)), + session_queue=MagicMock(), + logger=MagicMock(), + ) + + +def _queue_item(user_id: str = "user-1", item_id: int = 7) -> SimpleNamespace: + return SimpleNamespace(user_id=user_id, item_id=item_id) + + +def _active(user_id: str) -> SimpleNamespace: + return SimpleNamespace(user_id=user_id, is_active=True, is_admin=False) + + +def _inactive(user_id: str) -> SimpleNamespace: + return SimpleNamespace(user_id=user_id, is_active=False, is_admin=False) + + +class TestQueueOwnerIsActive: + def test_single_user_mode_is_always_active(self) -> None: + services = _services(multiuser=False) + assert queue_owner_is_active(services, _queue_item(user_id="anyone")) is True + + def test_system_user_is_always_active(self) -> None: + services = _services(multiuser=True) + assert queue_owner_is_active(services, _queue_item(user_id="system")) is True + + def test_active_user(self) -> None: + services = _services(users_by_id={"user-1": _active("user-1")}) + assert queue_owner_is_active(services, _queue_item()) is True + + def test_deactivated_user(self) -> None: + services = _services(users_by_id={"user-1": _inactive("user-1")}) + assert queue_owner_is_active(services, _queue_item()) is False + + def test_deleted_user(self) -> None: + services = _services(users_by_id={}) + assert queue_owner_is_active(services, _queue_item()) is False + + +class TestDequeueRejection: + """Items whose owner was deactivated while pending are canceled at dequeue and + never executed.""" + + def _processor(self, services: SimpleNamespace) -> DefaultSessionProcessor: + processor = DefaultSessionProcessor.__new__(DefaultSessionProcessor) + processor._invoker = SimpleNamespace(services=services) + return processor + + def test_inactive_owner_item_is_canceled(self) -> None: + services = _services(users_by_id={"user-1": _inactive("user-1")}) + processor = self._processor(services) + item = _queue_item() + + assert processor._cancel_queue_item_if_owner_inactive(item) is True + services.session_queue.cancel_queue_item.assert_called_once_with(7) + + def test_deleted_owner_item_is_canceled(self) -> None: + services = _services(users_by_id={}) + processor = self._processor(services) + + assert processor._cancel_queue_item_if_owner_inactive(_queue_item()) is True + services.session_queue.cancel_queue_item.assert_called_once() + + def test_active_owner_item_is_executed(self) -> None: + services = _services(users_by_id={"user-1": _active("user-1")}) + processor = self._processor(services) + + assert processor._cancel_queue_item_if_owner_inactive(_queue_item()) is False + services.session_queue.cancel_queue_item.assert_not_called() + + def test_system_item_is_executed(self) -> None: + services = _services(multiuser=True) + processor = self._processor(services) + + assert processor._cancel_queue_item_if_owner_inactive(_queue_item(user_id="system")) is False + services.session_queue.cancel_queue_item.assert_not_called() + + def test_missing_queue_item_does_not_raise(self) -> None: + """The item may be deleted concurrently; rejection still stands.""" + services = _services(users_by_id={}) + services.session_queue.cancel_queue_item.side_effect = SessionQueueItemNotFoundError("gone") + processor = self._processor(services) + + assert processor._cancel_queue_item_if_owner_inactive(_queue_item()) is True + + +class TestUserAccessChangedCancelsCurrentItem: + """Deactivating a user cancels their currently running queue item immediately.""" + + def _processor(self, services: SimpleNamespace, *queue_items: SimpleNamespace | None) -> DefaultSessionProcessor: + processor = DefaultSessionProcessor.__new__(DefaultSessionProcessor) + processor._invoker = SimpleNamespace(services=services) + # Each worker runs at most one item; an idle worker has `queue_item is None`. + processor._workers = [SimpleNamespace(queue_item=item) for item in queue_items] + return processor + + def _event(self, user_id: str, is_active: bool) -> tuple: + return ( + "user_access_changed", + SimpleNamespace(user_id=user_id, is_admin=False, is_active=is_active), + ) + + @pytest.mark.anyio + async def test_deactivation_cancels_owned_running_item(self) -> None: + services = _services() + processor = self._processor(services, _queue_item(user_id="user-1", item_id=11)) + + await processor._on_user_access_changed(self._event("user-1", is_active=False)) + + services.session_queue.cancel_queue_item.assert_called_once_with(11) + + @pytest.mark.anyio + async def test_deactivation_of_other_user_does_not_cancel(self) -> None: + services = _services() + processor = self._processor(services, _queue_item(user_id="user-1")) + + await processor._on_user_access_changed(self._event("user-2", is_active=False)) + + services.session_queue.cancel_queue_item.assert_not_called() + + @pytest.mark.anyio + async def test_role_change_alone_does_not_cancel(self) -> None: + services = _services() + processor = self._processor(services, _queue_item(user_id="user-1")) + + await processor._on_user_access_changed(self._event("user-1", is_active=True)) + + services.session_queue.cancel_queue_item.assert_not_called() + + @pytest.mark.anyio + async def test_no_current_item_is_a_noop(self) -> None: + services = _services() + processor = self._processor(services, None) + + await processor._on_user_access_changed(self._event("user-1", is_active=False)) + + services.session_queue.cancel_queue_item.assert_not_called() + + @pytest.mark.anyio + async def test_deactivation_cancels_items_on_every_worker(self) -> None: + """One user may occupy several workers at once; all of their items must stop.""" + services = _services() + processor = self._processor( + services, + _queue_item(user_id="user-1", item_id=11), + _queue_item(user_id="user-2", item_id=12), + None, + _queue_item(user_id="user-1", item_id=13), + ) + + await processor._on_user_access_changed(self._event("user-1", is_active=False)) + + assert sorted(c.args[0] for c in services.session_queue.cancel_queue_item.call_args_list) == [11, 13] + + +@pytest.fixture +def anyio_backend() -> str: + return "asyncio" + + +class TestRunnerStopsBetweenNodes: + """A session whose owner is deactivated mid-run stops before the next node.""" + + def _runner_with_services(self, services: SimpleNamespace) -> DefaultSessionRunner: + runner = DefaultSessionRunner() + runner.start(services=services, cancel_event=ThreadEvent(), profiler=None) + return runner + + def _multi_node_queue_item(self, nodes: list, user_id: str = "user-1") -> SimpleNamespace: + """A queue item whose session yields `nodes` then None.""" + node_iter = iter([*nodes, None]) + session = SimpleNamespace( + next=lambda: next(node_iter), + is_complete=lambda: False, + ) + return SimpleNamespace(user_id=user_id, item_id=21, status="in_progress", session=session) + + def test_deactivation_after_first_node_stops_later_nodes(self) -> None: + node1, node2 = SimpleNamespace(id="n1"), SimpleNamespace(id="n2") + # Owner is active for the first check, deactivated afterwards. + answers = iter([_active("user-1"), _inactive("user-1"), _inactive("user-1")]) + services = _services() + services.users = SimpleNamespace(get=lambda user_id: next(answers)) + runner = self._runner_with_services(services) + executed = [] + runner.run_node = lambda invocation, queue_item: executed.append(invocation.id) # type: ignore[method-assign] + queue_item = self._multi_node_queue_item([node1, node2]) + + runner._run_session_loop(queue_item) + + assert executed == ["n1"] + services.session_queue.cancel_queue_item.assert_called_once_with(21) + + def test_active_owner_runs_all_nodes(self) -> None: + node1, node2 = SimpleNamespace(id="n1"), SimpleNamespace(id="n2") + services = _services(users_by_id={"user-1": _active("user-1")}) + runner = self._runner_with_services(services) + executed = [] + runner.run_node = lambda invocation, queue_item: executed.append(invocation.id) # type: ignore[method-assign] + queue_item = self._multi_node_queue_item([node1, node2]) + + runner._run_session_loop(queue_item) + + assert executed == ["n1", "n2"] + services.session_queue.cancel_queue_item.assert_not_called() + + def test_single_user_mode_runs_all_nodes(self) -> None: + node1, node2 = SimpleNamespace(id="n1"), SimpleNamespace(id="n2") + services = _services(multiuser=False) + runner = self._runner_with_services(services) + executed = [] + runner.run_node = lambda invocation, queue_item: executed.append(invocation.id) # type: ignore[method-assign] + queue_item = self._multi_node_queue_item([node1, node2], user_id="system") + + runner._run_session_loop(queue_item) + + assert executed == ["n1", "n2"] + services.session_queue.cancel_queue_item.assert_not_called() diff --git a/tests/app/services/session_processor/test_session_processor_cancel_guard.py b/tests/app/services/session_processor/test_session_processor_cancel_guard.py index 4add666385e..88a262ec54b 100644 --- a/tests/app/services/session_processor/test_session_processor_cancel_guard.py +++ b/tests/app/services/session_processor/test_session_processor_cancel_guard.py @@ -105,7 +105,14 @@ def cancel_queue_item(self, item_id: int): processor = DefaultSessionProcessor() processor._invoker = SimpleNamespace( # type: ignore[attr-defined] - services=SimpleNamespace(session_queue=_RaceQueue(), logger=MagicMock(), image_moves=None) + services=SimpleNamespace( + session_queue=_RaceQueue(), + logger=MagicMock(), + image_moves=None, + # Single-user mode: the post-dequeue owner check short-circuits, keeping this + # test focused on the cancellation guard. + configuration=SimpleNamespace(multiuser=False), + ) ) processor._polling_interval = 0.001 processor._thread_semaphore = BoundedSemaphore(1) diff --git a/tests/app/services/shared/test_invocation_context_images.py b/tests/app/services/shared/test_invocation_context_images.py index db2d461b7e3..f14b7b8d30c 100644 --- a/tests/app/services/shared/test_invocation_context_images.py +++ b/tests/app/services/shared/test_invocation_context_images.py @@ -93,3 +93,45 @@ def test_image_read_allows_any_image_in_single_user_mode() -> None: images.get_dto("foreign-image") services.images.get_dto.assert_called_once_with("foreign-image") + + +def test_image_read_rejects_deactivated_queue_user() -> None: + """A deactivated account keeps no queue-time privileges, even for its own images.""" + images, services = _make_interface(BoardVisibility.Private) + services.users.get.return_value = MagicMock(is_admin=False, is_active=False) + services.image_records.get_user_id.return_value = "queue-user" + + with pytest.raises(PermissionError, match="not authorized"): + images.get_dto("own-image") + + services.images.get_dto.assert_not_called() + + +def test_image_read_rejects_deleted_queue_user() -> None: + images, services = _make_interface(BoardVisibility.Private) + services.users.get.return_value = None + + with pytest.raises(PermissionError, match="not authorized"): + images.get_dto("any-image") + + services.images.get_dto.assert_not_called() + + +def test_image_save_rejects_deactivated_queue_user() -> None: + """No output may be saved on behalf of a deactivated account — even uncategorized.""" + images, services = _make_interface(BoardVisibility.Private) + services.users.get.return_value = MagicMock(is_admin=False, is_active=False) + + with pytest.raises(PermissionError, match="not authorized"): + images.save(MagicMock()) + + services.images.create.assert_not_called() + + +def test_image_save_allows_active_queue_user_without_board() -> None: + images, services = _make_interface(BoardVisibility.Private) + services.users.get.return_value = MagicMock(is_admin=False, is_active=True) + + images.save(MagicMock()) + + services.images.create.assert_called_once() diff --git a/tests/app/services/shared/test_invocation_context_videos.py b/tests/app/services/shared/test_invocation_context_videos.py index 1a3e0cca1b2..3d601c58c7d 100644 --- a/tests/app/services/shared/test_invocation_context_videos.py +++ b/tests/app/services/shared/test_invocation_context_videos.py @@ -65,3 +65,35 @@ def test_video_dto_allows_foreign_shared_video() -> None: videos.get_dto("shared.mp4") services.videos.get_dto.assert_called_once_with("shared.mp4") + + +def test_video_read_rejects_deactivated_queue_user() -> None: + """A deactivated account keeps no queue-time privileges, even for its own videos.""" + videos, services = _make_interface(BoardVisibility.Private) + services.users.get.return_value = MagicMock(is_admin=False, is_active=False) + services.video_records.get_user_id.return_value = "queue-user" + + with pytest.raises(PermissionError, match="not authorized"): + videos.get_dto("own-video") + + services.videos.get_dto.assert_not_called() + + +def test_video_save_rejects_deactivated_queue_user() -> None: + """No output may be saved on behalf of a deactivated account — even uncategorized.""" + videos, services = _make_interface(BoardVisibility.Private) + services.users.get.return_value = MagicMock(is_admin=False, is_active=False) + + with pytest.raises(PermissionError, match="not authorized"): + videos.save(Path("output.mp4"), width=64, height=64, duration=1.0) + + services.videos.create.assert_not_called() + + +def test_video_save_allows_active_queue_user_without_board() -> None: + videos, services = _make_interface(BoardVisibility.Private) + services.users.get.return_value = MagicMock(is_admin=False, is_active=True) + + videos.save(Path("output.mp4"), width=64, height=64, duration=1.0) + + services.videos.create.assert_called_once() diff --git a/tests/app/test_socket_privilege_revocation.py b/tests/app/test_socket_privilege_revocation.py new file mode 100644 index 00000000000..61981b00142 --- /dev/null +++ b/tests/app/test_socket_privilege_revocation.py @@ -0,0 +1,195 @@ +"""Tests that socket connections lose (or gain) privileges when the backing user +record changes. + +Socket room membership is established at connect time. Without live re-authorization, +a demoted administrator's sockets would keep receiving other users' private events via +the admin room, and a deactivated user's sockets would keep receiving events +indefinitely; a demoted admin could also reconnect with an old token and rejoin the +admin room. +""" + +from types import SimpleNamespace +from unittest.mock import AsyncMock + +import pytest +from fastapi import FastAPI + +from invokeai.app.api.sockets import SocketIO +from invokeai.app.services.events.events_common import UserAccessChangedEvent + + +@pytest.fixture +def anyio_backend() -> str: + return "asyncio" + + +def _patch_multiuser_context( + monkeypatch: pytest.MonkeyPatch, + *, + user_id: str, + token_is_admin: bool, + db_is_admin: bool, + db_is_active: bool = True, +) -> None: + """Multiuser context where the token's claims and the database record can differ.""" + user = SimpleNamespace(user_id=user_id, is_active=db_is_active, is_admin=db_is_admin) + invoker = SimpleNamespace( + services=SimpleNamespace( + configuration=SimpleNamespace(multiuser=True), + users=SimpleNamespace(get=lambda candidate_user_id: user if candidate_user_id == user_id else None), + ) + ) + monkeypatch.setattr("invokeai.app.api.dependencies.ApiDependencies", SimpleNamespace(invoker=invoker)) + monkeypatch.setattr( + "invokeai.app.api.sockets.verify_token", + lambda token: SimpleNamespace(user_id=user_id, is_admin=token_is_admin) if token == "valid-token" else None, + ) + + +class TestConnectDerivesRoleFromDatabase: + """_handle_connect must trust the database record, not the token's is_admin claim.""" + + @pytest.mark.anyio + async def test_demoted_admin_reconnecting_with_old_token_does_not_join_admin_room( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + socketio = SocketIO(FastAPI()) + socketio._sio.enter_room = AsyncMock() + _patch_multiuser_context(monkeypatch, user_id="user-1", token_is_admin=True, db_is_admin=False) + + accepted = await socketio._handle_connect("sid-1", {}, {"token": "valid-token"}) + + assert accepted is True + rooms_entered = [call.args[1] for call in socketio._sio.enter_room.await_args_list] + assert "admin" not in rooms_entered + assert socketio._socket_users["sid-1"]["is_admin"] is False + + @pytest.mark.anyio + async def test_promoted_user_connecting_with_old_token_joins_admin_room( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + socketio = SocketIO(FastAPI()) + socketio._sio.enter_room = AsyncMock() + _patch_multiuser_context(monkeypatch, user_id="user-1", token_is_admin=False, db_is_admin=True) + + accepted = await socketio._handle_connect("sid-1", {}, {"token": "valid-token"}) + + assert accepted is True + rooms_entered = [call.args[1] for call in socketio._sio.enter_room.await_args_list] + assert "admin" in rooms_entered + assert socketio._socket_users["sid-1"]["is_admin"] is True + + @pytest.mark.anyio + async def test_deactivated_user_cannot_reconnect_with_old_token(self, monkeypatch: pytest.MonkeyPatch) -> None: + socketio = SocketIO(FastAPI()) + socketio._sio.enter_room = AsyncMock() + _patch_multiuser_context( + monkeypatch, user_id="user-1", token_is_admin=False, db_is_admin=False, db_is_active=False + ) + + accepted = await socketio._handle_connect("sid-1", {}, {"token": "valid-token"}) + + assert accepted is False + assert "sid-1" not in socketio._socket_users + + @pytest.mark.anyio + async def test_active_admin_still_joins_admin_room(self, monkeypatch: pytest.MonkeyPatch) -> None: + """Positive case: an unchanged administrator keeps full admin connectivity.""" + socketio = SocketIO(FastAPI()) + socketio._sio.enter_room = AsyncMock() + _patch_multiuser_context(monkeypatch, user_id="admin-1", token_is_admin=True, db_is_admin=True) + + accepted = await socketio._handle_connect("sid-1", {}, {"token": "valid-token"}) + + assert accepted is True + rooms_entered = [call.args[1] for call in socketio._sio.enter_room.await_args_list] + assert "admin" in rooms_entered + + +class TestUserAccessChangedHandler: + """_handle_user_access_changed re-authorizes already-connected sockets.""" + + def _connected_socketio(self) -> SocketIO: + socketio = SocketIO(FastAPI()) + socketio._sio.enter_room = AsyncMock() + socketio._sio.leave_room = AsyncMock() + socketio._sio.disconnect = AsyncMock() + socketio._socket_users = { + "sid-admin": {"user_id": "admin-1", "is_admin": True}, + "sid-user-a": {"user_id": "user-1", "is_admin": False}, + "sid-user-b": {"user_id": "user-1", "is_admin": False}, + "sid-other": {"user_id": "user-2", "is_admin": False}, + } + return socketio + + @pytest.mark.anyio + async def test_demoted_admin_sockets_leave_admin_room(self) -> None: + socketio = self._connected_socketio() + event = UserAccessChangedEvent.build(user_id="admin-1", is_admin=False, is_active=True) + + await socketio._handle_user_access_changed(("user_access_changed", event)) + + socketio._sio.leave_room.assert_awaited_once_with("sid-admin", "admin") + assert socketio._socket_users["sid-admin"]["is_admin"] is False + socketio._sio.disconnect.assert_not_awaited() + + @pytest.mark.anyio + async def test_demoted_admin_cannot_rejoin_admin_room_via_queue_subscription(self) -> None: + """After demotion, the cached is_admin is False, so _handle_sub_queue does not + re-add the socket to the admin room.""" + socketio = self._connected_socketio() + event = UserAccessChangedEvent.build(user_id="admin-1", is_admin=False, is_active=True) + await socketio._handle_user_access_changed(("user_access_changed", event)) + + await socketio._handle_sub_queue("sid-admin", {"queue_id": "default"}) + + rooms_entered = [call.args[1] for call in socketio._sio.enter_room.await_args_list] + assert "admin" not in rooms_entered + + @pytest.mark.anyio + async def test_deactivated_user_sockets_are_disconnected(self) -> None: + socketio = self._connected_socketio() + event = UserAccessChangedEvent.build(user_id="user-1", is_admin=False, is_active=False) + + await socketio._handle_user_access_changed(("user_access_changed", event)) + + disconnected = {call.args[0] for call in socketio._sio.disconnect.await_args_list} + assert disconnected == {"sid-user-a", "sid-user-b"} + + @pytest.mark.anyio + async def test_deleted_user_sockets_are_disconnected(self) -> None: + """Deletion is emitted as is_active=False and disconnects the user's sockets.""" + socketio = self._connected_socketio() + event = UserAccessChangedEvent.build(user_id="user-2", is_admin=False, is_active=False) + + await socketio._handle_user_access_changed(("user_access_changed", event)) + + disconnected = {call.args[0] for call in socketio._sio.disconnect.await_args_list} + assert disconnected == {"sid-other"} + + @pytest.mark.anyio + async def test_promoted_user_sockets_join_admin_room(self) -> None: + socketio = self._connected_socketio() + event = UserAccessChangedEvent.build(user_id="user-1", is_admin=True, is_active=True) + + await socketio._handle_user_access_changed(("user_access_changed", event)) + + rooms_entered = [(call.args[0], call.args[1]) for call in socketio._sio.enter_room.await_args_list] + assert ("sid-user-a", "admin") in rooms_entered + assert ("sid-user-b", "admin") in rooms_entered + assert socketio._socket_users["sid-user-a"]["is_admin"] is True + + @pytest.mark.anyio + async def test_other_users_sockets_are_untouched(self) -> None: + """Positive case: an access change for one user does not affect other users' + sockets — an unchanged administrator keeps receiving admin-room events.""" + socketio = self._connected_socketio() + event = UserAccessChangedEvent.build(user_id="user-1", is_admin=False, is_active=False) + + await socketio._handle_user_access_changed(("user_access_changed", event)) + + disconnected = {call.args[0] for call in socketio._sio.disconnect.await_args_list} + assert "sid-admin" not in disconnected + assert "sid-other" not in disconnected + socketio._sio.leave_room.assert_not_awaited() + assert socketio._socket_users["sid-admin"]["is_admin"] is True diff --git a/tests/app/test_workflow_socketio.py b/tests/app/test_workflow_socketio.py index ba317e35886..3d227256c6e 100644 --- a/tests/app/test_workflow_socketio.py +++ b/tests/app/test_workflow_socketio.py @@ -13,7 +13,9 @@ def anyio_backend() -> str: def _patch_multiuser_context(monkeypatch: pytest.MonkeyPatch, *, user_id: str, is_admin: bool) -> None: - user = SimpleNamespace(user_id=user_id, is_active=True) + # The connect handler derives is_admin from the database record, not the token, + # so the mocked user record carries the role. + user = SimpleNamespace(user_id=user_id, is_active=True, is_admin=is_admin) invoker = SimpleNamespace( services=SimpleNamespace( configuration=SimpleNamespace(multiuser=True), From 6b75acf645053cfdd904fe87b2479e4dbf049026 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sun, 19 Jul 2026 21:33:08 -0400 Subject: [PATCH 02/19] fix(events): keep server-internal events out of the API schema; fix admin 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) --- invokeai/app/services/events/events_common.py | 16 +++++++++++++--- tests/app/routers/test_app_info.py | 16 +++++++++++++--- tests/app/services/events/__init__.py | 0 .../app/services/events/test_events_common.py | 18 ++++++++++++++++++ 4 files changed, 44 insertions(+), 6 deletions(-) create mode 100644 tests/app/services/events/__init__.py create mode 100644 tests/app/services/events/test_events_common.py diff --git a/invokeai/app/services/events/events_common.py b/invokeai/app/services/events/events_common.py index 33065525cf1..1ed2a93a3c2 100644 --- a/invokeai/app/services/events/events_common.py +++ b/invokeai/app/services/events/events_common.py @@ -32,21 +32,29 @@ class EventBase(BaseModel): All other attributes should be defined as normal for a pydantic model. A timestamp is automatically added to the event when it is created. + + Events that are dispatched only within the server and never reach clients should set + `__server_internal__ = True` to keep themselves out of the generated API schema. """ __event_name__: ClassVar[str] + __server_internal__: ClassVar[bool] = False timestamp: int = Field(description="The timestamp of the event", default_factory=get_timestamp) model_config = ConfigDict(json_schema_serialization_defaults_required=True) @classmethod def get_events(cls) -> set[type["EventBase"]]: - """Get a set of all event models.""" + """Get a set of all client-facing event models. + + Consumed by the OpenAPI generator, so server-internal events are excluded — they + are not part of the client API surface. + """ event_subclasses: set[type["EventBase"]] = set() for subclass in cls.__subclasses__(): # We only want to include subclasses that are event models, not intermediary classes - if hasattr(subclass, "__event_name__"): + if hasattr(subclass, "__event_name__") and not subclass.__server_internal__: event_subclasses.add(subclass) event_subclasses.update(subclass.get_events()) @@ -879,10 +887,12 @@ class UserAccessChangedEvent(EventBase): immediately instead of trusting connect-time claims until reconnect. This event is server-internal: it is deliberately NOT registered with - `payload_schema` and is never emitted to clients. + `payload_schema`, is excluded from the generated API schema, and is never + emitted to clients. """ __event_name__ = "user_access_changed" + __server_internal__ = True user_id: str = Field(description="The ID of the affected user") is_admin: bool = Field(description="Whether the user currently has admin privileges") diff --git a/tests/app/routers/test_app_info.py b/tests/app/routers/test_app_info.py index d2fd5ad4bb6..afc7e323777 100644 --- a/tests/app/routers/test_app_info.py +++ b/tests/app/routers/test_app_info.py @@ -31,6 +31,16 @@ def __init__(self, invoker: Invoker) -> None: self.invoker = invoker +def _non_admin_user() -> Mock: + """An active, non-admin user record. + + Authorization is derived from the database record on every request, so the + stubbed user must carry concrete field values — a bare ``Mock`` fails + ``TokenData`` validation. + """ + return Mock(user_id="user-1", email="user@example.com", is_admin=False, is_active=True) + + def test_get_external_provider_statuses(monkeypatch: Any, mock_invoker: Invoker, client: TestClient) -> None: statuses = { "gemini": ExternalProviderStatus(provider_id="gemini", configured=True, message=None), @@ -396,7 +406,7 @@ def test_update_runtime_config_rejects_non_admin_users( "invokeai.app.api.auth_dependencies.verify_token", lambda _: TokenData(user_id="user-1", email="user@example.com", is_admin=False), ) - monkeypatch.setattr(mock_invoker.services.users, "get", Mock(return_value=Mock(is_active=True))) + monkeypatch.setattr(mock_invoker.services.users, "get", Mock(return_value=_non_admin_user())) response = client.patch( "/api/v1/app/runtime_config", @@ -418,7 +428,7 @@ def test_set_external_provider_config_rejects_non_admin_users( "invokeai.app.api.auth_dependencies.verify_token", lambda _: TokenData(user_id="user-1", email="user@example.com", is_admin=False), ) - monkeypatch.setattr(mock_invoker.services.users, "get", Mock(return_value=Mock(is_active=True))) + monkeypatch.setattr(mock_invoker.services.users, "get", Mock(return_value=_non_admin_user())) response = client.post( f"/api/v1/app/external_providers/config/{provider_id}", @@ -440,7 +450,7 @@ def test_reset_external_provider_config_rejects_non_admin_users( "invokeai.app.api.auth_dependencies.verify_token", lambda _: TokenData(user_id="user-1", email="user@example.com", is_admin=False), ) - monkeypatch.setattr(mock_invoker.services.users, "get", Mock(return_value=Mock(is_active=True))) + monkeypatch.setattr(mock_invoker.services.users, "get", Mock(return_value=_non_admin_user())) response = client.delete( f"/api/v1/app/external_providers/config/{provider_id}", diff --git a/tests/app/services/events/__init__.py b/tests/app/services/events/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/app/services/events/test_events_common.py b/tests/app/services/events/test_events_common.py new file mode 100644 index 00000000000..0cfd5a0fa4c --- /dev/null +++ b/tests/app/services/events/test_events_common.py @@ -0,0 +1,18 @@ +from invokeai.app.services.events.events_common import EventBase, UserAccessChangedEvent + + +def test_get_events_excludes_server_internal_events() -> None: + """Server-internal events must stay out of the generated client API schema. + + `EventBase.get_events()` feeds the OpenAPI generator, so any event it returns ends up + in `openapi.json`/`schema.ts`. `UserAccessChangedEvent` is dispatched only between + server components and is never delivered to clients. + """ + assert UserAccessChangedEvent not in EventBase.get_events() + + +def test_get_events_includes_client_facing_events() -> None: + event_names = {event.__event_name__ for event in EventBase.get_events()} + + assert "invocation_complete" in event_names + assert "queue_item_status_changed" in event_names From 742e863a7c981ea2dd53dfbabbbd3530e89306e5 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Fri, 31 Jul 2026 10:27:32 -0400 Subject: [PATCH 03/19] feat(auth): invalidate tokens on password change via a revocation epoch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- invokeai/app/api/auth_dependencies.py | 57 +++++-- invokeai/app/api/routers/auth.py | 80 ++++++++- invokeai/app/api/sockets.py | 27 ++- invokeai/app/api_app.py | 13 +- invokeai/app/services/auth/token_service.py | 6 + invokeai/app/services/events/events_base.py | 13 +- invokeai/app/services/events/events_common.py | 8 +- ...gration_2026_07_31_add_user_token_epoch.py | 41 +++++ invokeai/app/services/users/users_common.py | 3 + invokeai/app/services/users/users_default.py | 18 +- invokeai/frontend/web/openapi.json | 10 +- .../frontend/web/src/services/api/schema.ts | 17 ++ tests/app/api/test_sliding_window_token.py | 9 +- tests/app/api/test_video_upload_limits.py | 4 +- tests/app/routers/test_app_info.py | 2 +- .../routers/test_multiuser_authorization.py | 4 + .../app/routers/test_privilege_revocation.py | 158 ++++++++++++++++++ tests/app/services/auth/test_performance.py | 3 +- tests/app/services/users/test_user_service.py | 3 +- tests/app/test_socket_privilege_revocation.py | 76 ++++++++- tests/app/test_workflow_socketio.py | 9 +- 21 files changed, 511 insertions(+), 50 deletions(-) create mode 100644 invokeai/app/services/shared/sqlite_migrator/migrations/migration_2026_07_31_add_user_token_epoch.py diff --git a/invokeai/app/api/auth_dependencies.py b/invokeai/app/api/auth_dependencies.py index b5bf67d0606..3b19588faf0 100644 --- a/invokeai/app/api/auth_dependencies.py +++ b/invokeai/app/api/auth_dependencies.py @@ -17,6 +17,39 @@ # HTTP Bearer token security scheme security = HTTPBearer(auto_error=False) MEDIA_TOKEN_COOKIE = "invokeai_media_token" +# Deliberately indistinguishable from an ordinary expiry to a client: a token that fails +# the epoch check is simply no longer valid, and saying *why* would tell a holder of a +# stolen token that the account's password was just rotated. +TOKEN_REVOKED_DETAIL = "Invalid or expired authentication token" + + +def resolve_authorized_user(token_data: TokenData) -> "UserDTO | None": + """Return the account a verified token still grants access to, or None. + + This is the single place that decides whether a syntactically valid token is still + honored, and every authenticated entry point must go through it: the REST + dependencies below, the Socket.IO handshake, and the video-upload ASGI gate. Keeping + the rules in one function is deliberate — they were previously repeated at each call + site, and a check added to some copies but not others is indistinguishable from no + check at all on the paths that were missed. + + A token is honored when all three hold: + + - the account still exists, + - it is active, + - and the token carries the account's current revocation epoch. Any mismatch counts + as revoked: the token was not issued from the record as it now stands. Tokens + predating the claim decode to 0 and so remain valid against a record that has never + been bumped, which is why upgrading logs nobody out. + + Raises whatever the user service raises; callers that must fail closed should catch. + """ + user = ApiDependencies.invoker.services.users.get(token_data.user_id) + if user is None or not user.is_active: + return None + if token_data.token_epoch != user.token_epoch: + return None + return user def _validate_token(token: str, invalid_detail: str) -> TokenData: @@ -24,8 +57,8 @@ def _validate_token(token: str, invalid_detail: str) -> TokenData: if token_data is None: raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail=invalid_detail) - user = ApiDependencies.invoker.services.users.get(token_data.user_id) - if user is None or not user.is_active: + user = resolve_authorized_user(token_data) + if user is None: raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="User not found or inactive") return _db_derived_token_data(token_data, user) @@ -38,12 +71,16 @@ def _db_derived_token_data(token_data: TokenData, user: "UserDTO") -> TokenData: keeps admin rights until their token expires — and sliding-window refresh would renew that stale claim indefinitely. A promoted user symmetrically gains admin rights on their next request without re-login. + + The epoch is carried through from the record so a refreshed token stays valid + (callers only reach here once ``_token_epoch_is_current`` has passed). """ return TokenData( user_id=user.user_id, email=user.email, is_admin=user.is_admin, remember_me=token_data.remember_me, + token_epoch=user.token_epoch, ) @@ -82,11 +119,10 @@ async def get_current_user( headers={"WWW-Authenticate": "Bearer"}, ) - # Verify user still exists and is active - user_service = ApiDependencies.invoker.services.users - user = user_service.get(token_data.user_id) + # Verify the token still grants access: user exists, is active, epoch is current. + user = resolve_authorized_user(token_data) - if user is None or not user.is_active: + if user is None: raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="User account is inactive or does not exist", @@ -137,12 +173,11 @@ async def get_current_user_or_default( # Invalid token in multiuser mode - reject raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid or expired token") - # Verify user still exists and is active - user_service = ApiDependencies.invoker.services.users - user = user_service.get(token_data.user_id) + # Verify the token still grants access: user exists, is active, epoch is current. + user = resolve_authorized_user(token_data) - if user is None or not user.is_active: - # User doesn't exist or is inactive in multiuser mode - reject + if user is None: + # Missing, inactive, or revoked in multiuser mode - reject raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="User not found or inactive") return _db_derived_token_data(token_data, user) diff --git a/invokeai/app/api/routers/auth.py b/invokeai/app/api/routers/auth.py index 0c85862f155..fc87f3e4ca3 100644 --- a/invokeai/app/api/routers/auth.py +++ b/invokeai/app/api/routers/auth.py @@ -36,6 +36,31 @@ TOKEN_EXPIRATION_REMEMBER_ME = 7 # 7 days for "remember me" login +def _issue_replacement_token(http_request: Request, response: Response, user: UserDTO, remember_me: bool) -> None: + """Hand the caller a token minted under the user's *current* revocation epoch. + + A password change bumps the epoch, which kills every token issued before it — + including the one that authenticated the request making the change. Without a + replacement, changing a password would sign the caller out of their own session, and + the sliding-window middleware cannot fill the gap: it correctly refuses to refresh a + token whose epoch is already stale. It does leave an already-set header alone, so + what we write here survives. + """ + expires_delta = timedelta(days=TOKEN_EXPIRATION_REMEMBER_ME if remember_me else TOKEN_EXPIRATION_NORMAL) + replacement = create_access_token( + TokenData( + user_id=user.user_id, + email=user.email, + is_admin=user.is_admin, + remember_me=remember_me, + token_epoch=user.token_epoch, + ), + expires_delta, + ) + response.headers["X-Refreshed-Token"] = replacement + _set_media_cookie(http_request, response, replacement, int(expires_delta.total_seconds())) + + class LoginRequest(BaseModel): """Request body for user login.""" @@ -211,6 +236,7 @@ async def login( email=user.email, is_admin=user.is_admin, remember_me=login_request.remember_me, + token_epoch=user.token_epoch, ) token = create_access_token(token_data, expires_delta) _set_media_cookie(request, response, token, int(expires_delta.total_seconds())) @@ -517,9 +543,15 @@ async def update_user( user_id: Annotated[str, Path(description="User ID")], request: Annotated[AdminUserUpdateRequest, Body(description="User fields to update")], current_user: AdminUser, + http_request: Request, + response: Response, ) -> UserDTO: """Update a user. Requires admin privileges. + Resetting a password revokes the target's existing sessions. An admin resetting + their own password receives a replacement token in ``X-Refreshed-Token`` so they + are not signed out by their own action. + Args: user_id: The user ID request: Fields to update @@ -566,11 +598,27 @@ async def update_user( # Authorization state changed — notify live connections (open sockets, the # session processor) so demotion/deactivation takes effect immediately - # instead of persisting until reconnect or token expiry. - if before is not None and (before.is_admin != updated.is_admin or before.is_active != updated.is_active): + # instead of persisting until reconnect or token expiry. A password reset bumps + # the epoch without touching is_admin/is_active, and must drop the target's open + # sockets too, so it is part of this condition. + if before is not None and ( + before.is_admin != updated.is_admin + or before.is_active != updated.is_active + or before.token_epoch != updated.token_epoch + ): ApiDependencies.invoker.services.events.emit_user_access_changed( - user_id=updated.user_id, is_admin=updated.is_admin, is_active=updated.is_active + user_id=updated.user_id, + is_admin=updated.is_admin, + is_active=updated.is_active, + token_epoch=updated.token_epoch, ) + + # An admin resetting their *own* password would otherwise lock themselves out: the + # epoch bump kills the token that authenticated this request, and the sliding-window + # middleware correctly refuses to refresh a revoked one. Mirror what /auth/me does. + if request.password is not None and updated.user_id == current_user.user_id: + _issue_replacement_token(http_request, response, updated, current_user.remember_me) + return updated @@ -616,15 +664,24 @@ async def delete_user( async def update_current_user( request: Annotated[UserProfileUpdateRequest, Body(description="Profile fields to update")], current_user: CurrentUser, + http_request: Request, + response: Response, ) -> UserDTO: """Update the current user's own profile. To change the password, both ``current_password`` and ``new_password`` must be provided. The current password is verified before the change is applied. + A password change signs out the account's *other* sessions: it bumps the + revocation epoch, invalidating every previously issued token. This response + carries a replacement token in ``X-Refreshed-Token`` so the caller stays + signed in. + Args: request: Profile fields to update current_user: The authenticated user + http_request: The HTTP request, used to scope the replacement media cookie + response: The HTTP response, used to return the replacement token Returns: The updated user @@ -661,8 +718,23 @@ async def update_current_user( display_name=request.display_name, password=request.new_password, ) - return user_service.update( + updated = user_service.update( current_user.user_id, changes, strict_password_checking=config.strict_password_checking ) except ValueError as e: raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) from e + + if request.new_password is not None: + # Drop the account's other live sockets. They authenticated under the superseded + # epoch and would otherwise keep streaming this user's events even though every + # HTTP request from those sessions is now rejected. The account stays active, so + # the epoch — not is_active — is what marks them. + ApiDependencies.invoker.services.events.emit_user_access_changed( + user_id=updated.user_id, + is_admin=updated.is_admin, + is_active=updated.is_active, + token_epoch=updated.token_epoch, + ) + _issue_replacement_token(http_request, response, updated, current_user.remember_me) + + return updated diff --git a/invokeai/app/api/sockets.py b/invokeai/app/api/sockets.py index 753ffa93c22..698736b1860 100644 --- a/invokeai/app/api/sockets.py +++ b/invokeai/app/api/sockets.py @@ -177,28 +177,35 @@ async def _handle_connect(self, sid: str, environ: dict, auth: dict | None) -> b # exists and is active — mirrors the REST auth check in # auth_dependencies.py. A deleted or deactivated user whose # JWT has not yet expired must not be allowed to open a socket. + token_epoch = token_data.token_epoch if self._is_multiuser_enabled(): try: - from invokeai.app.api.dependencies import ApiDependencies + from invokeai.app.api.auth_dependencies import resolve_authorized_user - user = ApiDependencies.invoker.services.users.get(token_data.user_id) - if user is None or not user.is_active: - logger.warning(f"Rejecting socket {sid}: user {token_data.user_id} not found or inactive") + user = resolve_authorized_user(token_data) + if user is None: + logger.warning( + f"Rejecting socket {sid}: user {token_data.user_id} not found, inactive, or revoked" + ) return False # The token proves identity only — authorization comes from the # database. A demoted admin reconnecting with an old token must # not rejoin the admin room; a promoted user gets admin rooms # without re-login. is_admin = user.is_admin + token_epoch = user.token_epoch except Exception: # If user service is unavailable, fail closed logger.warning(f"Rejecting socket {sid}: unable to verify user record") return False - # Store user_id and is_admin in socket users dict + # Store user_id, is_admin and the epoch this socket authenticated under. + # The epoch lets `_handle_user_access_changed` drop exactly the sockets + # whose token a later revocation invalidated. self._socket_users[sid] = { "user_id": token_data.user_id, "is_admin": is_admin, + "token_epoch": token_epoch, } logger.info(f"Socket {sid} connected with user_id: {token_data.user_id}, is_admin: {is_admin}") await self._sio.enter_room(sid, f"user:{token_data.user_id}") @@ -258,6 +265,12 @@ async def _handle_user_access_changed(self, event: FastAPIEvent[UserAccessChange - Deactivated/deleted: disconnect every socket belonging to the user. A reconnect attempt with the old token is rejected by ``_handle_connect``'s database check. + - Sessions revoked (password change): disconnect the sockets that authenticated + under a superseded token epoch. Without this the account stays active, so the + branch above does not fire and revoked sessions keep streaming events from an + already-open socket — HTTP would be locked out while the socket was not. The + session that performed the change reconnects with its replacement token; the + others fail ``_handle_connect`` and stay out. - Demoted: leave the admin room and update the cached ``is_admin`` so ``_handle_sub_queue`` cannot re-add it. - Promoted: join the admin room, matching the DB-derived REST behavior. @@ -269,6 +282,10 @@ async def _handle_user_access_changed(self, event: FastAPIEvent[UserAccessChange logger.info(f"Disconnecting socket {sid}: user {event_data.user_id} deactivated or deleted") await self._sio.disconnect(sid) continue + if self._socket_users[sid].get("token_epoch", 0) != event_data.token_epoch: + logger.info(f"Disconnecting socket {sid}: user {event_data.user_id} revoked its earlier sessions") + await self._sio.disconnect(sid) + continue self._socket_users[sid]["is_admin"] = event_data.is_admin if event_data.is_admin: await self._sio.enter_room(sid, "admin") diff --git a/invokeai/app/api_app.py b/invokeai/app/api_app.py index f6e247e6f8b..a79aed667fb 100644 --- a/invokeai/app/api_app.py +++ b/invokeai/app/api_app.py @@ -143,6 +143,13 @@ async def dispatch(self, request: Request, call_next: RequestResponseEndpoint): user = await run_in_threadpool(ApiDependencies.invoker.services.users.get, token_data.user_id) if user is None or not user.is_active: return response + # Never refresh a revoked token. This runs after the route, so an + # authenticated route has already rejected it — but an unauthenticated + # route returning 2xx with a stale Bearer header still reaches here, + # and minting from the current record would launder the revoked token + # into a valid one. + if token_data.token_epoch != user.token_epoch: + return response # Use the remember_me claim from the token to determine the # correct refresh duration. This avoids the bug where a 7-day # token with <24h remaining would be silently downgraded to 1 day. @@ -156,6 +163,7 @@ async def dispatch(self, request: Request, call_next: RequestResponseEndpoint): email=user.email, is_admin=user.is_admin, remember_me=token_data.remember_me, + token_epoch=user.token_epoch, ) new_token = create_access_token(refreshed_data, expires_delta) response.headers["X-Refreshed-Token"] = new_token @@ -204,8 +212,9 @@ def _identify_video_upload_user(scope: Scope) -> tuple[bool, str | None]: token_data = verify_token(token) if token_data is None: return False, None - user = ApiDependencies.invoker.services.users.get(token_data.user_id) - if user is None or not user.is_active: + from invokeai.app.api.auth_dependencies import resolve_authorized_user + + if resolve_authorized_user(token_data) is None: return False, None return True, token_data.user_id diff --git a/invokeai/app/services/auth/token_service.py b/invokeai/app/services/auth/token_service.py index 1a5c0747388..2812dfeb023 100644 --- a/invokeai/app/services/auth/token_service.py +++ b/invokeai/app/services/auth/token_service.py @@ -22,6 +22,12 @@ class TokenData(BaseModel): email: str is_admin: bool remember_me: bool = False + # Revocation epoch copied from the user record when the token was minted. A token + # whose epoch no longer matches the record is rejected, which is how a password + # change invalidates sessions a JWT would otherwise keep alive until expiry. + # Defaults to 0 so tokens issued before this claim existed keep working against + # records that have never been bumped. + token_epoch: int = 0 def set_jwt_secret(secret: str) -> None: diff --git a/invokeai/app/services/events/events_base.py b/invokeai/app/services/events/events_base.py index 5ad7de3a871..ac3b6e21cf9 100644 --- a/invokeai/app/services/events/events_base.py +++ b/invokeai/app/services/events/events_base.py @@ -167,9 +167,16 @@ def emit_workflow_deleted(self, workflow_id: str, user_id: str, is_public: bool) # region User accounts - def emit_user_access_changed(self, user_id: str, is_admin: bool, is_active: bool) -> None: - """Emitted when a user's role or active status changes (server-internal; never sent to clients).""" - self.dispatch(UserAccessChangedEvent.build(user_id=user_id, is_admin=is_admin, is_active=is_active)) + def emit_user_access_changed(self, user_id: str, is_admin: bool, is_active: bool, token_epoch: int = 0) -> None: + """Emitted when a user's role, active status, or token epoch changes. + + Server-internal; never sent to clients. + """ + self.dispatch( + UserAccessChangedEvent.build( + user_id=user_id, is_admin=is_admin, is_active=is_active, token_epoch=token_epoch + ) + ) # endregion diff --git a/invokeai/app/services/events/events_common.py b/invokeai/app/services/events/events_common.py index 1ed2a93a3c2..275e45dc41e 100644 --- a/invokeai/app/services/events/events_common.py +++ b/invokeai/app/services/events/events_common.py @@ -897,7 +897,11 @@ class UserAccessChangedEvent(EventBase): user_id: str = Field(description="The ID of the affected user") is_admin: bool = Field(description="Whether the user currently has admin privileges") is_active: bool = Field(description="Whether the user account is currently active (False for deleted users)") + token_epoch: int = Field( + default=0, + description="The user's current token revocation epoch; sockets that authenticated under an older one are dropped", + ) @classmethod - def build(cls, user_id: str, is_admin: bool, is_active: bool) -> "UserAccessChangedEvent": - return cls(user_id=user_id, is_admin=is_admin, is_active=is_active) + def build(cls, user_id: str, is_admin: bool, is_active: bool, token_epoch: int = 0) -> "UserAccessChangedEvent": + return cls(user_id=user_id, is_admin=is_admin, is_active=is_active, token_epoch=token_epoch) diff --git a/invokeai/app/services/shared/sqlite_migrator/migrations/migration_2026_07_31_add_user_token_epoch.py b/invokeai/app/services/shared/sqlite_migrator/migrations/migration_2026_07_31_add_user_token_epoch.py new file mode 100644 index 00000000000..fe6d3279eb5 --- /dev/null +++ b/invokeai/app/services/shared/sqlite_migrator/migrations/migration_2026_07_31_add_user_token_epoch.py @@ -0,0 +1,41 @@ +"""Add a token revocation epoch to the users table. + +A JWT proves identity and is otherwise 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 the +sliding-window middleware renewed it indefinitely while the account stayed active. + +``token_epoch`` closes that: it is stamped into every minted token and compared +against the user record on every authenticated request. Bumping it invalidates all +tokens issued before the bump, which is the general "revoke everything issued so +far" primitive the auth layer was missing. + +Existing rows and existing tokens both start at 0, so upgrading does not log anyone +out — only an actual bump revokes. +""" + +import sqlite3 + +from invokeai.app.services.shared.sqlite_migrator.sqlite_migrator_common import Migration + + +class AddUserTokenEpochCallback: + def __call__(self, cursor: sqlite3.Cursor) -> None: + cursor.execute("PRAGMA table_info(users);") + existing_columns = {row[1] for row in cursor.fetchall()} + if "token_epoch" not in existing_columns: + cursor.execute("ALTER TABLE users ADD COLUMN token_epoch INTEGER NOT NULL DEFAULT 0;") + + +def build_migration() -> Migration: + """Add ``users.token_epoch``. + + Depends on migration_27, which creates the users table. + """ + return Migration( + id="2026_07_31_add_user_token_epoch", + depends_on="migration_27", + callback=AddUserTokenEpochCallback(), + ) diff --git a/invokeai/app/services/users/users_common.py b/invokeai/app/services/users/users_common.py index c13150a3369..c6ede1e24bd 100644 --- a/invokeai/app/services/users/users_common.py +++ b/invokeai/app/services/users/users_common.py @@ -82,6 +82,9 @@ class UserDTO(BaseModel): created_at: datetime = Field(description="When the user was created") updated_at: datetime = Field(description="When the user was last updated") last_login_at: datetime | None = Field(default=None, description="When user last logged in") + token_epoch: int = Field( + default=0, description="Revocation epoch; tokens minted before the current value are rejected" + ) @field_validator("email") @classmethod diff --git a/invokeai/app/services/users/users_default.py b/invokeai/app/services/users/users_default.py index 6e472882124..8253949505d 100644 --- a/invokeai/app/services/users/users_default.py +++ b/invokeai/app/services/users/users_default.py @@ -60,7 +60,7 @@ def get(self, user_id: str) -> UserDTO | None: with self._db.transaction() as cursor: cursor.execute( """ - SELECT user_id, email, display_name, is_admin, is_active, created_at, updated_at, last_login_at + SELECT user_id, email, display_name, is_admin, is_active, created_at, updated_at, last_login_at, token_epoch FROM users WHERE user_id = ? """, @@ -80,6 +80,7 @@ def get(self, user_id: str) -> UserDTO | None: created_at=datetime.fromisoformat(row[5]), updated_at=datetime.fromisoformat(row[6]), last_login_at=datetime.fromisoformat(row[7]) if row[7] else None, + token_epoch=int(row[8]), ) def get_by_email(self, email: str) -> UserDTO | None: @@ -87,7 +88,7 @@ def get_by_email(self, email: str) -> UserDTO | None: with self._db.transaction() as cursor: cursor.execute( """ - SELECT user_id, email, display_name, is_admin, is_active, created_at, updated_at, last_login_at + SELECT user_id, email, display_name, is_admin, is_active, created_at, updated_at, last_login_at, token_epoch FROM users WHERE email = ? """, @@ -107,6 +108,7 @@ def get_by_email(self, email: str) -> UserDTO | None: created_at=datetime.fromisoformat(row[5]), updated_at=datetime.fromisoformat(row[6]), last_login_at=datetime.fromisoformat(row[7]) if row[7] else None, + token_epoch=int(row[8]), ) def update(self, user_id: str, changes: UserUpdateRequest, strict_password_checking: bool = True) -> UserDTO: @@ -136,6 +138,11 @@ def update(self, user_id: str, changes: UserUpdateRequest, strict_password_check if changes.password is not None: updates.append("password_hash = ?") params.append(hash_password(changes.password)) + # Rotating the password revokes every token issued under the old one. A JWT is + # self-contained, so without this a stolen token survives the password change + # meant to evict the thief — and sliding-window refresh renews it indefinitely. + # Computed in SQL so concurrent bumps can't read-modify-write over each other. + updates.append("token_epoch = token_epoch + 1") if changes.is_admin is not None: updates.append("is_admin = ?") @@ -173,7 +180,7 @@ def authenticate(self, email: str, password: str) -> UserDTO | None: with self._db.transaction() as cursor: cursor.execute( """ - SELECT user_id, email, display_name, password_hash, is_admin, is_active, created_at, updated_at, last_login_at + SELECT user_id, email, display_name, password_hash, is_admin, is_active, created_at, updated_at, last_login_at, token_epoch FROM users WHERE email = ? """, @@ -204,6 +211,8 @@ def authenticate(self, email: str, password: str) -> UserDTO | None: created_at=datetime.fromisoformat(row[6]), updated_at=datetime.fromisoformat(row[7]), last_login_at=datetime.now(timezone.utc), + # password_hash occupies row[3] in this query, shifting the tail by one. + token_epoch=int(row[9]), ) def has_admin(self) -> bool: @@ -233,7 +242,7 @@ def list_users(self, limit: int = 100, offset: int = 0) -> list[UserDTO]: with self._db.transaction() as cursor: cursor.execute( """ - SELECT user_id, email, display_name, is_admin, is_active, created_at, updated_at, last_login_at + SELECT user_id, email, display_name, is_admin, is_active, created_at, updated_at, last_login_at, token_epoch FROM users ORDER BY created_at DESC LIMIT ? OFFSET ? @@ -252,6 +261,7 @@ def list_users(self, limit: int = 100, offset: int = 0) -> list[UserDTO]: created_at=datetime.fromisoformat(row[5]), updated_at=datetime.fromisoformat(row[6]), last_login_at=datetime.fromisoformat(row[7]) if row[7] else None, + token_epoch=int(row[8]), ) for row in rows ] diff --git a/invokeai/frontend/web/openapi.json b/invokeai/frontend/web/openapi.json index 6fe1fdf79d7..3ca9f342ab6 100644 --- a/invokeai/frontend/web/openapi.json +++ b/invokeai/frontend/web/openapi.json @@ -144,7 +144,7 @@ "patch": { "tags": ["authentication"], "summary": "Update Current User", - "description": "Update the current user's own profile.\n\nTo change the password, both ``current_password`` and ``new_password`` must\nbe provided. The current password is verified before the change is applied.\n\nArgs:\n request: Profile fields to update\n current_user: The authenticated user\n\nReturns:\n The updated user\n\nRaises:\n HTTPException: 400 if current password is incorrect or new password is weak\n HTTPException: 404 if user not found", + "description": "Update the current user's own profile.\n\nTo change the password, both ``current_password`` and ``new_password`` must\nbe provided. The current password is verified before the change is applied.\n\nA password change signs out the account's *other* sessions: it bumps the\nrevocation epoch, invalidating every previously issued token. This response\ncarries a replacement token in ``X-Refreshed-Token`` so the caller stays\nsigned in.\n\nArgs:\n request: Profile fields to update\n current_user: The authenticated user\n http_request: The HTTP request, used to scope the replacement media cookie\n response: The HTTP response, used to return the replacement token\n\nReturns:\n The updated user\n\nRaises:\n HTTPException: 400 if current password is incorrect or new password is weak\n HTTPException: 404 if user not found", "operationId": "update_current_user_api_v1_auth_me_patch", "requestBody": { "content": { @@ -375,7 +375,7 @@ "patch": { "tags": ["authentication"], "summary": "Update User", - "description": "Update a user. Requires admin privileges.\n\nArgs:\n user_id: The user ID\n request: Fields to update\n\nReturns:\n The updated user\n\nRaises:\n HTTPException: 400 if password is weak, or if the change would remove the\n last administrator\n HTTPException: 404 if user not found", + "description": "Update a user. Requires admin privileges.\n\nResetting a password revokes the target's existing sessions. An admin resetting\ntheir own password receives a replacement token in ``X-Refreshed-Token`` so they\nare not signed out by their own action.\n\nArgs:\n user_id: The user ID\n request: Fields to update\n\nReturns:\n The updated user\n\nRaises:\n HTTPException: 400 if password is weak, or if the change would remove the\n last administrator\n HTTPException: 404 if user not found", "operationId": "update_user_api_v1_auth_users__user_id__patch", "security": [ { @@ -83080,6 +83080,12 @@ ], "title": "Last Login At", "description": "When user last logged in" + }, + "token_epoch": { + "type": "integer", + "title": "Token Epoch", + "description": "Revocation epoch; tokens minted before the current value are rejected", + "default": 0 } }, "type": "object", diff --git a/invokeai/frontend/web/src/services/api/schema.ts b/invokeai/frontend/web/src/services/api/schema.ts index c3f71173ea5..882240d8001 100644 --- a/invokeai/frontend/web/src/services/api/schema.ts +++ b/invokeai/frontend/web/src/services/api/schema.ts @@ -157,9 +157,16 @@ export type paths = { * To change the password, both ``current_password`` and ``new_password`` must * be provided. The current password is verified before the change is applied. * + * A password change signs out the account's *other* sessions: it bumps the + * revocation epoch, invalidating every previously issued token. This response + * carries a replacement token in ``X-Refreshed-Token`` so the caller stays + * signed in. + * * Args: * request: Profile fields to update * current_user: The authenticated user + * http_request: The HTTP request, used to scope the replacement media cookie + * response: The HTTP response, used to return the replacement token * * Returns: * The updated user @@ -310,6 +317,10 @@ export type paths = { * Update User * @description Update a user. Requires admin privileges. * + * Resetting a password revokes the target's existing sessions. An admin resetting + * their own password receives a replacement token in ``X-Refreshed-Token`` so they + * are not signed out by their own action. + * * Args: * user_id: The user ID * request: Fields to update @@ -36324,6 +36335,12 @@ export type components = { * @description When user last logged in */ last_login_at?: string | null; + /** + * Token Epoch + * @description Revocation epoch; tokens minted before the current value are rejected + * @default 0 + */ + token_epoch?: number; }; /** * UserProfileUpdateRequest diff --git a/tests/app/api/test_sliding_window_token.py b/tests/app/api/test_sliding_window_token.py index 8cfdc711cac..09753a66152 100644 --- a/tests/app/api/test_sliding_window_token.py +++ b/tests/app/api/test_sliding_window_token.py @@ -21,6 +21,7 @@ def _setup_jwt_secret(monkeypatch: pytest.MonkeyPatch): email="test@test.com", is_admin=False, is_active=True, + token_epoch=0, ) users = SimpleNamespace(get=lambda user_id: user if user_id == user.user_id else None) monkeypatch.setattr( @@ -348,7 +349,7 @@ def test_demoted_admin_refresh_carries_db_role(self, monkeypatch: pytest.MonkeyP _patch_user_record( monkeypatch, - SimpleNamespace(user_id="test-user", email="test@test.com", is_admin=False, is_active=True), + SimpleNamespace(user_id="test-user", email="test@test.com", is_admin=False, is_active=True, token_epoch=0), ) app = _create_test_app() client = TestClient(app) @@ -370,7 +371,7 @@ def test_promoted_user_refresh_carries_db_role(self, monkeypatch: pytest.MonkeyP _patch_user_record( monkeypatch, - SimpleNamespace(user_id="test-user", email="test@test.com", is_admin=True, is_active=True), + SimpleNamespace(user_id="test-user", email="test@test.com", is_admin=True, is_active=True, token_epoch=0), ) app = _create_test_app() client = TestClient(app) @@ -390,7 +391,7 @@ def test_deactivated_user_gets_no_refresh(self, monkeypatch: pytest.MonkeyPatch) _patch_user_record( monkeypatch, - SimpleNamespace(user_id="test-user", email="test@test.com", is_admin=False, is_active=False), + SimpleNamespace(user_id="test-user", email="test@test.com", is_admin=False, is_active=False, token_epoch=0), ) app = _create_test_app() client = TestClient(app) @@ -421,7 +422,7 @@ def test_active_user_refresh_preserves_remember_me(self, monkeypatch: pytest.Mon _patch_user_record( monkeypatch, - SimpleNamespace(user_id="test-user", email="test@test.com", is_admin=False, is_active=True), + SimpleNamespace(user_id="test-user", email="test@test.com", is_admin=False, is_active=True, token_epoch=0), ) app = _create_test_app() client = TestClient(app) diff --git a/tests/app/api/test_video_upload_limits.py b/tests/app/api/test_video_upload_limits.py index 0d7d972e3bd..1d1df14fd3f 100644 --- a/tests/app/api/test_video_upload_limits.py +++ b/tests/app/api/test_video_upload_limits.py @@ -370,7 +370,7 @@ def test_production_upload_authentication( ) -> None: set_jwt_secret("test-secret-key-for-unit-tests-only-do-not-use-in-production") token_data = TokenData(user_id="user", email="user@example.com", is_admin=False) - user = SimpleNamespace(is_active=user_active) + user = SimpleNamespace(user_id="user", is_active=user_active, token_epoch=0) invoker = SimpleNamespace( services=SimpleNamespace( configuration=SimpleNamespace(multiuser=multiuser), @@ -399,7 +399,7 @@ async def run_in_threadpool(func: Any, *args: Any) -> Any: monkeypatch.setattr(api_app, "run_in_threadpool", run_in_threadpool) def slow_get(_user_id: str) -> SimpleNamespace: - return SimpleNamespace(is_active=True) + return SimpleNamespace(user_id="user", is_active=True, token_epoch=0) invoker = SimpleNamespace( services=SimpleNamespace( diff --git a/tests/app/routers/test_app_info.py b/tests/app/routers/test_app_info.py index afc7e323777..4ca610a0a43 100644 --- a/tests/app/routers/test_app_info.py +++ b/tests/app/routers/test_app_info.py @@ -38,7 +38,7 @@ def _non_admin_user() -> Mock: stubbed user must carry concrete field values — a bare ``Mock`` fails ``TokenData`` validation. """ - return Mock(user_id="user-1", email="user@example.com", is_admin=False, is_active=True) + return Mock(user_id="user-1", email="user@example.com", is_admin=False, is_active=True, token_epoch=0) def test_get_external_provider_statuses(monkeypatch: Any, mock_invoker: Invoker, client: TestClient) -> None: diff --git a/tests/app/routers/test_multiuser_authorization.py b/tests/app/routers/test_multiuser_authorization.py index be5d2a61beb..95c90c9e3b4 100644 --- a/tests/app/routers/test_multiuser_authorization.py +++ b/tests/app/routers/test_multiuser_authorization.py @@ -1751,6 +1751,10 @@ def socketio(self, mock_invoker: Invoker, monkeypatch: Any): # at request time. Patch it to point at the mock invoker. mock_deps = MockApiDependencies(mock_invoker) monkeypatch.setattr("invokeai.app.api.dependencies.ApiDependencies", mock_deps) + # Connect resolves the user record through `resolve_authorized_user`, which binds + # ApiDependencies at import time in auth_dependencies — patching the defining + # module alone would not reach it. + monkeypatch.setattr("invokeai.app.api.auth_dependencies.ApiDependencies", mock_deps) fastapi_app = FastAPI() return SocketIO(fastapi_app) diff --git a/tests/app/routers/test_privilege_revocation.py b/tests/app/routers/test_privilege_revocation.py index 4107371429d..782951e4198 100644 --- a/tests/app/routers/test_privilege_revocation.py +++ b/tests/app/routers/test_privilege_revocation.py @@ -332,6 +332,164 @@ def test_demoted_admin_allowed_mutation_refreshes_with_demoted_claim( assert refreshed.is_admin is False +class TestTokenEpochRevocation: + """A password change invalidates tokens issued before it. + + A JWT is self-contained, so nothing in the database can make an already-issued token + stop verifying. The epoch claim is the missing revocation primitive: rotating the + password bumps it, and every token carrying the old value is rejected. + """ + + def _change_own_password(self, client: TestClient, token: str, current: str, new: str) -> Any: + return client.patch( + "/api/v1/auth/me", + json={"current_password": current, "new_password": new}, + headers=_auth(token), + ) + + def test_password_change_revokes_other_sessions( + self, client: TestClient, mock_invoker: Invoker, admin_token: str + ) -> None: + _create_user(mock_invoker, "user@test.com", "User") + session_a = _login(client, "user@test.com") + session_b = _login(client, "user@test.com") + + r = self._change_own_password(client, session_a, "TestPass123", "BrandNewPass456") + assert r.status_code == 200 + + # The other session's token is dead even though the account is still active. + assert client.get("/api/v1/auth/me", headers=_auth(session_b)).status_code == status.HTTP_401_UNAUTHORIZED + + def test_password_change_keeps_the_calling_session( + self, client: TestClient, mock_invoker: Invoker, admin_token: str + ) -> None: + """Changing your own password must not sign you out of the tab you did it from.""" + _create_user(mock_invoker, "user@test.com", "User") + token = _login(client, "user@test.com") + + r = self._change_own_password(client, token, "TestPass123", "BrandNewPass456") + + assert r.status_code == 200 + replacement = r.headers["X-Refreshed-Token"] + assert client.get("/api/v1/auth/me", headers=_auth(replacement)).status_code == 200 + + def test_admin_password_reset_revokes_the_targets_sessions( + self, client: TestClient, mock_invoker: Invoker, admin_token: str + ) -> None: + user_id = _create_user(mock_invoker, "user@test.com", "User") + victim_token = _login(client, "user@test.com") + + r = client.patch( + f"/api/v1/auth/users/{user_id}", json={"password": "AdminReset789"}, headers=_auth(admin_token) + ) + assert r.status_code == 200 + + assert client.get("/api/v1/auth/me", headers=_auth(victim_token)).status_code == status.HTTP_401_UNAUTHORIZED + + def test_revoked_token_cannot_be_laundered_by_refresh( + self, client: TestClient, mock_invoker: Invoker, admin_token: str + ) -> None: + """The sliding window must not mint a fresh token from a revoked one. + + The middleware runs after the route and refreshes on any 2xx mutation, so an + unauthenticated route would otherwise hand a stale-epoch bearer a valid token. + """ + _create_user(mock_invoker, "user@test.com", "User") + stale = _login(client, "user@test.com") + _login(client, "user@test.com") + self._change_own_password(client, stale, "TestPass123", "BrandNewPass456") + + # /auth/login requires no bearer and returns 200, but carries our stale header. + r = client.post( + "/api/v1/auth/login", + json={"email": "user@test.com", "password": "BrandNewPass456", "remember_me": False}, + headers=_auth(stale), + ) + + assert r.status_code == 200 + assert "X-Refreshed-Token" not in r.headers + + def test_unrelated_profile_update_does_not_revoke( + self, client: TestClient, mock_invoker: Invoker, admin_token: str + ) -> None: + """Only a password change bumps the epoch — renaming must not sign anyone out.""" + _create_user(mock_invoker, "user@test.com", "User") + token = _login(client, "user@test.com") + + r = client.patch("/api/v1/auth/me", json={"display_name": "Renamed"}, headers=_auth(token)) + + assert r.status_code == 200 + assert client.get("/api/v1/auth/me", headers=_auth(token)).status_code == 200 + + def test_revoked_token_is_rejected_on_ordinary_routes( + self, client: TestClient, mock_invoker: Invoker, admin_token: str + ) -> None: + """The revocation must cover the dependency almost every route actually uses. + + `/auth/*` routes take `CurrentUser`; the rest of the API takes + `CurrentUserOrDefault`. Checking only the former leaves the check absent + everywhere it matters, which is indistinguishable from having no check at all. + """ + _create_user(mock_invoker, "user@test.com", "User") + stale = _login(client, "user@test.com") + _save_image(mock_invoker, "victim.png", mock_invoker.services.users.get_by_email("user@test.com").user_id) + second = _login(client, "user@test.com") + self._change_own_password(client, second, "TestPass123", "BrandNewPass456") + + # A CurrentUserOrDefault route, not an /auth/* one. + r = client.get("/api/v1/images/i/victim.png", headers=_auth(stale)) + + assert r.status_code == status.HTTP_401_UNAUTHORIZED + + def test_admin_resetting_own_password_is_not_locked_out( + self, client: TestClient, mock_invoker: Invoker, admin_token: str + ) -> None: + """The admin route bumps the epoch just like /auth/me, so it owes the caller a + replacement for the same reason — otherwise an admin who resets their own + password from the user-management screen locks themselves out.""" + admin = mock_invoker.services.users.get_by_email("admin@test.com") + assert admin is not None + + r = client.patch( + f"/api/v1/auth/users/{admin.user_id}", + json={"password": "AdminNewPass789"}, + headers=_auth(admin_token), + ) + + assert r.status_code == 200 + replacement = r.headers["X-Refreshed-Token"] + assert client.get("/api/v1/auth/me", headers=_auth(replacement)).status_code == 200 + assert client.get("/api/v1/auth/me", headers=_auth(admin_token)).status_code == status.HTTP_401_UNAUTHORIZED + + def test_password_change_emits_access_event_for_live_sockets( + self, client: TestClient, mock_invoker: Invoker, admin_token: str + ) -> None: + """HTTP revocation alone would leave already-open sockets streaming events.""" + _create_user(mock_invoker, "user@test.com", "User") + token = _login(client, "user@test.com") + + self._change_own_password(client, token, "TestPass123", "BrandNewPass456") + + events = [e for e in mock_invoker.services.events.events if isinstance(e, UserAccessChangedEvent)] + assert len(events) == 1 + assert events[0].is_active is True # not a deactivation — the epoch is the signal + assert events[0].token_epoch == 1 + + def test_tokens_predating_the_claim_still_work( + self, client: TestClient, mock_invoker: Invoker, admin_token: str + ) -> None: + """Upgrading must not log everyone out: no epoch claim decodes to 0, matching an + un-bumped record.""" + from invokeai.app.services.auth.token_service import TokenData, create_access_token + + user_id = _create_user(mock_invoker, "user@test.com", "User") + legacy = create_access_token( + TokenData(user_id=user_id, email="user@test.com", is_admin=False, remember_me=False) + ) + + assert client.get("/api/v1/auth/me", headers=_auth(legacy)).status_code == 200 + + class TestUserAccessChangedEmission: """Role/status changes emit the internal event that re-authorizes live connections.""" diff --git a/tests/app/services/auth/test_performance.py b/tests/app/services/auth/test_performance.py index ad033ac84cd..3da246259ce 100644 --- a/tests/app/services/auth/test_performance.py +++ b/tests/app/services/auth/test_performance.py @@ -39,7 +39,8 @@ def user_service(logger: Logger) -> UserService: is_active BOOLEAN NOT NULL DEFAULT TRUE, created_at DATETIME NOT NULL DEFAULT(STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')), updated_at DATETIME NOT NULL DEFAULT(STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')), - last_login_at DATETIME + last_login_at DATETIME, + token_epoch INTEGER NOT NULL DEFAULT 0 ); """) db._conn.commit() diff --git a/tests/app/services/users/test_user_service.py b/tests/app/services/users/test_user_service.py index d5d04964005..aa6b52a2ae2 100644 --- a/tests/app/services/users/test_user_service.py +++ b/tests/app/services/users/test_user_service.py @@ -30,7 +30,8 @@ def db(logger: Logger) -> SqliteDatabase: is_active BOOLEAN NOT NULL DEFAULT TRUE, created_at DATETIME NOT NULL DEFAULT(STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')), updated_at DATETIME NOT NULL DEFAULT(STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')), - last_login_at DATETIME + last_login_at DATETIME, + token_epoch INTEGER NOT NULL DEFAULT 0 ); """) db._conn.commit() diff --git a/tests/app/test_socket_privilege_revocation.py b/tests/app/test_socket_privilege_revocation.py index 61981b00142..592457d7fd2 100644 --- a/tests/app/test_socket_privilege_revocation.py +++ b/tests/app/test_socket_privilege_revocation.py @@ -30,9 +30,11 @@ def _patch_multiuser_context( token_is_admin: bool, db_is_admin: bool, db_is_active: bool = True, + db_epoch: int = 0, + token_epoch: int = 0, ) -> None: """Multiuser context where the token's claims and the database record can differ.""" - user = SimpleNamespace(user_id=user_id, is_active=db_is_active, is_admin=db_is_admin) + user = SimpleNamespace(user_id=user_id, is_active=db_is_active, is_admin=db_is_admin, token_epoch=db_epoch) invoker = SimpleNamespace( services=SimpleNamespace( configuration=SimpleNamespace(multiuser=True), @@ -40,9 +42,15 @@ def _patch_multiuser_context( ) ) monkeypatch.setattr("invokeai.app.api.dependencies.ApiDependencies", SimpleNamespace(invoker=invoker)) + # The connect handler resolves the record through `resolve_authorized_user`, which binds + # ApiDependencies at import time in auth_dependencies — patching the defining module alone + # would not reach it. + monkeypatch.setattr("invokeai.app.api.auth_dependencies.ApiDependencies", SimpleNamespace(invoker=invoker)) monkeypatch.setattr( "invokeai.app.api.sockets.verify_token", - lambda token: SimpleNamespace(user_id=user_id, is_admin=token_is_admin) if token == "valid-token" else None, + lambda token: SimpleNamespace(user_id=user_id, is_admin=token_is_admin, token_epoch=token_epoch) + if token == "valid-token" + else None, ) @@ -115,10 +123,10 @@ def _connected_socketio(self) -> SocketIO: socketio._sio.leave_room = AsyncMock() socketio._sio.disconnect = AsyncMock() socketio._socket_users = { - "sid-admin": {"user_id": "admin-1", "is_admin": True}, - "sid-user-a": {"user_id": "user-1", "is_admin": False}, - "sid-user-b": {"user_id": "user-1", "is_admin": False}, - "sid-other": {"user_id": "user-2", "is_admin": False}, + "sid-admin": {"user_id": "admin-1", "is_admin": True, "token_epoch": 0}, + "sid-user-a": {"user_id": "user-1", "is_admin": False, "token_epoch": 0}, + "sid-user-b": {"user_id": "user-1", "is_admin": False, "token_epoch": 0}, + "sid-other": {"user_id": "user-2", "is_admin": False, "token_epoch": 0}, } return socketio @@ -193,3 +201,59 @@ async def test_other_users_sockets_are_untouched(self) -> None: assert "sid-other" not in disconnected socketio._sio.leave_room.assert_not_awaited() assert socketio._socket_users["sid-admin"]["is_admin"] is True + + +class TestTokenEpochOnSockets: + """A revoked token must not open a socket, and must not keep an open one alive. + + Sockets authenticate once, at connect. Without both halves of this, HTTP would be + locked out while an already-connected socket kept streaming the same user's events. + """ + + @pytest.mark.anyio + async def test_revoked_token_cannot_connect(self, monkeypatch: pytest.MonkeyPatch) -> None: + socketio = SocketIO(FastAPI()) + socketio._sio.enter_room = AsyncMock() + _patch_multiuser_context( + monkeypatch, user_id="user-1", token_is_admin=False, db_is_admin=False, db_epoch=1, token_epoch=0 + ) + + accepted = await socketio._handle_connect("sid-1", {}, {"token": "valid-token"}) + + assert accepted is False + assert "sid-1" not in socketio._socket_users + socketio._sio.enter_room.assert_not_awaited() + + @pytest.mark.anyio + async def test_current_token_still_connects(self, monkeypatch: pytest.MonkeyPatch) -> None: + """The epoch check must not reject an ordinary, current token.""" + socketio = SocketIO(FastAPI()) + socketio._sio.enter_room = AsyncMock() + _patch_multiuser_context( + monkeypatch, user_id="user-1", token_is_admin=False, db_is_admin=False, db_epoch=3, token_epoch=3 + ) + + accepted = await socketio._handle_connect("sid-1", {}, {"token": "valid-token"}) + + assert accepted is True + assert socketio._socket_users["sid-1"]["token_epoch"] == 3 + + @pytest.mark.anyio + async def test_password_change_disconnects_superseded_sockets_only(self) -> None: + """The account stays active, so the deactivation branch does not fire — the epoch + is what identifies which of the user's sockets are now holding dead tokens.""" + socketio = SocketIO(FastAPI()) + socketio._sio.enter_room = AsyncMock() + socketio._sio.leave_room = AsyncMock() + socketio._sio.disconnect = AsyncMock() + socketio._socket_users = { + "sid-old": {"user_id": "user-1", "is_admin": False, "token_epoch": 0}, + "sid-new": {"user_id": "user-1", "is_admin": False, "token_epoch": 1}, + "sid-other": {"user_id": "user-2", "is_admin": False, "token_epoch": 0}, + } + event = UserAccessChangedEvent.build(user_id="user-1", is_admin=False, is_active=True, token_epoch=1) + + await socketio._handle_user_access_changed(("user_access_changed", event)) + + disconnected = {call.args[0] for call in socketio._sio.disconnect.await_args_list} + assert disconnected == {"sid-old"} diff --git a/tests/app/test_workflow_socketio.py b/tests/app/test_workflow_socketio.py index 3d227256c6e..3c6728090e0 100644 --- a/tests/app/test_workflow_socketio.py +++ b/tests/app/test_workflow_socketio.py @@ -15,7 +15,7 @@ def anyio_backend() -> str: def _patch_multiuser_context(monkeypatch: pytest.MonkeyPatch, *, user_id: str, is_admin: bool) -> None: # The connect handler derives is_admin from the database record, not the token, # so the mocked user record carries the role. - user = SimpleNamespace(user_id=user_id, is_active=True, is_admin=is_admin) + user = SimpleNamespace(user_id=user_id, is_active=True, is_admin=is_admin, token_epoch=0) invoker = SimpleNamespace( services=SimpleNamespace( configuration=SimpleNamespace(multiuser=True), @@ -23,9 +23,14 @@ def _patch_multiuser_context(monkeypatch: pytest.MonkeyPatch, *, user_id: str, i ) ) monkeypatch.setattr("invokeai.app.api.dependencies.ApiDependencies", SimpleNamespace(invoker=invoker)) + # Connect resolves the record via `resolve_authorized_user`, which binds + # ApiDependencies at import time in auth_dependencies. + monkeypatch.setattr("invokeai.app.api.auth_dependencies.ApiDependencies", SimpleNamespace(invoker=invoker)) monkeypatch.setattr( "invokeai.app.api.sockets.verify_token", - lambda token: SimpleNamespace(user_id=user_id, is_admin=is_admin) if token == "valid-token" else None, + lambda token: SimpleNamespace(user_id=user_id, is_admin=is_admin, token_epoch=0) + if token == "valid-token" + else None, ) From d46d3b32f75a55296cc50effa21a6c6cc6bba25f Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Fri, 31 Jul 2026 10:37:58 -0400 Subject: [PATCH 04/19] fix(auth): stop special-casing the system user in queued-execution checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- invokeai/app/api/routers/auth.py | 31 ++++++++++++++-- .../session_processor_default.py | 14 +++++--- invokeai/frontend/web/openapi.json | 2 +- .../frontend/web/src/services/api/schema.ts | 4 +-- .../app/routers/test_privilege_revocation.py | 36 +++++++++++++++++++ .../test_privilege_revocation.py | 19 +++++++--- 6 files changed, 91 insertions(+), 15 deletions(-) diff --git a/invokeai/app/api/routers/auth.py b/invokeai/app/api/routers/auth.py index fc87f3e4ca3..64a2f585599 100644 --- a/invokeai/app/api/routers/auth.py +++ b/invokeai/app/api/routers/auth.py @@ -35,6 +35,11 @@ TOKEN_EXPIRATION_NORMAL = 1 # 1 day for normal login TOKEN_EXPIRATION_REMEMBER_ME = 7 # 7 days for "remember me" login +# Owner of everything that predates multiuser support (created by migration_27). Not a +# login account: it has an empty password hash and is hidden from the user list. +SYSTEM_USER_ID = "system" +SYSTEM_USER_PROTECTED_DETAIL = "The system user cannot be deleted or deactivated" + def _issue_replacement_token(http_request: Request, response: Response, user: UserDTO, remember_me: bool) -> None: """Hand the caller a token minted under the user's *current* revocation epoch. @@ -482,7 +487,7 @@ async def list_users( List of all real users (system user excluded) """ user_service = ApiDependencies.invoker.services.users - return [u for u in user_service.list_users() if u.user_id != "system"] + return [u for u in user_service.list_users() if u.user_id != SYSTEM_USER_ID] @auth_router.post("/users", response_model=UserDTO, status_code=status.HTTP_201_CREATED) @@ -568,6 +573,16 @@ async def update_user( config = ApiDependencies.invoker.services.configuration before = user_service.get(user_id) + # The system user owns everything migrated from before multiuser support. Deactivating + # it would strand that content: its queue items stop at the dequeue gate, and reads and + # saves against system-owned media raise PermissionError. It is not a login account, so + # there is no reason to disable it. + if user_id == SYSTEM_USER_ID and request.is_active is False: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=SYSTEM_USER_PROTECTED_DETAIL, + ) + # Demoting or deactivating the last administrator is irreversible: authorization is # derived from the database on every request, so the caller loses admin access # immediately and no authenticated path back exists. It would also drop `has_admin()` @@ -630,13 +645,13 @@ async def delete_user( """Delete a user. Requires admin privileges. Admins can delete any user including other admins, but cannot delete the last - remaining admin. + remaining admin, nor the internal system user. Args: user_id: The user ID Raises: - HTTPException: 400 if attempting to delete the last admin + HTTPException: 400 if attempting to delete the last admin or the system user HTTPException: 404 if user not found """ user_service = ApiDependencies.invoker.services.users @@ -644,6 +659,16 @@ async def delete_user( if user is None: raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="User not found") + # The system user owns every board, image, and workflow migrated from before multiuser + # support. Deleting it orphans all of that content: reads and saves against it raise + # PermissionError and its queued items are rejected at dequeue. The last-admin guard + # below does not cover it — the system row is deliberately not an admin. + if user_id == SYSTEM_USER_ID: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=SYSTEM_USER_PROTECTED_DETAIL, + ) + # Prevent deleting the last active admin if user.is_admin and user.is_active and user_service.count_admins() <= 1: raise HTTPException( diff --git a/invokeai/app/services/session_processor/session_processor_default.py b/invokeai/app/services/session_processor/session_processor_default.py index a7b79eeae90..a1b6e65ad4e 100644 --- a/invokeai/app/services/session_processor/session_processor_default.py +++ b/invokeai/app/services/session_processor/session_processor_default.py @@ -53,9 +53,15 @@ def queue_owner_is_active(services: InvocationServices, queue_item: SessionQueue the next node boundary (and immediately mid-node for nodes with step callbacks, via the cancel event set when the item is canceled). - The ``system`` user represents single-user mode and has no database record, so - it is always considered active. The check is skipped entirely when multiuser - mode is disabled. + The check is skipped entirely when multiuser mode is disabled. + + The ``system`` user — which owns everything migrated from before multiuser support + (see migration_27) — is deliberately NOT special-cased. It has a real, active + database row, so it passes on its own merits. Exempting it here would only change + behaviour when the row is missing or inactive, and that is precisely the case where + this gate would then disagree with the save gates in `invocation_context`, which + have no such exemption: the item would burn GPU time and then fail at the first + `context.images.save()`. Better to reject it at dequeue. A failed lookup is treated as active. This runs between nodes on a path with no exception handling of its own, so letting a transient error (e.g. a busy-timeout @@ -66,8 +72,6 @@ def queue_owner_is_active(services: InvocationServices, queue_item: SessionQueue """ if not services.configuration.multiuser: return True - if queue_item.user_id == "system": - return True try: user = services.users.get(queue_item.user_id) except Exception: diff --git a/invokeai/frontend/web/openapi.json b/invokeai/frontend/web/openapi.json index 3ca9f342ab6..97e1d4238ec 100644 --- a/invokeai/frontend/web/openapi.json +++ b/invokeai/frontend/web/openapi.json @@ -432,7 +432,7 @@ "delete": { "tags": ["authentication"], "summary": "Delete User", - "description": "Delete a user. Requires admin privileges.\n\nAdmins can delete any user including other admins, but cannot delete the last\nremaining admin.\n\nArgs:\n user_id: The user ID\n\nRaises:\n HTTPException: 400 if attempting to delete the last admin\n HTTPException: 404 if user not found", + "description": "Delete a user. Requires admin privileges.\n\nAdmins can delete any user including other admins, but cannot delete the last\nremaining admin, nor the internal system user.\n\nArgs:\n user_id: The user ID\n\nRaises:\n HTTPException: 400 if attempting to delete the last admin or the system user\n HTTPException: 404 if user not found", "operationId": "delete_user_api_v1_auth_users__user_id__delete", "security": [ { diff --git a/invokeai/frontend/web/src/services/api/schema.ts b/invokeai/frontend/web/src/services/api/schema.ts index 882240d8001..eef427cfb3c 100644 --- a/invokeai/frontend/web/src/services/api/schema.ts +++ b/invokeai/frontend/web/src/services/api/schema.ts @@ -301,13 +301,13 @@ export type paths = { * @description Delete a user. Requires admin privileges. * * Admins can delete any user including other admins, but cannot delete the last - * remaining admin. + * remaining admin, nor the internal system user. * * Args: * user_id: The user ID * * Raises: - * HTTPException: 400 if attempting to delete the last admin + * HTTPException: 400 if attempting to delete the last admin or the system user * HTTPException: 404 if user not found */ delete: operations["delete_user_api_v1_auth_users__user_id__delete"]; diff --git a/tests/app/routers/test_privilege_revocation.py b/tests/app/routers/test_privilege_revocation.py index 782951e4198..451e3565dad 100644 --- a/tests/app/routers/test_privilege_revocation.py +++ b/tests/app/routers/test_privilege_revocation.py @@ -332,6 +332,42 @@ def test_demoted_admin_allowed_mutation_refreshes_with_demoted_claim( assert refreshed.is_admin is False +class TestSystemUserIsProtected: + """The system user owns everything migrated from before multiuser support, so removing + it would strand that content rather than merely removing an account.""" + + def test_system_user_cannot_be_deleted(self, client: TestClient, admin_token: str) -> None: + r = client.delete("/api/v1/auth/users/system", headers=_auth(admin_token)) + + assert r.status_code == status.HTTP_400_BAD_REQUEST + + def test_system_user_cannot_be_deactivated(self, client: TestClient, admin_token: str) -> None: + r = client.patch("/api/v1/auth/users/system", json={"is_active": False}, headers=_auth(admin_token)) + + assert r.status_code == status.HTTP_400_BAD_REQUEST + + def test_system_user_survives_both_attempts( + self, client: TestClient, mock_invoker: Invoker, admin_token: str + ) -> None: + client.delete("/api/v1/auth/users/system", headers=_auth(admin_token)) + client.patch("/api/v1/auth/users/system", json={"is_active": False}, headers=_auth(admin_token)) + + system = mock_invoker.services.users.get("system") + assert system is not None + assert system.is_active is True + + def test_ordinary_user_deletion_still_works( + self, client: TestClient, mock_invoker: Invoker, admin_token: str + ) -> None: + """The guard must be specific to the system id, not a blanket block.""" + user_id = _create_user(mock_invoker, "user@test.com", "User") + + r = client.delete(f"/api/v1/auth/users/{user_id}", headers=_auth(admin_token)) + + assert r.status_code == status.HTTP_204_NO_CONTENT + assert mock_invoker.services.users.get(user_id) is None + + class TestTokenEpochRevocation: """A password change invalidates tokens issued before it. diff --git a/tests/app/services/session_processor/test_privilege_revocation.py b/tests/app/services/session_processor/test_privilege_revocation.py index 88605480bdd..dc43f786ca9 100644 --- a/tests/app/services/session_processor/test_privilege_revocation.py +++ b/tests/app/services/session_processor/test_privilege_revocation.py @@ -5,7 +5,8 @@ - Pending items are rejected (canceled) at dequeue, before any invocation runs. - Running items are stopped at the next node boundary; canceling also sets the processor's cancel event, which stops step-callback nodes mid-node. -- Single-user mode and the ``system`` user are exempt. +- Single-user mode is exempt. The ``system`` user is not special-cased: it has a real, + active database row and passes on its own merits. """ from threading import Event as ThreadEvent @@ -49,10 +50,20 @@ def test_single_user_mode_is_always_active(self) -> None: services = _services(multiuser=False) assert queue_owner_is_active(services, _queue_item(user_id="anyone")) is True - def test_system_user_is_always_active(self) -> None: - services = _services(multiuser=True) + def test_system_user_passes_on_its_own_row(self) -> None: + """migration_27 creates an active `system` row, so no exemption is needed.""" + services = _services(users_by_id={"system": _active("system")}) assert queue_owner_is_active(services, _queue_item(user_id="system")) is True + def test_system_user_without_a_row_is_rejected(self) -> None: + """Agrees with the `invocation_context` save gates, which have no exemption either. + + Exempting `system` here would let the item consume GPU time and then fail at its + first save; rejecting it at dequeue is the coherent outcome. + """ + services = _services(users_by_id={}) + assert queue_owner_is_active(services, _queue_item(user_id="system")) is False + def test_active_user(self) -> None: services = _services(users_by_id={"user-1": _active("user-1")}) assert queue_owner_is_active(services, _queue_item()) is True @@ -98,7 +109,7 @@ def test_active_owner_item_is_executed(self) -> None: services.session_queue.cancel_queue_item.assert_not_called() def test_system_item_is_executed(self) -> None: - services = _services(multiuser=True) + services = _services(users_by_id={"system": _active("system")}) processor = self._processor(services) assert processor._cancel_queue_item_if_owner_inactive(_queue_item(user_id="system")) is False From e9317f65830b299607d073da74a61ae007fc9b39 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sat, 8 Aug 2026 09:41:53 -0400 Subject: [PATCH 05/19] fix(auth): enforce the last-administrator invariant inside the write transaction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- invokeai/app/services/users/users_base.py | 4 + invokeai/app/services/users/users_common.py | 8 + invokeai/app/services/users/users_default.py | 58 ++++- tests/app/routers/test_last_admin_routes.py | 89 +++++++ .../users/test_last_admin_invariant.py | 238 ++++++++++++++++++ 5 files changed, 396 insertions(+), 1 deletion(-) create mode 100644 tests/app/routers/test_last_admin_routes.py create mode 100644 tests/app/services/users/test_last_admin_invariant.py diff --git a/invokeai/app/services/users/users_base.py b/invokeai/app/services/users/users_base.py index dd789b561ee..0694fe1352b 100644 --- a/invokeai/app/services/users/users_base.py +++ b/invokeai/app/services/users/users_base.py @@ -64,6 +64,8 @@ def update(self, user_id: str, changes: UserUpdateRequest, strict_password_check Raises: ValueError: If user not found or (when strict) password is weak + LastAdministratorError: If demoting or deactivating this user would leave the + instance with no active administrator """ pass @@ -76,6 +78,8 @@ def delete(self, user_id: str) -> None: Raises: ValueError: If user not found + LastAdministratorError: If deleting this user would leave the instance with no + active administrator """ pass diff --git a/invokeai/app/services/users/users_common.py b/invokeai/app/services/users/users_common.py index c6ede1e24bd..0b9dd3b79c6 100644 --- a/invokeai/app/services/users/users_common.py +++ b/invokeai/app/services/users/users_common.py @@ -115,3 +115,11 @@ class UserUpdateRequest(BaseModel): password: str | None = Field(default=None, description="New password") is_admin: bool | None = Field(default=None, description="Whether user should have admin privileges") is_active: bool | None = Field(default=None, description="Whether user account should be active") + + +class LastAdministratorError(ValueError): + """Raised when a change would leave the instance with no active administrator. + + Subclasses :class:`ValueError` so that callers which already map service-layer + ``ValueError`` to a 400 keep working unchanged. + """ diff --git a/invokeai/app/services/users/users_default.py b/invokeai/app/services/users/users_default.py index 8253949505d..65acc4b23fd 100644 --- a/invokeai/app/services/users/users_default.py +++ b/invokeai/app/services/users/users_default.py @@ -7,7 +7,14 @@ from invokeai.app.services.auth.password_utils import hash_password, validate_password_strength, verify_password from invokeai.app.services.shared.sqlite.sqlite_database import SqliteDatabase from invokeai.app.services.users.users_base import UserServiceBase -from invokeai.app.services.users.users_common import UserCreateRequest, UserDTO, UserUpdateRequest +from invokeai.app.services.users.users_common import ( + LastAdministratorError, + UserCreateRequest, + UserDTO, + UserUpdateRequest, +) + +LAST_ADMIN_DETAIL = "Cannot remove the last administrator" class UserService(UserServiceBase): @@ -159,6 +166,13 @@ def update(self, user_id: str, changes: UserUpdateRequest, strict_password_check query = f"UPDATE users SET {', '.join(updates)} WHERE user_id = ?" with self._db.transaction() as cursor: + # BEGIN IMMEDIATE takes the write lock up front, so the guard below reads a count + # no other connection can change before this transaction commits. Without it the + # SELECT would run in autocommit and a second process (invoke-usermod) could slip + # its own write in between. In-process callers are additionally serialized by the + # database's shared RLock. + cursor.execute("BEGIN IMMEDIATE") + self._assert_not_last_admin(cursor, user_id, is_admin=changes.is_admin, is_active=changes.is_active) cursor.execute(query, params) updated_user = self.get(user_id) @@ -173,6 +187,10 @@ def delete(self, user_id: str) -> None: raise ValueError(f"User {user_id} not found") with self._db.transaction() as cursor: + # See the note in `update`: the guard and the write share one write-locked + # transaction so the admin count cannot change between them. + cursor.execute("BEGIN IMMEDIATE") + self._assert_not_last_admin(cursor, user_id, is_deleting=True) cursor.execute("DELETE FROM users WHERE user_id = ?", (user_id,)) def authenticate(self, email: str, password: str) -> UserDTO | None: @@ -286,3 +304,41 @@ def count_admins(self) -> int: cursor.execute("SELECT COUNT(*) FROM users WHERE is_admin = TRUE AND is_active = TRUE") row = cursor.fetchone() return int(row[0]) if row else 0 + + def _assert_not_last_admin( + self, + cursor: sqlite3.Cursor, + user_id: str, + *, + is_deleting: bool = False, + is_admin: bool | None = None, + is_active: bool | None = None, + ) -> None: + """Reject a change that would drop the number of active administrators to zero. + + Must be called on the cursor of the transaction that performs the write, after that + transaction has taken its write lock — see the ``BEGIN IMMEDIATE`` in the callers. + Reading the count in a separate transaction is what made this a TOCTOU: two callers + could each observe two admins and each proceed to remove one. + + ``is_admin``/``is_active`` are the *requested* values, where ``None`` means "not being + changed" — deletion is signalled separately by ``is_deleting`` rather than by both + being ``None``, which would also describe a rename. Only a change that actually + revokes administrator status is checked, so renaming the last admin stays allowed. + """ + cursor.execute("SELECT is_admin, is_active FROM users WHERE user_id = ?", (user_id,)) + row = cursor.fetchone() + if row is None: + return + + # An admin who is already inactive is not counted, so removing them changes nothing. + if not (bool(row[0]) and bool(row[1])): + return + + if not (is_deleting or is_admin is False or is_active is False): + return + + cursor.execute("SELECT COUNT(*) FROM users WHERE is_admin = TRUE AND is_active = TRUE") + count_row = cursor.fetchone() + if (int(count_row[0]) if count_row else 0) <= 1: + raise LastAdministratorError(LAST_ADMIN_DETAIL) diff --git a/tests/app/routers/test_last_admin_routes.py b/tests/app/routers/test_last_admin_routes.py new file mode 100644 index 00000000000..91ed9873b12 --- /dev/null +++ b/tests/app/routers/test_last_admin_routes.py @@ -0,0 +1,89 @@ +"""The last-admin invariant, at the level the user sees it. + +`tests/app/services/users/test_last_admin_invariant.py` covers the guard itself, including +its behaviour under concurrency. These pin the HTTP contract: the service raises a +`LastAdministratorError`, and because that subclasses `ValueError` the existing handlers in +`auth.py` turn it into a 400 rather than letting it escape as a 500. + +`PATCH /auth/users/{id}` is the case that had no guard at all before this change — only +`delete_user` checked, so demoting or deactivating the sole administrator succeeded and left +the instance with none. +""" + +from typing import Any + +from fastapi import status +from fastapi.testclient import TestClient + +from invokeai.app.services.invoker import Invoker + + +def _auth(token: str) -> dict[str, str]: + return {"Authorization": f"Bearer {token}"} + + +def _admin_id(mock_invoker: Invoker) -> str: + admin = mock_invoker.services.users.get_by_email("admin@test.com") + assert admin is not None + return admin.user_id + + +def test_demoting_the_last_admin_returns_400( + enable_multiuser: Any, client: TestClient, admin_token: str, mock_invoker: Invoker +) -> None: + user_id = _admin_id(mock_invoker) + + r = client.patch(f"/api/v1/auth/users/{user_id}", headers=_auth(admin_token), json={"is_admin": False}) + + assert r.status_code == status.HTTP_400_BAD_REQUEST, r.text + assert mock_invoker.services.users.count_admins() == 1 + + +def test_deactivating_the_last_admin_returns_400( + enable_multiuser: Any, client: TestClient, admin_token: str, mock_invoker: Invoker +) -> None: + user_id = _admin_id(mock_invoker) + + r = client.patch(f"/api/v1/auth/users/{user_id}", headers=_auth(admin_token), json={"is_active": False}) + + assert r.status_code == status.HTTP_400_BAD_REQUEST, r.text + assert mock_invoker.services.users.count_admins() == 1 + + +def test_deleting_the_last_admin_returns_400( + enable_multiuser: Any, client: TestClient, admin_token: str, mock_invoker: Invoker +) -> None: + user_id = _admin_id(mock_invoker) + + r = client.delete(f"/api/v1/auth/users/{user_id}", headers=_auth(admin_token)) + + assert r.status_code == status.HTTP_400_BAD_REQUEST, r.text + assert mock_invoker.services.users.count_admins() == 1 + + +def test_renaming_the_last_admin_still_succeeds( + enable_multiuser: Any, client: TestClient, admin_token: str, mock_invoker: Invoker +) -> None: + """The guard must not turn into a blanket lock on the last admin's record.""" + user_id = _admin_id(mock_invoker) + + r = client.patch(f"/api/v1/auth/users/{user_id}", headers=_auth(admin_token), json={"display_name": "Renamed"}) + + assert r.status_code == status.HTTP_200_OK, r.text + assert r.json()["display_name"] == "Renamed" + + +def test_demoting_an_admin_when_another_exists_succeeds( + enable_multiuser: Any, client: TestClient, admin_token: str, mock_invoker: Invoker +) -> None: + from invokeai.app.services.users.users_common import UserCreateRequest + + second = mock_invoker.services.users.create( + UserCreateRequest(email="admin2@test.com", display_name="Second", password="TestPass123", is_admin=True) + ) + + r = client.patch(f"/api/v1/auth/users/{second.user_id}", headers=_auth(admin_token), json={"is_admin": False}) + + assert r.status_code == status.HTTP_200_OK, r.text + assert r.json()["is_admin"] is False + assert mock_invoker.services.users.count_admins() == 1 diff --git a/tests/app/services/users/test_last_admin_invariant.py b/tests/app/services/users/test_last_admin_invariant.py new file mode 100644 index 00000000000..5de84c07f29 --- /dev/null +++ b/tests/app/services/users/test_last_admin_invariant.py @@ -0,0 +1,238 @@ +"""The instance must never be left with zero active administrators. + +Authorization is derived from the database on every request, so removing the last active +admin is irreversible from inside the app: no authenticated path back exists. Worse, it +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 guard used to live only in the `delete_user` route, where it read `count_admins()` in +its own transaction and then wrote in another. That is a TOCTOU: two callers each observe +two admins and each remove one. It is reachable three ways — + + * two concurrent requests, now that route handlers run in a threadpool; + * the `invoke-usermod` / `invoke-userdel` CLIs, which construct `UserService` directly + and never reach the route guard at all; + * a second process racing the server on the same database file. + +so the invariant belongs in the service, inside the transaction that performs the write. +""" + +import threading +from logging import Logger + +import pytest + +from invokeai.app.services.shared.sqlite.sqlite_database import SqliteDatabase +from invokeai.app.services.users.users_common import ( + LastAdministratorError, + UserCreateRequest, + UserUpdateRequest, +) +from invokeai.app.services.users.users_default import UserService + +PASSWORD = "Sup3rSecret!pass" + + +@pytest.fixture +def db() -> SqliteDatabase: + db = SqliteDatabase(db_path=None, logger=Logger("test_last_admin"), verbose=False) + db._conn.execute(""" + CREATE TABLE users ( + user_id TEXT NOT NULL PRIMARY KEY, + email TEXT NOT NULL UNIQUE, + display_name TEXT, + password_hash TEXT NOT NULL, + is_admin BOOLEAN NOT NULL DEFAULT FALSE, + is_active BOOLEAN NOT NULL DEFAULT TRUE, + created_at DATETIME NOT NULL DEFAULT(STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')), + updated_at DATETIME NOT NULL DEFAULT(STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')), + last_login_at DATETIME + ); + """) + db._conn.commit() + return db + + +@pytest.fixture +def users(db: SqliteDatabase) -> UserService: + return UserService(db) + + +def _make(users: UserService, email: str, *, is_admin: bool) -> str: + user = users.create( + UserCreateRequest(email=email, display_name=email, password=PASSWORD, is_admin=is_admin), + strict_password_checking=False, + ) + return user.user_id + + +# region single caller + + +def test_deleting_the_last_admin_is_rejected(users: UserService) -> None: + admin = _make(users, "admin@test.com", is_admin=True) + _make(users, "plain@test.com", is_admin=False) + + with pytest.raises(LastAdministratorError): + users.delete(admin) + + assert users.count_admins() == 1 + + +def test_demoting_the_last_admin_is_rejected(users: UserService) -> None: + """The gap this PR closes: only `delete` was ever guarded.""" + admin = _make(users, "admin@test.com", is_admin=True) + + with pytest.raises(LastAdministratorError): + users.update(admin, UserUpdateRequest(is_admin=False), strict_password_checking=False) + + assert users.count_admins() == 1 + assert users.get(admin).is_admin is True + + +def test_deactivating_the_last_admin_is_rejected(users: UserService) -> None: + """Deactivation removes an admin from `count_admins()` just as demotion does.""" + admin = _make(users, "admin@test.com", is_admin=True) + + with pytest.raises(LastAdministratorError): + users.update(admin, UserUpdateRequest(is_active=False), strict_password_checking=False) + + assert users.count_admins() == 1 + assert users.get(admin).is_active is True + + +def test_the_error_is_a_value_error(users: UserService) -> None: + """Route handlers and the CLIs already map service `ValueError` to a friendly message.""" + admin = _make(users, "admin@test.com", is_admin=True) + + with pytest.raises(ValueError): + users.delete(admin) + + +# endregion + +# region changes that must still be allowed + + +def test_renaming_the_last_admin_is_allowed(users: UserService) -> None: + """The guard keys on the requested values, not on the target being an admin.""" + admin = _make(users, "admin@test.com", is_admin=True) + + updated = users.update(admin, UserUpdateRequest(display_name="Renamed"), strict_password_checking=False) + + assert updated.display_name == "Renamed" + assert updated.is_admin is True + + +def test_password_change_for_the_last_admin_is_allowed(users: UserService) -> None: + admin = _make(users, "admin@test.com", is_admin=True) + + users.update(admin, UserUpdateRequest(password="An0ther!Password"), strict_password_checking=False) + + assert users.authenticate("admin@test.com", "An0ther!Password") is not None + + +def test_demoting_one_of_two_admins_is_allowed(users: UserService) -> None: + first = _make(users, "a1@test.com", is_admin=True) + _make(users, "a2@test.com", is_admin=True) + + users.update(first, UserUpdateRequest(is_admin=False), strict_password_checking=False) + + assert users.count_admins() == 1 + + +def test_deleting_an_already_inactive_admin_is_allowed(users: UserService) -> None: + """An inactive admin is not counted, so removing them cannot reach zero.""" + active = _make(users, "active@test.com", is_admin=True) + inactive = _make(users, "inactive@test.com", is_admin=True) + users.update(inactive, UserUpdateRequest(is_active=False), strict_password_checking=False) + assert users.count_admins() == 1 + + users.delete(inactive) + + assert users.get(inactive) is None + assert users.get(active) is not None + + +def test_deleting_a_non_admin_is_allowed(users: UserService) -> None: + _make(users, "admin@test.com", is_admin=True) + plain = _make(users, "plain@test.com", is_admin=False) + + users.delete(plain) + + assert users.get(plain) is None + + +# endregion + +# region concurrency — the reason the guard moved into the transaction + + +def _race(target, args_a, args_b) -> list[BaseException | None]: + """Run `target` twice concurrently, returning each call's exception (or None).""" + results: list[BaseException | None] = [None, None] + barrier = threading.Barrier(2) + + def run(index: int, args: tuple) -> None: + barrier.wait() + try: + target(*args) + except BaseException as e: # noqa: BLE001 - recorded and asserted on below + results[index] = e + + threads = [ + threading.Thread(target=run, args=(0, args_a)), + threading.Thread(target=run, args=(1, args_b)), + ] + for t in threads: + t.start() + for t in threads: + t.join(timeout=10) + assert not any(t.is_alive() for t in threads), "a racing thread deadlocked" + return results + + +def test_concurrent_deletes_cannot_remove_both_admins(users: UserService) -> None: + """Two admins, two concurrent deletes of *different* rows. Exactly one must survive. + + With the guard in the route this failed: both callers read `count_admins() == 2`, + both passed, and the instance was left with none. + """ + first = _make(users, "a1@test.com", is_admin=True) + second = _make(users, "a2@test.com", is_admin=True) + + errors = _race(users.delete, (first,), (second,)) + + assert users.count_admins() == 1, "both concurrent deletes succeeded; the invariant is not atomic" + assert sum(isinstance(e, LastAdministratorError) for e in errors) == 1 + + +def test_concurrent_demotions_cannot_remove_both_admins(users: UserService) -> None: + first = _make(users, "a1@test.com", is_admin=True) + second = _make(users, "a2@test.com", is_admin=True) + demote = UserUpdateRequest(is_admin=False) + + def update(user_id: str) -> None: + users.update(user_id, demote, strict_password_checking=False) + + errors = _race(update, (first,), (second,)) + + assert users.count_admins() == 1 + assert sum(isinstance(e, LastAdministratorError) for e in errors) == 1 + + +def test_concurrent_delete_and_demotion_cannot_remove_both_admins(users: UserService) -> None: + """The two paths must be mutually exclusive, not just each internally consistent.""" + first = _make(users, "a1@test.com", is_admin=True) + second = _make(users, "a2@test.com", is_admin=True) + + def demote(user_id: str) -> None: + users.update(user_id, UserUpdateRequest(is_admin=False), strict_password_checking=False) + + errors = _race(lambda uid: users.delete(uid) if uid == first else demote(uid), (first,), (second,)) + + assert users.count_admins() == 1 + assert sum(isinstance(e, LastAdministratorError) for e in errors) == 1 + + +# endregion From e55344fd50249e1859551507e0a97bb3620caa36 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sat, 8 Aug 2026 22:31:20 -0400 Subject: [PATCH 06/19] fix(sockets): a socket dropping mid-loop must not abandon re-authorization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_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) --- invokeai/app/api/sockets.py | 15 ++++- tests/app/test_socket_privilege_revocation.py | 57 +++++++++++++++++++ 2 files changed, 70 insertions(+), 2 deletions(-) diff --git a/invokeai/app/api/sockets.py b/invokeai/app/api/sockets.py index 698736b1860..6cba8118917 100644 --- a/invokeai/app/api/sockets.py +++ b/invokeai/app/api/sockets.py @@ -278,15 +278,26 @@ async def _handle_user_access_changed(self, event: FastAPIEvent[UserAccessChange _, event_data = event affected_sids = [sid for sid, info in self._socket_users.items() if info.get("user_id") == event_data.user_id] for sid in affected_sids: + # `affected_sids` is a snapshot, and `disconnect()` below yields to the event + # loop while it flushes the packet — the socket's own disconnect handler then + # removes it from `_socket_users`. Re-look-up rather than re-index: a socket + # that dropped (client, ping timeout, or an interleaved event for the same + # user) would otherwise raise KeyError here and abandon re-authorization for + # every remaining sid in the loop, leaving them on stale privileges with no + # retry. Nothing observes the failure either — the dispatcher runs this + # handler as a bare task. + info = self._socket_users.get(sid) + if info is None: + continue if not event_data.is_active: logger.info(f"Disconnecting socket {sid}: user {event_data.user_id} deactivated or deleted") await self._sio.disconnect(sid) continue - if self._socket_users[sid].get("token_epoch", 0) != event_data.token_epoch: + if info.get("token_epoch", 0) != event_data.token_epoch: logger.info(f"Disconnecting socket {sid}: user {event_data.user_id} revoked its earlier sessions") await self._sio.disconnect(sid) continue - self._socket_users[sid]["is_admin"] = event_data.is_admin + info["is_admin"] = event_data.is_admin if event_data.is_admin: await self._sio.enter_room(sid, "admin") logger.info(f"Socket {sid} joined admin room: user {event_data.user_id} promoted") diff --git a/tests/app/test_socket_privilege_revocation.py b/tests/app/test_socket_privilege_revocation.py index 592457d7fd2..1be245fd990 100644 --- a/tests/app/test_socket_privilege_revocation.py +++ b/tests/app/test_socket_privilege_revocation.py @@ -8,6 +8,7 @@ admin room. """ +import asyncio from types import SimpleNamespace from unittest.mock import AsyncMock @@ -202,6 +203,62 @@ async def test_other_users_sockets_are_untouched(self) -> None: socketio._sio.leave_room.assert_not_awaited() assert socketio._socket_users["sid-admin"]["is_admin"] is True + @pytest.mark.anyio + async def test_a_socket_dropping_mid_loop_does_not_abandon_the_rest(self) -> None: + """The real `disconnect` awaits I/O and then runs the disconnect handler, which + removes the socket from `_socket_users`. Any socket that goes away during that + yield — its own client dropping, a ping timeout, or a second event for the same + user — must not stop the loop: the remaining sockets would keep the privileges + this handler exists to revoke, silently, since the dispatcher runs it as a bare + task with nothing to observe the failure. + """ + socketio = self._connected_socketio() + socketio._socket_users["sid-user-c"] = {"user_id": "user-1", "is_admin": True, "token_epoch": 0} + + async def disconnect(sid: str) -> None: + # Stand in for the packet flush: yield, and drop an as-yet-unvisited socket + # of the same user while suspended, as a client disconnect would. + await asyncio.sleep(0) + socketio._socket_users.pop("sid-user-b", None) + await socketio._handle_disconnect(sid) + + socketio._sio.disconnect = AsyncMock(side_effect=disconnect) + event = UserAccessChangedEvent.build(user_id="user-1", is_admin=False, is_active=False) + + await socketio._handle_user_access_changed(("user_access_changed", event)) + + # sid-user-a is disconnected first and takes sid-user-b with it; sid-user-c must + # still be reached rather than left connected on revoked credentials. + disconnected = {call.args[0] for call in socketio._sio.disconnect.await_args_list} + assert disconnected == {"sid-user-a", "sid-user-c"} + assert "sid-user-c" not in socketio._socket_users + + @pytest.mark.anyio + async def test_demotion_survives_a_socket_dropping_mid_loop(self) -> None: + """Same interleaving on the demotion path, where an abandoned socket is worse: it + stays in the admin room *and* keeps a cached is_admin of True, which + `_handle_sub_queue` would use to re-add it on the next subscription.""" + socketio = self._connected_socketio() + socketio._socket_users = { + "sid-a": {"user_id": "admin-1", "is_admin": True, "token_epoch": 0}, + "sid-b": {"user_id": "admin-1", "is_admin": True, "token_epoch": 1}, + "sid-c": {"user_id": "admin-1", "is_admin": True, "token_epoch": 0}, + } + + async def disconnect(sid: str) -> None: + await asyncio.sleep(0) + socketio._socket_users.pop("sid-b", None) + await socketio._handle_disconnect(sid) + + socketio._sio.disconnect = AsyncMock(side_effect=disconnect) + # Epoch 1: sid-a and sid-c hold superseded tokens, sid-b is current. + event = UserAccessChangedEvent.build(user_id="admin-1", is_admin=False, is_active=True, token_epoch=1) + + await socketio._handle_user_access_changed(("user_access_changed", event)) + + disconnected = {call.args[0] for call in socketio._sio.disconnect.await_args_list} + assert disconnected == {"sid-a", "sid-c"} + class TestTokenEpochOnSockets: """A revoked token must not open a socket, and must not keep an open one alive. From 7de5f902aa58bd3d59e8e7682aafae8ae6d58e77 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sat, 8 Aug 2026 22:31:33 -0400 Subject: [PATCH 07/19] fix(session-processor): re-read the owner before cancelling its running item MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_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) --- .../session_processor_default.py | 26 +++++++++++++------ .../test_privilege_revocation.py | 26 +++++++++++++++++++ 2 files changed, 44 insertions(+), 8 deletions(-) diff --git a/invokeai/app/services/session_processor/session_processor_default.py b/invokeai/app/services/session_processor/session_processor_default.py index 5b1c3a82a59..387cb8c2dfc 100644 --- a/invokeai/app/services/session_processor/session_processor_default.py +++ b/invokeai/app/services/session_processor/session_processor_default.py @@ -630,15 +630,12 @@ async def _on_user_access_changed(self, event: FastAPIEvent[UserAccessChangedEve return # A single user may have items running on several workers concurrently, so # cancel every match rather than stopping at the first. - item_ids: list[int] = [] + queue_items: list[SessionQueueItem] = [] for worker in self._workers: queue_item = worker.queue_item if queue_item is not None and queue_item.user_id == event_data.user_id: - self._invoker.services.logger.warning( - f"Canceling queue item {queue_item.item_id}: owner {queue_item.user_id} was deactivated or deleted" - ) - item_ids.append(queue_item.item_id) - if not item_ids: + queue_items.append(queue_item) + if not queue_items: return # Run the cancellations in a thread. `cancel_queue_item` walks the workflow-call @@ -652,10 +649,23 @@ async def _on_user_access_changed(self, event: FastAPIEvent[UserAccessChangedEve # a cancel event set while the row is still non-terminal is treated as a stale # signal from a previous item and cleared (see the guard after dequeue), which # would discard this cancellation. + # Re-read the owner at the point of decision rather than trusting the event. + # Handlers are dispatched as independent tasks, so a deactivate immediately + # followed by a reactivate can leave this one parked here while the second event + # has already come and gone (it returns early above) — cancelling then would kill + # a running item of an account the database says is active, with nothing to undo + # it. `queue_owner_is_active` is the same gate the dequeue and between-node checks + # use, including its fail-to-active policy for a failed lookup: skipping is safe + # because the between-node gate re-checks at the very next node. def _cancel_all() -> None: - for item_id in item_ids: + for item in queue_items: + if queue_owner_is_active(self._invoker.services, item): + continue + self._invoker.services.logger.warning( + f"Canceling queue item {item.item_id}: owner {item.user_id} was deactivated or deleted" + ) with suppress(SessionQueueItemNotFoundError): - self._invoker.services.session_queue.cancel_queue_item(item_id) + self._invoker.services.session_queue.cancel_queue_item(item.item_id) await run_in_threadpool(_cancel_all) diff --git a/tests/app/services/session_processor/test_privilege_revocation.py b/tests/app/services/session_processor/test_privilege_revocation.py index dc43f786ca9..2b692c7530a 100644 --- a/tests/app/services/session_processor/test_privilege_revocation.py +++ b/tests/app/services/session_processor/test_privilege_revocation.py @@ -192,6 +192,32 @@ async def test_deactivation_cancels_items_on_every_worker(self) -> None: assert sorted(c.args[0] for c in services.session_queue.cancel_queue_item.call_args_list) == [11, 13] + @pytest.mark.anyio + async def test_reactivation_before_the_cancel_lands_spares_the_item(self) -> None: + """The owner is re-read at the point of decision rather than trusted from the event. + + Each event is dispatched as its own task, so a deactivate immediately followed by a + reactivate can leave the first handler still parked while the second has come and + gone (it returns early). Cancelling on the stale snapshot would kill a running item + of an account the database says is active, and nothing undoes a cancellation. + """ + services = _services(users_by_id={"user-1": _active("user-1")}) + processor = self._processor(services, _queue_item(user_id="user-1", item_id=11)) + + await processor._on_user_access_changed(self._event("user-1", is_active=False)) + + services.session_queue.cancel_queue_item.assert_not_called() + + @pytest.mark.anyio + async def test_single_user_mode_does_not_cancel(self) -> None: + """Ownership is not enforced anywhere else in single-user mode either.""" + services = _services(multiuser=False) + processor = self._processor(services, _queue_item(user_id="user-1", item_id=11)) + + await processor._on_user_access_changed(self._event("user-1", is_active=False)) + + services.session_queue.cancel_queue_item.assert_not_called() + @pytest.fixture def anyio_backend() -> str: From 8c3d78bf4955db76f5f1bf31f9f00318e422447f Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sat, 8 Aug 2026 22:31:33 -0400 Subject: [PATCH 08/19] fix(tests): stub `configuration` in the device-pin test's invoker `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) --- .../test_session_processor_cancel_guard.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/app/services/session_processor/test_session_processor_cancel_guard.py b/tests/app/services/session_processor/test_session_processor_cancel_guard.py index 24250349cc3..dbed63f0ee1 100644 --- a/tests/app/services/session_processor/test_session_processor_cancel_guard.py +++ b/tests/app/services/session_processor/test_session_processor_cancel_guard.py @@ -243,7 +243,14 @@ def run_item(item): worker = _SessionWorker(device=torch.device("cuda:1"), runner=runner) processor = DefaultSessionProcessor() processor._invoker = SimpleNamespace( # type: ignore[attr-defined] - services=SimpleNamespace(session_queue=_ClaimQueue(), logger=MagicMock(), image_moves=None) + services=SimpleNamespace( + session_queue=_ClaimQueue(), + logger=MagicMock(), + image_moves=None, + # Single-user mode: the post-dequeue owner check short-circuits, keeping this + # test focused on device pinning. + configuration=SimpleNamespace(multiuser=False), + ) ) processor._polling_interval = 0 processor._thread_semaphore = BoundedSemaphore(1) From 78359a37756e7a39f8f934e11592321aeac46fa8 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sat, 8 Aug 2026 22:31:56 -0400 Subject: [PATCH 09/19] fix(tests): add token_epoch to the last-admin fixture's users table 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) --- tests/app/services/users/test_last_admin_invariant.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/app/services/users/test_last_admin_invariant.py b/tests/app/services/users/test_last_admin_invariant.py index 5de84c07f29..fead1f686db 100644 --- a/tests/app/services/users/test_last_admin_invariant.py +++ b/tests/app/services/users/test_last_admin_invariant.py @@ -46,7 +46,8 @@ def db() -> SqliteDatabase: is_active BOOLEAN NOT NULL DEFAULT TRUE, created_at DATETIME NOT NULL DEFAULT(STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')), updated_at DATETIME NOT NULL DEFAULT(STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')), - last_login_at DATETIME + last_login_at DATETIME, + token_epoch INTEGER NOT NULL DEFAULT 0 ); """) db._conn.commit() From 10a22265dae4a7c8e8a8052074b509b3388fc462 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sat, 8 Aug 2026 22:32:11 -0400 Subject: [PATCH 10/19] fix(auth): stop the system account from laundering away the last-admin guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- invokeai/app/api/routers/auth.py | 44 ++++++---- invokeai/app/services/users/users_common.py | 19 ++++ invokeai/app/services/users/users_default.py | 47 +++++++++- tests/app/routers/test_last_admin_routes.py | 56 ++++++++++++ .../users/test_last_admin_invariant.py | 86 +++++++++++++++++++ 5 files changed, 233 insertions(+), 19 deletions(-) diff --git a/invokeai/app/api/routers/auth.py b/invokeai/app/api/routers/auth.py index 64a2f585599..163aa5b0661 100644 --- a/invokeai/app/api/routers/auth.py +++ b/invokeai/app/api/routers/auth.py @@ -23,6 +23,9 @@ get_token_remaining_seconds, ) from invokeai.app.services.users.users_common import ( + LAST_ADMIN_DETAIL, + SYSTEM_USER_ID, + SYSTEM_USER_PROTECTED_DETAIL, UserCreateRequest, UserDTO, UserUpdateRequest, @@ -35,11 +38,6 @@ TOKEN_EXPIRATION_NORMAL = 1 # 1 day for normal login TOKEN_EXPIRATION_REMEMBER_ME = 7 # 7 days for "remember me" login -# Owner of everything that predates multiuser support (created by migration_27). Not a -# login account: it has an empty password hash and is hidden from the user list. -SYSTEM_USER_ID = "system" -SYSTEM_USER_PROTECTED_DETAIL = "The system user cannot be deleted or deactivated" - def _issue_replacement_token(http_request: Request, response: Response, user: UserDTO, remember_me: bool) -> None: """Hand the caller a token minted under the user's *current* revocation epoch. @@ -565,19 +563,27 @@ async def update_user( The updated user Raises: - HTTPException: 400 if password is weak, or if the change would remove the - last administrator + HTTPException: 400 if password is weak, if the change would remove the last + administrator, or if it targets the protected system account HTTPException: 404 if user not found """ user_service = ApiDependencies.invoker.services.users config = ApiDependencies.invoker.services.configuration before = user_service.get(user_id) + # Match `get_user`/`delete_user`, which 404 for an unknown id. Without this the request + # falls through to the service's `ValueError("User ... not found")` and the route's + # `except ValueError` reports it as a 400, contradicting this endpoint's own contract. + if before is None: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="User not found") # The system user owns everything migrated from before multiuser support. Deactivating # it would strand that content: its queue items stop at the dequeue gate, and reads and - # saves against system-owned media raise PermissionError. It is not a login account, so - # there is no reason to disable it. - if user_id == SYSTEM_USER_ID and request.is_active is False: + # saves against system-owned media raise PermissionError. Promoting it or giving it a + # password is refused for a different reason — see `_assert_system_user_protected`, + # which is the backstop this friendly message fronts. + if user_id == SYSTEM_USER_ID and ( + request.is_active is False or request.is_admin is True or request.password is not None + ): raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, detail=SYSTEM_USER_PROTECTED_DETAIL, @@ -589,15 +595,14 @@ async def update_user( # to zero, which re-opens the unauthenticated `/auth/setup` endpoint to any caller. # `delete_user` guards the same invariant. if ( - before is not None - and before.is_admin + before.is_admin and before.is_active and (request.is_admin is False or request.is_active is False) and user_service.count_admins() <= 1 ): raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, - detail="Cannot remove the last administrator", + detail=LAST_ADMIN_DETAIL, ) try: @@ -616,7 +621,7 @@ async def update_user( # instead of persisting until reconnect or token expiry. A password reset bumps # the epoch without touching is_admin/is_active, and must drop the target's open # sockets too, so it is part of this condition. - if before is not None and ( + if ( before.is_admin != updated.is_admin or before.is_active != updated.is_active or before.token_epoch != updated.token_epoch @@ -631,7 +636,10 @@ async def update_user( # An admin resetting their *own* password would otherwise lock themselves out: the # epoch bump kills the token that authenticated this request, and the sliding-window # middleware correctly refuses to refresh a revoked one. Mirror what /auth/me does. - if request.password is not None and updated.user_id == current_user.user_id: + # An admin who deactivated themselves in the same request gets nothing: the token + # would be rejected on its next use anyway, and setting the media cookie for an + # account this request just disabled advertises a session that does not exist. + if request.password is not None and updated.user_id == current_user.user_id and updated.is_active: _issue_replacement_token(http_request, response, updated, current_user.remember_me) return updated @@ -669,11 +677,13 @@ async def delete_user( detail=SYSTEM_USER_PROTECTED_DETAIL, ) - # Prevent deleting the last active admin + # Prevent deleting the last active admin. Same wording as the service backstop: this + # pre-check can lose a race and let the service reject the delete instead, and one + # endpoint should not report one condition two different ways. if user.is_admin and user.is_active and user_service.count_admins() <= 1: raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, - detail="Cannot delete the last administrator", + detail=LAST_ADMIN_DETAIL, ) try: diff --git a/invokeai/app/services/users/users_common.py b/invokeai/app/services/users/users_common.py index 0b9dd3b79c6..723e2661478 100644 --- a/invokeai/app/services/users/users_common.py +++ b/invokeai/app/services/users/users_common.py @@ -117,9 +117,28 @@ class UserUpdateRequest(BaseModel): is_active: bool | None = Field(default=None, description="Whether user account should be active") +LAST_ADMIN_DETAIL = "Cannot remove the last administrator" + + class LastAdministratorError(ValueError): """Raised when a change would leave the instance with no active administrator. Subclasses :class:`ValueError` so that callers which already map service-layer ``ValueError`` to a 400 keep working unchanged. """ + + +# Owner of everything that predates multiuser support (created by migration_27). Not a +# login account: it has an empty password hash and is hidden from the user list. +SYSTEM_USER_ID = "system" +SYSTEM_USER_PROTECTED_DETAIL = ( + "The system user cannot be deleted, deactivated, promoted to administrator, or given a password" +) + + +class SystemUserProtectedError(ValueError): + """Raised when a change would delete, disable, or weaponize the ``system`` account. + + Subclasses :class:`ValueError` for the same reason as + :class:`LastAdministratorError`. + """ diff --git a/invokeai/app/services/users/users_default.py b/invokeai/app/services/users/users_default.py index 65acc4b23fd..b277b783fd1 100644 --- a/invokeai/app/services/users/users_default.py +++ b/invokeai/app/services/users/users_default.py @@ -8,14 +8,16 @@ from invokeai.app.services.shared.sqlite.sqlite_database import SqliteDatabase from invokeai.app.services.users.users_base import UserServiceBase from invokeai.app.services.users.users_common import ( + LAST_ADMIN_DETAIL, + SYSTEM_USER_ID, + SYSTEM_USER_PROTECTED_DETAIL, LastAdministratorError, + SystemUserProtectedError, UserCreateRequest, UserDTO, UserUpdateRequest, ) -LAST_ADMIN_DETAIL = "Cannot remove the last administrator" - class UserService(UserServiceBase): """SQLite-based user service.""" @@ -125,6 +127,10 @@ def update(self, user_id: str, changes: UserUpdateRequest, strict_password_check if user is None: raise ValueError(f"User {user_id} not found") + self._assert_system_user_protected( + user_id, is_admin=changes.is_admin, is_active=changes.is_active, password=changes.password + ) + # Validate password if provided if changes.password is not None: if strict_password_checking: @@ -186,6 +192,8 @@ def delete(self, user_id: str) -> None: if user is None: raise ValueError(f"User {user_id} not found") + self._assert_system_user_protected(user_id, is_deleting=True) + with self._db.transaction() as cursor: # See the note in `update`: the guard and the write share one write-locked # transaction so the admin count cannot change between them. @@ -342,3 +350,38 @@ def _assert_not_last_admin( count_row = cursor.fetchone() if (int(count_row[0]) if count_row else 0) <= 1: raise LastAdministratorError(LAST_ADMIN_DETAIL) + + def _assert_system_user_protected( + self, + user_id: str, + *, + is_deleting: bool = False, + is_admin: bool | None = None, + is_active: bool | None = None, + password: str | None = None, + ) -> None: + """Reject changes to the ``system`` account that no legitimate operation needs. + + The system row owns every board, image, workflow, and queue item carried over from + before multiuser support. Deleting or deactivating it strands all of that: queued + items are rejected at dequeue and media reads and saves raise ``PermissionError``. + + Promotion and password-setting are refused for a different reason. The system row + is active but has an empty password hash, so it can never authenticate — yet + ``count_admins()`` and ``has_admin()`` count any active admin row. Promoting it + therefore inflates the administrator count with an administrator nobody can log in + as, which is enough to satisfy the last-admin guard while the real administrator is + demoted, leaving the instance with no usable administration and no way back in. + Keeping the system row permanently non-admin is what makes that count mean + "administrators who can actually log in". Giving it a password would turn the owner + of all pre-multiuser content into a login account, which is the same hole from the + other end. + + Lives in the service rather than only in the routes so the ``invoke-usermod`` / + ``invoke-userdel`` CLIs, which construct :class:`UserService` directly, are covered + too — the same reasoning that moved the last-admin invariant down here. + """ + if user_id != SYSTEM_USER_ID: + return + if is_deleting or is_active is False or is_admin is True or password is not None: + raise SystemUserProtectedError(SYSTEM_USER_PROTECTED_DETAIL) diff --git a/tests/app/routers/test_last_admin_routes.py b/tests/app/routers/test_last_admin_routes.py index 91ed9873b12..3bacd553ecb 100644 --- a/tests/app/routers/test_last_admin_routes.py +++ b/tests/app/routers/test_last_admin_routes.py @@ -87,3 +87,59 @@ def test_demoting_an_admin_when_another_exists_succeeds( assert r.status_code == status.HTTP_200_OK, r.text assert r.json()["is_admin"] is False assert mock_invoker.services.users.count_admins() == 1 + + +def test_updating_an_unknown_user_returns_404( + enable_multiuser: Any, client: TestClient, admin_token: str, mock_invoker: Invoker +) -> None: + """`get_user` and `delete_user` 404 for an unknown id; this endpoint documents the same + contract but used to fall through to the service's "User ... not found" as a 400.""" + r = client.patch("/api/v1/auth/users/does-not-exist", headers=_auth(admin_token), json={"display_name": "x"}) + + assert r.status_code == status.HTTP_404_NOT_FOUND, r.text + + +def test_promoting_the_system_user_cannot_launder_away_the_last_admin( + enable_multiuser: Any, client: TestClient, admin_token: str, mock_invoker: Invoker +) -> None: + """The system row is active but has an empty password hash, so it can never log in. + Promoting it would raise `count_admins()` to 2 — enough for the last-admin guard to + allow the real administrator to be demoted, leaving nobody able to administer the + instance and `/auth/setup` still closed.""" + users = mock_invoker.services.users + admin_id = _admin_id(mock_invoker) + + promote = client.patch("/api/v1/auth/users/system", headers=_auth(admin_token), json={"is_admin": True}) + + assert promote.status_code == status.HTTP_400_BAD_REQUEST, promote.text + assert users.count_admins() == 1 + + demote = client.patch(f"/api/v1/auth/users/{admin_id}", headers=_auth(admin_token), json={"is_admin": False}) + + assert demote.status_code == status.HTTP_400_BAD_REQUEST, demote.text + assert users.count_admins() == 1 + + +def test_setting_a_password_on_the_system_user_returns_400( + enable_multiuser: Any, client: TestClient, admin_token: str, mock_invoker: Invoker +) -> None: + """Otherwise the owner of every pre-multiuser board, image, and workflow becomes a + login account.""" + r = client.patch("/api/v1/auth/users/system", headers=_auth(admin_token), json={"password": "SystemPass123"}) + + assert r.status_code == status.HTTP_400_BAD_REQUEST, r.text + + login = client.post( + "/api/v1/auth/login", + json={"email": "system@system.invokeai", "password": "SystemPass123", "remember_me": False}, + ) + assert login.status_code != status.HTTP_200_OK + + +def test_deleting_the_system_user_returns_400( + enable_multiuser: Any, client: TestClient, admin_token: str, mock_invoker: Invoker +) -> None: + r = client.delete("/api/v1/auth/users/system", headers=_auth(admin_token)) + + assert r.status_code == status.HTTP_400_BAD_REQUEST, r.text + assert mock_invoker.services.users.get("system") is not None diff --git a/tests/app/services/users/test_last_admin_invariant.py b/tests/app/services/users/test_last_admin_invariant.py index fead1f686db..861389b7b4b 100644 --- a/tests/app/services/users/test_last_admin_invariant.py +++ b/tests/app/services/users/test_last_admin_invariant.py @@ -24,7 +24,9 @@ from invokeai.app.services.shared.sqlite.sqlite_database import SqliteDatabase from invokeai.app.services.users.users_common import ( + SYSTEM_USER_ID, LastAdministratorError, + SystemUserProtectedError, UserCreateRequest, UserUpdateRequest, ) @@ -237,3 +239,87 @@ def demote(user_id: str) -> None: # endregion + +# region the system account + + +def _seed_system_user(db: SqliteDatabase) -> None: + """The row migration_27 creates: active, non-admin, and with an empty password hash.""" + db._conn.execute( + """ + INSERT INTO users (user_id, email, display_name, password_hash, is_admin, is_active) + VALUES ('system', 'system@system.invokeai', 'System', '', FALSE, TRUE); + """ + ) + db._conn.commit() + + +def test_the_system_user_cannot_be_promoted(db: SqliteDatabase, users: UserService) -> None: + """`count_admins()` counts admin rows, but the invariant that matters is "an admin who + can log in". The system row is active and can never authenticate — it has no password — + so promoting it would inflate the count with an unusable administrator, which is enough + to walk the last-admin guard past the real one: + + PATCH /auth/users/system {"is_admin": true} -> count_admins() 1 -> 2 + PATCH /auth/users/{real} {"is_admin": false} -> allowed, count 2 -> 1 + login as system -> 401, empty password hash + + leaving the instance with no usable administration and no authenticated way back. + """ + _seed_system_user(db) + admin = _make(users, "admin@test.com", is_admin=True) + + with pytest.raises(SystemUserProtectedError): + users.update(SYSTEM_USER_ID, UserUpdateRequest(is_admin=True), strict_password_checking=False) + + assert users.count_admins() == 1 + + # And with the first step refused, the second is still blocked. + with pytest.raises(LastAdministratorError): + users.update(admin, UserUpdateRequest(is_admin=False), strict_password_checking=False) + + +def test_the_system_user_cannot_be_given_a_password(db: SqliteDatabase, users: UserService) -> None: + """The other end of the same hole: a password turns the owner of every pre-multiuser + board, image, and workflow into a login account.""" + _seed_system_user(db) + + with pytest.raises(SystemUserProtectedError): + users.update(SYSTEM_USER_ID, UserUpdateRequest(password=PASSWORD), strict_password_checking=False) + + assert users.authenticate("system@system.invokeai", PASSWORD) is None + + +def test_the_system_user_cannot_be_deleted_or_deactivated(db: SqliteDatabase, users: UserService) -> None: + """The routes already refuse both, but `invoke-userdel` / `invoke-usermod` construct + this service directly and never reach a route — the same reason the last-admin guard + lives here.""" + _seed_system_user(db) + + with pytest.raises(SystemUserProtectedError): + users.delete(SYSTEM_USER_ID) + with pytest.raises(SystemUserProtectedError): + users.update(SYSTEM_USER_ID, UserUpdateRequest(is_active=False), strict_password_checking=False) + + system = users.get(SYSTEM_USER_ID) + assert system is not None and system.is_active is True + + +def test_renaming_the_system_user_is_allowed(db: SqliteDatabase, users: UserService) -> None: + """Not a blanket lock on the row — only the changes that would make it dangerous.""" + _seed_system_user(db) + + updated = users.update(SYSTEM_USER_ID, UserUpdateRequest(display_name="Renamed"), strict_password_checking=False) + + assert updated.display_name == "Renamed" + + +def test_the_system_error_is_a_value_error(db: SqliteDatabase, users: UserService) -> None: + """Same reason as the last-admin error: existing route and CLI handlers catch ValueError.""" + _seed_system_user(db) + + with pytest.raises(ValueError): + users.delete(SYSTEM_USER_ID) + + +# endregion From 0c0f8759b4955b5406ff452cfee7639ac9424733 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sat, 8 Aug 2026 22:54:49 -0400 Subject: [PATCH 11/19] fix(session-processor): honor the access-changed event when the owner re-read fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../session_processor_default.py | 26 ++++++++++++++----- .../test_privilege_revocation.py | 18 +++++++++++++ 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/invokeai/app/services/session_processor/session_processor_default.py b/invokeai/app/services/session_processor/session_processor_default.py index 387cb8c2dfc..e074e0b12d5 100644 --- a/invokeai/app/services/session_processor/session_processor_default.py +++ b/invokeai/app/services/session_processor/session_processor_default.py @@ -649,18 +649,32 @@ async def _on_user_access_changed(self, event: FastAPIEvent[UserAccessChangedEve # a cancel event set while the row is still non-terminal is treated as a stale # signal from a previous item and cleared (see the guard after dequeue), which # would discard this cancellation. - # Re-read the owner at the point of decision rather than trusting the event. + # Re-read the owner at the point of decision rather than trusting the event alone. # Handlers are dispatched as independent tasks, so a deactivate immediately # followed by a reactivate can leave this one parked here while the second event # has already come and gone (it returns early above) — cancelling then would kill # a running item of an account the database says is active, with nothing to undo - # it. `queue_owner_is_active` is the same gate the dequeue and between-node checks - # use, including its fail-to-active policy for a failed lookup: skipping is safe - # because the between-node gate re-checks at the very next node. + # it. + # + # Unlike the dequeue and between-node gates, a failed read here does NOT fail to + # "active". Those gates re-run at the next node; this handler is the only thing + # that stops a *single-node* graph, which is checked once before it starts and + # never again. The event is itself evidence of a committed deactivation, so when + # the re-read cannot contradict it, the event stands. def _cancel_all() -> None: for item in queue_items: - if queue_owner_is_active(self._invoker.services, item): - continue + if not self._invoker.services.configuration.multiuser: + return + try: + owner = self._invoker.services.users.get(item.user_id) + except Exception: + self._invoker.services.logger.warning( + f"Could not re-verify owner {item.user_id} of queue item {item.item_id}; " + "honoring the access-changed event and canceling" + ) + else: + if owner is not None and owner.is_active: + continue self._invoker.services.logger.warning( f"Canceling queue item {item.item_id}: owner {item.user_id} was deactivated or deleted" ) diff --git a/tests/app/services/session_processor/test_privilege_revocation.py b/tests/app/services/session_processor/test_privilege_revocation.py index 2b692c7530a..9de2b79a91e 100644 --- a/tests/app/services/session_processor/test_privilege_revocation.py +++ b/tests/app/services/session_processor/test_privilege_revocation.py @@ -218,6 +218,24 @@ async def test_single_user_mode_does_not_cancel(self) -> None: services.session_queue.cancel_queue_item.assert_not_called() + @pytest.mark.anyio + async def test_a_failed_re_read_still_cancels(self) -> None: + """The dequeue and between-node gates fail to "active" on a read error because they + re-run at the next node. This handler has no next node to fall back on — a + single-node graph is checked once, before it starts — so a read that cannot + contradict the event must not override it either.""" + services = _services() + + def explode(user_id: str) -> None: + raise RuntimeError("database is locked") + + services.users.get = explode + processor = self._processor(services, _queue_item(user_id="user-1", item_id=11)) + + await processor._on_user_access_changed(self._event("user-1", is_active=False)) + + services.session_queue.cancel_queue_item.assert_called_once_with(11) + @pytest.fixture def anyio_backend() -> str: From 76dc9208e59ac76b47dcc860f92467ccc01b6334 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sat, 8 Aug 2026 22:54:49 -0400 Subject: [PATCH 12/19] fix(auth): demote the system account on databases where it was promoted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- ...migration_2026_08_08_demote_system_user.py | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 invokeai/app/services/shared/sqlite_migrator/migrations/migration_2026_08_08_demote_system_user.py diff --git a/invokeai/app/services/shared/sqlite_migrator/migrations/migration_2026_08_08_demote_system_user.py b/invokeai/app/services/shared/sqlite_migrator/migrations/migration_2026_08_08_demote_system_user.py new file mode 100644 index 00000000000..4a10313c731 --- /dev/null +++ b/invokeai/app/services/shared/sqlite_migrator/migrations/migration_2026_08_08_demote_system_user.py @@ -0,0 +1,40 @@ +"""Return the ``system`` account to non-administrator, in case it was ever promoted. + +The system row (created by migration_27) owns everything carried over from before +multiuser support. It is deliberately not an administrator and has an empty password +hash, so it can never authenticate — but until the service-level guard landed, an +administrator could ``PATCH /auth/users/system {"is_admin": true}``. + +That is worth undoing rather than merely preventing, because the last-administrator +guard counts rows where ``is_admin AND is_active``, and this row satisfies both while +being unable to log in. A promoted system row therefore inflates the count by one +permanently: it makes the guard willing to demote the last *real* administrator, and it +keeps ``has_admin()`` true afterwards, so ``/auth/setup`` stays closed and no +authenticated path back exists. + +Demoting it restores the count's meaning — "administrators who can actually log in" — +and, on an instance already in that state, lets ``/auth/setup`` reopen so the operator +can recover. Nobody is logged out: no token can carry ``user_id="system"`` in the first +place, since authenticating as it is impossible. +""" + +import sqlite3 + +from invokeai.app.services.shared.sqlite_migrator.sqlite_migrator_common import Migration + + +class DemoteSystemUserCallback: + def __call__(self, cursor: sqlite3.Cursor) -> None: + cursor.execute("UPDATE users SET is_admin = FALSE WHERE user_id = 'system' AND is_admin = TRUE;") + + +def build_migration() -> Migration: + """Clear ``is_admin`` on the ``system`` row. + + Depends on migration_27, which creates the users table and seeds that row. + """ + return Migration( + id="2026_08_08_demote_system_user", + depends_on="migration_27", + callback=DemoteSystemUserCallback(), + ) From 8bfcb88678b81a28fcac2183f4a5cbaa7dd7f7d4 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sat, 8 Aug 2026 22:54:50 -0400 Subject: [PATCH 13/19] test(sockets): describe the mid-loop test by what it actually pins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- tests/app/test_socket_privilege_revocation.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/app/test_socket_privilege_revocation.py b/tests/app/test_socket_privilege_revocation.py index 1be245fd990..44e422d3803 100644 --- a/tests/app/test_socket_privilege_revocation.py +++ b/tests/app/test_socket_privilege_revocation.py @@ -204,13 +204,13 @@ async def test_other_users_sockets_are_untouched(self) -> None: assert socketio._socket_users["sid-admin"]["is_admin"] is True @pytest.mark.anyio - async def test_a_socket_dropping_mid_loop_does_not_abandon_the_rest(self) -> None: - """The real `disconnect` awaits I/O and then runs the disconnect handler, which - removes the socket from `_socket_users`. Any socket that goes away during that - yield — its own client dropping, a ping timeout, or a second event for the same - user — must not stop the loop: the remaining sockets would keep the privileges - this handler exists to revoke, silently, since the dispatcher runs it as a bare - task with nothing to observe the failure. + async def test_a_socket_dropping_mid_loop_is_skipped_not_redisconnected(self) -> None: + """A socket that goes away while the loop is suspended is skipped on its turn. + + The deactivation branch never indexed `_socket_users`, so it did not raise on a + mid-loop removal — see `test_demotion_survives_a_socket_dropping_mid_loop` for the + branch that did. What this pins is the weaker half: the loop's snapshot is not + treated as still-live, so an already-disconnected socket is not disconnected twice. """ socketio = self._connected_socketio() socketio._socket_users["sid-user-c"] = {"user_id": "user-1", "is_admin": True, "token_epoch": 0} From 1762d2d2eb9b3180b6265e596029385620edb577 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sat, 8 Aug 2026 22:54:50 -0400 Subject: [PATCH 14/19] chore(typegen): regenerate for the update_user docstring change 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) --- invokeai/frontend/web/openapi.json | 2 +- invokeai/frontend/web/src/services/api/schema.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/invokeai/frontend/web/openapi.json b/invokeai/frontend/web/openapi.json index 9a356641ac8..a0621351f81 100644 --- a/invokeai/frontend/web/openapi.json +++ b/invokeai/frontend/web/openapi.json @@ -375,7 +375,7 @@ "patch": { "tags": ["authentication"], "summary": "Update User", - "description": "Update a user. Requires admin privileges.\n\nResetting a password revokes the target's existing sessions. An admin resetting\ntheir own password receives a replacement token in ``X-Refreshed-Token`` so they\nare not signed out by their own action.\n\nArgs:\n user_id: The user ID\n request: Fields to update\n\nReturns:\n The updated user\n\nRaises:\n HTTPException: 400 if password is weak, or if the change would remove the\n last administrator\n HTTPException: 404 if user not found", + "description": "Update a user. Requires admin privileges.\n\nResetting a password revokes the target's existing sessions. An admin resetting\ntheir own password receives a replacement token in ``X-Refreshed-Token`` so they\nare not signed out by their own action.\n\nArgs:\n user_id: The user ID\n request: Fields to update\n\nReturns:\n The updated user\n\nRaises:\n HTTPException: 400 if password is weak, if the change would remove the last\n administrator, or if it targets the protected system account\n HTTPException: 404 if user not found", "operationId": "update_user_api_v1_auth_users__user_id__patch", "security": [ { diff --git a/invokeai/frontend/web/src/services/api/schema.ts b/invokeai/frontend/web/src/services/api/schema.ts index 41b6808d960..43d9f84eaea 100644 --- a/invokeai/frontend/web/src/services/api/schema.ts +++ b/invokeai/frontend/web/src/services/api/schema.ts @@ -329,8 +329,8 @@ export type paths = { * The updated user * * Raises: - * HTTPException: 400 if password is weak, or if the change would remove the - * last administrator + * HTTPException: 400 if password is weak, if the change would remove the last + * administrator, or if it targets the protected system account * HTTPException: 404 if user not found */ patch: operations["update_user_api_v1_auth_users__user_id__patch"]; From f765f314072595c2d2415f52009b2fbea190066c Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sun, 9 Aug 2026 17:03:14 -0400 Subject: [PATCH 15/19] fix(auth): close the system account's login path in all three directions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- invokeai/app/api/auth_dependencies.py | 20 ++- ...migration_2026_08_08_demote_system_user.py | 40 ++++-- invokeai/app/services/users/users_common.py | 4 +- invokeai/app/services/users/users_default.py | 10 ++ .../app/routers/test_privilege_revocation.py | 40 +++++- ...migration_2026_08_08_demote_system_user.py | 131 ++++++++++++++++++ .../users/test_last_admin_invariant.py | 28 ++++ 7 files changed, 259 insertions(+), 14 deletions(-) create mode 100644 tests/app/services/shared/sqlite_migrator/migrations/test_migration_2026_08_08_demote_system_user.py diff --git a/invokeai/app/api/auth_dependencies.py b/invokeai/app/api/auth_dependencies.py index 3b19588faf0..bd8c8c065f8 100644 --- a/invokeai/app/api/auth_dependencies.py +++ b/invokeai/app/api/auth_dependencies.py @@ -7,6 +7,7 @@ from invokeai.app.api.dependencies import ApiDependencies from invokeai.app.services.auth.token_service import TokenData, verify_token +from invokeai.app.services.users.users_common import SYSTEM_USER_ID from invokeai.backend.util.logging import logging if TYPE_CHECKING: @@ -33,8 +34,9 @@ def resolve_authorized_user(token_data: TokenData) -> "UserDTO | None": site, and a check added to some copies but not others is indistinguishable from no check at all on the paths that were missed. - A token is honored when all three hold: + A token is honored when all four hold: + - it does not claim the internal ``system`` account, - the account still exists, - it is active, - and the token carries the account's current revocation epoch. Any mismatch counts @@ -44,6 +46,22 @@ def resolve_authorized_user(token_data: TokenData) -> "UserDTO | None": Raises whatever the user service raises; callers that must fail closed should catch. """ + # `system` owns everything carried over from before multiuser support, and is not a + # login account: `UserService.authenticate` refuses it and the migration clears any + # password left on the row. Neither of those reaches a token that was *already issued* + # — on an instance that set a password through the old `PATCH /auth/users/system` hole + # and logged in before it was closed, the JWT survives the upgrade, and nothing else + # here would reject it: the row is deliberately kept active and its epoch still + # matches, so the sliding-window middleware would renew it indefinitely. Refusing the + # id is what actually ends those sessions, and it holds for databases that applied an + # earlier revision of the migration too. + # + # Single-user mode, where everything legitimately runs as `system`, never reaches here: + # its dependencies synthesize the TokenData and return before resolving anything (see + # `get_current_user_or_default`, `get_current_media_user_or_default`, and + # `_identify_video_upload_user`). So this refuses only real, minted tokens. + if token_data.user_id == SYSTEM_USER_ID: + return None user = ApiDependencies.invoker.services.users.get(token_data.user_id) if user is None or not user.is_active: return None diff --git a/invokeai/app/services/shared/sqlite_migrator/migrations/migration_2026_08_08_demote_system_user.py b/invokeai/app/services/shared/sqlite_migrator/migrations/migration_2026_08_08_demote_system_user.py index 4a10313c731..d288cc9314d 100644 --- a/invokeai/app/services/shared/sqlite_migrator/migrations/migration_2026_08_08_demote_system_user.py +++ b/invokeai/app/services/shared/sqlite_migrator/migrations/migration_2026_08_08_demote_system_user.py @@ -1,21 +1,36 @@ -"""Return the ``system`` account to non-administrator, in case it was ever promoted. +"""Return the ``system`` account to its intended shape: not an administrator, and unable +to authenticate. The system row (created by migration_27) owns everything carried over from before -multiuser support. It is deliberately not an administrator and has an empty password -hash, so it can never authenticate — but until the service-level guard landed, an -administrator could ``PATCH /auth/users/system {"is_admin": true}``. +multiuser support. It is seeded as a non-administrator with an empty password hash, so +it can never authenticate — but until the service-level guard landed, an administrator +could ``PATCH /auth/users/system`` and both promote it and give it a password. -That is worth undoing rather than merely preventing, because the last-administrator +Promotion is worth undoing rather than merely preventing, because the last-administrator guard counts rows where ``is_admin AND is_active``, and this row satisfies both while being unable to log in. A promoted system row therefore inflates the count by one permanently: it makes the guard willing to demote the last *real* administrator, and it keeps ``has_admin()`` true afterwards, so ``/auth/setup`` stays closed and no authenticated path back exists. -Demoting it restores the count's meaning — "administrators who can actually log in" — -and, on an instance already in that state, lets ``/auth/setup`` reopen so the operator -can recover. Nobody is logged out: no token can carry ``user_id="system"`` in the first -place, since authenticating as it is impossible. +A password is worse. The system row's email is fixed and public (``system@system.invokeai``), +so a hash left behind by that hole is a standing login for the account that owns every +pre-multiuser board, image, workflow, and queue item — and its tokens are indistinguishable +from any other user's. Clearing the hash restores the seeded state: ``verify_password`` +against ``""`` fails for every input. + +Neither change logs anybody out, because no token can carry ``user_id="system"`` unless the +instance was in exactly this damaged state, and such a token is meant to stop working. + +This migration cannot be the only defense, in two directions. A row damaged *after* it runs +— by direct SQL, or by a database that applied an earlier revision of this same migration id +— would slip through, since migrations run once; :meth:`UserService.authenticate` refuses +the system account outright for that reason. And a token *already issued* is not reached by +clearing the credential at all: the row is deliberately left active and its epoch untouched, +so nothing else would reject it and sliding-window refresh would renew it forever; +``resolve_authorized_user`` refuses the id for that reason. Between them the invariant holds +regardless of what the row contains or what is already in the wild; this migration's job is +to remove the standing credential itself. """ import sqlite3 @@ -25,11 +40,14 @@ class DemoteSystemUserCallback: def __call__(self, cursor: sqlite3.Cursor) -> None: - cursor.execute("UPDATE users SET is_admin = FALSE WHERE user_id = 'system' AND is_admin = TRUE;") + cursor.execute( + "UPDATE users SET is_admin = FALSE, password_hash = '' " + "WHERE user_id = 'system' AND (is_admin = TRUE OR password_hash != '');" + ) def build_migration() -> Migration: - """Clear ``is_admin`` on the ``system`` row. + """Clear ``is_admin`` and any password hash on the ``system`` row. Depends on migration_27, which creates the users table and seeds that row. """ diff --git a/invokeai/app/services/users/users_common.py b/invokeai/app/services/users/users_common.py index 723e2661478..706597e52cf 100644 --- a/invokeai/app/services/users/users_common.py +++ b/invokeai/app/services/users/users_common.py @@ -129,7 +129,9 @@ class LastAdministratorError(ValueError): # Owner of everything that predates multiuser support (created by migration_27). Not a -# login account: it has an empty password hash and is hidden from the user list. +# login account: it is seeded with an empty password hash, `UserService.authenticate` +# refuses it outright whatever the row holds, `resolve_authorized_user` refuses any token +# already carrying it, and it is hidden from the user list. SYSTEM_USER_ID = "system" SYSTEM_USER_PROTECTED_DETAIL = ( "The system user cannot be deleted, deactivated, promoted to administrator, or given a password" diff --git a/invokeai/app/services/users/users_default.py b/invokeai/app/services/users/users_default.py index 5a481dd837e..e722d12a1df 100644 --- a/invokeai/app/services/users/users_default.py +++ b/invokeai/app/services/users/users_default.py @@ -257,6 +257,16 @@ def authenticate(self, email: str, password: str) -> UserDTO | None: if row is None: return None + # The system account is not a login account. It owns everything migrated from + # before multiuser support, so a token bearing `user_id="system"` reads and writes + # all of it — and `_assert_system_user_protected` only stops a password being set + # from *now on*. An instance that set one through the old hole still carries a + # usable hash, and the migration that clears it cannot reach a row damaged by + # direct SQL afterwards. Refusing here makes "system cannot authenticate" hold + # regardless of what the row contains. + if row[0] == SYSTEM_USER_ID: + return None + password_hash = row[3] if not verify_password(password, password_hash): return None diff --git a/tests/app/routers/test_privilege_revocation.py b/tests/app/routers/test_privilege_revocation.py index 451e3565dad..af4b5fe6dd6 100644 --- a/tests/app/routers/test_privilege_revocation.py +++ b/tests/app/routers/test_privilege_revocation.py @@ -8,6 +8,7 @@ """ import logging +from datetime import timedelta from typing import Any from unittest.mock import MagicMock @@ -17,7 +18,7 @@ from invokeai.app.api.dependencies import ApiDependencies from invokeai.app.api_app import app -from invokeai.app.services.auth.token_service import verify_token +from invokeai.app.services.auth.token_service import TokenData, create_access_token, verify_token from invokeai.app.services.config.config_default import InvokeAIAppConfig from invokeai.app.services.events.events_common import UserAccessChangedEvent from invokeai.app.services.invocation_services import InvocationServices @@ -356,6 +357,43 @@ def test_system_user_survives_both_attempts( assert system is not None assert system.is_active is True + def test_a_token_claiming_the_system_account_is_refused( + self, client: TestClient, mock_invoker: Invoker, admin_token: str + ) -> None: + """Closing the login hole does not reach a token already in the wild. + + An instance that set a password on the system row through the old + `PATCH /auth/users/system` and logged in holds a JWT that survives the upgrade: + the row is deliberately kept active and its epoch still matches, so nothing else + rejects it, and the sliding-window middleware would renew it indefinitely. It + grants read/write over every board, image, workflow, and queue item carried over + from before multiuser support, so `resolve_authorized_user` refuses the id itself. + """ + _save_image(mock_invoker, "system-owned-img", "system") + system_token = create_access_token( + TokenData(user_id="system", email="system@system.invokeai", is_admin=False), + timedelta(days=1), + ) + + assert client.get("/api/v1/auth/me", headers=_auth(system_token)).status_code == (status.HTTP_401_UNAUTHORIZED) + r = client.get("/api/v1/images/i/system-owned-img", headers=_auth(system_token)) + assert r.status_code == status.HTTP_401_UNAUTHORIZED + assert "X-Refreshed-Token" not in r.headers + + def test_an_admin_token_claiming_the_system_account_is_refused(self, client: TestClient, admin_token: str) -> None: + """The refusal is on the id, so forging the admin claim does not help either.""" + system_token = create_access_token( + TokenData(user_id="system", email="system@system.invokeai", is_admin=True), + timedelta(days=1), + ) + + r = client.post( + "/api/v1/auth/users", + json={"email": "new@test.com", "display_name": "New", "password": "TestPass123", "is_admin": True}, + headers=_auth(system_token), + ) + assert r.status_code == status.HTTP_401_UNAUTHORIZED + def test_ordinary_user_deletion_still_works( self, client: TestClient, mock_invoker: Invoker, admin_token: str ) -> None: diff --git a/tests/app/services/shared/sqlite_migrator/migrations/test_migration_2026_08_08_demote_system_user.py b/tests/app/services/shared/sqlite_migrator/migrations/test_migration_2026_08_08_demote_system_user.py new file mode 100644 index 00000000000..c644265393b --- /dev/null +++ b/tests/app/services/shared/sqlite_migrator/migrations/test_migration_2026_08_08_demote_system_user.py @@ -0,0 +1,131 @@ +"""Tests for migration 2026_08_08_demote_system_user. + +The `system` row owns everything carried over from before multiuser support. It is seeded +as a non-administrator with an empty password hash, but until the service-level guard +landed an administrator could `PATCH /auth/users/system` and both promote it and give it a +password. Preventing that from now on does not repair an instance already in that state: + +- a promoted row inflates `count_admins()` with an administrator nobody can log in as, + which is enough to walk the last-admin guard past the real one; +- a password on the row is a standing login for the owner of all pre-multiuser content, + under a fixed, public email address. +""" + +import sqlite3 + +import pytest + +from invokeai.app.services.shared.sqlite_migrator.migrations.migration_2026_08_08_demote_system_user import ( + DemoteSystemUserCallback, + build_migration, +) + + +def _create_users_table(conn: sqlite3.Connection) -> None: + conn.execute( + """ + CREATE TABLE users ( + user_id TEXT NOT NULL PRIMARY KEY, + email TEXT NOT NULL UNIQUE, + display_name TEXT, + password_hash TEXT NOT NULL, + is_admin BOOLEAN NOT NULL DEFAULT FALSE, + is_active BOOLEAN NOT NULL DEFAULT TRUE + ); + """ + ) + + +def _insert(conn: sqlite3.Connection, user_id: str, email: str, password_hash: str, is_admin: bool) -> None: + conn.execute( + "INSERT INTO users (user_id, email, display_name, password_hash, is_admin) VALUES (?, ?, ?, ?, ?);", + (user_id, email, user_id, password_hash, is_admin), + ) + + +def _row(conn: sqlite3.Connection, user_id: str) -> tuple: + return conn.execute( + "SELECT is_admin, password_hash, is_active FROM users WHERE user_id = ?;", (user_id,) + ).fetchone() + + +# A hash shaped like the real thing; the migration must not care what it contains. +BCRYPT_HASH = "$2b$12$abcdefghijklmnopqrstuv0123456789012345678901234567890a" + + +@pytest.fixture +def db() -> sqlite3.Connection: + conn = sqlite3.connect(":memory:") + _create_users_table(conn) + return conn + + +class TestDemoteSystemUser: + def test_demotes_a_promoted_system_row(self, db: sqlite3.Connection) -> None: + _insert(db, "system", "system@system.invokeai", "", is_admin=True) + + DemoteSystemUserCallback()(db.cursor()) + db.commit() + + assert _row(db, "system")[0] == 0 + + def test_clears_a_password_left_on_the_system_row(self, db: sqlite3.Connection) -> None: + """Without this, the row remains a login for the owner of all pre-multiuser content.""" + _insert(db, "system", "system@system.invokeai", BCRYPT_HASH, is_admin=False) + + DemoteSystemUserCallback()(db.cursor()) + db.commit() + + assert _row(db, "system")[1] == "" + + def test_repairs_a_row_that_is_both_promoted_and_password_bearing(self, db: sqlite3.Connection) -> None: + _insert(db, "system", "system@system.invokeai", BCRYPT_HASH, is_admin=True) + + DemoteSystemUserCallback()(db.cursor()) + db.commit() + + is_admin, password_hash, is_active = _row(db, "system") + assert (is_admin, password_hash) == (0, "") + # Deactivating it would strand the content it owns — that is not this migration's job. + assert is_active == 1 + + def test_leaves_an_undamaged_system_row_alone(self, db: sqlite3.Connection) -> None: + _insert(db, "system", "system@system.invokeai", "", is_admin=False) + + DemoteSystemUserCallback()(db.cursor()) + db.commit() + + assert _row(db, "system") == (0, "", 1) + + def test_leaves_real_administrators_alone(self, db: sqlite3.Connection) -> None: + """Only the `system` id is repaired; demoting a real admin would lock the instance out.""" + _insert(db, "system", "system@system.invokeai", BCRYPT_HASH, is_admin=True) + _insert(db, "u1", "admin@example.com", BCRYPT_HASH, is_admin=True) + + DemoteSystemUserCallback()(db.cursor()) + db.commit() + + assert _row(db, "u1") == (1, BCRYPT_HASH, 1) + + def test_is_idempotent(self, db: sqlite3.Connection) -> None: + _insert(db, "system", "system@system.invokeai", BCRYPT_HASH, is_admin=True) + + DemoteSystemUserCallback()(db.cursor()) + DemoteSystemUserCallback()(db.cursor()) + db.commit() + + assert _row(db, "system") == (0, "", 1) + + def test_no_system_row_is_not_an_error(self, db: sqlite3.Connection) -> None: + """Databases created after migration_27 always have one, but the callback must not + assume it — a missing row is nothing to repair.""" + DemoteSystemUserCallback()(db.cursor()) + db.commit() + + assert db.execute("SELECT COUNT(*) FROM users;").fetchone()[0] == 0 + + def test_migration_metadata(self) -> None: + migration = build_migration() + + assert migration.id == "2026_08_08_demote_system_user" + assert migration.depends_on == "migration_27" diff --git a/tests/app/services/users/test_last_admin_invariant.py b/tests/app/services/users/test_last_admin_invariant.py index 861389b7b4b..050e1d01dcd 100644 --- a/tests/app/services/users/test_last_admin_invariant.py +++ b/tests/app/services/users/test_last_admin_invariant.py @@ -22,6 +22,7 @@ import pytest +from invokeai.app.services.auth.password_utils import hash_password from invokeai.app.services.shared.sqlite.sqlite_database import SqliteDatabase from invokeai.app.services.users.users_common import ( SYSTEM_USER_ID, @@ -314,6 +315,33 @@ def test_renaming_the_system_user_is_allowed(db: SqliteDatabase, users: UserServ assert updated.display_name == "Renamed" +def test_a_system_row_carrying_a_password_still_cannot_log_in(db: SqliteDatabase, users: UserService) -> None: + """The guard above only stops a password being set *from now on*. + + An instance that set one through the old `PATCH /auth/users/system` hole still carries + a usable hash, and its email is fixed and public — so the hash is a standing login for + the account that owns every pre-multiuser board, image, workflow, and queue item. The + migration clears it, but migrations run once and cannot reach a row damaged afterwards + by direct SQL, so `authenticate` refuses the account outright whatever the row holds. + """ + _seed_system_user(db) + db._conn.execute( + "UPDATE users SET password_hash = ? WHERE user_id = 'system'", + (hash_password(PASSWORD),), + ) + db._conn.commit() + + assert users.authenticate("system@system.invokeai", PASSWORD) is None + + +def test_refusing_the_system_account_does_not_block_other_logins(db: SqliteDatabase, users: UserService) -> None: + """The refusal is keyed on the user id, not on anything a real account shares.""" + _seed_system_user(db) + _make(users, "real@test.com", is_admin=False) + + assert users.authenticate("real@test.com", PASSWORD) is not None + + def test_the_system_error_is_a_value_error(db: SqliteDatabase, users: UserService) -> None: """Same reason as the last-admin error: existing route and CLI handlers catch ValueError.""" _seed_system_user(db) From b74d34e56706a9366f812db0f680215cf2ae4e56 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sun, 9 Aug 2026 17:03:20 -0400 Subject: [PATCH 16/19] fix(session-processor): fail closed when the queue item's owner cannot be read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) --- .../session_processor_default.py | 67 ++++++++++----- .../test_privilege_revocation.py | 85 ++++++++++++++++++- 2 files changed, 126 insertions(+), 26 deletions(-) diff --git a/invokeai/app/services/session_processor/session_processor_default.py b/invokeai/app/services/session_processor/session_processor_default.py index e074e0b12d5..1d9eeeef0d7 100644 --- a/invokeai/app/services/session_processor/session_processor_default.py +++ b/invokeai/app/services/session_processor/session_processor_default.py @@ -1,4 +1,5 @@ import gc +import time import traceback from contextlib import contextmanager, suppress from threading import BoundedSemaphore, Thread @@ -43,6 +44,13 @@ from invokeai.backend.util.device_pool import GENERATION_DEVICE_POOL from invokeai.backend.util.devices import TorchDevice +# A failed owner lookup is retried before the item is refused, so that a transient error +# — a busy-timeout on the shared SQLite connection under multi-GPU write contention, say — +# does not cost the user their queued work. Both call sites run on a worker thread, so the +# wait between attempts blocks nothing else. +OWNER_LOOKUP_ATTEMPTS = 3 +OWNER_LOOKUP_RETRY_SECONDS = 0.25 + def queue_owner_is_active(services: InvocationServices, queue_item: SessionQueueItem) -> bool: """Whether the queue item's owner is still permitted to execute work. @@ -63,23 +71,36 @@ def queue_owner_is_active(services: InvocationServices, queue_item: SessionQueue have no such exemption: the item would burn GPU time and then fail at the first `context.images.save()`. Better to reject it at dequeue. - A failed lookup is treated as active. This runs between nodes on a path with no - exception handling of its own, so letting a transient error (e.g. a busy-timeout - on the shared SQLite connection under multi-GPU write contention) escape would - abandon the session without its normal teardown. Denying execution on a failed - read would also revoke privileges the database never actually revoked; the next - node boundary re-checks, and the dequeue gate catches the item on its next run. + A lookup that keeps failing is treated as *not* authorized, after + ``OWNER_LOOKUP_ATTEMPTS`` tries. Returning "active" on an unreadable database would + make unknown state executable: the account may well have been deactivated a moment + ago, and this gate is what stands between that and GPU time spent on its behalf. + Failing closed costs a still-valid user a cancellation instead — recoverable, since + canceled items can be retried, and only reachable when the database has been + unreadable across every attempt, by which point the instance has larger problems. + The exception is swallowed rather than raised for the same reason as before: this + runs between nodes on a path with no exception handling of its own, and letting it + escape would abandon the session without its normal teardown. """ if not services.configuration.multiuser: return True - try: - user = services.users.get(queue_item.user_id) - except Exception: - services.logger.warning( - f"Could not verify owner {queue_item.user_id} of queue item {queue_item.item_id}; allowing execution" - ) - return True - return user is not None and user.is_active + for attempt in range(OWNER_LOOKUP_ATTEMPTS): + try: + user = services.users.get(queue_item.user_id) + except Exception: + services.logger.warning( + f"Could not verify owner {queue_item.user_id} of queue item {queue_item.item_id} " + f"(attempt {attempt + 1}/{OWNER_LOOKUP_ATTEMPTS})", + exc_info=True, + ) + if attempt + 1 < OWNER_LOOKUP_ATTEMPTS: + time.sleep(OWNER_LOOKUP_RETRY_SECONDS) + continue + return user is not None and user.is_active + services.logger.error( + f"Could not verify owner {queue_item.user_id} of queue item {queue_item.item_id}; refusing execution" + ) + return False class DefaultSessionRunner(SessionRunnerBase): @@ -147,7 +168,8 @@ def _run_session_loop(self, queue_item: SessionQueueItem) -> None: # why execution stopped. if not queue_owner_is_active(self._services, queue_item): self._services.logger.warning( - f"Canceling queue item {queue_item.item_id}: owner {queue_item.user_id} is deactivated or deleted" + f"Canceling queue item {queue_item.item_id}: owner {queue_item.user_id} is deactivated, " + "deleted, or could not be verified" ) with suppress(SessionQueueItemNotFoundError): self._services.session_queue.cancel_queue_item(queue_item.item_id) @@ -656,11 +678,11 @@ async def _on_user_access_changed(self, event: FastAPIEvent[UserAccessChangedEve # a running item of an account the database says is active, with nothing to undo # it. # - # Unlike the dequeue and between-node gates, a failed read here does NOT fail to - # "active". Those gates re-run at the next node; this handler is the only thing - # that stops a *single-node* graph, which is checked once before it starts and - # never again. The event is itself evidence of a committed deactivation, so when - # the re-read cannot contradict it, the event stands. + # A failed read fails closed here too, and for a stronger reason than at the + # dequeue and between-node gates: this handler is the only thing that stops a + # *single-node* graph, which is checked once before it starts and never again. + # The event is itself evidence of a committed deactivation, so when the re-read + # cannot contradict it, the event stands. def _cancel_all() -> None: for item in queue_items: if not self._invoker.services.configuration.multiuser: @@ -731,14 +753,15 @@ def _is_image_move_maintenance_active(self) -> bool: return image_moves is not None and image_moves.is_maintenance_active() def _cancel_queue_item_if_owner_inactive(self, queue_item: SessionQueueItem) -> bool: - """Cancel a dequeued item whose owner is deactivated or deleted. + """Cancel a dequeued item whose owner is deactivated, deleted, or unverifiable. Returns True if the item was rejected (canceled) and must not be executed. """ if queue_owner_is_active(self._invoker.services, queue_item): return False self._invoker.services.logger.warning( - f"Canceling queue item {queue_item.item_id}: owner {queue_item.user_id} is deactivated or deleted" + f"Canceling queue item {queue_item.item_id}: owner {queue_item.user_id} is deactivated, " + "deleted, or could not be verified" ) with suppress(SessionQueueItemNotFoundError): self._invoker.services.session_queue.cancel_queue_item(queue_item.item_id) diff --git a/tests/app/services/session_processor/test_privilege_revocation.py b/tests/app/services/session_processor/test_privilege_revocation.py index 9de2b79a91e..7f4fa0ef047 100644 --- a/tests/app/services/session_processor/test_privilege_revocation.py +++ b/tests/app/services/session_processor/test_privilege_revocation.py @@ -15,6 +15,7 @@ import pytest +from invokeai.app.services.session_processor import session_processor_default from invokeai.app.services.session_processor.session_processor_default import ( DefaultSessionProcessor, DefaultSessionRunner, @@ -76,6 +77,42 @@ def test_deleted_user(self) -> None: services = _services(users_by_id={}) assert queue_owner_is_active(services, _queue_item()) is False + def test_an_unreadable_owner_is_refused(self, monkeypatch: pytest.MonkeyPatch) -> None: + """Failing open here would make unknown database state executable. + + The account may have been deactivated a moment ago; nothing else stands between + that and GPU time spent on its behalf. Failing closed costs a still-valid user a + cancellation, which is retryable. + """ + monkeypatch.setattr(session_processor_default, "OWNER_LOOKUP_RETRY_SECONDS", 0) + services = _services() + attempts = [] + + def explode(user_id: str) -> None: + attempts.append(user_id) + raise RuntimeError("database is locked") + + services.users.get = explode + + assert queue_owner_is_active(services, _queue_item()) is False + assert len(attempts) == session_processor_default.OWNER_LOOKUP_ATTEMPTS + + def test_a_transient_read_failure_is_retried_not_refused(self, monkeypatch: pytest.MonkeyPatch) -> None: + """A busy-timeout under write contention must not cost the user their queued work.""" + monkeypatch.setattr(session_processor_default, "OWNER_LOOKUP_RETRY_SECONDS", 0) + services = _services() + answers = iter([RuntimeError("database is locked"), _active("user-1")]) + + def flaky(user_id: str): + answer = next(answers) + if isinstance(answer, Exception): + raise answer + return answer + + services.users.get = flaky + + assert queue_owner_is_active(services, _queue_item()) is True + class TestDequeueRejection: """Items whose owner was deactivated while pending are canceled at dequeue and @@ -123,6 +160,20 @@ def test_missing_queue_item_does_not_raise(self) -> None: assert processor._cancel_queue_item_if_owner_inactive(_queue_item()) is True + def test_unreadable_owner_item_is_canceled(self, monkeypatch: pytest.MonkeyPatch) -> None: + """An owner the database cannot answer for does not get to run.""" + monkeypatch.setattr(session_processor_default, "OWNER_LOOKUP_RETRY_SECONDS", 0) + services = _services() + + def explode(user_id: str) -> None: + raise RuntimeError("database is locked") + + services.users.get = explode + processor = self._processor(services) + + assert processor._cancel_queue_item_if_owner_inactive(_queue_item()) is True + services.session_queue.cancel_queue_item.assert_called_once_with(7) + class TestUserAccessChangedCancelsCurrentItem: """Deactivating a user cancels their currently running queue item immediately.""" @@ -220,10 +271,11 @@ async def test_single_user_mode_does_not_cancel(self) -> None: @pytest.mark.anyio async def test_a_failed_re_read_still_cancels(self) -> None: - """The dequeue and between-node gates fail to "active" on a read error because they - re-run at the next node. This handler has no next node to fall back on — a - single-node graph is checked once, before it starts — so a read that cannot - contradict the event must not override it either.""" + """A read that cannot contradict the event must not override it. + + The gates fail closed on an unreadable database too, but this handler has the + stronger claim: it is the only thing that stops a single-node graph, which is + checked once before it starts and never again.""" services = _services() def explode(user_id: str) -> None: @@ -275,6 +327,31 @@ def test_deactivation_after_first_node_stops_later_nodes(self) -> None: assert executed == ["n1"] services.session_queue.cancel_queue_item.assert_called_once_with(21) + def test_an_unreadable_owner_stops_later_nodes(self, monkeypatch: pytest.MonkeyPatch) -> None: + """A deactivation the database can no longer be asked about still stops the run.""" + monkeypatch.setattr(session_processor_default, "OWNER_LOOKUP_RETRY_SECONDS", 0) + node1, node2 = SimpleNamespace(id="n1"), SimpleNamespace(id="n2") + # Active for the first check; afterwards the record cannot be read at all. + answered = [] + + def flaky(user_id: str): + answered.append(user_id) + if len(answered) == 1: + return _active("user-1") + raise RuntimeError("database is locked") + + services = _services() + services.users = SimpleNamespace(get=flaky) + runner = self._runner_with_services(services) + executed = [] + runner.run_node = lambda invocation, queue_item: executed.append(invocation.id) # type: ignore[method-assign] + queue_item = self._multi_node_queue_item([node1, node2]) + + runner._run_session_loop(queue_item) + + assert executed == ["n1"] + services.session_queue.cancel_queue_item.assert_called_once_with(21) + def test_active_owner_runs_all_nodes(self) -> None: node1, node2 = SimpleNamespace(id="n1"), SimpleNamespace(id="n2") services = _services(users_by_id={"user-1": _active("user-1")}) From 68525ecdc7b2ca9aaecc572617dcfce597154ec2 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sun, 9 Aug 2026 17:03:31 -0400 Subject: [PATCH 17/19] fix(sockets): revalidate open sockets against the database MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) --- invokeai/app/api/sockets.py | 149 +++++++++- invokeai/app/api_app.py | 17 +- invokeai/app/util/user_management.py | 17 ++ tests/app/test_socket_privilege_revocation.py | 271 ++++++++++++++++++ 4 files changed, 447 insertions(+), 7 deletions(-) diff --git a/invokeai/app/api/sockets.py b/invokeai/app/api/sockets.py index 6cba8118917..4b94a09b2fd 100644 --- a/invokeai/app/api/sockets.py +++ b/invokeai/app/api/sockets.py @@ -1,11 +1,13 @@ # Copyright (c) 2022 Kyle Schouviller (https://github.com/kyle0654) +import asyncio from collections.abc import Collection from typing import Any from fastapi import FastAPI from pydantic import BaseModel from socketio import ASGIApp, AsyncServer +from starlette.concurrency import run_in_threadpool from invokeai.app.services.auth.token_service import verify_token from invokeai.app.services.config.config_default import get_config @@ -57,6 +59,14 @@ logger = InvokeAILogger.get_logger() +# How often open sockets are re-derived from the database. Route handlers emit +# `user_access_changed` the moment they mutate a user, so in-process changes are already +# immediate; this sweep exists for mutations this process never sees — the `invoke-usermod` +# / `invoke-userdel` CLIs run against the database file from a *separate* process, so no +# in-process event can be raised for them. It bounds how long a socket can outlive the +# authorization it was granted under. See `SocketIO._revalidate_socket_users`. +SOCKET_REVALIDATION_INTERVAL_SECONDS = 30.0 + class QueueSubscriptionEvent(BaseModel): """Event data for subscribing to the socket.io queue room. @@ -130,6 +140,9 @@ def __init__(self, app: FastAPI): # Track user information for each socket connection self._socket_users: dict[str, dict[str, Any]] = {} + # Periodic re-derivation of that cached state from the database; see `start`. + self._revalidation_task: asyncio.Task[None] | None = None + # Set up authentication middleware self._sio.on("connect", handler=self._handle_connect) self._sio.on("disconnect", handler=self._handle_disconnect) @@ -254,6 +267,108 @@ async def _handle_disconnect(self, sid: str) -> None: del self._socket_users[sid] logger.debug(f"Socket {sid} disconnected and cleaned up") + def start(self) -> None: + """Start background work. Called from the app's startup hook. + + Not started in `__init__`: this object is constructed at import time, where there + is no running event loop to attach a task to. + """ + if self._revalidation_task is None or self._revalidation_task.done(): + self._revalidation_task = asyncio.create_task(self._revalidation_loop()) + + def stop(self) -> None: + """Cancel background work. Called from the app's shutdown hook.""" + if self._revalidation_task is not None: + self._revalidation_task.cancel() + self._revalidation_task = None + + async def _revalidation_loop(self) -> None: + """Re-derive every open socket's authorization from the database, periodically.""" + while True: + await asyncio.sleep(SOCKET_REVALIDATION_INTERVAL_SECONDS) + if not self._socket_users: + continue + try: + await self._revalidate_socket_users() + except Exception: + logger.exception("Error revalidating socket authorization") + + async def _revalidate_socket_users(self) -> None: + """Emit `user_access_changed` for any connected user whose cached state is stale. + + The room membership and `is_admin` flag a socket carries are established at connect + time and refreshed by `_handle_user_access_changed`, which fires on an in-process + event. Nothing in this process emits that event for a change made by another + process — `invoke-usermod --no-admin` and `invoke-userdel` open the database + directly — so without this sweep a demoted administrator's socket would sit in the + admin room until it happened to reconnect. REST is unaffected either way: every + request re-reads the record. + + Rather than adjusting rooms here, differences are published as the same event the + route handlers emit. That keeps one implementation of "what a change to this user + means" — sockets re-authorize *and* the session processor cancels the user's running + items — instead of a second copy that drifts from the first. + + A lookup that fails leaves the socket alone and is retried on the next sweep. That + is the opposite of the queue gate's fail-closed policy, deliberately: nothing runs + on the user's behalf because a socket stays open for another interval, whereas + tearing down every live session on a transient database error would be an outage + this sweep caused by itself — and `_handle_connect` fails closed, so the clients + would not get back in. + + Scope: only users with an open socket are swept, which leaves one gap. An + out-of-process deletion of a user with no socket does not reach + `DefaultSessionProcessor._on_user_access_changed`, so a *single-node* graph of + theirs that is already running — the one case no other gate re-checks — runs to + completion. It cannot persist anything: the save gates in `invocation_context` + re-read the record and raise `PermissionError`. The cost is the wasted node. + """ + if not self._is_multiuser_enabled(): + return + + from invokeai.app.api.dependencies import ApiDependencies + + services = ApiDependencies.invoker.services + + # One lookup per distinct user, not per socket: a user with several tabs open is + # the common case, and every socket of a user gets the same answer. + cached_by_user: dict[str, list[dict[str, Any]]] = {} + for info in list(self._socket_users.values()): + cached_by_user.setdefault(info["user_id"], []).append(info) + + for user_id, cached_infos in cached_by_user.items(): + try: + user = await run_in_threadpool(services.users.get, user_id) + except Exception: + logger.warning(f"Could not revalidate socket user {user_id}; will retry", exc_info=True) + continue + + if user is None: + # Deleted. `_handle_user_access_changed` only reads `is_active` on this + # path, so the other fields just need to be inert. + is_admin, is_active, token_epoch = False, False, 0 + else: + is_admin, is_active, token_epoch = user.is_admin, user.is_active, user.token_epoch + + # Staleness is judged against *every* socket of the user, not a representative + # one. They need not agree: a session that reconnected after a password change + # holds the current epoch while the superseded session is still connected under + # the old one, and sampling only the first would find nothing to do and leave + # the revoked socket in place. + if is_active and all( + is_admin == cached.get("is_admin") and token_epoch == cached.get("token_epoch", 0) + for cached in cached_infos + ): + continue + + logger.info(f"Revalidation found stale socket authorization for user {user_id}; re-authorizing") + services.events.emit_user_access_changed( + user_id=user_id, + is_admin=is_admin, + is_active=is_active, + token_epoch=token_epoch, + ) + async def _handle_user_access_changed(self, event: FastAPIEvent[UserAccessChangedEvent]) -> None: """Re-authorize a user's open sockets when their role or active status changes. @@ -274,8 +389,34 @@ async def _handle_user_access_changed(self, event: FastAPIEvent[UserAccessChange - Demoted: leave the admin room and update the cached ``is_admin`` so ``_handle_sub_queue`` cannot re-add it. - Promoted: join the admin room, matching the DB-derived REST behavior. + + The event is a trigger; the record is re-read here and *that* is what is applied, + the same way ``DefaultSessionProcessor._on_user_access_changed`` does. Handlers are + dispatched as independent tasks, and the revalidation sweep's payload is a snapshot + taken before an await — so an event can arrive already superseded. Acting on it + would then re-grant the admin room to someone just demoted, or disconnect the + replacement session a password change had just issued, and nothing would correct + either until the next sweep. A read that fails leaves the event standing: it is + evidence of a committed change that the re-read could not contradict. """ _, event_data = event + is_admin, is_active, token_epoch = event_data.is_admin, event_data.is_active, event_data.token_epoch + try: + from invokeai.app.api.dependencies import ApiDependencies + + user = await run_in_threadpool(ApiDependencies.invoker.services.users.get, event_data.user_id) + except Exception: + logger.warning( + f"Could not re-read user {event_data.user_id} while re-authorizing its sockets; " + "honoring the access-changed event", + exc_info=True, + ) + else: + if user is None: + is_admin, is_active, token_epoch = False, False, 0 + else: + is_admin, is_active, token_epoch = user.is_admin, user.is_active, user.token_epoch + affected_sids = [sid for sid, info in self._socket_users.items() if info.get("user_id") == event_data.user_id] for sid in affected_sids: # `affected_sids` is a snapshot, and `disconnect()` below yields to the event @@ -289,16 +430,16 @@ async def _handle_user_access_changed(self, event: FastAPIEvent[UserAccessChange info = self._socket_users.get(sid) if info is None: continue - if not event_data.is_active: + if not is_active: logger.info(f"Disconnecting socket {sid}: user {event_data.user_id} deactivated or deleted") await self._sio.disconnect(sid) continue - if info.get("token_epoch", 0) != event_data.token_epoch: + if info.get("token_epoch", 0) != token_epoch: logger.info(f"Disconnecting socket {sid}: user {event_data.user_id} revoked its earlier sessions") await self._sio.disconnect(sid) continue - info["is_admin"] = event_data.is_admin - if event_data.is_admin: + info["is_admin"] = is_admin + if is_admin: await self._sio.enter_room(sid, "admin") logger.info(f"Socket {sid} joined admin room: user {event_data.user_id} promoted") else: diff --git a/invokeai/app/api_app.py b/invokeai/app/api_app.py index a79aed667fb..8c6d9b34861 100644 --- a/invokeai/app/api_app.py +++ b/invokeai/app/api_app.py @@ -75,9 +75,20 @@ async def lifespan(app: FastAPI): ) logger.handle(record) - yield - # Shut down threads - ApiDependencies.shutdown() + # Re-derive open sockets' authorization from the database on a timer. This is what + # catches user changes made by another process — the `invoke-usermod` / `invoke-userdel` + # CLIs — which cannot raise an in-process event. `socket_io` is created further down + # this module and is bound by the time the app is served. + socket_io.start() + + try: + yield + finally: + # In a `finally` so an exception propagating into the generator cannot leave the + # sweep running against a half-torn-down process, or skip the thread shutdown. + socket_io.stop() + # Shut down threads + ApiDependencies.shutdown() # Create the app diff --git a/invokeai/app/util/user_management.py b/invokeai/app/util/user_management.py index 24b1fe91ab9..9ddbf891a6d 100644 --- a/invokeai/app/util/user_management.py +++ b/invokeai/app/util/user_management.py @@ -21,6 +21,17 @@ "or $HOME/invokeai." ) +# These commands write to the database from their own process, so they cannot raise the +# in-process event that makes a running server re-authorize immediately. What the server +# derives from the database per request — REST authorization, queue-item execution, media +# reads and saves — changes the moment this commits. What it caches does not: an already +# open socket keeps the rooms it joined at connect time until the server's periodic +# revalidation sweep notices (see SOCKET_REVALIDATION_INTERVAL_SECONDS in api/sockets.py). +_LIVE_SERVER_NOTE = ( + " ℹ️ A running server applies this to new requests immediately; already-open\n" + " connections are re-checked within about a minute." +) + # --------------------------------------------------------------------------- # useradd # --------------------------------------------------------------------------- @@ -202,6 +213,7 @@ def _delete_user_interactive() -> bool: user_service.delete(user.user_id) print("\n✅ User deleted successfully!") + print(_LIVE_SERVER_NOTE) return True except ValueError as e: @@ -247,6 +259,7 @@ def _delete_user_cli(email: str, force: bool = False) -> bool: user_service.delete(user.user_id) print("✅ User deleted successfully!") + print(_LIVE_SERVER_NOTE) return True except ValueError as e: @@ -476,6 +489,8 @@ def _modify_user_interactive() -> bool: print(f" Display Name: {updated_user.display_name or '(not set)'}") print(f" Admin: {'Yes' if updated_user.is_admin else 'No'}") print(f" Active: {'Yes' if updated_user.is_active else 'No'}") + if password is not None or is_admin is not None: + print(_LIVE_SERVER_NOTE) return True except ValueError as e: @@ -532,6 +547,8 @@ def _modify_user_cli( print(f" Display Name: {updated_user.display_name or '(not set)'}") print(f" Admin: {'Yes' if updated_user.is_admin else 'No'}") print(f" Active: {'Yes' if updated_user.is_active else 'No'}") + if password is not None or is_admin is not None: + print(_LIVE_SERVER_NOTE) return True except ValueError as e: diff --git a/tests/app/test_socket_privilege_revocation.py b/tests/app/test_socket_privilege_revocation.py index 44e422d3803..09223b8ade1 100644 --- a/tests/app/test_socket_privilege_revocation.py +++ b/tests/app/test_socket_privilege_revocation.py @@ -314,3 +314,274 @@ async def test_password_change_disconnects_superseded_sockets_only(self) -> None disconnected = {call.args[0] for call in socketio._sio.disconnect.await_args_list} assert disconnected == {"sid-old"} + + +class TestRevalidationSweep: + """`_revalidate_socket_users` catches user changes made by another process. + + `invoke-usermod --no-admin` and `invoke-userdel` open the database directly, from + their own process. Nothing in the server process can raise `user_access_changed` for + them, so a socket would otherwise keep the rooms it joined at connect time until it + happened to reconnect. REST is unaffected either way — every request re-reads the + record — which is exactly why the socket cache is the thing that needs a sweep. + """ + + def _socketio(self, socket_users: dict[str, dict]) -> SocketIO: + socketio = SocketIO(FastAPI()) + socketio._socket_users = socket_users + return socketio + + def _patch_services( + self, + monkeypatch: pytest.MonkeyPatch, + users_by_id: dict[str, SimpleNamespace | None], + *, + multiuser: bool = True, + lookups: list[str] | None = None, + ) -> list: + """Bind ApiDependencies to a stub and return the list emitted events land in.""" + emitted: list = [] + + def get(user_id: str): + if lookups is not None: + lookups.append(user_id) + user = users_by_id[user_id] + if isinstance(user, Exception): + raise user + return user + + invoker = SimpleNamespace( + services=SimpleNamespace( + configuration=SimpleNamespace(multiuser=multiuser), + users=SimpleNamespace(get=get), + events=SimpleNamespace(emit_user_access_changed=lambda **kwargs: emitted.append(kwargs)), + ) + ) + monkeypatch.setattr("invokeai.app.api.dependencies.ApiDependencies", SimpleNamespace(invoker=invoker)) + return emitted + + def _user(self, user_id: str, *, is_admin: bool, is_active: bool = True, token_epoch: int = 0) -> SimpleNamespace: + return SimpleNamespace(user_id=user_id, is_admin=is_admin, is_active=is_active, token_epoch=token_epoch) + + @pytest.mark.anyio + async def test_cli_demotion_is_published(self, monkeypatch: pytest.MonkeyPatch) -> None: + socketio = self._socketio({"sid-1": {"user_id": "admin-1", "is_admin": True, "token_epoch": 0}}) + emitted = self._patch_services(monkeypatch, {"admin-1": self._user("admin-1", is_admin=False)}) + + await socketio._revalidate_socket_users() + + assert emitted == [{"user_id": "admin-1", "is_admin": False, "is_active": True, "token_epoch": 0}] + + @pytest.mark.anyio + async def test_cli_deletion_is_published(self, monkeypatch: pytest.MonkeyPatch) -> None: + socketio = self._socketio({"sid-1": {"user_id": "user-1", "is_admin": False, "token_epoch": 0}}) + emitted = self._patch_services(monkeypatch, {"user-1": None}) + + await socketio._revalidate_socket_users() + + assert emitted == [{"user_id": "user-1", "is_admin": False, "is_active": False, "token_epoch": 0}] + + @pytest.mark.anyio + async def test_cli_deactivation_is_published(self, monkeypatch: pytest.MonkeyPatch) -> None: + socketio = self._socketio({"sid-1": {"user_id": "user-1", "is_admin": False, "token_epoch": 0}}) + emitted = self._patch_services(monkeypatch, {"user-1": self._user("user-1", is_admin=False, is_active=False)}) + + await socketio._revalidate_socket_users() + + assert emitted == [{"user_id": "user-1", "is_admin": False, "is_active": False, "token_epoch": 0}] + + @pytest.mark.anyio + async def test_cli_password_reset_is_published(self, monkeypatch: pytest.MonkeyPatch) -> None: + """The account stays active and keeps its role; only the epoch moves.""" + socketio = self._socketio({"sid-1": {"user_id": "user-1", "is_admin": False, "token_epoch": 0}}) + emitted = self._patch_services(monkeypatch, {"user-1": self._user("user-1", is_admin=False, token_epoch=1)}) + + await socketio._revalidate_socket_users() + + assert emitted == [{"user_id": "user-1", "is_admin": False, "is_active": True, "token_epoch": 1}] + + @pytest.mark.anyio + async def test_unchanged_users_emit_nothing(self, monkeypatch: pytest.MonkeyPatch) -> None: + """The sweep runs forever; a no-op sweep must stay a no-op.""" + socketio = self._socketio( + { + "sid-1": {"user_id": "admin-1", "is_admin": True, "token_epoch": 2}, + "sid-2": {"user_id": "user-1", "is_admin": False, "token_epoch": 0}, + } + ) + emitted = self._patch_services( + monkeypatch, + { + "admin-1": self._user("admin-1", is_admin=True, token_epoch=2), + "user-1": self._user("user-1", is_admin=False), + }, + ) + + await socketio._revalidate_socket_users() + + assert emitted == [] + + @pytest.mark.anyio + async def test_a_user_with_several_sockets_is_looked_up_once(self, monkeypatch: pytest.MonkeyPatch) -> None: + """Several open tabs are the common case, and every socket of a user shares one answer.""" + socketio = self._socketio( + { + "sid-1": {"user_id": "admin-1", "is_admin": True, "token_epoch": 0}, + "sid-2": {"user_id": "admin-1", "is_admin": True, "token_epoch": 0}, + "sid-3": {"user_id": "admin-1", "is_admin": True, "token_epoch": 0}, + } + ) + lookups: list[str] = [] + emitted = self._patch_services(monkeypatch, {"admin-1": self._user("admin-1", is_admin=False)}, lookups=lookups) + + await socketio._revalidate_socket_users() + + assert lookups == ["admin-1"] + assert len(emitted) == 1 + + @pytest.mark.anyio + async def test_an_unreadable_record_is_left_for_the_next_sweep(self, monkeypatch: pytest.MonkeyPatch) -> None: + """Opposite of the queue gate's fail-closed policy, deliberately: nothing runs on the + user's behalf because a socket stays open one more interval, whereas tearing down + every live session on a transient database error would be a self-inflicted outage — + and `_handle_connect` fails closed, so the clients would not get back in.""" + socketio = self._socketio({"sid-1": {"user_id": "user-1", "is_admin": True, "token_epoch": 0}}) + emitted = self._patch_services(monkeypatch, {"user-1": RuntimeError("database is locked")}) + + await socketio._revalidate_socket_users() + + assert emitted == [] + + @pytest.mark.anyio + async def test_one_unreadable_record_does_not_abandon_the_others(self, monkeypatch: pytest.MonkeyPatch) -> None: + socketio = self._socketio( + { + "sid-1": {"user_id": "user-1", "is_admin": True, "token_epoch": 0}, + "sid-2": {"user_id": "admin-1", "is_admin": True, "token_epoch": 0}, + } + ) + emitted = self._patch_services( + monkeypatch, + {"user-1": RuntimeError("database is locked"), "admin-1": self._user("admin-1", is_admin=False)}, + ) + + await socketio._revalidate_socket_users() + + assert [event["user_id"] for event in emitted] == ["admin-1"] + + @pytest.mark.anyio + async def test_single_user_mode_is_skipped(self, monkeypatch: pytest.MonkeyPatch) -> None: + """Single-user sockets are cached as the system admin and have no record to check.""" + socketio = self._socketio({"sid-1": {"user_id": "system", "is_admin": True}}) + emitted = self._patch_services(monkeypatch, {"system": None}, multiuser=False) + + await socketio._revalidate_socket_users() + + assert emitted == [] + + @pytest.mark.anyio + async def test_a_stale_socket_is_found_even_beside_an_up_to_date_one(self, monkeypatch: pytest.MonkeyPatch) -> None: + """The user's sockets need not agree. A session that reconnected after a CLI + password reset holds the current epoch while the superseded session is still + connected under the old one; sampling only one of them would find nothing to do.""" + socketio = self._socketio( + { + "sid-new": {"user_id": "user-1", "is_admin": False, "token_epoch": 1}, + "sid-old": {"user_id": "user-1", "is_admin": False, "token_epoch": 0}, + } + ) + emitted = self._patch_services(monkeypatch, {"user-1": self._user("user-1", is_admin=False, token_epoch=1)}) + + await socketio._revalidate_socket_users() + + assert emitted == [{"user_id": "user-1", "is_admin": False, "is_active": True, "token_epoch": 1}] + + +class TestHandlerRereadsAtThePointOfDecision: + """`_handle_user_access_changed` applies the *record*, using the event only as a trigger. + + Handlers are dispatched as independent tasks, and the revalidation sweep's payload is a + snapshot taken before an await, so an event can arrive already superseded. + """ + + def _socketio(self, socket_users: dict[str, dict]) -> SocketIO: + socketio = SocketIO(FastAPI()) + socketio._sio.enter_room = AsyncMock() + socketio._sio.leave_room = AsyncMock() + socketio._sio.disconnect = AsyncMock() + socketio._socket_users = socket_users + return socketio + + def _patch_users(self, monkeypatch: pytest.MonkeyPatch, user) -> None: + invoker = SimpleNamespace( + services=SimpleNamespace( + configuration=SimpleNamespace(multiuser=True), + users=SimpleNamespace(get=lambda user_id: user), + ) + ) + monkeypatch.setattr("invokeai.app.api.dependencies.ApiDependencies", SimpleNamespace(invoker=invoker)) + + @pytest.mark.anyio + async def test_a_superseded_promotion_does_not_regrant_the_admin_room( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """A sweep that read `is_admin=True`, then a demotion committed while it was + suspended. Applying the stale payload would put a demoted user back in the admin + room — receiving every other user's events — until the next sweep.""" + socketio = self._socketio({"sid-1": {"user_id": "user-1", "is_admin": False, "token_epoch": 0}}) + self._patch_users(monkeypatch, SimpleNamespace(user_id="user-1", is_admin=False, is_active=True, token_epoch=0)) + stale = UserAccessChangedEvent.build(user_id="user-1", is_admin=True, is_active=True) + + await socketio._handle_user_access_changed(("user_access_changed", stale)) + + socketio._sio.enter_room.assert_not_awaited() + socketio._sio.leave_room.assert_awaited_once_with("sid-1", "admin") + assert socketio._socket_users["sid-1"]["is_admin"] is False + + @pytest.mark.anyio + async def test_a_superseded_epoch_does_not_disconnect_the_replacement_session( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """The user changed their password and reconnected with the replacement token. A + sweep still carrying the pre-change epoch must not kick that new session.""" + socketio = self._socketio({"sid-new": {"user_id": "user-1", "is_admin": False, "token_epoch": 1}}) + self._patch_users(monkeypatch, SimpleNamespace(user_id="user-1", is_admin=False, is_active=True, token_epoch=1)) + stale = UserAccessChangedEvent.build(user_id="user-1", is_admin=False, is_active=True, token_epoch=0) + + await socketio._handle_user_access_changed(("user_access_changed", stale)) + + socketio._sio.disconnect.assert_not_awaited() + + @pytest.mark.anyio + async def test_a_deleted_record_disconnects_even_if_the_event_says_active( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + socketio = self._socketio({"sid-1": {"user_id": "user-1", "is_admin": False, "token_epoch": 0}}) + self._patch_users(monkeypatch, None) + stale = UserAccessChangedEvent.build(user_id="user-1", is_admin=False, is_active=True) + + await socketio._handle_user_access_changed(("user_access_changed", stale)) + + socketio._sio.disconnect.assert_awaited_once_with("sid-1") + + @pytest.mark.anyio + async def test_an_unreadable_record_leaves_the_event_standing(self, monkeypatch: pytest.MonkeyPatch) -> None: + """The event is evidence of a committed change; a read that cannot contradict it + does not get to override it.""" + socketio = self._socketio({"sid-1": {"user_id": "user-1", "is_admin": True, "token_epoch": 0}}) + + def explode(user_id: str): + raise RuntimeError("database is locked") + + invoker = SimpleNamespace( + services=SimpleNamespace( + configuration=SimpleNamespace(multiuser=True), + users=SimpleNamespace(get=explode), + ) + ) + monkeypatch.setattr("invokeai.app.api.dependencies.ApiDependencies", SimpleNamespace(invoker=invoker)) + event = UserAccessChangedEvent.build(user_id="user-1", is_admin=False, is_active=False) + + await socketio._handle_user_access_changed(("user_access_changed", event)) + + socketio._sio.disconnect.assert_awaited_once_with("sid-1") From 8a3d38d5c6960e2fa323fbec51600ad85ae16976 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sun, 9 Aug 2026 17:03:36 -0400 Subject: [PATCH 18/19] docs(multiuser): describe live revocation, not just token expiry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../docs/features/Multi-User Mode/admin-guide.mdx | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/docs/src/content/docs/features/Multi-User Mode/admin-guide.mdx b/docs/src/content/docs/features/Multi-User Mode/admin-guide.mdx index d5fac328a73..d33c2625af6 100644 --- a/docs/src/content/docs/features/Multi-User Mode/admin-guide.mdx +++ b/docs/src/content/docs/features/Multi-User Mode/admin-guide.mdx @@ -290,7 +290,17 @@ This system uses stateless JWT tokens with HMAC signatures to identify users aft At the client side, tokens are stored in browser localStorage. Logging out clears them. No server-side session storage is required. -The tokens include the user's ID, email, and admin status, along with an HMAC signature. +The tokens include the user's ID, email, admin status, and a revocation counter, along with an HMAC signature. + +**Revocation:** A token proves *identity* only — expiry is not the only thing that can end a session. Every authenticated request re-reads the account from the database, so administrative changes take effect on the target's next request, without waiting for their token to expire and without requiring them to log out: + +- **Role changes.** The admin status carried in a token is ignored; each request uses the role currently recorded for the account. A demoted administrator loses administrative endpoints immediately, and a promoted user gains them without logging in again. +- **Deactivation and deletion.** Requests from a deactivated or deleted account are rejected, its open connections are closed, and any of its queue items are canceled — running ones stop at the next node, and pending ones are rejected when they reach the front of the queue. Work already completed is not undone. +- **Password changes.** Changing or resetting a password increments the account's revocation counter, which invalidates every token issued before it. All of that account's other sessions are signed out, including any stolen token. The session that performed the change is issued a replacement automatically and stays signed in. + +Changes made through the web UI apply to open connections immediately. Changes made with the `invoke-usermod` and `invoke-userdel` commands run in a separate process from the server: everything the server derives per request (above) still changes the instant the command commits, but connections that are already open are re-checked on a periodic sweep, so closing them can take up to about a minute. + +Two limits are worth stating plainly. Revocation is enforced by this server, so a token remains cryptographically valid until it expires — anything that accepts these tokens without consulting the database would still honor it. And a session already in flight is stopped, not rewound: media already delivered to the client and outputs already written stay written. ### Secret Key Management From 49d00b3a873ec0a9318ed9e5bf95912c7033c35a Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sun, 9 Aug 2026 17:34:29 -0400 Subject: [PATCH 19/19] fix(auth): route the sliding-window refresh through resolve_authorized_user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- invokeai/app/api_app.py | 44 ++++++++++++---------- tests/app/api/test_sliding_window_token.py | 29 ++++++++++++++ 2 files changed, 54 insertions(+), 19 deletions(-) diff --git a/invokeai/app/api_app.py b/invokeai/app/api_app.py index 8c6d9b34861..2fd273017d0 100644 --- a/invokeai/app/api_app.py +++ b/invokeai/app/api_app.py @@ -141,26 +141,32 @@ async def dispatch(self, request: Request, call_next: RequestResponseEndpoint): token_data = verify_token(token) if token_data is not None: - # The user lookup is a synchronous SQLite query behind a - # process-wide lock; run it off the event loop so a contended - # lock (e.g. generation-result writes) can't stall every - # concurrent request from inside this per-mutation middleware. - # Refresh only for a user that still exists and is active, and mint - # the new token from the *database* record — not the old token's - # claims. Otherwise a demoted administrator's stale is_admin claim - # (and the media cookie carrying it) would be renewed indefinitely - # by their own mutations, and a deactivated user could keep an - # active session alive. - user = await run_in_threadpool(ApiDependencies.invoker.services.users.get, token_data.user_id) - if user is None or not user.is_active: - return response - # Never refresh a revoked token. This runs after the route, so an - # authenticated route has already rejected it — but an unauthenticated - # route returning 2xx with a stale Bearer header still reaches here, - # and minting from the current record would launder the revoked token - # into a valid one. - if token_data.token_epoch != user.token_epoch: + # Decide through `resolve_authorized_user` rather than re-deriving + # "is this token still honored" here. This middleware used to carry + # its own copy of the exists/active/epoch checks, and a copy is how + # a rule added later — the refusal of the internal `system` id — + # reaches every other entry point but not this one, leaving a token + # nothing will accept being renewed indefinitely anyway. + # + # Never refresh a token that is no longer honored. This runs after + # the route, so an authenticated route has already rejected it — but + # an unauthenticated route returning 2xx with a stale Bearer header + # still reaches here, and minting from the current record would + # launder a revoked token into a valid one. + # + # The lookup inside is a synchronous SQLite query behind a + # process-wide lock; run it off the event loop so a contended lock + # (e.g. generation-result writes) can't stall every concurrent + # request from inside this per-mutation middleware. + from invokeai.app.api.auth_dependencies import resolve_authorized_user + + user = await run_in_threadpool(resolve_authorized_user, token_data) + if user is None: return response + # Mint the replacement from the *database* record, not the old + # token's claims: otherwise a demoted administrator's stale is_admin + # claim (and the media cookie carrying it) would be renewed + # indefinitely by their own mutations. # Use the remember_me claim from the token to determine the # correct refresh duration. This avoids the bug where a 7-day # token with <24h remaining would be silently downgraded to 1 day. diff --git a/tests/app/api/test_sliding_window_token.py b/tests/app/api/test_sliding_window_token.py index 09753a66152..0cefcf4e172 100644 --- a/tests/app/api/test_sliding_window_token.py +++ b/tests/app/api/test_sliding_window_token.py @@ -433,3 +433,32 @@ def test_active_user_refresh_preserves_remember_me(self, monkeypatch: pytest.Mon refreshed = verify_token(response.headers["X-Refreshed-Token"]) assert refreshed is not None assert refreshed.remember_me is True + + def test_a_system_token_is_never_refreshed(self, monkeypatch: pytest.MonkeyPatch): + """The middleware must decide through `resolve_authorized_user`, not its own copy. + + The `system` row is deliberately active with an untouched epoch, so a middleware + carrying its own exists/active/epoch checks would keep minting replacements for a + token every other entry point refuses — an indefinitely renewed session waiting for + one consumer to trust the token without re-checking the id. + """ + from types import SimpleNamespace + + _patch_user_record( + monkeypatch, + SimpleNamespace( + user_id="system", email="system@system.invokeai", is_admin=False, is_active=True, token_epoch=0 + ), + ) + app = _create_test_app() + client = TestClient(app) + token = create_access_token( + TokenData(user_id="system", email="system@system.invokeai", is_admin=False), + timedelta(days=1), + ) + + response = client.post("/test", headers={"Authorization": f"Bearer {token}"}) + + assert response.status_code == 200 + assert "X-Refreshed-Token" not in response.headers + assert response.cookies.get("invokeai_media_token") is None