fix(blobs): recover a group-chat avatar whose download URL expired

apps/flipcash/app/build.gradle.kts

@@ -362,4 +362,5 @@ dependencies {
     testImplementation(libs.junit)
     testImplementation(libs.kotlin.test.junit)
     testImplementation(libs.kotlinx.coroutines.test)
+    testImplementation(libs.mockk)
 }

apps/flipcash/app/src/main/kotlin/com/flipcash/app/ImmutableBlobCacheStrategy.kt

@@ -14,26 +14,48 @@ import coil3.request.Options
  * revalidate or re-download once a stored response looks stale, so images we already have on disk
  * still get re-fetched across launches — flashing the BlurHash before the same bytes reappear.
  *
- * Since the bytes are static there is nothing to revalidate: if we have a cached response we serve
- * it unconditionally and never hit the network; on a miss we download once and always persist. This
- * is paired with keying the Coil disk/memory cache on the durable blob id rather than the rotating
- * URL (see `MediaItem.cacheKeyForSize`); without a stable key this strategy would have nothing to
- * hit.
+ * Since the bytes are static there is nothing to revalidate: if we have the bytes we serve them
+ * unconditionally and never hit the network; on a miss we download once and persist. This is paired
+ * with keying the Coil disk/memory cache on the durable blob id rather than the rotating URL (see
+ * `MediaItem.cacheKeyForSize`); without a stable key this strategy would have nothing to hit.
+ *
+ * "Immutable" covers the bytes, not the failures. `NetworkFetcher` writes a response to the disk
+ * cache *before* it throws on the status code, so a fetch through an expired URL leaves a cached 403
+ * under the blob's key — and a strategy that serves any cached response replays that 403 on every
+ * later load without a request, which no amount of re-minting can recover from. So a response that
+ * would make `NetworkFetcher` throw is neither served nor stored.
  */
 internal object ImmutableBlobCacheStrategy : CacheStrategy {
 
-    // Only invoked when a cached response exists — serve it as-is, no revalidation, no network.
+    // Only invoked when a cached response exists — serve the bytes as-is, no revalidation, no
+    // network. A cached failure isn't bytes: fall through to the network so a re-minted URL gets
+    // its chance, which is also what un-poisons an entry an older build already wrote.
     override suspend fun read(
         cacheResponse: NetworkResponse,
         networkRequest: NetworkRequest,
         options: Options,
-    ): CacheStrategy.ReadResult = CacheStrategy.ReadResult(cacheResponse)
+    ): CacheStrategy.ReadResult = if (cacheResponse.isUsable) {
+        CacheStrategy.ReadResult(cacheResponse)
+    } else {
+        CacheStrategy.ReadResult(networkRequest)
+    }
 
-    // Always persist freshly downloaded bytes so the next load is a hit.
+    // Always persist freshly downloaded bytes so the next load is a hit. Declining a failure also
+    // leaves any entry already on disk intact, since the write is what would replace it.
     override suspend fun write(
         cacheResponse: NetworkResponse?,
         networkRequest: NetworkRequest,
         networkResponse: NetworkResponse,
         options: Options,
-    ): CacheStrategy.WriteResult = CacheStrategy.WriteResult(networkResponse)
+    ): CacheStrategy.WriteResult = if (networkResponse.isUsable) {
+        CacheStrategy.WriteResult(networkResponse)
+    } else {
+        CacheStrategy.WriteResult.DISABLED
+    }
+
+    /** Mirrors the status codes `NetworkFetcher` accepts instead of throwing `HttpException`. */
+    private val NetworkResponse.isUsable: Boolean
+        get() = code in 200 until 300 || code == HTTP_NOT_MODIFIED
+
+    private const val HTTP_NOT_MODIFIED = 304
 }

apps/flipcash/app/src/test/kotlin/com/flipcash/app/ImmutableBlobCacheStrategyTest.kt

@@ -0,0 +1,69 @@
+package com.flipcash.app
+
+import android.content.Context
+import coil3.network.NetworkRequest
+import coil3.network.NetworkResponse
+import coil3.request.Options
+import io.mockk.mockk
+import kotlinx.coroutines.test.runTest
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertNull
+import kotlin.test.assertSame
+
+/**
+ * A cached failure is the one thing this strategy must not treat as immutable: `NetworkFetcher`
+ * writes the response to the disk cache before it throws on the status code, and it re-throws
+ * whatever the strategy hands back from the cache without making a request. Serve a stored 403 and
+ * that blob is unloadable for the life of the cache entry, however fresh a download URL it is given.
+ */
+class ImmutableBlobCacheStrategyTest {
+
+    private val request = NetworkRequest(url = "https://cdn.example/images/blob/thumbnail_160x160.webp")
+    private val options = Options(context = mockk<Context>())
+
+    @Test
+    fun `a cached success is served without a request`() = runTest {
+        val cached = NetworkResponse(code = 200)
+
+        val result = ImmutableBlobCacheStrategy.read(cached, request, options)
+
+        assertSame(cached, result.response)
+        assertNull(result.request)
+    }
+
+    @Test
+    fun `a cached failure falls through to the network`() = runTest {
+        for (code in listOf(403, 404, 500)) {
+            val result = ImmutableBlobCacheStrategy.read(NetworkResponse(code = code), request, options)
+
+            assertNull(result.response, "$code was served from the cache")
+            assertSame(request, result.request, "$code did not issue a request")
+        }
+    }
+
+    @Test
+    fun `a success is persisted`() = runTest {
+        val response = NetworkResponse(code = 200)
+
+        val result = ImmutableBlobCacheStrategy.write(null, request, response, options)
+
+        assertSame(response, result.response)
+    }
+
+    @Test
+    fun `a failure is not persisted`() = runTest {
+        for (code in listOf(403, 404, 500)) {
+            val result = ImmutableBlobCacheStrategy.write(null, request, NetworkResponse(code = code), options)
+
+            assertNull(result.response, "$code was written to the cache")
+        }
+    }
+
+    @Test
+    fun `a not modified response is served, matching what the fetcher accepts`() = runTest {
+        val cached = NetworkResponse(code = 304)
+
+        assertEquals(cached, ImmutableBlobCacheStrategy.read(cached, request, options).response)
+    }
+}

services/flipcash/src/main/kotlin/com/flipcash/services/internal/network/api/BlobStorageApi.kt

@@ -4,6 +4,7 @@ import com.codeinc.flipcash.gen.blob.v1.BlobStorageGrpcKt
 import com.codeinc.flipcash.gen.blob.v1.BlobStorageService as RpcBlobStorageService
 import com.codeinc.flipcash.gen.blob.v1.Model
 import com.codeinc.flipcash.gen.blob.v1.validate
+import com.codeinc.flipcash.gen.common.v1.Common
 import com.flipcash.services.internal.annotations.FlipcashManagedChannel
 import com.flipcash.services.internal.network.extensions.asChatId
 import com.flipcash.services.internal.network.extensions.asUserId
@@ -86,34 +87,52 @@ internal class BlobStorageApi @Inject constructor(
         owner: Ed25519.KeyPair,
         context: BlobAccessContext,
     ): RpcBlobStorageService.GetBlobsResponse {
-        val request = RpcBlobStorageService.GetBlobsRequest.newBuilder()
-            .setBlobIds(
-                Model.BlobIdBatch.newBuilder()
-                    .addAllBlobIds(blobIds.map { it.toProto() })
-            )
-            .apply { setAuth(authenticate(owner)) }
-            // Omitted for Owned: the server resolves the caller's own blobs without one, and a
-            // scope the caller can't claim would only narrow the read.
-            .apply { context.toProto()?.let { setContext(it) } }
-            .build()
+        val request = getBlobsRequest(blobIds, context) { authenticate(owner) }
 
         request.validate().orThrow()
 
         return withContext(Dispatchers.IO) {
             api.getBlobs(request)
         }
     }
+}
 
