Fix OIDC subject collisions by introducing canonical provider-plus-subject principals
- Add canonical_principal(provider, sub) => 'oidc:v1:<provider>:<sub>' - 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
This commit is contained in:
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user