Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 25 additions & 7 deletions backend/app/repositories/session.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
"""Session repository (PostgreSQL async)."""

from datetime import UTC, datetime
from typing import Any
from uuid import UUID

from sqlalchemy import func, select, update
from sqlalchemy import Select, func, select, update
from sqlalchemy.ext.asyncio import AsyncSession

from app.db.models.session import Session
Expand All @@ -25,22 +26,39 @@ async def get_by_refresh_token_hash(db: AsyncSession, token_hash: str) -> Sessio
return result.scalar_one_or_none()


def _open(query: Select[tuple[Any]]) -> Select[tuple[Any]]:
"""Narrow a session query to the ones actually still usable.

`is_active` alone is not that. A session is deactivated when somebody signs
out or an administrator revokes it, but nothing sweeps the ones that simply
ran out: the row stays `is_active` until the next refresh finds it expired
and declines it. So a query on `is_active` counts sessions that cannot be
used again, which is how the admin drawer reported open sessions for an
account whose every session had lapsed (#1256).
"""
return query.where(Session.is_active.is_(True), Session.expires_at > datetime.now(UTC))
Comment thread
DEENUU1 marked this conversation as resolved.
Outdated


async def get_user_sessions(
db: AsyncSession,
user_id: UUID,
*,
active_only: bool = True,
open_only: bool = True,
skip: int = 0,
limit: int | None = None,
) -> list[Session]:
"""Get sessions for a user, most recently used first.

`limit` of None returns every session, which is what the callers that
revoke or validate one need. The listing route passes a page.

`open_only=False` is the whole history, which is what answers "when were
they last here": somebody who has signed out has no open session and has
very much been here.
"""
query = select(Session).where(Session.user_id == user_id)
if active_only:
query = query.where(Session.is_active.is_(True))
if open_only:
query = _open(query)
# `last_used_at` alone is not a total order - a user who signs in twice in
# the same request cycle gets two rows with the same timestamp, and an
# unstable order means a row can appear on two pages or on neither. `id`
Expand All @@ -56,12 +74,12 @@ async def count_user_sessions(
db: AsyncSession,
user_id: UUID,
*,
active_only: bool = True,
open_only: bool = True,
) -> int:
"""How many sessions the user has, for the page count."""
query = select(func.count(Session.id)).where(Session.user_id == user_id)
if active_only:
query = query.where(Session.is_active.is_(True))
if open_only:
query = _open(query)
return (await db.execute(query)).scalar_one()


Expand Down
6 changes: 3 additions & 3 deletions backend/app/services/session.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,10 @@ async def create_session(
)

async def get_user_sessions(self, user_id: UUID) -> list[Session]:
return await session_repo.get_user_sessions(self.db, user_id, active_only=True)
return await session_repo.get_user_sessions(self.db, user_id, open_only=True)

async def count_user_sessions(self, user_id: UUID) -> int:
return await session_repo.count_user_sessions(self.db, user_id, active_only=True)
return await session_repo.count_user_sessions(self.db, user_id, open_only=True)

