diff --git a/app/src/main/java/app/closer/data/remote/FirestoreCoupleDataSource.kt b/app/src/main/java/app/closer/data/remote/FirestoreCoupleDataSource.kt index dda0e764..aca67e0b 100644 --- a/app/src/main/java/app/closer/data/remote/FirestoreCoupleDataSource.kt +++ b/app/src/main/java/app/closer/data/remote/FirestoreCoupleDataSource.kt @@ -98,6 +98,20 @@ class FirestoreCoupleDataSource @Inject constructor( return if (snap.exists()) snap.toCouple() else null } + /** + * The couple, straight from the server — never the local cache. + * + * [getCoupleById] is `Source.DEFAULT`, which can answer from cache. That is fine for rendering and + * wrong for deciding the next `keyGeneration`: a stale generation makes the rotation write the + * value the document ALREADY has, which is not a change, so no partner ever adopts the new key + * (C-ROTATE-001 — it stranded a real fixture partner on 🔒 before this existed). Throws when the + * server can't be reached, which is the point: rotation must fail loudly rather than half-happen. + */ + suspend fun getCoupleByIdFromServer(coupleId: String): Couple? { + val snap = coupleRef(coupleId).get(com.google.firebase.firestore.Source.SERVER).await() + return if (snap.exists()) snap.toCouple() else null + } + suspend fun updateStreak(coupleId: String) { val snap = coupleRef(coupleId).get().await() val lastAnsweredAt = snap.millisOrNull("lastAnsweredAt") ?: 0L diff --git a/app/src/main/java/app/closer/data/repository/CoupleRepositoryImpl.kt b/app/src/main/java/app/closer/data/repository/CoupleRepositoryImpl.kt index df006666..ee20cdc4 100644 --- a/app/src/main/java/app/closer/data/repository/CoupleRepositoryImpl.kt +++ b/app/src/main/java/app/closer/data/repository/CoupleRepositoryImpl.kt @@ -64,9 +64,12 @@ class CoupleRepositoryImpl @Inject constructor( } override suspend fun rotateCoupleKey(coupleId: String): Result = runCatching { - // Generation truth lives on the couple doc — read it fresh so concurrent rotations collide - // at the rules' strictly-increasing check instead of silently overwriting each other. - val couple = coupleDataSource.getCoupleById(coupleId) ?: error("couple not found") + // Generation truth lives on the couple doc, and it MUST come from the server: a cached + // generation makes us write back the value already there, which Firestore doesn't count as a + // change — the rule passes, no trigger fires, and the partner never learns to adopt. That + // stranded a real partner on 🔒 (C-ROTATE-001). Offline throws here, which is correct: + // nothing is written and the user is told nothing changed. + val couple = coupleDataSource.getCoupleByIdFromServer(coupleId) ?: error("couple not found") val prepared = encryptionManager.prepareRotation(coupleId, couple.keyGeneration).getOrElse { throw it } // Server first: if this write fails, this device stays coherent on the old key and nothing // anywhere has changed. If the process dies right after it, this device self-heals through diff --git a/app/src/test/java/app/closer/data/repository/CoupleKeyRotationRepositoryTest.kt b/app/src/test/java/app/closer/data/repository/CoupleKeyRotationRepositoryTest.kt new file mode 100644 index 00000000..aa1997fc --- /dev/null +++ b/app/src/test/java/app/closer/data/repository/CoupleKeyRotationRepositoryTest.kt @@ -0,0 +1,106 @@ +package app.closer.data.repository + +import app.closer.core.crash.CrashReporter +import app.closer.crypto.CoupleEncryptionManager +import app.closer.crypto.RecoveryKeyManager +import app.closer.data.remote.FirestoreCoupleDataSource +import app.closer.data.remote.FirestoreUserDataSource +import app.closer.domain.model.Couple +import com.google.crypto.tink.KeysetHandle +import io.mockk.Runs +import io.mockk.coEvery +import io.mockk.coVerify +import io.mockk.just +import io.mockk.mockk +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * C-ROTATE-001, caught live: a rotation that doesn't ADVANCE `keyGeneration` strands the partner. + * + * The generation is the only signal the partner's device has to adopt the new key. Reading it from + * cache produced a stale value, so the rotation wrote back the number already on the document — + * Firestore doesn't count an unchanged value as a change, so the rules passed, no trigger fired, and + * the partner sat on "🔒 Couldn't unlock on this device" for every new message, permanently. It looked + * exactly like success from the rotating device. + * + * These pin the two properties that make that impossible: the generation comes from the SERVER, and + * the published generation is strictly greater than the one we read. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class CoupleKeyRotationRepositoryTest { + + private val coupleDataSource: FirestoreCoupleDataSource = mockk() + private val userDataSource: FirestoreUserDataSource = mockk() + private val encryptionManager: CoupleEncryptionManager = mockk() + private val crashReporter: CrashReporter = mockk(relaxUnitFun = true) + + private val repo = CoupleRepositoryImpl(coupleDataSource, userDataSource, encryptionManager, crashReporter) + + private val coupleId = "c1" + private val wrapped = RecoveryKeyManager.WrappedKey(cipherB64 = "cipher", saltB64 = "salt", params = "p") + + private fun prepared(newGeneration: Long) = CoupleEncryptionManager.PreparedRotation( + rotatedHandle = mockk(), + wrapped = wrapped, + newGeneration = newGeneration + ) + + @Test + fun `the generation comes from the server, never the cache`() = runTest { + // The cache is what lied. If rotation ever reads getCoupleById again, this fails. + coEvery { coupleDataSource.getCoupleByIdFromServer(coupleId) } returns Couple(id = coupleId, keyGeneration = 1L) + coEvery { encryptionManager.prepareRotation(coupleId, 1L) } returns Result.success(prepared(2L)) + coEvery { coupleDataSource.rotateWrappedKey(any(), any(), any()) } just Runs + every2() + + assertTrue(repo.rotateCoupleKey(coupleId).isSuccess) + + coVerify(exactly = 0) { coupleDataSource.getCoupleById(any()) } + coVerify { coupleDataSource.getCoupleByIdFromServer(coupleId) } + } + + @Test + fun `the published generation strictly advances past the server's`() = runTest { + coEvery { coupleDataSource.getCoupleByIdFromServer(coupleId) } returns Couple(id = coupleId, keyGeneration = 7L) + coEvery { encryptionManager.prepareRotation(coupleId, 7L) } returns Result.success(prepared(8L)) + coEvery { coupleDataSource.rotateWrappedKey(any(), any(), any()) } just Runs + every2() + + assertTrue(repo.rotateCoupleKey(coupleId).isSuccess) + + // 8 > 7: a real change, so the trigger fires and the partner adopts. Writing 7 here is the bug. + coVerify { coupleDataSource.rotateWrappedKey(coupleId, wrapped, 8L) } + } + + @Test + fun `server write happens before the local commit`() = runTest { + // Ordering IS the error handling: a failed write must leave this device on the old key. + coEvery { coupleDataSource.getCoupleByIdFromServer(coupleId) } returns Couple(id = coupleId, keyGeneration = 0L) + coEvery { encryptionManager.prepareRotation(coupleId, 0L) } returns Result.success(prepared(1L)) + coEvery { coupleDataSource.rotateWrappedKey(any(), any(), any()) } throws RuntimeException("rules rejected") + + val result = repo.rotateCoupleKey(coupleId) + + assertTrue(result.isFailure) + coVerify(exactly = 0) { encryptionManager.commitRotation(any(), any()) } + } + + @Test + fun `an offline server read fails the rotation instead of guessing`() = runTest { + coEvery { coupleDataSource.getCoupleByIdFromServer(coupleId) } throws RuntimeException("unavailable") + + val result = repo.rotateCoupleKey(coupleId) + + assertTrue(result.isFailure) + coVerify(exactly = 0) { coupleDataSource.rotateWrappedKey(any(), any(), any()) } + coVerify(exactly = 0) { encryptionManager.commitRotation(any(), any()) } + } + + /** commitRotation is a plain (non-suspend) store; relax it for the success paths. */ + private fun every2() { + io.mockk.every { encryptionManager.commitRotation(any(), any()) } just Runs + } +} diff --git a/firestore.rules b/firestore.rules index aab66599..e2779a51 100644 --- a/firestore.rules +++ b/firestore.rules @@ -182,9 +182,19 @@ service cloud.firestore { && request.resource.data.diff(resource.data).affectedKeys().hasOnly([ 'wrappedCoupleKey', 'kdfSalt', 'kdfParams', 'keyGeneration' ]) - // Key rotation bumps keyGeneration strictly upward alongside the new wrap; a plain - // phrase re-wrap doesn't touch it. Monotonic like encryptionVersion: never downgradeable, - // so a replayed/stale rotation write can't roll the couple back to an older wrap. + // A CHANGED wrap is a key rotation, and a rotation MUST advance keyGeneration: that bump is + // the only signal the partner's device has to adopt the new key. Publishing a new wrap while + // leaving the generation alone silently strands the partner on 🔒 forever — so the rules make + // it impossible rather than trusting every client to compute the next generation correctly + // (C-ROTATE-001: a cached read did exactly that). Note an unchanged value is absent from + // affectedKeys(), which is what let the stale write through before. + && ( + !request.resource.data.diff(resource.data).affectedKeys().hasAny(['wrappedCoupleKey']) + || (request.resource.data.keyGeneration is int + && request.resource.data.keyGeneration > resource.data.get('keyGeneration', 0)) + ) + // Monotonic like encryptionVersion: never downgradeable, so a replayed or stale rotation + // write can't roll the couple back to an older wrap. && ( !request.resource.data.diff(resource.data).affectedKeys().hasAny(['keyGeneration']) || (request.resource.data.keyGeneration is int