Allow entries with same user_key for Pushover #77904
Conversation
probot-home-assistant
bot
added
bugfix
has-tests
integration: pushover
small-pr
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@mdehaas Please... please... don't ping or ask for reviews. This is kinda demanding (even if you didn't mean it like that). Please have patience. Thanks 👍 |
| await self.async_set_unique_id(user_input[CONF_USER_KEY]) | ||
| self._abort_if_unique_id_configured() |
There was a problem hiding this comment.
So the combination of user key and API key is unique right?
Shouldn't that be used as the unique identifier instead?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
An API key can be regenerated which means for the same application an old api token can be revoked and new one used instead.
This means we should still provide a re-authentication flow to enter new api_key.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I added the check in the reauth. Please check and confirm.
There was a problem hiding this comment.
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?
frenck
added
the
smash
engrbm87
force-pushed
the
pushover-multi-app
branch
from
f770b55
to
40a115f
Compare
Update tests
* Allow entries with same user_key for Pushover * remove unique_id completely * Abort reauth if entered api_key already exists Update tests
Breaking change
Proposed change
Pushover allows configuring mutiple applications with different api key. These applications will use the same user key.
This PR removes the unique_id so multiple applications (notify services) can be configured.
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: