Handle 2FA-required errors separately from password errors in iCloud - #181390
Handle 2FA-required errors separately from password errors in iCloud#181390TeroPihlaja wants to merge 1 commit into
Conversation
|
Hey there @Quentame, @nzapponi, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
🟡 Changes recommended
The mandatory PR-template sections must be restored, and the new tests need minor convention and duplication fixes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Separates iCloud 2FA challenges from invalid-password failures while preserving sessions needed for verification.
Changes:
- Adds targeted authentication-error handling and reauth behavior.
- Adds coverage for 2FA, password, and first-fetch failures.
File summaries
| File | Description |
|---|---|
homeassistant/components/icloud/account.py |
Classifies authentication failures and preserves 2FA sessions. |
tests/components/icloud/test_init.py |
Tests authentication failure paths and retry behavior. |
Review details
Suppressed comments (1)
tests/components/icloud/test_init.py:142
- Fold this coverage into the existing
test_setup_password_failed. That test already performs the same setup, reauth-flow, and cleared-session checks, so the log assertions can be added there instead of duplicating the scenario.
@pytest.mark.usefixtures("service_2fa_failed")
async def test_setup_password_failed_reports_bad_password(
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
5752c06 to
9189eb5
Compare
A 2FA challenge and a rejected password both end up in the same error path during setup, so a routine 2FA re-challenge was reported to the user as "your account is no longer working", pointing them at the password instead of the verification code. Report the two cases separately and keep the authenticated session when the account only needs a 2FA code: the reauth flow reuses that session to send and validate the code, and dropping it would send the user back to the setup form to re-enter a password that was never wrong. Also handle PyiCloudAuthRequiredException, which the session can raise between logging in and the first device fetch. That still raises ConfigEntryNotReady so the entry is retried, because the fetch timer is only armed once update_devices() completes and an entry that returned early here would stay loaded without ever polling again.
9189eb5 to
d2c4d2f
Compare
Proposed change
A 2FA challenge and a rejected password both end up in the same error path during setup, so a routine 2FA re-challenge is reported to the user as "your iCloud account is no longer working" — pointing them at the password instead of the verification code.
This reports the two cases separately, and keeps the authenticated session when the account only needs a code. That part matters beyond the wording:
async_step_reauthreadsself.api = self._get_reauth_entry().runtime_data.apiand, when it isNone, falls back to_show_setup_form()to ask for the password again. Dropping the session on a 2FA challenge would therefore send the user back to re-enter a password that was never wrong, instead of straight to the code entry.It also handles the session being rejected between logging in and the first device fetch. Reading
api.devicesrefreshes the session, and a stored token that iCloud has since invalidated is only rejected at that point rather than while logging in — surfacing asPyiCloudFailedLoginException: Invalid authentication token.orPyiCloudAuthRequiredException. Previously both escaped as an unhandled traceback with no reauth flow started, so the user got a failed entry and nothing to act on. That path still raisesConfigEntryNotReadyso the entry is retried with backoff: the fetch timer is only armed onceupdate_devices()completes, so returning early there would leave an entry that is loaded and never polls again.Type of change
Additional information
Supersedes #169005, which was closed by the stale bot rather than on review. Rebuilt as a single commit on current
dev.On #169005 it was asked whether this duplicated #169001. That was a fair reading of the old branches — the polling-loop branch carried the 2FA message split as well. The two are now cleanly separated: this PR only classifies the setup-time error, and #181389 only keeps the fetch timer alive. They touch the same file but not the same behaviour.
The existing
test_setup_2facontinues to assert that the session survives a 2FA challenge, which is what keeps the reauth flow going straight to code entry.This needs the
bugfixlabel for therequired-labelscheck to pass — I cannot add it myself.Checklist
ruff format 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.To help with the load of incoming pull requests: