fix: report a rejected PKCE token exchange as an auth error (FLYTE-SDK-7D)

src/flyte/remote/_client/auth/_authenticators/pkce.py

@@ -22,11 +22,42 @@
 from flyte.remote._client.auth._authenticators.base import Authenticator
 from flyte.remote._client.auth._default_html import get_default_success_html
 from flyte.remote._client.auth._keyring import Credentials
-from flyte.remote._client.auth.errors import AccessTokenNotFoundError
+from flyte.remote._client.auth.errors import AccessTokenNotFoundError, AuthenticationError
 
 _utf_8 = "utf-8"
 _code_verifier_length = 64
 _random_seed_length = 40
+# Maximum number of characters of a non-JSON token-endpoint body to quote back to the user.
+# The body is usually an HTML error page from whatever answered instead of the IDP.
+_max_quoted_error_body = 200
+
+
+def _describe_oauth_error(resp: httpx.Response) -> str:
+    """Render a token-endpoint error response as something a human can act on.
+
+    RFC 6749 section 5.2 requires the token endpoint to answer a failed request with a JSON
+    body carrying an `error` code and, optionally, `error_description` / `error_uri`.
+    That description is the only part of the exchange that says *why* the login was
+    rejected -- "client_secret is missing.", "redirect_uri mismatch", "invalid_client" --
+    so it is what the user needs to see. Falls back to a truncated raw body when the
+    response is not the JSON the spec calls for (an HTML error page from a proxy, say).
+    """
+    try:
+        body = resp.json()
+    except Exception:
+        body = None
+    if isinstance(body, dict) and "error" in body:
+        description = body.get("error_description")
+        rendered = f"{body['error']}: {description}" if description else str(body["error"])
+        if body.get("error_uri"):
+            rendered = f"{rendered} (see {body['error_uri']})"
+        return rendered
+    text = (resp.text or "").strip()
+    if not text:
+        return "the response body was empty"
+    if len(text) > _max_quoted_error_body:
+        text = text[:_max_quoted_error_body] + "..."
+    return text
 
 
 class PKCEAuthenticator(Authenticator):
@@ -274,14 +305,18 @@ async def _credentials_from_response(self, auth_token_resp) -> Credentials:
             Credentials object created from the response
 
         Raises:
