diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 0b5520f4..d9b28a11 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -404,7 +404,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@4f940158c437b17ae61a8e90af084d658750629c") + install("git+https://github.com/parawanderer/FindMy.py@ddc7f2342fc9f32ebe315b85c22a4554ce419f6d") install("NSKeyedUnArchiver==1.5") diff --git a/app/src/test/python/requirements.txt b/app/src/test/python/requirements.txt index b96abbc8..a365cb9e 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@4f940158c437b17ae61a8e90af084d658750629c +git+https://github.com/parawanderer/FindMy.py@ddc7f2342fc9f32ebe315b85c22a4554ce419f6d NSKeyedUnArchiver==1.5 PyYAML==6.0.2 diff --git a/python/exporter/cli.py b/python/exporter/cli.py index 6ec77114..6cb21399 100644 --- a/python/exporter/cli.py +++ b/python/exporter/cli.py @@ -831,6 +831,21 @@ async def run(arguments: argparse.Namespace) -> int: for skipped in fetched.skipped: print(f"Not exportable: {skipped.beacon_id} {skipped.reason}", file=sys.stderr) + if fetched.undecryptable: + # **Not phrased as a failure**, because it usually is not one: a zone holds records + # belonging to other parties too. It is said at all because the alternative is a + # shorter list with nothing to explain it, and "fewer tags than expected" reads + # exactly like "some of those were never tags". + print( + f"\n{fetched.undecryptable} record(s) in the account could not be read, and are" + " not in the list above.", + file=sys.stderr, + ) + print("Records belonging to somebody else look like this too, so this is often" + " nothing. It is", file=sys.stderr) + print("worth a second look only if it is close to the number of tags you expected" + " to see.", file=sys.stderr) + if fetched.candidates: picked = ( _take_all(fetched.candidates, include_my_devices=arguments.include_my_devices) diff --git a/python/exporter/icloud.py b/python/exporter/icloud.py index a1c15fe1..fe9a804e 100644 --- a/python/exporter/icloud.py +++ b/python/exporter/icloud.py @@ -132,6 +132,34 @@ class Fetched: candidates: list[Candidate] skipped: list[Skipped] + undecryptable: int = 0 + """ + How many records in the zone did not decrypt at all. + + **Counted separately from `skipped`, and not as a failure.** `skipped` is per accessory and + names one: something was read, understood, and set aside for a stated reason. These were never + read - they have no beacon id to name, because the id is inside what would not open. + + **A non-zero count is not necessarily wrong.** A zone legitimately holds records belonging to + other parties, which this account has no key for and never should. So it is reported as a + number and a caveat rather than as an error, and the only thing that would make it alarming is + it being close to the number of tags somebody expected. + + It exists because it was invisible: FindMy.py counted these, logged the tally and dropped it, + and `fetch` builds its `skipped` list by walking what came back - so anything lost here was + absent from both lists at once. "Fewer tags than expected" and "some of those were never tags" + look identical from outside, which is the same sentence the per-accessory list exists for. + """ + + first_miss: str | None = None + """ + The first "no key held", in full, when there was one. + + The count alone cannot tell the two causes apart: records that genuinely belong to somebody + else, and this client comparing keys in the wrong encoding. Those lead in opposite directions, + and only the message says which. + """ + @dataclass(frozen=True) class ClientIdentity: @@ -550,7 +578,21 @@ async def fetch(client: AsyncFindMyClient) -> Fetched: candidates.append(_candidate(group.beacon, group.naming, group.alignment)) - return Fetched(candidates=candidates, skipped=skipped) + if decrypted.skipped_total: + # Reported, not raised. See `Fetched.undecryptable` for why a count here is not + # automatically wrong - and why it still must not be silent. + logger.info( + "%d record(s) in the zone did not decrypt: %s", + decrypted.skipped_total, + ", ".join(f"{reason} x{count}" for reason, count in decrypted.skipped.items()), + ) + + return Fetched( + candidates=candidates, + skipped=skipped, + undecryptable=decrypted.skipped_total, + first_miss=decrypted.first_miss, + ) def _why_not_locatable(beacon: DecryptedRecord) -> str: diff --git a/python/exporter/wizard.py b/python/exporter/wizard.py index 5ee251b6..feaddea8 100644 --- a/python/exporter/wizard.py +++ b/python/exporter/wizard.py @@ -147,6 +147,7 @@ def __init__(self, *args, **kwargs) -> None: # What the source could not export, kept rather than passed around: the list is redrawn # whenever a key file is added, and that redraw knows nothing about the original read. self.skipped: list = [] + self.undecryptable = 0 self.route = source.detect() self._build() @@ -396,6 +397,7 @@ def _load(self) -> None: return self.candidates = fetched.candidates + self.undecryptable = fetched.undecryptable # Read once. A second read would sign in again, register nothing new and rebuild the list # under the ticks somebody has already made. self.read_button.configure(state="disabled") @@ -583,6 +585,16 @@ def _say_where_things_stand(self) -> None: f"{len(self.skipped)} record(s) could not be exported: they carry no key material.", ) + if self.undecryptable: + # Deliberately not "could not be exported", which is what the line above means and is + # a different thing: those were read and set aside, these were never read at all. + # Records belonging to other people look like this too, so it says so rather than + # implying something went wrong. + notes.append( + f"{self.undecryptable} record(s) could not be read, and are not listed - often" + " somebody else's, so this is usually nothing.", + ) + if not self.choices.get_children(): # The empty list is the first thing anybody sees now, so it has to say what the two # buttons are for - including that the second one needs no Apple account, which is the diff --git a/python/pyproject.toml b/python/pyproject.toml index fbc09a45..f01ddc0f 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 = "4f940158c437b17ae61a8e90af084d658750629c" } +FindMy = { git = "https://github.com/parawanderer/FindMy.py", rev = "ddc7f2342fc9f32ebe315b85c22a4554ce419f6d" } [dependency-groups] # Only the release build installs this, with `uv sync --no-default-groups --group build`. It is diff --git a/python/test/test_undecryptable_count.py b/python/test/test_undecryptable_count.py new file mode 100644 index 00000000..89ad0255 --- /dev/null +++ b/python/test/test_undecryptable_count.py @@ -0,0 +1,118 @@ +""" +Records that never decrypted, and why the exporter has to mention them. + +**`fetch` builds its "not exportable" list by walking what came back**, so anything +`decrypt_records` dropped was absent from that list and from the candidates at once - it +disappeared between two lines and the user saw a shorter table with nothing to explain it. + +This is the residue of issue #89's fix. That bug was *one unreadable item ends the whole export*, +fixed by skipping. Skipping silently is the other half of the same mistake, and it was reported by +the same person who found #89's cause - who had to count raw records by hand to notice. + +**A count here is not automatically wrong**, which is the reason it is reported as a number and a +caveat rather than raised: a zone legitimately holds records belonging to other parties. +""" + +from __future__ import annotations + +import asyncio + +import pytest + +from exporter import icloud + + +class FakeDecrypted(list): + """Stands in for FindMy.py's `DecryptedRecords`, which is a list carrying a tally.""" + + def __init__(self, records=(), *, skipped=None, first_miss=None): + super().__init__(records) + self.skipped = dict(skipped or {}) + self.first_miss = first_miss + + @property + def skipped_total(self) -> int: + return sum(self.skipped.values()) + + +@pytest.fixture +def account(monkeypatch): + """`fetch` with its Apple half replaced, returning whatever tally a test wants.""" + def _fetch(*, skipped=None, first_miss=None): + decrypted = FakeDecrypted(skipped=skipped, first_miss=first_miss) + + monkeypatch.setattr(icloud, "decrypt_records", lambda *_a, **_k: decrypted) + monkeypatch.setattr(icloud, "group_records", lambda _d: []) + + return decrypted + + return _fetch + + +def run_fetch(client): + """Driven with `asyncio.run`, as the rest of this suite does - there is no pytest-asyncio.""" + return asyncio.run(icloud.fetch(client)) + + +class FakeClient: + """The three calls `fetch` makes before it starts counting.""" + + class _Store: + async def zone_record_defaults(self, _keys): + return {} + + def __init__(self): + self.store = self._Store() + + async def zone_keys(self): + return {} + + async def records(self): + return [] + + +class TestTheCountReachesTheCaller: + def test_a_clean_read_reports_nothing(self, account): + account() + + fetched = run_fetch(FakeClient()) + + assert fetched.undecryptable == 0 + assert fetched.first_miss is None + + def test_every_reason_is_counted_not_just_the_first(self, account): + # Two different failure kinds. The tally used to be logged as a whole and dropped, so a + # caller that guessed from one of them would undercount the other. + account(skipped={"MissingKeyError": 4, "PCSError": 2}) + + fetched = run_fetch(FakeClient()) + + assert fetched.undecryptable == 6 + + def test_the_first_miss_is_carried(self, account): + # The count alone cannot tell "somebody else's records" from "we are comparing keys in + # the wrong encoding", and those lead in opposite directions. + account(skipped={"MissingKeyError": 1}, first_miss="no key held for ABC") + + fetched = run_fetch(FakeClient()) + + assert fetched.first_miss == "no key held for ABC" + + def test_it_does_not_become_a_not_exportable_row(self, account): + # `skipped` is per accessory and names one. These have no beacon id to name - the id is + # inside the thing that would not open - so folding them in would invent rows. + account(skipped={"MissingKeyError": 3}) + + fetched = run_fetch(FakeClient()) + + assert fetched.skipped == [] + assert fetched.undecryptable == 3 + + def test_it_is_not_raised(self, account): + # A zone holds other parties' records legitimately, so this must not fail an export that + # otherwise worked. + account(skipped={"MissingKeyError": 99}) + + fetched = run_fetch(FakeClient()) + + assert fetched.undecryptable == 99 diff --git a/python/uv.lock b/python/uv.lock index 8cde8c92..241917a1 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=4f940158c437b17ae61a8e90af084d658750629c#4f940158c437b17ae61a8e90af084d658750629c" } +source = { git = "https://github.com/parawanderer/FindMy.py?rev=ddc7f2342fc9f32ebe315b85c22a4554ce419f6d#ddc7f2342fc9f32ebe315b85c22a4554ce419f6d" } dependencies = [ { name = "aiohttp" }, { name = "anisette" }, @@ -1032,7 +1032,7 @@ dev = [ [package.metadata] requires-dist = [ - { name = "findmy", git = "https://github.com/parawanderer/FindMy.py?rev=4f940158c437b17ae61a8e90af084d658750629c" }, + { name = "findmy", git = "https://github.com/parawanderer/FindMy.py?rev=ddc7f2342fc9f32ebe315b85c22a4554ce419f6d" }, { name = "pycryptodome", specifier = "==3.22.0" }, { name = "pyyaml", specifier = "==6.0.2" }, { name = "pyzipper", specifier = "==0.4.0" },