From 04dfbd2e032c2edf4f1e277c2d24872b6b50dec7 Mon Sep 17 00:00:00 2001 From: Allen Porter Date: Tue, 31 Oct 2023 19:48:33 -0700 Subject: [PATCH] Improve fitbit oauth token error handling in config flow (#103131) * Improve fitbit oauth token error handling in config flow * Apply suggestions from code review Co-authored-by: Martin Hjelmare * Update tests with updated error reason --------- Co-authored-by: Martin Hjelmare --- .../fitbit/application_credentials.py | 13 +++-- .../components/fitbit/config_flow.py | 15 ++++++ homeassistant/components/fitbit/strings.json | 5 +- tests/components/fitbit/test_config_flow.py | 53 ++++++++++++++++++- 4 files changed, 78 insertions(+), 8 deletions(-) diff --git a/homeassistant/components/fitbit/application_credentials.py b/homeassistant/components/fitbit/application_credentials.py index e66b9ca9014..caf0384eca2 100644 --- a/homeassistant/components/fitbit/application_credentials.py +++ b/homeassistant/components/fitbit/application_credentials.py @@ -59,13 +59,16 @@ class FitbitOAuth2Implementation(AuthImplementation): resp = await session.post(self.token_url, data=data, headers=self._headers) resp.raise_for_status() except aiohttp.ClientResponseError as err: - error_body = await resp.text() - _LOGGER.debug("Client response error body: %s", error_body) + if _LOGGER.isEnabledFor(logging.DEBUG): + error_body = await resp.text() if not session.closed else "" + _LOGGER.debug( + "Client response error status=%s, body=%s", err.status, error_body + ) if err.status == HTTPStatus.UNAUTHORIZED: - raise FitbitAuthException from err - raise FitbitApiException from err + raise FitbitAuthException(f"Unauthorized error: {err}") from err + raise FitbitApiException(f"Server error response: {err}") from err except aiohttp.ClientError as err: - raise FitbitApiException from err + raise FitbitApiException(f"Client connection error: {err}") from err return cast(dict, await resp.json()) @property diff --git a/homeassistant/components/fitbit/config_flow.py b/homeassistant/components/fitbit/config_flow.py index ee2340e7587..dd7e79e2c65 100644 --- a/homeassistant/components/fitbit/config_flow.py +++ b/homeassistant/components/fitbit/config_flow.py @@ -53,6 +53,21 @@ class OAuth2FlowHandler( return self.async_show_form(step_id="reauth_confirm") return await self.async_step_user() + async def async_step_creation( + self, user_input: dict[str, Any] | None = None + ) -> FlowResult: + """Create config entry from external data with Fitbit specific error handling.""" + try: + return await super().async_step_creation() + except FitbitAuthException as err: + _LOGGER.error( + "Failed to authenticate when creating Fitbit credentials: %s", err + ) + return self.async_abort(reason="invalid_auth") + except FitbitApiException as err: + _LOGGER.error("Failed to create Fitbit credentials: %s", err) + return self.async_abort(reason="cannot_connect") + async def async_oauth_create_entry(self, data: dict[str, Any]) -> FlowResult: """Create an entry for the flow, or update existing entry.""" diff --git a/homeassistant/components/fitbit/strings.json b/homeassistant/components/fitbit/strings.json index 2d74408a73f..889b56f1bbd 100644 --- a/homeassistant/components/fitbit/strings.json +++ b/homeassistant/components/fitbit/strings.json @@ -16,9 +16,10 @@ "already_configured": "[%key:common::config_flow::abort::already_configured_account%]", "already_in_progress": "[%key:common::config_flow::abort::already_in_progress%]", "cannot_connect": "[%key:common::config_flow::error::cannot_connect%]", - "oauth_error": "[%key:common::config_flow::abort::oauth2_error%]", - "missing_configuration": "[%key:common::config_flow::abort::oauth2_missing_configuration%]", "invalid_access_token": "[%key:common::config_flow::error::invalid_access_token%]", + "invalid_auth": "[%key:common::config_flow::error::invalid_auth%]", + "missing_configuration": "[%key:common::config_flow::abort::oauth2_missing_configuration%]", + "oauth_error": "[%key:common::config_flow::abort::oauth2_error%]", "reauth_successful": "[%key:common::config_flow::abort::reauth_successful%]", "unknown": "[%key:common::config_flow::error::unknown%]", "wrong_account": "The user credentials provided do not match this Fitbit account." diff --git a/tests/components/fitbit/test_config_flow.py b/tests/components/fitbit/test_config_flow.py index cf2d5d17f22..d51379c9adc 100644 --- a/tests/components/fitbit/test_config_flow.py +++ b/tests/components/fitbit/test_config_flow.py @@ -88,6 +88,57 @@ async def test_full_flow( } +@pytest.mark.parametrize( + ("status_code", "error_reason"), + [ + (HTTPStatus.UNAUTHORIZED, "invalid_auth"), + (HTTPStatus.INTERNAL_SERVER_ERROR, "cannot_connect"), + ], +) +async def test_token_error( + hass: HomeAssistant, + hass_client_no_auth: ClientSessionGenerator, + aioclient_mock: AiohttpClientMocker, + current_request_with_host: None, + profile: None, + setup_credentials: None, + status_code: HTTPStatus, + error_reason: str, +) -> None: + """Check full flow.""" + result = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": config_entries.SOURCE_USER} + ) + state = config_entry_oauth2_flow._encode_jwt( + hass, + { + "flow_id": result["flow_id"], + "redirect_uri": REDIRECT_URL, + }, + ) + assert result["type"] == FlowResultType.EXTERNAL_STEP + assert result["url"] == ( + f"{OAUTH2_AUTHORIZE}?response_type=code&client_id={CLIENT_ID}" + f"&redirect_uri={REDIRECT_URL}" + f"&state={state}" + "&scope=activity+heartrate+nutrition+profile+settings+sleep+weight&prompt=consent" + ) + + client = await hass_client_no_auth() + resp = await client.get(f"/auth/external/callback?code=abcd&state={state}") + assert resp.status == 200 + assert resp.headers["content-type"] == "text/html; charset=utf-8" + + aioclient_mock.post( + OAUTH2_TOKEN, + status=status_code, + ) + + result = await hass.config_entries.flow.async_configure(result["flow_id"]) + assert result.get("type") == FlowResultType.ABORT + assert result.get("reason") == error_reason + + @pytest.mark.parametrize( ("http_status", "json", "error_reason"), [ @@ -460,7 +511,7 @@ async def test_reauth_flow( "refresh_token": "updated-refresh-token", "access_token": "updated-access-token", "type": "Bearer", - "expires_in": 60, + "expires_in": "60", }, )