Addendum to Entra ID's... proclivities (#772)

* OIDC entra capture

* copilot being nitpicky

---------

Co-authored-by: pow3rtool <root@pow3rtools>

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()

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.
 

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")

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

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
             ]

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]:

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",

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
             ]

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")