Pairing survives a project whose rules are older than the app

README.md

@@ -360,7 +360,10 @@ You'll need [Android Studio](https://developer.android.com/studio)
    them into the console's Rules tab, or install the
    [Firebase CLI](https://firebase.google.com/docs/cli) and run
    `firebase deploy --only firestore:rules` from a directory containing a
-   `firebase.json` pointing at that file.
+   `firebase.json` pointing at that file. **Re-publish them whenever
+   `firebase/firestore.rules` changes in this repo** - the apps and the rules
+   are one design, and a build that writes something the deployed rules don't
+   know about yet is refused (pairing is the first place you'd notice).
 6. Open the project root in Android Studio and let it sync. Run the `kid`
    configuration on one device/emulator and `parent` on another (or the
    same emulator with two profiles) to test pairing end-to-end.

firebase/firestore.rules

@@ -3,8 +3,8 @@ rules_version = '2';
 // Data model:
 //   parents/{parentUid}                                  - one doc per parent account
 //   parents/{parentUid}/linkedDevices/{deviceUid}         - a kid device that claimed a pairing code for
-//                                                           this parent (#18, #39); created only in the
-//                                                           same transaction as that claim
+//                                                           this parent (#18, #39); writable only by the
+//                                                           device that already claimed that code
 //   parents/{parentUid}/children/{childId}                - a child profile + limits;
 //                                                           the parent's own self-tracked
 //                                                           profile (#8) always lives at
@@ -98,9 +98,11 @@ service cloud.firestore {
 
       // Which kid devices are linked to this parent (so they can read the parent's self-tracked stats if
       // the parent opts in - #18, visible by default, not a separate opt-in). One doc per device, keyed by
-      // its uid. A device can only create ITS OWN doc, and only in the same transaction that claims a
-      // live pairing code belonging to THIS parent (getAfter sees the post-transaction code doc), so
-      // nobody can link themselves to a family they haven't paired with. Only the parent can read or
+      // its uid. A device can only create ITS OWN doc, and only while naming a pairing code that it has
+      // itself claimed from THIS parent, so nobody can link themselves to a family they haven't paired
+      // with. (getAfter, not get, so this also holds when the write is part of a batch or transaction;
+      // the client writes it just after the claim, deliberately NOT inside it - a refusal here must
+      // never fail the pairing itself, which is what it did before.) Only the parent can read or
       // remove them. (The parent doc itself is readable only by the parent - the passcode hash/salt live
       // there - and get()/exists() calls in these rules bypass permissions without exposing contents.)
       match /linkedDevices/{deviceUid} {
@@ -119,7 +121,7 @@ service cloud.firestore {
         allow read, write: if request.auth != null && request.auth.uid == parentUid;
 
         // The paired kid device may read its own child doc (to pick up limit changes).
-        allow get: if request.auth != null && resource.data.deviceUid == request.auth.uid;
+        allow get: if request.auth != null && resource.data.get('deviceUid', '') == request.auth.uid;
 
         // Any device linked to this parent (see linkedDeviceUids above) may also read
         // the parent's own self-tracked profile - see #18. isSelf profiles always live
@@ -133,8 +135,14 @@ service cloud.firestore {
         // may change, and only as part of the same transaction that claims this child's own
         // live pairing code (getAfter sees the post-transaction code doc). So a stranger
         // can't claim an unpaired child without its unexpired code.
+        //
+        // get('deviceUid', null), not resource.data.deviceUid: reading a field that ISN'T
+        // THERE is an evaluation error, which denies the claim outright. A never-paired
+        // child written without the key at all (the iOS parent app used to drop nil fields)
+        // would otherwise be impossible to pair, with no way to tell that apart from a
+        // genuinely refused claim.
         allow update: if request.auth != null
-          && resource.data.deviceUid == null
+          && resource.data.get('deviceUid', null) == null
           && request.resource.data.deviceUid == request.auth.uid
           && request.resource.data.diff(resource.data).affectedKeys().hasOnly(['deviceUid', 'paired'])
           && getAfter(/databases/$(database)/documents/pairingCodes/$(resource.data.pairingCode)).data.claimedByUid == request.auth.uid

ios/Sources/OpenScreenTimeParentKit/Models/ChildProfile.swift

@@ -81,19 +81,28 @@ struct ChildProfile: Identifiable, Equatable {
             "showNotificationsOnKid": showNotificationsOnKid,
             "trackWebsites": trackWebsites
         ]
-        map["deviceUid"] = deviceUid
-        map["parentPasscodeHash"] = parentPasscodeHash
-        map["parentPasscodeSalt"] = parentPasscodeSalt
-        map["dailyUnlockGoal"] = dailyUnlockGoal
-        map["proposedDailyLimitMinutes"] = proposedDailyLimitMinutes
-        map["proposedAppLimits"] = proposedAppLimits
-        map["bedtimeStartMinutes"] = bedtimeStartMinutes
-        map["bedtimeEndMinutes"] = bedtimeEndMinutes
-        map["requestedExtraMinutes"] = requestedExtraMinutes
-        map["temporaryUnlockUntilMs"] = temporaryUnlockUntilMs
+        // Optional fields, written as real nulls rather than left out entirely - see orNull.
+        map["deviceUid"] = Self.orNull(deviceUid)
+        map["parentPasscodeHash"] = Self.orNull(parentPasscodeHash)
+        map["parentPasscodeSalt"] = Self.orNull(parentPasscodeSalt)
+        map["dailyUnlockGoal"] = Self.orNull(dailyUnlockGoal)
+        map["proposedDailyLimitMinutes"] = Self.orNull(proposedDailyLimitMinutes)
+        map["proposedAppLimits"] = Self.orNull(proposedAppLimits)
+        map["bedtimeStartMinutes"] = Self.orNull(bedtimeStartMinutes)
+        map["bedtimeEndMinutes"] = Self.orNull(bedtimeEndMinutes)
+        map["requestedExtraMinutes"] = Self.orNull(requestedExtraMinutes)
+        map["temporaryUnlockUntilMs"] = Self.orNull(temporaryUnlockUntilMs)
         return map
     }
 
+    /// A real Firestore null for an unset field, rather than no field at all. Assigning nil to a
+    /// dictionary subscript REMOVES the key, and a missing field is an evaluation error in the
+    /// security rules, not null - a child written without "deviceUid" could never be paired (see
+    /// the one-time claim rule in firebase/firestore.rules). The Android app writes nulls here too.
+    private static func orNull(_ value: Any?) -> Any {
+        value ?? NSNull()
+    }
+
     static func from(id: String, map: [String: Any]) -> ChildProfile {
         ChildProfile(
             id: id,

shared/src/androidTest/java/org/openscreentime/shared/PairingFlowEmulatorTest.kt

@@ -5,6 +5,7 @@ import androidx.test.platform.app.InstrumentationRegistry
 import com.google.firebase.FirebaseApp
 import com.google.firebase.FirebaseOptions
 import com.google.firebase.auth.FirebaseAuth
+import com.google.firebase.firestore.FieldValue
 import com.google.firebase.firestore.FirebaseFirestore
 import kotlinx.coroutines.async
 import kotlinx.coroutines.awaitAll
@@ -110,6 +111,26 @@ class PairingFlowEmulatorTest {
         assertEquals("The child should now point at the new code", newCode, claimedChild.pairingCode)
     }
 
+    @Test(timeout = TEST_TIMEOUT_MS)
+    fun childDocWithNoDeviceUidFieldCanStillBeClaimed() = runBlocking {
+        val parentRepo = FamilyRepository()
+        val parentUid = parentRepo.signUpParent(uniqueEmail(), "testpass123")
+        val child = parentRepo.createChild(parentUid, "LegacyChild")
+
+        // The iOS parent app used to drop nil fields instead of writing nulls, so a child could
+        // exist with no deviceUid key at all. A field that ISN'T THERE is an evaluation error in
+        // the security rules, not null, so every claim on such a child was refused - and from the
+        // kid's side that looks exactly like a dead code. Reproduce that shape of document here.
+        FirebaseFirestore.getInstance()
+            .document(FirestorePaths.childDoc(parentUid, child.id))
+            .update("deviceUid", FieldValue.delete()).await()
+        parentRepo.signOut()
+
+        val kidRepo = FamilyRepository()
+        val (_, claimed) = kidRepo.claimPairingCode(child.pairingCode)
+        assertTrue("A child written without a deviceUid field must still be pairable", claimed.paired)
+    }
+
     @Test(timeout = TEST_TIMEOUT_MS)
     fun pairingCodeCannotBeClaimedTwice() = runBlocking {
         val parentRepo = FamilyRepository()

shared/src/main/java/org/openscreentime/shared/repo/PairingRepository.kt

@@ -17,43 +17,47 @@ internal class PairingRepository(
         val uid = session.signInAnonymously()
         val codeRef = db.collection(FirestorePaths.PAIRING_CODES).document(code)
 
+        // Only the two writes that ARE the pairing go in the transaction: stamp this device
+        // onto the child, and mark the code used. Anything else in here - however small -
+        // fails the whole claim if the rules refuse it, which is how pairing broke before:
+        // the linkedDevices write below was part of this transaction, so on a project whose
+        // deployed firestore.rules predated that collection (#39), every claim was refused.
         val (parentUid, childId) = try {
             db.runTransaction { txn ->
-            val codeSnap = txn.get(codeRef)
-            require(codeSnap.exists()) { "That code doesn't match any account." }
-            val used = codeSnap.getBoolean("used") ?: false
-            require(!used) { "That code has already been used." }
-            val createdAt = codeSnap.getTimestamp("createdAt")
-            val ageMs = createdAt?.let { Timestamp.now().seconds - it.seconds } ?: 0
-            // Mirrors the security rule's own TTL check (see firestore.rules) so a
-            // stale code fails with a clear message instead of a raw permission error.
-            require(ageMs < FamilyRepository.PAIRING_CODE_TTL_SECONDS) { "That code has expired. Ask the parent for a new one." }
-            val parentUid = codeSnap.getString("parentUid")!!
-            val childId = codeSnap.getString("childId")!!
+                val codeSnap = txn.get(codeRef)
+                require(codeSnap.exists()) { "That code doesn't match any account." }
+                val used = codeSnap.getBoolean("used") ?: false
+                require(!used) { "That code has already been used." }
+                val createdAt = codeSnap.getTimestamp("createdAt")
+                val ageMs = createdAt?.let { Timestamp.now().seconds - it.seconds } ?: 0
+                // Mirrors the security rule's own TTL check (see firestore.rules) so a
+                // stale code fails with a clear message instead of a raw permission error.
+                require(ageMs < FamilyRepository.PAIRING_CODE_TTL_SECONDS) { "That code has expired. Ask the parent for a new one." }
+                val parentUid = codeSnap.getString("parentUid")!!
+                val childId = codeSnap.getString("childId")!!
 
-            val childRef = db.document(FirestorePaths.childDoc(parentUid, childId))
-            txn.update(childRef, mapOf("deviceUid" to uid, "paired" to true))
-            txn.update(codeRef, mapOf("used" to true, "claimedByUid" to uid))
-            // Lets this device later read the parent's self-tracked stats, if the parent has opted into
-            // self-tracking (see #18). Its own doc under the parent, created in this same transaction so
-            // the security rules can check it against the code being claimed (see #39) - it used to be an
-            // array on the parent doc that any signed-in user could add themselves to.
-            val linkedRef = db.document(FirestorePaths.linkedDeviceDoc(parentUid, uid))
-            txn.set(linkedRef, mapOf("code" to code, "createdAt" to FieldValue.serverTimestamp()))
-            parentUid to childId
+                val childRef = db.document(FirestorePaths.childDoc(parentUid, childId))
+                txn.update(childRef, mapOf("deviceUid" to uid, "paired" to true))
+                txn.update(codeRef, mapOf("used" to true, "claimedByUid" to uid))
+                parentUid to childId
             }.await()
         } catch (e: FirebaseFirestoreException) {
             // Expired, used, or nonexistent codes aren't readable at all (see firestore.rules),
             // so they surface as a permission error rather than one of the messages above.
             if (e.code == FirebaseFirestoreException.Code.PERMISSION_DENIED) {
-                // Say which step Firebase refused, so a tester (and we) can tell "no such live code" from "the pairing
-                // write itself was refused" - both otherwise look like the same permission error.
+                // Say which step Firebase refused, so a tester (and we) can tell "no such live code" from
+                // "the claim itself was refused" - both otherwise look like the same permission error.
+                // Read-only: nothing here writes, so a failed attempt never burns a good code.
                 val project = runCatching { FirebaseApp.getInstance().options.projectId }.getOrNull() ?: "unknown"
                 val codeVisible = runCatching { codeRef.get().await().exists() }.getOrNull()
-                val step = if (codeVisible == true) {
-                    "the code was found, but linking this phone was refused"
-                } else {
+                val step = if (codeVisible != true) {
                     "this phone could not read that code (it isn't there, is used, or ran out)"
+                } else {
+                    // The code is live, so the refusal was the child profile itself. The kid device can't
+                    // read that profile to say which, so name both causes a parent can actually act on.
+                    "the code is live, but this phone wasn't allowed to link to that child - " +
+                        "either it was already paired with another phone (remove it and add it again), " +
+                        "or this project's security rules are older than the app (deploy firebase/firestore.rules)"
                 }
                 throw IllegalStateException(
                     "That code isn't active. Ask the parent for a new one. (Project $project: $step.)"
@@ -62,6 +66,16 @@ internal class PairingRepository(
             throw e
         }
 
+        // Lets this device later read the parent's self-tracked stats, if the parent has opted into self-tracking
+        // (see #18). Its own doc under the parent; the security rules still check it against the code just claimed
+        // (#39) - getAfter on that code doc sees it already marked used by this uid, so the binding holds just as
+        // well outside the transaction. Deliberately best-effort: if it's refused, the phone is paired and works,
+        // it just can't read the parent's own stats.
+        runCatching {
+            db.document(FirestorePaths.linkedDeviceDoc(parentUid, uid))
+                .set(mapOf("code" to code, "createdAt" to FieldValue.serverTimestamp())).await()
+        }
+
         val childSnap = db.document(FirestorePaths.childDoc(parentUid, childId)).get().await()
         val child = ChildProfile.fromMap(childId, childSnap.data ?: emptyMap())
         return parentUid to child