diff --git a/python/exporter/icloud.py b/python/exporter/icloud.py index 164f08c0..949a7956 100644 --- a/python/exporter/icloud.py +++ b/python/exporter/icloud.py @@ -548,11 +548,18 @@ def code_was_already_spent(error: BaseException) -> bool: should be re-typed; this one has been consumed by the half that worked, so re-typing it is a guaranteed second failure. - The net is wide - `UnhandledProtocolError` is FindMy.py's "Apple said something this library - does not model" - but every failure it catches here happened *after* the submit returned, so - the code is gone in all of them. + **Only a refusal counts - an `AppleServiceUnavailableError`, which is what #168's 503 is now.** + This used to accept any `UnhandledProtocolError`, on the reasoning that everything raised after + the submit had consumed the code. True, and not enough: waiting and sending a new code is only + a recovery when the failure is weather. Issue #236 is Apple taking the code and then asking for + verification *again*, the same way every time - and the wide net turned that into a loop that + waited, sent a fresh code and failed identically, a dozen codes deep before the reporter read + the log. A `MobileMeDelegateError` is the same shape, and the wide net also kept it from the + terms handler both front ends put around :func:`log_in`. + + So anything else propagates unchanged, to the handlers that know what it means. """ - return isinstance(error, UnhandledProtocolError) + return isinstance(error, AppleServiceUnavailableError) async def _submit_code_with_retries(chosen, get_code, retry_code, announce=None): @@ -564,9 +571,9 @@ async def _submit_code_with_retries(chosen, get_code, retry_code, announce=None) (findmy-export 01-authentication ยง5). So somebody who mistyped a code they are still holding would lose it by being "helpfully" sent another, and a resend has to be a thing they choose. - **A code Apple took and then failed on is not re-typed and is not asked about**: it waits and - sends a new one by itself. Nothing the user could type would help - the code is spent - and the - only recovery anyone has observed is the passage of time. Asking "shall I try again?" of + **A code Apple took and then was refused on is not re-typed and is not asked about**: it waits + and sends a new one by itself. Nothing the user could type would help - the code is spent - and + the only recovery anyone has observed is the passage of time. Asking "shall I try again?" of somebody with no way to judge the answer is a worse interface than doing it. Bounded by the same attempt budget as a mistyped code, so the worst case is two waits and then @@ -576,6 +583,10 @@ async def _submit_code_with_retries(chosen, get_code, retry_code, announce=None) try: return await chosen.submit(await get_code()) except UnhandledProtocolError as e: + if not code_was_already_spent(e): + # Not weather, so a new code would meet the same answer. See #236. + raise + logger.info("Apple took the code and then failed to finish signing in: %s", e) # The last attempt has nothing left to wait for, and waiting before saying so would diff --git a/python/test/test_retries.py b/python/test/test_retries.py index f32447a4..b6431441 100644 --- a/python/test/test_retries.py +++ b/python/test/test_retries.py @@ -26,7 +26,11 @@ import pytest from findmy import InvalidCredentialsError, LoginState -from findmy.errors import UnhandledProtocolError +from findmy.errors import ( + AppleServiceUnavailableError, + MobileMeDelegateError, + UnhandledProtocolError, +) from findmy.keychain.recovery import RecoveryError from exporter import icloud @@ -267,11 +271,32 @@ async def submit(self, code: str): self.submitted.append(code) if len(self.submitted) <= self.fail_first: - raise UnhandledProtocolError("Error response for GSA request: 503") + raise AppleServiceUnavailableError(503, "The Grand Slam request") return LoginState.LOGGED_IN +class FakeFactorThatTakesTheCodeThenFailsWith: + """ + A second factor whose submit fails the same way every time, with an error that is not weather. + + Issue #236 is the model: the code is taken, and the re-authentication behind it comes back + asking for 2FA again. Counts what it was asked for, because the bug was what it cost. + """ + + def __init__(self, error: Exception) -> None: + self.error = error + self.submitted: list[str] = [] + self.requests = 0 + + async def request(self) -> None: + self.requests += 1 + + async def submit(self, code: str): + self.submitted.append(code) + raise self.error + + @pytest.fixture def no_waiting(monkeypatch): """ @@ -299,7 +324,7 @@ class TestACodeAppleTookAndThenFailedOn: def test_it_is_recognised_as_a_spent_code(self): assert icloud.code_was_already_spent( - UnhandledProtocolError("Error response for GSA request: 503")) + AppleServiceUnavailableError(503, "The Grand Slam request")) def test_a_rejected_code_is_not_a_spent_one(self): """The opposite case, and the one where re-typing is right.""" @@ -557,6 +582,55 @@ async def _unused(*_args, **_kwargs): raise AssertionError("should not have been asked") +STILL_ASKING_FOR_2FA = UnhandledProtocolError( + "Unexpected state after submitting 2FA: LoginState.REQUIRE_2FA") + + +class TestAFailureThatIsNotWeather: + """ + Issue #236: Apple took the code and asked for verification again, identically every time. + + The spent-code recovery waits and sends a new code, which is right for a refusal and useless + for this - it ran three codes per sign-in, the reporter tried six times, and nothing about the + answer changed. Anything that is not a refusal has to leave after one submit, unchanged, so the + front end's own handlers see it. + """ + + @pytest.mark.parametrize("error", [ + STILL_ASKING_FOR_2FA, + UnhandledProtocolError("SMS 2FA request failed: 400"), + ], ids=["still-asking-for-2fa", "a-status-that-is-not-a-refusal"]) + def test_it_is_not_a_spent_code(self, error): + assert not icloud.code_was_already_spent(error) + + def test_it_spends_one_code_and_stops(self, no_waiting): + factor = FakeFactorThatTakesTheCodeThenFailsWith(STILL_ASKING_FOR_2FA) + codes = iter(["111111", "222222", "333333"]) + + with pytest.raises(UnhandledProtocolError) as raised: + asyncio.run(icloud._submit_code_with_retries(factor, lambda: _next(codes), _unused)) + + assert raised.value is STILL_ASKING_FOR_2FA, "it has to arrive unchanged" + assert factor.submitted == ["111111"] + assert factor.requests == 0, "a new code was sent for a failure a new code cannot fix" + + def test_the_delegate_error_reaches_the_terms_handler(self, no_waiting): + """ + Both front ends catch `MobileMeDelegateError` around `log_in` to offer the terms. It is an + `UnhandledProtocolError` by inheritance, so the wide net took it first, and after 2FA that + handler could never run. + """ + error = MobileMeDelegateError(localized_error="TERMS", status=1) + factor = FakeFactorThatTakesTheCodeThenFailsWith(error) + codes = iter(["111111"]) + + with pytest.raises(MobileMeDelegateError) as raised: + asyncio.run(icloud._submit_code_with_retries(factor, lambda: _next(codes), _unused)) + + assert raised.value is error + assert factor.requests == 0 + + class TestTheDiagnosticsSurvive: """ `-vv` has to cover the module that explains a failure, not the modules that were interesting