From deff44bcea39bf19cd95d811f6534814d9c0a0ff Mon Sep 17 00:00:00 2001 From: metaclassing <1027733+metaclassing@users.noreply.github.com> Date: Sat, 4 Jul 2026 18:48:52 -0500 Subject: [PATCH] Addendum to Entra ID's... proclivities (#772) * OIDC entra capture * copilot being nitpicky --------- Co-authored-by: pow3rtool --- tests/test_migration_065.py | 104 ++++++++++++++++++ tests/test_oidc.py | 59 ++++++++++ tests/test_oidc_storage.py | 61 ++++++++++ turnstone/core/oidc.py | 15 ++- turnstone/core/storage/_postgresql.py | 25 ++++- turnstone/core/storage/_protocol.py | 12 +- turnstone/core/storage/_schema.py | 7 ++ turnstone/core/storage/_sqlite.py | 25 ++++- .../versions/065_oidc_identity_oid.py | 38 +++++++ 9 files changed, 339 insertions(+), 7 deletions(-) create mode 100644 tests/test_migration_065.py create mode 100644 turnstone/core/storage/migrations/versions/065_oidc_identity_oid.py diff --git a/tests/test_migration_065.py b/tests/test_migration_065.py new file mode 100644 index 00000000..0a322005 --- /dev/null +++ b/tests/test_migration_065.py @@ -0,0 +1,104 @@ +"""Tests for alembic migration 065 (capture Entra oid/tid on oidc_identities). + +Drives ``command.upgrade``/``downgrade`` against an isolated SQLite database per +test (the 060/062/063 harness pattern), then asserts: + +* upgrade adds the ``oid``/``tid`` columns and the ``idx_oidc_identities_oid`` + index; +* a pre-065 row migrates cleanly, gaining ``""`` for the new columns; +* downgrade removes the columns + index, returning ``oidc_identities`` to its + exact pre-065 shape — this pins the **clean-rollback** guarantee (the change + can be backed out with no orphaned state if the upstream PR is rejected). +""" + +from __future__ import annotations + +from pathlib import Path + +import sqlalchemy as sa +from alembic import command +from alembic.config import Config + +_MIGRATIONS_DIR = str( + Path(__file__).resolve().parent.parent / "turnstone" / "core" / "storage" / "migrations" +) + + +def _alembic_cfg(db_path: Path) -> Config: + cfg = Config() + cfg.set_main_option("script_location", _MIGRATIONS_DIR) + cfg.set_main_option("sqlalchemy.url", f"sqlite:///{db_path}") + return cfg + + +class TestMigration065: + def test_upgrade_adds_oid_tid_and_index(self, tmp_path: Path) -> None: + db_path = tmp_path / "065-up.db" + command.upgrade(_alembic_cfg(db_path), "065") + + engine = sa.create_engine(f"sqlite:///{db_path}") + try: + insp = sa.inspect(engine) + cols = {c["name"] for c in insp.get_columns("oidc_identities")} + assert {"oid", "tid"} <= cols + idx = {i["name"] for i in insp.get_indexes("oidc_identities")} + assert "idx_oidc_identities_oid" in idx + finally: + engine.dispose() + + def test_preexisting_row_migrates_with_empty_default(self, tmp_path: Path) -> None: + db_path = tmp_path / "065-default.db" + cfg = _alembic_cfg(db_path) + # Stop at 064, insert a pre-065 identity, THEN upgrade to 065. + command.upgrade(cfg, "064") + engine = sa.create_engine(f"sqlite:///{db_path}") + try: + with engine.begin() as conn: + conn.execute( + sa.text( + "INSERT INTO oidc_identities " + "(issuer, subject, user_id, email, created, last_login) " + "VALUES ('iss', 'sub', 'u1', '', " + "'2026-01-01T00:00:00', '2026-01-01T00:00:00')" + ) + ) + command.upgrade(cfg, "065") + with engine.connect() as conn: + row = conn.execute( + sa.text("SELECT oid, tid FROM oidc_identities WHERE subject = 'sub'") + ).fetchone() + assert row is not None + assert row[0] == "" and row[1] == "" + finally: + engine.dispose() + + def test_downgrade_removes_oid_tid_and_index(self, tmp_path: Path) -> None: + db_path = tmp_path / "065-down.db" + cfg = _alembic_cfg(db_path) + command.upgrade(cfg, "065") + command.downgrade(cfg, "064") + + engine = sa.create_engine(f"sqlite:///{db_path}") + try: + insp = sa.inspect(engine) + cols = {c["name"] for c in insp.get_columns("oidc_identities")} + assert "oid" not in cols and "tid" not in cols + idx = {i["name"] for i in insp.get_indexes("oidc_identities")} + assert "idx_oidc_identities_oid" not in idx + finally: + engine.dispose() + + def test_downgrade_then_upgrade_round_trip(self, tmp_path: Path) -> None: + """up -> down -> up must land cleanly (no leftover column/index conflict).""" + db_path = tmp_path / "065-roundtrip.db" + cfg = _alembic_cfg(db_path) + command.upgrade(cfg, "065") + command.downgrade(cfg, "064") + command.upgrade(cfg, "065") + + engine = sa.create_engine(f"sqlite:///{db_path}") + try: + cols = {c["name"] for c in sa.inspect(engine).get_columns("oidc_identities")} + assert {"oid", "tid"} <= cols + finally: + engine.dispose() diff --git a/tests/test_oidc.py b/tests/test_oidc.py index 6a929e7c..80540438 100644 --- a/tests/test_oidc.py +++ b/tests/test_oidc.py @@ -1643,6 +1643,65 @@ class TestProvisionOIDCUser: storage.assign_role.assert_not_called() + def test_provision_oidc_user_null_oid_tid_collapse_to_empty(self): + """A present-but-null oid/tid claim must store "" — never the string "None". + + `claims.get("oid", "")` returns None (not the "" default) when the key is + present with a JSON null, and str(None) == "None" would slip past both the + server_default and the truthy backfill guard, storing a bogus non-empty + sentinel that collides across every null-emitting user. New-user path. + """ + config = _make_config() + storage = _mock_storage() + storage.get_user.return_value = { + "user_id": "u-new", + "username": "bob", + "display_name": "Bob", + "password_hash": "!oidc", + } + + claims = {"sub": "sub-null", "preferred_username": "bob", "oid": None, "tid": None} + with patch("turnstone.core.oidc.uuid") as mock_uuid: + mock_uuid.uuid4.return_value = MagicMock(hex="u-new-hex-00000000000000000000") + provision_oidc_user(storage, config, claims) + + kwargs = storage.create_oidc_user.call_args.kwargs + assert kwargs["oid"] == "" + assert kwargs["tid"] == "" + + def test_provision_oidc_user_null_oid_tid_not_backfilled_existing(self): + """Existing-identity path: null oid/tid claims must not backfill "None". + + The truthy guard in update_oidc_identity_login only protects against ""; a + "None" produced by str(None) is truthy and would be written, clobbering a + real value captured on an earlier login. + """ + config = _make_config() + existing_user = { + "user_id": "u1", + "username": "alice", + "display_name": "Alice", + "password_hash": "!oidc", + } + existing_identity = { + "issuer": "https://idp.example.com", + "subject": "sub-123", + "user_id": "u1", + "email": "alice@example.com", + "created": "2024-01-01T00:00:00", + "last_login": "2024-01-01T00:00:00", + "oid": "obj-real", + "tid": "ten-real", + } + storage = _mock_storage(identity=existing_identity, user=existing_user) + + claims = {"sub": "sub-123", "email": "alice@example.com", "oid": None, "tid": None} + provision_oidc_user(storage, config, claims) + + kwargs = storage.update_oidc_identity_login.call_args.kwargs + assert kwargs["oid"] == "" + assert kwargs["tid"] == "" + def test_existing_identity_self_heals_zero_roles(self): """Existing identity user with zero roles -> safety-net assigns builtin-viewer. diff --git a/tests/test_oidc_storage.py b/tests/test_oidc_storage.py index f15b9d82..58c639f7 100644 --- a/tests/test_oidc_storage.py +++ b/tests/test_oidc_storage.py @@ -84,6 +84,40 @@ class TestCreateOIDCUser: assert identity is not None assert identity["user_id"] == "u-other" + def test_create_oidc_user_captures_oid_tid(self, db): + """Entra oid/tid are persisted and returned on the identity.""" + db.create_oidc_user( + user_id="u-oid", + username="carol", + display_name="Carol", + password_hash="!oidc", + issuer="https://idp.example.com", + subject="sub-oid", + email="carol@example.com", + oid="obj-123", + tid="tenant-abc", + ) + identity = db.get_oidc_identity("https://idp.example.com", "sub-oid") + assert identity is not None + assert identity["oid"] == "obj-123" + assert identity["tid"] == "tenant-abc" + + def test_create_oidc_user_oid_tid_default_empty(self, db): + """Omitting oid/tid (non-Entra IdP) stores "" — never NULL.""" + db.create_oidc_user( + user_id="u-noid", + username="dave", + display_name="Dave", + password_hash="!oidc", + issuer="https://idp.example.com", + subject="sub-noid", + email="dave@example.com", + ) + identity = db.get_oidc_identity("https://idp.example.com", "sub-noid") + assert identity is not None + assert identity["oid"] == "" + assert identity["tid"] == "" + # --------------------------------------------------------------------------- # OIDC Identity CRUD @@ -137,6 +171,33 @@ class TestOIDCIdentityCRUD: result = db.update_oidc_identity_login("https://idp.example.com", "sub-999") assert result is False + def test_update_oidc_identity_login_backfills_oid_tid(self, db): + """A login carrying oid/tid backfills them onto a pre-existing row.""" + db.create_oidc_identity("https://idp.example.com", "sub-bf", "u1", "a@example.com") + before = db.get_oidc_identity("https://idp.example.com", "sub-bf") + assert before is not None and before["oid"] == "" + + db.update_oidc_identity_login("https://idp.example.com", "sub-bf", oid="obj-9", tid="ten-9") + + after = db.get_oidc_identity("https://idp.example.com", "sub-bf") + assert after is not None + assert after["oid"] == "obj-9" + assert after["tid"] == "ten-9" + + def test_update_oidc_identity_login_omitted_does_not_clobber_oid_tid(self, db): + """A later login WITHOUT oid/tid must not wipe previously-captured values.""" + db.create_oidc_identity("https://idp.example.com", "sub-keep", "u1", "a@example.com") + db.update_oidc_identity_login( + "https://idp.example.com", "sub-keep", oid="obj-keep", tid="ten-keep" + ) + # Simulate a subsequent login where the token omitted oid/tid. + db.update_oidc_identity_login("https://idp.example.com", "sub-keep") + + identity = db.get_oidc_identity("https://idp.example.com", "sub-keep") + assert identity is not None + assert identity["oid"] == "obj-keep" + assert identity["tid"] == "ten-keep" + def test_list_oidc_identities_for_user(self, db): """Two identities for same user, list returns both.""" db.create_oidc_identity("https://idp1.example.com", "sub-A", "u1", "alice@idp1.com") diff --git a/turnstone/core/oidc.py b/turnstone/core/oidc.py index 1cd2da33..f95584dd 100644 --- a/turnstone/core/oidc.py +++ b/turnstone/core/oidc.py @@ -818,13 +818,24 @@ def provision_oidc_user( issuer = config.issuer sub = str(claims["sub"]) email = str(claims.get("email", "")) + # Entra `oid`+`tid` are the STABLE, cross-app user key. The `sub` above is + # pairwise (a different value per application), so it cannot correlate this + # user across services; oid+tid can. Captured here (present in every v2.0 ID + # token, no extra scope). "" for IdPs that don't emit them. + # `or ""` (not a get default) so a present-but-null claim collapses to the + # same "" sentinel — `.get(k, "")` returns None when the key is present with + # a JSON null, and str(None) would store the bogus non-empty value "None". + oid = str(claims.get("oid") or "") + tid = str(claims.get("tid") or "") display_name = str(claims.get("name", "") or claims.get("preferred_username", "") or email) # Try to find existing identity identity = storage.get_oidc_identity(issuer, sub) if identity is not None: user_id = identity["user_id"] - storage.update_oidc_identity_login(issuer, sub) + # Passing oid/tid backfills them onto identities created before this + # change, on the user's next login. + storage.update_oidc_identity_login(issuer, sub, oid=oid, tid=tid) desired_role_ids = apply_role_mapping(storage, user_id, claims, config) _ensure_default_role(storage, user_id, desired_role_ids) user: dict[str, str] | None = storage.get_user(user_id) @@ -845,6 +856,8 @@ def provision_oidc_user( issuer, sub, email, + oid=oid, + tid=tid, ) except StorageConflictError as exc: raise OIDCError(f"OIDC provisioning failed: {exc}") from exc diff --git a/turnstone/core/storage/_postgresql.py b/turnstone/core/storage/_postgresql.py index 7e300bd9..fef88c7b 100644 --- a/turnstone/core/storage/_postgresql.py +++ b/turnstone/core/storage/_postgresql.py @@ -5367,6 +5367,8 @@ class PostgreSQLBackend: issuer: str, subject: str, email: str, + oid: str = "", + tid: str = "", ) -> None: from turnstone.core.storage._protocol import StorageConflictError @@ -5392,6 +5394,8 @@ class PostgreSQLBackend: "email": email, "created": now, "last_login": now, + "oid": oid, + "tid": tid, }, ) except sa.exc.IntegrityError as exc: @@ -5441,6 +5445,8 @@ class PostgreSQLBackend: oidc_identities.c.email, oidc_identities.c.created, oidc_identities.c.last_login, + oidc_identities.c.oid, + oidc_identities.c.tid, ).where( (oidc_identities.c.issuer == issuer) & (oidc_identities.c.subject == subject) ) @@ -5453,19 +5459,30 @@ class PostgreSQLBackend: email=row[3], created=row[4], last_login=row[5], + oid=row[6], + tid=row[7], ) return None - def update_oidc_identity_login(self, issuer: str, subject: str) -> bool: + def update_oidc_identity_login( + self, issuer: str, subject: str, oid: str = "", tid: str = "" + ) -> bool: now = datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%S") + # last_login always; oid/tid only when supplied, so a login that omits + # them can't wipe a value captured on an earlier login. + values: dict[str, str] = {"last_login": now} + if oid: + values["oid"] = oid + if tid: + values["tid"] = tid with self._conn() as conn: result = conn.execute( sa.update(oidc_identities) .where( (oidc_identities.c.issuer == issuer) & (oidc_identities.c.subject == subject) ) - .values(last_login=now) + .values(**values) ) conn.commit() return result.rowcount > 0 @@ -5481,6 +5498,8 @@ class PostgreSQLBackend: oidc_identities.c.email, oidc_identities.c.created, oidc_identities.c.last_login, + oidc_identities.c.oid, + oidc_identities.c.tid, ) .where(oidc_identities.c.user_id == user_id) .order_by(oidc_identities.c.created.desc()) @@ -5493,6 +5512,8 @@ class PostgreSQLBackend: email=r[3], created=r[4], last_login=r[5], + oid=r[6], + tid=r[7], ) for r in rows ] diff --git a/turnstone/core/storage/_protocol.py b/turnstone/core/storage/_protocol.py index 3bf681cd..6e609a46 100644 --- a/turnstone/core/storage/_protocol.py +++ b/turnstone/core/storage/_protocol.py @@ -32,6 +32,10 @@ class OIDCIdentity(TypedDict): email: str created: str last_login: str + # Entra `oid`/`tid`: the stable cross-app user key (see oidc_identities schema). + # "" when the IdP did not supply them. + oid: str + tid: str class OIDCPendingState(TypedDict): @@ -947,6 +951,8 @@ class StorageBackend(Protocol): issuer: str, subject: str, email: str, + oid: str = "", + tid: str = "", ) -> None: """Atomically create a user row and bind their OIDC identity. @@ -972,8 +978,10 @@ class StorageBackend(Protocol): """Lookup turnstone user by OIDC issuer+subject. Returns dict or None.""" ... - def update_oidc_identity_login(self, issuer: str, subject: str) -> bool: - """Update last_login timestamp. Returns True if row existed.""" + def update_oidc_identity_login( + self, issuer: str, subject: str, oid: str = "", tid: str = "" + ) -> bool: + """Update last_login; backfill oid/tid when provided. Returns True if row existed.""" ... def list_oidc_identities_for_user(self, user_id: str) -> list[OIDCIdentity]: diff --git a/turnstone/core/storage/_schema.py b/turnstone/core/storage/_schema.py index e6215346..412707ce 100644 --- a/turnstone/core/storage/_schema.py +++ b/turnstone/core/storage/_schema.py @@ -923,10 +923,17 @@ oidc_identities = sa.Table( sa.Column("email", sa.Text, nullable=False, server_default=""), sa.Column("created", sa.Text, nullable=False), sa.Column("last_login", sa.Text, nullable=False), + # Entra ID: the immutable directory object id (`oid`) and tenant id (`tid`). + # Unlike `subject` (Azure `sub` is PAIRWISE — different per application), the + # oid+tid pair is stable across every app in the tenant, so external services + # can resolve this user by it. Populated on login; "" when the IdP omits them. + sa.Column("oid", sa.Text, nullable=False, server_default=""), + sa.Column("tid", sa.Text, nullable=False, server_default=""), sa.PrimaryKeyConstraint("issuer", "subject"), ) sa.Index("idx_oidc_identities_user_id", oidc_identities.c.user_id) +sa.Index("idx_oidc_identities_oid", oidc_identities.c.oid) oidc_pending_states = sa.Table( "oidc_pending_states", diff --git a/turnstone/core/storage/_sqlite.py b/turnstone/core/storage/_sqlite.py index 8d3a89a2..898ed6b5 100644 --- a/turnstone/core/storage/_sqlite.py +++ b/turnstone/core/storage/_sqlite.py @@ -5532,6 +5532,8 @@ class SQLiteBackend: issuer: str, subject: str, email: str, + oid: str = "", + tid: str = "", ) -> None: from turnstone.core.storage._protocol import StorageConflictError @@ -5560,6 +5562,8 @@ class SQLiteBackend: "email": email, "created": now, "last_login": now, + "oid": oid, + "tid": tid, }, ) except sa.exc.IntegrityError as exc: @@ -5602,6 +5606,8 @@ class SQLiteBackend: oidc_identities.c.email, oidc_identities.c.created, oidc_identities.c.last_login, + oidc_identities.c.oid, + oidc_identities.c.tid, ).where( (oidc_identities.c.issuer == issuer) & (oidc_identities.c.subject == subject) ) @@ -5614,19 +5620,30 @@ class SQLiteBackend: email=row[3], created=row[4], last_login=row[5], + oid=row[6], + tid=row[7], ) return None - def update_oidc_identity_login(self, issuer: str, subject: str) -> bool: + def update_oidc_identity_login( + self, issuer: str, subject: str, oid: str = "", tid: str = "" + ) -> bool: now = datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%S") + # last_login always; oid/tid only when supplied, so a login that omits + # them can't wipe a value captured on an earlier login. + values: dict[str, str] = {"last_login": now} + if oid: + values["oid"] = oid + if tid: + values["tid"] = tid with self._conn() as conn: result = conn.execute( sa.update(oidc_identities) .where( (oidc_identities.c.issuer == issuer) & (oidc_identities.c.subject == subject) ) - .values(last_login=now) + .values(**values) ) conn.commit() return result.rowcount > 0 @@ -5642,6 +5659,8 @@ class SQLiteBackend: oidc_identities.c.email, oidc_identities.c.created, oidc_identities.c.last_login, + oidc_identities.c.oid, + oidc_identities.c.tid, ) .where(oidc_identities.c.user_id == user_id) .order_by(oidc_identities.c.created.desc()) @@ -5654,6 +5673,8 @@ class SQLiteBackend: email=r[3], created=r[4], last_login=r[5], + oid=r[6], + tid=r[7], ) for r in rows ] diff --git a/turnstone/core/storage/migrations/versions/065_oidc_identity_oid.py b/turnstone/core/storage/migrations/versions/065_oidc_identity_oid.py new file mode 100644 index 00000000..71e129ea --- /dev/null +++ b/turnstone/core/storage/migrations/versions/065_oidc_identity_oid.py @@ -0,0 +1,38 @@ +"""Capture Entra `oid`/`tid` on oidc_identities. + +Adds the stable, cross-application user key to OIDC identity rows. The existing +`subject` column holds the OIDC `sub`, which on Microsoft Entra is a PAIRWISE +identifier — a different value in every application — so it cannot correlate a +user across services. `oid` (directory object id) + `tid` (tenant id) are stable +across all apps in the tenant and are what external services should match on. + +Both columns are nullable-free with a "" server default so existing rows migrate +cleanly; they are populated on each user's next login (see +`turnstone.core.oidc.provision_oidc_user`). Additive and reversible. + +Revision ID: 065 +Revises: 064 +Create Date: 2026-07-04 +""" + +import sqlalchemy as sa +from alembic import op + +revision = "065" +down_revision = "064" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + with op.batch_alter_table("oidc_identities") as batch_op: + batch_op.add_column(sa.Column("oid", sa.Text, nullable=False, server_default="")) + batch_op.add_column(sa.Column("tid", sa.Text, nullable=False, server_default="")) + op.create_index("idx_oidc_identities_oid", "oidc_identities", ["oid"]) + + +def downgrade() -> None: + op.drop_index("idx_oidc_identities_oid", table_name="oidc_identities") + with op.batch_alter_table("oidc_identities") as batch_op: + batch_op.drop_column("tid") + batch_op.drop_column("oid")