fix(oidc/console): unblock obo edits when OIDC off; latch config-invalid rediscovery

Round-8 review follow-up — two correctness follow-ons from the round-7
rediscovery/console-gate fixes, plus two cleanups.

- The console obo write gate ran the OIDC-deployment checks on EVERY
  update, so once OIDC was operator-disabled any edit of an existing
  oauth_obo server — including the natural remedy of setting
  enabled=false — was rejected 400, leaving DELETE as the only way out.
  The deployment-level checks (encryption key, OIDC enabled/configured,
  capture opt-in, valid grant profile) now run only when a write is a NEW
  obo enablement (create or flip INTO obo); a same-type edit keeps only
  the per-server validity checks (audience required, entra-scope reject),
  so an operator can always disable or edit an existing obo server.

- Probing rediscovery with enabled forced True carried the retryable boot
  flag into discover_oidc, whose config-error branches returned enabled=
  False without clearing it, so a config-invalid IdP (an endpoint failing
  SSRF/same-origin validation) re-probed every 60s forever. The config-
  error branches now latch discovery_retryable=False (terminal), and
  maybe_rediscover installs that terminal config so the node stops
  probing; the transient fetch/degraded branches keep retrying.

Cleanups: fold the obo missing-expires_in fallback into
_expires_at_from_response via a default_ttl_seconds param (one owner of
the stored-expiry format), and drop the redundant audience-change
inequality already guaranteed by the no-op normalization (matching the
sibling scopes_changing).

tests/test_mcp_admin_api.py

@@ -896,7 +896,10 @@ class TestUpdateMcpServer:
         assert r.status_code == 200, r.text
         assert r.json()["enabled"] is False
 
-        # A genuinely-absent OIDC (neither enabled nor retryable) still rejects.
+        # Even with OIDC fully operator-disabled (neither flag set), a same-type
+        # edit of the EXISTING obo server is still allowed — the deployment
+        # checks only fire on create / flip-into-obo, so an operator is never
+        # locked out of disabling or editing a server (review finding R8-1).
         client.app.state.oidc_config = SimpleNamespace(
             enabled=False,
             issuer="",
@@ -908,8 +911,32 @@ class TestUpdateMcpServer:
             "/v1/api/admin/mcp-servers/obo-retry-id",
             json={"enabled": True},
         )
