Read tags from an Apple account, and show one nothing ever named

CONTRIBUTING.md

@@ -277,8 +277,15 @@ Report: `app/build/reports/tests/testDebugUnitTest/index.html`
 
 ### Chaquopy bridge tests
 
-Cover `app/src/main/python/main.py`, the module Chaquopy packages into the APK. It imports
-no Android or Java types, so it runs on plain CPython.
+Cover everything under `app/src/main/python/`, which Chaquopy packages into the APK: `main.py`,
+`identity.py` and `icloud_bridge.py`. None of them import Android or Java types, so they run on
+plain CPython — that constraint is what makes them testable at all, and it is worth keeping.
+
+They also reach the shared `python/` tree, because `conftest.py` puts it on the path exactly as
+the build packages it. What that cannot tell you is whether a module survives *being* packaged —
+`icloud_bridge` reaches `findmy.cloudkit` and therefore protobuf, which resolves on a laptop and
+is the shape of thing that goes missing on a phone. `PythonPackagingTest`, on the managed device,
+is what answers that half.
 
 ```bash
 python -m venv .venv

app/build.gradle.kts

@@ -290,6 +290,12 @@ chaquopy {
             install("git+https://github.com/parawanderer/FindMy.py@23a9b8d7109b405f8362ea1e69ebe51f9ca82fca")
 
             install("NSKeyedUnArchiver==1.5")
+
+            // OPENTAGVIEWER.yml, read and written by opentagviewer_export.bundle - which the app
+            // reaches as soon as it touches AccessoryExport, and will need in earnest once it
+            // writes bundles of its own. Pinned to the exporter's version in python/pyproject.toml
+            // so one repository ships one PyYAML, for the same reason it ships one FindMy.py.
+            install("PyYAML==6.0.2")
         }
     }
     productFlavors {}
@@ -315,7 +321,22 @@ chaquopy {
             // has no top-level .py files for it to catch by accident, and the test is what
             // notices if that stops being true.
             srcDir("../python")
-            include("*.py", "opentagviewer_export/**")
+            // **Named files from `exporter/`, not the package.** That directory also holds the
+            // tkinter wizard, the questionary CLI and prompt_toolkit prompts, none of which
+            // exist on Android - importing the package wholesale would drag them in. These four
+            // are stdlib plus FindMy.py, and `exporter/__init__.py` is empty, so importing
+            // `exporter.icloud` reaches none of the rest.
+            //
+            // A list rather than a glob for the same reason: a module added to `exporter/` later
+            // should have to be considered before it ships in the APK, not swept in.
+            include(
+                "*.py",
+                "opentagviewer_export/**",
+                "exporter/__init__.py",
+                "exporter/icloud.py",
+                "exporter/device.py",
+                "exporter/identity.py",
+            )
             // The package's own test suite is not part of the app. It imports pytest, which is
             // not in the APK, so it is dead weight that would fail if anything ever touched it.
             exclude("opentagviewer_export/tests/**")

app/src/androidTest/java/dev/wander/android/opentagviewer/python/PythonPackagingTest.java

@@ -125,12 +125,83 @@ public void theExportersTestsAreNotOnThePath() {
      *
      * <p>It imports tkinter, which does not exist here - so a build that shipped it would fail
      * at import time on a phone rather than at build time on a desktop.
+     *
+     * <p><b>Note this is now a statement about named modules, not about the package.</b> Four
+     * files from {@code exporter/} are packaged deliberately - see below - so "the directory is
+     * absent" stopped being the guarantee and "the interactive parts are absent" took over.
      */
     @Test
     public void theDesktopWizardIsNotPackaged() {
-        assertThrows("python/exporter/ is the tkinter wizard and must not reach the APK",
+        assertThrows("exporter/asyncui.py drives tkinter and must not reach the APK",
                 Exception.class,
                 () -> Python.getInstance().getModule("exporter.asyncui"));
+        assertThrows("exporter/wizard.py is the tkinter window itself",
+                Exception.class,
+                () -> Python.getInstance().getModule("exporter.wizard"));
+        assertThrows("exporter/prompts.py needs prompt_toolkit and a terminal",
+                Exception.class,
+                () -> Python.getInstance().getModule("exporter.prompts"));
+        assertThrows("exporter/cli.py needs questionary",
+                Exception.class,
+                () -> Python.getInstance().getModule("exporter.cli"));
+    }
+
+    /**
+     * The iCloud pipeline imports, which is what lets the app read an account without a zip.
+     *
+     * <p><b>Packaged and importable are different claims, and only the second one matters.</b>
+     * These four modules are named individually in the Chaquopy {@code include} precisely because
+     * their neighbours cannot be imported here - so the thing worth asserting is that pulling
+     * {@code exporter.icloud} in does not transitively reach tkinter, questionary or
+     * prompt_toolkit. A build where it did would fail on a phone, at the moment somebody tried
+     * to sign in, having built cleanly on a desktop.
+     */
+    @Test
+    public void theicloudPipelineIsPackagedAndImports() {
+        for (final String module : new String[]{
+                "exporter", "exporter.icloud", "exporter.device", "exporter.identity"}) {
+            assertNotNull(module + " must be importable in the APK",
+                    Python.getInstance().getModule(module));
+        }
+    }
+
+    /**
+     * And it still knows who the exporter is, so the desktop side is unchanged.
+     *
+     * <p>The identity became a parameter so the app can present its own serial rather than the
+     * exporter's - two programs sharing one are one device to Apple, and removing either from
+     * the device list would break the other. This asserts the default did not move while that
+     * was done.
+     */
+    @Test
+    public void theexporterKeepsItsOwnIdentityByDefault() {
+        final PyObject icloud = Python.getInstance().getModule("exporter.icloud");
+        final PyObject identity = icloud.get("EXPORTER_IDENTITY");
+
+        assertNotNull("exporter.icloud must still expose its default identity", identity);
+        assertEquals("0PENTAGXPORT", identity.get("serial").toString());
+    }
+
+    /**
+     * And the app's own side of it imports, presenting the app's identity rather than the
+     * exporter's.
+     *
+     * <p>The one thing a desktop test suite cannot say. {@code test_icloud_bridge.py} runs the
+     * whole flow against fakes on CPython, which proves the logic and proves nothing about
+     * whether the module survives being packaged - it reaches {@code findmy.cloudkit}, and
+     * therefore protobuf, which is a dependency with a native half and the exact shape of thing
+     * that resolves on a laptop and is missing on a phone.
+     *
+     * <p>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.
+     */
+    @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());
     }
 
     /**

app/src/androidTest/java/dev/wander/android/opentagviewer/util/parse/CustomAccessoryImportTest.java

@@ -17,7 +17,9 @@
 import dev.wander.android.opentagviewer.ui.BeaconIcon;
 import dev.wander.android.opentagviewer.db.repo.model.BeaconData;
 import dev.wander.android.opentagviewer.db.repo.model.ImportData;
+import dev.wander.android.opentagviewer.db.room.entity.BeaconNamingRecord;
 import dev.wander.android.opentagviewer.db.room.entity.OwnedBeacon;
+import dev.wander.android.opentagviewer.db.util.BeaconCombinerUtil;
 import dev.wander.android.opentagviewer.db.room.entity.UserBeaconOptions;
 
 import org.json.JSONObject;
@@ -200,4 +202,85 @@ public void anapplePairedTagStillTakesTheOldPath() {
                 !CustomAccessoryParser.isCustomAccessory(
                         new BeaconData(paired.id, paired, null, null)));
     }
+
+    /**
+     * The one that was missing, and the reason all of the above passed while the feature did not
+     * work.
+     *
+     * <p>Every test before this hands {@link BeaconDataParser} a {@code BeaconData} it built
+     * itself, which skips the step that actually decides what reaches a screen.
+     * {@link BeaconCombinerUtil#combine} used to iterate the <i>naming records</i> - so a tag
+     * with none was dropped on the floor before the parser was ever asked about it. The import
+     * reported "1 device", the fetch path collected reports for it happily, and it appeared
+     * nowhere.
+     *
+     * <p>So this goes through the join, and the two that follow go through it for the two
+     * screens that call it.
+     */
+    @Test
+    public void itsurvivesTheJoinThatFeedsEveryScreen() {
+        final List<BeaconData> joined = BeaconCombinerUtil.combine(imported());
+
+        assertEquals("a tag with no naming record must not be dropped by the join",
+                1, joined.size());
+        assertEquals(IDENTIFIER, joined.get(0).getBeaconId());
+        assertNotNull("and it must keep the row that has its keys in it",
+                joined.get(0).getOwnedBeaconInfo());
+        assertNull("nothing ever named it, so there is nothing to join to",
+                joined.get(0).getBeaconNamingRecord());
+    }
+
+    /**
+     * The device list's own call, which passes user options as a third list.
+     *
+     * <p>A separate case because it is a different overload, and because the options have to
+     * survive the change of what the join iterates - they are keyed by beacon id either way,
+     * but that is worth an assertion rather than an assumption.
+     */
+    @Test
+    public void thedeviceListSeesItToo() {
+        final OwnedBeacon row = imported().getOwnedBeacons().get(0);
+        final UserBeaconOptions chosen = new UserBeaconOptions(
+                row.id, System.currentTimeMillis(), "My hidden bike", "🚲");
+
+        final List<BeaconData> joined = BeaconCombinerUtil.combine(
+                List.of(row), List.of(), List.of(chosen));
+
+        assertEquals(1, joined.size());
+        assertNotNull("the user's own name and emoji must still find it",
+                joined.get(0).getUserBeaconOptions());
+    }
+
+    /** End to end, as the screen does it: import, join, parse, and read the name off it. */
+    @Test
+    public void thewholeChainProducesSomethingToShow() {
+        final BeaconInformation info =
+                BeaconDataParser.parse(BeaconCombinerUtil.combine(imported())).get(0);
+
+        assertEquals(NAME, info.getName());
+        assertTrue(info.isCustomAccessory());
+    }
+
+    /**
+     * And an Apple tag still comes out of the join whole.
+     *
+     * <p>The risk in turning the join around is the mirror of the bug it fixes: driving from the
+     * owned beacons could just as easily leave the naming record behind, which would cost every
+     * real tag its name.
+     */
+    @Test
+    public void anapplePairedTagKeepsItsNamingRecord() {
+        final String id = "2C2A1B0C-9D8E-4F6A-8B4C-3D2E1F0A9B8C";
+        final OwnedBeacon paired = OwnedBeacon.builder().id(id).content("<plist/>").build();
+        final BeaconNamingRecord named =
+                BeaconNamingRecord.builder().id(id).content("<plist/>").build();
+
+        final List<BeaconData> joined =
+                BeaconCombinerUtil.combine(List.of(paired), List.of(named), List.of());
+
+        assertEquals(1, joined.size());
+        assertNotNull(joined.get(0).getOwnedBeaconInfo());
+        assertNotNull("without this an Apple tag loses its name and emoji",
+                joined.get(0).getBeaconNamingRecord());
+    }
 }

app/src/main/java/dev/wander/android/opentagviewer/db/repo/model/BeaconData.java

@@ -11,6 +11,13 @@
 public class BeaconData {
     private final String beaconId;
     private final OwnedBeacon ownedBeaconInfo;
+    /**
+     * What an Apple device wrote about this tag - its name and emoji.
+     *
+     * <p>Null for a tag that was never in an Apple account, because nothing ever named it. The
+     * name for one of those comes out of the accessory JSON instead; see
+     * {@link dev.wander.android.opentagviewer.util.parse.CustomAccessoryParser}.
+     */
     private final BeaconNamingRecord beaconNamingRecord;
     /**
      * Optional, only if configured

app/src/main/java/dev/wander/android/opentagviewer/db/util/BeaconCombinerUtil.java

@@ -18,24 +18,42 @@
 
 @NoArgsConstructor(access = AccessLevel.PRIVATE)
 public final class BeaconCombinerUtil {
+    /**
+     * Join a tag's rows into the shape the UI reads.
+     *
+     * <p><b>Driven by the owned beacons, because that is what a tag is.</b> A
+     * {@link BeaconNamingRecord} is something an Apple device wrote <i>about</i> a tag - a name,
+     * an emoji - and a tag that was never in an Apple account has none, because no iPad ever
+     * named it. Iterating the naming records instead made those tags invisible to every screen
+     * built on this, while the fetch path - which reads owned beacons directly - went on
+     * collecting reports for them perfectly happily. An imported self-generated tag reported
+     * "1 device imported" and then appeared nowhere.
+     *
+     * <p>For an Apple tag nothing changes: the importer inner-joins the two sets, so every owned
+     * beacon has exactly one naming record and vice versa. Driving from this side is in fact the
+     * safer of the two - the old direction could hand out a {@code BeaconData} whose
+     * {@code ownedBeaconInfo} was null, which is the half nothing downstream can work without.
+     *
+     * @return one entry per owned beacon, with a null naming record where there is none.
+     */
     public static List<BeaconData> combine(
             final List<OwnedBeacon> ownedBeacons,
             final List<BeaconNamingRecord> beaconNamingRecords,
             final List<UserBeaconOptions> userBeaconOptions) {
 
-        Map<String, OwnedBeacon> idToBeaconMap = ownedBeacons.stream()
-                .collect(Collectors.toMap((beacon) -> beacon.id, beacon -> beacon));
+        Map<String, BeaconNamingRecord> idToNamingRecordMap = beaconNamingRecords.stream()
+                .collect(Collectors.toMap((namingRec) -> namingRec.id, namingRec -> namingRec));
 
         Map<String, UserBeaconOptions> idToOptionsMap = userBeaconOptions.stream()
                 .collect(Collectors.toMap((options) -> options.beaconId, options -> options));
 
 
-        return beaconNamingRecords.stream()
-                .map(namingRec -> new BeaconData(
-                        namingRec.id,
-                        idToBeaconMap.get(namingRec.id),
-                        namingRec,
-                        idToOptionsMap.getOrDefault(namingRec.id, null)
+        return ownedBeacons.stream()
+                .map(beacon -> new BeaconData(
+                        beacon.id,
+                        beacon,
+                        idToNamingRecordMap.getOrDefault(beacon.id, null),
+                        idToOptionsMap.getOrDefault(beacon.id, null)
                 ))
                 .collect(Collectors.toList());
     }

app/src/main/java/dev/wander/android/opentagviewer/util/parse/BeaconDataParser.java

@@ -7,6 +7,7 @@
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Optional;
 
 import javax.xml.xpath.XPath;
 import javax.xml.xpath.XPathConstants;
@@ -75,7 +76,6 @@ public static List<BeaconInformation> parse(final List<BeaconData> rawBeaconData
                     continue;
                 }
 
-                final String beaconNamingRecordPList = beaconData.getBeaconNamingRecord().content;
                 final String ownedBeaconPList = beaconData.getOwnedBeaconInfo().content;
 
                 // Extract the most relevant fields from decrypted plist files
@@ -87,13 +87,34 @@ public static List<BeaconInformation> parse(final List<BeaconData> rawBeaconData
                 // multiple `<some id>.plist` files per `beacon-identifier`.
                 // In my testing thus far the directory for a single `beacon-identifier`
                 // only contained a single `<some id>.plist` file.
-                final Document beaconNamingData = XmlParser.parse(beaconNamingRecordPList);
-                final String beaconNamingRecordIdentifier = getString(beaconNamingData, identifierQuery);
-                final String associatedBeacon = getString(beaconNamingData, associatedBeaconQuery);
-                final String emoji = getString(beaconNamingData, emojiQuery);
-                final String name = getString(beaconNamingData, nameQuery);
-                // nested PLIST evaluation (this contains some useful info)
-                var metaData = BeaconNamingRecordInnerParser.extractBeaconNamingRecordMetadata(xPath, beaconNamingData);
+                //
+                // **All of it is optional.** A zip always carries the pair, because its importer
+                // inner-joins them, but an account read directly does not have to: CloudKit
+                // holds no naming record for an accessory nobody ever named. That is not a
+                // broken tag and must not be treated as one - it is a tag with no name yet,
+                // which the user can give it like any other. Skipping it here made it look like
+                // the import had lost it.
+                String beaconId = beaconData.getBeaconId();
+                String beaconNamingRecordIdentifier = null;
+                String emoji = null;
+                String name = null;
+                Optional<BeaconNamingRecordCloudKitMetadata> metaData = Optional.empty();
+
+                if (beaconData.getBeaconNamingRecord() != null) {
+                    final Document beaconNamingData =
+                            XmlParser.parse(beaconData.getBeaconNamingRecord().content);
+                    beaconNamingRecordIdentifier = getString(beaconNamingData, identifierQuery);
+                    final String associatedBeacon =
+                            getString(beaconNamingData, associatedBeaconQuery);
+                    if (associatedBeacon != null && !associatedBeacon.isEmpty()) {
+                        beaconId = associatedBeacon;
+                    }
+                    emoji = getString(beaconNamingData, emojiQuery);
+                    name = getString(beaconNamingData, nameQuery);
+                    // nested PLIST evaluation (this contains some useful info)
+                    metaData = BeaconNamingRecordInnerParser.extractBeaconNamingRecordMetadata(
+                            xPath, beaconNamingData);
+                }
 
 
                 // Extract the most relevant fields from decrypted plist files
@@ -120,10 +141,14 @@ public static List<BeaconInformation> parse(final List<BeaconData> rawBeaconData
 
                 var extractedData = BeaconInformation.builder()
                         // BeaconNamingRecord
-                        .beaconId(associatedBeacon)
+                        .beaconId(beaconId)
                         .namingRecordId(beaconNamingRecordIdentifier)
                         .originalEmoji(emoji)
-                        .originalName(name)
+                        // What the tag's own record says it is, when nobody has ever named it -
+                        // "AirTag" rather than a blank row. Not a name pretending to be the
+                        // user's choice: it is what Find My shows for an unnamed accessory,
+                        // and renaming it works exactly as it does for any other tag.
+                        .originalName(name == null || name.isEmpty() ? model : name)
                         // BeaconNamingRecord->cloudKitMetadata
                         .namingRecordModifiedTime(
                                 metaData.map(BeaconNamingRecordCloudKitMetadata::getModifiedTime).orElse(null))