diff --git a/changelog.d/128.added.md b/changelog.d/128.added.md new file mode 100644 index 0000000..d211469 --- /dev/null +++ b/changelog.d/128.added.md @@ -0,0 +1,4 @@ +**`set_if_equals(key, expected, value, ttl=None)` on every backend.** It stores +`value` only while `key` still holds `expected`, atomically on the memory, +Redis and Memcached backends. `BaseCacheBackend` provides a non-atomic +fallback for custom backends. diff --git a/changelog.d/128.changed.2.md b/changelog.d/128.changed.2.md new file mode 100644 index 0000000..eabd561 --- /dev/null +++ b/changelog.d/128.changed.2.md @@ -0,0 +1,8 @@ +**Rotating a session ID refuses a session that ended meanwhile.** +`regenerate_session_id()` removes the old record atomically and raises +`SessionNotFoundError` or `SessionInvalidError`, storing nothing under a new +ID, if another request deleted, invalidated or rotated the session since it +was read. `rotate_session_id()` answers `401` in that case and sends no token, +and `login()` starts a new session for the user without the ended session's +data. See the +[migration guide](https://fastapi-cachex.readthedocs.io/en/stable/MIGRATING_0_4/#session-writes). diff --git a/changelog.d/128.changed.md b/changelog.d/128.changed.md new file mode 100644 index 0000000..97651d2 --- /dev/null +++ b/changelog.d/128.changed.md @@ -0,0 +1,9 @@ +**Session saves are conditional.** The middleware's save of `request.session` +changes, sliding renewal in `get_session()` and `update_session()` store a +session only while the backend still holds what the request last read or +wrote, so a request cannot bring back a session that another request deleted, +invalidated or rotated. A dropped save is logged and the response is sent +without a session token, unless the request already stored a renewal for a +session that is still valid. When two requests change the same session at once, +the first save wins. `update_session()` returns `True` or `False`. See the +[migration guide](https://fastapi-cachex.readthedocs.io/en/stable/MIGRATING_0_4/#session-writes). diff --git a/docs/BACKENDS.md b/docs/BACKENDS.md index 426936f..2d1c0b3 100644 --- a/docs/BACKENDS.md +++ b/docs/BACKENDS.md @@ -325,8 +325,15 @@ if await backend.set_if_absent(f"stream:{user_id}", owner, ttl=300): Memory updates under its lock, Redis compares in Python then executes a Lua script (`GET` compare + `EXPIRE`), and Memcached uses `GETS` + `CAS` writing the same bytes with the new exptime (`TOUCH` takes no CAS token). - -All five have a non-atomic fallback on `BaseCacheBackend`, so a third-party backend +- `set_if_equals(key, expected, value, ttl=None) -> bool` — stores `value` only + while `key` still holds `expected`: a compare-and-set that fails if anything + changed, deleted or expired the key since the caller read it. Sessions save + through it (see [Session writes](MIGRATING_0_4.md#session-writes)). Memory + compares under its lock, Redis compares in Python then runs a Lua script + (`GET` compare + `SET`, with `EX` when `ttl` is set), and Memcached uses + `GETS` + a `CAS` write of the new value. + +All six have a non-atomic fallback on `BaseCacheBackend`, so a third-party backend that only implements the abstract methods keeps working; override them to get real atomicity. @@ -334,7 +341,7 @@ Complete runnable example: [`examples/rate_limit.py`](https://github.com/allen00 ## TTL values -Every `ttl` argument (`set`, `set_if_absent`, `increment`, and the `CacheManager` +Every `ttl` argument (`set`, `set_if_absent`, `set_if_equals`, `increment`, and the `CacheManager` and `StateManager` methods and defaults built on them) is either `None`, meaning the entry never expires, or an `int` number of seconds from 1 up to `MAX_TTL` (2**31 - 1, about 68 years). The checks run before any backend I/O: diff --git a/docs/MIGRATING_0_4.md b/docs/MIGRATING_0_4.md index 65f9577..dfe66ee 100644 --- a/docs/MIGRATING_0_4.md +++ b/docs/MIGRATING_0_4.md @@ -185,9 +185,13 @@ SessionConfig( 0.4.0 writes sessions conditionally ([#128](https://github.com/allen0099/FastAPI-CacheX/issues/128)): a request that loaded a session before another request deleted, invalidated or rotated it can no longer bring the record back when it saves. No code change is needed. -- Ordinary saves become conditional: the middleware's save of `request.session` changes, sliding renewal and `update_session()`. Each succeeds only while the stored record still equals what this request last read or wrote. Deleting, invalidating, expiring and rotating stay unconditional, so a security action always wins. -- A rejected save is dropped and logged. The response is still sent, without a session token. +- Ordinary saves become conditional: the middleware's save of `request.session` changes, sliding renewal and `update_session()`. Each succeeds only while the stored record still equals what this request last read or wrote. Deleting, invalidating and expiring stay unconditional, so a security action always wins. +- A rejected save is dropped and logged. The response is still sent, without a session token. The exception is a renewal that this request already stored: while the session is still valid (the save lost only to another save), the renewed token is sent, so a JWT client does not keep a token that expires before the record. +- Rotating the ID (`regenerate_session_id()`) removes the old record atomically and goes ahead only if that record was still valid, so a copy read before another request deleted, invalidated or rotated the session cannot come back under a new ID. Otherwise it raises `SessionNotFoundError` or `SessionInvalidError` and stores nothing under a new ID (the old record is removed either way), and of two concurrent rotations of one session only the first succeeds. A change saved by another request in between does not stop the rotation. `rotate_session_id()` answers `401` in that case and sends no token, and `login()` starts a new session for the user instead, without the data of the ended one. A `Session` built by hand, never read from or written to the backend, is rotated as before. - **Side effect:** when two requests change the same session at the same time (two tabs adding to a cart), the first save wins and the second is dropped. Today the last save wins, so one of the two changes is already lost; 0.4.0 changes which one. Merging such changes is tracked in [#376](https://github.com/allen0099/FastAPI-CacheX/issues/376). +- `update_session()` now returns `True` when the session was stored and `False` when the save was dropped. A `Session` built by hand, never read from the backend, is stored only if no record exists under its ID yet. +- When `get_session()` renews a session (sliding expiry) and the renewal loses a race with another save, it reads the session again and retries once. If it loses again, the request keeps the session it read and no renewed token is sent. +- `Session` equality ignores which backend entry each copy last read or wrote, so a loaded session still equals one built with the same fields. - The backend gains `set_if_equals(key, expected, value, ttl=None)`, next to `delete_if_equals` and `expire_if_equals`. The base class provides a non-atomic fallback, so a custom backend keeps working unchanged; override it to make the save atomic. ### Token sources {#token-source-priority} diff --git a/docs/SESSION.md b/docs/SESSION.md index cf40deb..e680463 100644 --- a/docs/SESSION.md +++ b/docs/SESSION.md @@ -121,8 +121,12 @@ settings from `REDIS_HOST`, `REDIS_PORT`, `REDIS_DB` and `REDIS_PASSWORD`. ``` -Changes made to a `Session` object inside a handler (flash messages, `session.data`, -`session.user`) are only persisted when you call `session_manager.update_session(session)`. +Changes made to a `Session` object inside a handler (flash messages, `session.data`) are only +persisted when you call `session_manager.update_session(session)`. The save is conditional: it +stores the session only while the backend still holds what this object last read or wrote, and +returns `False` otherwise. A session that another request deleted, invalidated or rotated in the +meantime is not brought back, and of two requests that change the same session at the same time, +the first save wins (see [Session writes](MIGRATING_0_4.md#session-writes)). `delete_user_sessions()` and `clear_expired_sessions()` enumerate every key in the backend via `get_all_keys()` and load each session under `backend_key_prefix`, so their cost grows with the @@ -181,7 +185,10 @@ async def me(session=Depends(require_user_session)): (`SessionManager.create_anonymous_session()`, with IP/User-Agent bindings applied as configured) and sends its token back through the request's transport. - Modifying it on a loaded session saves the new contents to the backend via `update_session()`, - replacing `Session.data` with the dict's contents. + replacing `Session.data` with the dict's contents. If another request deleted, invalidated or + rotated the session, or saved it first, while this one ran, the save is dropped and logged. The + response sends no session token, except a renewed one that this request already stored for a + session that is still valid. - Clearing it (`request.session.clear()`) on a loaded session logs out: the backend session is deleted even if its data was already empty, and a cookie client also receives a `Set-Cookie` that expires the cookie. Keys written after `clear()` in the same request go into a new @@ -564,12 +571,16 @@ new ID, keeping its data, user, `created_at` and expiry. Either middleware sees and sends a token for it through the transport the request used: `Set-Cookie` for a cookie, the response header for a header token. After that the old token no longer resolves to a session. For a new visitor there is no session to rotate, so it returns -`False`. +`False`. If another request deleted, invalidated or rotated the session while this one ran, +nothing is saved or sent and it answers `401`; `login()` starts a new session for the user instead (see +[Session writes](MIGRATING_0_4.md#session-writes)). A handler that already holds the request's session object can call -`await manager.regenerate_session_id(session)` directly, with the same effect. Get it from -`get_optional_session` and skip the call when it is `None`; `SessionDep` answers `401` to a -visitor who has no session yet. +`await manager.regenerate_session_id(session)` directly. Get it from `get_optional_session` and +skip the call when it is `None`; `SessionDep` answers `401` to a visitor who has no session yet. +Unlike `rotate_session_id()`, a direct call raises `SessionNotFoundError` or `SessionInvalidError` +when another request ended the session meanwhile, so catch `SessionError` and answer as for a +missing session. Outside a middleware, load the session with the same bindings the middleware would pass, and hand the returned token to the client yourself: diff --git a/fastapi_cachex/backends/base.py b/fastapi_cachex/backends/base.py index 036e540..d05f09f 100644 --- a/fastapi_cachex/backends/base.py +++ b/fastapi_cachex/backends/base.py @@ -255,6 +255,39 @@ async def expire_if_equals(self, key: str, expected: CacheEntry, ttl: int) -> bo await self.set(key, expected, ttl=ttl) return True + async def set_if_equals( + self, + key: str, + expected: CacheEntry, + value: CacheEntry, + ttl: int | None = None, + ) -> bool: + """Store ``value`` only while ``key`` still holds ``expected``. + + A compare-and-set: a caller that read ``expected`` earlier overwrites + it only if nothing changed, deleted or expired the key since. Sessions + save through it, so a request that loaded a session cannot bring it + back after another request deleted or invalidated it. + + The base implementation is a best-effort, NON-atomic get-compare-set + fallback for third-party backends; the built-in backends override it + with an atomic implementation. + + Args: + key: Cache key to overwrite + expected: The entry the caller last read or wrote (compared with ``==``) + value: Entry to store in its place + ttl: Time to live in seconds (``None`` = never expires) + + Returns: + Whether ``value`` was stored + """ + validate_ttl(ttl) + if await self.get(key) != expected: + return False + await self.set(key, value, ttl=ttl) + return True + async def increment(self, key: str, delta: int = 1, ttl: int | None = None) -> int: """Atomically add ``delta`` to the integer counter stored at ``key``. diff --git a/fastapi_cachex/backends/memcached.py b/fastapi_cachex/backends/memcached.py index 2952f77..a31af95 100644 --- a/fastapi_cachex/backends/memcached.py +++ b/fastapi_cachex/backends/memcached.py @@ -331,6 +331,50 @@ def _expire_if_equals( ) return bool(updated) + async def set_if_equals( + self, + key: str, + expected: CacheEntry, + value: CacheEntry, + ttl: int | None = None, + ) -> bool: + """Atomically store ``value`` while ``key`` holds ``expected`` (see base class). + + GETS reads the value to compare along with its CAS token, and the CAS + write succeeds only if nothing wrote or deleted the key since. + """ + validate_ttl(ttl) + # Converted up front so a ttl Memcached cannot store fails before I/O. + exptime = _expiry(ttl) + return await asyncio.to_thread( + self._set_if_equals, key, expected, value, ttl, exptime + ) + + def _set_if_equals( + self, + key: str, + expected: CacheEntry, + value: CacheEntry, + ttl: int | None, + exptime: int, + ) -> bool: + """Run ``set_if_equals``'s GETS, compare and CAS in one worker thread.""" + prefixed_key = self._make_key(key) + raw, cas_token = self.client.gets(prefixed_key) + if raw is None or decode_entry(raw) != expected: + logger.debug("Memcached SET_IF_EQUALS MISMATCH; key=%s", key) + return False + stored = self.client.cas( + prefixed_key, encode_entry(value), cas_token, exptime, noreply=False + ) + logger.debug( + "Memcached SET_IF_EQUALS %s; key=%s ttl=%s", + "HIT" if stored else "LOST RACE", + key, + ttl, + ) + return bool(stored) + def _add_delta(self, prefixed_key: str, delta: int) -> int | None: """Apply ``delta`` with INCR/DECR; ``None`` when the key does not exist.""" if delta < 0: diff --git a/fastapi_cachex/backends/memory.py b/fastapi_cachex/backends/memory.py index b9ca1f4..be350ae 100644 --- a/fastapi_cachex/backends/memory.py +++ b/fastapi_cachex/backends/memory.py @@ -240,6 +240,29 @@ async def expire_if_equals(self, key: str, expected: CacheEntry, ttl: int) -> bo logger.debug("Memory cache EXPIRE_IF_EQUALS HIT; key=%s ttl=%s", key, ttl) return True + async def set_if_equals( + self, + key: str, + expected: CacheEntry, + value: CacheEntry, + ttl: int | None = None, + ) -> bool: + """Atomically store ``value`` while ``key`` holds ``expected`` (see base class).""" + validate_ttl(ttl) + async with self.lock: + now = time.time() + item = self.cache.get(key) + if item is None or not _is_live(item, now): + logger.debug("Memory cache SET_IF_EQUALS MISS; key=%s", key) + return False + if item.value != expected: + logger.debug("Memory cache SET_IF_EQUALS MISMATCH; key=%s", key) + return False + expiry = now + ttl if ttl is not None else None + self.cache[key] = CacheItem(value=value, expiry=expiry) + logger.debug("Memory cache SET_IF_EQUALS HIT; key=%s ttl=%s", key, ttl) + return True + async def increment(self, key: str, delta: int = 1, ttl: int | None = None) -> int: """Atomically add ``delta`` to the counter at ``key`` (see base class). diff --git a/fastapi_cachex/backends/redis.py b/fastapi_cachex/backends/redis.py index 6bb141a..f092d99 100644 --- a/fastapi_cachex/backends/redis.py +++ b/fastapi_cachex/backends/redis.py @@ -62,6 +62,21 @@ return 0 """ +# SET that only fires while the key still holds the exact bytes the caller read, +# so a value replaced or deleted in the meantime stays as it is. KEYS[1] = key, +# ARGV[1] = bytes read, ARGV[2] = bytes to store, ARGV[3] = ttl (0 = none). +_SET_IF_EQUALS_SCRIPT = """ +if redis.call('GET', KEYS[1]) ~= ARGV[1] then + return 0 +end +if tonumber(ARGV[3]) > 0 then + redis.call('SET', KEYS[1], ARGV[2], 'EX', ARGV[3]) +else + redis.call('SET', KEYS[1], ARGV[2]) +end +return 1 +""" + # Constructor keywords that 0.4.0 removed. They would otherwise fall through # **kwargs to redis-py and change how keys and replies are encoded. @@ -174,6 +189,7 @@ def __init__( self._expire_if_equals_script = self.client.register_script( _EXPIRE_IF_EQUALS_SCRIPT ) + self._set_if_equals_script = self.client.register_script(_SET_IF_EQUALS_SCRIPT) @staticmethod def load_from_config(config: RedisConfig) -> "AsyncRedisCacheBackend": @@ -374,6 +390,36 @@ async def expire_if_equals(self, key: str, expected: CacheEntry, ttl: int) -> bo ) return bool(updated) + async def set_if_equals( + self, + key: str, + expected: CacheEntry, + value: CacheEntry, + ttl: int | None = None, + ) -> bool: + """Atomically store ``value`` while ``key`` holds ``expected`` (see base class). + + The stored value is decoded and compared here, then a Lua script + writes the key only if it still holds the bytes that were compared, + so a value written or deleted in between is never overwritten. + """ + validate_ttl(ttl) + prefixed_key = self._make_key(key) + raw = await self.client.get(prefixed_key) + if raw is None or decode_entry(raw) != expected: + logger.debug("Redis SET_IF_EQUALS MISMATCH; key=%s", key) + return False + stored = await self._set_if_equals_script( + keys=[prefixed_key], args=[raw, encode_entry(value), ttl or 0] + ) + logger.debug( + "Redis SET_IF_EQUALS %s; key=%s ttl=%s", + "HIT" if stored else "LOST RACE", + key, + ttl, + ) + return bool(stored) + async def increment(self, key: str, delta: int = 1, ttl: int | None = None) -> int: """Atomically add ``delta`` to the counter at ``key`` (see base class). diff --git a/fastapi_cachex/session/dependencies.py b/fastapi_cachex/session/dependencies.py index a3e61a1..741e812 100644 --- a/fastapi_cachex/session/dependencies.py +++ b/fastapi_cachex/session/dependencies.py @@ -14,6 +14,7 @@ from fastapi_cachex.exceptions import ProxyNotSetError +from .exceptions import SessionError from .middleware import _SESSION_READ_KEY from .middleware import _log_in from .middleware import _RequestSession @@ -249,14 +250,26 @@ async def sudo(request: Request, session: AuthenticatedSession): True if a loaded session was given a new ID, False if none was loaded Raises: - HTTPException: 500 if no session middleware has registered a - SessionManager yet + HTTPException: 401 if another request logged the session out (deleted, + invalidated or rotated it) since this one loaded it; 500 if no + session middleware has registered a SessionManager yet """ manager = get_session_manager(request) session: Session | None = getattr(request.state, "__fastapi_cachex_session", None) if session is None: return False - await manager.regenerate_session_id(session) + try: + await manager.regenerate_session_id(session) + except SessionError as e: + # Another request logged the session out while this one ran (#128). + request_session = request.scope.get("session") + if isinstance(request_session, _RequestSession): + request_session.ended = True + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail="Authentication required", + headers={"WWW-Authenticate": "Bearer"}, + ) from e return True @@ -283,6 +296,10 @@ async def login( - With no session loaded (a new visitor, or a token that did not resolve) a new session is created with the user, bound to the client IP and User-Agent as configured. + - A loaded session that another request deleted, invalidated or rotated + while this one ran is not rotated back to life (#128). A new session is + created as above, and nothing written to ``request.session`` before + the call is carried over. The middleware then saves the session, including anything written to ``request.session`` after the call (and, unless the loaded session was a diff --git a/fastapi_cachex/session/manager.py b/fastapi_cachex/session/manager.py index 891b8a2..fddc0e0 100644 --- a/fastapi_cachex/session/manager.py +++ b/fastapi_cachex/session/manager.py @@ -182,7 +182,7 @@ async def _create_session( ) # Store in backend - await self._save_session(session) + await self._save_session(session, conditional=False) token = self.issue_token(session) logger.debug( @@ -211,6 +211,12 @@ async def get_session( it is stored only when the session is next written, unless ``touch`` is set. + That write is conditional, like ``update_session()``. If another + request changed, deleted or invalidated the session since it was read, + the session is read and checked again, so a deleted or invalidated + session is refused and one another request already renewed is + returned as it is. + Args: token_string: Session token string ip_address: Current request IP address @@ -233,6 +239,20 @@ async def get_session( SessionSecurityError: If a ``simple`` token's signature is wrong, or an IP / User-Agent binding does not match """ + return await self._get_session( + token_string, ip_address, user_agent, touch=touch, retry=True + ) + + async def _get_session( + self, + token_string: str, + ip_address: str | None, + user_agent: str | None, + *, + touch: bool, + retry: bool, + ) -> tuple[Session, str | None]: + """``get_session()``, reading the session again once if its save loses a race.""" # Parse and verify token try: token = self._serializer.from_string(token_string) @@ -329,20 +349,54 @@ async def get_session( self.config.session_ttl, ) - if renewed_token is not None or touch: - await self._save_session(session) + if (renewed_token is not None or touch) and not await self._save_session( + session, conditional=True + ): + if retry: + return await self._get_session( + token_string, ip_address, user_agent, touch=touch, retry=False + ) + # Lost the race twice: the session was valid when read again, so + # return it without the renewal rather than refuse the request. + logger.debug( + "Session renewal dropped after a second concurrent write; id=%s", + session.session_id, + ) + renewed_token = None return session, renewed_token - async def update_session(self, session: Session) -> None: - """Update an existing session. + async def update_session(self, session: Session) -> bool: + """Save changes to an existing session. + + The save is conditional (#128): it succeeds only while the backend + still holds what ``session`` was last read from or written as. If + another request changed, deleted or invalidated the session in the + meantime, nothing is written and ``False`` is returned, so a request + that loaded the session earlier cannot bring back a logged-out or + rotated session. Two concurrent changes to one session therefore keep + the first save, not the last. + + Deleting and invalidating are unconditional. Regenerating the ID + requires only that the session is still valid, not unchanged. Args: - session: Session to update + session: Session to update, as returned by ``get_session()`` or + ``create_session()`` + + Returns: + Whether the session was saved """ session.update_last_accessed() - await self._save_session(session) + if not await self._save_session(session, conditional=True): + logger.info( + "Session save dropped: the session changed, was deleted or was " + "invalidated since it was read; id=%s", + session.session_id, + ) + return False logger.debug("Session updated; id=%s", session.session_id) + return True async def delete_session(self, session_id: str) -> None: """Delete a session. @@ -361,7 +415,7 @@ async def invalidate_session(self, session: Session) -> None: session: Session to invalidate """ session.invalidate() - await self._save_session(session) + await self._save_session(session, conditional=False) logger.debug("Session invalidated; id=%s", session.session_id) async def regenerate_session_id( @@ -375,21 +429,37 @@ async def regenerate_session_id( token through the request's transport, so a handler that only needs the cookie or header updated can ignore the returned token. + A session read from the backend is rotated only while its record is + still there and valid (#128): the old record is removed atomically, so + a copy read before another request deleted or invalidated the session + cannot come back under a new ID, and of two concurrent rotations only + one succeeds. The old record is gone either way, so the old token no + longer resolves, even when the rotation is refused. A session that was + never read from or written to the backend (built by hand) is rotated + without the check. + Args: session: Session to regenerate Returns: Tuple of (updated session, new token string) - """ - # Delete old session - await self.delete_session(session.session_id) - # Generate new ID + Raises: + SessionNotFoundError: The record was deleted since ``session`` was + read, or another request rotated it first + SessionInvalidError: The record was invalidated or has expired + since ``session`` was read + """ old_id = session.session_id + if session._stored is None: # noqa: SLF001 + await self.delete_session(old_id) + else: + await self._claim(old_id) + session.regenerate_id() # Save with new ID - await self._save_session(session) + await self._save_session(session, conditional=False) logger.debug( "Session ID regenerated; old_id=%s new_id=%s", old_id, session.session_id @@ -475,7 +545,7 @@ async def _iter_sessions(self) -> AsyncIterator[tuple[str, Session]]: async def _expire(self, session: Session, reason: str) -> None: """Persist ``session`` as expired and raise SessionExpiredError.""" session.status = SessionStatus.EXPIRED - await self._save_session(session) + await self._save_session(session, conditional=False) raise SessionExpiredError(reason) def _create_token( @@ -504,11 +574,17 @@ def _create_token( session_id=session_id, signature=signature, expires_at=expires_at ) - async def _save_session(self, session: Session) -> None: + async def _save_session(self, session: Session, *, conditional: bool) -> bool: """Save session to backend. Args: session: Session to save + conditional: Store only while the backend holds what ``session`` + was last read from or written as (only if the record is absent, + when it was never read or written); otherwise overwrite + + Returns: + Whether the session was saved (always True when not conditional) """ payload = session.model_dump_json() @@ -522,10 +598,18 @@ async def _save_session(self, session: Session) -> None: fingerprint=_SESSION_FINGERPRINT, content=payload.encode("utf-8"), ) - await self.backend.set( - self._get_backend_key(session.session_id), entry, ttl=ttl - ) + key = self._get_backend_key(session.session_id) + expected = session._stored # noqa: SLF001 + if not conditional: + await self.backend.set(key, entry, ttl=ttl) + elif expected is None: + if not await self.backend.set_if_absent(key, entry, ttl=ttl): + return False + elif not await self.backend.set_if_equals(key, expected, entry, ttl=ttl): + return False + session._stored = entry # noqa: SLF001 logger.debug("Session saved; id=%s ttl=%s", session.session_id, ttl) + return True async def _load_session(self, session_id: str) -> Session | None: """Load session from backend. @@ -538,6 +622,29 @@ async def _load_session(self, session_id: str) -> Session | None: """ return await self._load_session_by_key(self._get_backend_key(session_id)) + async def _claim(self, session_id: str) -> None: + """Remove the record under ``session_id``, raising unless it was valid.""" + claimed = await self.backend.get_and_delete(self._get_backend_key(session_id)) + current = self._decode(claimed) if claimed else None + if current is None: + msg = f"Session {session_id} was deleted before its ID could be regenerated" + logger.debug("Session rotation refused, not found; id=%s", session_id) + raise SessionNotFoundError(msg) + if not current.is_valid(): + msg = f"Session {session_id} ended before its ID could be regenerated" + logger.debug("Session rotation refused, not valid; id=%s", session_id) + raise SessionInvalidError(msg) + + @staticmethod + def _decode(entry: CacheEntry) -> Session | None: + """The session stored as ``entry``, or None if it does not parse.""" + try: + session = Session.model_validate_json(entry.content) + except (ValueError, TypeError): + return None + session._stored = entry # noqa: SLF001 + return session + async def _load_session_by_key(self, key: str) -> Session | None: """Load session from backend by key. @@ -552,8 +659,7 @@ async def _load_session_by_key(self, key: str) -> Session | None: logger.debug("Session load MISS; key=%s", key) return None - try: - return Session.model_validate_json(cached.content) - except (ValueError, TypeError): + session = self._decode(cached) + if session is None: logger.debug("Session load DESERIALIZE ERROR; key=%s", key) - return None + return session diff --git a/fastapi_cachex/session/middleware.py b/fastapi_cachex/session/middleware.py index e078f94..050088d 100644 --- a/fastapi_cachex/session/middleware.py +++ b/fastapi_cachex/session/middleware.py @@ -204,6 +204,10 @@ class _RequestSession(StarletteSession): """ cleared: bool = False + # rotate_session_id() found that another request ended the loaded session + # (#128). Nothing is saved or sent: the winner of a concurrent rotation may + # already have given the client its new token. + ended: bool = False # The backend session this dict belongs to: the one the middleware loaded, # or the one ``login()`` started or rotated. Read when the response starts. backend: "Session | None" = None @@ -376,6 +380,8 @@ async def _persist( # noqa: PLR0913, PLR0917 Returns: True if a token or a clearing cookie was written to the response """ + if session.ended: + return False sent_token = False target = backend_session if session.cleared and target is not None: @@ -477,7 +483,7 @@ async def _write_session( backend_session: "Session | None", loaded_token: str | None, renewed_token: str | None, - ) -> tuple[str, str | None]: + ) -> tuple[str | None, str | None]: """Create-or-update the backend session for a modified dict. The dict is non-empty, or was emptied on a session that has a user. @@ -494,6 +500,7 @@ async def _write_session( ``(cookie_token, new_token)`` where ``cookie_token`` is the token to (re)set as a cookie, and ``new_token`` is a genuinely new/renewed token to hand a header-based client (``None`` when the token is unchanged). + Both are ``None`` when the save was dropped. """ if backend_session is None: ( @@ -510,9 +517,22 @@ async def _write_session( assert loaded_token is not None # noqa: S101 new_token = renewed_token backend_session.data = dict(session) - await self.session_manager.update_session(backend_session) + if not await self.session_manager.update_session(backend_session): + # Another request changed, deleted or invalidated the session since + # this one read it (#128). The save is dropped. A renewal that + # get_session() already stored is still sent while the session is + # valid: a JWT client would otherwise keep a token that expires + # before the record does. Otherwise no token is sent. + if new_token is not None and await self._still_valid(backend_session): + return new_token, new_token + return None, None return loaded_token, new_token + async def _still_valid(self, session: "Session") -> bool: + """Whether the backend still holds a valid session under this ID.""" + current = await self.session_manager._load_session(session.session_id) # noqa: SLF001 + return current is not None and current.is_valid() + def _build_set_cookie_header(self, token: str) -> str: """Build a `Set-Cookie` header value carrying the session token. @@ -613,7 +633,18 @@ async def _log_in( # Attach the user before the rotation, so the record under the old ID # never holds it. current._attach_user(user) # noqa: SLF001 - await manager.regenerate_session_id(current) + try: + await manager.regenerate_session_id(current) + except SessionError: + # Another request ended the loaded session while this one ran + # (#128). Its data went with it: log in on a new session that + # starts with only what the handler writes after login(). + dict.clear(request_session) + current, _ = await manager.create_session( + user, + ip_address=get_client_ip(connection, middleware.config), + user_agent=connection.headers.get("user-agent"), + ) # The session is already stored with the user. Its ID differs from the one # the middleware loaded (if any), so the middleware sends a token for it diff --git a/fastapi_cachex/session/models.py b/fastapi_cachex/session/models.py index 00cdbf2..d56e437 100644 --- a/fastapi_cachex/session/models.py +++ b/fastapi_cachex/session/models.py @@ -11,6 +11,9 @@ from pydantic import BaseModel from pydantic import Field +from pydantic import PrivateAttr + +from fastapi_cachex.types import CacheEntry logger = logging.getLogger(__name__) @@ -63,6 +66,11 @@ class Session(BaseModel): model_config = {"use_enum_values": True} + # The backend entry this object was last read from or written as. An + # ordinary save stores only while the backend still holds it (#128); None + # means the object was never read or written, so the record must not exist. + _stored: CacheEntry | None = PrivateAttr(default=None) + # Hidden from type checkers: a visible __setattr__ would make them accept # assignment to any attribute name, typos included. if not TYPE_CHECKING: # pragma: no branch @@ -79,6 +87,19 @@ def __setattr__(self, name: str, value: Any) -> None: raise AttributeError(msg) super().__setattr__(name, value) + def __eq__(self, other: object) -> bool: + """Compare the fields only, not which backend entry each copy last saw.""" + if not isinstance(other, Session): + return NotImplemented + return ( + type(self) is type(other) + and self.__dict__ == other.__dict__ + and self.__pydantic_extra__ == other.__pydantic_extra__ + ) + + # Mutable, like every pydantic model that is not frozen. + __hash__ = None # type: ignore[assignment] + def _attach_user(self, user: SessionUser) -> None: """Set ``user``, for ``login()`` only, which rotates the ID right after.""" super().__setattr__("user", user) diff --git a/i18n/zh-TW/docs/BACKENDS.md b/i18n/zh-TW/docs/BACKENDS.md index 5bf7be8..114c42d 100644 --- a/i18n/zh-TW/docs/BACKENDS.md +++ b/i18n/zh-TW/docs/BACKENDS.md @@ -197,14 +197,15 @@ if await backend.set_if_absent(f"stream:{user_id}", owner, ttl=300): - `set_if_absent(key, value, ttl=None) -> bool`：只在 `key` 不存在時儲存 `value`（已過期的鍵視為不存在），並回報是否有寫入。記憶體後端在鎖內檢查，Redis 使用 `SET NX EX`，Memcached 使用 `ADD`。 - `delete_if_equals(key, expected) -> bool`：只在 `key` 仍存放 `expected` 時才移除它，因此項目已過期的持有者無法釋放已被他人取得的鎖。請在你儲存的項目中放入唯一的權杖，並以同一個項目釋放。記憶體後端在鎖內比較，Redis 透過 Lua 腳本刪除，並在腳本中重新檢查先前比較過的值，Memcached 則使用 `GETS` + 一個讓項目立即過期的 `CAS` 寫入（傳統協定的 `DELETE` 不接受 CAS 權杖）。 - `expire_if_equals(key, expected, ttl) -> bool`：只在 `key` 仍存放 `expected` 時，才把它的 TTL 更新為 `ttl` 秒，因此長時間執行的鎖持有者可以續約租期，而不會在鎖已過期時動到別人的鎖。記憶體後端在鎖內更新，Redis 先在 Python 中比較，再執行 Lua 腳本（`GET` 比較 + `EXPIRE`），Memcached 則使用 `GETS` + 以新 exptime 寫回相同位元組的 `CAS`（`TOUCH` 不接受 CAS 權杖）。 +- `set_if_equals(key, expected, value, ttl=None) -> bool`：只在 `key` 仍存放 `expected` 時才儲存 `value`。這是 compare-and-set：若呼叫端讀取之後有任何操作變更、刪除了該鍵，或它已過期，寫入就會失敗。Session 透過它儲存（見 [Session 寫入](MIGRATING_0_4.md#session-writes)）。記憶體後端在鎖內比較，Redis 先在 Python 中比較，再執行 Lua 腳本（`GET` 比較 + `SET`，設定了 `ttl` 時加上 `EX`），Memcached 則使用 `GETS` + 寫入新值的 `CAS`。 -這五個方法在 `BaseCacheBackend` 上都有非原子性的後備實作，因此只實作抽象方法的第三方後端仍可正常運作；覆寫它們才能得到真正的原子性。 +這六個方法在 `BaseCacheBackend` 上都有非原子性的後備實作，因此只實作抽象方法的第三方後端仍可正常運作；覆寫它們才能得到真正的原子性。 完整可執行範例（英文）：[`examples/rate_limit.py`](https://github.com/allen0099/FastAPI-CacheX/blob/master/examples/rate_limit.py)。 ## TTL 值 {#ttl-values} -每個 `ttl` 參數（`set`、`set_if_absent`、`increment`，以及建立在它們之上的 `CacheManager` 與 `StateManager` 方法和預設值）只能是 `None`（表示項目永不過期），或介於 1 到 `MAX_TTL`（2**31 - 1，約 68 年）之間的 `int` 秒數。這些檢查都在存取後端之前進行： +每個 `ttl` 參數（`set`、`set_if_absent`、`set_if_equals`、`increment`，以及建立在它們之上的 `CacheManager` 與 `StateManager` 方法和預設值）只能是 `None`（表示項目永不過期），或介於 1 到 `MAX_TTL`（2**31 - 1，約 68 年）之間的 `int` 秒數。這些檢查都在存取後端之前進行： - 零、負值與更大的值會拋出 `ValueError`。底層儲存對 `0` 的解讀各不相同：Memcached 把 exptime `0` 視為「永不過期」，Redis 拒絕 `EX 0`，而行程內的 dict 則會立即讓項目過期。 - `float`、`bool` 或其他型別會拋出 `TypeError`。float 過去只在記憶體後端上有效，而 `True` 會被當成一秒。`timedelta` 請以 `int(td.total_seconds())` 轉換。 diff --git a/i18n/zh-TW/docs/MIGRATING_0_4.md b/i18n/zh-TW/docs/MIGRATING_0_4.md index 534ba96..6f06fd4 100644 --- a/i18n/zh-TW/docs/MIGRATING_0_4.md +++ b/i18n/zh-TW/docs/MIGRATING_0_4.md @@ -184,9 +184,13 @@ SessionConfig( 0.4.0 會以有條件的方式寫入 Session（[#128](https://github.com/allen0099/FastAPI-CacheX/issues/128)）：若某個請求載入 Session 之後，另一個請求刪除、使其失效或輪替了它，前者儲存時不會再讓紀錄復活。不需要修改程式碼。 -- 一般的儲存改為有條件寫入：中介軟體儲存 `request.session` 的修改、滑動續期，以及 `update_session()`。只有在後端的紀錄仍等於這個請求最後一次讀到或寫入的值時才會成功。刪除、使其失效、過期與輪替 ID 仍無條件執行，因此安全動作永遠優先。 -- 被拒絕的儲存會被捨棄並記錄 log。回應照常送出，但不附 Session 權杖。 +- 一般的儲存改為有條件寫入：中介軟體儲存 `request.session` 的修改、滑動續期，以及 `update_session()`。只有在後端的紀錄仍等於這個請求最後一次讀到或寫入的值時才會成功。刪除、使其失效與過期仍無條件執行，因此安全動作永遠優先。 +- 被拒絕的儲存會被捨棄並記錄 log。回應照常送出，但不附 Session 權杖。例外是這個請求已儲存的續期：若 Session 仍然有效（這次儲存只是輸給另一次儲存），會送出續期後的權杖，避免 JWT 用戶端持有比紀錄更早過期的權杖。 +- 輪替 ID（`regenerate_session_id()`）會以原子操作移除舊紀錄，且只有在該紀錄仍然有效時才繼續，因此在另一個請求刪除、使其失效或輪替 Session 之前讀到的副本，無法以新 ID 復活。否則它會拋出 `SessionNotFoundError` 或 `SessionInvalidError`，不會以新 ID 儲存任何東西（舊紀錄無論如何都會被移除）；同一個 Session 同時被輪替兩次時，只有第一次會成功。期間另一個請求儲存的修改不會阻止輪替。此時 `rotate_session_id()` 回應 `401` 且不送出權杖，`login()` 則改為替使用者建立新的 Session，不帶已結束 Session 的資料。手動建立、從未自後端讀取或寫入的 `Session` 仍照舊輪替。 - **副作用：**兩個請求同時修改同一個 Session 時（例如兩個分頁同時加入購物車），先儲存的成功，後儲存的被捨棄。目前是後儲存的覆蓋先儲存的，本來就會遺失其中一筆修改；0.4.0 改變的是遺失哪一筆。合併這類修改的做法由 [#376](https://github.com/allen0099/FastAPI-CacheX/issues/376) 追蹤。 +- `update_session()` 現在會回傳值：已儲存時為 `True`，儲存被捨棄時為 `False`。手動建立、從未自後端讀取的 `Session`，只有在其 ID 底下尚無紀錄時才會被儲存。 +- `get_session()` 續期 Session（滑動過期）時，若續期與另一次儲存競爭而失敗，會重新讀取 Session 並重試一次。若再次失敗，這個請求沿用它讀到的 Session，且不送出續期後的權杖。 +- `Session` 的相等比較不考慮各副本最後讀到或寫入的後端項目，因此載入的 Session 仍等於以相同欄位建立的 Session。 - 後端新增 `set_if_equals(key, expected, value, ttl=None)`，與 `delete_if_equals`、`expire_if_equals` 同一系列。基底類別提供不具原子性的預設實作，因此自訂後端不需修改也能運作；覆寫它才能讓儲存具有原子性。 ### 權杖來源 {#token-source-priority} diff --git a/i18n/zh-TW/docs/SESSION.md b/i18n/zh-TW/docs/SESSION.md index 3ba06aa..dd9e991 100644 --- a/i18n/zh-TW/docs/SESSION.md +++ b/i18n/zh-TW/docs/SESSION.md @@ -81,7 +81,7 @@ app.add_middleware(FastAPICacheXSessionMiddleware) # 從 proxy 取得 ``` -在 handler 中對 `Session` 物件所做的變更（Flash 訊息、`session.data`、`session.user`），只有在呼叫 `session_manager.update_session(session)` 時才會被儲存。 +在 handler 中對 `Session` 物件所做的變更（Flash 訊息、`session.data`），只有在呼叫 `session_manager.update_session(session)` 時才會被儲存。這個儲存是有條件的：只有在後端仍存放這個物件最後一次讀到或寫入的內容時才會寫入，否則回傳 `False`。期間被另一個請求刪除、使其失效或輪替的 Session 不會被復活；兩個請求同時修改同一個 Session 時，先儲存的成功（見 [Session 寫入](MIGRATING_0_4.md#session-writes)）。 `delete_user_sessions()` 與 `clear_expired_sessions()` 會透過 `get_all_keys()` 列舉後端中的每一個鍵，並載入 `backend_key_prefix` 底下的每個 Session，因此其成本會隨後端的大小增加。在無法列舉鍵的 Memcached 後端上，它們找不到任何東西並回傳 `0`（後端會發出 `RuntimeWarning`）。 @@ -115,7 +115,7 @@ async def me(session=Depends(require_user_session)): `request.session` 是後端 Session 的 `data` dict 的一個視圖： - 在沒有載入任何 Session 時寫入 `request.session`，會建立一個新的**匿名** Session（`SessionManager.create_anonymous_session()`，並依設定套用 IP / User-Agent 綁定），並透過該請求的傳輸方式傳回其權杖。 -- 修改已載入 Session 的 `request.session`，會透過 `update_session()` 將新內容儲存到後端，以 dict 的內容取代 `Session.data`。 +- 修改已載入 Session 的 `request.session`，會透過 `update_session()` 將新內容儲存到後端，以 dict 的內容取代 `Session.data`。若這個請求執行期間，另一個請求刪除、使其失效或輪替了這個 Session，或先一步儲存了它，這次儲存會被捨棄並記錄 log。回應不附 Session 權杖，除非這個請求已儲存了續期，且 Session 仍然有效，此時會送出續期後的權杖。 - 在已載入的 Session 上清除它（`request.session.clear()`）即為登出：即使資料原本就是空的，也會刪除後端的 Session；Cookie 用戶端還會收到一個使 Cookie 過期的 `Set-Cookie`。同一個請求中在 `clear()` 之後寫入的鍵，會存進一個使用新 ID 的新匿名 Session。`await logout(request)` 的效果相同，但會立即刪除後端的 Session，而不是等到送出回應時，且在該請求剩下的處理中，`get_session` 找不到 Session。 - 以 `del` 或 `pop()` 移除最後一個鍵並不是登出。帶有使用者的 Session 會以空資料儲存；匿名 Session 已無任何內容，會和 `clear()` 一樣被刪除。 - 以寫入 `request.session` 的方式登入時，會沿用請求帶來的 Session ID。Starlette 的中介軟體中 Cookie *就是* Session，因此登入回應會取代任何被植入的 Cookie；這裡的 Cookie 只是指向伺服器端紀錄的名稱，被植入的 Cookie 會跟著受害者一起登入。請以 `await login(request, user)` 登入，它會為 Session 換一個新 ID 並附加使用者（見[登入後重新產生 Session ID](#5-regenerate-the-session-id-after-login)）。 @@ -367,9 +367,9 @@ async def sudo(request: Request, session: AuthenticatedSession): return {"ok": True} ``` -`rotate_session_id()` 會對請求的 Session 呼叫 `SessionManager.regenerate_session_id()`，刪除舊 ID 底下的後端紀錄，並以新 ID 儲存該 Session，保留其資料、使用者、`created_at` 與過期時間。任一個中介軟體都會看到新 ID，並透過該請求使用的傳輸方式送出對應的權杖：Cookie 使用 `Set-Cookie`，標頭權杖則使用回應標頭。之後舊的權杖就無法再解析出 Session。新訪客沒有可換 ID 的 Session，因此它會回傳 `False`。 +`rotate_session_id()` 會對請求的 Session 呼叫 `SessionManager.regenerate_session_id()`，刪除舊 ID 底下的後端紀錄，並以新 ID 儲存該 Session，保留其資料、使用者、`created_at` 與過期時間。任一個中介軟體都會看到新 ID，並透過該請求使用的傳輸方式送出對應的權杖：Cookie 使用 `Set-Cookie`，標頭權杖則使用回應標頭。之後舊的權杖就無法再解析出 Session。新訪客沒有可換 ID 的 Session，因此它會回傳 `False`。若這個請求執行期間，另一個請求刪除、使其失效或輪替了這個 Session，則不會儲存或送出任何東西，並回應 `401`；`login()` 則改為替使用者建立新的 Session（見 [Session 寫入](MIGRATING_0_4.md#session-writes)）。 -已經取得請求 Session 物件的 handler，也可以直接呼叫 `await manager.regenerate_session_id(session)`，效果相同。請從 `get_optional_session` 取得 Session，並在它為 `None` 時略過呼叫；`SessionDep` 會對還沒有 Session 的訪客回應 `401`。 +已經取得請求 Session 物件的 handler，也可以直接呼叫 `await manager.regenerate_session_id(session)`。請從 `get_optional_session` 取得 Session，並在它為 `None` 時略過呼叫；`SessionDep` 會對還沒有 Session 的訪客回應 `401`。與 `rotate_session_id()` 不同，若期間另一個請求結束了這個 Session，直接呼叫會拋出 `SessionNotFoundError` 或 `SessionInvalidError`，因此請捕捉 `SessionError`，並比照沒有 Session 的情況回應。 在中介軟體之外，請以中介軟體會傳入的相同綁定值載入 Session，並自行將回傳的權杖交給用戶端： diff --git a/tests/backends/test_base.py b/tests/backends/test_base.py index 4ccacd6..a000a21 100644 --- a/tests/backends/test_base.py +++ b/tests/backends/test_base.py @@ -123,6 +123,22 @@ async def test_expire_if_equals_fallback_updates_ttl_only_when_matching( assert await backend.expire_if_equals("missing", theirs, ttl=60) is False +async def test_set_if_equals_fallback_stores_only_over_a_matching_entry( + backend: DictBackend, +) -> None: + mine = CacheEntry(fingerprint="session", content=b"v1") + theirs = CacheEntry(fingerprint="session", content=b"v2") + new = CacheEntry(fingerprint="session", content=b"v3") + await backend.set("slot", theirs, ttl=30) + + assert await backend.set_if_equals("slot", mine, new, ttl=60) is False + assert backend.store["slot"] == (theirs, 30) + assert await backend.set_if_equals("slot", theirs, new, ttl=60) is True + assert backend.store["slot"] == (new, 60) + assert await backend.set_if_equals("missing", theirs, new) is False + assert "missing" not in backend.store + + async def test_aclose_is_a_no_op_by_default(backend: DictBackend) -> None: await backend.set("key", CacheEntry(fingerprint="f", content=b"v")) diff --git a/tests/backends/test_memcached.py b/tests/backends/test_memcached.py index 5d62393..34b48f8 100644 --- a/tests/backends/test_memcached.py +++ b/tests/backends/test_memcached.py @@ -316,6 +316,7 @@ async def _clear_path(backend: MemcachedBackend) -> int: lambda b: b.get_and_delete("k"), lambda b: b.delete_if_equals("k", _ENTRY), lambda b: b.expire_if_equals("k", _ENTRY, 5), + lambda b: b.set_if_equals("k", _ENTRY, _ENTRY, 5), lambda b: b.delete_many(["a", "b"]), lambda b: b.delete("k"), _clear_path, @@ -328,6 +329,7 @@ async def _clear_path(backend: MemcachedBackend) -> int: "get_and_delete", "delete_if_equals", "expire_if_equals", + "set_if_equals", "delete_many", "delete", "clear_path", @@ -948,6 +950,56 @@ def gets_then_overwrite(key): assert raw == b'{"overwritten": true}' +@requires_memcached +async def test_memcached_set_if_equals_stores_only_over_a_matching_entry( + memcached_backend: MemcachedBackend, +): + mine = CacheEntry(fingerprint="session", content=b"v1") + theirs = CacheEntry(fingerprint="session", content=b"v2") + new = CacheEntry(fingerprint="session", content=b"v3") + await memcached_backend.set("slot", theirs, 30) + + assert await memcached_backend.set_if_equals("slot", mine, new, 60) is False + assert await memcached_backend.get("slot") == theirs + assert await memcached_backend.set_if_equals("slot", theirs, new, 60) is True + assert await memcached_backend.get("slot") == new + assert await memcached_backend.set_if_equals("slot", new, mine) is True + assert await memcached_backend.get("slot") == mine + assert await memcached_backend.set_if_equals("missing", theirs, new) is False + assert await memcached_backend.get("missing") is None + + +@requires_memcached +@pytest.mark.parametrize("change", ["overwrite", "delete"]) +async def test_memcached_set_if_equals_keeps_a_change_made_after_the_compare( + memcached_backend: MemcachedBackend, change: str +): + """A write or delete between GETS and CAS is not undone.""" + mine = CacheEntry(fingerprint="session", content=b"v1") + new = CacheEntry(fingerprint="session", content=b"v3") + await memcached_backend.set("slot", mine, 60) + + client = memcached_backend.client + original_gets = client.gets + + def gets_then_change(key): + result = original_gets(key) + if change == "overwrite": + client.set(key, b'{"overwritten": true}', 60) + else: + client.delete(key) + return result + + client.gets = gets_then_change + try: + assert await memcached_backend.set_if_equals("slot", mine, new, 60) is False + finally: + client.gets = original_gets + + raw = await asyncio.to_thread(client.get, memcached_backend._make_key("slot")) + assert raw == (b'{"overwritten": true}' if change == "overwrite" else None) + + @requires_memcached async def test_lock_lifecycle_with_memcached( memcached_backend: MemcachedBackend, @@ -971,13 +1023,16 @@ def test_expiry_up_to_2038_is_sent_as_a_timestamp(monkeypatch) -> None: @pytest.mark.parametrize( - "operation", ["set", "set_if_absent", "increment", "expire_if_equals"] + "operation", + ["set", "set_if_absent", "increment", "expire_if_equals", "set_if_equals"], ) async def test_ttl_past_2038_is_rejected_before_io(operation: str) -> None: """Such a write used to succeed while Memcached dropped the item at once (#229).""" backend = stubbed_backend() entry = CacheEntry(fingerprint="e", content=b"v") - args = ("k",) if operation == "increment" else ("k", entry) + args: tuple[object, ...] = ("k",) if operation == "increment" else ("k", entry) + if operation == "set_if_equals": + args = ("k", entry, entry) with pytest.raises(ValueError, match="expires after 2038-01-19"): await getattr(backend, operation)(*args, ttl=2**31 - 1) @@ -1049,6 +1104,7 @@ def _stub_for_single_hop(backend: MemcachedBackend) -> None: ("get_and_delete", ("k",)), ("delete_if_equals", ("k", _ENTRY)), ("expire_if_equals", ("k", _ENTRY, 5)), + ("set_if_equals", ("k", _ENTRY, _ENTRY, 5)), ("delete_many", (["a", "b", "c"],)), ], ) diff --git a/tests/backends/test_memory.py b/tests/backends/test_memory.py index 426839b..303a6d4 100644 --- a/tests/backends/test_memory.py +++ b/tests/backends/test_memory.py @@ -946,6 +946,43 @@ async def test_memory_expire_if_equals_updates_ttl_only_when_matching( assert await memory_backend.expire_if_equals("missing", theirs, 60) is False +async def test_memory_set_if_equals_stores_only_over_a_matching_entry( + memory_backend: MemoryBackend, +): + mine = CacheEntry(fingerprint="session", content=b"v1") + theirs = CacheEntry(fingerprint="session", content=b"v2") + new = CacheEntry(fingerprint="session", content=b"v3") + await memory_backend.set("slot", theirs, 30) + + assert await memory_backend.set_if_equals("slot", mine, new, 60) is False + assert await memory_backend.get("slot") == theirs + assert await memory_backend.set_if_equals("slot", theirs, new, 60) is True + assert await memory_backend.get("slot") == new + expiry = memory_backend.cache["slot"].expiry + assert expiry is not None + assert 55 <= expiry - time.time() <= 60 + # ttl=None stores without expiry, as set() does. + assert await memory_backend.set_if_equals("slot", new, theirs) is True + assert memory_backend.cache["slot"].expiry is None + assert await memory_backend.set_if_equals("missing", theirs, new) is False + assert "missing" not in memory_backend.cache + + +async def test_memory_set_if_equals_ignores_an_expired_entry( + memory_backend: MemoryBackend, +): + entry = CacheEntry(fingerprint="session", content=b"v1") + await memory_backend.set("slot", entry, 60) + memory_backend.cache["slot"].expiry = time.time() - 1 + + assert ( + await memory_backend.set_if_equals( + "slot", entry, CacheEntry(fingerprint="session", content=b"v2"), 60 + ) + is False + ) + + async def test_memory_expire_if_equals_ignores_an_expired_entry( memory_backend: MemoryBackend, ): diff --git a/tests/backends/test_redis.py b/tests/backends/test_redis.py index 5adeec9..458b9db 100644 --- a/tests/backends/test_redis.py +++ b/tests/backends/test_redis.py @@ -1275,6 +1275,60 @@ async def get_then_overwrite(name): assert await async_redis_backend.get("slot") == theirs +@requires_redis +async def test_redis_set_if_equals_stores_only_over_a_matching_entry( + async_redis_backend: AsyncRedisCacheBackend, +) -> None: + mine = CacheEntry(fingerprint="session", content=b"v1") + theirs = CacheEntry(fingerprint="session", content=b"v2") + new = CacheEntry(fingerprint="session", content=b"v3") + await async_redis_backend.set("slot", theirs, 30) + key = async_redis_backend._make_key("slot") + + assert await async_redis_backend.set_if_equals("slot", mine, new, 60) is False + assert await async_redis_backend.get("slot") == theirs + assert await async_redis_backend.set_if_equals("slot", theirs, new, 60) is True + assert await async_redis_backend.get("slot") == new + assert 55000 <= await async_redis_backend.client.pttl(key) <= 60000 + # ttl=None stores without expiry, as set() does. + assert await async_redis_backend.set_if_equals("slot", new, theirs) is True + assert await async_redis_backend.client.pttl(key) == -1 + assert await async_redis_backend.set_if_equals("missing", theirs, new) is False + assert await async_redis_backend.get("missing") is None + + +@requires_redis +@pytest.mark.parametrize("change", ["overwrite", "delete"]) +async def test_redis_set_if_equals_keeps_a_change_made_after_the_compare( + async_redis_backend: AsyncRedisCacheBackend, change: str +) -> None: + """A write or delete between the compare and the SET is not undone.""" + mine = CacheEntry(fingerprint="session", content=b"v1") + theirs = CacheEntry(fingerprint="session", content=b"v2") + new = CacheEntry(fingerprint="session", content=b"v3") + await async_redis_backend.set("slot", mine, 60) + + client = async_redis_backend.client + original_get = client.get + + async def get_then_change(name): + raw = await original_get(name) + if change == "overwrite": + await async_redis_backend.set("slot", theirs, 60) + else: + await async_redis_backend.delete("slot") + return raw + + client.get = get_then_change # type: ignore[method-assign] + try: + assert await async_redis_backend.set_if_equals("slot", mine, new, 60) is False + finally: + client.get = original_get # type: ignore[method-assign] + + expected = theirs if change == "overwrite" else None + assert await async_redis_backend.get("slot") == expected + + @requires_redis def test_cached_route_with_ttl_zero_is_served() -> None: """End to end: `@cache(ttl=0)` used to send `SET ... EX 0` and answer 500.""" diff --git a/tests/backends/test_ttl_contract.py b/tests/backends/test_ttl_contract.py index 3ad97ad..f321861 100644 --- a/tests/backends/test_ttl_contract.py +++ b/tests/backends/test_ttl_contract.py @@ -63,11 +63,16 @@ def make_backends() -> list[BaseCacheBackend]: "set": lambda backend, ttl: backend.set("k", ENTRY, ttl=ttl), "set_if_absent": lambda backend, ttl: backend.set_if_absent("k", ENTRY, ttl=ttl), "increment": lambda backend, ttl: backend.increment("n", ttl=ttl), + "set_if_equals": lambda backend, ttl: backend.set_if_equals( + "k", ENTRY, ENTRY, ttl=ttl + ), } @pytest.mark.parametrize(("ttl", "error", "match"), BAD_TTLS) -@pytest.mark.parametrize("operation", ["set", "set_if_absent", "increment"]) +@pytest.mark.parametrize( + "operation", ["set", "set_if_absent", "increment", "set_if_equals"] +) async def test_backends_reject_invalid_ttl( operation: str, ttl: object, error: type[Exception], match: str ) -> None: diff --git a/tests/session/test_conditional_saves.py b/tests/session/test_conditional_saves.py new file mode 100644 index 0000000..fe9163c --- /dev/null +++ b/tests/session/test_conditional_saves.py @@ -0,0 +1,645 @@ +"""Ordinary session saves are conditional (#128). + +A request that loaded a session must not bring it back after another request +deleted, invalidated or rotated it, whether by saving it or by rotating its +ID. Deleting, invalidating and expiring stay unconditional, so a security +action always wins. +""" + +import logging +from collections.abc import AsyncGenerator +from typing import TYPE_CHECKING +from typing import Any + +import pytest +import pytest_asyncio +from fastapi import FastAPI +from fastapi import Request +from fastapi.testclient import TestClient + +from fastapi_cachex.backends.memory import MemoryBackend +from fastapi_cachex.session import login +from fastapi_cachex.session import rotate_session_id +from fastapi_cachex.session.config import SessionConfig +from fastapi_cachex.session.exceptions import SessionInvalidError +from fastapi_cachex.session.exceptions import SessionNotFoundError +from fastapi_cachex.session.manager import SessionManager +from fastapi_cachex.session.middleware import FastAPICacheXSessionMiddleware +from fastapi_cachex.session.models import Session +from fastapi_cachex.session.models import SessionStatus +from fastapi_cachex.session.models import SessionUser +from fastapi_cachex.session.proxy import SessionManagerProxy +from fastapi_cachex.types import CacheEntry +from tests.live_servers import MEMCACHED_SERVER +from tests.live_servers import REDIS_HOST +from tests.live_servers import REDIS_PORT +from tests.live_servers import flush_memcached +from tests.live_servers import requires_memcached +from tests.live_servers import requires_redis +from tests.live_servers import requires_redis_package + +from .test_login import _auth +from .test_login import _token_sent + +if TYPE_CHECKING: + from fastapi_cachex.backends.base import BaseCacheBackend + + +@pytest_asyncio.fixture( + params=[ + pytest.param("memory", id="memory"), + pytest.param( + "redis", id="redis", marks=[requires_redis, requires_redis_package] + ), + pytest.param("memcached", id="memcached", marks=[requires_memcached]), + ] +) +async def any_backend(request: Any) -> AsyncGenerator["BaseCacheBackend", Any]: + """A memory backend, and live Redis / Memcached ones when available.""" + if request.param == "memory": + yield MemoryBackend() + return + if request.param == "redis": + from fastapi_cachex.backends import AsyncRedisCacheBackend + + redis_backend = AsyncRedisCacheBackend( + host=REDIS_HOST, port=REDIS_PORT, key_prefix="test_conditional_saves:" + ) + await redis_backend.clear() + yield redis_backend + await redis_backend.clear() + return + from fastapi_cachex.backends import MemcachedBackend + + memcached_backend = MemcachedBackend(servers=[MEMCACHED_SERVER]) + await flush_memcached(memcached_backend) + yield memcached_backend + await flush_memcached(memcached_backend) + + +@pytest.fixture +def any_manager( + any_backend: "BaseCacheBackend", config: SessionConfig +) -> SessionManager: + return SessionManager(any_backend, config) + + +async def _two_copies(manager: SessionManager) -> tuple[Session, Session, str]: + """A stored session read by two requests, and its token.""" + _session, token = await manager.create_session(SessionUser(user_id="alice")) + first, _ = await manager.get_session(token) + second, _ = await manager.get_session(token) + return first, second, token + + +# --- SessionManager ----------------------------------------------------------- + + +async def test_a_stale_save_cannot_restore_a_deleted_session( + any_manager: SessionManager, +) -> None: + stale, _other, token = await _two_copies(any_manager) + await any_manager.delete_session(stale.session_id) + + stale.data["cart"] = ["book"] + assert await any_manager.update_session(stale) is False + + assert await any_manager._load_session(stale.session_id) is None + with pytest.raises(SessionNotFoundError): + await any_manager.get_session(token) + + +async def test_a_stale_save_cannot_reactivate_an_invalidated_session( + any_manager: SessionManager, +) -> None: + stale, other, token = await _two_copies(any_manager) + await any_manager.invalidate_session(other) + + stale.data["cart"] = ["book"] + assert await any_manager.update_session(stale) is False + + stored = await any_manager._load_session(stale.session_id) + assert stored is not None + assert stored.status == SessionStatus.INVALIDATED + assert "cart" not in stored.data + with pytest.raises(SessionInvalidError): + await any_manager.get_session(token) + + +async def test_a_stale_save_cannot_restore_the_id_before_a_rotation( + any_manager: SessionManager, +) -> None: + """After a rotation, the old token must stay dead.""" + stale, other, old_token = await _two_copies(any_manager) + old_id = stale.session_id + _rotated, new_token = await any_manager.regenerate_session_id(other) + + stale.data["cart"] = ["book"] + assert await any_manager.update_session(stale) is False + + assert await any_manager._load_session(old_id) is None + with pytest.raises(SessionNotFoundError): + await any_manager.get_session(old_token) + assert (await any_manager.get_session(new_token))[0].data == {} + + +async def test_the_first_of_two_concurrent_saves_wins( + any_manager: SessionManager, +) -> None: + """Two tabs change one session: the second save is dropped, not merged (#376).""" + first, second, _token = await _two_copies(any_manager) + + first.data["cart"] = ["book"] + assert await any_manager.update_session(first) is True + second.data["cart"] = ["pen"] + assert await any_manager.update_session(second) is False + + stored = await any_manager._load_session(first.session_id) + assert stored is not None + assert stored.data == {"cart": ["book"]} + + +async def test_saves_compare_against_the_objects_own_last_write( + any_manager: SessionManager, +) -> None: + session, _other, _token = await _two_copies(any_manager) + + for count in range(3): + session.data["count"] = count + assert await any_manager.update_session(session) is True + + stored = await any_manager._load_session(session.session_id) + assert stored is not None + assert stored.data == {"count": 2} + + +async def test_unconditional_writes_win_over_a_newer_save( + manager: SessionManager, +) -> None: + """Invalidating a stale copy still invalidates: security actions always win.""" + stale, newer, token = await _two_copies(manager) + newer.data["cart"] = ["book"] + assert await manager.update_session(newer) is True + + await manager.invalidate_session(stale) + + with pytest.raises(SessionInvalidError): + await manager.get_session(token) + + +async def test_a_session_never_read_is_saved_only_if_absent( + manager: SessionManager, +) -> None: + """A hand-built ``Session`` holds no stored entry, so it expects no record.""" + fresh = Session(data={"a": 1}) + assert await manager.update_session(fresh) is True + assert (await manager._load_session(fresh.session_id)) is not None + + existing, _token = await manager.create_session(SessionUser(user_id="alice")) + impostor = Session(session_id=existing.session_id, data={"planted": True}) + assert await manager.update_session(impostor) is False + stored = await manager._load_session(existing.session_id) + assert stored is not None + assert stored.data == {} + + +async def test_a_dropped_save_is_logged( + manager: SessionManager, caplog: pytest.LogCaptureFixture +) -> None: + stale, _other, _token = await _two_copies(manager) + await manager.delete_session(stale.session_id) + + with caplog.at_level(logging.INFO, logger="fastapi_cachex.session.manager"): + assert await manager.update_session(stale) is False + + assert any( + record.levelno == logging.INFO and "Session save dropped" in record.message + for record in caplog.records + ) + + +# --- Sliding renewal ---------------------------------------------------------- + + +@pytest.fixture +def renewing_config() -> SessionConfig: + """A config under which every lookup renews the session.""" + return SessionConfig( + secret_key="a" * 32, + cookie_name="session", + cookie_https_only=False, + sliding_threshold=1.0, + ) + + +@pytest.fixture +def renewing(backend: MemoryBackend, renewing_config: SessionConfig) -> SessionManager: + """A manager whose every lookup renews the session.""" + return SessionManager(backend, renewing_config) + + +async def test_a_save_after_a_renewal_compares_against_the_renewal( + renewing: SessionManager, +) -> None: + _session, token = await renewing.create_session(SessionUser(user_id="alice")) + session, renewed = await renewing.get_session(token) + assert renewed is not None + + session.data["cart"] = ["book"] + assert await renewing.update_session(session) is True + + +def _before_first_conditional_write( + backend: MemoryBackend, monkeypatch: pytest.MonkeyPatch, action: Any +) -> None: + """Run ``action`` once, just before the first ``set_if_equals`` compares.""" + original = backend.set_if_equals + pending = [action] + + async def racing( + key: str, expected: CacheEntry, value: CacheEntry, ttl: int | None = None + ) -> bool: + if pending: + await pending.pop()() + return await original(key, expected, value, ttl=ttl) + + monkeypatch.setattr(backend, "set_if_equals", racing) + + +async def test_a_renewal_that_loses_to_an_invalidation_is_refused( + renewing: SessionManager, + backend: MemoryBackend, + monkeypatch: pytest.MonkeyPatch, +) -> None: + session, token = await renewing.create_session(SessionUser(user_id="alice")) + + async def invalidate_elsewhere() -> None: + other = await renewing._load_session(session.session_id) + assert other is not None + await renewing.invalidate_session(other) + + _before_first_conditional_write(backend, monkeypatch, invalidate_elsewhere) + + with pytest.raises(SessionInvalidError): + await renewing.get_session(token) + stored = await renewing._load_session(session.session_id) + assert stored is not None + assert stored.status == SessionStatus.INVALIDATED + + +async def test_a_renewal_that_loses_to_a_save_reads_the_session_again( + renewing: SessionManager, + backend: MemoryBackend, + monkeypatch: pytest.MonkeyPatch, +) -> None: + session, token = await renewing.create_session(SessionUser(user_id="alice")) + + async def save_elsewhere() -> None: + other = await renewing._load_session(session.session_id) + assert other is not None + other.data["cart"] = ["book"] + assert await renewing.update_session(other) is True + + _before_first_conditional_write(backend, monkeypatch, save_elsewhere) + + loaded, _renewed = await renewing.get_session(token) + + assert loaded.data == {"cart": ["book"]} + stored = await renewing._load_session(session.session_id) + assert stored is not None + assert stored.data == {"cart": ["book"]} + + +async def test_a_renewal_that_loses_twice_is_dropped( + renewing: SessionManager, + backend: MemoryBackend, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The session was valid when read again, so the request keeps it.""" + session, token = await renewing.create_session(SessionUser(user_id="alice")) + calls = 0 + + async def always_lose(*_args: object, **_kwargs: object) -> bool: + nonlocal calls + calls += 1 + return False + + monkeypatch.setattr(backend, "set_if_equals", always_lose) + + loaded, renewed = await renewing.get_session(token) + + assert calls == 2 + assert loaded.session_id == session.session_id + assert renewed is None + + +# --- FastAPICacheXSessionMiddleware --------------------------------------------- + + +def _app(manager: SessionManager, config: SessionConfig, action: str) -> FastAPI: + app = FastAPI() + app.add_middleware( + FastAPICacheXSessionMiddleware, session_manager=manager, config=config + ) + + @app.post("/cart") + async def add_to_cart(request: Request) -> dict[str, bool]: + request.session["cart"] = ["book"] + # Another request logs the session out while this one is running. + session_id = request.session.backend.session_id # type: ignore[attr-defined] + if action == "delete": + await manager.delete_session(session_id) + elif action == "save": + other = await manager._load_session(session_id) + assert other is not None + other.data["cart"] = ["pen"] + assert await manager.update_session(other) is True + else: + other = await manager._load_session(session_id) + assert other is not None + await manager.invalidate_session(other) + return {"ok": True} + + return app + + +@pytest.mark.parametrize("action", ["delete", "invalidate"]) +@pytest.mark.parametrize("transport", ["cookie", "header"]) +async def test_the_middleware_drops_a_stale_save_and_sends_no_token( + manager: SessionManager, config: SessionConfig, action: str, transport: str +) -> None: + session, token = await manager.create_session(SessionUser(user_id="alice")) + client = TestClient(_app(manager, config, action)) + if transport == "cookie": + client.cookies.set(config.cookie_name, token) + response = client.post("/cart") + else: + response = client.post("/cart", headers=_auth(transport, config, token)) + + assert response.status_code == 200 + assert "set-cookie" not in response.headers + assert config.header_name.lower() not in response.headers + stored = await manager._load_session(session.session_id) + if action == "delete": + assert stored is None + else: + assert stored is not None + assert stored.status == SessionStatus.INVALIDATED + assert "cart" not in stored.data + + +@pytest.mark.parametrize("transport", ["cookie", "header"]) +async def test_a_dropped_save_still_sends_a_stored_renewal( + backend: MemoryBackend, transport: str +) -> None: + """The save lost to another save, but the renewal was stored: send its token. + + Without it, a JWT client would keep a token whose ``exp`` comes before the + renewed record's expiry. The session starts with a short TTL, so the + renewed JWT differs from the one the client sent. + """ + settings: dict[str, Any] = { + "secret_key": "a" * 32, + "cookie_name": "session", + "cookie_https_only": False, + "token_format": "jwt", + } + short = SessionManager(backend, SessionConfig(**settings, session_ttl=60)) + renewing_config = SessionConfig(**settings, sliding_threshold=1.0) + renewing = SessionManager(backend, renewing_config) + session, token = await short.create_session(SessionUser(user_id="alice")) + client = TestClient(_app(renewing, renewing_config, "save")) + if transport == "cookie": + client.cookies.set(renewing_config.cookie_name, token) + response = client.post("/cart") + else: + response = client.post( + "/cart", headers=_auth(transport, renewing_config, token) + ) + + assert response.status_code == 200 + sent = _token_sent(response, transport, renewing_config) + assert sent != token + loaded, _ = await renewing.get_session(sent) + assert loaded.session_id == session.session_id + assert loaded.data == {"cart": ["pen"]} + + +def test_sessions_compare_equal_whatever_entry_they_last_saw() -> None: + """``_stored`` is bookkeeping, not part of the session's value.""" + session = Session(data={"a": 1}) + loaded = Session.model_validate_json(session.model_dump_json()) + loaded._stored = CacheEntry(fingerprint="session", content=b"x") + + assert loaded == session + assert loaded != Session(data={"a": 2}) + assert session.__eq__("not a session") is NotImplemented + + +async def test_a_stored_renewal_is_not_sent_for_an_invalidated_session( + renewing: SessionManager, renewing_config: SessionConfig +) -> None: + _session, token = await renewing.create_session(SessionUser(user_id="alice")) + client = TestClient(_app(renewing, renewing_config, "invalidate")) + client.cookies.set(renewing_config.cookie_name, token) + + response = client.post("/cart") + + assert response.status_code == 200 + assert "set-cookie" not in response.headers + + +# --- ID rotation ---------------------------------------------------------------- + + +async def test_a_stale_copy_of_a_deleted_session_cannot_be_rotated( + any_manager: SessionManager, +) -> None: + stale, _other, token = await _two_copies(any_manager) + old_id = stale.session_id + await any_manager.delete_session(old_id) + + with pytest.raises(SessionNotFoundError): + await any_manager.regenerate_session_id(stale) + + assert stale.session_id == old_id + assert await any_manager._load_session(old_id) is None + with pytest.raises(SessionNotFoundError): + await any_manager.get_session(token) + + +async def test_a_stale_copy_of_an_invalidated_session_cannot_be_rotated( + any_manager: SessionManager, +) -> None: + stale, other, token = await _two_copies(any_manager) + old_id = stale.session_id + await any_manager.invalidate_session(other) + + with pytest.raises(SessionInvalidError): + await any_manager.regenerate_session_id(stale) + + assert stale.session_id == old_id + with pytest.raises(SessionNotFoundError): + await any_manager.get_session(token) + + +async def test_a_stale_copy_of_an_expired_session_cannot_be_rotated( + manager: SessionManager, +) -> None: + """``is_valid()`` covers expiry too, not only the status.""" + stale, other, _token = await _two_copies(manager) + other.expires_at = other.created_at + assert await manager.update_session(other) is True + + with pytest.raises(SessionInvalidError): + await manager.regenerate_session_id(stale) + + +async def test_only_one_of_two_concurrent_rotations_succeeds( + any_manager: SessionManager, +) -> None: + first, second, _token = await _two_copies(any_manager) + + _rotated, new_token = await any_manager.regenerate_session_id(first) + with pytest.raises(SessionNotFoundError): + await any_manager.regenerate_session_id(second) + + assert (await any_manager.get_session(new_token))[0].session_id == ( + first.session_id + ) + + +async def test_a_rotation_after_another_save_goes_ahead( + any_manager: SessionManager, +) -> None: + """The session is still valid; only its data changed.""" + stale, other, _token = await _two_copies(any_manager) + other.data["cart"] = ["book"] + assert await any_manager.update_session(other) is True + + rotated, new_token = await any_manager.regenerate_session_id(stale) + + assert (await any_manager.get_session(new_token))[0].session_id == ( + rotated.session_id + ) + + +async def test_a_session_never_read_is_rotated_without_the_check( + manager: SessionManager, +) -> None: + fresh = Session(data={"a": 1}) + + _rotated, new_token = await manager.regenerate_session_id(fresh) + + assert (await manager.get_session(new_token))[0].data == {"a": 1} + + +def _rotating_app(manager: SessionManager, config: SessionConfig) -> FastAPI: + app = FastAPI() + app.add_middleware( + FastAPICacheXSessionMiddleware, session_manager=manager, config=config + ) + + async def log_out_elsewhere(request: Request) -> None: + session_id = request.session.backend.session_id # type: ignore[attr-defined] + other = await manager._load_session(session_id) + assert other is not None + await manager.invalidate_session(other) + + @app.post("/sudo") + async def sudo(request: Request) -> dict[str, bool]: + request.session["seen"] = True + await log_out_elsewhere(request) + await rotate_session_id(request) + request.session["elevated"] = True + return {"ok": True} + + @app.post("/sudo-quiet") + async def sudo_quiet(request: Request) -> dict[str, bool]: + await log_out_elsewhere(request) + await rotate_session_id(request) + return {"ok": True} + + @app.post("/login") + async def log_in(request: Request) -> dict[str, bool]: + request.session["before"] = 1 + await log_out_elsewhere(request) + await login(request, SessionUser(user_id="alice")) + request.session["after"] = 2 + return {"ok": True} + + return app + + +async def test_rotate_session_id_refuses_a_session_logged_out_meanwhile( + manager: SessionManager, config: SessionConfig +) -> None: + SessionManagerProxy.set(manager) + session, token = await manager.create_session(SessionUser(user_id="alice")) + client = TestClient(_rotating_app(manager, config)) + client.cookies.set(config.cookie_name, token) + + response = client.post("/sudo") + + assert response.status_code == 401 + assert "set-cookie" not in response.headers + assert await manager.backend.get_all_keys() == [] + assert await manager._load_session(session.session_id) is None + + +async def test_a_refused_rotation_does_not_send_a_renewed_token( + renewing: SessionManager, renewing_config: SessionConfig +) -> None: + """The winner of a concurrent rotation may already hold the new cookie.""" + SessionManagerProxy.set(renewing) + _session, token = await renewing.create_session(SessionUser(user_id="alice")) + client = TestClient(_rotating_app(renewing, renewing_config)) + client.cookies.set(renewing_config.cookie_name, token) + + response = client.post("/sudo-quiet") + + assert response.status_code == 401 + assert "set-cookie" not in response.headers + + +@pytest.mark.parametrize("transport", ["cookie", "header"]) +async def test_login_starts_a_new_session_when_the_loaded_one_ended_meanwhile( + manager: SessionManager, config: SessionConfig, transport: str +) -> None: + session, token = await manager.create_anonymous_session(cart=["book"]) + client = TestClient(_rotating_app(manager, config)) + if transport == "cookie": + client.cookies.set(config.cookie_name, token) + response = client.post("/login") + else: + response = client.post("/login", headers=_auth(transport, config, token)) + + assert response.status_code == 200 + logged_in, _ = await manager.get_session(_token_sent(response, transport, config)) + assert logged_in.session_id != session.session_id + assert logged_in.user is not None + assert logged_in.user.user_id == "alice" + assert logged_in.data == {"after": 2} + assert await manager.backend.get_all_keys() == [ + manager._get_backend_key(logged_in.session_id) + ] + + +async def test_rotate_session_id_refuses_an_ended_session_without_the_middleware( + manager: SessionManager, +) -> None: + """A session put on ``request.state`` by hand is refused the same way.""" + SessionManagerProxy.set(manager) + stale, _other, _token = await _two_copies(manager) + await manager.delete_session(stale.session_id) + app = FastAPI() + setattr(app.state, "__fastapi_cachex_session_manager", manager) + + @app.post("/sudo") + async def sudo(request: Request) -> dict[str, bool]: + setattr(request.state, "__fastapi_cachex_session", stale) + return {"rotated": await rotate_session_id(request)} + + response = TestClient(app).post("/sudo") + + assert response.status_code == 401 diff --git a/tests/session/test_jwt.py b/tests/session/test_jwt.py index 1c22372..348c058 100644 --- a/tests/session/test_jwt.py +++ b/tests/session/test_jwt.py @@ -119,7 +119,7 @@ async def test_jwt_sliding_renewal_returns_new_token_with_updated_exp() -> None: # Shorten expires_at so time_remaining < 50% of 3600 s → triggers renewal shortened_expiry = datetime.now(timezone.utc) + timedelta(seconds=1000) created.expires_at = shortened_expiry - await manager._save_session(created) + await manager._save_session(created, conditional=False) renewed_session, renewed_token = await manager.get_session(original_token) diff --git a/tests/session/test_lookup_writes.py b/tests/session/test_lookup_writes.py index 57fad9d..55cf176 100644 --- a/tests/session/test_lookup_writes.py +++ b/tests/session/test_lookup_writes.py @@ -24,7 +24,7 @@ class SpyBackend(MemoryBackend): - """A memory backend that records every key passed to ``set``.""" + """A memory backend that records every key passed to ``set`` or ``set_if_equals``.""" def __init__(self) -> None: super().__init__() @@ -34,6 +34,16 @@ async def set(self, key: str, value: CacheEntry, ttl: int | None = None) -> None self.writes.append(key) await super().set(key, value, ttl=ttl) + async def set_if_equals( + self, + key: str, + expected: CacheEntry, + value: CacheEntry, + ttl: int | None = None, + ) -> bool: + self.writes.append(key) + return await super().set_if_equals(key, expected, value, ttl=ttl) + def _manager(**overrides: object) -> tuple[SessionManager, SpyBackend]: backend = SpyBackend() diff --git a/tests/session/test_manager.py b/tests/session/test_manager.py index 3cfcab2..9c400fd 100644 --- a/tests/session/test_manager.py +++ b/tests/session/test_manager.py @@ -294,7 +294,7 @@ async def test_save_session_with_expired_session( session.expires_at = datetime.now(timezone.utc) - timedelta(seconds=10) # Save the expired session - await manager._save_session(session) + await manager._save_session(session, conditional=False) # Try to retrieve - should fail with SessionExpiredError with pytest.raises(SessionExpiredError): diff --git a/tests/session/test_starlette_middleware.py b/tests/session/test_starlette_middleware.py index f3f41dc..8c39992 100644 --- a/tests/session/test_starlette_middleware.py +++ b/tests/session/test_starlette_middleware.py @@ -300,7 +300,7 @@ async def noop_route() -> dict[str, bool]: # Shorten expiry so time_remaining < sliding threshold (< 50% of session_ttl) shortened_expiry = datetime.now(timezone.utc) + timedelta(seconds=100) session.expires_at = shortened_expiry - await manager._save_session(session) + await manager._save_session(session, conditional=False) client = TestClient(app) client.cookies.set(config.cookie_name, token) @@ -391,7 +391,7 @@ async def test_route(request: Request) -> dict[str, bool]: user = SessionUser(user_id="test-user") session, token = await manager.create_session(user=user) session.expires_at = datetime.now(timezone.utc) - timedelta(seconds=10) - await manager._save_session(session) + await manager._save_session(session, conditional=False) client = TestClient(app) client.cookies.set(config.cookie_name, token) @@ -651,7 +651,7 @@ async def noop_route() -> dict[str, bool]: # Shorten expiry so time_remaining < sliding threshold (< 50% of session_ttl). shortened_expiry = datetime.now(timezone.utc) + timedelta(seconds=100) session.expires_at = shortened_expiry - await manager._save_session(session) + await manager._save_session(session, conditional=False) client = TestClient(app) response = client.get("/noop", headers={config.header_name: token}) @@ -783,7 +783,7 @@ async def _shorten_expiry(manager: SessionManager, token: str) -> None: """Push the session under the sliding threshold so the load renews it.""" session, _ = await manager.get_session(token) session.expires_at = datetime.now(timezone.utc) + timedelta(seconds=100) - await manager._save_session(session) + await manager._save_session(session, conditional=False) @pytest.mark.parametrize("write_data", [True, False])