Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: strip metadata from profile pictures (WPB-11170) #3590

Merged
merged 18 commits into from
Nov 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@

package com.wire.android.ui.userprofile.avatarpicker

import android.content.Context
import android.net.Uri
import androidx.compose.foundation.background
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
Expand All @@ -33,10 +31,8 @@ import androidx.compose.material3.HorizontalDivider
import androidx.compose.material3.MaterialTheme
import androidx.compose.runtime.Composable
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.rememberCoroutineScope
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.platform.LocalContext
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.unit.dp
import androidx.core.net.toUri
Expand Down Expand Up @@ -65,13 +61,6 @@ import com.wire.android.ui.common.visbility.rememberVisibilityState
import com.wire.android.ui.home.conversations.PermissionPermanentlyDeniedDialogState
import com.wire.android.ui.theme.wireColorScheme
import com.wire.android.ui.userprofile.avatarpicker.AvatarPickerViewModel.PictureState
import com.wire.android.util.ImageUtil
import com.wire.android.util.resampleImageAndCopyToTempPath
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
import okio.Path

@RootNavGraph
@WireDestination
Expand All @@ -84,18 +73,15 @@ fun AvatarPickerScreen(
val permissionPermanentlyDeniedDialogState =
rememberVisibilityState<PermissionPermanentlyDeniedDialogState>()

val context = LocalContext.current

val targetAvatarPath = viewModel.defaultAvatarPath
val targetAvatarUri = viewModel.temporaryAvatarUri

val scope = rememberCoroutineScope()
val state = rememberAvatarPickerState(
onImageSelected = { originalUri ->
onNewAvatarPicked(originalUri, targetAvatarPath, scope, context, viewModel)
viewModel.updatePickedAvatarUri(originalUri, targetAvatarPath.toFile().toUri())
},
onPictureTaken = {
onNewAvatarPicked(targetAvatarUri, targetAvatarPath, scope, context, viewModel)
viewModel.updatePickedAvatarUri(targetAvatarUri, targetAvatarPath.toFile().toUri())
},
targetPictureFileUri = targetAvatarUri,
onGalleryPermissionPermanentlyDenied = {
Expand Down Expand Up @@ -141,19 +127,6 @@ fun AvatarPickerScreen(
)
}

// TODO: Mateusz: I think we should refactor this, it takes some values from the ViewModel, part of the logic is executed inside
// the UI, part of the logic is exectued inside the ViewModel, I see no reasons to handle the logic inside the UI
// personally it was a confusing part for me to read when investing the bugs, unless there is a valid reason to move the logic to the UI
// that I am not aware of ?
fun onNewAvatarPicked(originalUri: Uri, targetAvatarPath: Path, scope: CoroutineScope, context: Context, viewModel: AvatarPickerViewModel) {
scope.launch {
sanitizeAvatarImage(originalUri, targetAvatarPath, context)
withContext(Dispatchers.Main) {
viewModel.updatePickedAvatarUri(targetAvatarPath.toFile().toUri())
}
}
}

@Composable
private fun AvatarPickerContent(
pictureState: PictureState,
Expand Down Expand Up @@ -287,7 +260,3 @@ private fun AvatarPickerTopBar(onCloseClick: () -> Unit) {
title = stringResource(R.string.profile_image_top_bar_label),
)
}

private suspend fun sanitizeAvatarImage(originalAvatarUri: Uri, avatarPath: Path, appContext: Context) {
originalAvatarUri.resampleImageAndCopyToTempPath(appContext, avatarPath, ImageUtil.ImageSizeClass.Small)
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ import com.wire.android.appLogger
import com.wire.android.datastore.UserDataStore
import com.wire.android.model.SnackBarMessage
import com.wire.android.util.AvatarImageManager
import com.wire.android.util.ImageUtil
import com.wire.android.util.dispatchers.DispatcherProvider
import com.wire.android.util.resampleImageAndCopyToTempPath
import com.wire.android.util.toByteArray
import com.wire.android.util.ui.UIText
import com.wire.kalium.logic.NetworkFailure
Expand Down Expand Up @@ -104,10 +106,24 @@ class AvatarPickerViewModel @Inject constructor(
}
}

fun updatePickedAvatarUri(updatedUri: Uri) = viewModelScope.launch(dispatchers.main()) {
fun updatePickedAvatarUri(originalUri: Uri, updatedUri: Uri) = viewModelScope.launch {
sanitizeAvatarImage(originalUri, defaultAvatarPath)
pictureState = PictureState.Picked(updatedUri)
}

/**
* Resamples the image and removes unnecessary metadata before uploading it.
* This to avoid uploading unnecessarily large images for profile pictures and sensitive metadata.
*/
private suspend fun sanitizeAvatarImage(originalAvatarUri: Uri, avatarPath: Path) {
originalAvatarUri.resampleImageAndCopyToTempPath(
context = appContext,
tempCachePath = avatarPath,
sizeClass = ImageUtil.ImageSizeClass.Small,
shouldRemoveMetadata = true
)
}

fun uploadNewPickedAvatar(onComplete: (avatarAssetId: String?) -> Unit) {
val imgUri = pictureState.avatarUri

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,9 @@ import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.setValue
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import com.wire.android.BuildConfig
import com.wire.android.appLogger
import com.wire.android.datastore.GlobalDataStore
import com.wire.android.datastore.UserDataStore
import com.wire.android.di.AuthServerConfigProvider
import com.wire.android.di.CurrentAccount
import com.wire.android.feature.AccountSwitchUseCase
import com.wire.android.feature.SwitchAccountActions
Expand All @@ -42,7 +40,6 @@ import com.wire.android.ui.legalhold.banner.LegalHoldUIState
import com.wire.android.ui.userprofile.self.dialog.StatusDialogData
import com.wire.android.util.dispatchers.DispatcherProvider
import com.wire.android.util.ui.WireSessionImageLoader
import com.wire.kalium.logic.configuration.server.ServerConfig
import com.wire.kalium.logic.data.call.Call
import com.wire.kalium.logic.data.id.QualifiedIdMapper
import com.wire.kalium.logic.data.id.toQualifiedID
Expand All @@ -61,7 +58,6 @@ import com.wire.kalium.logic.feature.team.GetUpdatedSelfTeamUseCase
import com.wire.kalium.logic.feature.user.GetSelfUserUseCase
import com.wire.kalium.logic.feature.user.IsReadOnlyAccountUseCase
import com.wire.kalium.logic.feature.user.ObserveValidAccountsUseCase
import com.wire.kalium.logic.feature.user.SelfServerConfigUseCase
import com.wire.kalium.logic.feature.user.UpdateSelfAvailabilityStatusUseCase
import com.wire.kalium.logic.functional.getOrNull
import dagger.hilt.android.lifecycle.HiltViewModel
Expand Down Expand Up @@ -94,8 +90,6 @@ class SelfUserProfileViewModel @Inject constructor(
private val observeLegalHoldStatusForSelfUser: ObserveLegalHoldStateForSelfUserUseCase,
private val dispatchers: DispatcherProvider,
private val wireSessionImageLoader: WireSessionImageLoader,
private val authServerConfigProvider: AuthServerConfigProvider,
private val selfServerLinks: SelfServerConfigUseCase,
private val otherAccountMapper: OtherAccountMapper,
private val observeEstablishedCalls: ObserveEstablishedCallsUseCase,
private val accountSwitch: AccountSwitchUseCase,
Expand Down Expand Up @@ -266,27 +260,6 @@ class SelfUserProfileViewModel @Inject constructor(
}
}

// todo. cleanup unused code
fun tryToInitAddingAccount(onSucceeded: () -> Unit) {
viewModelScope.launch {
// the total number of accounts is otherAccounts + 1 for the current account
val canAddNewAccounts: Boolean = (userProfileState.otherAccounts.size + 1) < BuildConfig.MAX_ACCOUNTS

if (!canAddNewAccounts) {
userProfileState = userProfileState.copy(maxAccountsReached = true)
return@launch
}

val selfServerLinks: ServerConfig.Links =
when (val result = selfServerLinks()) {
is SelfServerConfigUseCase.Result.Failure -> return@launch
is SelfServerConfigUseCase.Result.Success -> result.serverLinks.links
}
authServerConfigProvider.updateAuthServer(selfServerLinks)
onSucceeded()
}
}

fun dismissStatusDialog() {
userProfileState = userProfileState.copy(statusDialogData = null)
}
Expand Down Expand Up @@ -321,11 +294,6 @@ class SelfUserProfileViewModel @Inject constructor(
}
}

// todo. cleanup unused code
fun onMaxAccountReachedDialogDismissed() {
userProfileState = userProfileState.copy(maxAccountsReached = false)
}

private fun setNotShowStatusRationaleAgainIfNeeded(status: UserAvailabilityStatus) {
userProfileState.statusDialogData.let { dialogState ->
if (dialogState?.isCheckBoxChecked == true) {
Expand Down
Loading
Loading