mirror of
https://github.com/onyx-dot-app/onyx.git
synced 2026-10-23 11:53:08 -05:00
## Description
A password reset left every existing session of the user valid, and an
admin could reset their own password through the admin endpoint and stay
logged in (from an external security report).
- **Admin reset** (`POST /api/password/reset_password`) signs out all
sessions of the target user. Web cookie and mobile bearer sessions share
one token store, so both are covered.
- **Forgot-password email reset** signs out all sessions.
- **Change password** signs out the user's other sessions and keeps the
caller's session.
- **Self-reset** through the admin endpoint returns 400 and points to
the change-password flow.
- Route errors move to `OnyxError` (same status codes, same `detail`
field the frontend reads). The broad `except Exception` that returned
`str(e)` in a 500 is gone; a wrong current password is now a 400, not a
500.
Coverage per `AUTH_BACKEND`:
- **redis** (default; cloud and standard self-hosted):
- `write_token` / `refresh_token` add the token to a per-user set
(`fastapi_users_user_sessions:<user_id>`) with the session TTL.
`destroy_token` removes it.
- Revocation tombstones each indexed token, so rejected requests show as
`SESSION_TERMINATED`. There is no keyspace scan.
- Sessions written before this deploy join the set on their next
refresh. Until then they are not revoked (they expire within
`SESSION_EXPIRE_TIME_SECONDS`).
- **postgres**: delete the user's `AccessToken` rows, except the kept
token.
- **jwt** (opt-in, single-tenant only): stateless, so not revoked. This
is the same as logout today; it logs a warning.
Not in scope:
- Admins can still reset other users, including other admins.
- The one-time password is still shown to the admin.
- Forcing a password change at next login is a possible follow-up.
## How Has This Been Tested?
- `tests/unit/onyx/auth` + route contract tests: 500 passed.
`test_on_after_reset_password_emits_event` now also asserts revocation.
- New `tests/external_dependency_unit/auth/test_session_revocation.py`
(6 tests):
- Redis: other sessions revoked, kept session stays, another user's
sessions untouched.
- The index stays right across logout and refresh.
- Admin reset revokes all.
- A wrong old password revokes nothing.
- Postgres: keep one session, then revoke all.
- Not run locally (no local Postgres/Redis); CI runs them.
- New integration test
`tests/integration/tests/security/test_session_revocation_on_reset.py`:
- Admin reset: both old cookies are rejected and the new password logs
in.
- Self-reset returns 400.
- Change-password keeps the current session and rejects the other one.
- Not run locally (shared stack); CI runs it.
- Redis strategy flow checked against an in-memory fake (write, logout,
keep, revoke all, refresh). pre-commit passes.
## Additional Options
- [ ] [Optional] Please cherry-pick this PR to the latest release
version.
- [x] [Optional] Override Linear Check
🤖 Generated with [Claude Code](https://claude.com/claude-code)
<!-- This is an auto-generated description by cubic. -->
---
## Summary by cubic
Closes a security gap where a password reset or change left existing
sessions valid, and an admin could reset their own password through the
admin endpoint and stay logged in.
Now the admin reset (`POST /password/reset_password`) signs out all
sessions of the target user — web cookie and mobile bearer sessions
share one token store, so both are covered. The forgot-password email
reset signs out all sessions, and changing a password signs out the
user's other sessions while keeping the caller's session. An admin
resetting their own password via the admin endpoint gets a 400 pointing
to the change-password flow. Route errors now use `OnyxError` with the
same status codes and `detail` field, and a wrong current password
returns 400 instead of the previous 500. A refresh can no longer revive
a revoked or logged-out session: a failed refresh returns
`SESSION_TERMINATED` instead of the old logout/login fallback, which
could mint a new session after a concurrent revoke.
**Revocation coverage per `AUTH_BACKEND`:**
- **redis** (default): tokens are indexed in a per-user set; revocation
tombstones each indexed token so rejected requests show as
`SESSION_TERMINATED`, with no keyspace scan. Refresh extends under
WATCH/MULTI, so a tombstone landing between read and write fails the
extend and the session stays revoked. Sessions written before this
deploy join the index on their next refresh.
- **postgres**: deletes the user's `AccessToken` rows except the kept
token; refresh on a deleted row raises `SESSION_TERMINATED` instead of
minting a new token.
- **jwt** (opt-in): stateless, so tokens stay valid until expiry — the
same as logout today — and it logs a warning.
<sup>Written for commit 43a5f7afb7.
Summary will update on new commits.</sup>
<a
href="https://cubic.dev/pr/onyx-dot-app/onyx/pull/16049?utm_source=github"
target="_blank" rel="noopener noreferrer"
data-no-image-dialog="true"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source
media="(prefers-color-scheme: light)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img
alt="View guided diff"
src="https://www.cubic.dev/buttons/review-in-cubic-light.svg"></picture></a>
<a
href="https://www.cubic.dev/action/auto-fix/pr/onyx-dot-app/onyx/16049?returnTo=https%3A%2F%2Fgithub.com%2Fonyx-dot-app%2Fonyx%2Fpull%2F16049&source=description"
target="_blank" rel="noopener noreferrer"
data-no-image-dialog="true"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://www.cubic.dev/buttons/turn-on-auto-fix-dark.svg"><source
media="(prefers-color-scheme: light)"
srcset="https://www.cubic.dev/buttons/turn-on-auto-fix-light.svg"><img
alt="Turn on auto-fix"
src="https://www.cubic.dev/buttons/turn-on-auto-fix-light.svg"></picture></a>
<!-- End of auto-generated description by cubic. -->
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Security**
* Changing your password now signs out your other active sessions while
keeping your current session active.
* An admin password reset signs the affected user out of all sessions.
Admins cannot reset their own password through this action.
* If your current password is incorrect, the change is rejected and your
sessions remain active.
* Revoked or expired sessions can no longer be restored by refreshing
them.
* With JWT-based sessions, existing sessions may remain valid until
their tokens expire.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
340 lines
12 KiB
Python
340 lines
12 KiB
Python
"""
|
|
Revoking a user's sessions after a password reset or change. Real Redis +
|
|
Postgres; the strategy and UserManager are called directly.
|
|
"""
|
|
|
|
import secrets
|
|
from collections.abc import Generator
|
|
from typing import cast
|
|
|
|
import pytest
|
|
from fastapi_users.password import PasswordHelper
|
|
from fastapi_users_db_sqlalchemy import SQLAlchemyUserDatabase
|
|
from sqlalchemy import select
|
|
from sqlalchemy.orm import Session
|
|
|
|
from onyx.auth.schemas import AuthBackend
|
|
from onyx.auth.session_tokens import (
|
|
SESSION_TOKEN_GRACE_PERIOD_SECONDS,
|
|
SessionRejection,
|
|
SessionRejectionReason,
|
|
SessionTokenValue,
|
|
build_session_tombstone_value,
|
|
classify_session_token_value,
|
|
get_session_rejection,
|
|
)
|
|
from onyx.auth.users import TenantAwareRedisStrategy, UserManager
|
|
from onyx.configs.app_configs import (
|
|
REDIS_AUTH_KEY_PREFIX,
|
|
REDIS_USER_SESSIONS_KEY_PREFIX,
|
|
)
|
|
from onyx.db.engine.async_sql_engine import get_async_session_context_manager
|
|
from onyx.db.models import AccessToken, OAuthAccount, User
|
|
from onyx.error_handling.exceptions import OnyxError
|
|
from onyx.redis.redis_pool import get_raw_redis_client
|
|
from tests.external_dependency_unit.conftest import create_test_user
|
|
|
|
_OLD_PASSWORD = "Old-Passw0rd-For-Tests!"
|
|
_NEW_PASSWORD = "New-Passw0rd-For-Tests!"
|
|
|
|
|
|
@pytest.fixture
|
|
def written_tokens() -> Generator[list[str], None, None]:
|
|
tokens: list[str] = []
|
|
yield tokens
|
|
redis = get_raw_redis_client()
|
|
for token in tokens:
|
|
redis.delete(REDIS_AUTH_KEY_PREFIX + token)
|
|
|
|
|
|
def _cleanup_index(user: User) -> None:
|
|
get_raw_redis_client().delete(f"{REDIS_USER_SESSIONS_KEY_PREFIX}{user.id}")
|
|
|
|
|
|
def _indexed_tokens(user: User) -> set[str]:
|
|
members = cast(
|
|
set[bytes],
|
|
get_raw_redis_client().smembers(f"{REDIS_USER_SESSIONS_KEY_PREFIX}{user.id}"),
|
|
)
|
|
return {m.decode() for m in members}
|
|
|
|
|
|
async def _read_user_id(token: str) -> str | None:
|
|
async with get_async_session_context_manager() as async_session:
|
|
user_db = SQLAlchemyUserDatabase(async_session, User, OAuthAccount)
|
|
user = await TenantAwareRedisStrategy().read_token(token, UserManager(user_db))
|
|
return str(user.id) if user else None
|
|
|
|
|
|
def _set_password(db_session: Session, user: User, password: str) -> None:
|
|
user.hashed_password = PasswordHelper().hash(password)
|
|
db_session.commit()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
@pytest.mark.usefixtures("tenant_context")
|
|
async def test_redis_revoke_keeps_only_the_kept_session(
|
|
db_session: Session, written_tokens: list[str]
|
|
) -> None:
|
|
user = create_test_user(db_session, "revoke_keep")
|
|
other_user = create_test_user(db_session, "revoke_bystander")
|
|
strategy = TenantAwareRedisStrategy()
|
|
try:
|
|
kept, revoked_a, revoked_b, bystander = (
|
|
await strategy.write_token(user),
|
|
await strategy.write_token(user),
|
|
await strategy.write_token(user),
|
|
await strategy.write_token(other_user),
|
|
)
|
|
written_tokens.extend([kept, revoked_a, revoked_b, bystander])
|
|
assert _indexed_tokens(user) == {kept, revoked_a, revoked_b}
|
|
|
|
await strategy.revoke_user_sessions(str(user.id), keep_token=kept)
|
|
|
|
assert await _read_user_id(kept) == str(user.id)
|
|
assert await _read_user_id(bystander) == str(other_user.id)
|
|
for token in (revoked_a, revoked_b):
|
|
assert await _read_user_id(token) is None
|
|
rejection = get_session_rejection()
|
|
assert rejection is not None
|
|
assert rejection.reason == SessionRejectionReason.TERMINATED
|
|
assert _indexed_tokens(user) == {kept}
|
|
finally:
|
|
_cleanup_index(user)
|
|
_cleanup_index(other_user)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
@pytest.mark.usefixtures("tenant_context")
|
|
async def test_redis_logout_and_refresh_maintain_index(
|
|
db_session: Session, written_tokens: list[str]
|
|
) -> None:
|
|
user = create_test_user(db_session, "revoke_index")
|
|
strategy = TenantAwareRedisStrategy()
|
|
try:
|
|
logged_out = await strategy.write_token(user)
|
|
refreshed = await strategy.write_token(user)
|
|
written_tokens.extend([logged_out, refreshed])
|
|
|
|
await strategy.destroy_token(logged_out, user)
|
|
# A session written before the index existed joins it on refresh.
|
|
get_raw_redis_client().srem(
|
|
f"{REDIS_USER_SESSIONS_KEY_PREFIX}{user.id}", refreshed
|
|
)
|
|
assert await strategy.refresh_token(refreshed, user) == refreshed
|
|
|
|
assert _indexed_tokens(user) == {refreshed}
|
|
finally:
|
|
_cleanup_index(user)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
@pytest.mark.usefixtures("tenant_context")
|
|
async def test_index_prunes_expired_sessions_on_write(
|
|
db_session: Session, written_tokens: list[str]
|
|
) -> None:
|
|
user = create_test_user(db_session, "revoke_prune")
|
|
strategy = TenantAwareRedisStrategy()
|
|
try:
|
|
expired = await strategy.write_token(user)
|
|
written_tokens.append(expired)
|
|
get_raw_redis_client().delete(REDIS_AUTH_KEY_PREFIX + expired)
|
|
|
|
live = await strategy.write_token(user)
|
|
written_tokens.append(live)
|
|
|
|
assert _indexed_tokens(user) == {live}
|
|
finally:
|
|
_cleanup_index(user)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
@pytest.mark.usefixtures("tenant_context")
|
|
async def test_refresh_after_revoke_does_not_revive_session(
|
|
db_session: Session, written_tokens: list[str]
|
|
) -> None:
|
|
user = create_test_user(db_session, "revoke_then_refresh")
|
|
strategy = TenantAwareRedisStrategy()
|
|
try:
|
|
token = await strategy.write_token(user)
|
|
written_tokens.append(token)
|
|
# The refresh route authenticated the request while the session was live.
|
|
assert await _read_user_id(token) == str(user.id)
|
|
|
|
await strategy.revoke_user_sessions(str(user.id))
|
|
|
|
with pytest.raises(OnyxError):
|
|
await strategy.refresh_token(token, user)
|
|
assert await _read_user_id(token) is None
|
|
assert _indexed_tokens(user) == set()
|
|
finally:
|
|
_cleanup_index(user)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
@pytest.mark.usefixtures("tenant_context")
|
|
async def test_revoke_between_refresh_read_and_write_wins(
|
|
db_session: Session,
|
|
written_tokens: list[str],
|
|
monkeypatch: pytest.MonkeyPatch,
|
|
) -> None:
|
|
user = create_test_user(db_session, "revoke_race")
|
|
strategy = TenantAwareRedisStrategy()
|
|
try:
|
|
token = await strategy.write_token(user)
|
|
written_tokens.append(token)
|
|
|
|
calls = 0
|
|
|
|
def classify_then_revoke(
|
|
raw_value: str | bytes | None,
|
|
) -> SessionTokenValue | SessionRejection:
|
|
# Refresh has read the live value; a revoke lands before it writes.
|
|
nonlocal calls
|
|
calls += 1
|
|
result = classify_session_token_value(raw_value)
|
|
if calls == 1:
|
|
get_raw_redis_client().set(
|
|
REDIS_AUTH_KEY_PREFIX + token,
|
|
build_session_tombstone_value(raw_value, str(user.id)),
|
|
ex=SESSION_TOKEN_GRACE_PERIOD_SECONDS,
|
|
)
|
|
return result
|
|
|
|
monkeypatch.setattr(
|
|
"onyx.auth.users.classify_session_token_value", classify_then_revoke
|
|
)
|
|
|
|
with pytest.raises(OnyxError):
|
|
await strategy.refresh_token(token, user)
|
|
monkeypatch.undo()
|
|
|
|
assert calls == 2
|
|
assert await _read_user_id(token) is None
|
|
rejection = get_session_rejection()
|
|
assert rejection is not None
|
|
assert rejection.reason == SessionRejectionReason.TERMINATED
|
|
finally:
|
|
_cleanup_index(user)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
@pytest.mark.usefixtures("tenant_context")
|
|
async def test_admin_reset_revokes_every_redis_session(
|
|
db_session: Session, written_tokens: list[str]
|
|
) -> None:
|
|
user = create_test_user(db_session, "revoke_admin_reset")
|
|
strategy = TenantAwareRedisStrategy()
|
|
try:
|
|
web_token = await strategy.write_token(user)
|
|
mobile_token = await strategy.write_token(user)
|
|
written_tokens.extend([web_token, mobile_token])
|
|
|
|
async with get_async_session_context_manager() as async_session:
|
|
user_db = SQLAlchemyUserDatabase(async_session, User, OAuthAccount)
|
|
await UserManager(user_db).reset_password_as_admin(user.id)
|
|
|
|
assert await _read_user_id(web_token) is None
|
|
assert await _read_user_id(mobile_token) is None
|
|
assert _indexed_tokens(user) == set()
|
|
finally:
|
|
_cleanup_index(user)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
@pytest.mark.usefixtures("tenant_context")
|
|
async def test_change_password_keeps_current_redis_session(
|
|
db_session: Session, written_tokens: list[str]
|
|
) -> None:
|
|
user = create_test_user(db_session, "revoke_change_redis")
|
|
_set_password(db_session, user, _OLD_PASSWORD)
|
|
strategy = TenantAwareRedisStrategy()
|
|
try:
|
|
current = await strategy.write_token(user)
|
|
other = await strategy.write_token(user)
|
|
written_tokens.extend([current, other])
|
|
|
|
async with get_async_session_context_manager() as async_session:
|
|
user_db = SQLAlchemyUserDatabase(async_session, User, OAuthAccount)
|
|
manager = UserManager(user_db)
|
|
db_user = await manager.get(user.id)
|
|
await manager.change_password_if_old_matches(
|
|
db_user, _OLD_PASSWORD, _NEW_PASSWORD, keep_token=current
|
|
)
|
|
|
|
assert await _read_user_id(current) == str(user.id)
|
|
assert await _read_user_id(other) is None
|
|
finally:
|
|
_cleanup_index(user)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
@pytest.mark.usefixtures("tenant_context")
|
|
async def test_change_password_wrong_old_password_revokes_nothing(
|
|
db_session: Session, written_tokens: list[str]
|
|
) -> None:
|
|
user = create_test_user(db_session, "revoke_wrong_old")
|
|
_set_password(db_session, user, _OLD_PASSWORD)
|
|
strategy = TenantAwareRedisStrategy()
|
|
try:
|
|
token = await strategy.write_token(user)
|
|
written_tokens.append(token)
|
|
|
|
async with get_async_session_context_manager() as async_session:
|
|
user_db = SQLAlchemyUserDatabase(async_session, User, OAuthAccount)
|
|
manager = UserManager(user_db)
|
|
db_user = await manager.get(user.id)
|
|
with pytest.raises(OnyxError):
|
|
await manager.change_password_if_old_matches(
|
|
db_user, "not-the-old-password", _NEW_PASSWORD
|
|
)
|
|
|
|
assert await _read_user_id(token) == str(user.id)
|
|
finally:
|
|
_cleanup_index(user)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
@pytest.mark.usefixtures("tenant_context")
|
|
async def test_postgres_backend_deletes_other_access_tokens(
|
|
db_session: Session, monkeypatch: pytest.MonkeyPatch
|
|
) -> None:
|
|
monkeypatch.setattr("onyx.auth.users.AUTH_BACKEND", AuthBackend.POSTGRES)
|
|
user = create_test_user(db_session, "revoke_change_pg")
|
|
other_user = create_test_user(db_session, "revoke_bystander_pg")
|
|
_set_password(db_session, user, _OLD_PASSWORD)
|
|
current, other, bystander = (secrets.token_urlsafe() for _ in range(3))
|
|
db_session.add_all(
|
|
[
|
|
AccessToken(token=current, user_id=user.id),
|
|
AccessToken(token=other, user_id=user.id),
|
|
AccessToken(token=bystander, user_id=other_user.id),
|
|
]
|
|
)
|
|
db_session.commit()
|
|
|
|
async with get_async_session_context_manager() as async_session:
|
|
user_db = SQLAlchemyUserDatabase(async_session, User, OAuthAccount)
|
|
manager = UserManager(user_db)
|
|
db_user = await manager.get(user.id)
|
|
await manager.change_password_if_old_matches(
|
|
db_user, _OLD_PASSWORD, _NEW_PASSWORD, keep_token=current
|
|
)
|
|
|
|
def _remaining() -> set[str]:
|
|
db_session.expire_all()
|
|
rows = db_session.scalars(
|
|
select(AccessToken).where(
|
|
AccessToken.user_id.in_([user.id, other_user.id]) # ty: ignore[unresolved-attribute]
|
|
)
|
|
)
|
|
return {row.token for row in rows}
|
|
|
|
assert _remaining() == {current, bystander}
|
|
|
|
async with get_async_session_context_manager() as async_session:
|
|
user_db = SQLAlchemyUserDatabase(async_session, User, OAuthAccount)
|
|
await UserManager(user_db).reset_password_as_admin(user.id)
|
|
|
|
assert _remaining() == {bystander}
|