async def validate_refresh_token(self, refresh_token: str) -> Session | None:
token_hash = _hash_token(refresh_token)
Expand Down Expand Up @@ -114,7 +114,7 @@ async def list_sessions(
the client able to work out that the page is gone.
"""
sessions = await session_repo.get_user_sessions(
self.db, user_id, active_only=True, skip=skip, limit=limit
self.db, user_id, open_only=True, skip=skip, limit=limit
)
total = await self.count_user_sessions(user_id)
return SessionListResponse(
Expand Down
35 changes: 25 additions & 10 deletions backend/app/services/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,17 +110,31 @@ async def has_any(self) -> bool:
async def admin_detail(self, user_id: UUID) -> AdminUserDetail:
"""Where this person has access, when they were last here, what is open.

Three reads rather than one because they are three tables, and they are
here rather than in the drawer because a client assembling them would
make three round trips to answer one question - and would have to know
that "no sessions ever" and "no sessions now" are different answers.
Four reads rather than one because they are three tables and two
questions, and they are here rather than in the drawer because a client
assembling them would make as many round trips to answer one question -
and would have to know that "no sessions ever" and "no sessions now" are
different answers.

**Two scopes, not one.** Where somebody was last seen is a fact about
every session they have ever had; how many are open is a fact about the
ones still usable. Reading both off the open set answered "Never signed
in" for anybody who had signed out - which is most accounts most of the
time, and the opposite of the truth on the field this drawer exists for
(#1256).

The user is fetched first so an unknown id is a 404 rather than an empty
detail about nobody.
"""
await self.get_by_id(user_id)
memberships = await organization_repo.list_for_user(self.db, user_id)
sessions = await session_repo.get_user_sessions(self.db, user_id, active_only=True)
# `limit=1` because the query is ordered most-recently-used first and the
# head is the whole answer. Nothing prunes this table - every refresh
# deactivates a row and inserts another - so a year-old account has
# thousands of rows and reading them all to look at one grows without
# bound.
last_used = await session_repo.get_user_sessions(self.db, user_id, open_only=False, limit=1)
Comment thread
DEENUU1 marked this conversation as resolved.
open_sessions = await session_repo.get_user_sessions(self.db, user_id, open_only=True)
return AdminUserDetail(
memberships=[
AdminUserMembership(
Expand All @@ -132,11 +146,12 @@ async def admin_detail(self, user_id: UUID) -> AdminUserDetail:
)
for organization, role in memberships
],
# The sessions come back most-recently-used first, so the head is
# both answers: when they were last here, and the newest one open.
last_seen_at=sessions[0].last_used_at if sessions else None,
active_sessions=len(sessions),
newest_session_at=max((s.created_at for s in sessions), default=None),
last_seen_at=last_used[0].last_used_at if last_used else None,
active_sessions=len(open_sessions),
# Of the open ones: it is read beside their count, and "newest
# session August" under "0 open sessions" is a sentence about
# nothing.
newest_session_at=max((s.created_at for s in open_sessions), default=None),
)

async def admin_list_with_counts(
Expand Down
19 changes: 19 additions & 0 deletions backend/tests/api/test_admin_user_detail.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,25 @@ async def test_last_seen_is_the_newest_activity_and_the_count_is_what_is_open()
assert body["newest_session_at"].startswith("2026-08-19T09:00")


async def test_the_two_figures_are_two_scopes_and_the_history_read_is_bounded() -> None:
"""Last-seen is every session; open is the usable ones. And the history read
takes one row: nothing prunes `sessions` - every refresh deactivates a row
and inserts another - so reading a year of them to look at the head grows
without bound."""
service = UserService(AsyncMock())
reads = AsyncMock(return_value=[_session(last_used=NOW, created=NOW)])
with (
patch.object(service, "get_by_id", new=AsyncMock()),
patch(f"{MODULE}.organization_repo.list_for_user", new=AsyncMock(return_value=[])),
patch(f"{MODULE}.session_repo.get_user_sessions", new=reads),
):
async with _client(service=service) as client:
await client.get(ENDPOINT)

scopes = [call.kwargs for call in reads.await_args_list]
assert scopes == [{"open_only": False, "limit": 1}, {"open_only": True}]


async def test_an_account_that_has_never_signed_in_says_so_rather_than_nothing() -> None:
"""`null` is not zero. A dormant account and one that was created and never
used are different decisions, and the drawer has to be able to tell them
Expand Down
119 changes: 119 additions & 0 deletions backend/tests/integration/test_admin_user_last_seen.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
"""When the admin drawer says somebody was last here, and what it calls open.

Both figures used to come off the same read - the user's *active* sessions - and
both were wrong for it. Somebody who has signed out has no active session row at
all, so "when were they last here" came back null and the drawer answered "Never
signed in" for most accounts most of the time: the one case the field exists to
tell apart from an account created and never used. And nothing sweeps a session
that simply ran out, so a row past `expires_at` stays `is_active` until the next
refresh declines it and was counted as open (#1256).

Here rather than in the unit suite because both halves are a `WHERE` clause: what
`is_active AND expires_at > now()` selects is Postgres's answer, not a mock's.
"""

from __future__ import annotations

import uuid
from datetime import UTC, datetime, timedelta

import pytest

from app.db.models.session import Session
from app.db.models.user import User
from app.services.user import UserService

pytestmark = pytest.mark.anyio

NOW = datetime(2026, 8, 20, 9, 0, tzinfo=UTC)


async def _user(db) -> User:
user = User(
id=uuid.uuid4(),
email=f"{uuid.uuid4().hex}@example.com",
hashed_password="x",
is_active=True,
created_at=NOW - timedelta(days=90),
)
db.add(user)
await db.flush()
return user


async def _session(
db,
user: User,
*,
last_used_at: datetime,
is_active: bool = True,
expires_at: datetime | None = None,
) -> Session:
session = Session(
id=uuid.uuid4(),
user_id=user.id,
refresh_token_hash=uuid.uuid4().hex,
is_active=is_active,
created_at=last_used_at,
last_used_at=last_used_at,
expires_at=expires_at or datetime.now(UTC) + timedelta(days=7),
)
db.add(session)
await db.flush()
return session


class TestLastSeenIsEverySession:
async def test_somebody_who_signed_out_has_still_been_here(self, db) -> None:
user = await _user(db)
await _session(db, user, last_used_at=NOW - timedelta(days=30), is_active=False)
last = NOW - timedelta(days=3)
await _session(db, user, last_used_at=last, is_active=False)

detail = await UserService(db).admin_detail(user.id)

assert detail.last_seen_at == last
assert detail.active_sessions == 0
# No open session, so nothing to date - the drawer reads this beside the
# count and "newest session August" under "0 open sessions" says nothing.
assert detail.newest_session_at is None

async def test_an_account_that_never_signed_in_is_still_null(self, db) -> None:
"""The distinction the field exists for, and the half that was already
right: no session ever is not the same as none open now."""
user = await _user(db)

detail = await UserService(db).admin_detail(user.id)

assert detail.last_seen_at is None
assert detail.active_sessions == 0


class TestOpenMeansUsable:
async def test_an_expired_row_is_not_an_open_session(self, db) -> None:
"""Nothing deactivates a session that lapses: the row stays `is_active`
until a refresh finds it expired and declines it."""
user = await _user(db)
await _session(
db,
user,
last_used_at=NOW - timedelta(days=40),
expires_at=datetime.now(UTC) - timedelta(days=1),
)

detail = await UserService(db).admin_detail(user.id)

assert detail.active_sessions == 0
assert detail.last_seen_at == NOW - timedelta(days=40)

async def test_an_unexpired_row_is(self, db) -> None:
user = await _user(db)
await _session(db, user, last_used_at=NOW - timedelta(days=40), is_active=False)
opened = NOW - timedelta(hours=2)
await _session(db, user, last_used_at=opened)

detail = await UserService(db).admin_detail(user.id)

assert detail.active_sessions == 1
assert detail.newest_session_at == opened
assert detail.last_seen_at == opened
2 changes: 1 addition & 1 deletion backend/tests/test_sessions_pagination.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ async def test_a_page_reports_the_total_not_its_own_length(service: SessionServi

assert len(result.items) == 2
assert result.total == 17
assert fetch.await_args.kwargs == {"active_only": True, "skip": 10, "limit": 2}
assert fetch.await_args.kwargs == {"open_only": True, "skip": 10, "limit": 2}


async def test_the_page_is_what_the_caller_asked_for(service: SessionService) -> None:
Expand Down