From 0889ef0d085d95129a55f828a256b0b6ad96ca2b Mon Sep 17 00:00:00 2001 From: Mike Teehan Date: Wed, 12 Aug 2026 16:37:06 +0000 Subject: [PATCH] fix: prevent data loss in update_permissions and token_refresh - update_permissions: swap to upsert-first-then-delete-stale so a failed upsert mid-loop rolls back cleanly, leaving the user's permissions intact. Adds Q_DELETE_PERMISSION_SUBSYSTEM for targeted removal. - auth_refresh: generate and persist the new refresh token before blacklisting/clearing the old one, so a failure in generate_tokens doesn't leave the user locked out with no valid refresh token. --- daemon/handlers/auth.py | 8 ++++++-- lib/auth_users.py | 14 ++++++++++++-- lib/db.py | 1 + lib/db_sqlite.py | 4 ++++ 4 files changed, 23 insertions(+), 4 deletions(-) diff --git a/daemon/handlers/auth.py b/daemon/handlers/auth.py index 87399f0..b291ecc 100644 --- a/daemon/handlers/auth.py +++ b/daemon/handlers/auth.py @@ -184,13 +184,17 @@ def auth_refresh(_request: Any, body: Any) -> dict[str, Any]: if user is None: raise ValueError("User not found") + # Persist new tokens first, then invalidate the old ones. + # This prevents data loss if generate_tokens fails mid-way: + # the old refresh token remains valid and the user is not locked out. + permissions = user["permissions"] + tokens = generate_tokens(username, permissions) + jti = payload.get("jti") if jti: blacklist_token(jti, token_type="refresh") if username: _clear_refresh_token_after_rotation(username) - permissions = user["permissions"] - tokens = generate_tokens(username, permissions) return { "tokens": tokens, diff --git a/lib/auth_users.py b/lib/auth_users.py index f53e9e6..f81b516 100644 --- a/lib/auth_users.py +++ b/lib/auth_users.py @@ -16,7 +16,7 @@ from lib.auth import ( rotate_user_secret, ) from lib.db import ( - Q_DELETE_PERMISSIONS, + Q_DELETE_PERMISSION_SUBSYSTEM, Q_DELETE_USER, Q_INSERT_USER, Q_SELECT_PERMISSIONS, @@ -230,9 +230,19 @@ def update_permissions(username: str, permissions: dict[str, str]) -> None: db = get_db() with db.in_transaction() as tx: - tx.run(Q_DELETE_PERMISSIONS, (username,)) + # Upsert all new permissions first, then remove stale ones. + # This order ensures that if an upsert fails mid-loop, the user's + # permissions remain intact (transaction rolls back) rather than + # being permanently wiped. + existing = { + row["subsystem"]: row["level"] + for row in db.query(Q_SELECT_PERMISSIONS, (username,)) + } for subsystem, level in permissions.items(): tx.run(Q_UPSERT_PERMISSION, (username, subsystem, level)) + for subsystem in existing: + if subsystem not in permissions: + tx.run(Q_DELETE_PERMISSION_SUBSYSTEM, (username, subsystem)) blacklist_active_refresh_token(username) rotate_user_secret(username) diff --git a/lib/db.py b/lib/db.py index 7929446..f49fd15 100644 --- a/lib/db.py +++ b/lib/db.py @@ -40,6 +40,7 @@ Q_DELETE_USER = "delete_user" Q_UPSERT_PERMISSION = "upsert_permission" Q_SELECT_PERMISSIONS = "select_permissions" Q_DELETE_PERMISSIONS = "delete_permissions" +Q_DELETE_PERMISSION_SUBSYSTEM = "delete_permission_subsystem" Q_INSERT_BLACKLIST = "insert_blacklist" Q_SELECT_BLACKLIST = "select_blacklist_jti" Q_DELETE_EXPIRED_BLACKLIST = "delete_expired_blacklist" diff --git a/lib/db_sqlite.py b/lib/db_sqlite.py index 24fc66f..fb34418 100644 --- a/lib/db_sqlite.py +++ b/lib/db_sqlite.py @@ -13,6 +13,7 @@ from typing import Any, ClassVar from lib.db import ( Q_DELETE_EXPIRED_BLACKLIST, + Q_DELETE_PERMISSION_SUBSYSTEM, Q_DELETE_PERMISSIONS, Q_DELETE_REFRESH_TOKEN, Q_DELETE_USER, @@ -88,6 +89,9 @@ class SQLiteBackend(Database): "SELECT subsystem, level FROM permissions WHERE username = ?" ), Q_DELETE_PERMISSIONS: ("DELETE FROM permissions WHERE username = ?"), + Q_DELETE_PERMISSION_SUBSYSTEM: ( + "DELETE FROM permissions WHERE username = ? AND subsystem = ?" + ), # Token blacklist Q_INSERT_BLACKLIST: ( "INSERT OR IGNORE INTO token_blacklist (jti, token_type, expires) VALUES (?, ?, ?)"