diff --git a/apps/flipcash/app/build.gradle.kts b/apps/flipcash/app/build.gradle.kts index d535a63985..7080336941 100644 --- a/apps/flipcash/app/build.gradle.kts +++ b/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) } diff --git a/apps/flipcash/app/src/main/kotlin/com/flipcash/app/ImmutableBlobCacheStrategy.kt b/apps/flipcash/app/src/main/kotlin/com/flipcash/app/ImmutableBlobCacheStrategy.kt index beddc145c6..930a3ab5ac 100644 --- a/apps/flipcash/app/src/main/kotlin/com/flipcash/app/ImmutableBlobCacheStrategy.kt +++ b/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 } diff --git a/apps/flipcash/app/src/test/kotlin/com/flipcash/app/ImmutableBlobCacheStrategyTest.kt b/apps/flipcash/app/src/test/kotlin/com/flipcash/app/ImmutableBlobCacheStrategyTest.kt new file mode 100644 index 0000000000..ae12f66b5e --- /dev/null +++ b/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()) + + @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) + } +} diff --git a/services/flipcash/src/main/kotlin/com/flipcash/services/internal/network/api/BlobStorageApi.kt b/services/flipcash/src/main/kotlin/com/flipcash/services/internal/network/api/BlobStorageApi.kt index 969dfbeef9..f8740c37e6 100644 --- a/services/flipcash/src/main/kotlin/com/flipcash/services/internal/network/api/BlobStorageApi.kt +++ b/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,16 +87,7 @@ 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() @@ -103,17 +95,44 @@ internal class BlobStorageApi @Inject constructor( 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, + 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() } diff --git a/services/flipcash/src/main/kotlin/com/flipcash/services/models/chat/BlobId.kt b/services/flipcash/src/main/kotlin/com/flipcash/services/models/chat/BlobId.kt index ee2c70bd61..4e33312965 100644 --- a/services/flipcash/src/main/kotlin/com/flipcash/services/models/chat/BlobId.kt +++ b/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 { + 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)) +} diff --git a/services/flipcash/src/test/kotlin/com/flipcash/services/internal/network/api/GetBlobsRequestSigningTest.kt b/services/flipcash/src/test/kotlin/com/flipcash/services/internal/network/api/GetBlobsRequestSigningTest.kt new file mode 100644 index 0000000000..42c5b89e0f --- /dev/null +++ b/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(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(16) { 2 })) + + assertFalse(signed.hasAuth()) + } +} diff --git a/services/flipcash/src/test/kotlin/com/flipcash/services/models/chat/BlobIdEqualityTest.kt b/services/flipcash/src/test/kotlin/com/flipcash/services/models/chat/BlobIdEqualityTest.kt new file mode 100644 index 0000000000..49f93c41a2 --- /dev/null +++ b/services/flipcash/src/test/kotlin/com/flipcash/services/models/chat/BlobIdEqualityTest.kt @@ -0,0 +1,56 @@ +package com.flipcash.services.models.chat + +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNotEquals + +/** + * A blob id identifies bytes, so two ids over the same bytes are the same id however they were + * built. Every read of a persisted [MediaItem] decodes fresh arrays, so an id that compared by + * array identity would make each read a different [MediaItem] — and `remember(media)` would + * discard its state, and any in-flight work keyed on it, on every emission. + */ +class BlobIdEqualityTest { + + private val bytes = byteArrayOf(1, 2, 3, 4) + + @Test + fun `ids over equal bytes are equal`() { + assertEquals(BlobId(bytes.copyOf()), BlobId(bytes.copyOf())) + } + + @Test + fun `equal ids hash alike`() { + assertEquals(BlobId(bytes.copyOf()).hashCode(), BlobId(bytes.copyOf()).hashCode()) + } + + @Test + fun `ids over different bytes are not equal`() { + assertNotEquals(BlobId(bytes), BlobId(byteArrayOf(4, 3, 2, 1))) + } + + @Test + fun `ids survive a set, so a batch of blob ids deduplicates`() { + assertEquals(1, setOf(BlobId(bytes.copyOf()), BlobId(bytes.copyOf())).size) + } + + @Test + fun `media items decoded separately from the same bytes are equal`() { + fun item() = MediaItem( + renditions = listOf( + MediaItemRendition( + role = MediaItemRendition.Role.THUMBNAIL, + blobId = BlobId(bytes.copyOf()), + blob = BlobMetadata( + mimeType = "image/webp", + sizeBytes = 1, + downloadUrl = "https://cdn/thumb", + image = ImageMetadata(width = 160, height = 160, blurhash = "abc"), + ), + ) + ) + ) + + assertEquals(item(), item()) + } +} diff --git a/services/flipcash/src/test/kotlin/com/flipcash/services/models/chat/BlobIdSerializationTest.kt b/services/flipcash/src/test/kotlin/com/flipcash/services/models/chat/BlobIdSerializationTest.kt new file mode 100644 index 0000000000..ad63227885 --- /dev/null +++ b/services/flipcash/src/test/kotlin/com/flipcash/services/models/chat/BlobIdSerializationTest.kt @@ -0,0 +1,52 @@ +package com.flipcash.services.models.chat + +import kotlinx.serialization.json.Json +import kotlin.test.Test +import kotlin.test.assertContains +import kotlin.test.assertEquals + +/** + * A blob id is persisted inside `MediaItem` JSON — `user_profiles.profile_picture_json` and chat + * message contents — so its encoding is a storage format, not an implementation detail. Rows + * written while the id was a value class hold it inlined; a build that can't read those drops the + * whole `MediaItem` and the surface loses its image entirely. + */ +class BlobIdSerializationTest { + + private val json = Json { ignoreUnknownKeys = true } + + private val persistedByValueClassBuild = """ + {"renditions":[{"role":"ORIGINAL","blobId":[1,2,3,4],"blob":null}]} + """.trimIndent() + + @Test + fun `a media item persisted by an older build still decodes`() { + val decoded = json.decodeFromString(persistedByValueClassBuild) + + assertEquals(BlobId(byteArrayOf(1, 2, 3, 4)), decoded.renditions.single().blobId) + } + + @Test + fun `an id encodes as bare bytes, so an older build can still read it back`() { + val encoded = json.encodeToString( + MediaItem( + renditions = listOf( + MediaItemRendition( + role = MediaItemRendition.Role.ORIGINAL, + blobId = BlobId(byteArrayOf(1, 2, 3, 4)), + blob = null, + ), + ), + ), + ) + + assertContains(encoded, """"blobId":[1,2,3,4]""") + } + + @Test + fun `an id survives a round trip`() { + val id = BlobId(byteArrayOf(9, 8, 7)) + + assertEquals(id, json.decodeFromString(json.encodeToString(id))) + } +}