diff --git a/src/flyte/remote/_client/auth/_authenticators/pkce.py b/src/flyte/remote/_client/auth/_authenticators/pkce.py index af57c7f20..ff368f211 100644 --- a/src/flyte/remote/_client/auth/_authenticators/pkce.py +++ b/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) diff --git a/tests/flyte/remote/test_pkce_token_request.py b/tests/flyte/remote/test_pkce_token_request.py new file mode 100644 index 000000000..d1f42182c --- /dev/null +++ b/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="502 Bad Gateway")) + + 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"))