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.

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

Allow users to update their iCloud password when auth fails #39138

Merged
merged 8 commits into from
Oct 10, 2020

Conversation

raman325
Copy link
Contributor

@raman325 raman325 commented Aug 22, 2020

Proposed change

I recently changed my iCloud password and noticed I had to delete and recreate the integration to update it. This change sets up a new discovery config flow when authentication fails that will update the password of the existing config entry. I saw multiple suggestions for how to handle this in home-assistant/architecture#377 but there hasn't been consensus yet, and I saw this pattern in other integrations such as airvisual and azure_devops so I thought it was a safe approach. One thing I did differently here was to create a frontend notification for the user so that the user had more context of what to do.

I tried to minimize changes to existing code, and verified that the exception that is being used to capture auth failing is always due to an incorrect email/password combo.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link

Hey there @Quentame, mind taking a look at this pull request as its been labeled with an integration (icloud) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@raman325 raman325 force-pushed the icloud_reauthenticate branch 2 times, most recently from bd88662 to 9395d87 Compare August 22, 2020 06:56
@raman325 raman325 force-pushed the icloud_reauthenticate branch from 9a34e63 to 89d16fa Compare September 23, 2020 21:25
@raman325
Copy link
Contributor Author

raman325 commented Sep 23, 2020

I updated the logic to use SOURCE_REAUTH which was added in #40352 as a result of home-assistant/frontend#6808 which makes HA "re-auth needed"-aware

Copy link
Member

@Quentame Quentame left a comment

Choose a reason for hiding this comment

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

Did not check the tests

homeassistant/components/icloud/account.py Outdated Show resolved Hide resolved
homeassistant/components/icloud/account.py Outdated Show resolved Hide resolved
@raman325
Copy link
Contributor Author

raman325 commented Sep 25, 2020

BTW @Quentame I had made a suggestion in #36120 that's a bit ugly but would resolve that issue. On setup, when there is 2FA, we can store the timestamp for when that occurred in the config entry and then setup a followup callback to trigger a fresh token grab in the future (let's say 1 month from the last refresh).

Copy link
Member

@Quentame Quentame left a comment

Choose a reason for hiding this comment

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

LGTM

@raman325
Copy link
Contributor Author

raman325 commented Oct 9, 2020

test failure seems to be unrelated to this PR

@JulienFloris

This comment has been minimized.

@Quentame
Copy link
Member

Quentame commented Oct 9, 2020

I am not used to see persistant notifications, but:
Is it possible to add a link to the integration page ?
Or directly the link to the integration re_auth flow ?

@Quentame
Copy link
Member

Quentame commented Oct 9, 2020

@JulienFloris answered here #41315

@THATDONFC
Copy link

I appreciate the work being done on this. When it's time to re-authenticate iCloud I have to remove and re-add the integration. It would be so much quicker if I could just enter the code to authenticate when the authentication expires.

raman325 and others added 2 commits October 9, 2020 08:36
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
@raman325
Copy link
Contributor Author

I think we are good on this PR, going to go ahead and merge it so that I can work on #36120 as it appears to be affecting a lot of users

@raman325 raman325 merged commit 025bdd7 into home-assistant:dev Oct 10, 2020
@raman325 raman325 deleted the icloud_reauthenticate branch October 10, 2020 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants