Issue #176. The screen echoed the failure verbatim, so somebody entering correct + * credentials during a Grand Slam outage was shown a sentence about HTTP 503 and an Apple + * internal service name. It reads as a bug in this app, and was filed as one. + * + *
Asserted on all three halves, because each is a different wrong turn: that it does not + * blame the password, that it does not send them to check a working connection, and that the + * raw protocol text is gone from the screen. + */ + @Test + public void appleDecliningIsNotBlamedOnThePasswordOrTheNetwork() { + this.apple = FakeAppleAuthService.appleIsDeclining(); + AppDependencies.replaceAuthService(this.apple); + + launch(); + signIn(); + + final android.content.Context context = getInstrumentation().getTargetContext(); + + Eventually.check(() -> onView(withId(R.id.login_error_message_text)).check(matches( + withText(context.getString(R.string.login_failed_apple_declined))))); + + onView(withId(R.id.login_error_message_text)).check(matches( + not(withText(context.getString(R.string.login_failed_network))))); + onView(withId(R.id.login_error_message_text)).check(matches( + not(withText(containsString("503"))))); + onView(withId(R.id.login_error_message_text)).check(matches( + not(withText(containsString("Grand Slam"))))); + } + /** A rejected code says so, and gives the boxes back rather than stranding them. */ @Test public void aWrongCodeIsReportedAndTheBoxesComeBack() { diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/FetchFromICloudFlowTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/FetchFromICloudFlowTest.java index 9be60d1c..b15adf03 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/FetchFromICloudFlowTest.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/FetchFromICloudFlowTest.java @@ -353,6 +353,35 @@ public void aserviceHavingABadDayOffersARetryInstead() { TestPace.afterAStep(); } + /** + * Apple declining reaches the retry screen, saying which of the two it is. + * + *
Issue #176. Before this it fell through to the default branch, which puts the + * failure's own detail on screen - an HTTP status and Apple's internal service name. Correct, + * unreadable, and indistinguishable from a bug in this app. + * + *
Asserted on the raw text being absent as well as the sentence being present, because a + * branch that showed both would pass a check for only the second. + */ + @Test + public void appleDecliningSaysSoRatherThanShowingTheStatusCode() { + this.open(FakeICloudService.whereAppleIsDeclining()); + + final android.content.Context context = + androidx.test.platform.app.InstrumentationRegistry + .getInstrumentation().getTargetContext(); + + Eventually.check(() -> onView(withId(R.id.icloud_retry_container)) + .check(matches(isDisplayed()))); + Eventually.check(() -> onView(withId(R.id.icloud_retry_body)).check(matches( + withText(context.getString(R.string.icloud_apple_declined_body))))); + + onView(withId(R.id.icloud_retry_body)).check(matches( + not(withText(org.hamcrest.Matchers.containsString("503"))))); + onView(withId(R.id.icloud_no_tags_container)).check(matches(not(isDisplayed()))); + TestPace.afterAStep(); + } + /** Retrying starts a fresh session rather than reusing the one that failed. */ @Test public void retryingAsksAgain() { diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/python/FakeAppleAuthService.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/python/FakeAppleAuthService.java index 7d54e723..26bd37e7 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/python/FakeAppleAuthService.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/python/FakeAppleAuthService.java @@ -118,6 +118,24 @@ public static FakeAppleAuthService cannotReachApple() { return fake; } + /** + * Apple answered and refused to serve, which is neither a wrong password nor a dead network. + * + *
Its own state because the message is the giveaway: the real one is a sentence about the + * Grand Slam request and an HTTP status, which the screen used to echo verbatim. That reads + * as a bug in this app, and issue #176 is somebody reporting it as one. + */ + public static FakeAppleAuthService appleIsDeclining() { + final FakeAppleAuthService fake = + new FakeAppleAuthService(LOGIN_STATE.LOGGED_OUT, null); + fake.loginFailsWith = new PythonAccountLoginException( + "The Grand Slam request was refused with HTTP 503. This is Apple declining to" + + " serve the request rather than a response this library cannot read," + + " and it usually clears on its own -- wait and try again.", + PythonAccountLoginException.REASON_APPLE_DECLINED); + return fake; + } + /** Signing in works, but the code that gets typed is refused. */ public FakeAppleAuthService thatRejectsTheCode(final String message) { this.codeFailsWith = new PythonAccountLoginException(message); diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/python/icloud/FakeICloudService.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/python/icloud/FakeICloudService.java index df64c2d9..f9e0196d 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/python/icloud/FakeICloudService.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/python/icloud/FakeICloudService.java @@ -139,6 +139,22 @@ public static FakeICloudService whereTheServiceIsUnsure() { return fake; } + /** + * Apple answered and refused to serve, rather than reporting nothing usable. + * + *
Kept apart from {@link #whereTheServiceIsUnsure()} because both reach the retry screen + * and they are not the same fact. That one is the keychain service having a bad day; this is + * an endpoint returning a 5xx, and it was reaching the screen as UNKNOWN with the HTTP + * status on it - issue #176. + */ + public static FakeICloudService whereAppleIsDeclining() { + final FakeICloudService fake = new FakeICloudService(); + fake.optionsFailsWith = new ICloudException( + ICloudFailure.APPLE_DECLINED, + "The Grand Slam request was refused with HTTP 503."); + return fake; + } + /** An account with a Mac on it and no tags - the empty fetch, one step later. */ public static FakeICloudService withNoTagsOnTheAccount() { final FakeICloudService fake = new FakeICloudService(); diff --git a/app/src/main/java/dev/wander/android/opentagviewer/AppleLoginActivity.java b/app/src/main/java/dev/wander/android/opentagviewer/AppleLoginActivity.java index ed59f270..24045d64 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/AppleLoginActivity.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/AppleLoginActivity.java @@ -734,6 +734,13 @@ private String describeLoginFailure(final Throwable error) { return this.getString(R.string.login_failed_network); } + // **Before the fallback, which would show "The Grand Slam request was refused with HTTP + // 503" verbatim.** That is accurate and reads as a bug in this app, which is how issue + // #176 came to be filed. Nothing about the Apple ID or the password is wrong here. + if (PythonAccountLoginException.REASON_APPLE_DECLINED.equals(reason)) { + return this.getString(R.string.login_failed_apple_declined); + } + // Reached when the terms path was tried and produced nothing to accept, so the sentence // says what Apple said and then that accepting terms will not fix something else - // rather than asserting a cause that has not been established. diff --git a/app/src/main/java/dev/wander/android/opentagviewer/FetchFromICloudActivity.java b/app/src/main/java/dev/wander/android/opentagviewer/FetchFromICloudActivity.java index 55c895f4..73225ea9 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/FetchFromICloudActivity.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/FetchFromICloudActivity.java @@ -698,6 +698,16 @@ private void showFailure(final Throwable error) { SignInAgain.from(this); break; + case APPLE_DECLINED: + // The retry screen, like SERVICE_UNSURE, but saying which of the two it is. + // Falling through to `default` would put the raw detail on screen - an HTTP + // status and a sentence about Grand Slam - which reads as a bug here. + this.showOnly(R.id.icloud_retry_container, + R.string.icloud_apple_declined_title, Direction.FORWARD); + ((TextView) this.findViewById(R.id.icloud_retry_body)) + .setText(R.string.icloud_apple_declined_body); + break; + case SERVICE_UNSURE: default: // Everything unrecognised lands here on purpose: "try again later" is the safe diff --git a/app/src/main/java/dev/wander/android/opentagviewer/python/PythonAccountLoginException.java b/app/src/main/java/dev/wander/android/opentagviewer/python/PythonAccountLoginException.java index 016d78bb..fd2676c2 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/python/PythonAccountLoginException.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/python/PythonAccountLoginException.java @@ -34,6 +34,14 @@ public class PythonAccountLoginException extends RuntimeException { public static final String REASON_TERMS = "terms"; /** Anything not recognised. The detail is shown as-is rather than guessed at. */ + /** + * Apple answered and refused to serve. Matches {@code REASON_APPLE_DECLINED}. + * + *
Kept apart from {@link #REASON_NETWORK} because the advice differs: a network failure is + * usually the phone's and worth checking, and this one is Apple's and is not. + */ + public static final String REASON_APPLE_DECLINED = "apple_declined"; + public static final String REASON_UNKNOWN = "unknown"; private final String reason; diff --git a/app/src/main/java/dev/wander/android/opentagviewer/python/icloud/ICloudFailure.java b/app/src/main/java/dev/wander/android/opentagviewer/python/icloud/ICloudFailure.java index 3c3154f0..c9c56705 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/python/icloud/ICloudFailure.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/python/icloud/ICloudFailure.java @@ -100,6 +100,21 @@ public enum ICloudFailure { */ NOT_AN_ACCESSORY, + /** + * Apple answered and refused to serve. Worth waiting out. + * + *
Not {@link #CREDENTIALS_REJECTED} and not {@link #UNKNOWN}, and the difference is + * expensive in both directions. Treated as rejected credentials it signs somebody out + * over a 503, destroying a working session for nothing. Left unknown it reaches a screen + * showing an HTTP status and a sentence about Grand Slam, which reads as a bug in this app - + * see issue #176, where that is what somebody reported. + * + *
Closest to {@link #SERVICE_UNSURE}, which also means "try later", and kept apart + * because that one is about the keychain service reporting nothing usable rather than about + * an endpoint declining. + */ + APPLE_DECLINED, + /** Anything else. The detail carries what there is to say. */ UNKNOWN; @@ -123,6 +138,7 @@ public static ICloudFailure fromWire(final String reason) { case "no_such_record": return NO_SUCH_RECORD; case "not_unlocked": return NOT_UNLOCKED; case "credentials_rejected": return CREDENTIALS_REJECTED; + case "apple_declined": return APPLE_DECLINED; case "membership_unusable": return MEMBERSHIP_UNUSABLE; case "no_such_accessory": return NO_SUCH_ACCESSORY; case "not_an_accessory": return NOT_AN_ACCESSORY; diff --git a/app/src/main/java/dev/wander/android/opentagviewer/util/rx/ACodeAppleAlreadyTook.java b/app/src/main/java/dev/wander/android/opentagviewer/util/rx/ACodeAppleAlreadyTook.java index aeaccd0f..40cfab36 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/util/rx/ACodeAppleAlreadyTook.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/util/rx/ACodeAppleAlreadyTook.java @@ -61,15 +61,25 @@ private ACodeAppleAlreadyTook() { * code, which is inconvenient and correct. * *
Matched on the name because that is what survives the bridge: the failure arrives from - * Chaquopy as a {@code PyException} whose message carries the Python class. FindMy.py folds - * every non-OK status into {@code UnhandledProtocolError} with only the number in it, so - * there is nothing better to match on until the fork grows a transient-failure type - see the - * handover note, which explains why that was left out of the first fix. + * Chaquopy as a {@code PyException} whose message carries the Python class. + * + *
Two names, and the second one is why this had to change. FindMy.py used to fold + * every non-OK status into {@code UnhandledProtocolError}; the fork now raises + * {@code AppleServiceUnavailableError} for a 429 or a 5xx, which is a subclass and therefore + * invisible to a match on the parent's name. Matching only the old name would have made this + * quietly stop firing the moment the pin moved - the recovery would still be here, still + * tested, and never reached, because every test in this file writes the old name into its + * own fixture. + * + *
The old name is kept alongside it. It still covers a protocol failure after the submit
+ * that is not a 5xx, which is the wide net the paragraph above is about.
*/
public static boolean spentIt(final Throwable error) {
for (Throwable cause = error; cause != null; cause = cause.getCause()) {
final String message = cause.getMessage();
- if (message != null && message.contains("UnhandledProtocolError")) {
+ if (message != null
+ && (message.contains("UnhandledProtocolError")
+ || message.contains("AppleServiceUnavailableError"))) {
return true;
}
if (cause.getCause() == cause) {
diff --git a/app/src/main/python/icloud_bridge.py b/app/src/main/python/icloud_bridge.py
index 88b0de37..cbee16d9 100644
--- a/app/src/main/python/icloud_bridge.py
+++ b/app/src/main/python/icloud_bridge.py
@@ -43,7 +43,7 @@
import identity as app_identity
from exporter import icloud
-from findmy.errors import InvalidCredentialsError
+from findmy.errors import AppleServiceUnavailableError, InvalidCredentialsError
from findmy.keychain.enrolment import DeviceDescription
from findmy.keychain.join import JoinedPeer
from findmy.keychain.recovery import RecoveryError
@@ -125,6 +125,20 @@
naming record as its single source of truth.
"""
+REASON_APPLE_DECLINED = "apple_declined"
+"""
+Apple answered and refused to serve. Worth waiting out, and not the user's fault.
+
+**Separate from :data:`REASON_UNKNOWN` because the screen written for unknown says the wrong
+thing.** Unknown shows whatever detail there is, which here is a sentence about Grand Slam and an
+HTTP status - accurate, unreadable, and indistinguishable from a bug in this app. See
+OpenTagViewer#176, where somebody filed exactly that.
+
+Separate from :data:`REASON_CREDENTIALS_REJECTED` for the opposite reason: that one is permanent
+and ends in a forced sign-out. Signing somebody out over a 503 destroys a working session for
+nothing, which is rule 15's failure mode in the expensive direction.
+"""
+
REASON_UNKNOWN = "unknown"
"""Anything else, with the exception text carried through so a report can be answered."""
@@ -176,6 +190,11 @@ def _unexpected(what: str) -> str:
if _needsAFreshSignIn(sys.exc_info()[1]):
return _failure(REASON_CREDENTIALS_REJECTED, _lastLineOf(detail) or what + " was refused")
+ # After the credentials check and before the fallback. Apple declining is neither a dead
+ # session nor a mystery, and it is the one failure here that is worth simply waiting out.
+ if isinstance(sys.exc_info()[1], AppleServiceUnavailableError):
+ return _failure(REASON_APPLE_DECLINED, _lastLineOf(detail) or what + " was declined")
+
# `str(e)` is empty for several of these - TimeoutError most of all - so the last line of
# the traceback stands in. It names the exception type, which is not a good message but is
# infinitely better than a colon with nothing after it.
diff --git a/app/src/main/python/main.py b/app/src/main/python/main.py
index 2341818a..cd5555fd 100644
--- a/app/src/main/python/main.py
+++ b/app/src/main/python/main.py
@@ -10,6 +10,7 @@
import NSKeyedUnArchiver
from findmy import FindMyAccessory, MobileMeDelegateError
+from findmy.errors import AppleServiceUnavailableError
from findmy.accessory import FixedRollingKeyPairAccessory
from findmy.keys import KeyPairType
from findmy.reports import (
@@ -458,6 +459,21 @@ def assertAnisetteIsSupported(serializedAccountData: str) -> str | None:
screen that keeps refusing them for a reason nothing tells them.
"""
+REASON_APPLE_DECLINED = "apple_declined"
+"""
+Apple answered, and refused to serve the request. Not the password, and not the network.
+
+**A 503 from Grand Slam reads as a rejected sign-in to everybody who meets it**, because the only
+thing on screen is a failure at the moment a password was entered. It is neither: nothing was
+wrong with the credentials, nothing was reached and refused on their merits, and it clears on its
+own within minutes. Reported as OpenTagViewer#176, where one account met the same 503 three times
+in three minutes at three different call sites.
+
+Distinguished from :data:`REASON_NETWORK` because the advice differs. A network failure is
+usually the phone's - check the connection. This one is Apple's, and checking anything at this
+end is wasted effort.
+"""
+
REASON_UNKNOWN = "unknown"
"""Anything else. The detail is shown as-is, because a wrong guess is worse than raw text."""
@@ -484,6 +500,13 @@ def classifyLoginFailure(error: BaseException) -> str:
if isinstance(error, MobileMeDelegateError):
return REASON_TERMS
+ # Before the network checks, and not because of ordering hazards - it is a RuntimeError and
+ # collides with none of them. It is here because it reads as the same thing to a user and is
+ # not: Apple answered. Telling somebody to check their connection when their connection is
+ # fine sends them to reset a router over a 503.
+ if isinstance(error, AppleServiceUnavailableError):
+ return REASON_APPLE_DECLINED
+
if isinstance(error, (asyncio.TimeoutError, asyncio.CancelledError)):
return REASON_NETWORK
if isinstance(error, _NETWORK_ERRORS):
diff --git a/app/src/main/res/values-de/strings.xml b/app/src/main/res/values-de/strings.xml
index 9dc1ddd8..cfbb0154 100644
--- a/app/src/main/res/values-de/strings.xml
+++ b/app/src/main/res/values-de/strings.xml
@@ -366,6 +366,9 @@ Deine Tags und ihr Standortverlauf sind davon nicht betroffen, und aus deinem Ap
Issue #176: one account met the same 503 at three call sites in three minutes. Nothing + * distinguished it, so it arrived as {@code UNKNOWN} and the screen showed an HTTP status and a + * sentence about Grand Slam - which reads as a bug in this app, and was reported as one. + * + *
Pure mapping and a pure predicate, so JVM - rule 13. + */ +public class AppleDecliningIsItsOwnFailureTest { + + @Test + public void thebridgesWireValueMaps() { + assertEquals(ICloudFailure.APPLE_DECLINED, ICloudFailure.fromWire("apple_declined")); + } + + /** + * The expensive mistake, in the direction that costs a working session. + * + *
{@code CREDENTIALS_REJECTED} ends in {@code SignInAgain}. Reaching it over a fault that + * clears itself in minutes signs somebody out for nothing. + */ + @Test + public void itisNotRejectedCredentials() { + assertNotEquals(ICloudFailure.CREDENTIALS_REJECTED, ICloudFailure.fromWire("apple_declined")); + } + + /** + * And it must not reach the sign-out path through the shared predicate either. + * + *
Rule 15's whole point is that one question decides this for every screen. A new failure + * that answered it wrongly would take every caller with it. + */ + @Test + public void itdoesNotAskAnybodyToSignInAgain() { + assertFalse(ICloudFailures.meansSignInAgain( + new ICloudException(ICloudFailure.APPLE_DECLINED, "refused with HTTP 503"))); + } + + /** + * The other direction: it is no longer UNKNOWN. + * + *
Which is what it was when #176 was filed. + */ + @Test + public void itisNoLongerUnknown() { + assertNotEquals(ICloudFailure.UNKNOWN, ICloudFailure.fromWire("apple_declined")); + } + + /** + * An unrecognised reason still degrades rather than throwing. + * + *
A reason added in Python and not yet known here has to become a poor screen, never a + * crash on the screen that reports failures. + */ + @Test + public void areasonThisBuildHasNeverHeardOfIsStillUnknown() { + assertEquals(ICloudFailure.UNKNOWN, ICloudFailure.fromWire("something_invented_later")); + assertEquals(ICloudFailure.UNKNOWN, ICloudFailure.fromWire(null)); + } +} diff --git a/app/src/test/java/dev/wander/android/opentagviewer/util/rx/ACodeAppleAlreadyTookTest.java b/app/src/test/java/dev/wander/android/opentagviewer/util/rx/ACodeAppleAlreadyTookTest.java index 1b6fde2d..86dfaa6e 100644 --- a/app/src/test/java/dev/wander/android/opentagviewer/util/rx/ACodeAppleAlreadyTookTest.java +++ b/app/src/test/java/dev/wander/android/opentagviewer/util/rx/ACodeAppleAlreadyTookTest.java @@ -103,4 +103,44 @@ public void thereAreExactlyTwoAttemptsAndThenItGivesUp() { public void aNonsenseAttemptNumberDoesNotWait() { assertEquals(-1, ACodeAppleAlreadyTook.waitBefore(-1)); } + + /** + * The name the fork raises now, which is the one a real 503 arrives as. + * + *
Every other test here writes the old name into its own fixture, so all of them + * kept passing when FindMy.py started raising a subclass and this stopped matching. A test + * that supplies the string it is looking for cannot notice the string changing. + */ + @Test + public void a503ArrivingAsTheForksTransientTypeIsRecognised() { + assertTrue(ACodeAppleAlreadyTook.spentIt(fromTheBridge( + "findmy.errors.AppleServiceUnavailableError: The Grand Slam request was refused" + + " with HTTP 503. This is Apple declining to serve the request rather" + + " than a response this library cannot read, and it usually clears on" + + " its own -- wait and try again."))); + } + + /** + * And through a wrapper, because that is how it reaches the screen. + */ + @Test + public void theforksTypeIsFoundThroughAWrappingException() { + final Throwable wrapped = new RuntimeException( + "submitCode failed", + fromTheBridge("findmy.errors.AppleServiceUnavailableError: refused with HTTP 503")); + + assertTrue(ACodeAppleAlreadyTook.spentIt(wrapped)); + } + + /** + * The old name still counts. + * + *
It covers a protocol failure after the submit that is not a 5xx, which the fork still + * reports as the parent type. + */ + @Test + public void theolderNameIsStillRecognisedSoTheWideNetSurvives() { + assertTrue(ACodeAppleAlreadyTook.spentIt(fromTheBridge( + "UnhandledProtocolError: Error response for GSA request: 418"))); + } } diff --git a/app/src/test/python/requirements.txt b/app/src/test/python/requirements.txt index a365cb9e..84952e1d 100644 --- a/app/src/test/python/requirements.txt +++ b/app/src/test/python/requirements.txt @@ -9,7 +9,7 @@ # `FindMy==0.9.8` here for as long as the app built the fork, so every bridge test # ran against a library the app does not ship - which is not a small difference: # the fork's Anisette providers take `serial=` and PyPI's do not. -git+https://github.com/parawanderer/FindMy.py@ddc7f2342fc9f32ebe315b85c22a4554ce419f6d +git+https://github.com/parawanderer/FindMy.py@3c2b4926252193e9cd265b39fa52252adcbaad4e NSKeyedUnArchiver==1.5 PyYAML==6.0.2 diff --git a/app/src/test/python/test_icloud_bridge.py b/app/src/test/python/test_icloud_bridge.py index 7fee4ffd..f1fb5618 100644 --- a/app/src/test/python/test_icloud_bridge.py +++ b/app/src/test/python/test_icloud_bridge.py @@ -997,6 +997,49 @@ def test_an_unrelated_value_error_is_not_a_dead_session(self): assert answer["reason"] == icloud_bridge.REASON_UNKNOWN + def test_apple_declining_is_its_own_reason(self): + """ + Issue #176. Neither a dead session nor a mystery. + + Classified in `_unexpected` so all eight entry points get it - the same argument the + credentials check above is here for. + """ + from findmy.errors import AppleServiceUnavailableError + + try: + raise AppleServiceUnavailableError(503, "The Grand Slam request") + except AppleServiceUnavailableError: + answer = json.loads(icloud_bridge._unexpected("opening the Find My client")) + + assert answer["reason"] == icloud_bridge.REASON_APPLE_DECLINED + + def test_apple_declining_does_not_sign_anybody_out(self): + """ + The expensive half of getting this wrong. + + CREDENTIALS_REJECTED ends in a forced sign-out. Reaching it over a 503 destroys a working + session for a fault that clears itself in minutes. + """ + from findmy.errors import AppleServiceUnavailableError + + try: + raise AppleServiceUnavailableError(503, "The Grand Slam request") + except AppleServiceUnavailableError: + answer = json.loads(icloud_bridge._unexpected("reading the account's accessories")) + + assert answer["reason"] != icloud_bridge.REASON_CREDENTIALS_REJECTED + + def test_an_ordinary_protocol_error_is_still_unknown(self): + """A response this library cannot read is still worth reporting, so it stays UNKNOWN.""" + from findmy.errors import UnhandledProtocolError + + try: + raise UnhandledProtocolError("Error response for GSA request: 418") + except UnhandledProtocolError: + answer = json.loads(icloud_bridge._unexpected("opening the Find My client")) + + assert answer["reason"] == icloud_bridge.REASON_UNKNOWN + def test_an_unauthorized_error_is_deliberately_left_alone(self): """ **Two meanings, so it stays unclassified until they can be told apart.** diff --git a/app/src/test/python/test_main.py b/app/src/test/python/test_main.py index db46614e..94bd2624 100644 --- a/app/src/test/python/test_main.py +++ b/app/src/test/python/test_main.py @@ -1192,6 +1192,44 @@ def test_anything_else_is_left_unclassified(self): """ assert main.classifyLoginFailure(ValueError("bad password")) == main.REASON_UNKNOWN + def test_apple_declining_is_not_a_network_failure(self): + """ + Issue #176. Apple answered, and refused. + + Told to check their connection, somebody with a perfectly good connection resets a + router over a 503. The two failures look identical on screen and want opposite advice. + """ + from findmy.errors import AppleServiceUnavailableError + + declined = AppleServiceUnavailableError(503, "The Grand Slam request") + + assert main.classifyLoginFailure(declined) == main.REASON_APPLE_DECLINED + assert main.classifyLoginFailure(declined) != main.REASON_NETWORK + + def test_apple_declining_is_not_left_unclassified(self): + """ + UNKNOWN shows the detail verbatim, which here is a sentence about Grand Slam and an HTTP + status. Accurate, and it reads as a bug in this app - which is what got reported. + """ + from findmy.errors import AppleServiceUnavailableError + + assert main.classifyLoginFailure( + AppleServiceUnavailableError(429, "The two-factor request"), + ) != main.REASON_UNKNOWN + + def test_an_ordinary_protocol_error_is_still_unclassified(self): + """ + The subclass is the signal, not the parent. + + A response this library genuinely cannot read is still a bug worth reporting, and must + not be swept into "wait a few minutes". + """ + from findmy.errors import UnhandledProtocolError + + assert main.classifyLoginFailure( + UnhandledProtocolError("Error response for GSA request: 418"), + ) == main.REASON_UNKNOWN + def test_a_library_error_is_recognised_by_its_module(self): class ClientConnectorError(Exception): pass diff --git a/python/exporter/cli.py b/python/exporter/cli.py index 113f30a1..245a8ef6 100644 --- a/python/exporter/cli.py +++ b/python/exporter/cli.py @@ -32,7 +32,7 @@ from typing import Sequence from findmy import InvalidCredentialsError, LoginState, MobileMeDelegateError, TermsError -from findmy.errors import UnhandledProtocolError +from findmy.errors import AppleServiceUnavailableError, UnhandledProtocolError from findmy.keychain.recovery import RecoveryError from exporter import icloud, localsource, prompts, secrets, source, terms @@ -949,6 +949,12 @@ def _run_and_return(arguments: argparse.Namespace) -> int: ) as e: print(f"\n{e}", file=sys.stderr) return 1 + except AppleServiceUnavailableError as e: + # **Before the UnhandledProtocolError handler below**, which asks for a bug report and a + # -vv rerun. A 503 is Apple declining rather than a response this program cannot read; + # issue #176 is somebody filing one because nothing said otherwise. + print(f"\n{icloud.describe_apple_declining(e)}", file=sys.stderr) + return 1 except RecoveryError as e: # **Before the handler below, and deliberately not through it.** RecoveryError is an # UnhandledProtocolError by inheritance, but it is not an unmodelled shape: it is a diff --git a/python/exporter/icloud.py b/python/exporter/icloud.py index b232d69d..91bba515 100644 --- a/python/exporter/icloud.py +++ b/python/exporter/icloud.py @@ -36,7 +36,7 @@ SmsSecondFactorMethod, TrustedDeviceSecondFactorMethod, ) -from findmy.errors import UnhandledProtocolError +from findmy.errors import AppleServiceUnavailableError, UnhandledProtocolError from findmy.accessory import _extract_serial_from_stable_id # noqa: PLC2701 - see _candidate from findmy.cloudkit.beacons import ( AsyncBeaconStore, @@ -562,6 +562,47 @@ async def _wait_then_send_a_new_code(chosen, seconds: int, announce=None) -> Non await chosen.request() +def apple_is_declining(error: BaseException) -> AppleServiceUnavailableError | None: + """ + The `AppleServiceUnavailableError` in this failure, if there is one. + + **A 503 from Apple is not a bug in this program, and until FindMy.py could say so there was + no way to tell.** Every non-OK status arrived as `UnhandledProtocolError`, whose meaning is + "Apple said something this library does not model" - so the wizard's catch-all offered the + issue tracker, and people took it up. Issue #176 is one account meeting the same 503 at three + different call sites in three minutes, and reporting it. + + Searches the cause chain because a failure from inside `open_client` arrives wrapped. + + Only the 2FA path recovers on its own, in :func:`_wait_then_send_a_new_code`, because that is + the one place the program knows what to retry: the code has been spent and a new one can be + requested. There is no equivalent for a refused `login` or `request_pet` - the caller has + nothing to re-do but the whole sign-in - so those say what happened and stop. + """ + seen = set() + cause: BaseException | None = error + while cause is not None and id(cause) not in seen: + if isinstance(cause, AppleServiceUnavailableError): + return cause + seen.add(id(cause)) + cause = cause.__cause__ or cause.__context__ + + return None + + +def describe_apple_declining(error: AppleServiceUnavailableError) -> str: + """What to tell somebody whose sign-in was refused by Apple rather than by their password.""" + return ( + f"Apple's sign-in service refused the request with HTTP {error.status_code}.\n\n" + "Apple declined it rather than anything being wrong with your Apple ID, your password or" + " your verification code. Nothing was changed and nothing was sent.\n\n" + "Trying again shortly is worth doing - it has cleared by itself for some people. It has" + " also been reported as lasting across many attempts, so if it keeps refusing, please" + " report it with this log rather than waiting it out. Why it persists for some accounts" + " and not others is not yet known." + ) + + def _apple_failed_after_taking_the_code(error: BaseException) -> SignInInterrupted: """ What to tell somebody whose code was accepted and whose sign-in failed anyway. @@ -584,13 +625,14 @@ def _apple_failed_after_taking_the_code(error: BaseException) -> SignInInterrupt """ return SignInInterrupted( f"Apple accepted your verification code and then failed to finish signing in: {error}\n\n" - "This is a fault on Apple's side rather than anything you did, and it clears on its own." - " Nothing was changed and nothing was sent.\n\n" + "This is a fault on Apple's side rather than anything you did. Nothing was changed and" + " nothing was sent.\n\n" f"It was given {len(SPENT_CODE_WAITS)} chances to settle - waiting" f" {' and then '.join(f'{s}s' for s in SPENT_CODE_WAITS)} - and did not.\n\n" "Leave it a few minutes and sign in again. Apple may also refuse the password once while" " it settles - that is part of the same hiccup rather than a second problem, and trying" - " once more is the answer.", + " once more is the answer. If it keeps refusing, please report it with your log: this" + " has been seen to last for some accounts, and why is not yet known.", ) diff --git a/python/exporter/wizard.py b/python/exporter/wizard.py index 7341a373..f02e1117 100644 --- a/python/exporter/wizard.py +++ b/python/exporter/wizard.py @@ -54,6 +54,7 @@ MobileMeDelegateError, TermsError, ) +from findmy.errors import AppleServiceUnavailableError from findmy.keychain.recovery import RecoveryError from exporter.icloud import Candidate, ExportSourceError @@ -395,18 +396,22 @@ def _load(self) -> None: f"{e}\n\nNothing was changed. You can try again whenever you like.", ) return - except Exception as e: # noqa: BLE001 - anything else is still the user's problem to see - logger.exception("Reading accessories failed") + except AppleServiceUnavailableError as e: + # **Before the catch-all below, which offers the issue tracker.** A 503 from Apple is + # not a bug in this program, and somebody took that offer up - issue #176, one account + # meeting the same 503 at three call sites in three minutes. This catches the failures + # raised directly by `log_in`; the ones raised deeper arrive wrapped and are handled + # in `_report_unexpected`. + logger.info("Apple declined the request: %s", e) messagebox.showerror( - "Could not read your accessories", - # The type, not just the message: "[Errno 2] No such file or directory" with - # nothing else is a real message this produced, and it says nothing about what - # was being opened or by whom. - f"{type(e).__name__}: {e}\n\n" - f"The details are in:\n{log_file()}\n\n" - f"If this looks like a bug, please report it with that file:\n{GITHUB_ISSUES_LINK}", + "Apple is not accepting sign-ins right now", + icloud.describe_apple_declining(e), ) return + except Exception as e: # noqa: BLE001 - anything else is still the user's problem to see + logger.exception("Reading accessories failed") + self._report_unexpected(e) + return self.candidates = fetched.candidates self.undecryptable = fetched.undecryptable @@ -415,6 +420,35 @@ def _load(self) -> None: self.read_button.configure(state="disabled") self._show(fetched.skipped) + def _report_unexpected(self, error: BaseException) -> None: + """ + Say what went wrong when nothing above recognised it. + + **The 503 check here is not redundant with the handler in `_load`.** One raised directly + by `log_in` is caught there by type. One raised inside `open_client` is several frames + down and arrives wrapped, so it reaches the catch-all instead - which is how issue #176's + `request_pet` failure came to offer the issue tracker. + + Its own method because `_load` was already at flake8's complexity limit. + """ + declining = icloud.apple_is_declining(error) + if declining is not None: + messagebox.showerror( + "Apple is not accepting sign-ins right now", + icloud.describe_apple_declining(declining), + ) + return + + messagebox.showerror( + "Could not read your accessories", + # The type, not just the message: "[Errno 2] No such file or directory" with + # nothing else is a real message this produced, and it says nothing about what + # was being opened or by whom. + f"{type(error).__name__}: {error}\n\n" + f"The details are in:\n{log_file()}\n\n" + f"If this looks like a bug, please report it with that file:\n{GITHUB_ISSUES_LINK}", + ) + async def _read_local(self, _asker: Asker): """The local route needs nothing from the user that macOS does not ask for itself.""" return localsource.fetch(localsource.read_key()) diff --git a/python/pyproject.toml b/python/pyproject.toml index f01ddc0f..a80d929b 100644 --- a/python/pyproject.toml +++ b/python/pyproject.toml @@ -69,7 +69,7 @@ constraint-dependencies = [ ] [tool.uv.sources] -FindMy = { git = "https://github.com/parawanderer/FindMy.py", rev = "ddc7f2342fc9f32ebe315b85c22a4554ce419f6d" } +FindMy = { git = "https://github.com/parawanderer/FindMy.py", rev = "3c2b4926252193e9cd265b39fa52252adcbaad4e" } [dependency-groups] # Only the release build installs this, with `uv sync --no-default-groups --group build`. It is diff --git a/python/test/test_apple_declining.py b/python/test/test_apple_declining.py new file mode 100644 index 00000000..0150c350 --- /dev/null +++ b/python/test/test_apple_declining.py @@ -0,0 +1,128 @@ +""" +A 503 from Apple is not a bug report. + +Issue #176. One account met the same 503 three times in three minutes, at `td_2fa_submit`, at +`request_pet` a second later, and at `login` a minute after that. Only the first had any handling, +so the other two reached the wizard's catch-all - the one that names an exception type and links +the issue tracker - and the user filed an issue, because the program asked them to. + +The 2FA path recovers on its own and that half works: the same log shows it waiting, requesting a +new code, and signing in. This is about the other two, where there is nothing to retry but the +whole sign-in. +""" + +from __future__ import annotations + +import pytest +from findmy.errors import AppleServiceUnavailableError, UnhandledProtocolError + +from exporter import icloud + + +def a_503() -> AppleServiceUnavailableError: + return AppleServiceUnavailableError(503, "The Grand Slam request") + + +class TestFindingItInWhateverWrappedIt: + def test_itisFoundWhenRaisedDirectly(self): + """What `log_in` does: the error arrives as itself.""" + error = a_503() + + assert icloud.apple_is_declining(error) is error + + def test_itisFoundThroughACauseChain(self): + """ + What `open_client` does. + + `request_pet` fails several frames down and the failure is re-raised wrapped, which is + exactly the path that reached the bug-report dialog in #176. + """ + declining = a_503() + wrapped = RuntimeError("could not open the client") + wrapped.__cause__ = declining + + assert icloud.apple_is_declining(wrapped) is declining + + def test_itisFoundThroughImplicitChaining(self): + """`raise X` inside an `except` sets __context__ rather than __cause__.""" + declining = a_503() + try: + try: + raise declining + except AppleServiceUnavailableError: + raise RuntimeError("while handling") # noqa: B904, TRY200 + except RuntimeError as raised: + assert icloud.apple_is_declining(raised) is declining + + def test_anordinaryProtocolErrorIsNotThis(self): + """ + The distinction the whole change turns on. + + `UnhandledProtocolError` still means "report this". Only the subclass means "wait". + """ + assert icloud.apple_is_declining( + UnhandledProtocolError("Error response for GSA request: 418")) is None + + def test_nothingAtAllIsNotThis(self): + assert icloud.apple_is_declining(ValueError("unrelated")) is None + + def test_acycleDoesNotHangIt(self): + """ + A self-referencing chain must terminate. + + Contrived, but this walks `__cause__` and `__context__` on exceptions this program did + not construct, and an infinite loop here would hang a wizard rather than fail it. + """ + first = RuntimeError("a") + second = RuntimeError("b") + first.__cause__ = second + second.__cause__ = first + + assert icloud.apple_is_declining(first) is None + + +class TestWhatTheUserIsTold: + @pytest.fixture + def message(self) -> str: + return icloud.describe_apple_declining(a_503()) + + def test_itcarriesTheStatus(self, message): + """503 is what makes a report answerable if this ever turns out not to be weather.""" + assert "503" in message + + def test_itsaysTheFaultIsApples(self, message): + assert "Apple declined it" in message + assert "rather than anything being wrong with your Apple ID" in message + + def test_itclearsThePasswordAndTheCode(self, message): + """Otherwise the next thing tried is a password reset, which cannot help.""" + assert "password" in message + assert "verification code" in message + + def test_itsaysNothingWasChanged(self, message): + """The first question after a failed sign-in to your own Apple account.""" + assert "nothing was changed" in message.lower() + + def test_itasksForAReportWhenItKeepsHappening(self, message): + """ + Not "wait it out", because for at least one person it never cleared. + + Five people have reported this and only @parawanderer has confirmed it resolving. + crishpeen on #168 says the opposite: every attempt, every 2FA method, device-identity.json + cleared. A message promising it passes would have them wait on something that does not. + """ + assert "report it with this log" in message + assert "not yet known" in message + + def test_itdoesNotPromiseItWillClear(self, message): + """ + The overclaim this replaced. + + The first version said it "usually clears on its own within a few minutes", which was one + confirmed recovery presented as a rule. + """ + assert "usually clears" not in message + + def test_itdoesNotAskForABugReportOutright(self, message): + """The behaviour being fixed.""" + assert "github.com" not in message.lower() diff --git a/python/test/test_sign_in_interrupted_dialog.py b/python/test/test_sign_in_interrupted_dialog.py index 3280f096..ad53d18e 100644 --- a/python/test/test_sign_in_interrupted_dialog.py +++ b/python/test/test_sign_in_interrupted_dialog.py @@ -86,16 +86,23 @@ def test_everything_else_keeps_the_heading_it_had(self, window): class TestTheBodySaysWhoseFaultItIsAndWhatToDo: - def test_it_does_not_ask_for_a_bug_report(self, window): + def test_it_does_not_present_itself_as_a_bug_in_this_program(self, window): """ - The complaint, asserted. + The complaint, asserted - and narrower than it first looked. - This is what the catch-all handler adds, and reaching it is what produced issue #168. + What produced #168 was the catch-all handler naming an exception type and linking the + issue tracker, so the failure read as a defect here. That is what must not happen. + + **Asking for a report when it keeps happening is a different thing, and is now correct.** + Five people have reported this 503 and only one has confirmed it clearing; crishpeen on + #168 says every attempt fails, across every 2FA method. A message that only ever says + "wait" would have them wait on something that does not pass. """ _title, body = load_failing_with(window, THE_REAL_ONE) - assert "report" not in body.lower() assert "github.com" not in body.lower() + assert "please report it with your log" in body.lower(), ( + "it has to leave a route open for the people it does not clear for") def test_it_says_the_fault_is_apples(self, window): _title, body = load_failing_with(window, THE_REAL_ONE) diff --git a/python/uv.lock b/python/uv.lock index 241917a1..c03d1477 100644 --- a/python/uv.lock +++ b/python/uv.lock @@ -659,7 +659,7 @@ wheels = [ [[package]] name = "findmy" version = "0.10.1" -source = { git = "https://github.com/parawanderer/FindMy.py?rev=ddc7f2342fc9f32ebe315b85c22a4554ce419f6d#ddc7f2342fc9f32ebe315b85c22a4554ce419f6d" } +source = { git = "https://github.com/parawanderer/FindMy.py?rev=3c2b4926252193e9cd265b39fa52252adcbaad4e#3c2b4926252193e9cd265b39fa52252adcbaad4e" } dependencies = [ { name = "aiohttp" }, { name = "anisette" }, @@ -1032,7 +1032,7 @@ dev = [ [package.metadata] requires-dist = [ - { name = "findmy", git = "https://github.com/parawanderer/FindMy.py?rev=ddc7f2342fc9f32ebe315b85c22a4554ce419f6d" }, + { name = "findmy", git = "https://github.com/parawanderer/FindMy.py?rev=3c2b4926252193e9cd265b39fa52252adcbaad4e" }, { name = "pycryptodome", specifier = "==3.22.0" }, { name = "pyyaml", specifier = "==6.0.2" }, { name = "pyzipper", specifier = "==0.4.0" },