-        assert r2.status_code == 400
-        assert "OIDC" in r2.json()["error"]
+        assert r2.status_code == 200, r2.text
+
+    def test_create_new_obo_still_rejected_when_oidc_operator_disabled(self, client):
+        """The deployment gate still fires for a NEW obo enablement: creating a
+        fresh oauth_obo server (or flipping one into obo) while OIDC is fully
+        operator-disabled is rejected — only same-type edits of an existing obo
+        server skip the deployment checks."""
+        client.app.state.oidc_config = SimpleNamespace(
+            enabled=False,
+            issuer="",
+            obo_grant_profile="entra",
+            capture_user_credential=True,
+            discovery_retryable=False,
+        )
+        r = client.post(
+            "/v1/api/admin/mcp-servers",
+            json={
+                "name": "obo-new-nooidc",
+                "transport": "streamable-http",
+                "url": "https://mcp.example.com/sse",
+                "auth_type": "oauth_obo",
+                "oauth_audience": "api://mcp-a",
+            },
+        )
+        assert r.status_code == 400, r.text
+        assert "OIDC" in r.json()["error"]
 
     def test_create_obo_rejected_on_invalid_grant_profile(self, client):
         """Review finding: a typo'd deployment obo_grant_profile leaves the mint

tests/test_oidc.py

@@ -2560,6 +2560,54 @@ class TestRuntimeRediscovery:
         # The healed config no longer advertises a retryable failure.
         assert state.oidc_config.discovery_retryable is False
 
+    def test_rediscover_latches_terminal_on_config_error_and_stops_probing(self):
+        """Review finding: probing with enabled forced True carries the
+        retryable boot flag into discover_oidc, whose config-error branches must
+        latch discovery_retryable=False (terminal) — and maybe_rediscover must
+        INSTALL that terminal config — or a config-invalid IdP (endpoint failing
+        SSRF/same-origin) re-probes every cooldown window forever. Drives the
+        real discover_oidc: the discovered token_endpoint is on a foreign host,
+        so validation rejects it as a config error."""
+        from turnstone.core.oidc import maybe_rediscover_oidc
+
+        state = self._disabled_retryable_state()  # issuer=https://idp.example.com
+        bad_doc = {
+            "authorization_endpoint": "https://idp.example.com/authorize",
+            "token_endpoint": "https://attacker.example/token",  # foreign host
+            "userinfo_endpoint": "https://idp.example.com/userinfo",
+            "jwks_uri": "https://idp.example.com/.well-known/jwks.json",
+        }
+        mock_response = MagicMock()
+        mock_response.json.return_value = bad_doc
+        mock_response.raise_for_status = MagicMock()
+
+        probes = {"n": 0}
+
+        async def _get(url):
+            probes["n"] += 1
+            return mock_response
+
+        async def _run():
+            client = _mock_async_client(_get)
+            with (
+                patch("socket.getaddrinfo", return_value=[(2, 1, 6, "", ("93.184.216.34", 0))]),
+                patch("httpx.AsyncClient", return_value=client),
+            ):
+                await maybe_rediscover_oidc(state)
+                # Same window: cooldown already gates a second probe.
+                await maybe_rediscover_oidc(state)
+                # Force the cooldown open — but the config is now terminal, so
+                # the retryable guard should short-circuit before any probe.
+                state.oidc_rediscover_last = None
+                await maybe_rediscover_oidc(state)
+
+        asyncio.run(_run())
+
+        # Still disabled, but LATCHED terminal (not retryable) — one probe only.
+        assert state.oidc_config.enabled is False
+        assert state.oidc_config.discovery_retryable is False
+        assert probes["n"] == 1
+
     def test_rediscover_cooldown_gates_repeat_probes(self):
         from turnstone.core.oidc import maybe_rediscover_oidc
 

turnstone/console/server.py

@@ -9842,10 +9842,27 @@ def _oauth_columns_to_clear(auth_type: str | None) -> dict[str, None]:
 
 
 def _enforce_oauth_obo_requirements(
-    request: Request, auth_type: str, *, audience: str | None, scopes: str | None
+    request: Request,
+    auth_type: str,
+    *,
+    audience: str | None,
+    scopes: str | None,
+    check_oidc_deployment: bool = True,
 ) -> JSONResponse | None:
     """Write-time validation for ``oauth_obo`` rows (issue #551).
 
+    ``check_oidc_deployment`` splits the checks by what they guard. The
+    DEPLOYMENT-level checks (token-encryption key, OIDC configured/enabled,
+    capture opt-in, valid grant profile) reject configuring a NEW obo mint that
+    could never work — they run on create and on a flip INTO obo. They are
+    SKIPPED for a same-type edit of an existing obo server (``False``): OIDC
+    being operator-disabled is a deployment state, not a per-server one, so
+    blocking every edit — including the natural remedy of setting
+    ``enabled=false`` — would only lock the operator out (the row can still be
+    DELETEd, but not disabled). The PER-SERVER validity checks (audience
+    required; ``oauth_scopes`` rejected under the entra profile) always run so a
+    same-type edit can't leave the row itself invalid.
+
     Rejects at the write choke point what would otherwise fail per-dispatch at
     runtime (or worse, at the next boot):
 
@@ -9872,61 +9889,60 @@ def _enforce_oauth_obo_requirements(
     """
     if auth_type != "oauth_obo":
         return None
-    if getattr(request.app.state, "mcp_token_store", None) is None:
-        return JSONResponse({"error": _OAUTH_TOKEN_STORE_503_MSG}, status_code=503)
-    oidc_config = getattr(request.app.state, "oidc_config", None)
-    # Accept a config that is enabled OR merely transiently un-discovered
-    # (``discovery_retryable`` — the IdP was unreachable at this process's boot).
-    # OIDC is still CONFIGURED there (issuer set); rejecting it would make every
-    # oauth_obo server un-editable/un-disable-able on the console — which never
-    # runs the runtime rediscovery that heals server nodes — until a manual
-    # restart. Reject only a genuinely absent / operator-disabled OIDC (neither
-    # flag set: no issuer configured).
-    oidc_configured = oidc_config is not None and (
-        getattr(oidc_config, "enabled", False) or getattr(oidc_config, "discovery_retryable", False)
-    )
-    if not oidc_configured:
-        return JSONResponse(
-            {
-                "error": (
-                    "auth_type=oauth_obo requires OIDC sign-in to be configured and "
-                    "enabled (it mints per-server tokens from each user's Turnstone "
-                    "sign-in). Configure [oidc] and capture_user_credential, or use "
-                    "auth_type=oauth_user for per-server consent."
-                )
-            },
-            status_code=400,
-        )
-    if not getattr(oidc_config, "capture_user_credential", False):
-        # The mint redeems the user's CAPTURED IdP refresh token; with capture
-        # off, login persists nothing, so every dispatch returns kind="missing"
-        # and the "sign in again" remedy can never succeed — a permanent
-        # misconfig this choke point exists to reject. (Enabling capture also
-        # requires the encryption key, checked at boot.)
-        return JSONResponse(
-            {
-                "error": (
-                    "auth_type=oauth_obo requires [oidc] capture_user_credential=true "
-                    "so each user's sign-in credential is captured for minting; it is "
-                    "currently disabled, so this server could never obtain a token."
-                )
-            },
-            status_code=400,
-        )
     profile = _obo_profile(request)
-    from turnstone.core.mcp_oauth import OBO_GRANT_PROFILES
-
-    if profile not in OBO_GRANT_PROFILES:
-        return JSONResponse(
-            {
-                "error": (
-                    f"[oidc] obo_grant_profile={profile!r} is not a valid grant profile "
-                    f"({', '.join(sorted(OBO_GRANT_PROFILES))}); fix the deployment "
-                    "config before configuring oauth_obo servers"
-                )
-            },
-            status_code=400,
+    if check_oidc_deployment:
+        if getattr(request.app.state, "mcp_token_store", None) is None:
+            return JSONResponse({"error": _OAUTH_TOKEN_STORE_503_MSG}, status_code=503)
+        oidc_config = getattr(request.app.state, "oidc_config", None)
+        # Accept a config that is enabled OR merely transiently un-discovered
+        # (``discovery_retryable`` — the IdP was unreachable at this process's
+        # boot). OIDC is still CONFIGURED there (issuer set); reject only a
+        # genuinely absent / operator-disabled OIDC (neither flag set).
+        oidc_configured = oidc_config is not None and (
+            getattr(oidc_config, "enabled", False)
+            or getattr(oidc_config, "discovery_retryable", False)
         )
+        if not oidc_configured:
+            return JSONResponse(
+                {
+                    "error": (
+                        "auth_type=oauth_obo requires OIDC sign-in to be configured and "
+                        "enabled (it mints per-server tokens from each user's Turnstone "
+                        "sign-in). Configure [oidc] and capture_user_credential, or use "
+                        "auth_type=oauth_user for per-server consent."
+                    )
+                },
+                status_code=400,
+            )
+        if not getattr(oidc_config, "capture_user_credential", False):
+            # The mint redeems the user's CAPTURED IdP refresh token; with capture
+            # off, login persists nothing, so every dispatch returns kind="missing"
+            # and the "sign in again" remedy can never succeed — a permanent
+            # misconfig this choke point exists to reject. (Enabling capture also
+            # requires the encryption key, checked at boot.)
+            return JSONResponse(
+                {
+                    "error": (
+                        "auth_type=oauth_obo requires [oidc] capture_user_credential=true "
+                        "so each user's sign-in credential is captured for minting; it is "
+                        "currently disabled, so this server could never obtain a token."
+                    )
+                },
+                status_code=400,
+            )
+        from turnstone.core.mcp_oauth import OBO_GRANT_PROFILES
+
+        if profile not in OBO_GRANT_PROFILES:
+            return JSONResponse(
+                {
+                    "error": (
+                        f"[oidc] obo_grant_profile={profile!r} is not a valid grant profile "
+                        f"({', '.join(sorted(OBO_GRANT_PROFILES))}); fix the deployment "
+                        "config before configuring oauth_obo servers"
+                    )
+                },
+                status_code=400,
+            )
     if not (audience or "").strip():
         return JSONResponse(
             {
@@ -10637,12 +10653,11 @@ async def admin_update_mcp_server(request: Request) -> JSONResponse:
     # minted obo bearers (and oauth_user tokens) are audience-scoped, so cached
     # rows for the OLD audience must be purged or a privilege reduction silently
     # doesn't take effect until they expire. (auth_type flips already purge, so
-    # only guard the same-type edit here.)
+    # only guard the same-type edit here.) Like ``scopes_changing`` below, the
+    # same-type no-op normalization above already dropped an equal re-sent value
+    # from ``updates``, so "in updates" already means a genuine change.
     audience_changing = (
-        is_user_scoped_auth(old_auth)
-        and not is_flip
-        and "oauth_audience" in updates
-        and (existing.get("oauth_audience") or "") != (updates["oauth_audience"] or "")
+        is_user_scoped_auth(old_auth) and not is_flip and "oauth_audience" in updates
     )
     # Changing oauth_scopes on an oauth_obo row is the same token-binding
     # change under the rfc8693 profile: the exchange scope shapes the minted
@@ -10686,8 +10701,16 @@ async def admin_update_mcp_server(request: Request) -> JSONResponse:
     # pre-filled field, so every unrelated save would 400.
     audience_now = updates.get("oauth_audience", existing.get("oauth_audience"))
     scopes_being_set = updates.get("oauth_scopes") if "oauth_scopes" in updates else None
+    # Run the DEPLOYMENT-level OIDC checks only when this write is a NEW obo
+    # enablement (a flip INTO obo); a same-type edit of an existing obo server
+    # keeps only the per-server validity checks, so an operator can still
+    # disable / edit an obo server after OIDC was operator-disabled.
     err_resp = _enforce_oauth_obo_requirements(
-        request, auth_type_now, audience=audience_now, scopes=scopes_being_set
+        request,
+        auth_type_now,
+        audience=audience_now,
+        scopes=scopes_being_set,
+        check_oidc_deployment=is_flip,
     )
     if err_resp is not None:
         return err_resp

turnstone/core/mcp_oauth.py

@@ -2609,15 +2609,14 @@ async def get_obo_access_token_classified(
         # expiry is never NULL for an obo row (see _OBO_DEFAULT_TTL_SECONDS): a
         # missing expires_in falls back to a conservative default so the
         # freshness gate can never serve a short-lived minted token forever.
-        obo_expires_at = _expires_at_from_response(tokens) or (
-            datetime.now(UTC) + timedelta(seconds=_OBO_DEFAULT_TTL_SECONDS)
-        ).strftime("%Y-%m-%dT%H:%M:%S")
         await _persist_obo_cache_row(
             token_store,
             user_id,
             server_name,
             access_token=access_token,
-            expires_at=obo_expires_at,
+            expires_at=_expires_at_from_response(
+                tokens, default_ttl_seconds=_OBO_DEFAULT_TTL_SECONDS
+            ),
             scopes=scopes,
             issuer=issuer,
             audience=audience,
@@ -2763,20 +2762,26 @@ async def _refresh_and_persist(
     return new_access, rotated_refresh, new_expires_at
 
 
-def _expires_at_from_response(tokens: dict[str, Any]) -> str | None:
+def _expires_at_from_response(
+    tokens: dict[str, Any], *, default_ttl_seconds: int | None = None
+) -> str | None:
     """Convert an AS ``expires_in`` to an ISO timestamp.
 
     Accepts int, float, or string-serialised numerics — some real ASes
     return ``"3600"`` (string), some return ``3600.0`` (float). Returns
-    ``None`` when the field is missing, malformed, or non-positive.
+    ``None`` when the field is missing, malformed, or non-positive — UNLESS
+    *default_ttl_seconds* is given, in which case that fallback lifetime is
+    used (the obo mint path passes ``_OBO_DEFAULT_TTL_SECONDS`` so a minted
+    row is never cached with a NULL, read-as-never-expiring expiry). One owner
+    of the stored-expiry timestamp format.
     """
     expires_in = tokens.get("expires_in")
-    seconds: int
+    seconds: int | None
     if isinstance(expires_in, bool):
         # ``bool`` is a subclass of ``int`` — reject explicitly so True
         # doesn't silently parse as 1 second.
-        return None
-    if isinstance(expires_in, int):
+        seconds = None
+    elif isinstance(expires_in, int):
         seconds = expires_in
     elif isinstance(expires_in, float):
         seconds = int(expires_in)
@@ -2784,11 +2789,13 @@ def _expires_at_from_response(tokens: dict[str, Any]) -> str | None:
         try:
             seconds = int(float(expires_in))
         except (TypeError, ValueError):
-            return None
+            seconds = None
     else:
-        return None
-    if seconds <= 0:
-        return None
+        seconds = None
+    if seconds is None or seconds <= 0:
+        if default_ttl_seconds is None:
+            return None
+        seconds = default_ttl_seconds
     return (datetime.now(UTC) + timedelta(seconds=seconds)).strftime("%Y-%m-%dT%H:%M:%S")
 
 

turnstone/core/oidc.py

@@ -449,8 +449,14 @@ async def discover_oidc(
     setup across calls; when ``None`` a transient client is used (the
     legacy shape, kept so tests don't need lifecycle management).
     """
+    # Config-error branches force ``discovery_retryable=False`` (terminal): they
+    # reflect a bad CONFIG (no issuer, an SSRF-rejected URL), not a transient IdP
+    # outage, so a runtime re-probe would only fail identically forever. This is
+    # explicit rather than preserved-from-input because ``maybe_rediscover_oidc``
+    # probes with the retryable boot config (``discovery_retryable=True``); left
+    # preserved, a config-invalid IdP would re-probe every cooldown window.
     if not config.issuer:
-        return dataclasses.replace(config, enabled=False)
+        return dataclasses.replace(config, enabled=False, discovery_retryable=False)
 
     try:
         issuer_parsed = _validate_url_no_ssrf(
@@ -458,7 +464,7 @@ async def discover_oidc(
         )
     except OIDCError as exc:
         log.warning("OIDC issuer URL rejected: %s", exc)
-        return dataclasses.replace(config, enabled=False)
+        return dataclasses.replace(config, enabled=False, discovery_retryable=False)
 
     url = config.issuer.rstrip("/") + "/.well-known/openid-configuration"
     try:
@@ -517,8 +523,10 @@ async def discover_oidc(
                 allow_private=config.allow_private_network,
             )
         except OIDCError as exc:
+            # Config error (discovered endpoint fails SSRF/validation), not a
+            # transient outage — latch terminal so rediscovery stops re-probing.
             log.warning("OIDC discovered %s rejected (url=%s): %s", name, endpoint_url, exc)
-            return dataclasses.replace(config, enabled=False)
+            return dataclasses.replace(config, enabled=False, discovery_retryable=False)
 
     if userinfo_endpoint:
         try:
@@ -535,7 +543,8 @@ async def discover_oidc(
                 userinfo_endpoint,
                 exc,
             )
-            return dataclasses.replace(config, enabled=False)
+            # Config error — latch terminal (see the issuer-rejection branch).
+            return dataclasses.replace(config, enabled=False, discovery_retryable=False)
 
     log.info("OIDC discovery complete: %s", config.issuer)
     return dataclasses.replace(
@@ -746,7 +755,14 @@ async def maybe_rediscover_oidc(app_state: Any) -> None:
         # failure clears it back to False).
         fresh = await discover_oidc(dataclasses.replace(cfg, enabled=True))
         if not fresh.enabled:
-            return  # still failing — next probe after the cooldown lapses
+            if not fresh.discovery_retryable:
+                # Discovery now fails for a CONFIG reason (bad issuer, an
+                # SSRF-rejected endpoint), not a transient outage — latch that
+                # terminal config onto app_state so this node stops re-probing
+                # every cooldown window. Without installing it, app_state would
+                # keep the retryable flag and re-probe an IdP that can never heal.
+                app_state.oidc_config = fresh
+            return  # still disabled (transient → retry next window; terminal → latched)
         app_state.oidc_config = dataclasses.replace(fresh, discovery_retryable=False)
         log.info("OIDC discovery recovered at runtime: %s", fresh.issuer)
     finally: