Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow entries with same user_key for Pushover #77904

Merged
merged 3 commits into from Sep 29, 2022
Merged

Allow entries with same user_key for Pushover #77904

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4740021
Allow entries with same user_key for Pushover
engrbm87 Sep 6, 2022
40a115f
remove unique_id completely
engrbm87 Sep 7, 2022
14feb12
Abort reauth if entered api_key already exists
engrbm87 Sep 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions homeassistant/components/pushover/__init__.py
View file
Open in desktop
Expand Up @@ -25,6 +25,10 @@ async def async_setup(hass: HomeAssistant, config: ConfigType) -> bool:
async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
"""Set up pushover from a config entry."""

# remove unique_id for beta users
if entry.unique_id is not None:
hass.config_entries.async_update_entry(entry, unique_id=None)

pushover_api = PushoverAPI(entry.data[CONF_API_KEY])
try:
await hass.async_add_executor_job(
Expand Down
14 changes: 12 additions & 2 deletions homeassistant/components/pushover/config_flow.py
View file
Open in desktop
Expand Up @@ -62,6 +62,12 @@ async def async_step_reauth_confirm(
errors = {}
if user_input is not None and self._reauth_entry:
user_input = {**self._reauth_entry.data, **user_input}
self._async_abort_entries_match(
{
CONF_USER_KEY: user_input[CONF_USER_KEY],
CONF_API_KEY: user_input[CONF_API_KEY],
}
)
errors = await validate_input(self.hass, user_input)
if not errors:
self.hass.config_entries.async_update_entry(
Expand All @@ -87,9 +93,13 @@ async def async_step_user(
errors = {}

if user_input is not None:
await self.async_set_unique_id(user_input[CONF_USER_KEY])
self._abort_if_unique_id_configured()
Comment on lines -90 to -91
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the combination of user key and API key is unique right?

Shouldn't that be used as the unique identifier instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I first thought of that. But if the api key is removed the user can reauthenticate with a new api key. So that makes the api key not unique.
Can we update the unique_id when reauthenticating?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I read the documentation of the Pushover API the API token is unique. Wouldn't that solve the problem? If a user registers this in HA it would be a new instance. Just make the API not changeable for an existing configuration but document the procedure to change the API key is create a new configuration?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An API key can be regenerated which means for the same application an old api token can be revoked and new one used instead.
image
This means we should still provide a re-authentication flow to enter new api_key.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@engrbm87 Can I help you in any way? If a re-authentication flow is problematic I think a valid scenario for now would be to consider the API Token as unique and document the need to remove and recreate the configuration through the interface.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just create a GUID and use that as the unique_id. It is not connected to any combination of User Key and App Key and therefore you can reconfigure both without adjusting the unique_id. I feel this is not the right choice because there would be nothing stopping you from creating multiple services with the same User/App key pair. On the other hand, this does not break anything, just the behavior on the phone-app might not be wat the user expects.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just create a GUID and use that as the unique_id. It is not connected to any combination of User Key and App Key and therefore you can reconfigure both without adjusting the unique_id. I feel this is not the right choice because there would be nothing stopping you from creating multiple services with the same User/App key pair. On the other hand, this does not break anything, just the behavior on the phone-app might not be wat the user expects.

i know i'm a fringe user as this issue affects me to begin with, but i think i might be 'really' fringe user as i have(had until the HA update) two different Pushover User Keys defined. one for personal use and one for work (Group Key) that gets used by the team. locking the User Key to a single instance would still be a breaking change from the YAML config option that was originally available.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to reject adding another config entry with the same user_key + api_key as implemented in the PR.
However, should that check also be done in the reauth flow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the check in the reauth. Please check and confirm.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for progress here!
Do the code changes handle only creating new entries by GUI or would it also allow importing multiple entries from existing configuration.yaml?


self._async_abort_entries_match(
{
CONF_USER_KEY: user_input[CONF_USER_KEY],
CONF_API_KEY: user_input[CONF_API_KEY],
}
)
self._async_abort_entries_match({CONF_NAME: user_input[CONF_NAME]})

errors = await validate_input(self.hass, user_input)
Expand Down
51 changes: 45 additions & 6 deletions tests/components/pushover/test_config_flow.py
View file
Open in desktop
Expand Up @@ -48,12 +48,11 @@ async def test_flow_user(hass: HomeAssistant) -> None:
assert result["data"] == MOCK_CONFIG


async def test_flow_user_key_already_configured(hass: HomeAssistant) -> None:
"""Test user initialized flow with duplicate user key."""
async def test_flow_user_key_api_key_exists(hass: HomeAssistant) -> None:
"""Test user initialized flow with duplicate user key / api key pair."""
entry = MockConfigEntry(
domain=DOMAIN,
data=MOCK_CONFIG,
unique_id="MYUSERKEY",
)

entry.add_to_hass(hass)
Expand Down Expand Up @@ -171,7 +170,7 @@ async def test_reauth_success(hass: HomeAssistant) -> None:
data=MOCK_CONFIG,
)

assert result["type"] == "form"
assert result["type"] == FlowResultType.FORM
assert result["step_id"] == "reauth_confirm"

result2 = await hass.config_entries.flow.async_configure(
Expand All @@ -181,7 +180,7 @@ async def test_reauth_success(hass: HomeAssistant) -> None:
},
)

assert result2["type"] == "abort"
assert result2["type"] == FlowResultType.ABORT
assert result2["reason"] == "reauth_successful"


Expand Down Expand Up @@ -213,7 +212,47 @@ async def test_reauth_failed(hass: HomeAssistant, mock_pushover: MagicMock) -> N
},
)

assert result2["type"] == "form"
assert result2["type"] == FlowResultType.FORM
assert result2["errors"] == {
CONF_API_KEY: "invalid_api_key",
}


async def test_reauth_with_existing_config(hass: HomeAssistant) -> None:
"""Test reauth fails if the api key entered exists in another entry."""
entry = MockConfigEntry(
domain=DOMAIN,
data=MOCK_CONFIG,
)
entry.add_to_hass(hass)

second_entry = MOCK_CONFIG.copy()
second_entry[CONF_API_KEY] = "MYAPIKEY2"

entry2 = MockConfigEntry(
domain=DOMAIN,
data=second_entry,
)
entry2.add_to_hass(hass)

result = await hass.config_entries.flow.async_init(
DOMAIN,
context={
"source": config_entries.SOURCE_REAUTH,
"entry_id": entry.entry_id,
},
data=MOCK_CONFIG,
)

assert result["type"] == FlowResultType.FORM
assert result["step_id"] == "reauth_confirm"

result2 = await hass.config_entries.flow.async_configure(
result["flow_id"],
{
CONF_API_KEY: "MYAPIKEY2",
},
)

assert result2["type"] == FlowResultType.ABORT
assert result2["reason"] == "already_configured"
10 changes: 10 additions & 0 deletions tests/components/pushover/test_init.py
View file
Open in desktop
Expand Up @@ -68,6 +68,16 @@ async def test_async_setup_entry_success(hass: HomeAssistant) -> None:
assert entry.state == ConfigEntryState.LOADED


async def test_unique_id_updated(hass: HomeAssistant) -> None:
"""Test updating unique_id to new format."""
entry = MockConfigEntry(domain=DOMAIN, data=MOCK_CONFIG, unique_id="MYUSERKEY")
entry.add_to_hass(hass)
await hass.config_entries.async_setup(entry.entry_id)
await hass.async_block_till_done()
assert entry.state == ConfigEntryState.LOADED
assert entry.unique_id is None


async def test_async_setup_entry_failed_invalid_api_key(
hass: HomeAssistant, mock_pushover: MagicMock
) -> None:
Expand Down