From 17a732197bb4f5b5a9b139bc7bdde707ffd37a02 Mon Sep 17 00:00:00 2001 From: corneyl Date: Sat, 12 Feb 2022 17:15:36 +0100 Subject: [PATCH] Add Picnic re-auth flow (#62938) * Add re-auth handler for Picnic * Extracted authentication part so right form/errors can be shown during re-auth flow * Add tests for Picnic's re-authentication flow * Simplify re-auth flow by using the same step as step_user * Use user step also for re-auth flow instead of having an authenticate step * Add check for when re-auth is done with different account * Remove unnecessary else in Picnic config flow * Fix the step id in the translation strings file --- .../components/picnic/config_flow.py | 35 +++- homeassistant/components/picnic/strings.json | 6 +- .../components/picnic/translations/en.json | 7 +- tests/components/picnic/test_config_flow.py | 194 ++++++++++++++++-- 4 files changed, 207 insertions(+), 35 deletions(-) diff --git a/homeassistant/components/picnic/config_flow.py b/homeassistant/components/picnic/config_flow.py index 09a1d524283..c2d48ca9415 100644 --- a/homeassistant/components/picnic/config_flow.py +++ b/homeassistant/components/picnic/config_flow.py @@ -9,6 +9,7 @@ import requests import voluptuous as vol from homeassistant import config_entries, core, exceptions +from homeassistant.config_entries import SOURCE_REAUTH from homeassistant.const import CONF_ACCESS_TOKEN, CONF_PASSWORD, CONF_USERNAME from .const import CONF_COUNTRY_CODE, COUNTRY_CODES, DOMAIN @@ -71,8 +72,12 @@ class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): VERSION = 1 + async def async_step_reauth(self, _): + """Perform the re-auth step upon an API authentication error.""" + return await self.async_step_user() + async def async_step_user(self, user_input=None): - """Handle the initial step.""" + """Handle the authentication step, this is the generic step for both `step_user` and `step_reauth`.""" if user_input is None: return self.async_show_form( step_id="user", data_schema=STEP_USER_DATA_SCHEMA @@ -90,17 +95,25 @@ class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): _LOGGER.exception("Unexpected exception") errors["base"] = "unknown" else: - # Set the unique id and abort if it already exists - await self.async_set_unique_id(info["unique_id"]) - self._abort_if_unique_id_configured() + data = { + CONF_ACCESS_TOKEN: auth_token, + CONF_COUNTRY_CODE: user_input[CONF_COUNTRY_CODE], + } + existing_entry = await self.async_set_unique_id(info["unique_id"]) - return self.async_create_entry( - title=info["title"], - data={ - CONF_ACCESS_TOKEN: auth_token, - CONF_COUNTRY_CODE: user_input[CONF_COUNTRY_CODE], - }, - ) + # Abort if we're adding a new config and the unique id is already in use, else create the entry + if self.source != SOURCE_REAUTH: + self._abort_if_unique_id_configured() + return self.async_create_entry(title=info["title"], data=data) + + # In case of re-auth, only continue if an exiting account exists with the same unique id + if existing_entry: + self.hass.config_entries.async_update_entry(existing_entry, data=data) + await self.hass.config_entries.async_reload(existing_entry.entry_id) + return self.async_abort(reason="reauth_successful") + + # Set the error because the account is different + errors["base"] = "different_account" return self.async_show_form( step_id="user", data_schema=STEP_USER_DATA_SCHEMA, errors=errors diff --git a/homeassistant/components/picnic/strings.json b/homeassistant/components/picnic/strings.json index 7fbd5e9bef6..9eb51b2fd2a 100644 --- a/homeassistant/components/picnic/strings.json +++ b/homeassistant/components/picnic/strings.json @@ -12,10 +12,12 @@ "error": { "cannot_connect": "[%key:common::config_flow::error::cannot_connect%]", "invalid_auth": "[%key:common::config_flow::error::invalid_auth%]", - "unknown": "[%key:common::config_flow::error::unknown%]" + "unknown": "[%key:common::config_flow::error::unknown%]", + "different_account": "Account should be the same as used for setting up the integration" }, "abort": { - "already_configured": "[%key:common::config_flow::abort::already_configured_device%]" + "already_configured": "[%key:common::config_flow::abort::already_configured_device%]", + "reauth_successful": "[%key:common::config_flow::abort::reauth_successful%]" } } } diff --git a/homeassistant/components/picnic/translations/en.json b/homeassistant/components/picnic/translations/en.json index c7097df12a9..13b62c78757 100644 --- a/homeassistant/components/picnic/translations/en.json +++ b/homeassistant/components/picnic/translations/en.json @@ -1,10 +1,12 @@ { "config": { "abort": { - "already_configured": "Device is already configured" + "already_configured": "Device is already configured", + "reauth_successful": "Re-authentication was successful" }, "error": { "cannot_connect": "Failed to connect", + "different_account": "Account should be the same as used for setting up the integration", "invalid_auth": "Invalid authentication", "unknown": "Unexpected error" }, @@ -17,6 +19,5 @@ } } } - }, - "title": "Picnic" + } } \ No newline at end of file diff --git a/tests/components/picnic/test_config_flow.py b/tests/components/picnic/test_config_flow.py index b6bcc17a03d..3ea54cee593 100644 --- a/tests/components/picnic/test_config_flow.py +++ b/tests/components/picnic/test_config_flow.py @@ -1,23 +1,20 @@ """Test the Picnic config flow.""" from unittest.mock import patch +import pytest from python_picnic_api.session import PicnicAuthError import requests -from homeassistant import config_entries +from homeassistant import config_entries, data_entry_flow from homeassistant.components.picnic.const import CONF_COUNTRY_CODE, DOMAIN from homeassistant.const import CONF_ACCESS_TOKEN +from tests.common import MockConfigEntry -async def test_form(hass): - """Test we get the form.""" - - result = await hass.config_entries.flow.async_init( - DOMAIN, context={"source": config_entries.SOURCE_USER} - ) - assert result["type"] == "form" - assert result["errors"] is None +@pytest.fixture +def picnic_api(): + """Create PicnicAPI mock with set response data.""" auth_token = "af3wh738j3fa28l9fa23lhiufahu7l" auth_data = { "user_id": "f29-2a6-o32n", @@ -29,13 +26,27 @@ async def test_form(hass): } with patch( "homeassistant.components.picnic.config_flow.PicnicAPI", - ) as mock_picnic, patch( + ) as picnic_mock: + picnic_mock().session.auth_token = auth_token + picnic_mock().get_user.return_value = auth_data + + yield picnic_mock + + +async def test_form(hass, picnic_api): + """Test we get the form and a config entry is created.""" + + result = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": config_entries.SOURCE_USER} + ) + assert result["type"] == data_entry_flow.RESULT_TYPE_FORM + assert result["step_id"] == "user" + assert result["errors"] is None + + with patch( "homeassistant.components.picnic.async_setup_entry", return_value=True, ) as mock_setup_entry: - mock_picnic().session.auth_token = auth_token - mock_picnic().get_user.return_value = auth_data - result2 = await hass.config_entries.flow.async_configure( result["flow_id"], { @@ -49,14 +60,14 @@ async def test_form(hass): assert result2["type"] == "create_entry" assert result2["title"] == "Teststreet 123b" assert result2["data"] == { - CONF_ACCESS_TOKEN: auth_token, + CONF_ACCESS_TOKEN: picnic_api().session.auth_token, CONF_COUNTRY_CODE: "NL", } assert len(mock_setup_entry.mock_calls) == 1 async def test_form_invalid_auth(hass): - """Test we handle invalid auth.""" + """Test we handle invalid authentication.""" result = await hass.config_entries.flow.async_init( DOMAIN, context={"source": config_entries.SOURCE_USER} ) @@ -74,12 +85,12 @@ async def test_form_invalid_auth(hass): }, ) - assert result2["type"] == "form" + assert result2["type"] == data_entry_flow.RESULT_TYPE_FORM assert result2["errors"] == {"base": "invalid_auth"} async def test_form_cannot_connect(hass): - """Test we handle cannot connect error.""" + """Test we handle connection errors.""" result = await hass.config_entries.flow.async_init( DOMAIN, context={"source": config_entries.SOURCE_USER} ) @@ -97,7 +108,7 @@ async def test_form_cannot_connect(hass): }, ) - assert result2["type"] == "form" + assert result2["type"] == data_entry_flow.RESULT_TYPE_FORM assert result2["errors"] == {"base": "cannot_connect"} @@ -120,5 +131,150 @@ async def test_form_exception(hass): }, ) - assert result2["type"] == "form" + assert result2["type"] == data_entry_flow.RESULT_TYPE_FORM assert result2["errors"] == {"base": "unknown"} + + +async def test_form_already_configured(hass, picnic_api): + """Test that an entry with unique id can only be added once.""" + # Create a mocked config entry and make sure to use the same user_id as set for the picnic_api mock response. + MockConfigEntry( + domain=DOMAIN, + unique_id=picnic_api().get_user()["user_id"], + data={CONF_ACCESS_TOKEN: "a3p98fsen.a39p3fap", CONF_COUNTRY_CODE: "NL"}, + ).add_to_hass(hass) + + result_init = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": config_entries.SOURCE_USER} + ) + + result_configure = await hass.config_entries.flow.async_configure( + result_init["flow_id"], + { + "username": "test-username", + "password": "test-password", + "country_code": "NL", + }, + ) + await hass.async_block_till_done() + + assert result_configure["type"] == data_entry_flow.RESULT_TYPE_ABORT + assert result_configure["reason"] == "already_configured" + + +async def test_step_reauth(hass, picnic_api): + """Test the re-auth flow.""" + # Create a mocked config entry + conf = {CONF_ACCESS_TOKEN: "a3p98fsen.a39p3fap", CONF_COUNTRY_CODE: "NL"} + + MockConfigEntry( + domain=DOMAIN, + unique_id=picnic_api().get_user()["user_id"], + data=conf, + ).add_to_hass(hass) + + # Init a re-auth flow + result_init = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": config_entries.SOURCE_REAUTH}, data=conf + ) + assert result_init["type"] == data_entry_flow.RESULT_TYPE_FORM + assert result_init["step_id"] == "user" + + with patch( + "homeassistant.components.picnic.async_setup_entry", + return_value=True, + ): + result_configure = await hass.config_entries.flow.async_configure( + result_init["flow_id"], + { + "username": "test-username", + "password": "test-password", + "country_code": "NL", + }, + ) + await hass.async_block_till_done() + + # Check that the returned flow has type abort because of successful re-authentication + assert result_configure["type"] == data_entry_flow.RESULT_TYPE_ABORT + assert result_configure["reason"] == "reauth_successful" + + assert len(hass.config_entries.async_entries()) == 1 + + +async def test_step_reauth_failed(hass): + """Test the re-auth flow when authentication fails.""" + # Create a mocked config entry + user_id = "f29-2a6-o32n" + conf = {CONF_ACCESS_TOKEN: "a3p98fsen.a39p3fap", CONF_COUNTRY_CODE: "NL"} + + MockConfigEntry( + domain=DOMAIN, + unique_id=user_id, + data=conf, + ).add_to_hass(hass) + + # Init a re-auth flow + result_init = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": config_entries.SOURCE_REAUTH}, data=conf + ) + assert result_init["type"] == data_entry_flow.RESULT_TYPE_FORM + assert result_init["step_id"] == "user" + + with patch( + "homeassistant.components.picnic.config_flow.PicnicHub.authenticate", + side_effect=PicnicAuthError, + ): + result_configure = await hass.config_entries.flow.async_configure( + result_init["flow_id"], + { + "username": "test-username", + "password": "test-password", + "country_code": "NL", + }, + ) + await hass.async_block_till_done() + + # Check that the returned flow has type form with error set + assert result_configure["type"] == "form" + assert result_configure["errors"] == {"base": "invalid_auth"} + + assert len(hass.config_entries.async_entries()) == 1 + + +async def test_step_reauth_different_account(hass, picnic_api): + """Test the re-auth flow when authentication is done with a different account.""" + # Create a mocked config entry, unique_id should be different that the user id in the api response + conf = {CONF_ACCESS_TOKEN: "a3p98fsen.a39p3fap", CONF_COUNTRY_CODE: "NL"} + + MockConfigEntry( + domain=DOMAIN, + unique_id="3fpawh-ues-af3ho", + data=conf, + ).add_to_hass(hass) + + # Init a re-auth flow + result_init = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": config_entries.SOURCE_REAUTH}, data=conf + ) + assert result_init["type"] == data_entry_flow.RESULT_TYPE_FORM + assert result_init["step_id"] == "user" + + with patch( + "homeassistant.components.picnic.async_setup_entry", + return_value=True, + ): + result_configure = await hass.config_entries.flow.async_configure( + result_init["flow_id"], + { + "username": "test-username", + "password": "test-password", + "country_code": "NL", + }, + ) + await hass.async_block_till_done() + + # Check that the returned flow has type form with error set + assert result_configure["type"] == "form" + assert result_configure["errors"] == {"base": "different_account"} + + assert len(hass.config_entries.async_entries()) == 1