Fix media viewer for medias not related to events displaying invalid state (#7280)

* When no URL is provided to `ContentValidationState.collectMediaState`, assume it's valid

That way we can do:

`if (validationState.isValid() && state.collectMediaState(possiblyNullUrl).isValid()) ...`

* Remove the `eventId` param from `ContentScannerService.scan`

It's not really used, since we then provide a state that can be mutated, and fetching that is what the previous parameter was used for

* Make sure to always validate the medias in the viewer, both the actual media and the thumbnail

* Update screenshots

---------

Co-authored-by: ElementBot <android@element.io>
This commit is contained in:
Jorge Martin Espinosa
2026-07-27 18:16:35 +02:00
committed by GitHub
parent d8b01cf3e7
commit 0442fbd307
24 changed files with 92 additions and 65 deletions
@@ -7,17 +7,16 @@
package io.element.android.features.contentscanner.api
import io.element.android.libraries.matrix.api.core.EventId
import io.element.android.libraries.matrix.api.media.MediaSource
import io.element.android.libraries.matrix.api.scanner.ContentScanner
import io.element.android.libraries.matrix.ui.media.contentvalidation.ContentValidationState
/**
* Service to perform security scans in the contents for a given event and media source.
* Service to perform security scans in the contents for a given media source.
*
* The default implementation (FOSS) always returns a valid state.
* This will only process media when a [ContentScanner] is provided when building the client in Pro.
*/
fun interface ContentScannerService {
fun scan(eventId: EventId, mediaSources: List<MediaSource>, contentValidationState: ContentValidationState)
fun scan(mediaSources: List<MediaSource>, contentValidationState: ContentValidationState)
}
@@ -9,7 +9,6 @@ package io.element.android.features.contentscanner.impl
import io.element.android.features.contentscanner.api.ContentScannerService
import io.element.android.libraries.core.coroutine.CoroutineDispatchers
import io.element.android.libraries.matrix.api.core.EventId
import io.element.android.libraries.matrix.api.exception.ClientException
import io.element.android.libraries.matrix.api.media.MediaSource
import io.element.android.libraries.matrix.api.scanner.ContentScanner
@@ -29,7 +28,7 @@ class DefaultContentScannerService(
) : ContentScannerService {
private val dispatcher = coroutineDispatchers.io.limitedParallelism(4)
override fun scan(eventId: EventId, mediaSources: List<MediaSource>, contentValidationState: ContentValidationState) {
override fun scan(mediaSources: List<MediaSource>, contentValidationState: ContentValidationState) {
for (mediaSource in mediaSources) {
val url = mediaSource.safeUrl
val currentState = contentValidationState.getCurrentMediaState(url)
@@ -8,7 +8,6 @@
package io.element.android.features.contentscanner.impl.di
import io.element.android.features.contentscanner.api.ContentScannerService
import io.element.android.libraries.matrix.api.core.EventId
import io.element.android.libraries.matrix.api.media.MediaSource
import io.element.android.libraries.matrix.ui.media.contentvalidation.ContentValidationState
import io.element.android.libraries.matrix.ui.media.contentvalidation.ContentValidationValue
@@ -19,7 +18,7 @@ import io.element.android.libraries.matrix.ui.media.contentvalidation.ContentVal
* This is used when the content scanner feature is not enabled or available.
*/
class AlwaysValidContentScannerService : ContentScannerService {
override fun scan(eventId: EventId, mediaSources: List<MediaSource>, contentValidationState: ContentValidationState) {
override fun scan(mediaSources: List<MediaSource>, contentValidationState: ContentValidationState) {
// Always return success for the noop implementation
for (mediaSource in mediaSources) {
val url = mediaSource.safeUrl
@@ -11,7 +11,6 @@ import com.google.common.truth.Truth.assertThat
import io.element.android.libraries.core.coroutine.CoroutineDispatchers
import io.element.android.libraries.matrix.api.media.MediaSource
import io.element.android.libraries.matrix.api.scanner.ContentScanner
import io.element.android.libraries.matrix.test.AN_EVENT_ID
import io.element.android.libraries.matrix.test.media.aMediaSource
import io.element.android.libraries.matrix.test.scanner.FakeContentScanner
import io.element.android.libraries.matrix.ui.media.contentvalidation.ContentValidationValue
@@ -39,7 +38,6 @@ class DefaultContentScannerServiceTest {
createDefaultContentScannerService(
contentScanner = scanner,
).scan(
eventId = AN_EVENT_ID,
mediaSources = emptyList(),
contentValidationState = contentValidationState,
)
@@ -58,7 +56,6 @@ class DefaultContentScannerServiceTest {
createDefaultContentScannerService(
contentScanner = scanner,
).scan(
eventId = AN_EVENT_ID,
mediaSources = listOf(mediaSource),
contentValidationState = contentValidationState,
)
@@ -87,7 +84,6 @@ class DefaultContentScannerServiceTest {
createDefaultContentScannerService(
contentScanner = scanner,
).scan(
eventId = AN_EVENT_ID,
mediaSources = listOf(mediaSourceA, mediaSourceB),
contentValidationState = contentValidationState,
)
@@ -113,14 +109,12 @@ class DefaultContentScannerServiceTest {
// First scan attempt
service.scan(
eventId = AN_EVENT_ID,
mediaSources = listOf(mediaSource),
contentValidationState = contentValidationState,
)
// Immediately after, a new scan attempt is made for the same media source
service.scan(
eventId = AN_EVENT_ID,
mediaSources = listOf(mediaSource),
contentValidationState = contentValidationState,
)
@@ -144,7 +138,6 @@ class DefaultContentScannerServiceTest {
)
service.scan(
eventId = AN_EVENT_ID,
mediaSources = listOf(mediaSource),
contentValidationState = contentValidationState,
)
@@ -166,7 +159,6 @@ class DefaultContentScannerServiceTest {
)
service.scan(
eventId = AN_EVENT_ID,
mediaSources = listOf(mediaSource),
contentValidationState = contentValidationState,
)
@@ -191,7 +183,6 @@ class DefaultContentScannerServiceTest {
)
service.scan(
eventId = AN_EVENT_ID,
mediaSources = listOf(mediaSource),
contentValidationState = contentValidationState,
)
@@ -858,7 +858,7 @@ class MessageComposerPresenter(
if (currentComposerMode is MessageComposerMode.Reply) {
val mediaSources = currentComposerMode.replyToDetails.content()?.mediaSources() ?: return@launch
val contentValidationState = contentValidationCache[currentComposerMode.replyToDetails.eventId()]
contentScannerService.scan(currentComposerMode.replyToDetails.eventId(), mediaSources, contentValidationState)
contentScannerService.scan(mediaSources, contentValidationState)
}
// When coming from edit, just clear the composer as it'd be weird to reset a volatile draft in this scenario.
if (currentComposerMode.isEditing) {
@@ -259,7 +259,7 @@ class TimelinePresenter(
)
}
is TimelineEvent.ValidateMedia -> {
timelineProtectionState.eventSink(TimelineProtectionEvent.ValidateContent(event.eventId, event.mediaSources, event.validationState))
timelineProtectionState.eventSink(TimelineProtectionEvent.ValidateContent(event.mediaSources, event.validationState))
}
}
}
@@ -15,7 +15,6 @@ import io.element.android.libraries.matrix.ui.media.contentvalidation.ContentVal
sealed interface TimelineProtectionEvent {
data class ShowContent(val eventId: EventId?) : TimelineProtectionEvent
data class ValidateContent(
val eventId: EventId,
val mediaSources: List<MediaSource>,
val validationState: ContentValidationState,
) : TimelineProtectionEvent
@@ -59,7 +59,7 @@ class TimelineProtectionPresenter(
allowedEvents += setOfNotNull(event.eventId)
}
is TimelineProtectionEvent.ValidateContent -> {
contentScannerService.scan(event.eventId, event.mediaSources, event.validationState)
contentScannerService.scan(event.mediaSources, event.validationState)
}
}
}
@@ -316,7 +316,7 @@ class MessageComposerPresenterSlashCommandTest {
notificationConversationService = notificationConversationService,
slashCommandService = slashCommandService,
featureFlagService = featureFlagService,
contentScannerService = { _, _, _ -> },
contentScannerService = { _, _ -> },
contentValidationCache = InMemoryEventContentValidationCache(),
).apply {
isTesting = true
@@ -1650,7 +1650,7 @@ class MessageComposerPresenterTest : RobolectricTest() {
notificationConversationService = notificationConversationService,
slashCommandService = slashCommandService,
featureFlagService = featureFlagService,
contentScannerService = { _, _, _ -> },
contentScannerService = { _, _ -> },
contentValidationCache = InMemoryEventContentValidationCache(),
).apply {
isTesting = true
@@ -10,7 +10,6 @@ package io.element.android.features.messages.impl.timeline.protection
import com.google.common.truth.Truth.assertThat
import io.element.android.features.contentscanner.api.ContentScannerService
import io.element.android.libraries.matrix.api.core.EventId
import io.element.android.libraries.matrix.api.media.MediaPreviewConfig
import io.element.android.libraries.matrix.api.media.MediaPreviewService
import io.element.android.libraries.matrix.api.media.MediaPreviewValue
@@ -104,7 +103,7 @@ class TimelineProtectionPresenterTest {
val mediaPreviewConfig = MediaPreviewConfig(mediaPreviewValue = MediaPreviewValue.Private, hideInviteAvatar = false)
val mediaPreviewService = FakeMediaPreviewService(mediaPreviewConfigFlow = MutableStateFlow(mediaPreviewConfig))
val room = FakeBaseRoom(initialRoomInfo = aRoomInfo(joinRule = JoinRule.Invite), roomCoroutineScope = backgroundScope)
val contentScannerService = lambdaRecorder { _: EventId, _: List<MediaSource>, state: ContentValidationState ->
val contentScannerService = lambdaRecorder { _: List<MediaSource>, state: ContentValidationState ->
state.update(url, ContentValidationValue.Valid)
}
val presenter = createPresenter(
@@ -119,7 +118,6 @@ class TimelineProtectionPresenterTest {
val initialState = awaitItem()
initialState.eventSink(
TimelineProtectionEvent.ValidateContent(
eventId = AN_EVENT_ID,
mediaSources = listOf(MediaSource(url)),
validationState = validationState
)
@@ -135,7 +133,7 @@ class TimelineProtectionPresenterTest {
private fun createPresenter(
room: BaseRoom = FakeBaseRoom(),
mediaPreviewService: MediaPreviewService = FakeMediaPreviewService(),
contentScannerService: ContentScannerService = ContentScannerService { _: EventId, _: List<MediaSource>, _: ContentValidationState -> },
contentScannerService: ContentScannerService = ContentScannerService { _: List<MediaSource>, _: ContentValidationState -> },
) = TimelineProtectionPresenter(
mediaPreviewService = mediaPreviewService,
room = room,
@@ -189,7 +189,7 @@ fun ContentValidationState.collectOverallState(): State<ContentValidationValue>
@Composable
fun ContentValidationState.collectMediaState(url: String?): State<ContentValidationValue> {
if (url == null) {
return remember { mutableStateOf(ContentValidationValue.Unknown) }
return remember { mutableStateOf(ContentValidationValue.Valid) }
}
return produceState(getCurrentMediaState(url), key1 = url) {
@@ -84,9 +84,8 @@ class MediaGalleryPresenter(
if (groupedItems is AsyncData.Success) {
val items = groupedItems.data.getItems(mode)
items.forEach { item ->
if (item is MediaItem.Event) {
val eventId = item.eventId() ?: return@forEach
coroutineScope.validateMedia(eventId, item)
if (item is MediaItem.Event && item.eventId() != null) {
coroutineScope.validateMedia(item)
}
}
}
@@ -237,7 +236,7 @@ class MediaGalleryPresenter(
}
}
private fun CoroutineScope.validateMedia(eventId: EventId, mediaItem: MediaItem.Event) {
private fun CoroutineScope.validateMedia(mediaItem: MediaItem.Event) {
launch {
val thumbnailSource = mediaItem.thumbnailSource()
val mediaSource = mediaItem.mediaSource()
@@ -248,7 +247,7 @@ class MediaGalleryPresenter(
return@launch
}
contentScannerService.scan(eventId, listOfNotNull(thumbnailSource, mediaSource).distinct(), contentValidationState)
contentScannerService.scan(listOfNotNull(thumbnailSource, mediaSource).distinct(), contentValidationState)
}
}
@@ -242,18 +242,15 @@ class MediaViewerDataSource(
}
}
fun validateMedia(
eventId: EventId,
mediaSource: MediaSource,
) {
fun validateMedia(mediaSource: MediaSource, thumbnailMediaSource: MediaSource?) {
val validationState = mediaValidationState[mediaSource.safeUrl] ?: return
val currentState = validationState.getCurrentMediaState(mediaSource.safeUrl)
val currentState = validationState.getCurrentOverallState()
if (currentState.isLoading() || currentState.isValid()) {
return
}
contentScannerService.scan(eventId, listOf(mediaSource), validationState)
contentScannerService.scan(listOfNotNull(mediaSource, thumbnailMediaSource), validationState)
}
fun cancelLoadingMedia(data: MediaViewerPageData.MediaViewerData) {
@@ -31,5 +31,5 @@ sealed interface MediaViewerEvent {
data class OnNavigateTo(val index: Int) : MediaViewerEvent
data class LoadMore(val direction: Timeline.PaginationDirection) : MediaViewerEvent
data class CancelLoadingMedia(val data: MediaViewerPageData.MediaViewerData) : MediaViewerEvent
data class ValidateMedia(val eventId: EventId, val mediaSource: MediaSource) : MediaViewerEvent
data class ValidateMedia(val mediaSource: MediaSource, val thumbnailMediaSource: MediaSource?) : MediaViewerEvent
}
@@ -194,7 +194,7 @@ class MediaViewerPresenter(
is MediaViewerEvent.LoadMore -> coroutineScope.launch {
dataSource.loadMore(event.direction)
}
is MediaViewerEvent.ValidateMedia -> dataSource.validateMedia(event.eventId, event.mediaSource)
is MediaViewerEvent.ValidateMedia -> dataSource.validateMedia(event.mediaSource, event.thumbnailMediaSource)
}
}
@@ -95,6 +95,7 @@ import io.element.android.libraries.designsystem.utils.snackbar.SnackbarHost
import io.element.android.libraries.designsystem.utils.snackbar.rememberSnackbarHostState
import io.element.android.libraries.matrix.api.media.MediaSource
import io.element.android.libraries.matrix.ui.media.MediaRequestData
import io.element.android.libraries.matrix.ui.media.contentvalidation.collectMediaState
import io.element.android.libraries.mediaviewer.api.MediaInfo
import io.element.android.libraries.mediaviewer.api.local.LocalMedia
import io.element.android.libraries.mediaviewer.impl.details.MediaBottomSheetState
@@ -217,20 +218,24 @@ fun MediaViewerView(
)
}
is MediaViewerPageData.MediaViewerData -> {
val eventId = dataForPage.eventId
val isMediaValid = dataForPage.validationState.getCurrentMediaState(dataForPage.mediaSource.safeUrl).isValid()
// We need to check the 2 validation states and can't use the overall state because these might be part of a gallery event
// If that was the case, the overall state would be invalid if any of the sources for any media in the gallery was invalid, e.g.:
// Media 1: mediaSource = valid, thumbnailSource = valid
// Media 2: mediaSource = invalid, thumbnailSource = valid
// The overall state would be invalid, so the whole gallery event and all its medias would be considered invalid.
val mediaValidationState by dataForPage.validationState.collectMediaState(dataForPage.mediaSource.safeUrl)
val thumbnailValidationState by dataForPage.validationState.collectMediaState(dataForPage.thumbnailSource?.safeUrl)
// Check if the media to display is valid or dangerous
LaunchedEffect(eventId) {
if (eventId != null) {
state.eventSink(MediaViewerEvent.ValidateMedia(eventId, dataForPage.mediaSource))
}
LaunchedEffect(Unit) {
state.eventSink(
MediaViewerEvent.ValidateMedia(mediaSource = dataForPage.mediaSource, thumbnailMediaSource = dataForPage.thumbnailSource)
)
}
var bottomPaddingInPixels by remember { mutableIntStateOf(defaultBottomPaddingInPixels) }
val loadMediaOnVisibilityChangedModifier = remember(isMediaValid) {
if (isMediaValid == true) {
val loadMediaOnVisibilityChangedModifier = remember(mediaValidationState) {
if (mediaValidationState.isValid() && thumbnailValidationState.isValid()) {
Modifier.onVisibilityChanged(minDurationMs = 200L) { isVisible ->
if (isVisible) {
state.eventSink(MediaViewerEvent.LoadMedia(dataForPage))
@@ -383,6 +388,8 @@ private fun MediaViewerPage(
) {
val downloadedMedia by data.downloadedMedia
val showProgress = rememberShowProgress(downloadedMedia)
val mediaValidationState by data.validationState.collectMediaState(data.mediaSource.safeUrl)
val thumbnailValidationState by data.validationState.collectMediaState(data.thumbnailSource?.safeUrl)
Box(
modifier = Modifier
@@ -390,7 +397,7 @@ private fun MediaViewerPage(
.navigationBarsPadding()
) {
AnimatedVisibility(
visible = data.validationState.getCurrentMediaState(data.mediaSource.safeUrl).isLoading(),
visible = mediaValidationState.isLoading(),
modifier = Modifier.padding(top = containerPadding.calculateTopPadding()).align(Alignment.TopCenter),
enter = fadeIn(spring(stiffness = 500F)),
exit = fadeOut(spring(stiffness = 500F)),
@@ -423,20 +430,18 @@ private fun MediaViewerPage(
}
}
val isMediaValid = data.validationState.getCurrentMediaState(data.mediaSource.safeUrl).isValid()
if (isMediaValid == false) {
if (mediaValidationState.isInvalid() || thumbnailValidationState.isInvalid()) {
Column(
modifier = Modifier.fillMaxSize(),
verticalArrangement = Arrangement.Center,
) {
IconTitleSubtitleMolecule(
iconStyle = BigIcon.Style.AlertSolid,
title = "The file is not safe",
subTitle = "Preview and download have been disabled.",
title = stringResource(CommonStrings.content_scanner_unsafe_title),
subTitle = stringResource(CommonStrings.content_scanner_unsafe_message),
)
}
} else {
} else if (mediaValidationState.isValid() && thumbnailValidationState.isValid()) {
LocalMediaView(
modifier = Modifier.fillMaxSize(),
isDisplayed = isDisplayed,
@@ -462,6 +467,7 @@ private fun MediaViewerPage(
)
}
}
if (showError) {
ErrorView(
errorMessage = stringResource(id = CommonStrings.error_unknown),
@@ -74,7 +74,7 @@ class DefaultMediaViewerEntryPointTest {
sessionId = A_SESSION_ID,
enterpriseService = FakeEnterpriseService(),
contentValidationCache = NoopEventContentValidationCache(),
contentScannerService = ContentScannerService { _, _, _ -> },
contentScannerService = ContentScannerService { _, _ -> },
)
}
val callback = object : MediaViewerEntryPoint.Callback {
@@ -123,7 +123,7 @@ class DefaultMediaViewerEntryPointTest {
sessionId = A_SESSION_ID,
enterpriseService = FakeEnterpriseService(),
contentValidationCache = NoopEventContentValidationCache(),
contentScannerService = ContentScannerService { _, _, _ -> },
contentScannerService = ContentScannerService { _, _ -> },
)
}
val callback = object : MediaViewerEntryPoint.Callback {
@@ -534,7 +534,7 @@ class MediaGalleryPresenterTest {
mediaLoader = matrixMediaLoader,
localMediaActions = localMediaActions,
snackbarDispatcher = snackbarDispatcher,
contentScannerService = { _, _, _ -> },
contentScannerService = { _, _ -> },
)
}
}
@@ -291,7 +291,7 @@ class MediaViewerDataSourceTest {
localMediaFactory = localMediaFactory,
systemClock = FakeSystemClock(),
pagerKeysHandler = PagerKeysHandler(),
contentScannerService = { _, _, _ -> },
contentScannerService = { _, _ -> },
contentValidationCache = NoopEventContentValidationCache(),
)
}
@@ -970,7 +970,7 @@ internal fun TestScope.createMediaViewerPresenter(
localMediaFactory = localMediaFactory,
systemClock = FakeSystemClock(),
pagerKeysHandler = PagerKeysHandler(),
contentScannerService = { _, _, _ -> },
contentScannerService = { _, _ -> },
contentValidationCache = NoopEventContentValidationCache(),
),
room = room,
@@ -47,6 +47,24 @@ import kotlin.time.Duration.Companion.milliseconds
class MediaViewerViewTest : RobolectricTest() {
private val mockMediaUrl: Uri = mockk("localMediaUri")
@Test
fun `displaying a media will automatically validate it`() = runAndroidComposeUiTest {
val eventsRecorder = EventsRecorder<MediaViewerEvent>()
val pagerData = aMediaViewerPageData(downloadedMedia = AsyncData.Success(aLocalMedia(uri = mockMediaUrl)))
val state = aMediaViewerState(
listData = listOf(pagerData),
eventSink = eventsRecorder
)
setMediaViewerView(
state = state,
)
eventsRecorder.assertList(
listOf(
MediaViewerEvent.ValidateMedia(mediaSource = pagerData.mediaSource, thumbnailMediaSource = pagerData.thumbnailSource),
)
)
}
@Test
fun `clicking on back invokes expected callback`() = runAndroidComposeUiTest {
val eventsRecorder = EventsRecorder<MediaViewerEvent>()
@@ -59,6 +77,9 @@ class MediaViewerViewTest : RobolectricTest() {
onBackClick = callback,
)
// Remove the `ValidateMedia` event that is emitted when the media is loaded, since we are not testing it here.
eventsRecorder.clear()
// Wait for enough time for the onVisibilityChanged modifier to trigger
mainClock.advanceTimeBy(200)
@@ -120,6 +141,9 @@ class MediaViewerViewTest : RobolectricTest() {
),
)
// Remove the `ValidateMedia` event that is emitted when the media is loaded, since we are not testing it here.
eventsRecorder.clear()
// Wait for enough time for the onVisibilityChanged modifier to trigger
mainClock.advanceTimeBy(200)
@@ -157,9 +181,11 @@ class MediaViewerViewTest : RobolectricTest() {
eventSink = eventsRecorder
),
)
clickOn(textRes)
eventsRecorder.assertList(
listOf(
MediaViewerEvent.ValidateMedia(mediaSource = data.mediaSource, thumbnailMediaSource = data.thumbnailSource),
MediaViewerEvent.LoadMedia(data),
expectedEvent,
)
@@ -175,6 +201,10 @@ class MediaViewerViewTest : RobolectricTest() {
setMediaViewerView(
state = state,
)
// Remove the `ValidateMedia` event that is emitted when the media is loaded, since we are not testing it here.
eventsRecorder.clear()
// Ensure that the action are visible
val resources = activity!!.resources
val contentDescription = resources.getString(CommonStrings.action_share)
@@ -205,6 +235,10 @@ class MediaViewerViewTest : RobolectricTest() {
state = state,
onBackClick = callback,
)
// Remove the `ValidateMedia` event that is emitted when the media is loaded, since we are not testing it here.
eventsRecorder.clear()
val imageContentDescription = activity!!.getString(CommonStrings.common_image)
onNodeWithContentDescription(imageContentDescription).performTouchInput { swipeDown(startY = centerY) }
mainClock.advanceTimeBy(1_000)
@@ -229,6 +263,9 @@ class MediaViewerViewTest : RobolectricTest() {
),
)
// Remove the `ValidateMedia` event that is emitted when the media is loaded, since we are not testing it here.
eventsRecorder.clear()
// Wait for enough time for the onVisibilityChanged modifier to trigger
mainClock.advanceTimeBy(200)
@@ -254,6 +291,9 @@ class MediaViewerViewTest : RobolectricTest() {
),
)
// Remove the `ValidateMedia` event that is emitted when the media is loaded, since we are not testing it here.
eventsRecorder.clear()
// Wait for enough time for the onVisibilityChanged modifier to trigger
mainClock.advanceTimeBy(200)