diff --git a/api/app/app/core/token_store.py b/api/app/app/core/token_store.py index db7a1d6..737baad 100644 --- a/api/app/app/core/token_store.py +++ b/api/app/app/core/token_store.py @@ -59,7 +59,17 @@ def revoke_refresh_jti(jti: str) -> None: logger.error("Failed to revoke refresh jti: %s", exc) -def revoke_all_user_refresh_tokens(user_id: str) -> int: +def revoke_all_user_refresh_tokens(user_id: str) -> int | None: + """Revoke every refresh token issued to a user. + + Returns the number of tokens revoked (0 if none existed), or None if + the operation could not be completed (e.g. Redis unreachable) -- the + caller MUST treat None as "revocation not guaranteed", not as + "nothing to revoke". This matters most for password-reset, which + relies on this call to actually invalidate any session an attacker + may hold; silently reporting success on a Redis hiccup would leave + those sessions valid with no indication anything went wrong. + """ try: redis = get_redis() index_key = f"{USER_INDEX_PREFIX}{user_id}" @@ -74,5 +84,5 @@ def revoke_all_user_refresh_tokens(user_id: str) -> int: pipe.execute() return len(jtis) except Exception as exc: - logger.error("Failed to bulk-revoke refresh tokens: %s", exc) - return 0 + logger.error("Failed to bulk-revoke refresh tokens for %s: %s", user_id, exc) + return None diff --git a/api/app/app/modules/auth/router.py b/api/app/app/modules/auth/router.py index 8d8b386..4b8e888 100644 --- a/api/app/app/modules/auth/router.py +++ b/api/app/app/modules/auth/router.py @@ -126,7 +126,21 @@ def password_reset_confirm_endpoint( detail="Invalid or expired reset token.", ) if result.user_id: - revoke_all_user_refresh_tokens(result.user_id) + revoked = revoke_all_user_refresh_tokens(result.user_id) + if revoked is None: + # The password itself was already changed -- don't pretend + # that didn't happen -- but we can't guarantee any refresh + # token an attacker holds was actually invalidated, which is + # the whole point of revoking on reset. Surface that instead + # of silently reporting full success. + raise HTTPException( + status_code=status.HTTP_503_SERVICE_UNAVAILABLE, + detail=( + "Your password was changed, but we couldn't confirm " + "your other sessions were signed out. Please sign " + "out of other devices manually, or try again shortly." + ), + ) return SimpleStatusResponse() diff --git a/api/app/tests/test_token_store_revocation.py b/api/app/tests/test_token_store_revocation.py new file mode 100644 index 0000000..9ba7645 --- /dev/null +++ b/api/app/tests/test_token_store_revocation.py @@ -0,0 +1,68 @@ +"""revoke_all_user_refresh_tokens must distinguish "nothing to revoke" +(0) from "couldn't complete the revocation" (None) -- callers like +password-reset confirm rely on this to know whether it's safe to report +success.""" + +from typing import Any + + +from app.core import token_store + + +class FakePipeline: + def __init__(self, redis: "FakeRedis") -> None: + self._redis = redis + self._ops: list[tuple[str, tuple[Any, ...]]] = [] + + def delete(self, key: str) -> "FakePipeline": + self._ops.append(("delete", (key,))) + return self + + def execute(self) -> list[Any]: + for op, args in self._ops: + getattr(self._redis, f"_do_{op}")(*args) + return [None] * len(self._ops) + + +class FakeRedis: + def __init__( + self, members: set[bytes] | None = None, fail: bool = False + ) -> None: + self._members = members or set() + self._fail = fail + self._store: dict[str, bytes] = {} + + def smembers(self, _key: str) -> set[bytes]: + if self._fail: + raise ConnectionError("redis unreachable") + return self._members + + def pipeline(self) -> FakePipeline: + return FakePipeline(self) + + def _do_delete(self, key: str) -> None: + self._store.pop(key, None) + + +def test_returns_zero_when_user_has_no_tokens(monkeypatch: Any) -> None: + fake = FakeRedis(members=set()) + monkeypatch.setattr(token_store, "get_redis", lambda: fake) + assert token_store.revoke_all_user_refresh_tokens("user-1") == 0 + + +def test_returns_count_when_tokens_revoked(monkeypatch: Any) -> None: + fake = FakeRedis(members={b"jti-a", b"jti-b", b"jti-c"}) + monkeypatch.setattr(token_store, "get_redis", lambda: fake) + assert token_store.revoke_all_user_refresh_tokens("user-1") == 3 + + +def test_returns_none_not_zero_when_redis_fails(monkeypatch: Any) -> None: + fake = FakeRedis(fail=True) + monkeypatch.setattr(token_store, "get_redis", lambda: fake) + result = token_store.revoke_all_user_refresh_tokens("user-1") + # The critical assertion: a Redis failure must be distinguishable + # from "there was nothing to revoke". Returning 0 here would let a + # caller (e.g. password-reset confirm) believe revocation succeeded + # when it didn't run at all. + assert result is None + assert result != 0