Only wait out a refusal after a 2FA code, not every protocol error

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

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