From 71493f720bf3a629ba40bed26bf4ebcb4e623302 Mon Sep 17 00:00:00 2001 From: Walter Oggioni Date: Sat, 24 Oct 2026 22:25:44 +0000 Subject: [PATCH] Fix OIDC subject collisions by introducing canonical provider-plus-subject principals - Add canonical_principal(provider, sub) => 'oidc:v1::' - Preserve provider identity across HTTP sessions and WebSockets - Apply principals throughout: game creation/joining/seating, WebSocket views/actions, Redis engine state, match history, leaderboard focus, ratings, /api/me, OpenAPI, and telemetry - Widen identity columns to 1024 chars with schema-only aerich migration (5_20261024221522_canonical_principals.py) - Document provider names as stable security identifiers - Add regression tests: same subject at different providers cannot share games, private hands, join codes, moves, history, or ratings; distinct seat and rating ownership verified - All platform, integration, game, and frontend tests pass; mypy clean --- server/README.md | 14 +- .../5_20261024221522_canonical_principals.py | 47 +++++ .../packages/tavolo-app/src/tavolo/config.py | 7 +- .../tavolo-app/src/tavolo/telemetry.py | 13 +- .../src/tavolo/platform/auth.py | 69 ++++++- .../src/tavolo/platform/engine.py | 18 +- .../src/tavolo/platform/models.py | 16 +- .../src/tavolo/platform/openapi.py | 16 +- .../src/tavolo/platform/routes/games.py | 29 +-- .../src/tavolo/platform/routes/me.py | 6 +- .../src/tavolo/platform/routes/stats.py | 9 +- .../src/tavolo/platform/stats.py | 7 +- .../tavolo-platform/src/tavolo/platform/ws.py | 2 +- .../packages/tavolo-platform/tests/helpers.py | 52 ++++- .../tavolo-platform/tests/test_auth.py | 49 ++++- .../tavolo-platform/tests/test_routes.py | 59 +++++- .../tavolo-platform/tests/test_stats.py | 190 ++++++++++++------ .../packages/tavolo-platform/tests/test_ws.py | 23 ++- server/tests/helpers/__init__.py | 4 +- server/tests/helpers/oidc.py | 18 +- server/tests/test_briscola.py | 18 +- server/tests/test_briscola_chiamata.py | 14 +- server/tests/test_routes_games.py | 32 ++- server/tests/test_routes_me.py | 4 +- server/tests/test_scopa.py | 28 +-- server/tests/test_stats.py | 26 +-- server/tests/test_telemetry.py | 10 +- server/tests/test_websocket.py | 28 +-- web/src/components/avatar.rs | 7 +- web/src/model.rs | 4 + 30 files changed, 591 insertions(+), 228 deletions(-) create mode 100644 server/migrations/models/5_20261024221522_canonical_principals.py diff --git a/server/README.md b/server/README.md index 64f63de..87501be 100644 --- a/server/README.md +++ b/server/README.md @@ -138,9 +138,12 @@ bookmarks and scripts), and `GET /auth/logout` signs out whichever provider the session is logged in with. Sessions hold a single identity: logging in with another provider -replaces the previous one. Each provider's `sub` is a separate account, -so match history and Elo ratings are per provider identity (someone using -two providers has two accounts). Migrating from the pre-0.0.6 unindexed +replaces the previous one. Accounts are keyed by the canonical principal +`oidc:v1::`, not by `sub` alone, so equal subjects at two +providers cannot share games, history, or ratings (someone using two +providers has two accounts). Provider names are permanent security +identifiers: do not reuse a name for a different issuer, and treat renaming +one as an identity migration. Migrating from the pre-0.0.6 unindexed variables: rename `OIDC_ISSUER`/`OIDC_CLIENT_ID`/... to `OIDC_0_...`, add `OIDC_0_PROVIDER`, and register the new callback path (`/auth//callback`) at the provider. Sessions survive only when @@ -249,8 +252,9 @@ aggregate with `max`, not `sum`. per-deal audit; for briscola chiamata: the calling side's points against the defenders', the declared target, the called card and whether the caller played alone). -- `match_player` — one row per participant: the OIDC `sub`, display name, - seat, game-defined team label (nullable), whether they won, the points +- `match_player` — one row per participant: the canonical provider-qualified + principal (`user_sub`), display name, seat, game-defined team label + (nullable), whether they won, the points they scored, the Elo change the match produced (`elo_delta`) and game-specific extras (`details` JSON). Unique per `(match, user_sub)`. - `player_rating` — current Elo rating and leaderboard totals per diff --git a/server/migrations/models/5_20261024221522_canonical_principals.py b/server/migrations/models/5_20261024221522_canonical_principals.py new file mode 100644 index 0000000..38b09c0 --- /dev/null +++ b/server/migrations/models/5_20261024221522_canonical_principals.py @@ -0,0 +1,47 @@ +from tortoise import BaseDBAsyncClient + +RUN_IN_TRANSACTION = True + + +async def upgrade(db: BaseDBAsyncClient) -> str: + return """ + ALTER TABLE "match_player" ALTER COLUMN "user_sub" TYPE VARCHAR(1024) USING "user_sub"::VARCHAR(1024); + ALTER TABLE "player_rating" ALTER COLUMN "user_sub" TYPE VARCHAR(1024) USING "user_sub"::VARCHAR(1024);""" + + +async def downgrade(db: BaseDBAsyncClient) -> str: + return """ + ALTER TABLE "match_player" ALTER COLUMN "user_sub" TYPE VARCHAR(255) USING "user_sub"::VARCHAR(255); + ALTER TABLE "player_rating" ALTER COLUMN "user_sub" TYPE VARCHAR(255) USING "user_sub"::VARCHAR(255);""" + + +MODELS_STATE = ( + "eJztmutv4jgQwP+VKJ96Uq8q6Wt1Wp0UKNVyC6UCunfaahWZxFCrxs46zrGo1//9bOf94t" + "EChYoP2yVjj+P8Jh7PjPOsT6gDsXfSAdx+1P/QnnUCJlD8yDYcazpw3UQsBRwMcdAz7jL0" + "OAM2F8IRwB4UIgd6NkMuR5TIrqZm04mLIYeOptQ0OtIogfI//gg1BsfI45CJ5rGYh8ZnLv" + "RO5NgOtcXgiIzfNoxP0E8fWpyOoejIxGAPP4QYEQf+gl506T5ZIwSxkwGCHDmAkltyQCm7" + "v29d36iecopDy6bYn5Cktzvjj5TE3X0fOSdSR7aNIYEMiEdI4SI+xiHWSBTMWAg482E8VS" + "cROHAEfCyh659HPrEla03dSf45/zOcWqqbZd12B1a/ObAsvWAjOYUc71BkUyLtiwiXoJ5f" + "gnETIEqqyxs0vpi9o7PL3xQC6vExU40Kl/6iFAEHgaqCnlCW9gp4FWA3HgErh51RyjEXU3" + "4N7UgwB3dEagNsxZr6ZWFIxlwuyjNjDutvZi/AbSjcVCzAYFnehi2GapLUE8oeB0ywsAAv" + "Yr4WlDiawHLUWc0caydUPYl+rId84ky2gH4O6kGr0+wPzM6dHH7ieT+x4mUOmrLFUNJZTn" + "p0mTNLPIj2d2vwRZOX2vfubTO/VuJ+g++6nBPwObUInVrASTOJxJEoY+YRIsh7fJWdc6oH" + "Q++0ocVQkk3Bxn/1u7fl9k008qZFNtf+07DYQDdl1NQ+NfQR5oh4J/K277FVSUIZK0du86" + "hj/pP3qI12t543nxygLtyrjBtGT6k9TQqGwH6aAuZYmZbEbi4GM8i8ouHqoeLN1x7EQD1v" + "0VTpEO1OjbS/yzCRhlutAkoNWkW02DQxJnkJIGCsHkneW96pBFlV0JsQXRD6Wm7Sc2EEfC" + "e2T2QjV9k0Clt9DzINEfVbDVkMeVfQK4lxH5IgXepYnj/UfxwC310KfGO7rBD3pnU+Xthb" + "OzXOlwh8ZbfK0DdozAa/DvLkirXU9Qq083qbIr5V95tFbpyeLkFc9KoErtpyyQYsCz/7E4" + "Bxi/CKNAOWxp1ye9hDxmM5id/PjKvLT6JVzVFeXM1h3e+Y7XbrdpBjySGYrPLORv1f9a6G" + "S39nMG44KZ4GQVYuEKMUQ0DK6U5Lw7KhUNnDt3QOznq3284EyfXWIMf1vlNvCl+scItOiC" + "tx8QX2bMpKvO4NpqDKFUQaOcwjqbIpzqfbh3zdva+3m9pdr9lo9VthVhJnmaoxC7fXNNs5" + "uhBTMWfMwar+NqP4Kqe7a85ifT7XgRwgXJKkVWfXKZVDer2W9DptkCDrWi0tSeusMznZXa" + "e9IBcp1CtyfEt8tPDDaEy+wplC3BITAcQuc835s6P9hFooSQgxA9M4Q868U+LpxTPDwDM3" + "zH7DvG7qL9U1oE1WN4LKRQ/w4CEL5Y1M+/G8+kZQ2bBY0nVhgaPhMwYJ15qYaoGeBoijiS" + "DKgWxIxcNpnHKhGxUxgntorvgnz3GKpY+1jFhaFEnnz8kZUr4w8pA9X0pgTIVnDVaXenOO" + "wzcCekFFyCmWWw71lUN9ZXdSqE0VWHb3DPdD5auJI8oyrgzyE4XtlVVqF6ebyqXCEN+onV" + "+dfzq7PI/j/FgyL9gvxvlF570k16Li9vjuC9xor1wSaby1HkDmQCbBxtIVlETlUEJZWELZ" + "v9MBfVOB2BbOBnxXfkHymg9Uspof8PsUnYkEp0vwTA8PxKvh7/j3KvFx/pLfR2wyNzYhQ+" + "VfuoYtx/PyYZD0WZQIV9t5zd+gVu6hpZliyQ4apntvSxE3X8p94w5anRn+C5mHys5+qp19" + "SuUDpirGxcUynv7iotrTy7asp5eLagXCYfcPSLe21D5am7OP1or7qLgjh2SlLwBTKts/o3" + "ifkvh7fOy3/s3s5X9zkOhU" +) diff --git a/server/packages/tavolo-app/src/tavolo/config.py b/server/packages/tavolo-app/src/tavolo/config.py index d9b8eff..fd1158c 100644 --- a/server/packages/tavolo-app/src/tavolo/config.py +++ b/server/packages/tavolo-app/src/tavolo/config.py @@ -72,9 +72,10 @@ _DEFAULT_OIDC_CLIENT_ID = "tavolo" class OidcProviderSettings: """One OIDC provider, parsed from the ``OIDC__*`` variables. - ``provider`` names the provider in URLs (``/auth//login``) - and namespaces its session state (``oidc::...``); it must - match kaya-oidc's ``[A-Za-z0-9_-]+`` pattern. + ``provider`` names the provider in URLs (``/auth//login``), + namespaces its session state (``oidc::...``), and is part of + the canonical user principal. It must match kaya-oidc's + ``[A-Za-z0-9_-]+`` pattern and remain stable across deployments. """ provider: str diff --git a/server/packages/tavolo-app/src/tavolo/telemetry.py b/server/packages/tavolo-app/src/tavolo/telemetry.py index a68a8df..e18b5b4 100644 --- a/server/packages/tavolo-app/src/tavolo/telemetry.py +++ b/server/packages/tavolo-app/src/tavolo/telemetry.py @@ -95,10 +95,11 @@ def session_tag_hook(span: Any, carrier: Any) -> None: The session id is the session cookie, i.e. a credential, so only a truncated SHA-256 of it is exported; session ids are ``secrets.token_urlsafe(32)`` (256 bits), which makes the digest - irreversible in practice. The OIDC ``sub`` is a pseudonymous - identifier already persisted in Postgres and served by the API, so it - is exported verbatim as ``user.id`` (OpenTelemetry's current end-user - attribute; ``enduser.id`` is PII-flagged and being superseded). + irreversible in practice. The canonical provider-qualified principal + is a pseudonymous identifier already persisted in Postgres and served + by the API, so it is exported verbatim as ``user.id`` (OpenTelemetry's + current end-user attribute; ``enduser.id`` is PII-flagged and being + superseded). """ session = getattr(carrier, "session", None) if session is None: @@ -111,9 +112,7 @@ def session_tag_hook(span: Any, carrier: Any) -> None: ) user = session_user(session) if user is not None: - sub = user.get("sub") - if sub: - span.set_attribute("user.id", str(sub)) + span.set_attribute("user.id", user.principal) class _NoOtelLogs(logging.Filter): diff --git a/server/packages/tavolo-platform/src/tavolo/platform/auth.py b/server/packages/tavolo-platform/src/tavolo/platform/auth.py index edb870e..57f2098 100644 --- a/server/packages/tavolo-platform/src/tavolo/platform/auth.py +++ b/server/packages/tavolo-platform/src/tavolo/platform/auth.py @@ -2,10 +2,14 @@ The platform has no application roles: every authenticated user may create and join games. Authorization beyond login is game membership, -checked against the seats of the live session. +checked against the seats of the live session. Seats and persisted stats +use a canonical provider-plus-subject principal because a bare OIDC ``sub`` +is not unique across providers. """ from __future__ import annotations +import re +from dataclasses import dataclass from typing import Any, Callable, Mapping, Optional from kaya.core import HttpContext, WebSocket @@ -17,25 +21,62 @@ from kaya.session import Session # fixed key or provider name. _OIDC_PREFIX = "oidc:" _USER_SUFFIX = ":user" +_PRINCIPAL_PREFIX = "oidc:v1:" +_PROVIDER_NAME = re.compile(r"^[A-Za-z0-9_-]+$") -def session_user(session: Session) -> Optional[OIDCUser]: - """Return the authenticated user stored in a session, if any. +@dataclass(frozen=True) +class AuthenticatedUser: + """OIDC claims together with their stable, provider-qualified identity.""" + + provider: str + claims: OIDCUser + principal: str + + +def canonical_principal(provider: str, sub: str) -> str: + """Return the global principal for one provider's OIDC subject. + + Provider names are restricted to characters that cannot include ``:``, + so the tuple remains unambiguous even when the subject contains colons. + They are persisted security identifiers and must not be reused for a + different issuer. + """ + if not isinstance(provider, str) or _PROVIDER_NAME.fullmatch(provider) is None: + raise ValueError("invalid OIDC provider") + if not isinstance(sub, str) or not sub: + raise ValueError("invalid OIDC subject") + return f"{_PRINCIPAL_PREFIX}{provider}:{sub}" + + +def _authenticated_user( + provider: str, claims: OIDCUser +) -> Optional[AuthenticatedUser]: + try: + principal = canonical_principal(provider, claims.sub) + except (KeyError, TypeError, ValueError): + return None + return AuthenticatedUser(provider=provider, claims=claims, principal=principal) + + +def session_user(session: Session) -> Optional[AuthenticatedUser]: + """Return the authenticated identity stored in a session, if any. The kaya-oidc mixin writes the userinfo at login under a per-provider - key, so any provider's user is accepted (the configured provider is - ``default`` unless the app says otherwise). + key. Retaining that provider is essential: an OIDC subject is only + unique within one provider, not across all of them. """ for key in session: if key.startswith(_OIDC_PREFIX) and key.endswith(_USER_SUFFIX): claims = session.get(key) if isinstance(claims, Mapping): - return OIDCUser(claims) + provider = key.removeprefix(_OIDC_PREFIX).removesuffix(_USER_SUFFIX) + return _authenticated_user(provider, OIDCUser(claims)) return None -def get_ws_user(ws: WebSocket) -> Optional[OIDCUser]: - """Return the authenticated user of a WebSocket connection, if any. +def get_ws_user(ws: WebSocket) -> Optional[AuthenticatedUser]: + """Return the authenticated identity of a WebSocket connection, if any. The session mixin injects ``session`` into the websocket wrapper; the OIDC mixin stores the userinfo there at login. Patched in tests. @@ -73,13 +114,19 @@ def picture(user: OIDCUser) -> Optional[str]: return None -def current_user(oidc: OIDCProviders, ctx: HttpContext) -> Optional[OIDCUser]: - """Return the authenticated user of an HTTP request, if any. +def current_user( + oidc: OIDCProviders, ctx: HttpContext +) -> Optional[AuthenticatedUser]: + """Return the authenticated identity of an HTTP request, if any. Sessions are single-identity by default, so at most one provider has a user; if several do, the first configured provider wins. """ - return next(iter(oidc.get_users(ctx).values()), None) + users = oidc.get_users(ctx) + if not users: + return None + provider, claims = next(iter(users.items())) + return _authenticated_user(provider, claims) def require_auth(oidc: OIDCProviders) -> Callable[[Callable[..., Any]], Callable[..., Any]]: diff --git a/server/packages/tavolo-platform/src/tavolo/platform/engine.py b/server/packages/tavolo-platform/src/tavolo/platform/engine.py index 5f24c2a..a754a55 100644 --- a/server/packages/tavolo-platform/src/tavolo/platform/engine.py +++ b/server/packages/tavolo-platform/src/tavolo/platform/engine.py @@ -40,6 +40,7 @@ class Seat: persisting participations. """ + # Canonical provider-plus-subject principal, despite the legacy field name. user_sub: str display_name: str # Game-defined team label ("A"/"B", ...); ``None`` for games without @@ -73,6 +74,7 @@ class GameSession: id: str game_type: str join_code: str + # Canonical principal of the creator. creator_sub: str players: List[Seat] = field(default_factory=list) created_at: Optional[datetime] = None @@ -81,13 +83,13 @@ class GameSession: stats_saved: bool = False state: Any = None - def seated(self, sub: str) -> bool: - """Whether ``sub`` occupies a seat in this game.""" - return self.seat_of(sub) is not None + def seated(self, principal: str) -> bool: + """Whether ``principal`` occupies a seat in this game.""" + return self.seat_of(principal) is not None - def seat_of(self, sub: str) -> Optional[Seat]: + def seat_of(self, principal: str) -> Optional[Seat]: for seat in self.players: - if seat.user_sub == sub: + if seat.user_sub == principal: return seat return None @@ -112,6 +114,7 @@ class Deadline: class PlayerResult: """The outcome of a finished match for one participant.""" + # Canonical provider-plus-subject principal, despite the legacy field name. user_sub: str seat: int won: bool @@ -126,7 +129,7 @@ class PlayerResult: class MatchResult: """The game-independent outcome of a finished match. - ``teams`` groups the participants' subs into the sides the Elo rating + ``teams`` groups the participants' principals into the sides the Elo rating treats as opponents (exactly two for now); ``winner_team`` is the index of the winning side. ``summary`` is persisted verbatim as the match's JSON ``result`` column. @@ -145,6 +148,9 @@ class GameEngine(ABC): logging: all persistence, locking and transport concerns belong to the platform. Rule violations are reported by raising :class:`~tavolo.platform.errors.GameError` subclasses. + + Every ``user_sub`` value in this contract is an opaque, canonical + provider-plus-subject principal, never a bare cross-provider OIDC ``sub``. """ #: Unique id carried by sessions and persisted matches. diff --git a/server/packages/tavolo-platform/src/tavolo/platform/models.py b/server/packages/tavolo-platform/src/tavolo/platform/models.py index 8211bc3..df1df4d 100644 --- a/server/packages/tavolo-platform/src/tavolo/platform/models.py +++ b/server/packages/tavolo-platform/src/tavolo/platform/models.py @@ -6,11 +6,11 @@ completed matches are written here. The schema is game-independent: * :class:`Match` — one row per finished match; everything game-specific (scores, hands played, options, per-hand audit trail) lives in the JSON ``result`` column produced by the game's engine. -* :class:`MatchPlayer` — one row per participant, linking an OIDC - ``sub`` to a seat, an optional team label, whether they won, the - points they scored and their Elo delta. +* :class:`MatchPlayer` — one row per participant, linking a canonical + provider-qualified principal to a seat, an optional team label, whether + they won, the points they scored and their Elo delta. * :class:`PlayerRating` — current chess-style Elo rating and leaderboard - totals (wins, points, latest display name) of a player for one game + totals (wins, points, latest display name) of a principal for one game type, updated transactionally with every finished match (see :mod:`tavolo.platform.elo` and :mod:`tavolo.platform.stats`). """ @@ -48,8 +48,8 @@ class MatchPlayer(Model): match: fields.ForeignKeyRelation[Match] = fields.ForeignKeyField( "models.Match", related_name="players", on_delete=fields.CASCADE ) - # OIDC subject of the player; no local users table. - user_sub = fields.CharField(max_length=255, db_index=True) + # Canonical provider-plus-subject principal; no local users table. + user_sub = fields.CharField(max_length=1024, db_index=True) display_name = fields.CharField(max_length=200) seat = fields.SmallIntField() # Game-defined team label; null for games without fixed teams. @@ -73,8 +73,8 @@ class PlayerRating(Model): """Current Elo rating and leaderboard totals of one player per game.""" id = fields.UUIDField(pk=True) - # OIDC subject of the player; no local users table. - user_sub = fields.CharField(max_length=255) + # Canonical provider-plus-subject principal; no local users table. + user_sub = fields.CharField(max_length=1024) # Which game the rating applies to (an id from the game registry). game_type = fields.CharField(max_length=32) rating = fields.IntField(default=INITIAL_RATING) diff --git a/server/packages/tavolo-platform/src/tavolo/platform/openapi.py b/server/packages/tavolo-platform/src/tavolo/platform/openapi.py index 32874e9..43b0b18 100644 --- a/server/packages/tavolo-platform/src/tavolo/platform/openapi.py +++ b/server/packages/tavolo-platform/src/tavolo/platform/openapi.py @@ -90,6 +90,14 @@ def _array(items: Dict[str, Any]) -> Dict[str, Any]: return {"type": "array", "items": items} +def _principal() -> Dict[str, Any]: + return { + "type": "string", + "description": "Canonical provider-qualified principal: " + "oidc:v1::.", + } + + def _nullable(schema: Dict[str, Any]) -> Dict[str, Any]: """A copy of ``schema`` that also accepts ``null`` (OpenAPI 3.1 types).""" nullable = dict(schema) @@ -124,7 +132,7 @@ HEALTH_SCHEMA = _object( ME_SCHEMA = _object( { - "sub": {"type": "string"}, + "sub": _principal(), "name": {"type": "string"}, "picture": _nullable({"type": "string"}), }, @@ -164,7 +172,7 @@ GAME_TYPES_PAGE_SCHEMA = _object( LOBBY_PLAYER_SCHEMA = _object( { - "sub": {"type": "string"}, + "sub": _principal(), "name": {"type": "string"}, "seat": {"type": "integer"}, "team": _nullable({ @@ -258,7 +266,7 @@ ONGOING_GAMES_PAGE_SCHEMA = _object( MATCH_PLAYER_SCHEMA = _object( { - "user_sub": {"type": "string"}, + "user_sub": _principal(), "display_name": {"type": "string"}, "seat": {"type": "integer"}, "team": _nullable({"type": "string"}), @@ -314,7 +322,7 @@ MATCHES_PAGE_SCHEMA = _object( LEADERBOARD_ENTRY_SCHEMA = _object( { - "user_sub": {"type": "string"}, + "user_sub": _principal(), "display_name": {"type": "string"}, "elo": {"type": "integer"}, "matches": {"type": "integer"}, diff --git a/server/packages/tavolo-platform/src/tavolo/platform/routes/games.py b/server/packages/tavolo-platform/src/tavolo/platform/routes/games.py index d067af8..7ffaea6 100644 --- a/server/packages/tavolo-platform/src/tavolo/platform/routes/games.py +++ b/server/packages/tavolo-platform/src/tavolo/platform/routes/games.py @@ -225,12 +225,13 @@ def register(app: KayaApp, platform: "Platform") -> None: user = current_user(platform.oidc, ctx) assert user is not None # enforced by @require_auth + principal = user.principal session = GameSession( id=str(uuid.uuid4()), game_type=engine.id, join_code=await _unique_code(platform), - creator_sub=user.sub, - players=[Seat(user_sub=user.sub, display_name=display_name(user))], + creator_sub=principal, + players=[Seat(user_sub=principal, display_name=display_name(user.claims))], created_at=datetime.now(timezone.utc), ) try: @@ -242,7 +243,7 @@ def register(app: KayaApp, platform: "Platform") -> None: log.info( "game %s created by %s (%s, options %r)", session.id, - user.sub, + principal, engine.id, options, ) @@ -290,9 +291,10 @@ def register(app: KayaApp, platform: "Platform") -> None: user = current_user(platform.oidc, ctx) assert user is not None + principal = user.principal existing = await platform.game_store.find_by_code(code) if existing is None: - log.debug("join rejected for %s: unknown code %r", user.sub, code) + log.debug("join rejected for %s: unknown code %r", principal, code) await send_error(ctx, 404, "unknown join code") return @@ -303,9 +305,9 @@ def register(app: KayaApp, platform: "Platform") -> None: return engine = platform.registry.require(session.game_type) try: - engine.join(session, user.sub, display_name(user)) + engine.join(session, principal, display_name(user.claims)) except GameError as exc: - log.debug("join rejected for %s in game %s: %s", user.sub, session.id, exc) + log.debug("join rejected for %s in game %s: %s", principal, session.id, exc) await send_error(ctx, 409, str(exc)) return await platform.game_store.save(session) @@ -314,16 +316,16 @@ def register(app: KayaApp, platform: "Platform") -> None: # armed a deadline; queue it so it fires even if nobody ever # connects. await platform.scheduler.sync_deadline(session) - seat = next(i for i, s in enumerate(session.players) if s.user_sub == user.sub) + seat = next(i for i, s in enumerate(session.players) if s.user_sub == principal) if engine.in_lobby(session): log.info( "%s joined game %s (seat %d, %d/%d players)", - user.sub, session.id, seat, len(session.players), engine.max_players, + principal, session.id, seat, len(session.players), engine.max_players, ) await send_json(ctx, 200, _lobby_payload(platform, session)) return - log.info("%s joined game %s (seat %d); match started", user.sub, session.id, seat) - await send_json(ctx, 200, _view_payload(platform, session, user.sub)) + log.info("%s joined game %s (seat %d); match started", principal, session.id, seat) + await send_json(ctx, 200, _view_payload(platform, session, principal)) @app.GET("/api/games/preview") @operation(summary="Preview a game by join code", @@ -376,7 +378,7 @@ def register(app: KayaApp, platform: "Platform") -> None: return user = current_user(platform.oidc, ctx) await send_json( - ctx, 200, _view_payload(platform, session, user.sub if user else None) + ctx, 200, _view_payload(platform, session, user.principal if user else None) ) @app.GET("/api/me/games") @@ -395,11 +397,12 @@ def register(app: KayaApp, platform: "Platform") -> None: async def my_games(ctx: HttpContext) -> None: user = current_user(platform.oidc, ctx) assert user is not None # enforced by @require_auth + principal = user.principal sessions = await platform.game_store.list_ongoing() await send_json(ctx, 200, { "results": [ - _ongoing_game_payload(platform, session, user.sub) + _ongoing_game_payload(platform, session, principal) for session in sessions - if session.seated(user.sub) + if session.seated(principal) ] }) diff --git a/server/packages/tavolo-platform/src/tavolo/platform/routes/me.py b/server/packages/tavolo-platform/src/tavolo/platform/routes/me.py index 46a924e..2e1eea7 100644 --- a/server/packages/tavolo-platform/src/tavolo/platform/routes/me.py +++ b/server/packages/tavolo-platform/src/tavolo/platform/routes/me.py @@ -34,8 +34,8 @@ def register(app: KayaApp, platform: "Platform") -> None: ctx, 200, { - "sub": user.sub, - "name": display_name(user), - "picture": picture(user), + "sub": user.principal, + "name": display_name(user.claims), + "picture": picture(user.claims), }, ) diff --git a/server/packages/tavolo-platform/src/tavolo/platform/routes/stats.py b/server/packages/tavolo-platform/src/tavolo/platform/routes/stats.py index 11e94a5..6536be3 100644 --- a/server/packages/tavolo-platform/src/tavolo/platform/routes/stats.py +++ b/server/packages/tavolo-platform/src/tavolo/platform/routes/stats.py @@ -154,7 +154,8 @@ def register(app: KayaApp, platform: "Platform") -> None: return user = current_user(platform.oidc, ctx) assert user is not None - queryset = Match.filter(players__user_sub=user.sub).distinct() + principal = user.principal + queryset = Match.filter(players__user_sub=principal).distinct() if game_type is not None: queryset = queryset.filter(game_type=game_type) matches, next_cursor = await paginate( @@ -173,7 +174,7 @@ def register(app: KayaApp, platform: "Platform") -> None: # declare it on the model class, hence the getattr. by_match[getattr(player, "match_id")].append(player) results = [ - _serialize_match(m, user.sub, by_match.get(m.id, [])) for m in matches + _serialize_match(m, principal, by_match.get(m.id, [])) for m in matches ] await send_json(ctx, 200, {"results": results, "next_cursor": next_cursor}) @@ -226,7 +227,7 @@ def register(app: KayaApp, platform: "Platform") -> None: yours: Optional[PlayerRating] = None if user is not None: yours = await PlayerRating.get_or_none( - user_sub=user.sub, game_type=rating_game + user_sub=user.principal, game_type=rating_game ) your_rank: Optional[int] = None your_page: Optional[int] = None @@ -276,7 +277,7 @@ def register(app: KayaApp, platform: "Platform") -> None: async def my_ratings(ctx: HttpContext) -> None: user = current_user(platform.oidc, ctx) assert user is not None - rows = await PlayerRating.filter(user_sub=user.sub).order_by("game_type") + rows = await PlayerRating.filter(user_sub=user.principal).order_by("game_type") await send_json(ctx, 200, { "results": [ { diff --git a/server/packages/tavolo-platform/src/tavolo/platform/stats.py b/server/packages/tavolo-platform/src/tavolo/platform/stats.py index a78499d..0f40923 100644 --- a/server/packages/tavolo-platform/src/tavolo/platform/stats.py +++ b/server/packages/tavolo-platform/src/tavolo/platform/stats.py @@ -29,6 +29,7 @@ log = getLogger(__name__) class PlayerStats: """One participant's data feeding a ``player_rating`` row update.""" + # Canonical provider-plus-subject principal, despite the legacy field name. user_sub: str won: bool score: float @@ -43,9 +44,9 @@ async def apply_match_stats( ) -> Dict[str, int]: """Update the ratings and leaderboard aggregates of ``teams``. - ``teams`` groups the participants' subs into the two opposing sides; - ``winner_team`` is the index of the winning side. Ratings are read - from (and written back to) the ``player_rating`` table; unrated + ``teams`` groups the participants' canonical principals into the two + opposing sides; ``winner_team`` is the index of the winning side. Ratings + are read from (and written back to) the ``player_rating`` table; unrated players start at the initial rating. Alongside the Elo delta the transaction records each participant's wins, accumulated points and latest display name, so the leaderboard is a single-table query. diff --git a/server/packages/tavolo-platform/src/tavolo/platform/ws.py b/server/packages/tavolo-platform/src/tavolo/platform/ws.py index ed9bbf0..952d834 100644 --- a/server/packages/tavolo-platform/src/tavolo/platform/ws.py +++ b/server/packages/tavolo-platform/src/tavolo/platform/ws.py @@ -121,7 +121,7 @@ def register(app: KayaApp, platform: "Platform") -> None: # Anonymous and non-seated viewers are welcome as read-only # spectators; only an unknown game id is refused. user = auth.get_ws_user(ws) - sub: Optional[str] = user.sub if user is not None else None + sub: Optional[str] = user.principal if user is not None else None session = await platform.game_store.load(game_id) if session is None: diff --git a/server/packages/tavolo-platform/tests/helpers.py b/server/packages/tavolo-platform/tests/helpers.py index 6402e66..52ba4b0 100644 --- a/server/packages/tavolo-platform/tests/helpers.py +++ b/server/packages/tavolo-platform/tests/helpers.py @@ -43,7 +43,11 @@ from tavolo.platform import ( PlayerResult, Seat, ) -from tavolo.platform.auth import get_ws_user # noqa: F401 (re-exported for patching) +from tavolo.platform.auth import ( + AuthenticatedUser, + canonical_principal, + get_ws_user, # noqa: F401 (re-exported for patching) +) from tavolo.platform.deadlines import DeadlineScheduler from tavolo.platform.openapi import SCHEMAS from tavolo.platform.registry import GameRegistry @@ -326,13 +330,32 @@ def make_user(sub: str, name: Optional[str] = None) -> OIDCUser: return OIDCUser({"sub": sub, "preferred_username": name or sub}) +def make_principal(sub: str, provider: str = "default") -> str: + return canonical_principal(provider, sub) + + +def make_identity( + sub: str, name: Optional[str] = None, provider: str = "default" +) -> AuthenticatedUser: + claims = make_user(sub, name) + return AuthenticatedUser( + provider=provider, + claims=claims, + principal=canonical_principal(provider, sub), + ) + + @contextlib.contextmanager -def oidc_user(oidc: OIDCProviders, sub: str, name: Optional[str] = None) -> Iterator[OIDCUser]: - """Context manager: make ``oidc.get_users`` return this user for the - first configured provider.""" +def oidc_user( + oidc: OIDCProviders, + sub: str, + name: Optional[str] = None, + provider: Optional[str] = None, +) -> Iterator[OIDCUser]: + """Context manager: fake a login, using the first provider by default.""" user = make_user(sub, name) - provider = next(iter(oidc.providers)) - patcher = _mock.patch.object(oidc, "get_users", return_value={provider: user}) + selected = provider or next(iter(oidc.providers)) + patcher = _mock.patch.object(oidc, "get_users", return_value={selected: user}) patcher.start() try: yield user @@ -341,13 +364,20 @@ def oidc_user(oidc: OIDCProviders, sub: str, name: Optional[str] = None) -> Iter @contextlib.contextmanager -def ws_users(users: Sequence[OIDCUser]) -> Iterator[None]: - """Context manager: patch ``auth.get_ws_user`` to hand out ``users`` - one per websocket connection, in order. Once exhausted it keeps - returning the last user.""" +def ws_users( + users: Sequence[OIDCUser | AuthenticatedUser], provider: str = "default" +) -> Iterator[None]: + """Patch ``auth.get_ws_user`` to hand out identities in sequence.""" from tavolo.platform import auth - remaining = list(users) + remaining = [ + user if isinstance(user, AuthenticatedUser) else AuthenticatedUser( + provider=provider, + claims=user, + principal=canonical_principal(provider, user.sub), + ) + for user in users + ] last = remaining[-1] if remaining else None def _next(_ws): diff --git a/server/packages/tavolo-platform/tests/test_auth.py b/server/packages/tavolo-platform/tests/test_auth.py index e7470e9..8d0ddb2 100644 --- a/server/packages/tavolo-platform/tests/test_auth.py +++ b/server/packages/tavolo-platform/tests/test_auth.py @@ -16,6 +16,7 @@ from kaya.session import InMemorySessionStore, Session, SessionMixin from helpers import async_test from tavolo.platform.auth import ( + canonical_principal, current_user, display_name, get_ws_user, @@ -53,12 +54,16 @@ class SessionUserTest(unittest.TestCase): def test_finds_default_provider_user(self) -> None: user = session_user(Session("sid", {"oidc:default:user": {"sub": "alice"}})) assert user is not None - self.assertEqual("alice", user.sub) + self.assertEqual("alice", user.claims.sub) + self.assertEqual("default", user.provider) + self.assertEqual(canonical_principal("default", "alice"), user.principal) def test_finds_user_of_any_provider(self) -> None: user = session_user(Session("sid", {"oidc:google:user": {"sub": "alice"}})) assert user is not None - self.assertEqual("alice", user.sub) + self.assertEqual("alice", user.claims.sub) + self.assertEqual("google", user.provider) + self.assertEqual(canonical_principal("google", "alice"), user.principal) def test_ignores_flow_state_and_tokens(self) -> None: session = Session( @@ -79,6 +84,12 @@ class SessionUserTest(unittest.TestCase): session = Session("sid", {"oidc:default:user": "alice"}) self.assertIsNone(session_user(session)) + def test_ignores_missing_or_invalid_subjects(self) -> None: + for claims in ({}, {"sub": ""}, {"sub": 42}): + with self.subTest(claims=claims): + session = Session("sid", {"oidc:default:user": claims}) + self.assertIsNone(session_user(session)) + class GetWsUserTest(unittest.TestCase): def test_returns_user_from_provider_namespaced_session(self) -> None: @@ -88,12 +99,33 @@ class GetWsUserTest(unittest.TestCase): ) user = get_ws_user(_ws(session)) assert user is not None - self.assertEqual("alice", user.sub) + self.assertEqual("alice", user.claims.sub) def test_missing_session_returns_none(self) -> None: self.assertIsNone(get_ws_user(SimpleNamespace())) +class CanonicalPrincipalTest(unittest.TestCase): + def test_same_subject_at_different_providers_is_distinct(self) -> None: + google = canonical_principal("google", "shared") + github = canonical_principal("github", "shared") + self.assertNotEqual(google, github) + self.assertEqual("oidc:v1:google:shared", google) + self.assertEqual("oidc:v1:github:shared", github) + + def test_subject_may_contain_colons(self) -> None: + self.assertEqual( + "oidc:v1:google:tenant:alice", + canonical_principal("google", "tenant:alice"), + ) + + def test_invalid_components_are_rejected(self) -> None: + for provider, sub in (("", "alice"), ("bad:provider", "alice"), ("google", "")): + with self.subTest(provider=provider, sub=sub): + with self.assertRaises(ValueError): + canonical_principal(provider, sub) + + class DisplayNameTest(unittest.TestCase): """The claim priority used for the name shown at the table.""" @@ -149,7 +181,9 @@ class CurrentUserTest(unittest.TestCase): _ctx({"oidc:google:user": {"sub": "alice"}}), ) assert user is not None - self.assertEqual("alice", user.sub) + self.assertEqual("alice", user.claims.sub) + self.assertEqual("google", user.provider) + self.assertEqual(canonical_principal("google", "alice"), user.principal) def test_first_configured_provider_wins(self) -> None: # Single-identity sessions never hold two users; this only pins the @@ -164,7 +198,8 @@ class CurrentUserTest(unittest.TestCase): ), ) assert user is not None - self.assertEqual("alice", user.sub) + self.assertEqual("alice", user.claims.sub) + self.assertEqual(canonical_principal("default", "alice"), user.principal) class RequireAuthTest(unittest.TestCase): @@ -198,7 +233,7 @@ class RequireAuthTest(unittest.TestCase): async def handler(request_ctx: Any) -> None: user = current_user(oidc, request_ctx) assert user is not None - handled.append(user.sub) + handled.append(user.principal) await handler(ctx) - self.assertEqual(["alice"], handled) + self.assertEqual([canonical_principal("default", "alice")], handled) diff --git a/server/packages/tavolo-platform/tests/test_routes.py b/server/packages/tavolo-platform/tests/test_routes.py index 08aa30b..355258f 100644 --- a/server/packages/tavolo-platform/tests/test_routes.py +++ b/server/packages/tavolo-platform/tests/test_routes.py @@ -5,7 +5,7 @@ import unittest from httpx import ASGITransport, AsyncClient -from helpers import async_test, make_platform, oidc_user +from helpers import async_test, make_platform, make_principal, oidc_user class GamesRouteTest(unittest.TestCase): @@ -118,7 +118,7 @@ class GamesRouteTest(unittest.TestCase): self.assertEqual("lobby", body["phase"]) self.assertEqual(5, body["target"]) self.assertEqual(1, body["seats_open"]) - self.assertEqual("alice", body["players"][0]["sub"]) + self.assertEqual(make_principal("alice"), body["players"][0]["sub"]) # Join codes are case-insensitive. with oidc_user(platform.oidc, "mallory"): @@ -219,7 +219,7 @@ class GamesRouteTest(unittest.TestCase): self.assertEqual(1, entry["seats_open"]) self.assertEqual(0, entry["your_seat"]) self.assertNotIn("join_code", entry) - self.assertEqual("alice", entry["players"][0]["sub"]) + self.assertEqual(make_principal("alice"), entry["players"][0]["sub"]) # Bob's own game is the only one he sees; Alice's is not his. with oidc_user(platform.oidc, "bob"): @@ -246,6 +246,57 @@ class GamesRouteTest(unittest.TestCase): self.assertEqual(0, by_id[mine.json()["id"]]["seats_open"]) +class CrossProviderGamesTest(unittest.TestCase): + @async_test + async def test_same_sub_other_provider_does_not_inherit_player_access(self) -> None: + app, platform, _ = make_platform(oidc_providers=("default", "google")) + transport = ASGITransport(app=app) + async with AsyncClient(transport=transport, base_url="http://127.0.0.1") as client: + with oidc_user(platform.oidc, "alice"): + created = await client.post("/api/games", json={}) + code = created.json()["join_code"] + game_id = created.json()["id"] + with oidc_user(platform.oidc, "bob"): + await client.post("/api/games/join", json={"code": code}) + + with oidc_user(platform.oidc, "alice", provider="google"): + listed = await client.get("/api/me/games") + watched = await client.get(f"/api/games/{game_id}") + self.assertEqual([], listed.json()["results"]) + self.assertTrue(watched.json()["spectator"]) + self.assertFalse(watched.json()["viewer_seated"]) + self.assertNotIn("join_code", watched.json()) + + # The original provider's identical subject remains seated. + with oidc_user(platform.oidc, "alice"): + owned = await client.get(f"/api/games/{game_id}") + self.assertFalse(owned.json()["spectator"]) + self.assertTrue(owned.json()["viewer_seated"]) + self.assertIn("join_code", owned.json()) + + @async_test + async def test_same_sub_at_two_providers_occupies_distinct_seats(self) -> None: + app, platform, _ = make_platform(oidc_providers=("default", "google")) + transport = ASGITransport(app=app) + async with AsyncClient(transport=transport, base_url="http://127.0.0.1") as client: + with oidc_user(platform.oidc, "alice"): + created = await client.post("/api/games", json={}) + code = created.json()["join_code"] + with oidc_user(platform.oidc, "alice", provider="google"): + joined = await client.post("/api/games/join", json={"code": code}) + self.assertEqual(200, joined.status_code) + self.assertTrue(joined.json()["viewer_seated"]) + session = await platform.game_store.load(created.json()["id"]) + assert session is not None + self.assertEqual( + { + make_principal("alice"), + make_principal("alice", "google"), + }, + {seat.user_sub for seat in session.players}, + ) + + class GameTypesRouteTest(unittest.TestCase): @async_test async def test_lists_available_game_types(self) -> None: @@ -311,7 +362,7 @@ class MeRouteTest(unittest.TestCase): response = await client.get("/api/me") self.assertEqual(200, response.status_code) self.assertEqual( - {"sub": "alice", "name": "alice", "picture": None}, + {"sub": make_principal("alice"), "name": "alice", "picture": None}, response.json(), ) diff --git a/server/packages/tavolo-platform/tests/test_stats.py b/server/packages/tavolo-platform/tests/test_stats.py index 759513a..fb1fa62 100644 --- a/server/packages/tavolo-platform/tests/test_stats.py +++ b/server/packages/tavolo-platform/tests/test_stats.py @@ -12,7 +12,10 @@ from httpx import ASGITransport, AsyncClient from tavolo.platform.elo import INITIAL_RATING from tavolo.platform.models import Match, MatchPlayer, PlayerRating from tavolo.platform.stats import save_match_result -from helpers import DummyEngine, async_test, make_platform, oidc_user, use_db +from helpers import DummyEngine, async_test, make_platform, make_principal, oidc_user, use_db + +ALICE = make_principal("alice") +BOB = make_principal("bob") def _finished_session(target: int = 2): @@ -24,12 +27,12 @@ def _finished_session(target: int = 2): id="stats-game", game_type=engine.id, join_code="STATS1", - creator_sub="alice", - players=[Seat(user_sub="alice", display_name="alice", team="A")], + creator_sub=ALICE, + players=[Seat(user_sub=ALICE, display_name="alice", team="A")], ) engine.create(session, {"target": target}) - engine.join(session, "bob", "bob") - engine.handle_action(session, "alice", "play", {}) + engine.join(session, BOB, "bob") + engine.handle_action(session, ALICE, "play", {}) return engine, session @@ -39,7 +42,7 @@ class SaveMatchResultTest(unittest.TestCase): _, _, tortoise_mixin = make_platform() ctx = await use_db(tortoise_mixin) engine, session = _finished_session() - engine.handle_action(session, "alice", "play", {}) + engine.handle_action(session, ALICE, "play", {}) self.assertTrue(engine.is_finished(session)) with ctx: @@ -53,19 +56,19 @@ class SaveMatchResultTest(unittest.TestCase): # The game type travels from the session onto the row; the # engine's summary is stored verbatim as the result. self.assertEqual("dummy", match.game_type) - self.assertEqual("alice", match.result["winner"]) + self.assertEqual(ALICE, match.result["winner"]) self.assertEqual(2, match.result["plays"]) winners = await MatchPlayer.filter(won=True) - self.assertEqual({"alice"}, {p.user_sub for p in winners}) + self.assertEqual({ALICE}, {p.user_sub for p in winners}) scores = {p.user_sub: p.score for p in await MatchPlayer.all()} - self.assertEqual({"alice": 2.0, "bob": 0.0}, scores) + self.assertEqual({ALICE: 2.0, BOB: 0.0}, scores) @async_test async def test_finished_match_updates_elo_ratings(self) -> None: _, _, tortoise_mixin = make_platform() ctx = await use_db(tortoise_mixin) engine, session = _finished_session() - engine.handle_action(session, "alice", "play", {}) + engine.handle_action(session, ALICE, "play", {}) with ctx: await save_match_result(session, engine) @@ -75,29 +78,59 @@ class SaveMatchResultTest(unittest.TestCase): } self.assertEqual(2, len(ratings)) # Two players at 1500: winner gains K/2, loser loses it. - self.assertEqual(INITIAL_RATING + 16, ratings["alice"].rating) - self.assertEqual(1, ratings["alice"].matches_played) - self.assertEqual(INITIAL_RATING - 16, ratings["bob"].rating) - self.assertEqual(1, ratings["bob"].matches_played) + self.assertEqual(INITIAL_RATING + 16, ratings[ALICE].rating) + self.assertEqual(1, ratings[ALICE].matches_played) + self.assertEqual(INITIAL_RATING - 16, ratings[BOB].rating) + self.assertEqual(1, ratings[BOB].matches_played) # The leaderboard aggregates ride along in the same transaction. - self.assertEqual(1, ratings["alice"].wins) - self.assertEqual(0, ratings["bob"].wins) - self.assertEqual(2.0, ratings["alice"].points) - self.assertEqual(0.0, ratings["bob"].points) - self.assertEqual("alice", ratings["alice"].display_name) + self.assertEqual(1, ratings[ALICE].wins) + self.assertEqual(0, ratings[BOB].wins) + self.assertEqual(2.0, ratings[ALICE].points) + self.assertEqual(0.0, ratings[BOB].points) + self.assertEqual("alice", ratings[ALICE].display_name) # The per-match delta is recorded on each participation row. deltas = { p.user_sub: p.elo_delta for p in await MatchPlayer.all() } - self.assertEqual({"alice": 16, "bob": -16}, deltas) + self.assertEqual({ALICE: 16, BOB: -16}, deltas) + + @async_test + async def test_same_sub_at_two_providers_has_independent_stats(self) -> None: + from tavolo.platform import GameSession, Seat + + _, _, tortoise_mixin = make_platform() + ctx = await use_db(tortoise_mixin) + engine = DummyEngine() + google_alice = make_principal("alice", "google") + session = GameSession( + id="cross-provider-stats", + game_type=engine.id, + join_code="XPROV1", + creator_sub=ALICE, + players=[Seat(user_sub=ALICE, display_name="Alice", team="A")], + ) + engine.create(session, {"target": 1}) + engine.join(session, google_alice, "Alice") + engine.handle_action(session, ALICE, "play", {}) + + with ctx: + await save_match_result(session, engine) + self.assertEqual( + {ALICE, google_alice}, + {row.user_sub for row in await MatchPlayer.all()}, + ) + self.assertEqual( + {ALICE, google_alice}, + {row.user_sub for row in await PlayerRating.all()}, + ) @async_test async def test_elo_ratings_accumulate_across_matches(self) -> None: _, _, tortoise_mixin = make_platform() ctx = await use_db(tortoise_mixin) engine, session = _finished_session() - engine.handle_action(session, "alice", "play", {}) + engine.handle_action(session, ALICE, "play", {}) from tavolo.platform import GameSession, Seat @@ -105,14 +138,14 @@ class SaveMatchResultTest(unittest.TestCase): id="stats-game-2", game_type="dummy", join_code="STATS2", - creator_sub="alice", - players=[Seat(user_sub="alice", display_name="alice", team="A")], + creator_sub=ALICE, + players=[Seat(user_sub=ALICE, display_name="alice", team="A")], ) engine2.create(session2, {"target": 2}) - engine2.join(session2, "bob", "bob") + engine2.join(session2, BOB, "bob") # Bob wins the second match. - engine2.handle_action(session2, "bob", "play", {}) - engine2.handle_action(session2, "bob", "play", {}) + engine2.handle_action(session2, BOB, "play", {}) + engine2.handle_action(session2, BOB, "play", {}) with ctx: await save_match_result(session, engine) @@ -123,9 +156,9 @@ class SaveMatchResultTest(unittest.TestCase): } # Match 1: even teams, alice wins (+16/-16). Match 2: alice # is now the favourite (1516 vs 1484), so losing costs 17. - self.assertEqual(INITIAL_RATING - 1, ratings["alice"]) - self.assertEqual(INITIAL_RATING + 1, ratings["bob"]) - bob = await PlayerRating.get(user_sub="bob") + self.assertEqual(INITIAL_RATING - 1, ratings[ALICE]) + self.assertEqual(INITIAL_RATING + 1, ratings[BOB]) + bob = await PlayerRating.get(user_sub=BOB) self.assertEqual(2, bob.matches_played) @async_test @@ -160,11 +193,12 @@ async def _seed_two_matches( result={"winner": winner, "plays": 3 + index}, ) for seat, sub in enumerate(("alice", "bob")): + principal = make_principal(sub) score = 2.0 + index if sub == winner else 1.0 await MatchPlayer.create( id=uuid.uuid4(), match=match, - user_sub=sub, + user_sub=principal, display_name=sub, seat=seat, team="A" if seat == 0 else "B", @@ -172,22 +206,22 @@ async def _seed_two_matches( score=score, ) aggregate = aggregates.setdefault( - (sub, game_types[index]), - {"matches": 0, "wins": 0, "points": 0.0}, + (principal, game_types[index]), + {"matches": 0, "wins": 0, "points": 0.0, "display_name": sub}, ) aggregate["matches"] += 1 aggregate["wins"] += 1 if sub == winner else 0 aggregate["points"] += score - for (sub, game_type), aggregate in aggregates.items(): + for (principal, game_type), aggregate in aggregates.items(): await PlayerRating.create( id=uuid.uuid4(), - user_sub=sub, + user_sub=principal, game_type=game_type, rating=INITIAL_RATING, matches_played=aggregate["matches"], wins=aggregate["wins"], points=aggregate["points"], - display_name=sub, + display_name=aggregate["display_name"], ) @@ -266,13 +300,13 @@ class StatsRouteTest(unittest.TestCase): response = await client.get("/api/leaderboard") self.assertEqual(200, response.status_code) by_sub = {row["user_sub"]: row for row in response.json()["results"]} - self.assertEqual(2, by_sub["alice"]["matches"]) - self.assertEqual(1, by_sub["alice"]["wins"]) # alice won match 1 - self.assertEqual(3.0, by_sub["alice"]["points"]) # 2.0 + 1.0 - self.assertEqual(1, by_sub["bob"]["wins"]) # bob won match 2 - self.assertEqual(4.0, by_sub["bob"]["points"]) # 1.0 + 3.0 + self.assertEqual(2, by_sub[ALICE]["matches"]) + self.assertEqual(1, by_sub[ALICE]["wins"]) # alice won match 1 + self.assertEqual(3.0, by_sub[ALICE]["points"]) # 2.0 + 1.0 + self.assertEqual(1, by_sub[BOB]["wins"]) # bob won match 2 + self.assertEqual(4.0, by_sub[BOB]["points"]) # 1.0 + 3.0 # Bob leads on points after tying Alice on wins. - self.assertEqual("bob", response.json()["results"][0]["user_sub"]) + self.assertEqual(BOB, response.json()["results"][0]["user_sub"]) @async_test async def test_leaderboard_includes_elo_and_sorts_by_it(self) -> None: @@ -282,7 +316,7 @@ class StatsRouteTest(unittest.TestCase): with ctx: # Alice outranks everyone despite Bob leading on points. await PlayerRating.filter( - user_sub="alice", game_type="dummy" + user_sub=ALICE, game_type="dummy" ).update(rating=1600) transport = ASGITransport(app=app) async with AsyncClient(transport=transport, base_url="http://127.0.0.1") as client: @@ -290,10 +324,10 @@ class StatsRouteTest(unittest.TestCase): self.assertEqual(200, response.status_code) results = response.json()["results"] by_sub = {row["user_sub"]: row for row in results} - self.assertEqual(1600, by_sub["alice"]["elo"]) - self.assertEqual(INITIAL_RATING, by_sub["bob"]["elo"]) + self.assertEqual(1600, by_sub[ALICE]["elo"]) + self.assertEqual(INITIAL_RATING, by_sub[BOB]["elo"]) # Elo outranks wins/points. - self.assertEqual("alice", results[0]["user_sub"]) + self.assertEqual(ALICE, results[0]["user_sub"]) @async_test async def test_leaderboard_elo_scoped_by_game_type(self) -> None: @@ -304,13 +338,13 @@ class StatsRouteTest(unittest.TestCase): ctx = await use_db(tortoise_mixin) with ctx: await PlayerRating.filter( - user_sub="bob", game_type="dummy" + user_sub=BOB, game_type="dummy" ).update(rating=1516) # Bob's rating in another game must not leak into the # dummy leaderboard. await PlayerRating.create( id=uuid.uuid4(), - user_sub="bob", + user_sub=BOB, game_type="second", rating=1800, matches_played=1, @@ -321,8 +355,8 @@ class StatsRouteTest(unittest.TestCase): self.assertEqual(200, response.status_code) results = response.json()["results"] by_sub = {row["user_sub"]: row for row in results} - self.assertEqual(1516, by_sub["bob"]["elo"]) - self.assertEqual(INITIAL_RATING, by_sub["alice"]["elo"]) + self.assertEqual(1516, by_sub[BOB]["elo"]) + self.assertEqual(INITIAL_RATING, by_sub[ALICE]["elo"]) self.assertEqual("second", platform.registry.all()[1].id) @async_test @@ -334,7 +368,7 @@ class StatsRouteTest(unittest.TestCase): for index, sub in enumerate(("carol", "dave", "erin", "frank")): await PlayerRating.create( id=uuid.uuid4(), - user_sub=sub, + user_sub=make_principal(sub), game_type="dummy", rating=1600 + index, matches_played=1, @@ -354,20 +388,21 @@ class StatsRouteTest(unittest.TestCase): self.assertEqual(6, body["total"]) self.assertEqual(2, len(body["results"])) self.assertEqual( - ["frank", "erin"], [row["user_sub"] for row in body["results"]] + [make_principal("frank"), make_principal("erin")], + [row["user_sub"] for row in body["results"]], ) self.assertEqual( - ["dave", "carol"], + [make_principal("dave"), make_principal("carol")], [row["user_sub"] for row in second.json()["results"]], ) self.assertEqual( - ["bob", "alice"], + [BOB, ALICE], [row["user_sub"] for row in third.json()["results"]], ) # Out-of-range pages clamp to the last page. self.assertEqual(3, beyond.json()["page"]) self.assertEqual( - ["bob", "alice"], + [BOB, ALICE], [row["user_sub"] for row in beyond.json()["results"]], ) @@ -380,7 +415,7 @@ class StatsRouteTest(unittest.TestCase): for index, sub in enumerate(("carol", "dave")): await PlayerRating.create( id=uuid.uuid4(), - user_sub=sub, + user_sub=make_principal(sub), game_type="dummy", rating=1600 + index, matches_played=1, @@ -398,7 +433,7 @@ class StatsRouteTest(unittest.TestCase): self.assertEqual(4, body["page"]) self.assertEqual(4, body["your_rank"]) self.assertEqual(4, body["your_page"]) - self.assertEqual("alice", body["results"][0]["user_sub"]) + self.assertEqual(ALICE, body["results"][0]["user_sub"]) anonymous_body = anonymous.json() self.assertEqual(1, anonymous_body["page"]) self.assertIsNone(anonymous_body["your_rank"]) @@ -421,7 +456,7 @@ class StatsRouteTest(unittest.TestCase): app, platform, tortoise_mixin = make_platform() ctx = await use_db(tortoise_mixin) engine, session = _finished_session() - engine.handle_action(session, "alice", "play", {}) + engine.handle_action(session, ALICE, "play", {}) with ctx: await save_match_result(session, engine) transport = ASGITransport(app=app) @@ -433,10 +468,37 @@ class StatsRouteTest(unittest.TestCase): p["user_sub"]: p for p in response.json()["results"][0]["players"] } - self.assertEqual(16, players["alice"]["elo_delta"]) - self.assertEqual(-16, players["bob"]["elo_delta"]) + self.assertEqual(16, players[ALICE]["elo_delta"]) + self.assertEqual(-16, players[BOB]["elo_delta"]) self.assertEqual(16, response.json()["results"][0]["your_elo_delta"]) + @async_test + async def test_same_sub_other_provider_cannot_read_history_or_ratings(self) -> None: + app, platform, tortoise_mixin = make_platform( + oidc_providers=("default", "google") + ) + ctx = await use_db(tortoise_mixin) + engine, session = _finished_session() + engine.handle_action(session, ALICE, "play", {}) + with ctx: + await save_match_result(session, engine) + + transport = ASGITransport(app=app) + async with AsyncClient(transport=transport, base_url="http://127.0.0.1") as client: + with oidc_user(platform.oidc, "alice", provider="google"): + matches = await client.get("/api/me/matches") + ratings = await client.get("/api/me/ratings") + board = await client.get("/api/leaderboard?focus=me") + self.assertEqual([], matches.json()["results"]) + self.assertEqual([], ratings.json()["results"]) + self.assertIsNone(board.json()["your_rank"]) + + with oidc_user(platform.oidc, "alice"): + owned_matches = await client.get("/api/me/matches") + owned_ratings = await client.get("/api/me/ratings") + self.assertEqual(1, len(owned_matches.json()["results"])) + self.assertEqual(1, len(owned_ratings.json()["results"])) + @async_test async def test_my_ratings_requires_auth(self) -> None: app, _, _ = make_platform() @@ -452,14 +514,14 @@ class StatsRouteTest(unittest.TestCase): with ctx: await PlayerRating.create( id=uuid.uuid4(), - user_sub="alice", + user_sub=ALICE, game_type="dummy", rating=1516, matches_played=1, ) await PlayerRating.create( id=uuid.uuid4(), - user_sub="bob", + user_sub=BOB, game_type="dummy", rating=1484, matches_played=1, @@ -516,9 +578,9 @@ class GameTypeFilterTest(unittest.TestCase): self.assertEqual(200, scoped.status_code) by_sub = {row["user_sub"]: row for row in scoped.json()["results"]} # Only the first match counts: one match per player, alice won. - self.assertEqual(1, by_sub["alice"]["matches"]) - self.assertEqual(1, by_sub["alice"]["wins"]) - self.assertEqual(0, by_sub["bob"]["wins"]) + self.assertEqual(1, by_sub[ALICE]["matches"]) + self.assertEqual(1, by_sub[ALICE]["wins"]) + self.assertEqual(0, by_sub[BOB]["wins"]) self.assertEqual(400, unknown.status_code) diff --git a/server/packages/tavolo-platform/tests/test_ws.py b/server/packages/tavolo-platform/tests/test_ws.py index da92256..ab313fc 100644 --- a/server/packages/tavolo-platform/tests/test_ws.py +++ b/server/packages/tavolo-platform/tests/test_ws.py @@ -10,7 +10,7 @@ from httpx_ws import WebSocketDisconnect as HttpxWebSocketDisconnect, aconnect_w from httpx_ws.transport import ASGIWebSocketTransport from kaya.core import WebSocket, WebSocketDisconnect, WebSocketMessage -from helpers import async_test, make_platform, make_user, oidc_user, ws_users +from helpers import async_test, make_identity, make_platform, make_user, oidc_user, ws_users class AbruptCloseSocket(WebSocket): @@ -288,6 +288,27 @@ class WebSocketTest(unittest.TestCase): self.assertEqual("error", error["type"]) self.assertEqual("forbidden", error["code"]) + @async_test + async def test_same_sub_other_provider_is_a_spectator(self) -> None: + app, platform, _ = make_platform(oidc_providers=("default", "google")) + api_transport = ASGITransport(app=app) + async with AsyncClient(transport=api_transport, base_url="http://127.0.0.1") as client: + state = await self._started_game(client, platform.oidc) + game_id = state["id"] + + ws_transport = ASGIWebSocketTransport(app=app) + async with AsyncClient(transport=ws_transport, base_url="http://testserver") as ws_client: + with ws_users([make_identity("alice", provider="google")]): + async with aconnect_ws(f"/ws/games/{game_id}", ws_client) as ws: + first = await ws.receive_json() + self.assertTrue(first["game"]["spectator"]) + self.assertFalse(first["game"]["viewer_seated"]) + self.assertNotIn("join_code", first["game"]) + await ws.send_json({"action": "play"}) + error = await ws.receive_json() + self.assertEqual("error", error["type"]) + self.assertEqual("forbidden", error["code"]) + @async_test async def test_anonymous_user_watches_as_spectator(self) -> None: app, platform, _ = make_platform() diff --git a/server/tests/helpers/__init__.py b/server/tests/helpers/__init__.py index 818336c..39b4d80 100644 --- a/server/tests/helpers/__init__.py +++ b/server/tests/helpers/__init__.py @@ -1,5 +1,5 @@ """Test helpers package for the tavolo-app integration suite.""" from .asynctest import async_test -from .oidc import make_user, oidc_user, ws_users +from .oidc import make_principal, make_user, oidc_user, ws_users -__all__ = ["async_test", "make_user", "oidc_user", "ws_users"] +__all__ = ["async_test", "make_principal", "make_user", "oidc_user", "ws_users"] diff --git a/server/tests/helpers/oidc.py b/server/tests/helpers/oidc.py index 2652395..c537ea8 100644 --- a/server/tests/helpers/oidc.py +++ b/server/tests/helpers/oidc.py @@ -17,6 +17,7 @@ from kaya.oidc import OIDCUser # ``tavolo.platform.auth`` themselves. from tavolo.app import oidc from tavolo.platform import auth +from tavolo.platform.auth import AuthenticatedUser, canonical_principal # The app's first configured provider (the test environment configures one # named "default"). @@ -37,12 +38,17 @@ def make_user( return OIDCUser(claims) +def make_principal(sub: str, provider: str = _PROVIDER) -> str: + return canonical_principal(provider, sub) + + @contextlib.contextmanager def oidc_user( sub: str, name: Optional[str] = None, email: Optional[str] = None, picture: Optional[str] = None, + provider: Optional[str] = None, ) -> Iterator[OIDCUser]: """Context manager: make ``oidc.get_users`` return this user. @@ -52,7 +58,8 @@ def oidc_user( scope's avatar claim. """ user = make_user(sub, name, email, picture) - patcher = _mock.patch.object(oidc, "get_users", return_value={_PROVIDER: user}) + selected = provider or _PROVIDER + patcher = _mock.patch.object(oidc, "get_users", return_value={selected: user}) patcher.start() try: yield user @@ -65,7 +72,14 @@ def ws_users(users: Sequence[OIDCUser]) -> Iterator[None]: """Context manager: patch ``auth.get_ws_user`` to hand out ``users`` one per websocket connection, in order. Once exhausted it keeps returning the last user.""" - remaining = list(users) + remaining = [ + AuthenticatedUser( + provider=_PROVIDER, + claims=user, + principal=canonical_principal(_PROVIDER, user.sub), + ) + for user in users + ] last = remaining[-1] if remaining else None def _next(_ws): diff --git a/server/tests/test_briscola.py b/server/tests/test_briscola.py index 86b921e..8f3aa8c 100644 --- a/server/tests/test_briscola.py +++ b/server/tests/test_briscola.py @@ -23,7 +23,7 @@ from tavolo.briscola.state import ( PlayerState, ) from tavolo.platform import GameSession, Seat -from tests.helpers import async_test, make_user, oidc_user, ws_users +from tests.helpers import async_test, make_principal, make_user, oidc_user, ws_users class BriscolaRoutesTest(unittest.TestCase): @@ -235,14 +235,14 @@ class BriscolaTurnTimeoutTest(unittest.TestCase): id="briscola-turn-timeout-1", game_type=engine.id, join_code="BT0001", - creator_sub="alice", + creator_sub=make_principal("alice"), players=[ - Seat(user_sub="alice", display_name="Alice", team="A"), + Seat(user_sub=make_principal("alice"), display_name="Alice", team="A"), ], ) engine.create(session, {}) session.state.turn_timeout = 1 - engine.join(session, "bob", "Bob") + engine.join(session, make_principal("bob"), "Bob") await game_store.save(session) ws_transport = ASGIWebSocketTransport(app=app) @@ -283,10 +283,10 @@ async def _seed_hand_end_state() -> str: id="briscola-hand-end-1", game_type=engine.id, join_code="BH0001", - creator_sub="alice", + creator_sub=make_principal("alice"), players=[ - Seat(user_sub="alice", display_name="Alice", team="A"), - Seat(user_sub="bob", display_name="Bob", team="B"), + Seat(user_sub=make_principal("alice"), display_name="Alice", team="A"), + Seat(user_sub=make_principal("bob"), display_name="Bob", team="B"), ], ) session.state = BriscolaState( @@ -298,11 +298,11 @@ async def _seed_hand_end_state() -> str: briscola_suit="S", players=[ PlayerState( - sub="alice", name="Alice", seat=0, + sub=make_principal("alice"), name="Alice", seat=0, hand=[Card.parse("02D")], captured=[Card.parse("01C")], ), PlayerState( - sub="bob", name="Bob", seat=1, + sub=make_principal("bob"), name="Bob", seat=1, hand=[Card.parse("05D")], captured=[Card.parse("10C")], ), ], diff --git a/server/tests/test_briscola_chiamata.py b/server/tests/test_briscola_chiamata.py index 9be273e..316d110 100644 --- a/server/tests/test_briscola_chiamata.py +++ b/server/tests/test_briscola_chiamata.py @@ -23,7 +23,7 @@ from tavolo.briscola.chiamata import ( Bid, ) from tavolo.platform import GameSession, Seat -from tests.helpers import async_test, make_user, oidc_user, ws_users +from tests.helpers import async_test, make_principal, make_user, oidc_user, ws_users PLAYERS = ("alice", "bob", "carol", "dave", "erin") @@ -163,7 +163,7 @@ class ChiamataWebSocketTest(unittest.TestCase): session = await game_store.load(game_id) assert session is not None for sub in ("carol", "dave", "erin", "alice"): - engine.handle_action(session, sub, "pass", {}) + engine.handle_action(session, make_principal(sub), "pass", {}) await game_store.save(session) self.assertEqual(PHASE_CALLING, session.state.phase) self.assertEqual(1, session.state.caller) @@ -227,15 +227,17 @@ class ChiamataTimeoutTest(unittest.TestCase): id=game_id, game_type="briscola_chiamata", join_code=join_code, - creator_sub=PLAYERS[0], + creator_sub=make_principal(PLAYERS[0]), players=[ - Seat(user_sub=name, display_name=name.capitalize()) + Seat(user_sub=make_principal(name), display_name=name.capitalize()) for name in PLAYERS ], ) - session.state = rules.create_game(PLAYERS[0], "Alice", turn_timeout=1) + session.state = rules.create_game( + make_principal(PLAYERS[0]), "Alice", turn_timeout=1 + ) for name in PLAYERS[1:]: - rules.join_game(session.state, name, name.capitalize()) + rules.join_game(session.state, make_principal(name), name.capitalize()) state_mutator(session.state) await game_store.save(session) return session.id diff --git a/server/tests/test_routes_games.py b/server/tests/test_routes_games.py index c401a9a..823e8b6 100644 --- a/server/tests/test_routes_games.py +++ b/server/tests/test_routes_games.py @@ -6,7 +6,7 @@ import unittest from httpx import ASGITransport, AsyncClient from tavolo.app import app -from tests.helpers import async_test, oidc_user +from tests.helpers import async_test, make_principal, oidc_user class GamesRouteTest(unittest.TestCase): @@ -37,7 +37,7 @@ class GamesRouteTest(unittest.TestCase): with oidc_user("alice"): snapshot = await client.get(f"/api/games/{game_id}") self.assertEqual(200, snapshot.status_code) - self.assertEqual("alice", snapshot.json()["players"][0]["sub"]) + self.assertEqual(make_principal("alice"), snapshot.json()["players"][0]["sub"]) # A logged-in non-player is a read-only spectator: no join # code, and the view is marked as a spectator view. @@ -98,6 +98,32 @@ class GamesRouteTest(unittest.TestCase): self.assertEqual(0, participant["captured_count"]) self.assertEqual(0, participant["scope"]) + @async_test + async def test_same_sub_other_provider_cannot_read_private_hand(self) -> None: + transport = ASGITransport(app=app) + async with AsyncClient(transport=transport, base_url="http://127.0.0.1") as client: + with oidc_user("alice"): + created = await client.post("/api/games", json={}) + code = created.json()["join_code"] + game_id = created.json()["id"] + for player in ("bob", "carol", "dave"): + with oidc_user(player): + await client.post("/api/games/join", json={"code": code}) + + with oidc_user("alice", provider="google"): + watched = await client.get(f"/api/games/{game_id}") + self.assertEqual(200, watched.status_code) + spectator = watched.json() + self.assertTrue(spectator["spectator"]) + self.assertNotIn("join_code", spectator) + for participant in spectator["players"]: + self.assertNotIn("hand", participant) + + with oidc_user("alice"): + owned = await client.get(f"/api/games/{game_id}") + self.assertFalse(owned.json()["spectator"]) + self.assertIn("hand", owned.json()["players"][0]) + @async_test async def test_create_napola_option(self) -> None: transport = ASGITransport(app=app) @@ -180,7 +206,7 @@ class GamesRouteTest(unittest.TestCase): self.assertFalse(body["napola"]) self.assertTrue(body["asso_pigliatutto"]) self.assertEqual(3, body["seats_open"]) - self.assertEqual(["alice"], [p["sub"] for p in body["players"]]) + self.assertEqual([make_principal("alice")], [p["sub"] for p in body["players"]]) for player in ("bob", "carol", "dave"): with oidc_user(player): diff --git a/server/tests/test_routes_me.py b/server/tests/test_routes_me.py index a829043..2db782c 100644 --- a/server/tests/test_routes_me.py +++ b/server/tests/test_routes_me.py @@ -11,7 +11,7 @@ from httpx import ASGITransport, AsyncClient from tavolo.app import app from tavolo.config import settings -from tests.helpers import async_test, oidc_user +from tests.helpers import async_test, make_principal, oidc_user class MeRouteTest(unittest.TestCase): @@ -25,7 +25,7 @@ class MeRouteTest(unittest.TestCase): # The email claim the IdP returns is neither stored nor exposed; # no picture claim in this request. self.assertEqual( - {"sub": "alice", "name": "alice", "picture": None}, + {"sub": make_principal("alice"), "name": "alice", "picture": None}, response.json(), ) diff --git a/server/tests/test_scopa.py b/server/tests/test_scopa.py index 13eb287..89e5b40 100644 --- a/server/tests/test_scopa.py +++ b/server/tests/test_scopa.py @@ -23,7 +23,7 @@ from tavolo.scopa.state import ( ScopaState, ) from tavolo.platform import GameSession, Seat -from tests.helpers import async_test, make_user, oidc_user, ws_users +from tests.helpers import async_test, make_principal, make_user, oidc_user, ws_users class ScopaRoutesTest(unittest.TestCase): @@ -249,11 +249,11 @@ class ScopaWebSocketTest(unittest.TestCase): turn=0, players=[ PlayerState( - sub="alice", name="Alice", seat=0, + sub=make_principal("alice"), name="Alice", seat=0, hand=[Card.parse("01D"), Card.parse("09C")], ), PlayerState( - sub="bob", name="Bob", seat=1, + sub=make_principal("bob"), name="Bob", seat=1, hand=[Card.parse("04D")], ), ], @@ -264,10 +264,10 @@ class ScopaWebSocketTest(unittest.TestCase): id="scopa-asso-pigliatutto-1", game_type="scopa", join_code="SA0001", - creator_sub="alice", + creator_sub=make_principal("alice"), players=[ - Seat(user_sub="alice", display_name="Alice", team="A"), - Seat(user_sub="bob", display_name="Bob", team="B"), + Seat(user_sub=make_principal("alice"), display_name="Alice", team="A"), + Seat(user_sub=make_principal("bob"), display_name="Bob", team="B"), ], ) session.state = state @@ -304,10 +304,10 @@ class ScopaWebSocketTest(unittest.TestCase): turn=0, players=[ PlayerState( - sub="alice", name="Alice", seat=0, + sub=make_principal("alice"), name="Alice", seat=0, hand=[Card.parse("02D")], ), - PlayerState(sub="bob", name="Bob", seat=1), + PlayerState(sub=make_principal("bob"), name="Bob", seat=1), ], table=[Card.parse("02C")], ) @@ -315,10 +315,10 @@ class ScopaWebSocketTest(unittest.TestCase): id="scopa-hand-end-1", game_type="scopa", join_code="SH0001", - creator_sub="alice", + creator_sub=make_principal("alice"), players=[ - Seat(user_sub="alice", display_name="Alice", team="A"), - Seat(user_sub="bob", display_name="Bob", team="B"), + Seat(user_sub=make_principal("alice"), display_name="Alice", team="A"), + Seat(user_sub=make_principal("bob"), display_name="Bob", team="B"), ], ) session.state = state @@ -374,14 +374,14 @@ class ScopaTurnTimeoutTest(unittest.TestCase): id="scopa-turn-timeout-1", game_type=engine.id, join_code="ST0001", - creator_sub="alice", + creator_sub=make_principal("alice"), players=[ - Seat(user_sub="alice", display_name="Alice", team="A"), + Seat(user_sub=make_principal("alice"), display_name="Alice", team="A"), ], ) engine.create(session, {}) session.state.turn_timeout = 1 - engine.join(session, "bob", "Bob") + engine.join(session, make_principal("bob"), "Bob") await game_store.save(session) ws_transport = ASGIWebSocketTransport(app=app) diff --git a/server/tests/test_stats.py b/server/tests/test_stats.py index ff7491b..a539862 100644 --- a/server/tests/test_stats.py +++ b/server/tests/test_stats.py @@ -21,7 +21,7 @@ from tavolo.platform.models import Match, MatchPlayer, PlayerRating from tavolo.platform.stats import save_match_result from tavolo.scopa import engine as scopa_rules from tavolo.scopone import engine as rules -from tests.helpers import async_test, oidc_user +from tests.helpers import async_test, make_principal, oidc_user PLAYERS = ("alice", "bob", "carol", "dave") @@ -32,12 +32,12 @@ async def _finished_session(target_score: int = 1) -> GameSession: id="stats-scope-1", game_type=engine.id, join_code="SS0001", - creator_sub="alice", - players=[Seat(user_sub="alice", display_name="Alice")], + creator_sub=make_principal("alice"), + players=[Seat(user_sub=make_principal("alice"), display_name="Alice")], ) engine.create(session, {"target_score": target_score}) for name in PLAYERS[1:]: - engine.join(session, name, name.capitalize()) + engine.join(session, make_principal(name), name.capitalize()) moves = 0 while not engine.is_finished(session) and moves < 200000: state = session.state @@ -160,11 +160,11 @@ async def _finished_briscola_session() -> GameSession: id="stats-briscola-1", game_type=engine.id, join_code="BS0001", - creator_sub="alice", - players=[Seat(user_sub="alice", display_name="Alice")], + creator_sub=make_principal("alice"), + players=[Seat(user_sub=make_principal("alice"), display_name="Alice")], ) engine.create(session, {}) - engine.join(session, "bob", "Bob") + engine.join(session, make_principal("bob"), "Bob") moves = 0 while not engine.is_finished(session) and moves < 200000: state = session.state @@ -246,15 +246,15 @@ async def _finished_chiamata_session() -> GameSession: id="stats-chiamata-1", game_type=engine.id, join_code="BC0001", - creator_sub="alice", + creator_sub=make_principal("alice"), players=[ - Seat(user_sub=name, display_name=name.capitalize()) + Seat(user_sub=make_principal(name), display_name=name.capitalize()) for name in ("alice", "bob", "carol", "dave", "erin") ], ) engine.create(session, {}) for name in ("bob", "carol", "dave", "erin"): - engine.join(session, name, name.capitalize()) + engine.join(session, make_principal(name), name.capitalize()) moves = 0 while not engine.is_finished(session) and moves < 200000: state = session.state @@ -344,15 +344,15 @@ class ScopaStatsTest(unittest.TestCase): id=f"stats-scopa-{player_count}", game_type=engine.id, join_code="SC0001", - creator_sub="alice", - players=[Seat(user_sub="alice", display_name="Alice")], + creator_sub=make_principal("alice"), + players=[Seat(user_sub=make_principal("alice"), display_name="Alice")], ) engine.create( session, {"target_score": 1, "asso_pigliatutto": asso_pigliatutto}, ) for name in PLAYERS[1:player_count]: - engine.join(session, name, name.capitalize()) + engine.join(session, make_principal(name), name.capitalize()) moves = 0 while not engine.is_finished(session) and moves < 200000: state = session.state diff --git a/server/tests/test_telemetry.py b/server/tests/test_telemetry.py index 713c26d..559dbc6 100644 --- a/server/tests/test_telemetry.py +++ b/server/tests/test_telemetry.py @@ -25,7 +25,7 @@ from tavolo.telemetry import ( ) from tavolo.config import Settings -from tests.helpers import async_test +from tests.helpers import async_test, make_principal try: from opentelemetry.sdk._logs import LoggingHandler, LoggerProvider @@ -158,7 +158,7 @@ class SessionTagHookTest(unittest.TestCase): hashlib.sha256(b"sid").hexdigest()[:16], span.attributes["session.id"], ) - self.assertEqual("alice", span.attributes["user.id"]) + self.assertEqual(make_principal("alice"), span.attributes["user.id"]) def test_tags_user_of_any_provider(self) -> None: span = _RecordedSpan() @@ -166,7 +166,7 @@ class SessionTagHookTest(unittest.TestCase): session=Session("sid", {"oidc:google:user": {"sub": "alice"}}) ) session_tag_hook(span, carrier) - self.assertEqual("alice", span.attributes["user.id"]) + self.assertEqual(make_principal("alice", "google"), span.attributes["user.id"]) def test_ignores_legacy_unprefixed_user_key(self) -> None: span = _RecordedSpan() @@ -190,7 +190,7 @@ class SessionTagHookTest(unittest.TestCase): ) session_tag_hook(span, carrier) self.assertNotIn("session.id", span.attributes) - self.assertEqual("alice", span.attributes["user.id"]) + self.assertEqual(make_principal("alice"), span.attributes["user.id"]) def test_missing_session_is_a_noop(self) -> None: span = _RecordedSpan() @@ -266,7 +266,7 @@ class SessionTagIntegrationTest(unittest.TestCase): attributes = span.attributes assert attributes is not None self.assertEqual(digest, attributes["session.id"], name) - self.assertEqual("alice", attributes["user.id"], name) + self.assertEqual(make_principal("alice"), attributes["user.id"], name) def _metric_points(reader) -> dict: diff --git a/server/tests/test_websocket.py b/server/tests/test_websocket.py index bf2e553..fc5f201 100644 --- a/server/tests/test_websocket.py +++ b/server/tests/test_websocket.py @@ -11,7 +11,7 @@ from httpx_ws.transport import ASGIWebSocketTransport from tavolo.app import app, game_store, platform from tavolo.platform import GameSession, Seat from tavolo.scopone.state import Card, PlayerState, ScoponeState -from tests.helpers import async_test, make_user, oidc_user, ws_users +from tests.helpers import async_test, make_principal, make_user, oidc_user, ws_users PLAYERS = ("alice", "bob", "carol", "dave") @@ -27,8 +27,8 @@ def _started_session( id=game_id, game_type=engine.id, join_code=code, - creator_sub="alice", - players=[Seat(user_sub="alice", display_name="Alice")], + creator_sub=make_principal("alice"), + players=[Seat(user_sub=make_principal("alice"), display_name="Alice")], ) engine.create( session, @@ -42,7 +42,7 @@ def _started_session( session.state.hand_ack_timeout = hand_ack_timeout session.state.turn_timeout = turn_timeout for name in PLAYERS[1:]: - engine.join(session, name, name.capitalize()) + engine.join(session, make_principal(name), name.capitalize()) return session @@ -83,7 +83,7 @@ class WebSocketTest(unittest.TestCase): alice_first = await alice_ws.receive_json() self.assertEqual("state", alice_first["type"]) self.assertEqual( - "alice", alice_first["game"]["players"][0]["sub"] + make_principal("alice"), alice_first["game"]["players"][0]["sub"] ) self.assertNotIn("hand", alice_first["game"]["players"][1]) @@ -189,12 +189,12 @@ async def _seed_last_play_state(hand_ack_timeout: int = 30) -> str: id="hand-end-1", game_type=engine.id, join_code="HEND01", - creator_sub="alice", + creator_sub=make_principal("alice"), players=[ - Seat(user_sub="alice", display_name="Alice", team="A"), - Seat(user_sub="bob", display_name="Bob", team="B"), - Seat(user_sub="carol", display_name="Carol", team="A"), - Seat(user_sub="dave", display_name="Dave", team="B"), + Seat(user_sub=make_principal("alice"), display_name="Alice", team="A"), + Seat(user_sub=make_principal("bob"), display_name="Bob", team="B"), + Seat(user_sub=make_principal("carol"), display_name="Carol", team="A"), + Seat(user_sub=make_principal("dave"), display_name="Dave", team="B"), ], ) session.state = ScoponeState( @@ -203,10 +203,10 @@ async def _seed_last_play_state(hand_ack_timeout: int = 30) -> str: turn=0, table=[Card.parse("02C")], players=[ - PlayerState(sub="alice", name="Alice", seat=0, hand=[Card.parse("02D")]), - PlayerState(sub="bob", name="Bob", seat=1), - PlayerState(sub="carol", name="Carol", seat=2), - PlayerState(sub="dave", name="Dave", seat=3), + PlayerState(sub=make_principal("alice"), name="Alice", seat=0, hand=[Card.parse("02D")]), + PlayerState(sub=make_principal("bob"), name="Bob", seat=1), + PlayerState(sub=make_principal("carol"), name="Carol", seat=2), + PlayerState(sub=make_principal("dave"), name="Dave", seat=3), ], hand_ack_timeout=hand_ack_timeout, ) diff --git a/web/src/components/avatar.rs b/web/src/components/avatar.rs index 2dba59c..e28a447 100644 --- a/web/src/components/avatar.rs +++ b/web/src/components/avatar.rs @@ -1,5 +1,6 @@ //! User avatar: the provider's `picture` claim when available, a -//! deterministic identicon generated from the OIDC `sub` otherwise. +//! deterministic identicon generated from the canonical user principal +//! otherwise. //! //! The identicon is a 5x5 mirrored pixel grid derived from an inline //! FNV-1a hash. The hash is implemented here (instead of @@ -62,8 +63,8 @@ fn safe_picture_url(url: &str) -> Option { } /// The user's avatar: their profile picture, or an identicon generated -/// from the `sub`. A picture that fails to load falls back to the -/// identicon as well. +/// from the canonical principal. A picture that fails to load falls back to +/// the identicon as well. pub fn avatar(sub: &str, picture: Option<&str>) -> View { let Some(url) = picture.and_then(safe_picture_url) else { return identicon(sub); diff --git a/web/src/model.rs b/web/src/model.rs index 96419f2..ea04441 100644 --- a/web/src/model.rs +++ b/web/src/model.rs @@ -9,6 +9,7 @@ fn default_true() -> bool { #[derive(Debug, Clone, Deserialize)] #[allow(dead_code)] pub struct User { + /// Canonical provider-qualified principal (`oidc:v1::`). pub sub: String, pub name: String, #[serde(default)] @@ -18,6 +19,7 @@ pub struct User { #[derive(Debug, Clone, Deserialize)] #[allow(dead_code)] pub struct PlayerView { + /// Canonical provider-qualified principal. pub sub: String, pub name: String, pub seat: usize, @@ -386,6 +388,7 @@ pub enum ServerMessage { #[derive(Debug, Clone, Deserialize)] #[allow(dead_code)] pub struct MatchPlayer { + /// Canonical provider-qualified principal (legacy API field name). pub user_sub: String, pub display_name: String, pub seat: usize, @@ -475,6 +478,7 @@ fn default_page() -> u32 { #[derive(Debug, Clone, Deserialize)] #[allow(dead_code)] pub struct LeaderboardEntry { + /// Canonical provider-qualified principal (legacy API field name). pub user_sub: String, pub display_name: String, /// Chess-style Elo rating for the requested game type.