diff --git a/AGENTS.md b/AGENTS.md index e81c4dce..44a57da7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -17,7 +17,7 @@ What that means in practice: does not belong. Rule 10 has the test. - **Do not carry this register outside the repository.** It is written for agents who need to be argued out of breaking something. A pull request review, an issue reply, or anything else a - person reads is a different job. See rule 16. + person reads is a different job. See rule 17. ## What this project is @@ -239,9 +239,9 @@ Four things follow: **Today that is always `LEGACY_MAC`.** `Hardware.DEFAULT` is it, a fresh install gets it, and `IPHONE` is built and deliberately not chosen — see `generate()`. So every entry this project - registers currently appears as a `MacBookPro` on macOS 13.1, and the serial `0PENTAGVIEWR` is - the only thing distinguishing it from real hardware. The exporter is separately a MacBook Pro - on macOS 13.4.1, FindMy.py's own default, with serial `0PENTAGXPORT`. + registers currently appears as a `MacBookPro` on macOS 13.1, and the serial is the only thing + distinguishing it from real hardware. The exporter is separately a MacBook Pro on macOS 13.4.1, + FindMy.py's own default. Changing which profile a fresh install gets is not a cosmetic edit: it registers a *second* device rather than renaming the first, for anybody who reinstalls. That is why `IPHONE` exists @@ -250,9 +250,24 @@ Four things follow: describe one real release ([findmy-export §2.2](./docs/findmy-export/01-authentication.md)). Claiming a Mac in one string and an iPhone in another is a contradiction Apple's own clients never produce. -- **The serial is a label, and the only field here the user actually sees.** `0PENTAGVIEWR` is - confirmed accepted and displayed. Without one, Apple omits the row entirely, leaving an entry - with nothing to tell it apart from real hardware. +- **The serial is a label, and the only field here the user actually sees.** Uppercase + alphanumeric, confirmed accepted and displayed. Without one, Apple omits the row entirely, + leaving an entry with nothing to tell it apart from real hardware. +- **It is drawn per install, and the prefix is what carries the recognition.** The app sends + `0PENTAGV` plus four characters, the exporter `0PENTAGX` plus four, from an alphabet that + leaves out the pairs a person comparing two screens would confuse. **Do not put it back to a + constant.** It was one, and that meant a single serial arriving at Apple from thousands of + installs, against thousands of different machine identities and Apple IDs, from every continent + at once — a shape no real hardware produces, and the leading suspect for the Grand Slam 503s in + [#168](https://github.com/parawanderer/OpenTagViewer/issues/168), + [#176](https://github.com/parawanderer/OpenTagViewer/issues/176) and + [#181](https://github.com/parawanderer/OpenTagViewer/issues/181), one of whom cleared their + device identity to no effect — which regenerates the ids and not the serial. +- **An install that already has one keeps it**, including the installs that predate this and have + no stored serial at all: those keep `0PENTAGVIEWR` / `0PENTAGXPORT`. Neither of those can be + drawn — both contain a letter the alphabet excludes — so a serial with an `I` or an `O` in it + is, by construction, an install from before the change. That is worth preserving when reading a + report, and `DrawingTheSerialTest` pins it. - **Sending the same value is not the same as sending the same bytes.** Two of these fields are transformed on the way out, and only by one side. FindMy.py sends `X-Apple-I-MD-LU` as `base64(uid)` and uppercases `X-Mme-Device-Id`; the Java ADI path sends what it is given. So @@ -263,8 +278,12 @@ Four things follow: before believing two paths agree. **Changing it later adds an entry rather than renaming one**, and may require signing in again, -so it is not a thing to adjust casually once shipped. Document what the app registers as, so a -user reading their device list can recognise it — see the wiki. +so it is not a thing to adjust casually once shipped — which is the whole reason the serial is +drawn once, on the first run that needs an identity, and then persisted beside the rest of it +(`LocalAnisette.KEY_SERIAL`, and `device-identity.json` for the exporter). A serial redrawn per +sign-in would add a device-list entry every time, which is a worse bug than the constant was. +Document what the app registers as, so a user reading their device list can recognise it — see +the wiki, which names the prefix rather than a full serial for this reason. ### 12. A UI change gets a test that inflates it, and one that drives it @@ -431,7 +450,34 @@ Every one of these is tested twice: `WhichFailuresNeedAFreshSignInTest` on the J decision, `EveryPathAsksForAFreshSignInTest` on a device for each caller honouring it. A shared predicate does not stop a fourth screen being written that never asks. -### 16. Do not carry this file's voice into anything a person reads +### 16. A pull request based on anything but `main` runs almost no CI + +Every workflow that matters here is gated `pull_request: branches: [ "main" ]` — +`build-debug.yml`, `static-checks.yml`, `macos-scripts-python.yml`, and with them the APK build, +the Chaquopy bridge tests, the JVM suite and the whole emulator suite. A PR opened against another +branch, to stack a change on one still in review, matches none of those filters. + +**It does not report as skipped. It reports as green**, because the one workflow with no branch +filter (`exporter-build-check.yml`) runs, passes, and is the only tick on the page. `gh pr checks` +prints a single passing line and looks exactly like a small change with a small amount of CI. + +This has already happened: a change touching twelve Java files, four Python modules and six test +classes sat on a PR based on another branch, with one green Windows-binary check and not one line +of Java compiled anywhere. + +**So check what actually ran before believing a PR is green**, and count the checks rather than +reading the colour: + +```bash +gh pr checks # one line is not a passing build, it is an empty one +gh pr view --json baseRefName +``` + +Stacking is still fine — base it on `main` anyway. The diff carries the other branch's commits +until that merges, which is cosmetic and collapses on its own; a PR whose base is not `main` buys +a tidier diff by not being tested. + +### 17. Do not carry this file's voice into anything a person reads **@parawanderer has not read this file**, nor most of `docs/`, most docstrings, or most commit messages. Agents wrote them. So an agent reading them cannot tell the maintainer's house style from diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 175b36aa..f7c484b1 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -407,7 +407,7 @@ chaquopy { // wheel for desktop platforms and a pure-Python `py3-none-any` one as well. // There is no Android wheel, so pip falls back to the pure-Python build - which // is correct but markedly slower. The messages here are small enough not to care. - install("git+https://github.com/parawanderer/FindMy.py@ddc7f2342fc9f32ebe315b85c22a4554ce419f6d") + install("git+https://github.com/parawanderer/FindMy.py@3c2b4926252193e9cd265b39fa52252adcbaad4e") install("NSKeyedUnArchiver==1.5") diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/AppleLoginFlowTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/AppleLoginFlowTest.java index 2bf086f7..f56a3231 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/AppleLoginFlowTest.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/AppleLoginFlowTest.java @@ -13,6 +13,7 @@ import static androidx.test.espresso.matcher.ViewMatchers.withText; import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; @@ -309,6 +310,38 @@ public void anunrecognisedFailureStillShowsItsDetail() { .check(matches(withText(containsString("Bad password"))))); } + /** + * Apple declining is told apart from a wrong password and from a dead network. + * + *

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..9d78a4e9 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/FetchFromICloudFlowTest.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/FetchFromICloudFlowTest.java @@ -87,6 +87,44 @@ public void forgetAnyStoredMembership() { .forget().blockingAwait(); } + /** + * A serial only the generator could have produced, stored before the screen is opened. + * + *

Not {@link AdiDeviceIdentity#LEGACY_SERIAL}, because that is what the screen + * falls back to when nothing is stored - so a test asserting the fallback would pass against + * a screen that never read this install's identity at all, which is the regression that + * matters: the app registers under one serial and the screen names another, and the row the + * user finds looks like somebody else's device. + */ + private static final String THE_STORED_SERIAL = "0PENTAGVK7QX"; + + @org.junit.Before + public void giveThisInstallASerialToShow() { + final android.content.Context context = + InstrumentationRegistry.getInstrumentation().getTargetContext(); + + context.getSharedPreferences( + dev.wander.android.opentagviewer.anisette.LocalAnisette.PREFERENCES, + android.content.Context.MODE_PRIVATE) + .edit() + .putString( + dev.wander.android.opentagviewer.anisette.LocalAnisette.KEY_SERIAL, + THE_STORED_SERIAL) + .commit(); + } + + /** The identity is shared with the rest of the suite; one left behind is adopted as real. */ + @After + public void takeTheSerialBackOut() { + InstrumentationRegistry.getInstrumentation().getTargetContext() + .getSharedPreferences( + dev.wander.android.opentagviewer.anisette.LocalAnisette.PREFERENCES, + android.content.Context.MODE_PRIVATE) + .edit() + .remove(dev.wander.android.opentagviewer.anisette.LocalAnisette.KEY_SERIAL) + .commit(); + } + private void open(final FakeICloudService fake) { this.icloud = fake; AppDependencies.replaceICloud(() -> fake); @@ -209,13 +247,15 @@ public void theresultsSayWhatIsNowOnTheAppleAccount() { .check(matches(withText(containsString(hardware.marketingName())))); onView(withId(R.id.icloud_registered_device_model)) .check(matches(withText(containsString(hardware.osVersion())))); + // The serial this install stored, not a constant: it is drawn per install now, so a + // screen showing a literal would send the user looking for a row that is not theirs. onView(withId(R.id.icloud_registered_device_serial)) - .check(matches(withText(containsString(AdiDeviceIdentity.APP_SERIAL)))); + .check(matches(withText(containsString(THE_STORED_SERIAL)))); TestPace.afterAStep(); onView(withId(R.id.icloud_registered_body)).perform(scrollTo()); onView(withId(R.id.icloud_registered_body)) - .check(matches(withText(containsString(AdiDeviceIdentity.APP_SERIAL)))); + .check(matches(withText(containsString(THE_STORED_SERIAL)))); // The serial reaches the sentence as well as the tile. The resource holds a ^1 slot, and // a template that lost it expands to a sentence about "the serial" that never says which. @@ -353,6 +393,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/anisette/FakeAnisetteSource.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/anisette/FakeAnisetteSource.java index cb28d555..3b4680e7 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/anisette/FakeAnisetteSource.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/anisette/FakeAnisetteSource.java @@ -131,6 +131,20 @@ public String deviceIdsJson() { return "{\"uid\":\"" + UID + "\",\"devid\":\"" + DEVID + "\"}"; } + /** + * A drawn serial, not {@link AdiDeviceIdentity#LEGACY_SERIAL}. + * + *

Deliberately a value only the generator could produce, so a test asserting that this + * reached Apple cannot pass against code that fell back to the constant - which is the + * regression that matters here, and the one a fake returning the old literal would hide. + */ + public static final String SERIAL = "0PENTAGVK7QX"; + + @Override + public String serial() { + return SERIAL; + } + @Override public String describe() { return this.ready ? "fake, ready" : "fake, unavailable: " + this.unavailableReason; diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/anisette/LocalAnisetteIdentityTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/anisette/LocalAnisetteIdentityTest.java index dea98a17..edee27b0 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/anisette/LocalAnisetteIdentityTest.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/anisette/LocalAnisetteIdentityTest.java @@ -5,6 +5,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import android.content.Context; @@ -280,4 +281,85 @@ public void ahalfWrittenIdentityIsTreatedAsAbsent() { assertNotEquals(OLD_DEVICE_ID, this.preferences.getString(LocalAnisette.KEY_DEVICE_ID, null)); } + + /** + * An install from before serials were drawn keeps the one it has been presenting. + * + *

Same shape as the hardware profile, and the same reason: three keys and no serial can + * only be an install that has been telling Apple {@code 0PENTAGVIEWR} for its whole life. + * Drawing it a new one now registers a second device beside the row the user already + * recognises, and may cost them a sign-in - to change a label. + */ + @Test + public void anIdentityWrittenBeforeSerialsWereDrawnStillPresentsTheOldOne() { + writeTheOldShape(); + + assertEquals(AdiDeviceIdentity.LEGACY_SERIAL, subject().serial()); + } + + /** And it is not back-filled, so it stays distinguishable from a serial that was drawn. */ + @Test + public void thelegacySerialIsNotWrittenBackOverTheOldShape() { + writeTheOldShape(); + + subject().serial(); + + assertNull("writing it back would make an old install look like one that drew it", + this.preferences.getString(LocalAnisette.KEY_SERIAL, null)); + } + + /** + * A fresh install draws one, and keeps it. + * + *

The keeping is the part that matters. A serial redrawn per sign-in would add a + * device-list entry every time, which is the failure the constant did not have and this + * change could easily introduce. + */ + @Test + public void afreshInstallDrawsASerialAndThenKeepsIt() { + final String drawn = subject().serial(); + + assertTrue("a drawn serial has to be recognisable as this app", + drawn.startsWith(AdiDeviceIdentity.SERIAL_PREFIX)); + assertNotEquals("a fresh install is not an install from before this change", + AdiDeviceIdentity.LEGACY_SERIAL, drawn); + assertEquals("it was stored, so a restart presents the same device", + drawn, this.preferences.getString(LocalAnisette.KEY_SERIAL, null)); + assertEquals("asked twice, answered twice the same", drawn, subject().serial()); + } + + /** A stored serial is presented unchanged, whatever it is. */ + @Test + public void astoredSerialIsWhatGoesToApple() { + writeTheOldShape(); + assertTrue(this.preferences.edit() + .putString(LocalAnisette.KEY_SERIAL, "0PENTAGVK7QX").commit()); + + assertEquals("0PENTAGVK7QX", subject().serial()); + } + + /** + * The screen asks read-only, and a screen must not be what decides an install's identity. + * + *

Same contract as {@link LocalAnisette#profileToShow}: opening a page on a device that + * has never signed in would otherwise mint and store an identity, fixing what this install + * is by having looked at it. + */ + @Test + public void showingTheSerialDoesNotMintOne() { + assertEquals(AdiDeviceIdentity.LEGACY_SERIAL, LocalAnisette.serialToShow(this.context)); + + assertNull("reading it for a label must not write one", + this.preferences.getString(LocalAnisette.KEY_SERIAL, null)); + assertNull(this.preferences.getString(LocalAnisette.KEY_DEVICE_ID, null)); + } + + /** And once there is one, that is what the screen shows. */ + @Test + public void showingTheSerialShowsTheOneThisInstallWillSend() { + final String drawn = subject().serial(); + + assertEquals("the screen named a serial Apple never saw for this user", + drawn, LocalAnisette.serialToShow(this.context)); + } } 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/IdentityBridgeTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/python/IdentityBridgeTest.java index bbe35abf..204b9bca 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/python/IdentityBridgeTest.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/python/IdentityBridgeTest.java @@ -63,11 +63,12 @@ private static PyObject profileFor(Hardware hardware) { } /** - * The serial Java shows the user is the serial Python sends Apple. + * The serial Java stores is the serial Python sends Apple. * - *

Python owns {@code APP_SERIAL}; {@code AdiDeviceIdentity.APP_SERIAL} is a copy, so that - * the screen naming the device-list entry does not have to start CPython to draw a label. Two - * copies of one value is what rule 11 is about, and this is the pin that stops them drifting. + *

It used to be a constant on each side, pinned equal here. It is drawn per install now, + * so there is nothing to pin - and that is the stronger position: Python asks across the + * bridge, so there is only one value and it cannot drift. What is left to check is that the + * asking works, which is what this does. * *

The failure it prevents is quiet and nasty: the app registers under one serial and the * screen tells the user to look for another, so the row they find looks like somebody else's @@ -75,9 +76,26 @@ private static PyObject profileFor(Hardware hardware) { */ @Test public void theserialOnScreenIsTheSerialOnTheWire() { - assertEquals("Java shows a serial Python never sends", - identity().get("APP_SERIAL").toString(), - AdiDeviceIdentity.APP_SERIAL); + assertEquals("Python did not ask Java which serial this install has", + FakeAnisetteSource.SERIAL, + identity().callAttr("appSerial", FakeAnisetteSource.ready()).toString()); + } + + /** + * And it is a drawn serial, not the one every install used to share. + * + *

Worth its own assertion because the fallback is the old constant: code that never + * reached the bridge at all would return a perfectly plausible serial, and the test above + * would be the only thing to notice. + */ + @Test + public void thefallbackIsNotMistakenForAnAnswer() { + assertEquals("with nothing to ask, Python must still name this app", + AdiDeviceIdentity.LEGACY_SERIAL, + identity().callAttr("appSerial", (Object) null).toString()); + + assertNotEquals("the fake must not answer the fallback, or nothing here proves anything", + AdiDeviceIdentity.LEGACY_SERIAL, FakeAnisetteSource.SERIAL); } /** @@ -142,7 +160,7 @@ public void anewSignInIsGivenBothHalves() { "identityForNewSession", FakeAnisetteSource.ready().claiming(Hardware.IPHONE)); - assertEquals("0PENTAGVIEWR", kwargs.callAttr("get", "serial").toString()); + assertEquals(FakeAnisetteSource.SERIAL, kwargs.callAttr("get", "serial").toString()); assertEquals("iPhone15,2", kwargs.callAttr("get", "identity").get("model").toString()); } @@ -180,8 +198,8 @@ public void alegacyInstallSigningInAgainStillClaimsTheMac() { assertEquals("MacBookPro13,2", kwargs.callAttr("get", "identity").get("model").toString()); - assertEquals("the serial is a label, and a new sign-in is a new entry either way", - "0PENTAGVIEWR", kwargs.callAttr("get", "serial").toString()); + assertEquals("the machine is what ADI pinned; the serial is stored separately", + FakeAnisetteSource.SERIAL, kwargs.callAttr("get", "serial").toString()); } /** @@ -255,7 +273,8 @@ public void withNoBridgeThereIsNoMachineToClaim() { final PyObject kwargs = identity().callAttr("identityForNewSession", (Object) null); - assertEquals("0PENTAGVIEWR", kwargs.callAttr("get", "serial").toString()); + assertEquals(AdiDeviceIdentity.LEGACY_SERIAL, + kwargs.callAttr("get", "serial").toString()); assertFalse("with no machine to claim, none should be asserted", kwargs.callAttr("__contains__", "identity").toBoolean()); } diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/python/PythonPackagingTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/python/PythonPackagingTest.java index b87e1d12..fbd6b38b 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/python/PythonPackagingTest.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/python/PythonPackagingTest.java @@ -11,6 +11,8 @@ import com.chaquo.python.Python; import com.chaquo.python.android.AndroidPlatform; +import dev.wander.android.opentagviewer.anisette.AdiDeviceIdentity; + import androidx.test.ext.junit.runners.AndroidJUnit4; import org.junit.BeforeClass; @@ -224,13 +226,22 @@ public void theexporterKeepsItsOwnIdentityByDefault() { *

The serial is asserted here as well as in the Python tests because this is where it is * real. Rule 11: a phone presenting {@code 0PENTAGXPORT} would share a device-list entry * with the desktop exporter, and removing either would break the other. + * + *

It is built from the session rather than read off the module, because the serial is + * drawn per install - a constant here would be right for a fresh install and wrong for every + * other one, in the field CloudKit writes into the escrow record. */ @Test public void theappsIcloudBridgeIsPackagedAndKnowsWhoItIs() { final PyObject bridge = Python.getInstance().getModule("icloud_bridge"); assertNotNull("icloud_bridge must be importable in the APK", bridge); - assertEquals("0PENTAGVIEWR", bridge.get("APP_IDENTITY").get("serial").toString()); + + final PyObject identity = bridge.callAttr("appIdentity", new Object[]{null}); + + assertEquals("with no session to read, it still names this app", + AdiDeviceIdentity.LEGACY_SERIAL, identity.get("serial").toString()); + assertEquals("OpenTagViewer", identity.get("device_name").toString()); } /** 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/androidTest/java/dev/wander/android/opentagviewer/python/icloud/TheWholeICloudFlowAcrossTheBridgeTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/python/icloud/TheWholeICloudFlowAcrossTheBridgeTest.java index 05803c1d..92cd4b59 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/python/icloud/TheWholeICloudFlowAcrossTheBridgeTest.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/python/icloud/TheWholeICloudFlowAcrossTheBridgeTest.java @@ -21,6 +21,7 @@ import java.util.List; +import dev.wander.android.opentagviewer.anisette.AdiDeviceIdentity; import dev.wander.android.opentagviewer.python.PythonAppleAccount; /** @@ -168,6 +169,10 @@ public void unlockingAndJoiningProduceAmembership() { *

Rule 11: the serial is what distinguishes peers in the trust circle, and the only field * of this the user actually sees - in a list next to a Remove from Account button. A * path that composed its own would register a second device. + * + *

The session's serial, not a constant. Serials are drawn per install, so the value + * asserted here is one only the generator could produce - code that ignored the session and + * fell back to {@link AdiDeviceIdentity#LEGACY_SERIAL} would fail rather than look right. */ @Test public void thejoinCarriesTheAppsOwnSerial() { @@ -175,8 +180,8 @@ public void thejoinCarriesTheAppsOwnSerial() { this.service.unlock(A_SERIAL, THE_RIGHT_PASSCODE).blockingAwait(); this.service.join("an-escrow-passcode").blockingFirst(); - assertEquals("the peer was registered under something other than the app's serial", - "0PENTAGVIEWR", this.reached("joinedSerial")); + assertEquals("the peer was registered under something other than this session's serial", + "0PENTAGVK7QX", this.reached("joinedSerial")); } /** diff --git a/app/src/debug/python/icloud_test_double.py b/app/src/debug/python/icloud_test_double.py index b53c8169..400e76ce 100644 --- a/app/src/debug/python/icloud_test_double.py +++ b/app/src/debug/python/icloud_test_double.py @@ -229,6 +229,10 @@ def uninstall() -> None: theClient = None +SESSION_SERIAL = "0PENTAGVK7QX" +"""The serial the fake session presents. Drawable, and deliberately not `LEGACY_SERIAL`.""" + + def anAccount() -> Any: """ An account object shaped like the one the app signs in with. @@ -236,10 +240,14 @@ def anAccount() -> Any: ``openSession`` guards on the two private attributes FindMy.py's account carries, and a join reads the identity and serial off the async half - rule 11's single source of truth, which is why they are here rather than invented further down. + + The serial is a **drawn** one rather than the value every install used to share, so a test + asserting it reached the join cannot pass against code that never read the session at all: + that path falls back to `identity.LEGACY_SERIAL`, and the two must not be the same string. """ return SimpleNamespace( _asyncacc=SimpleNamespace( - serial="0PENTAGVIEWR", + serial=SESSION_SERIAL, identity=SimpleNamespace( model="iPhone17,1", os_name="iPhone OS", os_version="18.1", os_build="22B83", cfnetwork="1568.100.1", darwin="24.1.0")), 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..df49c8b7 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/FetchFromICloudActivity.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/FetchFromICloudActivity.java @@ -571,7 +571,7 @@ private void showWhatWeRegisteredAs() { ? R.drawable.smartphone_24px : R.drawable.laptop_24px); // The same chip in the prose. It is the value the sentence is about, and it reads as a - // typo in body text - "0PENTAGVIEWR" has a zero for an O and no vowel in VIEWR. + // typo in body text - it has a zero for an O and no vowel after "0PENTAGV". // Read from the resource rather than from the view: the view may already hold an expanded // copy from a previous visit to this step, and expanding twice leaves the ^1 gone and the // chip applied to nothing. @@ -586,10 +586,18 @@ private void showWhatWeRegisteredAs() { Direction.FORWARD); } - /** The serial, as a chip, ready to drop into a sentence or a label. */ + /** + * The serial, as a chip, ready to drop into a sentence or a label. + * + *

This install's, not a constant. Serials are drawn per install, so a literal here + * would name a serial Apple never saw for this user - who would then go looking for it in + * their device list, not find it, and conclude the row in front of them is somebody else's. + * Read-only: see {@link LocalAnisette#serialToShow}, and note that this screen is only + * reached once a sign-in has registered a device, so one has been drawn by now. + */ private CharSequence serialAsCode() { - return CodeChipSpan.applyTo( - AdiDeviceIdentity.APP_SERIAL, AdiDeviceIdentity.APP_SERIAL, this.codeChip()); + final String serial = LocalAnisette.serialToShow(this); + return CodeChipSpan.applyTo(serial, serial, this.codeChip()); } private CodeChipSpan codeChip() { @@ -698,6 +706,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/anisette/AdiDeviceIdentity.java b/app/src/main/java/dev/wander/android/opentagviewer/anisette/AdiDeviceIdentity.java index 26c33b02..2c06350b 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/anisette/AdiDeviceIdentity.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/anisette/AdiDeviceIdentity.java @@ -34,18 +34,23 @@ public final class AdiDeviceIdentity { private final String uniqueDeviceIdentifier; private final String adiIdentifier; private final String localUserUuid; + private final String serial; private final Hardware hardware; /** + * @param serial what Apple prints against this install's device-list entry. An install + * that already existed before serials were drawn passes + * {@link #LEGACY_SERIAL}, and must keep doing so. * @param hardware which machine this install claims to be. An install that already existed * before profiles were introduced passes {@link Hardware#LEGACY_MAC}, and * must keep doing so - see the enum. */ public AdiDeviceIdentity(String uniqueDeviceIdentifier, String adiIdentifier, - String localUserUuid, Hardware hardware) { + String localUserUuid, String serial, Hardware hardware) { this.uniqueDeviceIdentifier = uniqueDeviceIdentifier; this.adiIdentifier = adiIdentifier; this.localUserUuid = localUserUuid; + this.serial = serial; this.hardware = hardware; } @@ -68,18 +73,51 @@ public AdiDeviceIdentity(String uniqueDeviceIdentifier, String adiIdentifier, * themselves to Apple as the same machine. */ /** - * The serial Apple prints against this app's entry in the user's device list. + * The eight characters every serial this app presents begins with. * - *

Python owns it - {@code identity.APP_SERIAL} is what actually goes to Apple, and - * this is a copy for the screen that shows the user what to look for. A copy at all only - * because a UI thread should not have to start CPython to render a label. + *

Recognisability lives in the prefix rather than in the whole string. An entry reading + * {@code 0PENTAGV} followed by anything is identifiably this app, which is what stops + * somebody pressing Remove from Account on it - and it sorts next to the desktop + * exporter's {@code 0PENTAGX...}, so the two read as one project. + */ + public static final String SERIAL_PREFIX = "0PENTAGV"; + + /** + * What the four characters after the prefix are drawn from. * - *

Two copies of one value is exactly what rule 11 warns about, so they are pinned - * together: {@code IdentityBridgeTest} fails if they ever differ. A screen naming a serial - * Apple never saw is worse than naming none, because the user would go looking for it, - * not find it, and conclude the row in front of them belongs to somebody else. + *

Uppercase alphanumeric, which is the shape Apple accepts, with the pairs a person + * comparing a serial on this screen against a row in their Apple device list is most likely + * to confuse left out - no {@code O} against {@code 0}, no {@code I} or {@code 1}, no + * {@code S} against {@code 5}, no {@code B} against {@code 8}, no {@code Z} against + * {@code 2}. Nobody types this; they only ever compare it, and that is the whole job it has. + * + *

The same alphabet as {@code python/exporter/identity.py}, deliberately. */ - public static final String APP_SERIAL = "0PENTAGVIEWR"; + public static final String SERIAL_ALPHABET = "ACDEFGHJKLMNPQRTUVWXY34679"; + + /** + * The serial every install presented before this was drawn per install. + * + *

Still presented, by every install that already has an identity. Changing the + * serial on an install that works costs a second device-list entry and may cost a sign-in, + * for no benefit to somebody who is not affected - see the warning on {@link #serial()}. + * + *

It shares the prefix with a drawn serial but is not one and cannot be: {@code IEWR} + * contains an {@code I}, which {@link #SERIAL_ALPHABET} leaves out. So a serial with an + * {@code I} in it is, by construction, an install from before this change. + * + *

Why this stopped being the only one. It was a constant, so every install of this + * app anywhere presented Apple the same serial while presenting a different machine + * identity: one serial against thousands of device ids and thousands of Apple IDs, from every + * continent, at once. Real hardware does not look like that. The 503s from Grand Slam that + * some accounts never recover from - issues #168, #176 and #181 - are consistent with that + * fingerprint being refused, and one reporter cleared their device identity to no effect, + * which is what would happen if the serial were the part being matched on. + * + *

That is a hypothesis and is written down as one. It has not been confirmed against + * Apple, and the cheap way to confirm it is exactly this change. + */ + public static final String LEGACY_SERIAL = "0PENTAGVIEWR"; public static AdiDeviceIdentity generate() { final SecureRandom random = new SecureRandom(); @@ -89,9 +127,30 @@ public static AdiDeviceIdentity generate() { UUID.randomUUID().toString().toUpperCase(Locale.ROOT), hex(random, 8).toLowerCase(Locale.ROOT), hardware.newLocalUserId(random), + generateSerial(random), hardware); } + /** + * A serial for an install that does not have one yet. + * + *

Twelve characters, of which the last four vary - about 450,000 of them, which is not a + * large space and does not need to be. The point is that two installs are unlikely to share + * one, not that a serial is unguessable; there is nothing here to guess. + * + *

{@link SecureRandom}, and it matters which one. A seedable generator would hand + * every fresh install the same serial and reproduce exactly the fingerprint this change + * exists to break up. {@code SecureRandom}'s no-argument constructor is seeded by the + * platform and cannot be pinned from here; nothing in this app calls {@code setSeed}. + */ + static String generateSerial(SecureRandom random) { + final StringBuilder tail = new StringBuilder(4); + for (int i = 0; i < 4; i++) { + tail.append(SERIAL_ALPHABET.charAt(random.nextInt(SERIAL_ALPHABET.length()))); + } + return SERIAL_PREFIX + tail; + } + private static String hex(SecureRandom random, int bytes) { final byte[] buffer = new byte[bytes]; random.nextBytes(buffer); @@ -172,7 +231,7 @@ public String localUserHeader(String localUserUuid) { * eligible as a second factor: that is decided by the push token, which FindMy.py has no * parameter for and never sends, and an iPhone-shaped entry still reports "This device * cannot be used to receive Apple Account verification codes". And it arrives together - * with the serial {@code 0PENTAGVIEWR}, which is what keeps it recognisable - an iPhone + * with a {@code 0PENTAGV} serial, which is what keeps it recognisable - an iPhone * claim on its own, unnamed and unserialled, would be worse than the Mac it replaces. */ IPHONE( @@ -293,7 +352,7 @@ public String marketingName() { * *

So the name cannot be what a user matches on, and the serial has to be. Every * install of this app produces a row with this same title and model, which is why - * {@code 0PENTAGVIEWR} carries the whole weight of telling it apart - rule 11. + * the serial carries the whole weight of telling it apart - rule 11. */ public String deviceListName() { return this.model.replaceAll("\\d+,\\d+$", ""); @@ -373,4 +432,21 @@ public String adiIdentifier() { public String localUserUuid() { return this.localUserUuid; } + + /** + * The serial Apple prints against this app's entry in the user's device list. + * + *

The only field here the user actually sees, and the only thing telling their + * entry apart from anyone else's: every install of this app produces a row with the same + * title and the same model, so the serial carries the whole weight of recognising it + * (rule 11). Python reads this across the bridge at sign-in rather than holding a copy, and + * the screen that tells the user what to look for reads the same stored value. + * + *

Changing it adds an entry rather than renaming one, and may require signing in + * again - Apple binds a session to the identity that established it. So it is drawn once, + * on the first run that needs an identity, and then kept for the life of the install. + */ + public String serial() { + return this.serial; + } } diff --git a/app/src/main/java/dev/wander/android/opentagviewer/anisette/AnisetteSource.java b/app/src/main/java/dev/wander/android/opentagviewer/anisette/AnisetteSource.java index 6a58a83a..ba5bf60e 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/anisette/AnisetteSource.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/anisette/AnisetteSource.java @@ -79,6 +79,21 @@ public interface AnisetteSource { */ String deviceIdsJson(); + /** + * The serial this install presents to Apple, in {@code X-Apple-I-SRL-NO}. + * + *

Drawn once per install and then kept, because Apple binds a session to the identity + * that established it and the user has a row in their device list with this printed on it. + * See {@link AdiDeviceIdentity#serial()}. + * + *

Python asks rather than holding a copy. It used to be a constant on both sides, + * pinned equal by a test; a per-install value cannot be, and a copy of it would be the + * second source of truth rule 11 is about - here in the one field the user actually reads. + * + *

Answerable without ADI, for the same reason as {@link #hardwareProfileJson}. + */ + String serial(); + /** Short human-readable state, for logs and diagnostics. */ String describe(); } diff --git a/app/src/main/java/dev/wander/android/opentagviewer/anisette/LocalAnisette.java b/app/src/main/java/dev/wander/android/opentagviewer/anisette/LocalAnisette.java index 8fe50492..c3cdf213 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/anisette/LocalAnisette.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/anisette/LocalAnisette.java @@ -60,6 +60,21 @@ public final class LocalAnisette implements AnisetteSource { */ public static final String KEY_HARDWARE = "hardwareProfile"; + /** + * The serial Apple prints against this install's device-list entry. + * + *

Added last, and its absence beside the others is meaningful in the same way: it + * marks an install from before serials were drawn per install, which can only have been + * presenting {@link AdiDeviceIdentity#LEGACY_SERIAL}. That install keeps it - a new serial + * on a working install is a second device-list entry and possibly a sign-in, in exchange for + * nothing the user asked for. + * + *

Not back-filled for those installs on read, deliberately. Writing the legacy value here + * would make an old install indistinguishable from one that drew that value, and the whole + * point of the alphabet excluding {@code I} is that it cannot have drawn it. + */ + public static final String KEY_SERIAL = "serial"; + /** Apple's, in dependency order. CoreFoundation and mediaplatform are our stubs. */ private static final List FROM_APPLE = Arrays.asList( "libc++_shared.so", "libstoreservicescore.so"); @@ -283,7 +298,11 @@ private AdiDeviceIdentity loadOrCreateIdentity() { hardwareFrom(preferences.getString(KEY_HARDWARE, null), AdiDeviceIdentity.Hardware.LEGACY_MAC); - return new AdiDeviceIdentity(deviceId, adiId, localUser, hardware); + // Same argument for the serial, for the same reason: an install with an identity + // and no stored serial has been telling Apple it is 0PENTAGVIEWR, and drawing it a + // new one now would register a second device beside the row it already has. + return new AdiDeviceIdentity(deviceId, adiId, localUser, + serialFrom(preferences), hardware); } final AdiDeviceIdentity fresh = AdiDeviceIdentity.generate(); @@ -292,6 +311,7 @@ private AdiDeviceIdentity loadOrCreateIdentity() { .putString(KEY_ADI_ID, fresh.adiIdentifier()) .putString(KEY_LOCAL_USER, fresh.localUserUuid()) .putString(KEY_HARDWARE, fresh.hardware().name()) + .putString(KEY_SERIAL, fresh.serial()) .apply(); Log.i(TAG, "generated a new device identity as " + fresh.hardware() @@ -319,6 +339,29 @@ public static AdiDeviceIdentity.Hardware profileToShow(final Context context) { AdiDeviceIdentity.Hardware.DEFAULT); } + /** + * The serial this install presents, for showing the user - and nothing else. + * + *

Read-only, like {@link #profileToShow}, and for the same reason: a screen must + * not be the thing that decides what this install's identity is. + * + *

Only ask this where an identity already exists. The screen it is for is the one + * shown after a sign-in registered a device, so by then one has been drawn and stored. An + * install with nothing stored gets {@link AdiDeviceIdentity#LEGACY_SERIAL}, which is right + * for every install that has an identity without a serial and wrong for one that has no + * identity at all - but the alternative is minting one to render a label, which fixes the + * install's identity by having looked at a page. + */ + public static String serialToShow(final Context context) { + return serialFrom(context.getSharedPreferences(PREFERENCES, Context.MODE_PRIVATE)); + } + + /** The stored serial, or the value an install from before this change has been sending. */ + private static String serialFrom(final SharedPreferences preferences) { + final String stored = preferences.getString(KEY_SERIAL, null); + return stored != null && !stored.isEmpty() ? stored : AdiDeviceIdentity.LEGACY_SERIAL; + } + /** * The stored profile, or {@code fallback} when there is nothing usable stored. * @@ -366,6 +409,18 @@ public synchronized String hardwareProfileJson() { * twice - which is a value Apple has never seen, from a client claiming to be the same * installation. See {@code AdiDeviceIdentity.Hardware#localUserHeader}. */ + /** + * {@inheritDoc} + * + *

Answerable without ADI, for the same reason as {@link #hardwareProfileJson} - and it + * has to be, because a sign-in that fell back to a remote server must present the same + * serial as one that did not. The serial is not a property of how Anisette was obtained. + */ + @Override + public synchronized String serial() { + return currentIdentity().serial(); + } + @Override public synchronized String deviceIdsJson() { final AdiDeviceIdentity current = currentIdentity(); 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/ui/CodeChipSpan.java b/app/src/main/java/dev/wander/android/opentagviewer/ui/CodeChipSpan.java index 5ec28062..3c41d276 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/ui/CodeChipSpan.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/ui/CodeChipSpan.java @@ -14,7 +14,7 @@ * A value rendered as inline code - monospace, on a tinted rounded chip. * *

For the one string in this app that people compare character by character. The serial - * {@code 0PENTAGVIEWR} is the only field distinguishing this app's entry in an Apple device list + * The serial is the only field distinguishing this app's entry in an Apple device list * from real hardware, and it is deliberately near-miss shaped: a zero where an O belongs, and no * vowel in VIEWR. In body text it reads as a typo. Set as code it reads as a value to be matched, * which is what somebody is about to do with it. 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..5f39d6e2 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,21 +125,41 @@ 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.""" -APP_IDENTITY = icloud.ClientIdentity( - serial=app_identity.APP_SERIAL, - device_name=app_identity.APP_CLOUDKIT_DEVICE_NAME, -) -""" -Who this app says it is to CloudKit. +def appIdentity(asyncAccount: Any) -> Any: + """ + Who this app says it is to CloudKit, **for this session**. -Rule 11: the same identity every path already sends. Defaulting this would present Apple with -`0PENTAGXPORT` - the *desktop exporter* - from a phone, and the two would share one device-list -entry that neither could be removed from safely. -""" + Rule 11: the same identity every path already sends. Defaulting it would present Apple with + `0PENTAGXPORT` - the *desktop exporter* - from a phone, and the two would share one + device-list entry that neither could be removed from safely. + + **Per session rather than per process**, because the serial is per install and a restored + session keeps whatever it was established with. A constant here would be right for a fresh + install and wrong for every other one, in the field CloudKit writes into the escrow record + and the recovery picker matches on. + """ + return icloud.ClientIdentity( + serial=app_identity.serialOfSession(asyncAccount), + device_name=app_identity.APP_CLOUDKIT_DEVICE_NAME, + ) def _toUnixEpochMs(when: Any) -> int | None: @@ -176,6 +196,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. @@ -279,6 +304,9 @@ def __init__(self, account: Any, asyncAccount: Any, loop: Any) -> None: self._account = account self._async = asyncAccount self._loop = loop + # Resolved once, here, so `open` and `recoveryOptions` cannot disagree about who this + # is - which would write a record under one serial and then fail to recognise it. + self._identity = appIdentity(asyncAccount) self._client: Any = None self._records: list[Any] = [] # The peer an unlock recovered, kept because it is what sponsors a join. @@ -296,7 +324,7 @@ def open(self) -> str: try: client = self._loop.run_until_complete( - icloud.open_client(self._async, APP_IDENTITY)) + icloud.open_client(self._async, self._identity)) self._loop.run_until_complete(client.__aenter__()) self._client = client @@ -330,21 +358,21 @@ def recoveryOptions(self) -> str: # **This app's own escrow record is dropped, not shown.** # # Joining the trust circle registers this app as a device, so the account then holds a - # record for `0PENTAGVIEWR` alongside the user's real hardware - and the picker offered + # record for this app alongside the user's real hardware - and the picker offered # it, asking for "the screen-lock passcode of one of your Apple devices" for a device # that has no screen and no lock. There is no answer to that question: the escrow # passcode was generated, never shown, and is not the user's to know. # - # Matched on the serial from `APP_IDENTITY`, which is the one place this app's identity - # is written (rule 11) - not on the name, which Apple does not carry for this entry, nor - # on a literal, which would be a second copy of the identity to keep in step. + # Matched on the serial this session presents, which is the one place this app's + # identity is written (rule 11) - not on the name, which Apple does not carry for this + # entry, nor on a literal, which would be a second copy of the identity to keep in step. # # Filtered here rather than in the screen so the count below is honest: an account whose # only recoverable record is this app's has nothing the user can recover from, and # should be told so rather than shown one unusable tile. self._records = [ record for record in options.recoverable - if record.serial != APP_IDENTITY.serial + if record.serial != self._identity.serial ] if not self._records: diff --git a/app/src/main/python/identity.py b/app/src/main/python/identity.py index d04cf0a3..c73763ef 100644 --- a/app/src/main/python/identity.py +++ b/app/src/main/python/identity.py @@ -39,16 +39,28 @@ first turns that into a visible fallback instead of an invisible hybrid. """ -APP_SERIAL = "0PENTAGVIEWR" +LEGACY_SERIAL = "0PENTAGVIEWR" """ -The serial a new session presents, in `X-Apple-I-SRL-NO`. +The serial every install presented before this was drawn per install. Twelve uppercase alphanumeric characters, which is the shape Apple accepts, and deliberately implausible as real hardware so nothing mistakes it for a Mac. It shares its prefix with the exporter's `0PENTAGXPORT` so a user seeing both recognises them as the same project. -Without this a session presents FindMy.py's default, `0FINDMYPY001` - which names the library -rather than the program, in the one place the user ever looks. +Without a serial at all a session presents FindMy.py's default, `0FINDMYPY001` - which names the +library rather than the program, in the one place the user ever looks. So this is the fallback +when Java cannot be asked, not an absence of one. + +**Java owns the live value now, and this is not a copy of it.** It was a constant on both sides, +pinned equal by `IdentityBridgeTest`; a serial drawn per install cannot be pinned that way, and +a second copy of it is exactly what rule 11 is about. `appSerial` asks. See +`AdiDeviceIdentity.LEGACY_SERIAL` for why a constant was wrong: one serial against thousands of +machine identities and Apple IDs is not a shape real hardware produces, and it is the leading +suspect for the 503s in issues #168, #176 and #181. + +**An install that has one keeps it**, which is why this value still goes out at all: an install +from before the change has been presenting it, and drawing it a new serial now would register a +second device in that user's account. """ APP_CLOUDKIT_DEVICE_NAME = "OpenTagViewer" @@ -61,7 +73,7 @@ client that set nothing here would be named by the library instead. That is the same second-identity problem as the serial, one layer down. -Distinct from the exporter's `OpenTagViewer Exporter` for the same reason `APP_SERIAL` is +Distinct from the exporter's `OpenTagViewer Exporter` for the same reason this app's serial is distinct from `0PENTAGXPORT`: two programs, two devices, deliberately. """ @@ -73,7 +85,7 @@ # so for the installed base it does not work, and getting it to work means those users signing # in again. Not worth a re-login for a label. # -# The serial carries the recognisability on its own: an entry reading `0PENTAGVIEWR` is +# The serial carries the recognisability on its own: an entry reading `0PENTAGV...` is # identifiable as software the user installed, which is the thing that stops them removing it. # The row is still titled after the claimed model, and that is accepted. @@ -122,6 +134,35 @@ def hardwareProfile(localAnisette: Any) -> DeviceIdentity | None: return DeviceIdentity.from_json(payload) +def appSerial(localAnisette: Any) -> str: + """ + The serial this install presents, **read from Java rather than decided here**. + + It is drawn once, on the first run that needs an identity, and stored in the same + SharedPreferences as the rest of the device identity - so an install that already had one + keeps it, and an install from before serials were drawn keeps `LEGACY_SERIAL`. Java is the + only side that can answer that, which is why this asks rather than holding a value. + + Falls back to `LEGACY_SERIAL` when there is nothing to ask or the answer is unusable. That + is the value the install most likely already has, and it is in any case better than letting + FindMy.py name itself `0FINDMYPY001` in the one field the user reads. + """ + if localAnisette is None: + return LEGACY_SERIAL + + try: + serial = str(localAnisette.serial()) + except Exception: + print(f"Could not read this installation's serial from Java: {traceback.format_exc()}") + return LEGACY_SERIAL + + if not serial: + print("Java gave an empty serial - presenting the one every older install presents.") + return LEGACY_SERIAL + + return serial + + def identityForNewSession(localAnisette: Any) -> dict: """ The identity a **new** sign-in presents: this app's serial, and Java's machine. @@ -129,15 +170,16 @@ def identityForNewSession(localAnisette: Any) -> dict: Only for a new sign-in. Everything restored from a stored account keeps whatever it was established with - see `identityForRestore`, and the warning at the top of this module. - The two halves are not the same kind of thing, which is why only one of them is asked for. - The serial is a label, chosen by this app and free to be the same on every install. The - machine is not a choice at all: it has to match what this particular install already told - Apple during ADI provisioning, so it is read rather than decided. + Both halves are read from Java, and for the same reason: both are per install. The machine + has to match what this particular install already told Apple during ADI provisioning, and + the serial has to be the one this install has been presenting - a serial that changed + between sign-ins would register a second device in the user's account rather than reusing + the row they already have. Returns keyword arguments for the provider, so a library that grows another identity field fails loudly here instead of silently dropping it. """ - kwargs: dict = {"serial": APP_SERIAL} + kwargs: dict = {"serial": appSerial(localAnisette)} identity = hardwareProfile(localAnisette) if identity is not None: @@ -186,12 +228,46 @@ def deviceIdsForNewSession(localAnisette: Any) -> dict: return ids +def serialOfSession(asyncAccount: Any) -> str: + """ + The serial the session in hand **is already presenting**, read off its own provider. + + Not `appSerial`, and the difference is the whole point. A session signed in before serials + were drawn - or before any of this - is bound to whatever established it, and CloudKit has + to be told the same thing: the escrow record this app writes carries the serial, and the + picker recognises its own record by matching on it. Asking Java would give the serial this + install *would* draw today, which for a restored session is a different device. + + `BaseAppleAccount.serial` is public API and says the same thing this does - one value + describes one device, and a path sending a different one registers a second. So it is read + rather than reconstructed from the provider underneath it. + + Falls back to `LEGACY_SERIAL` when the account cannot answer. That is wrong in the same small + way it has always been wrong for everyone - the app's own escrow record stops being filtered + out of the recovery picker, which shows one tile nobody can use - and it is better than + defaulting the identity, which would put `0FINDMYPY001` on the record. + """ + try: + serial = asyncAccount.serial + except Exception: + print( + "Could not read the serial off this session, so CloudKit will be told" + f" {LEGACY_SERIAL}: {traceback.format_exc()}") + return LEGACY_SERIAL + + if not isinstance(serial, str) or not serial: + print(f"This session has no serial, so CloudKit will be told {LEGACY_SERIAL}.") + return LEGACY_SERIAL + + return serial + + def identityForRestore(previous: Any) -> dict: """ The identity a *restored* account should keep: whatever it already had. **Not this app's.** A session signed in before any of this was bound to FindMy.py's - defaults, and handing it `APP_SERIAL` now would present Apple with a different machine on + defaults, and handing it this app's serial now would present Apple with a different machine on an existing session - a sign-in for the user, and a second device-list entry they did not ask for. The gain would be a nicer name on an entry they have already learned to recognise. diff --git a/app/src/main/python/main.py b/app/src/main/python/main.py index 2341818a..1d97d8cd 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 ( @@ -220,7 +221,7 @@ def to_json(self, dst=None, /): was dropped. That is not hypothetical: writing only the type and the URL meant a session established - as `0PENTAGVIEWR` came back as FindMy.py's `0FINDMYPY001` on the next launch, and one + as this app's serial came back as FindMy.py's `0FINDMYPY001` on the next launch, and one established as a MacBookPro13,2 came back as a MacBookPro18,3. Two names and two machines for one session, which is exactly what rule 11 exists to prevent. @@ -296,7 +297,7 @@ def loginSync(email: str, password: str, anisetteServerUrl: str, try: # A new sign-in, so this is the one place the app's own identity is used. Everything # restored from a stored account keeps whatever it was established with - see - # identity.identityForRestore, and the warning on identity.APP_SERIAL. + # identity.identityForRestore, and the warning on identity.LEGACY_SERIAL. # # The machine half comes from Java, which persists it per install: an install from # before there was a choice keeps the Mac its ADI was provisioned with, and a fresh one @@ -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/layout/icloud_registered_note.xml b/app/src/main/res/layout/icloud_registered_note.xml index 4d17bb56..98ccf818 100644 --- a/app/src/main/res/layout/icloud_registered_note.xml +++ b/app/src/main/res/layout/icloud_registered_note.xml @@ -4,7 +4,7 @@ **A replica of the row, not a description of it.** Connecting registers this app as a device, and Apple renders that entry from the claimed model: title "MacBookPro", model "MacBook Pro 13"", - macOS 13.1, serial 0PENTAGVIEWR - confirmed against a real account in macOS Settings > Apple + macOS 13.1, serial 0PENTAGV**** - confirmed against a real account in macOS Settings > Apple Account > My Devices. The app does send a name and Apple ignores it, so nothing here says "OpenTagViewer", because that is not what the user will be looking at. @@ -92,7 +92,7 @@ android:layout_marginTop="5dp" android:textColor="?attr/colorOnSurfaceVariant" android:textSize="13sp" - tools:text="Serial Number: 0PENTAGVIEWR" /> + tools:text="Serial Number: 0PENTAGVK7QX" /> 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 Apple hat deinen Code angenommen und danach die Anmeldung nicht abschließen können. Der Code ist damit verbraucht, es wird also ein neuer gebraucht – wir warten kurz, bevor wir ihn anfordern, denn sofortiges Anfordern wird abgelehnt. Neuer Code wird in %1$d s bei Apple angefordert … Apple schließt die Anmeldung weiterhin nicht ab. Das liegt an Apple, nicht an dir und nicht an deinem Code.\n\nVersuche es in ein paar Minuten erneut. Dein Passwort wird dabei möglicherweise einmal abgelehnt – das gehört zum selben Fehler, gib es also einfach noch einmal ein, statt es für falsch zu halten. + Apple hat die Anmeldung abgelehnt. Das ist eine Ablehnung durch Apple und kein Problem mit deiner Apple-ID oder deinem Passwort. Ein erneuter Versuch nach kurzer Zeit lohnt sich, aber wenn es weiterhin abgelehnt wird, melde es bitte mit einem Protokoll, statt zu warten. + Apple antwortet gerade nicht + Apple hat die Anfrage abgelehnt, statt gar nicht zu antworten. Mit deinem Account ist alles in Ordnung, und es wurde nichts geändert. Versuche es bald erneut; wenn es weiterhin abgelehnt wird, melde es mit einem Protokoll. Verlauf importieren Verlaufsimport abgeschlossen Gelesene Zeilen: %1$d\nHinzugefügte Zeilen: %2$d\nBereits vorhanden: %3$d\nFehlerhaft: %4$d\nÜbersprungen (Tag nicht importiert): %5$d diff --git a/app/src/main/res/values-en/strings.xml b/app/src/main/res/values-en/strings.xml index 2fee314f..c01dbdf7 100644 --- a/app/src/main/res/values-en/strings.xml +++ b/app/src/main/res/values-en/strings.xml @@ -366,6 +366,9 @@ Your tags and their location history are not affected, and nothing was removed f Apple accepted your code and then had a problem finishing the sign-in. The code is used up, so a new one is needed — waiting a moment before asking for it, because asking straight away is refused. Asking Apple for a new code in %1$d s… Apple is still not finishing the sign-in. This is a fault on Apple\'s side, not something you did, and not your code.\n\nTry again in a few minutes. Your password may be refused once when you do — that is part of the same fault, so enter it again rather than assuming it is wrong. + Apple refused the sign-in. This is Apple declining, not a problem with your Apple ID or password. Trying again shortly is worth doing, but if it keeps refusing please report it with a log rather than waiting it out. + Apple is not answering right now + Apple refused the request rather than failing to answer it. Nothing is wrong with your account, and nothing was changed. Try again shortly; if it keeps refusing, report it with a log. Import History History import complete Rows read: %1$d\nRows added: %2$d\nAlready present: %3$d\nMalformed: %4$d\nSkipped (tag not imported): %5$d diff --git a/app/src/main/res/values-fr/strings.xml b/app/src/main/res/values-fr/strings.xml index 80849080..615a8cd0 100644 --- a/app/src/main/res/values-fr/strings.xml +++ b/app/src/main/res/values-fr/strings.xml @@ -366,6 +366,9 @@ Vos tags et leur historique de position ne sont pas touchés, et rien n’a ét Apple a accepté votre code puis n’a pas pu terminer la connexion. Le code est donc utilisé et il en faut un nouveau — nous patientons un instant avant de le demander, car une demande immédiate est refusée. Nouveau code demandé à Apple dans %1$d s… Apple ne termine toujours pas la connexion. C’est une panne du côté d’Apple, pas quelque chose que vous avez fait, ni votre code.\n\nRéessayez dans quelques minutes. Votre mot de passe pourra être refusé une fois à ce moment-là : cela fait partie de la même panne, saisissez-le à nouveau plutôt que de le croire erroné. + Apple a refusé la connexion. C\'est un refus d\'Apple, pas un problème avec votre identifiant ou votre mot de passe. Réessayer un peu plus tard vaut la peine, mais si le refus persiste, signalez-le avec un journal plutôt que d\'attendre. + Apple ne répond pas pour le moment + Apple a refusé la requête plutôt que de ne pas y répondre. Votre compte n\'a aucun problème et rien n\'a été modifié. Réessayez bientôt ; si le refus persiste, signalez-le avec un journal. Importer l’historique Importation de l’historique terminée Lignes lues : %1$d\nLignes ajoutées : %2$d\nDéjà présentes : %3$d\nIncorrectes : %4$d\nIgnorées (balise non importée) : %5$d diff --git a/app/src/main/res/values-ja/strings.xml b/app/src/main/res/values-ja/strings.xml index 7ef39d77..c913842d 100644 --- a/app/src/main/res/values-ja/strings.xml +++ b/app/src/main/res/values-ja/strings.xml @@ -366,6 +366,9 @@ Apple はコードを受け付けたあと、サインインを完了できませんでした。コードは使用済みなので新しいものが必要です。すぐに要求しても拒否されるため、少し待ってから要求します。 %1$d 秒後に Apple へ新しいコードを要求します… Apple はまだサインインを完了できていません。これは Apple 側の障害であり、あなたの操作やコードのせいではありません。\n\n数分後にもう一度お試しください。そのときパスワードが一度だけ拒否されることがありますが、これも同じ障害の一部です。間違っていると考えず、もう一度入力してください。 + Apple がサインインを拒否しました。Apple ID やパスワードの問題ではなく、Apple 側の拒否です。少し時間をおいて再試行する価値はありますが、拒否され続ける場合は待たずにログを添えて報告してください。 + 現在 Apple が応答していません + Apple は応答しなかったのではなく、要求を拒否しました。アカウントに問題はなく、何も変更されていません。少し後に再試行してください。拒否が続く場合はログを添えて報告してください。 履歴をインポート 履歴のインポートが完了しました 読み込んだ行: %1$d\n追加した行: %2$d\n既存の行: %3$d\n不正な行: %4$d\nスキップ (タグ未インポート): %5$d diff --git a/app/src/main/res/values-ko/strings.xml b/app/src/main/res/values-ko/strings.xml index 2b4a04eb..20b3796f 100644 --- a/app/src/main/res/values-ko/strings.xml +++ b/app/src/main/res/values-ko/strings.xml @@ -366,6 +366,9 @@ Apple이 코드를 받은 뒤 로그인을 끝내지 못했습니다. 코드는 이미 사용되었으므로 새 코드가 필요합니다. 바로 요청하면 거부되기 때문에 잠시 기다린 뒤 요청합니다. %1$d초 후에 Apple에 새 코드를 요청합니다… Apple이 아직 로그인을 마치지 못하고 있습니다. 이는 Apple 쪽 장애이며, 사용자의 잘못도 코드 문제도 아닙니다.\n\n몇 분 뒤에 다시 시도하세요. 그때 비밀번호가 한 번 거부될 수 있는데, 이것도 같은 장애의 일부이므로 틀렸다고 생각하지 말고 다시 입력하세요. + Apple이 로그인을 거부했습니다. Apple ID나 비밀번호의 문제가 아니라 Apple 쪽의 거부입니다. 잠시 후 다시 시도해 볼 만하지만, 계속 거부된다면 기다리지 말고 로그와 함께 신고해 주세요. + 지금은 Apple이 응답하지 않습니다 + Apple이 응답하지 못한 것이 아니라 요청을 거부했습니다. 계정에는 문제가 없으며 변경된 것도 없습니다. 잠시 후 다시 시도하고, 계속 거부되면 로그와 함께 신고해 주세요. 기록 가져오기 기록 가져오기 완료 읽은 행: %1$d\n추가된 행: %2$d\n이미 존재함: %3$d\n잘못된 행: %4$d\n건너뜀(태그를 가져오지 않음): %5$d diff --git a/app/src/main/res/values-nl/strings.xml b/app/src/main/res/values-nl/strings.xml index e6c492e9..bfa3c6d2 100644 --- a/app/src/main/res/values-nl/strings.xml +++ b/app/src/main/res/values-nl/strings.xml @@ -366,6 +366,9 @@ Je tags en hun locatiegeschiedenis blijven ongemoeid, en er is niets uit je Appl Apple heeft je code geaccepteerd en kon daarna het inloggen niet afronden. De code is dus opgebruikt en er is een nieuwe nodig — we wachten even voordat we die aanvragen, want meteen aanvragen wordt geweigerd. Over %1$d s wordt een nieuwe code bij Apple aangevraagd… Apple rondt het inloggen nog steeds niet af. Dit ligt aan Apple, niet aan jou en niet aan je code.\n\nProbeer het over een paar minuten opnieuw. Je wachtwoord kan dan één keer worden geweigerd — dat hoort bij dezelfde storing, dus voer het gewoon nog een keer in in plaats van aan te nemen dat het fout is. + Apple heeft de aanmelding geweigerd. Dit is een weigering van Apple, geen probleem met je Apple ID of wachtwoord. Het is de moeite waard het zo weer te proberen, maar blijft het geweigerd worden, meld het dan met een logbestand in plaats van af te wachten. + Apple reageert momenteel niet + Apple heeft het verzoek geweigerd in plaats van er niet op te antwoorden. Er is niets mis met je account en er is niets gewijzigd. Probeer het zo opnieuw; blijft het geweigerd worden, meld het dan met een logbestand. Geschiedenis importeren Geschiedenis geïmporteerd Gelezen rijen: %1$d\nToegevoegde rijen: %2$d\nAl aanwezig: %3$d\nOngeldig: %4$d\nOvergeslagen (tag niet geïmporteerd): %5$d diff --git a/app/src/main/res/values-ru/strings.xml b/app/src/main/res/values-ru/strings.xml index 9b7316aa..fbd9e9ef 100644 --- a/app/src/main/res/values-ru/strings.xml +++ b/app/src/main/res/values-ru/strings.xml @@ -366,6 +366,9 @@ Apple приняла ваш код, а затем не смогла завершить вход. Код уже использован, поэтому нужен новый — подождём немного перед запросом, потому что сразу запрашивать бесполезно. Запросим новый код у Apple через %1$d с… Apple по-прежнему не завершает вход. Это сбой на стороне Apple — не ваша вина и не проблема кода.\n\nПопробуйте снова через несколько минут. Пароль при этом может быть отклонён один раз: это часть того же сбоя, поэтому введите его ещё раз, а не считайте неверным. + Apple отклонила вход. Это отказ со стороны Apple, а не проблема с вашим Apple ID или паролем. Стоит повторить попытку чуть позже, но если отказы продолжаются, сообщите об этом с журналом, а не ждите. + Apple сейчас не отвечает + Apple отклонила запрос, а не осталась без ответа. С вашей учётной записью всё в порядке, и ничего не изменилось. Повторите попытку чуть позже; если отказы продолжаются, сообщите об этом с журналом. Импортировать историю Импорт истории завершён Прочитано строк: %1$d\nДобавлено строк: %2$d\nУже есть: %3$d\nПовреждено: %4$d\nПропущено (метка не импортирована): %5$d diff --git a/app/src/main/res/values-zh-rCN/strings.xml b/app/src/main/res/values-zh-rCN/strings.xml index e3e93695..d7b640ca 100644 --- a/app/src/main/res/values-zh-rCN/strings.xml +++ b/app/src/main/res/values-zh-rCN/strings.xml @@ -366,6 +366,9 @@ Apple 已接受你的验证码，随后未能完成登录。该验证码已被用掉，需要一个新的——我们会先等一会儿再申请，因为立刻申请会被拒绝。 将在 %1$d 秒后向 Apple 申请新验证码… Apple 仍未完成登录。这是 Apple 一侧的故障，不是你的操作问题，也不是验证码的问题。\n\n请过几分钟再试。届时你的密码可能会被拒绝一次——这属于同一个故障，请再输入一次，而不要以为密码错了。 + Apple 拒绝了此次登录。这是 Apple 的拒绝，不是你的 Apple ID 或密码有问题。稍后重试是值得的，但如果持续被拒绝，请附上日志报告，而不要一直等待。 + Apple 目前没有响应 + Apple 拒绝了该请求，而不是没有响应。你的账户没有问题，也没有任何更改。请稍后重试；如果持续被拒绝，请附上日志报告。 导入历史记录 历史记录导入完成 读取的行数：%1$d\n新增的行数：%2$d\n已存在：%3$d\n格式错误：%4$d\n已跳过（标签未导入）：%5$d diff --git a/app/src/main/res/values-zh-rTW/strings.xml b/app/src/main/res/values-zh-rTW/strings.xml index 09d28f1f..8db5db9a 100644 --- a/app/src/main/res/values-zh-rTW/strings.xml +++ b/app/src/main/res/values-zh-rTW/strings.xml @@ -366,6 +366,9 @@ Apple 已接受你的驗證碼，隨後未能完成登入。該驗證碼已被用掉，需要一個新的——我們會先等一下再申請，因為立刻申請會被拒絕。 將在 %1$d 秒後向 Apple 申請新驗證碼… Apple 仍未完成登入。這是 Apple 一側的故障，不是你的操作問題，也不是驗證碼的問題。\n\n請過幾分鐘再試。屆時你的密碼可能會被拒絕一次——這屬於同一個故障，請再輸入一次，而不要以為密碼錯了。 + Apple 拒絕了此次登入。這是 Apple 的拒絕，不是你的 Apple ID 或密碼有問題。稍後重試是值得的，但如果持續被拒絕，請附上紀錄檔回報，而不要一直等待。 + Apple 目前沒有回應 + Apple 拒絕了該請求，而不是沒有回應。你的帳號沒有問題，也沒有任何變更。請稍後重試；如果持續被拒絕，請附上紀錄檔回報。 匯入歷史記錄 歷史記錄匯入完成 讀取的列數：%1$d\n新增的列數：%2$d\n已存在：%3$d\n格式錯誤：%4$d\n已略過（標籤未匯入）：%5$d diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 1b0f5a80..88b57366 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -399,6 +399,9 @@ Your tags and their location history are not affected, and nothing was removed f Apple accepted your code and then had a problem finishing the sign-in. The code is used up, so a new one is needed — waiting a moment before asking for it, because asking straight away is refused. Asking Apple for a new code in %1$d s… Apple is still not finishing the sign-in. This is a fault on Apple\'s side, not something you did, and not your code.\n\nTry again in a few minutes. Your password may be refused once when you do — that is part of the same fault, so enter it again rather than assuming it is wrong. + Apple refused the sign-in. This is Apple declining, not a problem with your Apple ID or password. Trying again shortly is worth doing, but if it keeps refusing please report it with a log rather than waiting it out. + Apple is not answering right now + Apple refused the request rather than failing to answer it. Nothing is wrong with your account, and nothing was changed. Try again shortly; if it keeps refusing, report it with a log. Import History History import complete Rows read: %1$d\nRows added: %2$d\nAlready present: %3$d\nMalformed: %4$d\nSkipped (tag not imported): %5$d diff --git a/app/src/test/java/dev/wander/android/opentagviewer/anisette/DrawingTheSerialTest.java b/app/src/test/java/dev/wander/android/opentagviewer/anisette/DrawingTheSerialTest.java new file mode 100644 index 00000000..150b1dff --- /dev/null +++ b/app/src/test/java/dev/wander/android/opentagviewer/anisette/DrawingTheSerialTest.java @@ -0,0 +1,133 @@ +package dev.wander.android.opentagviewer.anisette; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertTrue; + +import org.junit.Test; + +import java.security.SecureRandom; +import java.util.HashSet; +import java.util.Set; + +/** + * The serial a fresh install draws, and the one property that matters about it. + * + *

Every install must not get the same one. That is the whole point of the change this + * belongs to: the serial was a constant, so one value went to Apple from thousands of installs, + * against thousands of different machine identities and Apple IDs, from every continent at once. + * Real hardware does not look like that, and it is the leading suspect for the Grand Slam 503s + * in issues #168, #176 and #181. Replacing one constant with a generator that is effectively + * another constant would change nothing while looking like a fix. + * + *

So the draws below take a fresh {@link SecureRandom} each time, which is the case + * being tested: a first run is one process, calling {@link AdiDeviceIdentity#generate()} once, + * and then never again. A single shared instance producing a good spread would prove nothing + * about that - it is exactly the shape that hides a fixed seed. + * + *

JVM rather than instrumented (rule 13): this is arithmetic over a string, and nothing here + * touches Android. + */ +public class DrawingTheSerialTest { + + private static final int DRAWS = 500; + + /** As a first run does it: one process, one generator, one draw. */ + private static String asAFreshInstallWould() { + return AdiDeviceIdentity.generateSerial(new SecureRandom()); + } + + @Test + public void ithasTheShapeAppleAccepts() { + final String serial = asAFreshInstallWould(); + + assertEquals("Apple's serials are twelve characters", 12, serial.length()); + assertTrue("uppercase alphanumeric only", serial.matches("[0-9A-Z]{12}")); + } + + @Test + public void itisRecognisableAsThisApp() { + assertTrue("a user who cannot recognise the entry removes it", + asAFreshInstallWould().startsWith(AdiDeviceIdentity.SERIAL_PREFIX)); + } + + @Test + public void everyCharacterDrawnComesFromTheDeclaredAlphabet() { + for (int i = 0; i < DRAWS; i++) { + final String tail = + asAFreshInstallWould().substring(AdiDeviceIdentity.SERIAL_PREFIX.length()); + + for (final char drawn : tail.toCharArray()) { + assertTrue("drew " + drawn + ", which is outside the alphabet", + AdiDeviceIdentity.SERIAL_ALPHABET.indexOf(drawn) >= 0); + } + } + } + + /** + * The assertion this class exists for. + * + *

Five hundred separate first runs, five hundred separate generators. The threshold is + * loose on purpose - four characters from a 26-character alphabet is about 457,000 values, so + * a collision in 500 draws is possible and not a fault. A seeded generator would + * produce one distinct value, not 450. + */ + @Test + public void afreshInstallDoesNotGetWhatEveryOtherFreshInstallGot() { + final Set drawn = new HashSet<>(); + for (int i = 0; i < DRAWS; i++) { + drawn.add(asAFreshInstallWould()); + } + + assertTrue("only " + drawn.size() + " distinct serials in " + DRAWS + " first runs," + + " which is what a seeded generator looks like", + drawn.size() > 450); + } + + /** And the identity as a whole carries it, rather than the serial being drawn elsewhere. */ + @Test + public void ageneratedIdentityAlreadyHasOne() { + final AdiDeviceIdentity identity = AdiDeviceIdentity.generate(); + + assertTrue(identity.serial().startsWith(AdiDeviceIdentity.SERIAL_PREFIX)); + assertNotEquals("a fresh install is not an install from before serials were drawn", + AdiDeviceIdentity.LEGACY_SERIAL, identity.serial()); + assertNotEquals("two installs are two identities", + identity.serial(), AdiDeviceIdentity.generate().serial()); + } + + /** + * The pairs a person comparing this screen against their device list would confuse. + * + *

Nobody types a serial. They read it off one screen and look for it on another, and the + * only failure mode is looking at the right row and believing it is the wrong one. + */ + @Test + public void thealphabetLeavesOutWhatAReaderWouldConfuse() { + for (final char confusable : "O0I1S5B8Z2".toCharArray()) { + assertEquals(confusable + " is a confusable and must not be drawable", + -1, AdiDeviceIdentity.SERIAL_ALPHABET.indexOf(confusable)); + } + } + + /** + * The old constant can never be drawn, which is what makes it legible in a bug report. + * + *

{@code IEWR} contains an {@code I}, and the alphabet does not. So a serial with an + * {@code I} in it is an install from before this change - and that is worth pinning, because + * a widened alphabet would quietly take the distinction away. + */ + @Test + public void thelegacySerialIsNotSomethingAnInstallCanDraw() { + final String tail = AdiDeviceIdentity.LEGACY_SERIAL + .substring(AdiDeviceIdentity.SERIAL_PREFIX.length()); + + boolean undrawable = false; + for (final char character : tail.toCharArray()) { + undrawable |= AdiDeviceIdentity.SERIAL_ALPHABET.indexOf(character) < 0; + } + + assertTrue("every character of " + AdiDeviceIdentity.LEGACY_SERIAL + " is drawable," + + " so a drawn serial can no longer be told from a legacy one", undrawable); + } +} diff --git a/app/src/test/java/dev/wander/android/opentagviewer/python/icloud/AppleDecliningIsItsOwnFailureTest.java b/app/src/test/java/dev/wander/android/opentagviewer/python/icloud/AppleDecliningIsItsOwnFailureTest.java new file mode 100644 index 00000000..84cf8c11 --- /dev/null +++ b/app/src/test/java/dev/wander/android/opentagviewer/python/icloud/AppleDecliningIsItsOwnFailureTest.java @@ -0,0 +1,69 @@ +package dev.wander.android.opentagviewer.python.icloud; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; + +import org.junit.Test; + +/** + * A 503 from Apple is its own answer, and specifically not the two it used to be mistaken for. + * + *

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..6eff5a90 100644 --- a/app/src/test/python/test_icloud_bridge.py +++ b/app/src/test/python/test_icloud_bridge.py @@ -27,6 +27,7 @@ from findmy.errors import InvalidCredentialsError, UnauthorizedError import icloud_bridge +import identity as app_identity from exporter import icloud from cryptography.hazmat.primitives.asymmetric import ec from findmy.keychain.join import JoinedPeer @@ -169,7 +170,7 @@ class FakeAsyncAccount: than composed - a path that invents its own makes one client look like several. """ - serial = "0PENTAGVIEWR" + serial = "0PENTAGVK7QX" identity = SimpleNamespace( model="iPhone17,1", os_name="iPhone OS", os_version="18.1", os_build="22B83", cfnetwork="1568.100.1", darwin="24.1.0") @@ -239,15 +240,39 @@ class TestTheIdentityItPresents: A default here would present `0PENTAGXPORT` from a phone - the desktop exporter's serial - and the two programs would share one device-list entry that neither could be removed from without breaking the other. + + **The serial is the session's, not a constant.** It is drawn per install now, and a restored + session keeps whatever established it - so a constant here would be right for a fresh install + and wrong for every other one, in the field CloudKit writes into the escrow record. """ - def test_it_is_this_app_and_not_the_exporter(self): - assert icloud_bridge.APP_IDENTITY.serial == "0PENTAGVIEWR" - assert icloud_bridge.APP_IDENTITY != icloud.EXPORTER_IDENTITY + def test_it_is_the_serial_this_session_presents(self): + identity = icloud_bridge.appIdentity(FakeAsyncAccount()) + + assert identity.serial == FakeAsyncAccount.serial + + def test_it_is_not_the_exporters(self): + assert icloud_bridge.appIdentity(FakeAsyncAccount()) != icloud.EXPORTER_IDENTITY def test_the_cloudkit_name_is_set_rather_than_left_to_the_library(self): - assert icloud_bridge.APP_IDENTITY.device_name - assert icloud_bridge.APP_IDENTITY.device_name != icloud.EXPORTER_IDENTITY.device_name + identity = icloud_bridge.appIdentity(FakeAsyncAccount()) + + assert identity.device_name + assert identity.device_name != icloud.EXPORTER_IDENTITY.device_name + + def test_a_session_that_cannot_say_names_this_app_anyway(self): + """ + Never the library's `0FINDMYPY001`, which is the one thing worse than a stale serial. + + The record would be written under a name that says nothing about what put it there, on + an account the user then has to recognise it in. + """ + class Mute: + @property + def serial(self): + raise RuntimeError("FindMy.py moved") + + assert icloud_bridge.appIdentity(Mute()).serial == app_identity.LEGACY_SERIAL def test_it_is_what_reaches_open_client(self, loop, monkeypatch): seen = {} @@ -257,9 +282,11 @@ async def fake_open_client(account, identity=None): return FakeClient() monkeypatch.setattr(icloud, "open_client", fake_open_client) - icloud_bridge.openSession(FakeAccount(loop)).open() + session = icloud_bridge.openSession(FakeAccount(loop)) + session.open() - assert seen["identity"] is icloud_bridge.APP_IDENTITY + assert seen["identity"].serial == FakeAsyncAccount.serial + assert seen["identity"] is session._identity class TestWhatCanBeRecoveredFrom: @@ -283,7 +310,7 @@ def test_this_apps_own_record_is_not_offered_to_unlock_with(self, session): """ made = session(FakeClient(FakeOptions([ FakeRecord("F2LX9Q"), - FakeRecord(icloud_bridge.APP_IDENTITY.serial, name="OpenTagViewer"), + FakeRecord(FakeAsyncAccount.serial, name="OpenTagViewer"), FakeRecord("C02XK"), ]))) @@ -301,11 +328,11 @@ def test_it_cannot_be_unlocked_with_either(self, session): """ made = session(FakeClient(FakeOptions([ FakeRecord("F2LX9Q"), - FakeRecord(icloud_bridge.APP_IDENTITY.serial), + FakeRecord(FakeAsyncAccount.serial), ]))) made.recoveryOptions() - answer = json.loads(made.unlock(icloud_bridge.APP_IDENTITY.serial, "123456")) + answer = json.loads(made.unlock(FakeAsyncAccount.serial, "123456")) assert not answer["ok"] @@ -317,7 +344,7 @@ def test_an_account_holding_only_this_apps_record_has_nothing_to_recover_from(se from, then presenting a list with nothing in it. """ made = session(FakeClient(FakeOptions( - [FakeRecord(icloud_bridge.APP_IDENTITY.serial)], trustworthy=True))) + [FakeRecord(FakeAsyncAccount.serial)], trustworthy=True))) answer = json.loads(made.recoveryOptions()) @@ -742,7 +769,7 @@ def test_it_describes_itself_with_the_identity_it_already_presents(self, session made.join("a-passcode") device = made.client.joinedWith.device - assert device.serial == "0PENTAGVIEWR" + assert device.serial == FakeAsyncAccount.serial assert device.model == "iPhone17,1" assert device.build == "22B83" assert made.client.joinedWith.os_version == "18.1" @@ -997,6 +1024,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_identity.py b/app/src/test/python/test_identity.py index f9adaf21..d2b0d778 100644 --- a/app/src/test/python/test_identity.py +++ b/app/src/test/python/test_identity.py @@ -50,12 +50,26 @@ DEVID = "1A2B3C4D-5E6F-4071-8293-A4B5C6D7E8F9" +DRAWN_SERIAL = "0PENTAGVK7QX" +""" +A serial only the generator could have produced, standing in for a real install's. + +Not `LEGACY_SERIAL`, deliberately: a fake answering the old constant would let every assertion +below pass against code that ignored the bridge and fell back, which is the regression these +tests exist for. `IEWR` contains an `I` and the alphabet does not, so the two cannot be confused. +""" + + class Bridge: """A local-Anisette bridge that works.""" - def __init__(self, profile=None, ids=None): + def __init__(self, profile=None, ids=None, serial=DRAWN_SERIAL): self._profile = IPHONE if profile is None else profile self._ids = {"uid": UID, "devid": DEVID} if ids is None else ids + self._serial = serial + + def serial(self): + return self._serial def deviceIdsJson(self): return json.dumps(self._ids) @@ -86,20 +100,58 @@ def unavailableReason(self): return "no libraries" -class TestTheIdentityANewSessionPresents: - def test_the_serial_is_the_apps_own_and_not_the_librarys(self): - assert identity.APP_SERIAL == "0PENTAGVIEWR" - assert identity.APP_SERIAL != CLIENT_SERIAL +class TestTheSerialThisInstallPresents: + """ + Java draws it once per install and stores it; this side asks rather than deciding. + + The constant it replaced went to every install of this app in the world at once, against + thousands of different machine identities - see `AdiDeviceIdentity.LEGACY_SERIAL`. + """ - def test_the_serial_is_the_shape_apple_accepts(self): - assert len(identity.APP_SERIAL) == 12 - assert identity.APP_SERIAL.isalnum() - assert identity.APP_SERIAL.upper() == identity.APP_SERIAL + def test_it_is_whatever_java_says_this_install_drew(self): + assert identity.appSerial(Bridge()) == DRAWN_SERIAL + + def test_it_is_not_the_librarys(self): + assert identity.appSerial(Bridge()) != CLIENT_SERIAL + + def test_the_legacy_value_is_still_what_an_older_install_presents(self): + """ + Java returns it for an install that has an identity and no stored serial. - def test_the_serial_is_not_the_exporters(self): + That install has been telling Apple this for its whole life, and a new serial now would + register a second device beside the row the user already has. + """ + assert identity.appSerial(Bridge(serial=identity.LEGACY_SERIAL)) == identity.LEGACY_SERIAL + assert identity.LEGACY_SERIAL == "0PENTAGVIEWR" + + def test_the_legacy_value_is_the_shape_apple_accepts(self): + assert len(identity.LEGACY_SERIAL) == 12 + assert identity.LEGACY_SERIAL.isalnum() + assert identity.LEGACY_SERIAL.upper() == identity.LEGACY_SERIAL + + def test_it_is_not_the_exporters(self): # Two installs, two entries, each removable without breaking the other. - assert identity.APP_SERIAL != "0PENTAGXPORT" - assert identity.APP_SERIAL[:5] == "0PENT" + assert identity.appSerial(Bridge()) != "0PENTAGXPORT" + assert identity.appSerial(Bridge())[:5] == "0PENT" + + def test_a_bridge_that_cannot_answer_falls_back_rather_than_failing_the_login(self): + class Broken(Bridge): + def serial(self): + raise RuntimeError("no such method") + + assert identity.appSerial(Broken()) == identity.LEGACY_SERIAL + + def test_an_empty_answer_is_not_passed_on(self): + """An empty serial would let FindMy.py name the device-list row `0FINDMYPY001`.""" + assert identity.appSerial(Bridge(serial="")) == identity.LEGACY_SERIAL + + def test_no_bridge_at_all_still_names_this_app(self): + assert identity.appSerial(None) == identity.LEGACY_SERIAL + + +class TestTheIdentityANewSessionPresents: + def test_the_serial_is_the_one_this_install_drew(self): + assert identity.identityForNewSession(Bridge())["serial"] == DRAWN_SERIAL def test_the_machine_is_whichever_one_java_says_this_install_is(self): """ @@ -133,7 +185,7 @@ def test_a_new_login_uses_it_on_both_transports(self): **identity.identityForNewSession(bridge), ) - assert provider.serial == identity.APP_SERIAL + assert provider.serial == DRAWN_SERIAL assert provider.identity == DeviceIdentity(**IPHONE) def test_a_legacy_install_signing_in_again_presents_the_mac_it_provisioned_with(self): @@ -151,14 +203,14 @@ def test_a_legacy_install_signing_in_again_presents_the_mac_it_provisioned_with( assert kwargs["identity"] == DeviceIdentity(**LEGACY_MAC) assert kwargs["identity"].platform == " " - def test_the_serial_is_the_apps_even_for_a_legacy_install(self): + def test_the_serial_is_this_installs_even_for_a_legacy_machine_profile(self): """ - The serial is a label and the machine is not, so they are not gated together. + The two are stored together and read together, but they are not the same question. - A new sign-in gets a new device-list entry whatever happens, so there is nothing to - preserve by withholding a recognisable name from it. + An install can be a `LEGACY_MAC` and still have drawn a serial - the profile records what + ADI was provisioned as, the serial records what Apple prints on the row. """ - assert identity.identityForNewSession(Bridge(LEGACY_MAC))["serial"] == identity.APP_SERIAL + assert identity.identityForNewSession(Bridge(LEGACY_MAC))["serial"] == DRAWN_SERIAL class TestTheIdsANewSessionIntroducesItselfWith: @@ -256,7 +308,7 @@ class TestWhenJavaCannotBeAsked: def test_no_bridge_at_all_imposes_nothing(self): assert identity.hardwareProfile(None) is None - assert identity.identityForNewSession(None) == {"serial": identity.APP_SERIAL} + assert identity.identityForNewSession(None) == {"serial": identity.LEGACY_SERIAL} assert identity.deviceIdsForNewSession(None) == {} def test_a_bridge_that_cannot_give_ids_lets_the_library_mint_its_own(self): @@ -351,14 +403,14 @@ def test_a_restored_new_style_account_keeps_the_apps_identity_too(self): established = DeviceIdentity(**IPHONE) previous = RemoteAnisetteProvider( "https://example.invalid", - serial=identity.APP_SERIAL, + serial=DRAWN_SERIAL, identity=established, ) carried = identity.identityForRestore(previous) replacement = main.LocalAnisetteProvider(Bridge(), "https://example.invalid", **carried) - assert replacement.serial == identity.APP_SERIAL + assert replacement.serial == DRAWN_SERIAL assert replacement.identity == established def test_a_restore_never_asks_java_what_this_install_is(self): @@ -464,7 +516,7 @@ class TestWhatSurvivesBeingStored: A field left out of it is not defaulted, it is **reverted** - silently, on a session Apple has already bound to the value that was dropped. Both of these shipped: a session established - as `0PENTAGVIEWR` came back as `0FINDMYPY001`, and one established as a MacBookPro13,2 came + with a drawn serial came back as `0FINDMYPY001`, and one established as a MacBookPro13,2 came back as FindMy.py's MacBookPro18,3. """ @@ -473,11 +525,11 @@ class TestWhatSurvivesBeingStored: def _stored(self): return main.LocalAnisetteProvider( Bridge(), "https://example.invalid", - serial=identity.APP_SERIAL, identity=self.MAC, + serial=DRAWN_SERIAL, identity=self.MAC, ).to_json() def test_the_serial_the_session_was_established_with_survives(self): - assert self._stored()["serial"] == identity.APP_SERIAL + assert self._stored()["serial"] == DRAWN_SERIAL def test_the_machine_it_was_established_as_survives(self): assert self._stored()["identity"] == self.MAC.to_json() @@ -485,7 +537,7 @@ def test_the_machine_it_was_established_as_survives(self): def test_a_restored_provider_presents_both_again(self): restored = RemoteAnisetteProvider.from_json(self._stored()) - assert restored.serial == identity.APP_SERIAL + assert restored.serial == DRAWN_SERIAL assert restored.identity == self.MAC def test_and_identityForRestore_then_carries_them_onward(self): @@ -498,7 +550,7 @@ def test_and_identityForRestore_then_carries_them_onward(self): carried = identity.identityForRestore(restored) - assert carried["serial"] == identity.APP_SERIAL + assert carried["serial"] == DRAWN_SERIAL assert carried["identity"] == self.MAC def test_a_provider_that_imposed_nothing_writes_nothing(self): 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/docs/how-to-export-with-the-cli.md b/docs/how-to-export-with-the-cli.md index 3054867a..fa919022 100644 --- a/docs/how-to-export-with-the-cli.md +++ b/docs/how-to-export-with-the-cli.md @@ -395,13 +395,16 @@ Mac, or appleid.apple.com: | --- | --- | | Model | **MacBook Pro** (`MacBookPro18,3`) | | Version | **macOS 13.4.1** | -| Serial Number | **`0PENTAGXPORT`** | +| Serial Number | **`0PENTAGX` followed by four characters** | **It is not a Mac and you do not own one of these.** The model and OS come from FindMy.py, which has always presented itself as a MacBook Pro and authenticates fine that way; the serial is this -exporter's, chosen to be legible and deliberately implausible as real hardware. That serial is the -part to recognise it by — `0PENTAGXPORT` is the exporter, and `0PENTAGVIEWR` is the Android app if -you also use that. +exporter's, chosen to be legible and deliberately implausible as real hardware. + +**The prefix is the part to recognise it by**, because the last four characters are drawn once on +this machine and are yours alone: `0PENTAGX…` is the exporter and `0PENTAGV…` is the Android app, +if you also use that. Installs from before this changed show `0PENTAGXPORT` and `0PENTAGVIEWR` +exactly, and keep doing so — there is nothing to do about that and no reason to. **It is one entry, not one per export.** The identity is stable, so running this again reuses it. 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/device.py b/python/exporter/device.py index 0129a1f7..20aae78f 100644 --- a/python/exporter/device.py +++ b/python/exporter/device.py @@ -9,8 +9,10 @@ So the identity is kept between runs. What that means precisely: -- **What is stored**: two UUIDs Apple knows this installation by, and the Anisette provisioning - data - the state that makes this machine the same machine to Apple's servers. +- **What is stored**: two UUIDs Apple knows this installation by, the serial it presents, and the + Anisette provisioning data - the state that makes this machine the same machine to Apple's + servers. The serial is here rather than a constant in the source because a constant meant every + install of this program presented Apple the same one; see `identity.EXPORTER_SERIAL`. - **What is not**: the Apple ID, the password, any session token, any keychain key, anything about the accessories. :func:`_only_identity` enforces that rather than trusting this docstring, and refuses to write a file carrying anything else. @@ -37,7 +39,7 @@ # Everything this file may contain. A key outside this set is a bug or a change upstream, and # either way the file is not written - the point of keeping it is that it holds nothing sensitive, # and a check is worth more than an intention. -_ALLOWED = frozenset({"uid", "devid", "anisette"}) +_ALLOWED = frozenset({"uid", "devid", "anisette", "serial"}) # Nothing named like this reaches disk, whatever else changes. Belt and braces with `_ALLOWED`, # because the anisette mapping comes from a library and its shape is not this project's to fix. @@ -82,7 +84,13 @@ def load(path: Path | None = None) -> dict[str, Any] | None: return stored -def save(uid: str, devid: str, anisette: Any, path: Path | None = None) -> None: +def save( + uid: str, + devid: str, + anisette: Any, + path: Path | None = None, + serial: str | None = None, +) -> None: """ Store the identity for next time. @@ -90,7 +98,13 @@ def save(uid: str, devid: str, anisette: Any, path: Path | None = None) -> None: export that has already succeeded costs the export. """ path = path or identity_path() - document = {"uid": uid, "devid": devid, "anisette": anisette} + document: dict[str, Any] = {"uid": uid, "devid": devid, "anisette": anisette} + + # Omitted rather than defaulted when a caller does not pass one, so that a save from a path + # that does not know the serial cannot overwrite a stored one with a guess. `serial_from` + # reads an absent key as "an install from before this varied", which is the right answer. + if serial: + document["serial"] = serial try: _only_identity(document) diff --git a/python/exporter/icloud.py b/python/exporter/icloud.py index b232d69d..8af7032f 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, @@ -55,7 +55,7 @@ from findmy.keychain.session import AsyncKeychainSession from exporter import device -from exporter.identity import DEVICE_NAME, EXPORTER_SERIAL +from exporter.identity import DEVICE_NAME, EXPORTER_SERIAL, serial_from from opentagviewer_export import AccessoryExport from opentagviewer_export.hardware import identify @@ -244,6 +244,17 @@ def make_account( """ stored = device.load(identity_path) + # **The serial is per install, and this is where that is decided.** A caller that named one + # explicitly keeps it - the app passes its own, and tests pin theirs - but the default + # identity's placeholder is replaced by whatever this installation has been using, or by a + # fresh one if it has never run. See `identity.EXPORTER_SERIAL` for why a constant was wrong. + if identity is EXPORTER_IDENTITY: + identity = ClientIdentity( + serial=serial_from(stored), device_name=identity.device_name) + + # After the substitution above, never before: the provider is what actually sends + # `X-Apple-I-SRL-NO`, so building it from the placeholder identity would present the constant + # this change exists to stop presenting. if provider is None: provider = _make_provider(anisette_url, libs_path, stored, identity) @@ -301,7 +312,11 @@ def _make_provider( libs_path=libs_path, serial=identity.serial, state_blob=state_blob) -def remember(account: AsyncAppleAccount, identity_path: Path | None = None) -> None: +def remember( + account: AsyncAppleAccount, + identity_path: Path | None = None, + serial: str | None = None, +) -> None: """ Store this account's device identity, so the next export is the same device. @@ -321,16 +336,25 @@ def remember(account: AsyncAppleAccount, identity_path: Path | None = None) -> N the announce was tried, refused, and removed rather than left failing on every fresh install. **The serial is the label instead**, which is what §13 designed it for: `X-Apple-I-SRL-NO` is - sent during sign-in, needs no announce, and `0PENTAGXPORT` is the one field in that row a - person can actually read. See :mod:`exporter.identity`. + sent during sign-in, needs no announce, and it is the one field in that row a person can + actually read. See :mod:`exporter.identity`. + + :param serial: What this run presented, if a caller has some reason to override it. Normally + left out, and read off the account - **which is the only source that cannot be wrong.** + Re-deriving it from disk instead is subtly broken on the run that matters: a first run has + no file, so `serial_from` would draw a *second* serial and store that, and the next run + would introduce itself as a different device than the one just registered. """ + if serial is None: + serial = account.serial # The account's own serialisation, with only the harmless parts taken out of it. It also # carries the username, the password and the login state - which is exactly why this picks # fields rather than handing the whole mapping to `device.save`. state = account.to_json() device.save( - state["ids"]["uid"], state["ids"]["devid"], state["anisette"], identity_path) + state["ids"]["uid"], state["ids"]["devid"], state["anisette"], identity_path, + serial=serial) MAX_LOGIN_ATTEMPTS = 3 @@ -562,6 +586,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 +649,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/identity.py b/python/exporter/identity.py index b020c94f..f8664b23 100644 --- a/python/exporter/identity.py +++ b/python/exporter/identity.py @@ -29,15 +29,90 @@ from __future__ import annotations +SERIAL_PREFIX = "0PENTAGX" +""" +The eight characters every serial this exporter presents begins with. + +Recognisability lives here rather than in the whole string. An entry reading `0PENTAGX` followed +by anything is identifiably this project, which is what stops somebody removing it - see the +warning above about what removing it costs. +""" + +SERIAL_ALPHABET = "ACDEFGHJKLMNPQRTUVWXY34679" +""" +What the four characters after the prefix are drawn from. + +Uppercase alphanumeric, which is the shape Apple accepts, with the pairs a person reading a serial +off one screen and comparing it to another is most likely to confuse left out - no `O` against `0`, +no `I` or `1`, no `S` against `5`, no `B` against `8`, no `Z` against `2`. The user does not type +this, but they do compare it, and that is the whole job it has. +""" + EXPORTER_SERIAL = "0PENTAGXPORT" """ -The serial this exporter presents, in `X-Apple-I-SRL-NO`. +The serial every install presented before this was drawn per install. + +**Kept, and still used**, by anything that already has an identity: changing the serial on an +install that works costs a second device-list entry and may cost a sign-in, for no benefit to +somebody who is not affected. + +It shares the prefix and the shape of a drawn serial but is **not** one, and cannot be: `PORT` +contains an `O`, which :data:`SERIAL_ALPHABET` leaves out as a confusable. So a serial with an `O` +in it is, by construction, an install from before this change - useful when reading a report, and +the reason this is a named constant rather than a value that happens to come out of the generator. + +**Why this stopped being the only one.** It was a constant, so every install of this program, +everywhere, presented Apple the same serial while presenting a *different* machine identity: one +serial against thousands of device IDs and thousands of Apple IDs, from every continent, at once. +Real hardware does not look like that. A 503 from Grand Slam that some accounts never recover from +- issues #168, #176 and #181 - is consistent with that fingerprint being refused, and one reporter +cleared their device identity to no effect, which is what would happen if the serial were the part +being matched on. -Twelve uppercase alphanumeric characters, which is the shape Apple accepts, and deliberately -implausible as real hardware so that nothing mistakes it for a Mac. It shares its prefix with the -Android app's `0PENTAGVIEWR` so a user seeing both recognises them as the same project. +That is a hypothesis and is written down as one. It has not been confirmed against Apple, and the +cheap way to confirm it is exactly this change: an affected user deleting their identity file now +draws a different serial instead of the same one. """ + +def generate_serial() -> str: + """ + A serial for an install that does not have one yet. + + Twelve characters, of which the last four vary - about 450,000 of them, which is not a large + space and does not need to be. The point is that two installs are unlikely to share one, not + that a serial is unguessable; there is nothing to guess. + + `secrets` rather than `random` for no security reason: it is seeded from the OS, and a program + that starts twice in the same second should not be able to draw the same serial twice. + """ + import secrets + + tail = "".join(secrets.choice(SERIAL_ALPHABET) for _ in range(4)) + return SERIAL_PREFIX + tail + + +def serial_from(stored: dict | None) -> str: + """ + The serial this install should present, given whatever is on disk. + + Three cases, and the middle one is the reason this is a function: + + - **A stored serial**: use it. This is every run after the first. + - **A stored identity with no serial**: an install from before serials varied. It keeps + :data:`EXPORTER_SERIAL`, because it has been presenting that to Apple and changing it now + would re-identify a working install. + - **Nothing stored**: a new install, or one whose identity file was deleted. It draws a new + one. Deleting the file is therefore the remedy for an account that Apple is refusing, which + is a thing a person can do without a new release. + """ + if stored is None: + return generate_serial() + + serial = stored.get("serial") + return serial if isinstance(serial, str) and serial else EXPORTER_SERIAL + + DEVICE_NAME = "OpenTagViewer Exporter" """ What this client tells CloudKit it is called. 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_serial_is_per_install.py b/python/test/test_serial_is_per_install.py new file mode 100644 index 00000000..c8c0d218 --- /dev/null +++ b/python/test/test_serial_is_per_install.py @@ -0,0 +1,245 @@ +""" +Every install presenting the same serial is what this stops. + +`EXPORTER_SERIAL` was a constant, so every copy of this program anywhere told Apple it was the +same machine - one serial against thousands of device identities and thousands of Apple IDs, from +everywhere, at once. Issues #168, #176 and #181 are a 503 from Grand Slam that some accounts never +recover from, and one reporter cleared their device identity to no effect, which is what would +happen if the serial were the part being matched on. + +That is a hypothesis. These tests are about the properties the fix has to have either way. +""" + +from __future__ import annotations + +import random +import subprocess +import sys +from pathlib import Path + +import pytest + +from exporter import device, icloud +from exporter.identity import ( + EXPORTER_SERIAL, + SERIAL_ALPHABET, + SERIAL_PREFIX, + generate_serial, + serial_from, +) + + +class TestTheShapeAppleAccepts: + def test_itis_twelve_uppercase_alphanumerics(self): + serial = generate_serial() + + assert len(serial) == 12 + assert serial.isalnum() + assert serial == serial.upper() + + def test_itis_recognisable_as_this_project(self): + """The prefix is the whole reason a user does not remove the entry.""" + assert generate_serial().startswith(SERIAL_PREFIX) + + def test_the_serial_it_replaced_has_the_same_prefix_and_shape(self): + """ + So a user seeing either one reads the same project. + + It is *not* a member of the drawn set, and cannot be: `PORT` contains an `O`, which the + alphabet excludes as a confusable. That is worth knowing rather than fixing - a serial + containing `O` is by construction an install from before this change, and nothing here + depends on the old value being drawable. + """ + assert EXPORTER_SERIAL.startswith(SERIAL_PREFIX) + assert len(EXPORTER_SERIAL) == 12 + + def test_the_serial_it_replaced_can_never_be_drawn(self): + """Pinned, because the docstring above claims it and a widened alphabet would break it.""" + tail = EXPORTER_SERIAL[len(SERIAL_PREFIX):] + assert any(character not in SERIAL_ALPHABET for character in tail) + + def test_the_alphabet_leaves_out_what_a_reader_would_confuse(self): + """A person compares this across two screens; that is the only job it has.""" + for confusable in "O0I1S5B8Z2": + assert confusable not in SERIAL_ALPHABET, confusable + + +class TestItIsActuallyRandom: + """ + The failure that would quietly reinstate the constant. + + A generator seeded the same way on every machine hands every fresh install the same serial, + and nothing downstream would notice: the shape is right, it persists, it looks per-install. + """ + + def test_many_draws_are_almost_all_different(self): + drawn = [generate_serial() for _ in range(500)] + + assert len(set(drawn)) > 450, "the tail is not varying" + + def test_seeding_the_ordinary_random_module_does_not_pin_it(self): + """ + `secrets` reads OS entropy and cannot be seeded. Asserted because switching it to + `random.choice` would pass every other test in this file. + """ + random.seed(0) + first = generate_serial() + random.seed(0) + second = generate_serial() + + assert first != second + + def test_separate_processes_do_not_agree(self): + """ + The real case: two people installing on two machines. + + In-process draws share one generator, so they would differ even from a seeded PRNG. Only + a fresh interpreter shows whether the entropy is per-process. + """ + script = ( + "import sys; sys.path.insert(0, %r);" + "from exporter.identity import generate_serial; print(generate_serial())" + % str(Path(__file__).resolve().parents[1]) + ) + drawn = { + subprocess.run( + [sys.executable, "-c", script], + capture_output=True, text=True, check=True, + ).stdout.strip() + for _ in range(5) + } + + assert len(drawn) == 5, f"separate processes agreed on a serial: {drawn}" + + +class TestWhichSerialAnInstallPresents: + def test_a_stored_serial_is_kept(self): + """Every run after the first. The file exists so this is stable.""" + assert serial_from({"uid": "u", "devid": "d", "serial": "0PENTAGXA3K9"}) == "0PENTAGXA3K9" + + def test_an_install_from_before_this_keeps_what_it_has_been_presenting(self): + """ + It has an identity and no serial, so it predates serials varying. + + Re-identifying a working install costs a second device-list entry and may cost a sign-in, + for no benefit to somebody who is not affected. + """ + assert serial_from({"uid": "u", "devid": "d"}) == EXPORTER_SERIAL + + def test_nothing_stored_draws_a_new_one(self): + """ + A new install - and also the remedy for an account Apple is refusing. + + Deleting the identity file now draws a different serial instead of the same one, which is + something a person can do without waiting for a release. + """ + assert serial_from(None) != EXPORTER_SERIAL + assert serial_from(None).startswith(SERIAL_PREFIX) + + def test_rubbish_in_the_file_is_treated_as_absent(self): + for nonsense in ({"serial": ""}, {"serial": None}, {"serial": 12}, {"serial": []}): + assert serial_from({"uid": "u", "devid": "d", **nonsense}) == EXPORTER_SERIAL + + +class TestItSurvivesBeingWrittenDown: + def test_it_round_trips_through_the_identity_file(self, tmp_path): + path = tmp_path / "device-identity.json" + device.save("uid-1", "devid-1", {"a": 1}, path, serial="0PENTAGXQ7WM") + + assert serial_from(device.load(path)) == "0PENTAGXQ7WM" + + def test_saving_without_one_does_not_overwrite_what_is_there(self, tmp_path): + """ + A save from a path that does not know the serial must not blank it. + + The key is omitted rather than written as null, so `serial_from` reads it the same way it + reads a file from before this existed. + """ + path = tmp_path / "device-identity.json" + device.save("uid-1", "devid-1", {"a": 1}, path) + + assert "serial" not in (device.load(path) or {}) + + def test_the_identity_file_still_refuses_anything_else(self, tmp_path): + """ + The guard that keeps credentials out of this file has to still be a guard. + + Adding a key to `_ALLOWED` is exactly when that gets loosened by accident. + """ + assert "serial" in device._ALLOWED + for forbidden in ("username", "password", "token", "session"): + assert forbidden not in device._ALLOWED + + +class TestItIsWhatActuallyGoesToApple: + """ + The provider is what sends `X-Apple-I-SRL-NO`, so the drawn serial has to reach *it*. + + Substituting the identity and then building the provider from the old one would leave every + request presenting the constant while every test about `serial_from` stayed green - which is + not hypothetical: the line building the provider was dropped while writing this change, and + nothing in the suite noticed, because every other test passes a provider in. + """ + + def test_an_account_built_without_a_provider_still_gets_one(self, tmp_path): + """ + `account.serial` reads through the provider, so it is also the check that there is one. + + With the provider left as None this raises rather than returning a wrong serial, which + is how the dropped line would have presented in the field: the first thing the wizard + does after building an account is fail on an attribute of None. + """ + account = icloud.make_account( + "https://example.invalid", identity_path=tmp_path / "identity.json") + + assert isinstance(account.serial, str) + + def test_and_that_provider_presents_the_drawn_serial(self, tmp_path): + account = icloud.make_account( + "https://example.invalid", identity_path=tmp_path / "identity.json") + + assert account.serial.startswith(SERIAL_PREFIX) + assert account.serial != EXPORTER_SERIAL + + def test_the_first_run_stores_the_serial_it_actually_signed_in_with(self, tmp_path): + """ + The one that is wrong in the way nobody notices until the second run. + + A first run has no identity file, so anything that re-derives the serial from disk draws + a *second* one and stores that - and the next run introduces itself to Apple as a + different device than the one just registered, adding an entry to the user's device list + every other time they export. + """ + path = tmp_path / "device-identity.json" + account = icloud.make_account("https://example.invalid", identity_path=path) + presented = account.serial + + icloud.remember(account, path) + + assert serial_from(device.load(path)) == presented + + def test_and_the_run_after_that_presents_the_same_one(self, tmp_path): + path = tmp_path / "device-identity.json" + first = icloud.make_account("https://example.invalid", identity_path=path) + icloud.remember(first, path) + + second = icloud.make_account("https://example.invalid", identity_path=path) + + assert second.serial == first.serial + + def test_a_caller_that_names_an_identity_keeps_it(self, tmp_path): + """The app passes its own, and must not have it replaced by the exporter's.""" + theirs = icloud.ClientIdentity(serial="0PENTAGVK7QX", device_name="OpenTagViewer") + + account = icloud.make_account( + "https://example.invalid", + identity=theirs, + identity_path=tmp_path / "identity.json") + + assert account.serial == "0PENTAGVK7QX" + + +@pytest.mark.parametrize("draw", range(20)) +def test_every_draw_is_within_the_declared_alphabet(draw): + """A character outside it would be a serial Apple might not accept.""" + assert set(generate_serial()[len(SERIAL_PREFIX):]) <= set(SERIAL_ALPHABET) 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" },