-            ValueError: If the response does not contain an access token
+            AuthenticationError: If the response does not contain an access token
         """
         response_body = auth_token_resp.json()
         refresh_token = None
         expires_in = None
 
         if "access_token" not in response_body:
-            raise ValueError('Expected "access_token" in response from oauth server')
+            raise AuthenticationError(
+                f"The identity provider at {self._token_endpoint} returned a successful response with no "
+                f'"access_token" field. Contact whoever administers your Flyte/Union deployment\'s identity '
+                f"provider; the OAuth2 application backing browser (PKCE) login is misconfigured."
+            )
         if "refresh_token" in response_body:
             refresh_token = response_body["refresh_token"]
         if "expires_in" in response_body:
@@ -314,8 +349,12 @@ async def _request_access_token(self, auth_code) -> Credentials:
         )
 
         if resp.status_code != _StatusCodes.OK:
-            raise RuntimeError(
-                "Failed to request access token with response: [{}] {!r}".format(resp.status_code, resp.content)
+            logger.error(f"Status Code ({resp.status_code}) received from IDP: {resp.text}")
+            raise AuthenticationError(
+                f"The identity provider at {self._token_endpoint} rejected the login "
+                f"({resp.status_code}): {_describe_oauth_error(resp)}. This is a configuration problem with "
+                f"the OAuth2 application backing browser (PKCE) login, not something the Flyte SDK can "
+                f"retry; contact whoever administers your Flyte/Union deployment."
             )
 
         return await self._credentials_from_response(resp)

tests/flyte/remote/test_pkce_token_request.py

@@ -0,0 +1,147 @@
+"""
+Tests for the authorization-code -> access-token exchange in the PKCE (browser) login flow.
+
+A token endpoint that rejects the exchange is telling us the deployment's OAuth2
+application is misconfigured -- "client_secret is missing.", a redirect_uri mismatch, an
+unknown client. That used to surface as a bare `RuntimeError` carrying the raw response
+bytes, which reads as an SDK crash (FLYTE-SDK-7D) and got reported to Sentry as one.
+"""
+
+from types import SimpleNamespace
+from unittest.mock import AsyncMock, MagicMock
+
+import pytest
+
+from flyte.remote._client.auth._authenticators.pkce import AuthorizationClient
+from flyte.remote._client.auth.errors import AuthenticationError
+
+
+def _client(response) -> AuthorizationClient:
+    session = MagicMock()
+    session.post = AsyncMock(return_value=response)
+    return AuthorizationClient(
+        endpoint="dns:///example.union.ai",
+        auth_endpoint="https://example.union.ai/oauth2/authorize",
+        token_endpoint="https://example.union.ai/oauth2/token",
+        http_session=session,
+        client_id="flytepropeller",
+        redirect_uri="http://localhost:8080/callback",
+    )
+
+
+def _response(status_code: int, *, json_body=None, text: str = "") -> MagicMock:
+    resp = MagicMock()
+    resp.status_code = status_code
+    resp.text = text
+    if json_body is None:
+        resp.json.side_effect = ValueError("not json")
+    else:
+        resp.json.return_value = json_body
+    return resp
+
+
+def _auth_code(state: str) -> SimpleNamespace:
+    return SimpleNamespace(code="the-auth-code", state=state)
+
+
+@pytest.mark.asyncio
+async def test_rejected_token_request_raises_authentication_error():
+    """The exact FLYTE-SDK-7D shape: 400 invalid_request / "client_secret is missing."."""
+    client = _client(
+        _response(
+            400,
+            json_body={"error": "invalid_request", "error_description": "client_secret is missing."},
+            text='{"error": "invalid_request", "error_description": "client_secret is missing."}',
+        )
+    )
+
+    with pytest.raises(AuthenticationError) as exc_info:
+        await client._request_access_token(_auth_code(client._state))
+
+    message = str(exc_info.value)
+    # The IDP's own description is the only part that says *why*, so it has to survive.
+    assert "client_secret is missing." in message
+    assert "invalid_request" in message
+    assert "400" in message
+    assert "https://example.union.ai/oauth2/token" in message
+
+
+@pytest.mark.asyncio
+async def test_rejected_token_request_is_filtered_from_sentry():
+    """AuthenticationError is on _sentry's user-error list; a bare RuntimeError was not."""
+    from flyte._sentry import _is_user_error
+
+    client = _client(_response(401, json_body={"error": "invalid_client"}, text='{"error": "invalid_client"}'))
+
+    with pytest.raises(AuthenticationError) as exc_info:
+        await client._request_access_token(_auth_code(client._state))
+
+    assert _is_user_error(exc_info.value)
+
+
+@pytest.mark.asyncio
+async def test_rejected_token_request_with_non_json_body_quotes_the_body():
+    """A proxy or login page answering the token endpoint returns HTML, not RFC 6749 JSON."""
+    client = _client(_response(502, text="<html><body>502 Bad Gateway</body></html>"))
+
+    with pytest.raises(AuthenticationError) as exc_info:
+        await client._request_access_token(_auth_code(client._state))
+
+    assert "502 Bad Gateway" in str(exc_info.value)
+
+
+@pytest.mark.asyncio
+async def test_rejected_token_request_truncates_a_huge_body():
+    client = _client(_response(500, text="x" * 5000))
+
+    with pytest.raises(AuthenticationError) as exc_info:
+        await client._request_access_token(_auth_code(client._state))
+
+    assert len(str(exc_info.value)) < 1000
+    assert "..." in str(exc_info.value)
+
+
+@pytest.mark.asyncio
+async def test_rejected_token_request_with_empty_body_still_explains_itself():
+    client = _client(_response(403, text=""))
+
+    with pytest.raises(AuthenticationError) as exc_info:
+        await client._request_access_token(_auth_code(client._state))
+
+    assert "the response body was empty" in str(exc_info.value)
+    assert "403" in str(exc_info.value)
+
+
+@pytest.mark.asyncio
+async def test_successful_token_request_still_returns_credentials():
+    client = _client(
+        _response(
+            200,
+            json_body={"access_token": "at", "refresh_token": "rt", "expires_in": 3600},
+            text="",
+        )
+    )
+
+    creds = await client._request_access_token(_auth_code(client._state))
+
+    assert creds.access_token == "at"
+    assert creds.refresh_token == "rt"
+
+
+@pytest.mark.asyncio
+async def test_response_without_access_token_raises_authentication_error():
+    client = _client(_response(200, json_body={"token_type": "Bearer"}, text="{}"))
+
+    with pytest.raises(AuthenticationError) as exc_info:
+        await client._request_access_token(_auth_code(client._state))
+
+    assert "access_token" in str(exc_info.value)
+
+
+@pytest.mark.asyncio
+async def test_state_mismatch_still_raises_value_error():
+    """The state check guards against a forged callback and is deliberately left alone."""
+    client = _client(_response(200, json_body={"access_token": "at"}, text=""))
+
+    with pytest.raises(ValueError, match="Unexpected state parameter"):
+        await client._request_access_token(_auth_code("not-the-state-we-sent"))