-    private fun BlobId.toProto(): Model.BlobId =
-        Model.BlobId.newBuilder().setValue(bytes.toByteString()).build()
-
-    private fun BlobAccessContext.toProto(): Model.AccessContext? = when (this) {
-        BlobAccessContext.Owned -> null
-        is BlobAccessContext.Profile ->
-            Model.AccessContext.newBuilder().setUserProfile(userId.asUserId()).build()
-        is BlobAccessContext.Chat ->
-            Model.AccessContext.newBuilder().setChat(chatId.asChatId()).build()
-        is BlobAccessContext.ChatProfile ->
-            Model.AccessContext.newBuilder().setChatProfile(chatId.asChatId()).build()
-    }
+/**
+ * Builds a `GetBlobs` request with [context] already in place when [authenticate] runs.
+ *
+ * Order matters and isn't cosmetic: `authenticate` signs the message as built so far, while the
+ * server verifies that signature against the whole request minus the auth field. Anything set
+ * afterwards is in what the server checks and missing from what the client signed, so the call
+ * comes back `UNAUTHENTICATED` — which for an access context means every re-mint of a blob the
+ * caller doesn't own fails, and the avatar it was for stays on its BlurHash.
+ *
+ * [authenticate] is a parameter so the ordering can be asserted without a signing key.
+ */
+internal fun getBlobsRequest(
+    blobIds: List<BlobId>,
+    context: BlobAccessContext,
+    authenticate: RpcBlobStorageService.GetBlobsRequest.Builder.() -> Common.Auth,
+): RpcBlobStorageService.GetBlobsRequest =
+    RpcBlobStorageService.GetBlobsRequest.newBuilder()
+        .setBlobIds(
+            Model.BlobIdBatch.newBuilder()
+                .addAllBlobIds(blobIds.map { it.toProto() })
+        )
+        // Omitted for Owned: the server resolves the caller's own blobs without one, and a
+        // scope the caller can't claim would only narrow the read.
+        .apply { context.toProto()?.let { setContext(it) } }
+        .apply { setAuth(authenticate()) }
+        .build()
+
+private fun BlobId.toProto(): Model.BlobId =
+    Model.BlobId.newBuilder().setValue(bytes.toByteString()).build()
+
+private fun BlobAccessContext.toProto(): Model.AccessContext? = when (this) {
+    BlobAccessContext.Owned -> null
+    is BlobAccessContext.Profile ->
+        Model.AccessContext.newBuilder().setUserProfile(userId.asUserId()).build()
+    is BlobAccessContext.Chat ->
+        Model.AccessContext.newBuilder().setChat(chatId.asChatId()).build()
+    is BlobAccessContext.ChatProfile ->
+        Model.AccessContext.newBuilder().setChatProfile(chatId.asChatId()).build()
 }

services/flipcash/src/main/kotlin/com/flipcash/services/models/chat/BlobId.kt

@@ -2,9 +2,53 @@ package com.flipcash.services.models.chat
 
 import android.os.Parcelable
 import kotlinx.parcelize.Parcelize
+import kotlinx.serialization.ExperimentalSerializationApi
+import kotlinx.serialization.KSerializer
 import kotlinx.serialization.Serializable
+import kotlinx.serialization.builtins.ByteArraySerializer
+import kotlinx.serialization.descriptors.SerialDescriptor
+import kotlinx.serialization.encoding.Decoder
+import kotlinx.serialization.encoding.Encoder
 
+/**
+ * Identifies one immutable set of blob bytes.
+ *
+ * Not a value class, because a value class may not override [equals], and the default over a
+ * `ByteArray` compares array identity. Every read of a persisted [MediaItem] decodes fresh arrays,
+ * so identity equality makes each read a distinct [MediaItem] — and Compose keys off that, so
+ * `remember(media)` drops its state and cancels the effects holding it on every emission, which is
+ * enough to stop a re-minted download URL from ever reaching the image loader.
+ *
+ * [BlobIdSerializer] keeps the wrapper invisible on the wire, which a value class got for free.
+ */
 @Parcelize
-@Serializable
-@JvmInline
-value class BlobId(val bytes: ByteArray): Parcelable
+@Serializable(with = BlobIdSerializer::class)
+data class BlobId(val bytes: ByteArray) : Parcelable {
+
+    override fun equals(other: Any?): Boolean =
+        this === other || (other is BlobId && bytes.contentEquals(other.bytes))
+
+    override fun hashCode(): Int = bytes.contentHashCode()
+}
+
+/**
+ * Encodes a [BlobId] as its bare bytes.
+ *
+ * `MediaItem`s are persisted as JSON (`user_profiles.profile_picture_json`, chat message contents),
+ * and every row written while [BlobId] was a value class holds the id inlined — `"blobId":[1,2,…]`.
+ * The class-shaped default would write `"blobId":{"bytes":[1,2,…]}` and fail to decode those rows,
+ * which drops the whole `MediaItem` and leaves the surface with no image at all.
+ */
+object BlobIdSerializer : KSerializer<BlobId> {
+    private val delegate = ByteArraySerializer()
+
+    @OptIn(ExperimentalSerializationApi::class)
+    override val descriptor: SerialDescriptor =
+        SerialDescriptor("com.flipcash.services.models.chat.BlobId", delegate.descriptor)
+
+    override fun serialize(encoder: Encoder, value: BlobId) =
+        encoder.encodeSerializableValue(delegate, value.bytes)
+
+    override fun deserialize(decoder: Decoder): BlobId =
+        BlobId(decoder.decodeSerializableValue(delegate))
+}

