From f22009c6980f1820d394dfc576f6ff643dde3085 Mon Sep 17 00:00:00 2001 From: Brandon McAnsh Date: Thu, 3 Sep 2026 20:15:19 -0400 Subject: [PATCH 1/3] fix(blobs): re-mint expired download URLs before loading them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Profile photos showed the grey placeholder in the chat header and on the user profile screen while the same photo still rendered in the chats list. The list had just been refreshed from GetDmChatFeed; the other two surfaces read a profile persisted earlier, whose CloudFront download URL — signed for about 15 minutes — had long since expired. Coil draws the error drawable on a failed load, so the placeholder was the 403. BlobMetadata now carries download_url.expires_at through the proto mapping and into the persisted JSON, so a stored copy can tell that its URL is dead. MediaUrlResolver checks that before handing a URL to Coil and calls GetBlobs to re-mint the id, memoising the result for the process so every surface showing the same avatar re-mints once. A load that fails anyway gets one further re-mint from the error callback before giving up. Re-minting a blob the caller does not own needs GetBlobsRequest.context to name what authorizes the read, so BlobAccessContext threads a scope (owned, a profile id, or a chat id) from each avatar call site down to the API. Several call sites had no user id to pass: the chat member -> UserProfile mapping dropped it in both mapping paths, leaving the nested profile's userId null even though the server sets it on the member. Avatars draw the BlurHash while a re-mint is in flight and when a load fails outright, rather than the grey person glyph. Coil requests are keyed on the durable blob id instead of the per-fetch URL, so a re-mint reuses bytes that are already cached; notification avatars use the same key and now go through the resolver too. --- .../kotlin/com/flipcash/app/MainActivity.kt | 6 + .../app/internal/ui/AppNavigationBar.kt | 11 +- .../app/core/media/ImageCachePresence.kt | 45 +++++ .../app/core/media/MediaUrlResolver.kt | 107 +++++++++++ .../app/core/media/RememberMediaUrl.kt | 85 +++++++++ .../app/core/media/MediaUrlResolverTest.kt | 173 ++++++++++++++++++ .../screens/components/ParticipantAvatar.kt | 9 +- .../blocklist/BlocklistScreenContent.kt | 2 + .../userprofile/UserProfileScreenContent.kt | 3 + .../tipping/internal/components/TipChatRow.kt | 2 + .../internal/photo/PhotoSelectionScreen.kt | 2 + .../shared/chat/ui/ChatSummaryMapping.kt | 1 + .../shared/chat/ui/ConversationReference.kt | 7 + .../shared/common/ui/ContactAvatar.kt | 95 +++++++--- .../app/notifications/NotificationService.kt | 28 ++- .../session/internal/RealSessionController.kt | 4 + .../SessionControllerEventRoutingTest.kt | 1 + .../SessionControllerGiftCardErrorTest.kt | 1 + .../controllers/BlobStorageController.kt | 32 +++- .../internal/domain/ChatMetadataMapper.kt | 7 +- .../internal/network/api/BlobStorageApi.kt | 26 ++- .../network/extensions/ProtobufToLocal.kt | 13 +- .../network/services/BlobStorageService.kt | 4 +- .../InternalBlobStorageRepository.kt | 4 +- .../services/models/chat/BlobAccessContext.kt | 35 ++++ .../services/models/chat/BlobMetadata.kt | 41 ++++- .../services/models/chat/MediaItem.kt | 31 +++- .../models/chat/MediaItemRendition.kt | 15 ++ .../repository/BlobStorageRepository.kt | 7 +- .../controllers/BlobStorageControllerTest.kt | 12 +- .../internal/domain/ChatMetadataMapperTest.kt | 25 +++ .../extensions/ChatMetadataExtensionTest.kt | 60 ++++++ .../services/models/chat/BlobExpiryTest.kt | 116 ++++++++++++ 33 files changed, 963 insertions(+), 47 deletions(-) create mode 100644 apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/media/ImageCachePresence.kt create mode 100644 apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/media/MediaUrlResolver.kt create mode 100644 apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/media/RememberMediaUrl.kt create mode 100644 apps/flipcash/core/src/test/kotlin/com/flipcash/app/core/media/MediaUrlResolverTest.kt create mode 100644 services/flipcash/src/main/kotlin/com/flipcash/services/models/chat/BlobAccessContext.kt create mode 100644 services/flipcash/src/test/kotlin/com/flipcash/services/internal/network/extensions/ChatMetadataExtensionTest.kt create mode 100644 services/flipcash/src/test/kotlin/com/flipcash/services/models/chat/BlobExpiryTest.kt diff --git a/apps/flipcash/app/src/main/kotlin/com/flipcash/app/MainActivity.kt b/apps/flipcash/app/src/main/kotlin/com/flipcash/app/MainActivity.kt index 17839396bf..90151d7cf3 100644 --- a/apps/flipcash/app/src/main/kotlin/com/flipcash/app/MainActivity.kt +++ b/apps/flipcash/app/src/main/kotlin/com/flipcash/app/MainActivity.kt @@ -22,6 +22,8 @@ import com.flipcash.app.billing.BillingClient import com.flipcash.app.contacts.ContactCoordinator import com.flipcash.app.contacts.LocalContactCoordinator import com.flipcash.app.core.LocalUserManager +import com.flipcash.app.core.media.LocalMediaUrlResolver +import com.flipcash.app.core.media.MediaUrlResolver import com.flipcash.app.core.tipping.LocalTipCoordinator import com.flipcash.app.core.toast.LocalToastController import com.flipcash.app.core.toast.ToastController @@ -142,6 +144,9 @@ class MainActivity : FragmentActivity() { @Inject lateinit var tippingCoordinator: TippingCoordinator + @Inject + lateinit var mediaUrlResolver: MediaUrlResolver + override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) handleUncaughtException() @@ -177,6 +182,7 @@ class MainActivity : FragmentActivity() { LocalToastController provides toastController, LocalCoinbaseOnRampController provides coinbaseOnRampController, LocalTipCoordinator provides tippingCoordinator, + LocalMediaUrlResolver provides mediaUrlResolver, LocalUiTesting provides intent.getBooleanExtra(UI_TEST, false), ) { ProvidePermissionChecker(permissionChecker) { diff --git a/apps/flipcash/app/src/main/kotlin/com/flipcash/app/internal/ui/AppNavigationBar.kt b/apps/flipcash/app/src/main/kotlin/com/flipcash/app/internal/ui/AppNavigationBar.kt index a7d69bd70e..b51a91279b 100644 --- a/apps/flipcash/app/src/main/kotlin/com/flipcash/app/internal/ui/AppNavigationBar.kt +++ b/apps/flipcash/app/src/main/kotlin/com/flipcash/app/internal/ui/AppNavigationBar.kt @@ -28,6 +28,7 @@ import com.flipcash.app.core.navigation.destinationRoute import com.flipcash.app.core.ui.NavigationBar import com.flipcash.app.core.ui.rememberNavigationBarState import com.flipcash.app.session.LocalSessionController +import com.flipcash.services.models.chat.BlobAccessContext import com.flipcash.services.user.AuthState import com.flipcash.shared.common.ui.ContactAvatar import com.getcode.manager.BottomBarManager @@ -157,5 +158,13 @@ private fun rememberProfileAvatar(): (@Composable (Modifier) -> Unit)? { val picture = profile?.profilePicture ?: return null val displayName = profile?.displayName.orEmpty() - return { modifier -> ContactAvatar(picture, displayName, modifier) } + // The account's own picture, so its blobs are the caller's own — no access context needed. + return { modifier -> + ContactAvatar( + image = picture, + displayName = displayName, + access = BlobAccessContext.Owned, + modifier = modifier, + ) + } } diff --git a/apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/media/ImageCachePresence.kt b/apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/media/ImageCachePresence.kt new file mode 100644 index 0000000000..8c8dcff514 --- /dev/null +++ b/apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/media/ImageCachePresence.kt @@ -0,0 +1,45 @@ +package com.flipcash.app.core.media + +import android.content.Context +import coil3.SingletonImageLoader +import dagger.Binds +import dagger.Module +import dagger.hilt.InstallIn +import dagger.hilt.android.qualifiers.ApplicationContext +import dagger.hilt.components.SingletonComponent +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.withContext +import javax.inject.Inject +import javax.inject.Singleton + +/** + * Whether the image loader already holds the bytes filed under a stable cache key. Blob renditions + * are cached by their durable blob id rather than their download URL, so a hit here means the URL + * will never be dereferenced — and an expired one costs nothing. + */ +interface ImageCachePresence { + suspend fun holds(cacheKey: String): Boolean +} + +@Singleton +class CoilImageCachePresence @Inject constructor( + @param:ApplicationContext private val context: Context, +) : ImageCachePresence { + + override suspend fun holds(cacheKey: String): Boolean = withContext(Dispatchers.IO) { + val loader = SingletonImageLoader.get(context) + if (loader.memoryCache?.keys.orEmpty().any { it.key == cacheKey }) { + return@withContext true + } + // openSnapshot takes a read lock on the entry; close it immediately — we only wanted to + // know it exists, the actual read happens inside Coil's own fetcher. + loader.diskCache?.openSnapshot(cacheKey)?.use { true } ?: false + } +} + +@Module +@InstallIn(SingletonComponent::class) +internal abstract class ImageCachePresenceModule { + @Binds + abstract fun bindImageCachePresence(impl: CoilImageCachePresence): ImageCachePresence +} diff --git a/apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/media/MediaUrlResolver.kt b/apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/media/MediaUrlResolver.kt new file mode 100644 index 0000000000..53a33c7d91 --- /dev/null +++ b/apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/media/MediaUrlResolver.kt @@ -0,0 +1,107 @@ +package com.flipcash.app.core.media + +import com.flipcash.app.core.time.TimeProvider +import com.flipcash.services.controllers.BlobStorageController +import com.flipcash.services.models.chat.BlobAccessContext +import com.flipcash.services.models.chat.BlobMetadata +import com.flipcash.services.models.chat.MediaItem +import com.flipcash.services.models.chat.MediaItemRendition +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock +import javax.inject.Inject +import javax.inject.Singleton + +/** + * Hands out download URLs for [MediaItem] renditions that are actually usable. + * + * A blob's bytes are immutable but its `download_url` is a short-lived signed URL carrying an + * expiry; the client is expected to call `GetBlobs` for a fresh one once that passes. Because the + * whole [MediaItem] is persisted (profile pictures live in the `user_profiles` table), a stored URL + * is routinely older than its expiry by the time a surface asks for it, and handing that straight + * to the image loader produces a failed load and a placeholder. + * + * Two things stop this from turning into a request per avatar: + * - renditions whose bytes are already cached are left alone, since their URL is never fetched, and + * - re-mints are memoised for the process lifetime and serialised, so a screenful of surfaces + * sharing a rendition costs one call. + * + * Every entry point takes the [BlobAccessContext] the media is being read from, because that is + * what authorizes a re-mint of a blob the caller doesn't own — another user's avatar resolves to + * nothing without it. + * + * [refreshUrlForSize] is the recovery path for a URL that failed anyway — metadata persisted before + * expiry was modelled carries no expiry at all, so it can only be found stale by trying it. + */ +@Singleton +class MediaUrlResolver @Inject constructor( + private val blobStorage: BlobStorageController, + private val cache: ImageCachePresence, + private val time: TimeProvider, +) { + private val mutex = Mutex() + private val reminted = mutableMapOf() + + /** + * A usable download URL for the rendition [MediaItem.renditionForSize] picks for + * [targetLongestSidePx], re-minting first if the stored one has expired. Null when the item has + * no rendition for that size. Falls back to the stored URL if a re-mint fails, so an offline + * client still gets whatever chance the cache gives it. + */ + suspend fun urlForSize( + media: MediaItem, + targetLongestSidePx: Int, + access: BlobAccessContext, + ): String? { + val rendition = media.renditionForSize(targetLongestSidePx) ?: return null + val stored = rendition.blob ?: return null + + reminted[rendition.cacheKey]?.let { return it.downloadUrl } + if (!stored.isDownloadUrlExpired(time.now())) return stored.downloadUrl + if (cache.holds(rendition.cacheKey)) return stored.downloadUrl + + return remint(rendition, access, staleUrl = stored.downloadUrl)?.downloadUrl + ?: stored.downloadUrl + } + + /** + * Re-mints the rendition for [targetLongestSidePx] after [failedUrl] failed to load, returning + * a URL to try instead, or null if the blob could not be resolved. This is the only signal + * available for metadata that carries no expiry — every row persisted before expiry was + * modelled — so a failure is taken to mean the URL was stale. + * + * Passing the URL that actually failed is what keeps this from duplicating work: if another + * caller has already minted a URL past that one, it is returned as-is. + */ + suspend fun refreshUrlForSize( + media: MediaItem, + targetLongestSidePx: Int, + failedUrl: String?, + access: BlobAccessContext, + ): String? { + val rendition = media.renditionForSize(targetLongestSidePx) ?: return null + return remint(rendition, access, staleUrl = failedUrl)?.downloadUrl + } + + /** Drops memoised URLs — they are minted for the signed-in owner and don't outlive the session. */ + suspend fun reset() { + mutex.withLock { reminted.clear() } + } + + // Serialised so a screenful of surfaces resolving the same rendition makes one call, not one + // each. Callers that were already waiting take whatever the winner minted, as long as it isn't + // the [staleUrl] they came in holding. + private suspend fun remint( + rendition: MediaItemRendition, + access: BlobAccessContext, + staleUrl: String?, + ): BlobMetadata? = mutex.withLock { + reminted[rendition.cacheKey] + ?.takeIf { it.downloadUrl != staleUrl } + ?.let { return@withLock it } + + blobStorage.refreshMetadata(listOf(rendition.blobId), access) + .getOrNull() + ?.get(rendition.cacheKey) + ?.also { reminted[rendition.cacheKey] = it } + } +} diff --git a/apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/media/RememberMediaUrl.kt b/apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/media/RememberMediaUrl.kt new file mode 100644 index 0000000000..9be4926596 --- /dev/null +++ b/apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/media/RememberMediaUrl.kt @@ -0,0 +1,85 @@ +package com.flipcash.app.core.media + +import androidx.compose.runtime.Composable +import androidx.compose.runtime.LaunchedEffect +import androidx.compose.runtime.Stable +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableIntStateOf +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember +import androidx.compose.runtime.setValue +import androidx.compose.runtime.staticCompositionLocalOf +import com.flipcash.services.models.chat.BlobAccessContext +import com.flipcash.services.models.chat.MediaItem + +val LocalMediaUrlResolver = staticCompositionLocalOf { null } + +/** + * The download URL a blob-backed surface should render, and whether rendering it is hopeless. + * + * [url] starts as the URL stored on the [MediaItem] and is replaced if it needs re-minting, so a + * cached rendition draws on the first frame instead of waiting on a resolve. [hasFailed] only goes + * true once a re-minted URL has failed too — a first failure is treated as a stale URL and retried + * silently, leaving whatever placeholder is on screen in place. + */ +@Stable +class MediaUrlState internal constructor(initialUrl: String?) { + var url: String? by mutableStateOf(initialUrl) + internal set + + var hasFailed: Boolean by mutableStateOf(false) + internal set + + internal var failures by mutableIntStateOf(0) + private set + + /** The URL the last failure was reported against — what a re-mint has to improve on. */ + internal var failedUrl: String? = null + private set + + /** Report that the image loader could not load [url]. */ + fun onLoadFailed() { + failedUrl = url + failures++ + } +} + +/** + * Resolves [media]'s rendition for [targetLongestSidePx] into a URL that can be handed to the image + * loader, keeping it usable as the stored one expires. See [MediaUrlResolver] for why a stored URL + * often isn't. [access] names the surface [media] is being read from — a re-mint of media the + * caller doesn't own resolves to nothing without it. + */ +@Composable +fun rememberMediaUrl( + media: MediaItem?, + targetLongestSidePx: Int, + access: BlobAccessContext, +): MediaUrlState { + val resolver = LocalMediaUrlResolver.current + val state = remember(media, targetLongestSidePx) { + MediaUrlState(media?.urlForSize(targetLongestSidePx)) + } + + LaunchedEffect(state, resolver) { + if (media == null || resolver == null) return@LaunchedEffect + resolver.urlForSize(media, targetLongestSidePx, access)?.let { state.url = it } + } + + LaunchedEffect(state, resolver, state.failures) { + if (state.failures == 0) return@LaunchedEffect + // Only the first failure is worth a re-mint; a fresh URL that also fails is a real error. + if (media == null || resolver == null || state.failures > 1) { + state.hasFailed = true + return@LaunchedEffect + } + val fresh = resolver.refreshUrlForSize(media, targetLongestSidePx, state.failedUrl, access) + if (fresh != null && fresh != state.url) { + state.url = fresh + } else { + state.hasFailed = true + } + } + + return state +} diff --git a/apps/flipcash/core/src/test/kotlin/com/flipcash/app/core/media/MediaUrlResolverTest.kt b/apps/flipcash/core/src/test/kotlin/com/flipcash/app/core/media/MediaUrlResolverTest.kt new file mode 100644 index 0000000000..6675f5f530 --- /dev/null +++ b/apps/flipcash/core/src/test/kotlin/com/flipcash/app/core/media/MediaUrlResolverTest.kt @@ -0,0 +1,173 @@ +package com.flipcash.app.core.media + +import com.flipcash.app.core.time.FakeTimeProvider +import com.flipcash.services.controllers.BlobStorageController +import com.flipcash.services.models.chat.BlobAccessContext +import com.flipcash.services.models.chat.BlobId +import com.flipcash.services.models.chat.BlobMetadata +import com.flipcash.services.models.chat.ImageMetadata +import com.flipcash.services.models.chat.MediaItem +import com.flipcash.services.models.chat.MediaItemRendition +import com.getcode.utils.base58 +import io.mockk.coEvery +import io.mockk.coVerify +import io.mockk.mockk +import kotlinx.coroutines.async +import kotlinx.coroutines.test.runTest +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.time.Duration.Companion.minutes +import kotlin.time.Instant + +class MediaUrlResolverTest { + + private val now = Instant.fromEpochMilliseconds(1_700_000_000_000) + private val time = FakeTimeProvider(now) + private val controller = mockk() + + private var cachedKeys = emptySet() + private val cache = object : ImageCachePresence { + override suspend fun holds(cacheKey: String): Boolean = cacheKey in cachedKeys + } + + private val resolver = MediaUrlResolver(controller, cache, time) + + // Every avatar in these tests belongs to someone else, which is the case that needs a context. + private val access = BlobAccessContext.Profile(listOf(7)) + + private val thumbId = BlobId(byteArrayOf(1)) + private val thumbKey = byteArrayOf(1).base58 + + private fun media(expiresAt: Instant?, url: String = "https://cdn/stored") = MediaItem( + listOf( + MediaItemRendition( + role = MediaItemRendition.Role.THUMBNAIL, + blobId = thumbId, + blob = BlobMetadata( + mimeType = "image/png", + sizeBytes = 1, + downloadUrl = url, + image = ImageMetadata(width = 160, height = 160, blurhash = "abc"), + expiresAtMillis = expiresAt?.toEpochMilliseconds(), + ), + ) + ) + ) + + private fun freshMetadata(url: String) = BlobMetadata( + mimeType = "image/png", + sizeBytes = 1, + downloadUrl = url, + image = ImageMetadata(width = 160, height = 160, blurhash = "abc"), + expiresAtMillis = (now + 10.minutes).toEpochMilliseconds(), + ) + + @Test + fun `a live URL is handed straight back`() = runTest { + val url = resolver.urlForSize(media(expiresAt = now + 10.minutes), 160, access) + + assertEquals("https://cdn/stored", url) + coVerify(exactly = 0) { controller.refreshMetadata(any(), any()) } + } + + @Test + fun `an expired URL is re-minted`() = runTest { + coEvery { controller.refreshMetadata(any(), any()) } returns + Result.success(mapOf(thumbKey to freshMetadata("https://cdn/fresh"))) + + val url = resolver.urlForSize(media(expiresAt = now - 1.minutes), 160, access) + + assertEquals("https://cdn/fresh", url) + coVerify(exactly = 1) { controller.refreshMetadata(listOf(thumbId), any()) } + } + + @Test + fun `an expired URL whose bytes are already cached costs no round trip`() = runTest { + cachedKeys = setOf(thumbKey) + + val url = resolver.urlForSize(media(expiresAt = now - 1.minutes), 160, access) + + assertEquals("https://cdn/stored", url) + coVerify(exactly = 0) { controller.refreshMetadata(any(), any()) } + } + + @Test + fun `a failed re-mint falls back to the stored URL`() = runTest { + coEvery { controller.refreshMetadata(any(), any()) } returns Result.failure(Throwable("offline")) + + val url = resolver.urlForSize(media(expiresAt = now - 1.minutes), 160, access) + + assertEquals("https://cdn/stored", url) + } + + @Test + fun `surfaces sharing a rendition share one re-mint`() = runTest { + coEvery { controller.refreshMetadata(any(), any()) } returns + Result.success(mapOf(thumbKey to freshMetadata("https://cdn/fresh"))) + val item = media(expiresAt = now - 1.minutes) + + val first = async { resolver.urlForSize(item, 160, access) } + val second = async { resolver.urlForSize(item, 160, access) } + + assertEquals("https://cdn/fresh", first.await()) + assertEquals("https://cdn/fresh", second.await()) + coVerify(exactly = 1) { controller.refreshMetadata(any(), any()) } + } + + @Test + fun `refreshUrlForSize re-mints metadata that carries no expiry`() = runTest { + // Rows persisted before expiry was modelled: nothing marks them stale but a failed load. + coEvery { controller.refreshMetadata(any(), any()) } returns + Result.success(mapOf(thumbKey to freshMetadata("https://cdn/fresh"))) + val item = media(expiresAt = null) + + assertEquals("https://cdn/stored", resolver.urlForSize(item, 160, access)) + assertEquals("https://cdn/fresh", resolver.refreshUrlForSize(item, 160, "https://cdn/stored", access)) + } + + @Test + fun `reset drops URLs minted for the previous owner`() = runTest { + coEvery { controller.refreshMetadata(any(), any()) } returns + Result.success(mapOf(thumbKey to freshMetadata("https://cdn/fresh"))) + val item = media(expiresAt = now - 1.minutes) + resolver.urlForSize(item, 160, access) + + resolver.reset() + resolver.urlForSize(item, 160, access) + + coVerify(exactly = 2) { controller.refreshMetadata(any(), any()) } + } + + @Test + fun `a load that failed on an already-superseded URL reuses the mint it missed`() = runTest { + // urlForSize re-mints while a surface is still showing the stale URL; that surface's + // failure must not spend a second call re-minting what it can already be handed. + coEvery { controller.refreshMetadata(any(), any()) } returns + Result.success(mapOf(thumbKey to freshMetadata("https://cdn/fresh"))) + val item = media(expiresAt = now - 1.minutes) + resolver.urlForSize(item, 160, access) + + val retry = resolver.refreshUrlForSize(item, 160, failedUrl = "https://cdn/stored", access) + + assertEquals("https://cdn/fresh", retry) + coVerify(exactly = 1) { controller.refreshMetadata(any(), any()) } + } + + @Test + fun `the caller's access context is what re-mints someone else's media`() = runTest { + // Without it the server omits blobs the caller doesn't own, so the re-mint comes back empty + // and the avatar falls back — the failure this whole path exists to avoid. + coEvery { controller.refreshMetadata(any(), any()) } returns + Result.success(mapOf(thumbKey to freshMetadata("https://cdn/fresh"))) + + resolver.urlForSize(media(expiresAt = now - 1.minutes), 160, access) + + coVerify(exactly = 1) { controller.refreshMetadata(listOf(thumbId), access) } + } + + @Test + fun `an item with no rendition resolves to null`() = runTest { + assertEquals(null, resolver.urlForSize(MediaItem(emptyList()), 160, access)) + assertEquals(null, resolver.refreshUrlForSize(MediaItem(emptyList()), 160, null, access)) + } +} diff --git a/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/screens/components/ParticipantAvatar.kt b/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/screens/components/ParticipantAvatar.kt index bfc508417f..065e99a21f 100644 --- a/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/screens/components/ParticipantAvatar.kt +++ b/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/screens/components/ParticipantAvatar.kt @@ -20,7 +20,14 @@ internal fun ParticipantAvatar( ) { when (participant) { is ChatParticipant.Contact -> ContactAvatar(contact = participant.contact, modifier = modifier) - is ChatParticipant.TipUser -> ContactAvatar(userProfile = participant.profile, modifier = modifier) + // The member's own id, not profile.userId: the server sets the id on the chat member and + // leaves it unset inside the nested profile, and without it the picture's expired download + // URL cannot be re-minted. + is ChatParticipant.TipUser -> ContactAvatar( + userProfile = participant.profile, + modifier = modifier, + userId = participant.userId, + ) null -> ContactAvatar(contact = null, modifier = modifier) } } diff --git a/apps/flipcash/features/myaccount/src/main/kotlin/com/flipcash/app/myaccount/internal/blocklist/BlocklistScreenContent.kt b/apps/flipcash/features/myaccount/src/main/kotlin/com/flipcash/app/myaccount/internal/blocklist/BlocklistScreenContent.kt index 92709fbb80..ee42f3cca2 100644 --- a/apps/flipcash/features/myaccount/src/main/kotlin/com/flipcash/app/myaccount/internal/blocklist/BlocklistScreenContent.kt +++ b/apps/flipcash/features/myaccount/src/main/kotlin/com/flipcash/app/myaccount/internal/blocklist/BlocklistScreenContent.kt @@ -33,6 +33,7 @@ import androidx.paging.compose.itemKey import kotlinx.coroutines.delay import com.flipcash.app.core.blocklist.BlockedUserProfile import com.flipcash.features.myaccount.R +import com.flipcash.services.models.chat.BlobAccessContext import com.flipcash.shared.common.ui.ContactAvatar import com.getcode.theme.CodeTheme import com.getcode.ui.theme.CodeCircularProgressIndicator @@ -113,6 +114,7 @@ private fun BlockedUserRow( ContactAvatar( image = user.profilePicture, displayName = user.name.orEmpty(), + access = BlobAccessContext.profile(user.userId), // Blocked users are shown obscured, per the design. blurred = true, modifier = Modifier diff --git a/apps/flipcash/features/myaccount/src/main/kotlin/com/flipcash/app/myaccount/internal/userprofile/UserProfileScreenContent.kt b/apps/flipcash/features/myaccount/src/main/kotlin/com/flipcash/app/myaccount/internal/userprofile/UserProfileScreenContent.kt index 2bf81fac49..f4855604a8 100644 --- a/apps/flipcash/features/myaccount/src/main/kotlin/com/flipcash/app/myaccount/internal/userprofile/UserProfileScreenContent.kt +++ b/apps/flipcash/features/myaccount/src/main/kotlin/com/flipcash/app/myaccount/internal/userprofile/UserProfileScreenContent.kt @@ -63,6 +63,7 @@ import androidx.compose.runtime.remember import androidx.compose.runtime.setValue import androidx.compose.ui.unit.sp import com.flipcash.app.core.util.abbreviatedLink +import com.flipcash.services.models.chat.BlobAccessContext import com.getcode.theme.White05 import com.getcode.theme.extraSmall import com.getcode.ui.core.verticalScrollStateGradient @@ -323,6 +324,8 @@ private fun ProfileHeader( ContactAvatar( image = profilePicture, displayName = displayName, + // This screen is the account's own public profile, so it owns the picture's blobs. + access = BlobAccessContext.Owned, modifier = Modifier .size(96.dp) .clip(CircleShape), diff --git a/apps/flipcash/features/tipping/src/main/kotlin/com/flipcash/app/tipping/internal/components/TipChatRow.kt b/apps/flipcash/features/tipping/src/main/kotlin/com/flipcash/app/tipping/internal/components/TipChatRow.kt index 6aab7b47e2..c98008eed8 100644 --- a/apps/flipcash/features/tipping/src/main/kotlin/com/flipcash/app/tipping/internal/components/TipChatRow.kt +++ b/apps/flipcash/features/tipping/src/main/kotlin/com/flipcash/app/tipping/internal/components/TipChatRow.kt @@ -6,6 +6,7 @@ import androidx.compose.material3.Text import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier import androidx.compose.ui.draw.clip +import com.flipcash.services.models.chat.BlobAccessContext import com.flipcash.shared.chat.ui.ChatListRow import com.flipcash.shared.chat.ui.ChatRowSubtitle import com.flipcash.shared.chat.ui.ChatRowTrailing @@ -28,6 +29,7 @@ internal fun TipChatRow( ContactAvatar( image = chat.image, displayName = chat.name.orEmpty(), + access = BlobAccessContext.profile(chat.userId), modifier = Modifier .requiredSize(CodeTheme.dimens.staticGrid.x8) .clip(CircleShape), diff --git a/apps/flipcash/features/user-profile/src/main/kotlin/com/flipcash/app/userprofile/internal/photo/PhotoSelectionScreen.kt b/apps/flipcash/features/user-profile/src/main/kotlin/com/flipcash/app/userprofile/internal/photo/PhotoSelectionScreen.kt index 824094b3fc..acb76c7e37 100644 --- a/apps/flipcash/features/user-profile/src/main/kotlin/com/flipcash/app/userprofile/internal/photo/PhotoSelectionScreen.kt +++ b/apps/flipcash/features/user-profile/src/main/kotlin/com/flipcash/app/userprofile/internal/photo/PhotoSelectionScreen.kt @@ -41,6 +41,7 @@ import com.flipcash.app.core.ui.transitions.sharedBoundsTransition import com.flipcash.app.core.userprofile.UpdateProfileResult import com.flipcash.app.core.userprofile.UpdateProfileStep import com.flipcash.core.R +import com.flipcash.services.models.chat.BlobAccessContext import com.flipcash.shared.common.ui.ContactAvatar import com.getcode.navigation.flow.rememberFlowNavigator import com.getcode.theme.CodeTheme @@ -183,6 +184,7 @@ private fun PhotoSelectionScreenContent( ContactAvatar( image = state.savedPicture, displayName = state.name, + access = BlobAccessContext.Owned, modifier = Modifier.fillMaxSize(), ) } diff --git a/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/ui/ChatSummaryMapping.kt b/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/ui/ChatSummaryMapping.kt index 4a14fee51f..f1b96b66d5 100644 --- a/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/ui/ChatSummaryMapping.kt +++ b/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/ui/ChatSummaryMapping.kt @@ -30,6 +30,7 @@ fun ChatSummary.toConversationReference( val other = metadata.members.firstOrNull { it.userId != selfId } return ConversationReference( chatId = metadata.chatId, + userId = other?.userId, displayName = other?.userProfile?.displayName, handle = other?.userProfile?.handle, image = other?.userProfile?.profilePicture, diff --git a/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/ui/ConversationReference.kt b/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/ui/ConversationReference.kt index 670c74a2ac..4bf9cf1cc9 100644 --- a/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/ui/ConversationReference.kt +++ b/apps/flipcash/shared/chat-ui/src/main/kotlin/com/flipcash/shared/chat/ui/ConversationReference.kt @@ -1,5 +1,6 @@ package com.flipcash.shared.chat.ui +import com.getcode.opencode.model.core.ID import com.flipcash.services.models.chat.ChatId import com.flipcash.services.models.chat.MediaItem import com.flipcash.services.models.nameOrHandle @@ -8,6 +9,12 @@ import kotlin.time.Instant /** Presentation state derived from an existing DM with a contact. */ data class ConversationReference( val chatId: ChatId, + /** + * Counterparty account id, when the chat has a resolved member. Rows need it to re-mint + * [image]'s download URL: the picture is not the caller's, so only that user's profile + * authorizes reading it. + */ + val userId: ID? = null, /** Counterparty display name — used when the row has no separate contact (e.g. tip DMs). */ val displayName: String? = null, /** diff --git a/apps/flipcash/shared/common-ui/src/main/kotlin/com/flipcash/shared/common/ui/ContactAvatar.kt b/apps/flipcash/shared/common-ui/src/main/kotlin/com/flipcash/shared/common/ui/ContactAvatar.kt index 3251205439..2fee4b4565 100644 --- a/apps/flipcash/shared/common-ui/src/main/kotlin/com/flipcash/shared/common/ui/ContactAvatar.kt +++ b/apps/flipcash/shared/common-ui/src/main/kotlin/com/flipcash/shared/common/ui/ContactAvatar.kt @@ -1,5 +1,6 @@ package com.flipcash.shared.common.ui +import android.graphics.Bitmap import androidx.compose.foundation.Image import androidx.compose.foundation.background import androidx.compose.foundation.border @@ -33,11 +34,15 @@ import coil3.asImage import coil3.compose.AsyncImage import coil3.request.ImageRequest import coil3.request.crossfade +import coil3.request.error import coil3.request.placeholder import com.flipcash.app.core.contacts.DeviceContact +import com.flipcash.app.core.media.rememberMediaUrl import com.flipcash.services.models.HandlePrefix import com.flipcash.services.models.UserProfile +import com.flipcash.services.models.chat.BlobAccessContext import com.flipcash.services.models.chat.MediaItem +import com.getcode.opencode.model.core.ID import com.getcode.theme.CodeTheme import com.getcode.ui.core.addIf @@ -103,13 +108,22 @@ fun ContactAvatar( } } +/** + * [userId] identifies whose profile this is, which is what authorizes re-minting the picture's + * expired download URL. It defaults to the profile's own id and only needs passing where the + * caller holds a more reliable one — a chat member's profile, for instance, arrives with the id + * on the member rather than inside the nested profile. + */ @Composable fun ContactAvatar( userProfile: UserProfile, modifier: Modifier = Modifier, + userId: ID? = userProfile.userId, ) { ProfileAvatar( image = userProfile.profilePicture, + // The picture belongs to this profile, so the profile is what authorizes re-minting it. + access = BlobAccessContext.profile(userId), modifier = modifier, fallback = { UnknownContactAvatar(includeBorder = true) }, ) @@ -122,16 +136,22 @@ fun ContactAvatar( * display sizes) so the image is never grainy or over-fetched, and bridges the load with the * item's BlurHash plus any already-cached smaller rendition. Falls back to [displayName]'s * initials when there's no picture. + * + * [access] is the surface the picture is being read from. A stored download URL expires, and + * re-minting one for a blob the caller doesn't own is only authorized by naming that surface — + * [BlobAccessContext.profile] covers every avatar, given the id of the profile it belongs to. */ @Composable fun ContactAvatar( image: MediaItem?, displayName: String, + access: BlobAccessContext, modifier: Modifier = Modifier, blurred: Boolean = false, ) { ProfileAvatar( image = image, + access = access, modifier = modifier, blurred = blurred, fallback = { InitialsText(displayName) }, @@ -141,6 +161,7 @@ fun ContactAvatar( @Composable private fun ProfileAvatar( image: MediaItem?, + access: BlobAccessContext, modifier: Modifier, blurred: Boolean = false, fallback: @Composable BoxWithConstraintsScope.() -> Unit, @@ -150,23 +171,21 @@ private fun ProfileAvatar( Brush.linearGradient(CodeTheme.colors.contactAvatar.colors) ) ) { + // Decoded once for the whole avatar, not just the loading state: it is the placeholder + // while the rendition downloads, what stays on screen while a stale URL is re-minted, and + // what's left if the load never succeeds — anything is a better likeness of the person than + // the grey Person glyph. + // Kept as the decoded Bitmap rather than either wrapper: the two consumers want different + // ones (Compose's ImageBitmap here, Coil's Image in the request), and decoding is the + // expensive half. + val blurBitmap = remember(image) { + BlurHash.decode(image?.blurhash(), width = 24, height = 24) + } if (blurred) { // Blocked users are shown intentionally obscured — render the media item's self-contained // BlurHash preview instead of the real image, so the avatar stays blurred on every API // level (Modifier.blur needs API 31+) without ever fetching the sharp photo. - val blurBitmap = remember(image) { - BlurHash.decode(image?.blurhash(), width = 24, height = 24)?.asImageBitmap() - } - if (blurBitmap != null) { - Image( - bitmap = blurBitmap, - contentDescription = null, - modifier = Modifier.matchParentSize(), - contentScale = ContentScale.Crop, - ) - } else { - fallback() - } + BlurHashOr(blurBitmap, fallback) return@BoxWithConstraints } // Pick the rendition by the avatar's actual pixel size — the longest bounded side of the @@ -176,9 +195,13 @@ private fun ProfileAvatar( val h = if (constraints.hasBoundedHeight) constraints.maxHeight else 0 maxOf(w, h).takeIf { it > 0 } ?: Int.MAX_VALUE } - val photoUri = remember(image, targetPx) { image?.urlForSize(targetPx) } + // Not `image.urlForSize(targetPx)` directly: the stored download URL expires, and a + // persisted profile picture is routinely past it by the time it's rendered. The resolver + // hands back a re-minted URL when that's the case, and again if a load fails anyway. + val media = rememberMediaUrl(image, targetPx, access) + val photoUri = media.url if (image != null && photoUri != null) { - var isError by rememberSaveable(photoUri) { mutableStateOf(false) } + val isError = media.hasFailed if (!isError) { val context = LocalContext.current // Two progressively better placeholders bridge the load so we never flash a blank @@ -187,9 +210,7 @@ private fun ProfileAvatar( // 2. the next-smaller rendition — if another surface already cached it (e.g. the // list loaded the 160 this 320 avatar sits above), Coil shows it immediately // (see placeholderMemoryCacheKey) and upgrades in place. - val blurHash = remember(image) { - BlurHash.decode(image.blurhash(), width = 24, height = 24)?.asImage() - } + val blurHash = remember(blurBitmap) { blurBitmap?.asImage() } // Cache identity is the durable blob id, NOT the download URL: the server re-mints // and expires `download_url` on every fetch, so a URL-keyed cache misses on the next // fetch even though the bytes are immutable — every load would re-download and flash @@ -211,7 +232,14 @@ private fun ProfileAvatar( .memoryCacheKey(cacheKey) .diskCacheKey(cacheKey) .apply { - blurHash?.let { placeholder(it) } + blurHash?.let { + placeholder(it) + // `placeholder` only covers the load itself. A failed load draws + // the error drawable, so without this the avatar blanks the moment + // an expired URL 403s — including for the whole re-mint round trip, + // which is exactly when the BlurHash is worth the most. + error(it) + } previewKey?.let { placeholderMemoryCacheKey(it) } } .build() @@ -224,18 +252,41 @@ private fun ProfileAvatar( // AsyncImage infers the Coil request scale from this, so the request is fine. contentScale = ContentScale.Crop, contentDescription = null, - onError = { isError = true }, + onError = { media.onLoadFailed() }, ) } if (isError) { - fallback() + BlurHashOr(blurBitmap, fallback) } } else { - fallback() + BlurHashOr(blurBitmap, fallback) } } } +/** + * The [blurBitmap] preview if the media carried a BlurHash, otherwise [fallback]. Preferred over + * the initials/Person fallback wherever a hash exists: it is the real photo's colours, and it is + * already in hand — no fetch, no expiry, nothing to fail. + */ +@Composable +private fun BoxWithConstraintsScope.BlurHashOr( + blurBitmap: Bitmap?, + fallback: @Composable BoxWithConstraintsScope.() -> Unit, +) { + val bitmap = remember(blurBitmap) { blurBitmap?.asImageBitmap() } + if (bitmap != null) { + Image( + bitmap = bitmap, + contentDescription = null, + modifier = Modifier.matchParentSize(), + contentScale = ContentScale.Crop, + ) + } else { + fallback() + } +} + @Composable private fun UnknownContactAvatar( includeBorder: Boolean, diff --git a/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/NotificationService.kt b/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/NotificationService.kt index 17a3adcd32..02805ef5ca 100644 --- a/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/NotificationService.kt +++ b/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/NotificationService.kt @@ -22,6 +22,7 @@ import coil3.request.ImageRequest import coil3.request.SuccessResult import coil3.request.allowHardware import coil3.toBitmap +import com.flipcash.app.core.media.MediaUrlResolver import com.flipcash.app.auth.AuthManager import com.flipcash.app.contacts.ContactCoordinator import com.flipcash.app.contacts.ContactResolver @@ -41,6 +42,7 @@ import com.flipcash.services.models.NotificationCategory import com.flipcash.services.models.NotificationPayload import com.flipcash.services.models.PushChatMetadata import com.flipcash.services.models.Substitution +import com.flipcash.services.models.chat.BlobAccessContext import com.flipcash.services.models.chat.ChatType import com.flipcash.services.user.UserManager import com.flipcash.shared.notifications.R @@ -82,6 +84,9 @@ class NotificationService : FirebaseMessagingService(), @Inject lateinit var pushController: PushController + @Inject + lateinit var mediaUrlResolver: MediaUrlResolver + @Inject lateinit var notificationManager: NotificationManagerCompat @@ -267,9 +272,18 @@ class NotificationService : FirebaseMessagingService(), // the tiny 32px one looks grainy on the notification's person icon. val avatarPx = resources.getDimensionPixelSize(android.R.dimen.notification_large_icon_width) val avatar = e164?.let { resolveContactPhoto(it) } - ?: member?.userProfile?.profilePicture - ?.urlForSize(avatarPx) - ?.let { loadRemoteAvatar(it) } + ?: member?.userProfile?.profilePicture?.let { picture -> + // Through the resolver, not `picture.urlForSize` — the stored download URL expires + // and the profile it came from may have been persisted days ago. + mediaUrlResolver.urlForSize( + media = picture, + targetLongestSidePx = avatarPx, + // The counterparty's picture, so their profile is what authorizes a re-mint. + access = BlobAccessContext.profile(member?.userId), + )?.let { url -> + loadRemoteAvatar(url, picture.cacheKeyForSize(avatarPx)) + } + } trace( tag = "NotificationService", @@ -317,11 +331,17 @@ class NotificationService : FirebaseMessagingService(), * notification. Returns `null` on timeout or failure (the notification then * posts with the name monogram). */ - private suspend fun loadRemoteAvatar(url: String): Bitmap? = + private suspend fun loadRemoteAvatar(url: String, cacheKey: String?): Bitmap? = withTimeoutOrNull(AVATAR_FETCH_TIMEOUT_MS.milliseconds) { runCatching { val request = ImageRequest.Builder(this@NotificationService) .data(url) + // Keyed on the durable blob id like the in-app avatars, so a notification hits + // the rendition they already cached instead of re-downloading under a URL that + // is different on every mint. + .apply { + cacheKey?.let { memoryCacheKey(it); diskCacheKey(it) } + } .allowHardware(false) // notification icons require a software bitmap .build() val result = SingletonImageLoader.get(this@NotificationService).execute(request) diff --git a/apps/flipcash/shared/session/src/main/kotlin/com/flipcash/app/session/internal/RealSessionController.kt b/apps/flipcash/shared/session/src/main/kotlin/com/flipcash/app/session/internal/RealSessionController.kt index a18357c4c1..8e4685eadc 100644 --- a/apps/flipcash/shared/session/src/main/kotlin/com/flipcash/app/session/internal/RealSessionController.kt +++ b/apps/flipcash/shared/session/src/main/kotlin/com/flipcash/app/session/internal/RealSessionController.kt @@ -8,6 +8,7 @@ import com.flipcash.app.billing.BillingClient import com.flipcash.app.blocklist.BlocklistCoordinator import com.flipcash.app.contacts.ContactCoordinator import com.flipcash.app.blob.BlobStorageCoordinator +import com.flipcash.app.core.media.MediaUrlResolver import com.flipcash.services.models.chat.ChatType import com.flipcash.shared.chat.ChatCoordinator import com.flipcash.app.core.bill.Scannable @@ -110,6 +111,7 @@ class RealSessionController @Inject constructor( private val chatCoordinator: ChatCoordinator, private val blocklistCoordinator: BlocklistCoordinator, private val blobStorageCoordinator: BlobStorageCoordinator, + private val mediaUrlResolver: MediaUrlResolver, networkObserver: NetworkConnectivityListener, featureFlagController: FeatureFlagController, appSettingsCoordinator: AppSettingsCoordinator, @@ -191,6 +193,8 @@ class RealSessionController @Inject constructor( depositDelegate.cancelSweep() scope.launch { contactCoordinator.reset() } scope.launch { chatCoordinator.teardown() } + // Blob download URLs are minted for the signed-in owner. + scope.launch { mediaUrlResolver.reset() } stateHolder.reset() } diff --git a/apps/flipcash/shared/session/src/test/kotlin/com/flipcash/app/session/internal/SessionControllerEventRoutingTest.kt b/apps/flipcash/shared/session/src/test/kotlin/com/flipcash/app/session/internal/SessionControllerEventRoutingTest.kt index 2226ad184d..735eb77174 100644 --- a/apps/flipcash/shared/session/src/test/kotlin/com/flipcash/app/session/internal/SessionControllerEventRoutingTest.kt +++ b/apps/flipcash/shared/session/src/test/kotlin/com/flipcash/app/session/internal/SessionControllerEventRoutingTest.kt @@ -121,6 +121,7 @@ class SessionControllerEventRoutingTest { chatCoordinator = mockk(relaxed = true), blocklistCoordinator = mockk(relaxed = true), blobStorageCoordinator = mockk(relaxed = true), + mediaUrlResolver = mockk(relaxed = true), featureFlagController = featureFlagController, appSettingsCoordinator = appSettingsCoordinator, dispatchers = dispatchers, diff --git a/apps/flipcash/shared/session/src/test/kotlin/com/flipcash/app/session/internal/SessionControllerGiftCardErrorTest.kt b/apps/flipcash/shared/session/src/test/kotlin/com/flipcash/app/session/internal/SessionControllerGiftCardErrorTest.kt index 0daee54d4a..51cd8374b3 100644 --- a/apps/flipcash/shared/session/src/test/kotlin/com/flipcash/app/session/internal/SessionControllerGiftCardErrorTest.kt +++ b/apps/flipcash/shared/session/src/test/kotlin/com/flipcash/app/session/internal/SessionControllerGiftCardErrorTest.kt @@ -147,6 +147,7 @@ class SessionControllerGiftCardErrorTest { chatCoordinator = mockk(relaxed = true), blocklistCoordinator = mockk(relaxed = true), blobStorageCoordinator = mockk(relaxed = true), + mediaUrlResolver = mockk(relaxed = true), featureFlagController = mockk(relaxed = true), appSettingsCoordinator = mockk(relaxed = true), dispatchers = dispatchers, diff --git a/services/flipcash/src/main/kotlin/com/flipcash/services/controllers/BlobStorageController.kt b/services/flipcash/src/main/kotlin/com/flipcash/services/controllers/BlobStorageController.kt index 222401e5ee..bdd3675ec9 100644 --- a/services/flipcash/src/main/kotlin/com/flipcash/services/controllers/BlobStorageController.kt +++ b/services/flipcash/src/main/kotlin/com/flipcash/services/controllers/BlobStorageController.kt @@ -4,11 +4,14 @@ import com.flipcash.services.BlobUploader import com.flipcash.services.models.BlobNotReadyException import com.flipcash.services.models.BlobRejectedException import com.flipcash.services.models.blob.UploadPolicy +import com.flipcash.services.models.chat.BlobAccessContext import com.flipcash.services.models.chat.BlobId +import com.flipcash.services.models.chat.BlobMetadata import com.flipcash.services.models.chat.BlobState import com.flipcash.services.repository.BlobStorageRepository import com.flipcash.services.user.UserManager import com.getcode.ed25519.Ed25519 +import com.getcode.utils.base58 import kotlinx.coroutines.delay import javax.inject.Inject import kotlin.time.Duration @@ -58,11 +61,38 @@ class BlobStorageController @Inject constructor( return awaitReady(reservation.blobId, owner) } + /** + * Re-resolves [blobIds] to freshly minted [BlobMetadata] — the recovery `DownloadUrl.expires_at` + * calls for. A blob's bytes are immutable but its `download_url` is per-fetch and expiring, so + * any metadata that has been held a while (persisted profile pictures, chat media) needs this + * before its URL can be fetched again. + * + * Returns only the ids that came back READY, keyed by base58 blob id — ids still processing or + * rejected are simply absent, leaving the caller's existing metadata in place. + * + * [context] names the surface the blobs are being read from. It is what authorizes ids the + * caller does not own — another user's avatar resolves only through + * [BlobAccessContext.Profile], chat media only through [BlobAccessContext.Chat] — and without + * it the server omits them from the response rather than failing, so a wrong context looks + * exactly like a blob that isn't ready yet. + */ + suspend fun refreshMetadata( + blobIds: List, + context: BlobAccessContext, + ): Result> { + if (blobIds.isEmpty()) return Result.success(emptyMap()) + val owner = owner() ?: return noAccount() + return repository.getBlobs(blobIds, owner, context).map { blobs -> + blobs.filterIsInstance() + .associate { it.id.bytes.base58 to it.metadata } + } + } + private suspend fun awaitReady(blobId: BlobId, owner: Ed25519.KeyPair): Result { var elapsed: Duration = Duration.ZERO while (elapsed < POLL_TIMEOUT) { // A single-id query resolves to at most one blob, so first() is the one we asked for. - val blob = repository.getBlobs(listOf(blobId), owner) + val blob = repository.getBlobs(listOf(blobId), owner, BlobAccessContext.Owned) .getOrElse { return Result.failure(it) } .firstOrNull() diff --git a/services/flipcash/src/main/kotlin/com/flipcash/services/internal/domain/ChatMetadataMapper.kt b/services/flipcash/src/main/kotlin/com/flipcash/services/internal/domain/ChatMetadataMapper.kt index 530805973f..fc5b4ed8f8 100644 --- a/services/flipcash/src/main/kotlin/com/flipcash/services/internal/domain/ChatMetadataMapper.kt +++ b/services/flipcash/src/main/kotlin/com/flipcash/services/internal/domain/ChatMetadataMapper.kt @@ -22,7 +22,12 @@ class ChatMetadataMapper @Inject constructor( members = from.membersList.map { member -> ChatMember( userId = member.userId.toId(), - userProfile = userProfileMapper.map(member.userProfile), + // The server sets the id on the member and usually not again inside the nested + // profile, and this profile is by definition that member's. Leaving it null + // costs callers the id that authorizes re-minting the profile picture's + // download URL, so the avatar can never recover once the stored URL expires. + userProfile = userProfileMapper.map(member.userProfile) + .let { if (it.userId == null) it.copy(userId = member.userId.toId()) else it }, pointers = member.pointersList.map { it.toPointer() }, ) }, 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 a36755cb07..3fda74bd2a 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 @@ -5,7 +5,10 @@ import com.codeinc.flipcash.gen.blob.v1.BlobStorageService as RpcBlobStorageServ import com.codeinc.flipcash.gen.blob.v1.Model import com.codeinc.flipcash.gen.blob.v1.validate import com.flipcash.services.internal.annotations.FlipcashManagedChannel +import com.flipcash.services.internal.network.extensions.asChatId +import com.flipcash.services.internal.network.extensions.asUserId import com.flipcash.services.internal.network.extensions.authenticate +import com.flipcash.services.models.chat.BlobAccessContext import com.flipcash.services.models.chat.BlobId import com.getcode.ed25519.Ed25519 import com.getcode.opencode.internal.network.core.GrpcApi @@ -81,6 +84,7 @@ internal class BlobStorageApi @Inject constructor( suspend fun getBlobs( blobIds: List, owner: Ed25519.KeyPair, + context: BlobAccessContext, ): RpcBlobStorageService.GetBlobsResponse { val request = RpcBlobStorageService.GetBlobsRequest.newBuilder() .setBlobIds( @@ -90,13 +94,33 @@ internal class BlobStorageApi @Inject constructor( .apply { setAuth(authenticate(owner)) } .build() + // Validated before the context is attached, because AccessContext cannot pass client-side + // validation: the generated validator asserts BOTH oneof arms are set + // (`checkRequired(scopeCase == CHAT, "chat")` and the same for `profile`), so any scope + // fails as `context.chat: value is required`. The rule the proto declares — required *if + // selected* — is what the server enforces, and every other rule on the request is still + // checked here. Drop this split once the validator generator handles oneof members. request.validate().orThrow() + // 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. + val authorized = context.toProto() + ?.let { request.toBuilder().setContext(it).build() } + ?: request + return withContext(Dispatchers.IO) { - api.getBlobs(request) + api.getBlobs(authorized) } } 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().setProfile(userId.asUserId()).build() + is BlobAccessContext.Chat -> + Model.AccessContext.newBuilder().setChat(chatId.asChatId()).build() + } } diff --git a/services/flipcash/src/main/kotlin/com/flipcash/services/internal/network/extensions/ProtobufToLocal.kt b/services/flipcash/src/main/kotlin/com/flipcash/services/internal/network/extensions/ProtobufToLocal.kt index 70bc3912f6..0bccc3a3cc 100644 --- a/services/flipcash/src/main/kotlin/com/flipcash/services/internal/network/extensions/ProtobufToLocal.kt +++ b/services/flipcash/src/main/kotlin/com/flipcash/services/internal/network/extensions/ProtobufToLocal.kt @@ -214,6 +214,12 @@ internal fun com.codeinc.flipcash.gen.blob.v1.Model.BlobMetadata.toBlobMetadata( sizeBytes = sizeBytes, downloadUrl = downloadUrl.url, image = if (hasImage()) image.toImageMetadata() else null, + // Carried, not dropped: this metadata gets persisted (profile pictures live in + // `user_profiles`), so without the expiry a stored copy hands Coil a URL that has long + // since 403'd, with nothing able to tell that it should re-resolve the id first. + expiresAtMillis = downloadUrl.takeIf { it.hasExpiresAt() } + ?.expiresAt + ?.let { it.seconds * 1_000 + it.nanos / 1_000_000 }, ) } @@ -377,7 +383,12 @@ internal fun ChatModel.Metadata.toChatMetadata(): ChatMetadata { phoneNumber = phoneNumber.value.takeIf { it.isNotEmpty() }?.let { VerifiableContactMethod(it, verified = true) }, email = emailAddress.value.takeIf { it.isNotEmpty() }?.let { VerifiableContactMethod(it, verified = true) }, profilePicture = if (hasProfilePicture()) profilePicture.toMediaItem() else null, - userId = if (hasUserId()) userId.toId() else null, + // Falls back to the member's own id: the server sets it on the member but + // usually not again inside the nested profile, and this profile is by + // definition that member's. Dropping it here leaves callers unable to name + // the profile that authorizes re-minting the picture's download URL, so the + // avatar can never recover once the stored URL expires. + userId = if (hasUserId()) userId.toId() else member.userId.toId(), username = if (hasUsername()) username.value else null, ) }, diff --git a/services/flipcash/src/main/kotlin/com/flipcash/services/internal/network/services/BlobStorageService.kt b/services/flipcash/src/main/kotlin/com/flipcash/services/internal/network/services/BlobStorageService.kt index cee1b0696e..3d19b913cc 100644 --- a/services/flipcash/src/main/kotlin/com/flipcash/services/internal/network/services/BlobStorageService.kt +++ b/services/flipcash/src/main/kotlin/com/flipcash/services/internal/network/services/BlobStorageService.kt @@ -12,6 +12,7 @@ import com.flipcash.services.models.GetUploadPolicyError import com.flipcash.services.models.InitiateExternalUploadError import com.flipcash.services.models.blob.UploadPolicy import com.flipcash.services.models.blob.UploadReservation +import com.flipcash.services.models.chat.BlobAccessContext import com.flipcash.services.models.chat.BlobId import com.flipcash.services.models.chat.BlobState import com.flipcash.services.models.chat.BlobStatus @@ -101,8 +102,9 @@ internal class BlobStorageService @Inject constructor( suspend fun getBlobs( blobIds: List, owner: Ed25519.KeyPair, + context: BlobAccessContext, ): Result> { - return runCatching { api.getBlobs(blobIds, owner) } + return runCatching { api.getBlobs(blobIds, owner, context) } .foldWithSuppression( onSuccess = { response -> when (response.result) { diff --git a/services/flipcash/src/main/kotlin/com/flipcash/services/internal/repositories/InternalBlobStorageRepository.kt b/services/flipcash/src/main/kotlin/com/flipcash/services/internal/repositories/InternalBlobStorageRepository.kt index 95dfba528d..67bf8d0069 100644 --- a/services/flipcash/src/main/kotlin/com/flipcash/services/internal/repositories/InternalBlobStorageRepository.kt +++ b/services/flipcash/src/main/kotlin/com/flipcash/services/internal/repositories/InternalBlobStorageRepository.kt @@ -3,6 +3,7 @@ package com.flipcash.services.internal.repositories import com.flipcash.services.internal.network.services.BlobStorageService import com.flipcash.services.models.blob.UploadPolicy import com.flipcash.services.models.blob.UploadReservation +import com.flipcash.services.models.chat.BlobAccessContext import com.flipcash.services.models.chat.BlobId import com.flipcash.services.models.chat.BlobState import com.flipcash.services.models.chat.BlobStatus @@ -34,6 +35,7 @@ internal class InternalBlobStorageRepository( override suspend fun getBlobs( blobIds: List, owner: Ed25519.KeyPair, - ): Result> = service.getBlobs(blobIds, owner) + context: BlobAccessContext, + ): Result> = service.getBlobs(blobIds, owner, context) .onFailure { ErrorUtils.handleError(it) } } diff --git a/services/flipcash/src/main/kotlin/com/flipcash/services/models/chat/BlobAccessContext.kt b/services/flipcash/src/main/kotlin/com/flipcash/services/models/chat/BlobAccessContext.kt new file mode 100644 index 0000000000..046d7c8a51 --- /dev/null +++ b/services/flipcash/src/main/kotlin/com/flipcash/services/models/chat/BlobAccessContext.kt @@ -0,0 +1,35 @@ +package com.flipcash.services.models.chat + +import com.getcode.opencode.model.core.ID + +/** + * The surface a caller is reading blobs from, used to authorize `GetBlobs`. + * + * Blobs the caller owns resolve on their own, but an id the caller does *not* own is treated as + * unauthorized and silently omitted from the response unless a context that grants it is supplied. + * Re-minting another user's avatar therefore fails closed — an empty response, not an error — so + * every re-mint of someone else's media has to name the surface it is being read from. + */ +sealed interface BlobAccessContext { + /** The caller owns these blobs (their own profile picture, their own uploads). */ + data object Owned : BlobAccessContext + + /** + * Read from [userId]'s public profile. Grants only the renditions of that user's *current* + * profile picture — a superseded picture stops resolving through it. + */ + data class Profile(val userId: ID) : BlobAccessContext + + /** Read from within [chatId]. Granted iff the caller is a member and the blob was shared into it. */ + data class Chat(val chatId: ChatId) : BlobAccessContext + + companion object { + /** + * [Profile] for [userId], falling back to [Owned] when the id isn't known. The fallback is + * for surfaces that hold a picture without the profile it belongs to; it resolves the + * caller's own blobs and quietly resolves nothing for anyone else's, which is the same + * outcome as passing no context at all. + */ + fun profile(userId: ID?): BlobAccessContext = userId?.let(::Profile) ?: Owned + } +} diff --git a/services/flipcash/src/main/kotlin/com/flipcash/services/models/chat/BlobMetadata.kt b/services/flipcash/src/main/kotlin/com/flipcash/services/models/chat/BlobMetadata.kt index aee4821ecd..e8c716ed51 100644 --- a/services/flipcash/src/main/kotlin/com/flipcash/services/models/chat/BlobMetadata.kt +++ b/services/flipcash/src/main/kotlin/com/flipcash/services/models/chat/BlobMetadata.kt @@ -1,6 +1,9 @@ package com.flipcash.services.models.chat import android.os.Parcelable +import kotlin.time.Duration +import kotlin.time.Duration.Companion.seconds +import kotlin.time.Instant import kotlinx.parcelize.Parcelize import kotlinx.serialization.Serializable @@ -11,4 +14,40 @@ data class BlobMetadata( val sizeBytes: Long, val downloadUrl: String, val image: ImageMetadata?, -): Parcelable + /** + * When [downloadUrl] stops working, as epoch milliseconds — null for metadata that predates + * this field (already-persisted rows) or a server that didn't send one. + * + * Every other field here is intrinsic to the immutable bytes; this one is not. The server + * mints [downloadUrl] per fetch and expires it, so a metadata copy that outlives its expiry — + * and these are persisted, see `user_profiles.profile_picture_json` — carries a URL that 403s. + * Past this instant the id must be re-resolved through `GetBlobs` for a fresh URL. + * + * Stored as millis rather than an `Instant` so the type stays Parcelable and its persisted + * JSON stays a primitive. + */ + val expiresAtMillis: Long? = null, +): Parcelable { + + val expiresAt: Instant? + get() = expiresAtMillis?.let { Instant.fromEpochMilliseconds(it) } + + /** + * Whether [downloadUrl] is past — or within [margin] of — its expiry at [now], and so must be + * re-minted before use. Metadata carrying no expiry is treated as usable: it either predates + * the field or the server declined to bound it, and failing closed there would re-resolve + * every blob on every load. + */ + fun isDownloadUrlExpired(now: Instant, margin: Duration = EXPIRY_MARGIN): Boolean { + val expiry = expiresAt ?: return false + return now + margin >= expiry + } + + companion object { + /** + * Treat a URL as already dead this far ahead of its stated expiry, so one that would die + * mid-download doesn't produce a load error we'd have to recover from. + */ + val EXPIRY_MARGIN: Duration = 30.seconds + } +} diff --git a/services/flipcash/src/main/kotlin/com/flipcash/services/models/chat/MediaItem.kt b/services/flipcash/src/main/kotlin/com/flipcash/services/models/chat/MediaItem.kt index bfc7261ad5..105ce557a3 100644 --- a/services/flipcash/src/main/kotlin/com/flipcash/services/models/chat/MediaItem.kt +++ b/services/flipcash/src/main/kotlin/com/flipcash/services/models/chat/MediaItem.kt @@ -1,7 +1,7 @@ package com.flipcash.services.models.chat import android.os.Parcelable -import com.getcode.utils.base58 +import kotlin.time.Instant import kotlinx.parcelize.Parcelize import kotlinx.serialization.Serializable @@ -91,6 +91,31 @@ data class MediaItem( fun blurhash(): String? = renditions.firstNotNullOfOrNull { it.blob?.image?.blurhash?.takeIf(String::isNotEmpty) } + /** + * The rendition [renditionForSize] resolves to, if its download URL has expired at [now] and + * so needs re-minting through `GetBlobs` before it can be fetched. Null when the URL is still + * good, when there is no rendition for the size, or when the metadata carries no expiry. + */ + fun expiredRenditionForSize(targetLongestSidePx: Int, now: Instant): MediaItemRendition? = + renditionForSize(targetLongestSidePx)?.takeIf { it.isDownloadUrlExpired(now) } + + /** + * A copy with every rendition whose [MediaItemRendition.cacheKey] appears in [fresh] carrying + * that re-minted metadata instead. Renditions absent from [fresh] are left untouched, so a + * partial refresh (only the sizes a surface actually needed) is safe to apply. + * + * Keyed by the base58 cache key rather than [BlobId]: BlobId wraps a ByteArray, so its + * equality is referential and it cannot be used as a map key. + */ + fun withRefreshedBlobs(fresh: Map): MediaItem { + if (fresh.isEmpty()) return this + return copy( + renditions = renditions.map { rendition -> + fresh[rendition.cacheKey]?.let { rendition.copy(blob = it) } ?: rendition + } + ) + } + /** Available, non-ORIGINAL renditions paired with their longest side, in list order. */ private fun sizedRenditions(): List> = renditions @@ -108,9 +133,5 @@ data class MediaItem( /** Longest image side (px) of a rendition, or null if it has no image dimensions. */ private val MediaItemRendition.longestSide: Int? get() = blob?.image?.let { maxOf(it.width, it.height) } - - /** Stable, URL-independent cache key for a rendition — its durable blob id, base58-encoded. */ - private val MediaItemRendition.cacheKey: String - get() = blobId.bytes.base58 } } diff --git a/services/flipcash/src/main/kotlin/com/flipcash/services/models/chat/MediaItemRendition.kt b/services/flipcash/src/main/kotlin/com/flipcash/services/models/chat/MediaItemRendition.kt index f355a5bfe8..00ee018398 100644 --- a/services/flipcash/src/main/kotlin/com/flipcash/services/models/chat/MediaItemRendition.kt +++ b/services/flipcash/src/main/kotlin/com/flipcash/services/models/chat/MediaItemRendition.kt @@ -1,6 +1,8 @@ package com.flipcash.services.models.chat import android.os.Parcelable +import com.getcode.utils.base58 +import kotlin.time.Instant import kotlinx.parcelize.Parcelize import kotlinx.serialization.Serializable @@ -17,4 +19,17 @@ data class MediaItemRendition( DISPLAY, THUMBNAIL, } + + /** + * Stable, URL-independent image-cache key — the durable blob id, base58-encoded. The bytes a + * blob id addresses never change, while `download_url` is re-minted per fetch, so keying a + * cache on the URL misses every time even though the bytes are already held. + */ + // Computed, not a stored property: a backing field here would be written by both Parcelize + // and the persisted JSON for a value fully derived from blobId. + val cacheKey: String + get() = blobId.bytes.base58 + + /** Whether this rendition's download URL needs re-minting before use at [now]. */ + fun isDownloadUrlExpired(now: Instant): Boolean = blob?.isDownloadUrlExpired(now) ?: false } diff --git a/services/flipcash/src/main/kotlin/com/flipcash/services/repository/BlobStorageRepository.kt b/services/flipcash/src/main/kotlin/com/flipcash/services/repository/BlobStorageRepository.kt index 3ad63c036a..05c6675946 100644 --- a/services/flipcash/src/main/kotlin/com/flipcash/services/repository/BlobStorageRepository.kt +++ b/services/flipcash/src/main/kotlin/com/flipcash/services/repository/BlobStorageRepository.kt @@ -2,6 +2,7 @@ package com.flipcash.services.repository import com.flipcash.services.models.blob.UploadPolicy import com.flipcash.services.models.blob.UploadReservation +import com.flipcash.services.models.chat.BlobAccessContext import com.flipcash.services.models.chat.BlobId import com.flipcash.services.models.chat.BlobState import com.flipcash.services.models.chat.BlobStatus @@ -18,5 +19,9 @@ interface BlobStorageRepository { suspend fun completeExternalUpload(blobId: BlobId, owner: Ed25519.KeyPair): Result - suspend fun getBlobs(blobIds: List, owner: Ed25519.KeyPair): Result> + suspend fun getBlobs( + blobIds: List, + owner: Ed25519.KeyPair, + context: BlobAccessContext, + ): Result> } diff --git a/services/flipcash/src/test/kotlin/com/flipcash/services/controllers/BlobStorageControllerTest.kt b/services/flipcash/src/test/kotlin/com/flipcash/services/controllers/BlobStorageControllerTest.kt index a7e7cfcad8..5c6fc3f3c9 100644 --- a/services/flipcash/src/test/kotlin/com/flipcash/services/controllers/BlobStorageControllerTest.kt +++ b/services/flipcash/src/test/kotlin/com/flipcash/services/controllers/BlobStorageControllerTest.kt @@ -81,7 +81,7 @@ class BlobStorageControllerTest { fun `upload returns the blob id once READY`() = runTest { stubOwner() happyPathStubs() - coEvery { repository.getBlobs(any(), any()) } returns Result.success(listOf(readyBlob())) + coEvery { repository.getBlobs(any(), any(), any()) } returns Result.success(listOf(readyBlob())) val result = controller.upload(byteArrayOf(1, 2, 3), "image/png") @@ -92,7 +92,7 @@ class BlobStorageControllerTest { fun `upload polls until the blob becomes READY`() = runTest { stubOwner() happyPathStubs() - coEvery { repository.getBlobs(any(), any()) } returnsMany listOf( + coEvery { repository.getBlobs(any(), any(), any()) } returnsMany listOf( // Non-terminal polls resolve to an empty list; the controller keeps polling. Result.success(emptyList()), Result.success(emptyList()), @@ -102,14 +102,14 @@ class BlobStorageControllerTest { val result = controller.upload(byteArrayOf(1, 2, 3), "image/png") assertEquals(blobId, result.getOrNull()) - coVerify(exactly = 3) { repository.getBlobs(any(), any()) } + coVerify(exactly = 3) { repository.getBlobs(any(), any(), any()) } } @Test fun `upload fails with BlobRejectedException when the blob is REJECTED`() = runTest { stubOwner() happyPathStubs() - coEvery { repository.getBlobs(any(), any()) } returns Result.success(listOf(rejectedBlob())) + coEvery { repository.getBlobs(any(), any(), any()) } returns Result.success(listOf(rejectedBlob())) val result = controller.upload(byteArrayOf(1, 2, 3), "image/png") @@ -121,7 +121,7 @@ class BlobStorageControllerTest { stubOwner() happyPathStubs() // Never terminal — every poll resolves to an empty list until the timeout trips. - coEvery { repository.getBlobs(any(), any()) } returns Result.success(emptyList()) + coEvery { repository.getBlobs(any(), any(), any()) } returns Result.success(emptyList()) val result = controller.upload(byteArrayOf(1, 2, 3), "image/png") @@ -161,7 +161,7 @@ class BlobStorageControllerTest { coEvery { uploader.upload(any(), any(), any()) } returns Result.success(Unit) coEvery { repository.completeExternalUpload(any(), any()) } returns Result.failure(RuntimeException("complete failed")) - coEvery { repository.getBlobs(any(), any()) } returns Result.success(listOf(readyBlob())) + coEvery { repository.getBlobs(any(), any(), any()) } returns Result.success(listOf(readyBlob())) val result = controller.upload(byteArrayOf(1, 2, 3), "image/png") diff --git a/services/flipcash/src/test/kotlin/com/flipcash/services/internal/domain/ChatMetadataMapperTest.kt b/services/flipcash/src/test/kotlin/com/flipcash/services/internal/domain/ChatMetadataMapperTest.kt index c438a2db95..0833759ccd 100644 --- a/services/flipcash/src/test/kotlin/com/flipcash/services/internal/domain/ChatMetadataMapperTest.kt +++ b/services/flipcash/src/test/kotlin/com/flipcash/services/internal/domain/ChatMetadataMapperTest.kt @@ -111,6 +111,31 @@ class ChatMetadataMapperTest { assertEquals(5L, result.members[0].pointers[0].value) } + @Test + fun `member profile takes the member's user id when the nested profile omits it`() { + val result = mapper.map(metadata { addMembers(member(userIdByte = 9)) }) + assertEquals( + ByteArray(16) { 9 }.toList(), + result.members[0].userProfile.userId, + ) + } + + @Test + fun `member profile keeps its own user id when the server sets one`() { + val withProfileId = ChatModel.Member.newBuilder(member(userIdByte = 9)) + .setUserProfile( + ProfileModel.UserProfile.newBuilder() + .setDisplayName("User") + .setUserId(userId(3)) + ) + .build() + val result = mapper.map(metadata { addMembers(withProfileId) }) + assertEquals( + ByteArray(16) { 3 }.toList(), + result.members[0].userProfile.userId, + ) + } + @Test fun `maps last message when present`() { val result = mapper.map(metadata { setLastMessage(message(text = "hey")) }) diff --git a/services/flipcash/src/test/kotlin/com/flipcash/services/internal/network/extensions/ChatMetadataExtensionTest.kt b/services/flipcash/src/test/kotlin/com/flipcash/services/internal/network/extensions/ChatMetadataExtensionTest.kt new file mode 100644 index 0000000000..508d6ad5a2 --- /dev/null +++ b/services/flipcash/src/test/kotlin/com/flipcash/services/internal/network/extensions/ChatMetadataExtensionTest.kt @@ -0,0 +1,60 @@ +package com.flipcash.services.internal.network.extensions + +import com.codeinc.flipcash.gen.chat.v1.Model as ChatModel +import com.codeinc.flipcash.gen.common.v1.Common +import com.codeinc.flipcash.gen.profile.v1.Model as ProfileModel +import com.google.protobuf.ByteString +import com.google.protobuf.Timestamp +import org.junit.Test +import kotlin.test.assertEquals + +/** + * The member id is what authorizes re-minting a profile picture's expired download URL, and the + * server sets it on the chat member rather than inside the nested profile. + */ +class ChatMetadataExtensionTest { + + private fun userId(byte: Byte): Common.UserId = + Common.UserId.newBuilder() + .setValue(ByteString.copyFrom(ByteArray(16) { byte })) + .build() + + private fun metadata(member: ChatModel.Member): ChatModel.Metadata = + ChatModel.Metadata.newBuilder() + .setChatId( + Common.ChatId.newBuilder() + .setValue(ByteString.copyFrom(ByteArray(32) { 1 })) + ) + .setType(ChatModel.ChatType.CONTACT_DM) + .setLastActivity(Timestamp.newBuilder().setSeconds(2000)) + .addMembers(member) + .build() + + @Test + fun `member profile takes the member's user id when the nested profile omits it`() { + val member = ChatModel.Member.newBuilder() + .setUserId(userId(9)) + .setUserProfile(ProfileModel.UserProfile.newBuilder().setDisplayName("User")) + .build() + + val result = metadata(member).toChatMetadata() + + assertEquals(ByteArray(16) { 9 }.toList(), result.members[0].userProfile.userId) + } + + @Test + fun `member profile keeps its own user id when the server sets one`() { + val member = ChatModel.Member.newBuilder() + .setUserId(userId(9)) + .setUserProfile( + ProfileModel.UserProfile.newBuilder() + .setDisplayName("User") + .setUserId(userId(3)) + ) + .build() + + val result = metadata(member).toChatMetadata() + + assertEquals(ByteArray(16) { 3 }.toList(), result.members[0].userProfile.userId) + } +} diff --git a/services/flipcash/src/test/kotlin/com/flipcash/services/models/chat/BlobExpiryTest.kt b/services/flipcash/src/test/kotlin/com/flipcash/services/models/chat/BlobExpiryTest.kt new file mode 100644 index 0000000000..f034dc59c0 --- /dev/null +++ b/services/flipcash/src/test/kotlin/com/flipcash/services/models/chat/BlobExpiryTest.kt @@ -0,0 +1,116 @@ +package com.flipcash.services.models.chat + +import com.getcode.utils.base58 +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertNull +import kotlin.test.assertSame +import kotlin.test.assertTrue +import kotlin.time.Duration.Companion.minutes +import kotlin.time.Duration.Companion.seconds +import kotlin.time.Instant + +/** + * `DownloadUrl.expires_at` is what tells a persisted [MediaItem] that its URL needs re-minting + * before it can be fetched again — these cover the model's half of that. + */ +class BlobExpiryTest { + + private val now = Instant.fromEpochMilliseconds(1_700_000_000_000) + + private fun blob(url: String, expiresAt: Instant?) = BlobMetadata( + mimeType = "image/png", + sizeBytes = 1, + downloadUrl = url, + image = ImageMetadata(width = 160, height = 160, blurhash = "abc"), + expiresAtMillis = expiresAt?.toEpochMilliseconds(), + ) + + private fun rendition(id: Byte, longestSide: Int, expiresAt: Instant?) = MediaItemRendition( + role = MediaItemRendition.Role.THUMBNAIL, + blobId = BlobId(byteArrayOf(id)), + blob = BlobMetadata( + mimeType = "image/png", + sizeBytes = 1, + downloadUrl = "https://cdn/$id", + image = ImageMetadata(width = longestSide, height = longestSide, blurhash = "abc"), + expiresAtMillis = expiresAt?.toEpochMilliseconds(), + ), + ) + + @Test + fun `metadata without an expiry is never reported expired`() { + assertFalse(blob("https://cdn/a", expiresAt = null).isDownloadUrlExpired(now)) + } + + @Test + fun `an expiry comfortably ahead of now is not expired`() { + assertFalse(blob("https://cdn/a", now + 5.minutes).isDownloadUrlExpired(now)) + } + + @Test + fun `an expiry in the past is expired`() { + assertTrue(blob("https://cdn/a", now - 1.seconds).isDownloadUrlExpired(now)) + } + + @Test + fun `an expiry inside the margin is expired, so a URL cannot die mid-flight`() { + val insideMargin = now + BlobMetadata.EXPIRY_MARGIN - 1.seconds + assertTrue(blob("https://cdn/a", insideMargin).isDownloadUrlExpired(now)) + + val outsideMargin = now + BlobMetadata.EXPIRY_MARGIN + 1.seconds + assertFalse(blob("https://cdn/a", outsideMargin).isDownloadUrlExpired(now)) + } + + @Test + fun `expiresAt round-trips the stored millis`() { + assertEquals(now, blob("https://cdn/a", now).expiresAt) + assertNull(blob("https://cdn/a", null).expiresAt) + } + + @Test + fun `cacheKey is the base58 blob id, so it survives a re-mint`() { + val id = BlobId(byteArrayOf(7, 8, 9)) + val before = MediaItemRendition(MediaItemRendition.Role.THUMBNAIL, id, blob("https://cdn/a", now)) + val after = before.copy(blob = blob("https://cdn/b", now + 1.minutes)) + assertEquals(id.bytes.base58, before.cacheKey) + assertEquals(before.cacheKey, after.cacheKey) + } + + @Test + fun `expiredRenditionForSize only reports the rendition the size actually resolves to`() { + // The 160 the small surfaces use is still good; the 320 the large ones use has expired. + val item = MediaItem( + listOf( + rendition(id = 1, longestSide = 160, expiresAt = now + 5.minutes), + rendition(id = 2, longestSide = 320, expiresAt = now - 5.minutes), + ) + ) + assertNull(item.expiredRenditionForSize(96, now)) + assertEquals(byteArrayOf(2).base58, item.expiredRenditionForSize(300, now)?.cacheKey) + } + + @Test + fun `withRefreshedBlobs swaps only the renditions it was given`() { + val item = MediaItem( + listOf( + rendition(id = 1, longestSide = 160, expiresAt = now - 5.minutes), + rendition(id = 2, longestSide = 320, expiresAt = now - 5.minutes), + ) + ) + val fresh = blob("https://cdn/2-fresh", now + 10.minutes) + + val refreshed = item.withRefreshedBlobs(mapOf(byteArrayOf(2).base58 to fresh)) + + assertEquals("https://cdn/1", refreshed.urlForSize(96)) + assertEquals("https://cdn/2-fresh", refreshed.urlForSize(300)) + assertFalse(refreshed.renditionForSize(300)!!.isDownloadUrlExpired(now)) + } + + @Test + fun `withRefreshedBlobs on an empty map is the same instance`() { + val item = MediaItem(listOf(rendition(id = 1, longestSide = 160, expiresAt = null))) + assertSame(item, item.withRefreshedBlobs(emptyMap())) + } +} From 5facbefbb7bc45118c0fdab35781ad60fa6adae4 Mon Sep 17 00:00:00 2001 From: Brandon McAnsh Date: Thu, 3 Sep 2026 22:20:26 -0400 Subject: [PATCH 2/3] fix(blobs): validate the blob request with its access context attached flipcash2-client-protocol 0.4.1 regenerates the validators with protovalidate-kt 0.1.2, which checks a oneof arm's required rule only when that arm is selected. AccessContext can pass client-side validation now, so the request no longer has to be validated before the context is attached to avoid it. The test pins that: a profile scope and a chat scope each validate, and an unset scope does not. All three fail against 0.4.0. --- gradle/libs.versions.toml | 4 +- .../internal/network/api/BlobStorageApi.kt | 17 ++----- .../api/BlobAccessContextValidationTest.kt | 50 +++++++++++++++++++ 3 files changed, 56 insertions(+), 15 deletions(-) create mode 100644 services/flipcash/src/test/kotlin/com/flipcash/services/internal/network/api/BlobAccessContextValidationTest.kt diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 035cc07257..13da67359c 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -61,7 +61,7 @@ grpc = "1.83.1" grpc-okhttp = "1.83.1" grpc-kotlin = "1.5.0" protobuf = "4.36.0" -protovalidate-kt = "0.1.1" +protovalidate-kt = "0.1.2" # Generated client SDKs for the two backend contracts. Separate versions on purpose: the # protos are independent (flipcash2 does not import ocp), so the packages move on their own @@ -69,7 +69,7 @@ protovalidate-kt = "0.1.1" # 0.3.0 is the first release of either package to ship R8 keep rules for its generated # messages, which is what lets proguard-rules.pro drop its own. ocp-client-protocol = "0.3.0" -flipcash2-client-protocol = "0.4.0" +flipcash2-client-protocol = "0.4.1" # The Android port is the ONLY libphonenumber this app depends on, deliberately. Google's # `com.googlecode` artifact used to sit alongside it; the two ship separate copies of the metadata, 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 3fda74bd2a..92a664e084 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 @@ -92,24 +92,15 @@ internal class BlobStorageApi @Inject constructor( .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() - // Validated before the context is attached, because AccessContext cannot pass client-side - // validation: the generated validator asserts BOTH oneof arms are set - // (`checkRequired(scopeCase == CHAT, "chat")` and the same for `profile`), so any scope - // fails as `context.chat: value is required`. The rule the proto declares — required *if - // selected* — is what the server enforces, and every other rule on the request is still - // checked here. Drop this split once the validator generator handles oneof members. request.validate().orThrow() - // 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. - val authorized = context.toProto() - ?.let { request.toBuilder().setContext(it).build() } - ?: request - return withContext(Dispatchers.IO) { - api.getBlobs(authorized) + api.getBlobs(request) } } diff --git a/services/flipcash/src/test/kotlin/com/flipcash/services/internal/network/api/BlobAccessContextValidationTest.kt b/services/flipcash/src/test/kotlin/com/flipcash/services/internal/network/api/BlobAccessContextValidationTest.kt new file mode 100644 index 0000000000..d1a9822c52 --- /dev/null +++ b/services/flipcash/src/test/kotlin/com/flipcash/services/internal/network/api/BlobAccessContextValidationTest.kt @@ -0,0 +1,50 @@ +package com.flipcash.services.internal.network.api + +import com.codeinc.flipcash.gen.blob.v1.Model +import com.codeinc.flipcash.gen.blob.v1.validate +import com.flipcash.services.internal.network.extensions.asChatId +import com.flipcash.services.internal.network.extensions.asUserId +import com.flipcash.services.models.chat.ChatId +import dev.bmcreations.protovalidate.ValidationResult +import org.junit.Test +import kotlin.test.assertEquals + +/** + * `AccessContext.scope` is a oneof whose arms are each declared `required`, which the generated + * validator got wrong until protovalidate-kt 0.1.2: it asserted every arm was the selected one, so + * whichever arm you set, the others failed as `value is required` and no context could be sent. + * These pin the behaviour the client relies on — [BlobStorageApi.getBlobs] validates the request + * with the context already attached. + */ +class BlobAccessContextValidationTest { + + private fun userId(byte: Byte = 2) = ByteArray(16) { byte }.toList() + + private fun chatId(byte: Byte = 1) = ChatId(ByteArray(32) { byte }) + + @Test + fun `a profile scope validates`() { + val context = Model.AccessContext.newBuilder() + .setProfile(userId().asUserId()) + .build() + + assertEquals(ValidationResult.Valid, context.validate()) + } + + @Test + fun `a chat scope validates`() { + val context = Model.AccessContext.newBuilder() + .setChat(chatId().asChatId()) + .build() + + assertEquals(ValidationResult.Valid, context.validate()) + } + + @Test + fun `an unset scope does not validate`() { + val context = Model.AccessContext.newBuilder().build() + + val result = context.validate() + assertEquals(true, result is ValidationResult.Invalid) + } +} From fd48d4dae2bf569851ab43941b965e7a21192891 Mon Sep 17 00:00:00 2001 From: Brandon McAnsh Date: Thu, 3 Sep 2026 22:25:28 -0400 Subject: [PATCH 3/3] test(chat): assert the selected bubble by value, not identity Gating the actions on the edit and delete windows (#1403) made the ToggleMessageSelection reducer re-narrow the bubble it selects, so the selection is a copy of the bubble the event carried. Three assertSame checks were pinning the old identity and have failed on code/cash since that merge, taking down the messenger module's test task on every PR. The narrowing is a no-op under MessagePolicy.Default, whose windows are both null, so the copy is value-equal and assertEquals asserts what the tests meant. --- .../messenger/internal/ChatMessageActionReducerTest.kt | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/apps/flipcash/features/messenger/src/test/kotlin/com/flipcash/app/messenger/internal/ChatMessageActionReducerTest.kt b/apps/flipcash/features/messenger/src/test/kotlin/com/flipcash/app/messenger/internal/ChatMessageActionReducerTest.kt index 4a22f139b8..fdbeadda33 100644 --- a/apps/flipcash/features/messenger/src/test/kotlin/com/flipcash/app/messenger/internal/ChatMessageActionReducerTest.kt +++ b/apps/flipcash/features/messenger/src/test/kotlin/com/flipcash/app/messenger/internal/ChatMessageActionReducerTest.kt @@ -9,13 +9,15 @@ import kotlin.test.assertEquals import kotlin.test.assertFalse import kotlin.test.assertNotNull import kotlin.test.assertNull -import kotlin.test.assertSame import kotlin.test.assertTrue import kotlin.time.Instant /** * The selection bar and the composer takeover are both plain state, so the reducer is where their * rules live: one message selected at a time, and an edit that never costs the user their draft. + * + * Selection is asserted by value, not identity: the reducer re-narrows the bubble's capabilities to + * the windows open now, so what it stores is a copy of the bubble it was handed. */ class ChatMessageActionReducerTest { @@ -52,7 +54,7 @@ class ChatMessageActionReducerTest { ChatViewModel.Event.ToggleMessageSelection(target), ) - assertSame(target, state.selection) + assertEquals(target, state.selection) assertEquals(target.capabilities, state.selectionCapabilities) } @@ -81,7 +83,7 @@ class ChatMessageActionReducerTest { val state = reduce(selected, ChatViewModel.Event.ToggleMessageSelection(second)) - assertSame(second, state.selection) + assertEquals(second, state.selection) } @Test @@ -108,7 +110,7 @@ class ChatMessageActionReducerTest { val state = reduce(selected, ChatViewModel.Event.DeleteMessage(target.messageId)) - assertSame(target, state.selection) + assertEquals(target, state.selection) assertTrue(state.confirmingDelete) }