From 5446641f09460191226405c300472ec70974a957 Mon Sep 17 00:00:00 2001 From: Robert Van Gorkom Date: Mon, 22 Jun 2020 08:55:41 -0700 Subject: [PATCH] User defined profile name for Withings (#36864) --- homeassistant/components/withings/__init__.py | 35 +++-- .../components/withings/config_flow.py | 50 ++++-- .../components/withings/strings.json | 12 +- tests/components/withings/common.py | 9 +- tests/components/withings/test_config_flow.py | 97 ++++++++++++ tests/components/withings/test_init.py | 144 +++++++----------- 6 files changed, 224 insertions(+), 123 deletions(-) create mode 100644 tests/components/withings/test_config_flow.py diff --git a/homeassistant/components/withings/__init__.py b/homeassistant/components/withings/__init__.py index bd8e118adc9..29fca81ca79 100644 --- a/homeassistant/components/withings/__init__.py +++ b/homeassistant/components/withings/__init__.py @@ -39,18 +39,23 @@ DOMAIN = const.DOMAIN CONFIG_SCHEMA = vol.Schema( { - DOMAIN: vol.Schema( - { - vol.Required(CONF_CLIENT_ID): vol.All(cv.string, vol.Length(min=1)), - vol.Required(CONF_CLIENT_SECRET): vol.All(cv.string, vol.Length(min=1)), - vol.Optional(const.CONF_USE_WEBHOOK, default=False): cv.boolean, - vol.Required(const.CONF_PROFILES): vol.All( - cv.ensure_list, - vol.Unique(), - vol.Length(min=1), - [vol.All(cv.string, vol.Length(min=1))], - ), - } + DOMAIN: vol.All( + cv.deprecated(const.CONF_PROFILES, invalidation_version="0.114"), + vol.Schema( + { + vol.Required(CONF_CLIENT_ID): vol.All(cv.string, vol.Length(min=1)), + vol.Required(CONF_CLIENT_SECRET): vol.All( + cv.string, vol.Length(min=1) + ), + vol.Optional(const.CONF_USE_WEBHOOK, default=False): cv.boolean, + vol.Optional(const.CONF_PROFILES): vol.All( + cv.ensure_list, + vol.Unique(), + vol.Length(min=1), + [vol.All(cv.string, vol.Length(min=1))], + ), + } + ), ) }, extra=vol.ALLOW_EXTRA, @@ -87,8 +92,10 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: config_updates = {} # Add a unique id if it's an older config entry. - if entry.unique_id != entry.data["token"]["userid"]: - config_updates["unique_id"] = entry.data["token"]["userid"] + if entry.unique_id != entry.data["token"]["userid"] or not isinstance( + entry.unique_id, str + ): + config_updates["unique_id"] = str(entry.data["token"]["userid"]) # Add the webhook configuration. if CONF_WEBHOOK_ID not in entry.data: diff --git a/homeassistant/components/withings/config_flow.py b/homeassistant/components/withings/config_flow.py index d07ed419fce..e1a1dee3191 100644 --- a/homeassistant/components/withings/config_flow.py +++ b/homeassistant/components/withings/config_flow.py @@ -1,5 +1,6 @@ """Config flow for Withings.""" import logging +from typing import Dict, Union import voluptuous as vol from withings_api.common import AuthScope @@ -7,6 +8,7 @@ from withings_api.common import AuthScope from homeassistant import config_entries from homeassistant.components.withings import const from homeassistant.helpers import config_entry_oauth2_flow +from homeassistant.util import slugify _LOGGER = logging.getLogger(__name__) @@ -18,7 +20,8 @@ class WithingsFlowHandler( DOMAIN = const.DOMAIN CONNECTION_CLASS = config_entries.CONN_CLASS_CLOUD_POLL - _current_data = None + # Temporarily holds authorization data during the profile step. + _current_data: Dict[str, Union[None, str, int]] = {} @property def logger(self) -> logging.Logger: @@ -46,33 +49,58 @@ class WithingsFlowHandler( async def async_step_profile(self, data: dict) -> dict: """Prompt the user to select a user profile.""" - profile = data.get(const.PROFILE) + errors = {} + # pylint: disable=no-member # https://github.com/PyCQA/pylint/issues/3167 + reauth_profile = ( + self.context.get(const.PROFILE) + if self.context.get("source") == "reauth" + else None + ) + profile = data.get(const.PROFILE) or reauth_profile if profile: - new_data = {**self._current_data, **{const.PROFILE: profile}} - self._current_data = None - return await self.async_step_finish(new_data) + existing_entries = [ + config_entry + for config_entry in self.hass.config_entries.async_entries(const.DOMAIN) + if slugify(config_entry.data.get(const.PROFILE)) == slugify(profile) + ] + + if reauth_profile or not existing_entries: + new_data = {**self._current_data, **data, const.PROFILE: profile} + self._current_data = {} + return await self.async_step_finish(new_data) + + errors["base"] = "profile_exists" - profiles = self.hass.data[const.DOMAIN][const.CONFIG][const.CONF_PROFILES] return self.async_show_form( step_id="profile", - data_schema=vol.Schema({vol.Required(const.PROFILE): vol.In(profiles)}), + data_schema=vol.Schema({vol.Required(const.PROFILE): str}), + errors=errors, ) - async def async_step_reauth(self, data: dict) -> dict: + async def async_step_reauth(self, data: dict = None) -> dict: """Prompt user to re-authenticate.""" if data is not None: return await self.async_step_user() + # pylint: disable=no-member # https://github.com/PyCQA/pylint/issues/3167 + placeholders = {const.PROFILE: self.context["profile"]} + + self.context.update({"title_placeholders": placeholders}) + return self.async_show_form( step_id="reauth", # pylint: disable=no-member # https://github.com/PyCQA/pylint/issues/3167 - description_placeholders={"profile": self.context["profile"]}, + description_placeholders=placeholders, ) async def async_step_finish(self, data: dict) -> dict: """Finish the flow.""" - self._current_data = None + self._current_data = {} + + await self.async_set_unique_id( + str(data["token"]["userid"]), raise_on_progress=False + ) + self._abort_if_unique_id_configured(data) - await self.async_set_unique_id(data["token"]["userid"], raise_on_progress=False) return self.async_create_entry(title=data[const.PROFILE], data=data) diff --git a/homeassistant/components/withings/strings.json b/homeassistant/components/withings/strings.json index 7141d1eb0d5..e7763a1db0c 100644 --- a/homeassistant/components/withings/strings.json +++ b/homeassistant/components/withings/strings.json @@ -4,18 +4,22 @@ "step": { "profile": { "title": "User Profile.", - "description": "Which profile did you select on the Withings website? It's important the profiles match, otherwise data will be mis-labeled.", - "data": { "profile": "Profile" } + "description": "Provide a unique profile name for this data. Typically this is the name of the profile you selected in the previous step.", + "data": { "profile": "Profile Name" } }, "pick_implementation": { "title": "Pick Authentication Method" }, "reauth": { - "title": "Re-authenticate {profile}", + "title": "Re-authenticate Profile", "description": "The \"{profile}\" profile needs to be re-authenticated in order to continue receiving Withings data." } }, + "error": { + "profile_exists": "User profile is already configured. Please provide a unique profile name." + }, "abort": { "authorize_url_timeout": "Timeout generating authorize url.", - "missing_configuration": "The Withings integration is not configured. Please follow the documentation." + "missing_configuration": "The Withings integration is not configured. Please follow the documentation.", + "already_configured": "Configuration updated for profile." }, "create_entry": { "default": "Successfully authenticated with Withings." } } diff --git a/tests/components/withings/common.py b/tests/components/withings/common.py index 8c770fff5e6..3ed3b39daee 100644 --- a/tests/components/withings/common.py +++ b/tests/components/withings/common.py @@ -141,9 +141,6 @@ class ComponentFactory: CONF_CLIENT_ID: self._client_id, CONF_CLIENT_SECRET: self._client_secret, const.CONF_USE_WEBHOOK: True, - const.CONF_PROFILES: [ - profile_config.profile for profile_config in self._profile_configs - ], }, } @@ -233,11 +230,9 @@ class ComponentFactory: result = await self._hass.config_entries.flow.async_configure(result["flow_id"]) assert result.get("type") == "form" assert result.get("step_id") == "profile" - assert result.get("data_schema").schema["profile"].container == [ - profile.profile for profile in self._profile_configs - ] + assert "profile" in result.get("data_schema").schema - # Select the user profile. + # Provide the user profile. result = await self._hass.config_entries.flow.async_configure( result["flow_id"], {const.PROFILE: profile_config.profile} ) diff --git a/tests/components/withings/test_config_flow.py b/tests/components/withings/test_config_flow.py new file mode 100644 index 00000000000..f47a8f95e53 --- /dev/null +++ b/tests/components/withings/test_config_flow.py @@ -0,0 +1,97 @@ +"""Tests for config flow.""" +from aiohttp.test_utils import TestClient + +from homeassistant.components.withings import const +from homeassistant.config import async_process_ha_core_config +from homeassistant.const import ( + CONF_CLIENT_ID, + CONF_CLIENT_SECRET, + CONF_EXTERNAL_URL, + CONF_UNIT_SYSTEM, + CONF_UNIT_SYSTEM_METRIC, +) +from homeassistant.core import DOMAIN as HA_DOMAIN, HomeAssistant +from homeassistant.helpers import config_entry_oauth2_flow +from homeassistant.helpers.config_entry_oauth2_flow import AUTH_CALLBACK_PATH +from homeassistant.setup import async_setup_component + +from tests.common import MockConfigEntry + + +async def test_config_non_unique_profile(hass: HomeAssistant) -> None: + """Test setup a non-unique profile.""" + config_entry = MockConfigEntry( + domain=const.DOMAIN, data={const.PROFILE: "person0"}, unique_id="0" + ) + config_entry.add_to_hass(hass) + + result = await hass.config_entries.flow.async_init( + const.DOMAIN, context={"source": "profile"}, data={const.PROFILE: "person0"} + ) + + assert result + assert result["errors"]["base"] == "profile_exists" + + +async def test_config_reauth_profile( + hass: HomeAssistant, aiohttp_client, aioclient_mock +) -> None: + """Test reauth an existing profile re-creates the config entry.""" + hass_config = { + HA_DOMAIN: { + CONF_UNIT_SYSTEM: CONF_UNIT_SYSTEM_METRIC, + CONF_EXTERNAL_URL: "http://127.0.0.1:8080/", + }, + const.DOMAIN: { + CONF_CLIENT_ID: "my_client_id", + CONF_CLIENT_SECRET: "my_client_secret", + const.CONF_USE_WEBHOOK: False, + }, + } + await async_process_ha_core_config(hass, hass_config.get(HA_DOMAIN)) + assert await async_setup_component(hass, const.DOMAIN, hass_config) + await hass.async_block_till_done() + + config_entry = MockConfigEntry( + domain=const.DOMAIN, data={const.PROFILE: "person0"}, unique_id="0" + ) + config_entry.add_to_hass(hass) + + result = await hass.config_entries.flow.async_init( + const.DOMAIN, context={"source": "reauth", "profile": "person0"} + ) + assert result + assert result["type"] == "form" + assert result["step_id"] == "reauth" + assert result["description_placeholders"] == {const.PROFILE: "person0"} + + result = await hass.config_entries.flow.async_configure(result["flow_id"], {},) + + # pylint: disable=protected-access + state = config_entry_oauth2_flow._encode_jwt(hass, {"flow_id": result["flow_id"]}) + + client: TestClient = await aiohttp_client(hass.http.app) + resp = await client.get(f"{AUTH_CALLBACK_PATH}?code=abcd&state={state}") + assert resp.status == 200 + assert resp.headers["content-type"] == "text/html; charset=utf-8" + + aioclient_mock.clear_requests() + aioclient_mock.post( + "https://account.withings.com/oauth2/token", + json={ + "refresh_token": "mock-refresh-token", + "access_token": "mock-access-token", + "type": "Bearer", + "expires_in": 60, + "userid": "0", + }, + ) + + result = await hass.config_entries.flow.async_configure(result["flow_id"]) + assert result + assert result["type"] == "abort" + assert result["reason"] == "already_configured" + + entries = hass.config_entries.async_entries(const.DOMAIN) + assert entries + assert entries[0].data["token"]["refresh_token"] == "mock-refresh-token" diff --git a/tests/components/withings/test_init.py b/tests/components/withings/test_init.py index 29f8ca5e3b8..565a2d7e921 100644 --- a/tests/components/withings/test_init.py +++ b/tests/components/withings/test_init.py @@ -1,14 +1,22 @@ """Tests for the Withings component.""" -from asynctest import MagicMock, patch import pytest import voluptuous as vol from withings_api.common import UnauthorizedException +import homeassistant.components.webhook as webhook from homeassistant.components.withings import CONFIG_SCHEMA, DOMAIN, async_setup, const -from homeassistant.components.withings.common import DataManager -from homeassistant.const import CONF_CLIENT_ID, CONF_CLIENT_SECRET -from homeassistant.core import HomeAssistant +from homeassistant.components.withings.common import ConfigEntryWithingsApi, DataManager +from homeassistant.config import async_process_ha_core_config +from homeassistant.const import ( + CONF_CLIENT_ID, + CONF_CLIENT_SECRET, + CONF_EXTERNAL_URL, + CONF_UNIT_SYSTEM, + CONF_UNIT_SYSTEM_METRIC, +) +from homeassistant.core import DOMAIN as HA_DOMAIN, HomeAssistant from homeassistant.helpers.update_coordinator import DataUpdateCoordinator +from homeassistant.setup import async_setup_component from .common import ( ComponentFactory, @@ -17,7 +25,7 @@ from .common import ( new_profile_config, ) -from tests.common import MockConfigEntry +from tests.common import MagicMock, MockConfigEntry, patch def config_schema_validate(withings_config) -> dict: @@ -43,71 +51,40 @@ def test_config_schema_basic_config() -> None: CONF_CLIENT_ID: "my_client_id", CONF_CLIENT_SECRET: "my_client_secret", const.CONF_USE_WEBHOOK: True, - const.CONF_PROFILES: ["Person 1", "Person 2"], } ) def test_config_schema_client_id() -> None: """Test schema.""" + config_schema_assert_fail({CONF_CLIENT_SECRET: "my_client_secret"}) config_schema_assert_fail( - { - CONF_CLIENT_SECRET: "my_client_secret", - const.CONF_PROFILES: ["Person 1", "Person 2"], - } - ) - config_schema_assert_fail( - { - CONF_CLIENT_SECRET: "my_client_secret", - CONF_CLIENT_ID: "", - const.CONF_PROFILES: ["Person 1"], - } + {CONF_CLIENT_SECRET: "my_client_secret", CONF_CLIENT_ID: ""} ) config_schema_validate( - { - CONF_CLIENT_SECRET: "my_client_secret", - CONF_CLIENT_ID: "my_client_id", - const.CONF_PROFILES: ["Person 1"], - } + {CONF_CLIENT_SECRET: "my_client_secret", CONF_CLIENT_ID: "my_client_id"} ) def test_config_schema_client_secret() -> None: """Test schema.""" - config_schema_assert_fail( - {CONF_CLIENT_ID: "my_client_id", const.CONF_PROFILES: ["Person 1"]} - ) - config_schema_assert_fail( - { - CONF_CLIENT_ID: "my_client_id", - CONF_CLIENT_SECRET: "", - const.CONF_PROFILES: ["Person 1"], - } - ) + config_schema_assert_fail({CONF_CLIENT_ID: "my_client_id"}) + config_schema_assert_fail({CONF_CLIENT_ID: "my_client_id", CONF_CLIENT_SECRET: ""}) config_schema_validate( - { - CONF_CLIENT_ID: "my_client_id", - CONF_CLIENT_SECRET: "my_client_secret", - const.CONF_PROFILES: ["Person 1"], - } + {CONF_CLIENT_ID: "my_client_id", CONF_CLIENT_SECRET: "my_client_secret"} ) def test_config_schema_use_webhook() -> None: """Test schema.""" config_schema_validate( - { - CONF_CLIENT_ID: "my_client_id", - CONF_CLIENT_SECRET: "my_client_secret", - const.CONF_PROFILES: ["Person 1"], - } + {CONF_CLIENT_ID: "my_client_id", CONF_CLIENT_SECRET: "my_client_secret"} ) config = config_schema_validate( { CONF_CLIENT_ID: "my_client_id", CONF_CLIENT_SECRET: "my_client_secret", const.CONF_USE_WEBHOOK: True, - const.CONF_PROFILES: ["Person 1"], } ) assert config[const.DOMAIN][const.CONF_USE_WEBHOOK] is True @@ -116,7 +93,6 @@ def test_config_schema_use_webhook() -> None: CONF_CLIENT_ID: "my_client_id", CONF_CLIENT_SECRET: "my_client_secret", const.CONF_USE_WEBHOOK: False, - const.CONF_PROFILES: ["Person 1"], } ) assert config[const.DOMAIN][const.CONF_USE_WEBHOOK] is False @@ -125,49 +101,6 @@ def test_config_schema_use_webhook() -> None: CONF_CLIENT_ID: "my_client_id", CONF_CLIENT_SECRET: "my_client_secret", const.CONF_USE_WEBHOOK: "A", - const.CONF_PROFILES: ["Person 1"], - } - ) - - -def test_config_schema_profiles() -> None: - """Test schema.""" - config_schema_assert_fail( - {CONF_CLIENT_ID: "my_client_id", CONF_CLIENT_SECRET: "my_client_secret"} - ) - config_schema_assert_fail( - { - CONF_CLIENT_ID: "my_client_id", - CONF_CLIENT_SECRET: "my_client_secret", - const.CONF_PROFILES: "", - } - ) - config_schema_assert_fail( - { - CONF_CLIENT_ID: "my_client_id", - CONF_CLIENT_SECRET: "my_client_secret", - const.CONF_PROFILES: [], - } - ) - config_schema_assert_fail( - { - CONF_CLIENT_ID: "my_client_id", - CONF_CLIENT_SECRET: "my_client_secret", - const.CONF_PROFILES: ["Person 1", "Person 1"], - } - ) - config_schema_validate( - { - CONF_CLIENT_ID: "my_client_id", - CONF_CLIENT_SECRET: "my_client_secret", - const.CONF_PROFILES: ["Person 1"], - } - ) - config_schema_validate( - { - CONF_CLIENT_ID: "my_client_id", - CONF_CLIENT_SECRET: "my_client_secret", - const.CONF_PROFILES: ["Person 1", "Person 2"], } ) @@ -252,3 +185,40 @@ async def test_set_config_unique_id( await hass.config_entries.async_setup(config_entry.entry_id) assert config_entry.unique_id == "my_user_id" + + +async def test_set_convert_unique_id_to_string(hass: HomeAssistant) -> None: + """Test upgrading configs to use a unique id.""" + config_entry = MockConfigEntry( + domain=DOMAIN, + data={ + "token": {"userid": 1234}, + "auth_implementation": "withings", + "profile": "person0", + }, + ) + config_entry.add_to_hass(hass) + + hass_config = { + HA_DOMAIN: { + CONF_UNIT_SYSTEM: CONF_UNIT_SYSTEM_METRIC, + CONF_EXTERNAL_URL: "http://127.0.0.1:8080/", + }, + const.DOMAIN: { + CONF_CLIENT_ID: "my_client_id", + CONF_CLIENT_SECRET: "my_client_secret", + const.CONF_USE_WEBHOOK: False, + }, + } + + with patch( + "homeassistant.components.withings.common.ConfigEntryWithingsApi", + spec=ConfigEntryWithingsApi, + ): + await async_process_ha_core_config(hass, hass_config.get(HA_DOMAIN)) + assert await async_setup_component(hass, HA_DOMAIN, {}) + assert await async_setup_component(hass, webhook.DOMAIN, hass_config) + assert await async_setup_component(hass, const.DOMAIN, hass_config) + await hass.async_block_till_done() + + assert config_entry.unique_id == "1234"