services/flipcash/src/test/kotlin/com/flipcash/services/internal/network/api/GetBlobsRequestSigningTest.kt

@@ -0,0 +1,94 @@
+package com.flipcash.services.internal.network.api
+
+import com.codeinc.flipcash.gen.blob.v1.BlobStorageService as RpcBlobStorageService
+import com.codeinc.flipcash.gen.common.v1.Common
+import com.flipcash.services.models.chat.BlobAccessContext
+import com.flipcash.services.models.chat.BlobId
+import com.flipcash.services.models.chat.ChatId
+import org.junit.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertFalse
+import kotlin.test.assertTrue
+
+/**
+ * `authenticate` signs the request as built so far and the server verifies against the whole
+ * request minus the auth, so every field has to be set before the signature is taken. `GetBlobs`
+ * set its access context afterwards, which made the server reject the call as `UNAUTHENTICATED`:
+ * re-minting another user's expired avatar URL always failed, and group-chat sender avatars stayed
+ * on their BlurHash. These assert on the bytes handed to the signer, so no signing key is needed.
+ */
+class GetBlobsRequestSigningTest {
+
+    private val blobId = BlobId(ByteArray(16) { 7 })
+
+    /** The message the signature is taken over, as the signer sees it. */
+    private fun signedBytes(
+        context: BlobAccessContext,
+    ): RpcBlobStorageService.GetBlobsRequest {
+        var signed: ByteArray? = null
+        getBlobsRequest(listOf(blobId), context) {
+            signed = buildPartial().toByteArray()
+            Common.Auth.getDefaultInstance()
+        }
+        return RpcBlobStorageService.GetBlobsRequest.parseFrom(
+            requireNonNull(signed)
+        )
+    }
+
+    private fun requireNonNull(bytes: ByteArray?): ByteArray =
+        bytes ?: error("the authenticator was never invoked")
+
+    @Test
+    fun `a profile context is signed`() {
+        val userId = List<Byte>(16) { 2 }
+
+        val signed = signedBytes(BlobAccessContext.Profile(userId))
+
+        assertTrue(signed.hasContext())
+        assertEquals(userId, signed.context.userProfile.value.toByteArray().toList())
+    }
+
+    @Test
+    fun `a chat context is signed`() {
+        val chatId = ChatId(ByteArray(32) { 1 })
+
+        val signed = signedBytes(BlobAccessContext.Chat(chatId))
+
+        assertTrue(signed.hasContext())
+        assertEquals(chatId.bytes.toList(), signed.context.chat.value.toByteArray().toList())
+    }
+
+    @Test
+    fun `a chat profile context is signed`() {
+        val chatId = ChatId(ByteArray(32) { 1 })
+
+        val signed = signedBytes(BlobAccessContext.ChatProfile(chatId))
+
+        assertTrue(signed.hasContext())
+        assertEquals(chatId.bytes.toList(), signed.context.chatProfile.value.toByteArray().toList())
+    }
+
+    @Test
+    fun `the blob ids are signed`() {
+        val signed = signedBytes(BlobAccessContext.Owned)
+
+        assertEquals(
+            listOf(blobId.bytes.toList()),
+            signed.blobIds.blobIdsList.map { it.value.toByteArray().toList() },
+        )
+    }
+
+    @Test
+    fun `an owned read signs no context`() {
+        val signed = signedBytes(BlobAccessContext.Owned)
+
+        assertFalse(signed.hasContext())
+    }
+
+    @Test
+    fun `the auth is not part of what is signed`() {
+        val signed = signedBytes(BlobAccessContext.Profile(List<Byte>(16) { 2 }))
+
+        assertFalse(signed.hasAuth())
+    }
+}