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

Fix empty verification bottom sheet #7130

Merged
merged 4 commits into from
Sep 16, 2022
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
1 change: 1 addition & 0 deletions changelog.d/7130.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix empty verification bottom sheet.
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,13 @@ fun Session.getRoomSummary(roomIdOrAlias: String): RoomSummary? = roomService().

/**
* Get a user using the UserService of a Session.
* @param userId the userId to look for.
* @return a user with userId or null if the User is not known yet by the SDK.
* See [org.matrix.android.sdk.api.session.user.UserService.resolveUser] to ensure that a User is retrieved.
*/
fun Session.getUser(userId: String): User? = userService().getUser(userId)

/**
* Similar to [getUser], but fallback to a User without details if the User is not known by the SDK, or if Session is null.
*/
fun Session?.getUserOrDefault(userId: String): User = this?.userService()?.getUser(userId) ?: User(userId)
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ interface UserService {
/**
* Get a user from a userId.
* @param userId the userId to look for.
* @return a user with userId or null
* @return a user with userId or null if the User is not known yet by the SDK. See [resolveUser] to ensure that a User is retrieved.
*/
fun getUser(userId: String): User?

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,10 @@ import kotlinx.coroutines.launch
import org.matrix.android.sdk.api.extensions.orFalse
import org.matrix.android.sdk.api.raw.RawService
import org.matrix.android.sdk.api.session.Session
import org.matrix.android.sdk.api.session.getUser
import org.matrix.android.sdk.api.session.getUserOrDefault
import org.matrix.android.sdk.api.session.permalinks.PermalinkData
import org.matrix.android.sdk.api.session.permalinks.PermalinkParser
import org.matrix.android.sdk.api.session.room.model.create.CreateRoomParams
import org.matrix.android.sdk.api.session.user.model.User

class CreateDirectRoomViewModel @AssistedInject constructor(
@Assisted initialState: CreateDirectRoomViewState,
Expand Down Expand Up @@ -78,11 +77,7 @@ class CreateDirectRoomViewModel @AssistedInject constructor(
_viewEvents.post(CreateDirectRoomViewEvents.DmSelf)
} else {
// Try to get user from known users and fall back to creating a User object from MXID
val qrInvitee = if (session.getUser(mxid) != null) {
session.getUser(mxid)!!
} else {
User(mxid, null, null)
}
val qrInvitee = session.getUserOrDefault(mxid)
onSubmitInvitees(setOf(PendingSelection.UserPendingSelection(qrInvitee)))
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import org.matrix.android.sdk.api.session.crypto.verification.PendingVerificatio
import org.matrix.android.sdk.api.session.crypto.verification.VerificationService
import org.matrix.android.sdk.api.session.crypto.verification.VerificationTransaction
import org.matrix.android.sdk.api.session.crypto.verification.VerificationTxState
import org.matrix.android.sdk.api.session.getUserOrDefault
import org.matrix.android.sdk.api.util.toMatrixItem
import timber.log.Timber
import javax.inject.Inject
Expand Down Expand Up @@ -67,8 +68,8 @@ class IncomingVerificationRequestHandler @Inject constructor(
when (tx.state) {
is VerificationTxState.OnStarted -> {
// Add a notification for every incoming request
val user = session?.userService()?.getUser(tx.otherUserId)
val name = user?.toMatrixItem()?.getBestName() ?: tx.otherUserId
val user = session.getUserOrDefault(tx.otherUserId).toMatrixItem()
val name = user.getBestName()
val alert = VerificationVectorAlert(
uid,
context.getString(R.string.sas_incoming_request_notif_title),
Expand All @@ -86,7 +87,7 @@ class IncomingVerificationRequestHandler @Inject constructor(
}
)
.apply {
viewBinder = VerificationVectorAlert.ViewBinder(user?.toMatrixItem(), avatarRenderer.get())
viewBinder = VerificationVectorAlert.ViewBinder(user, avatarRenderer.get())
contentAction = Runnable {
(weakCurrentActivity?.get() as? VectorBaseActivity<*>)?.let {
it.navigator.performDeviceVerification(it, tx.otherUserId, tx.transactionId)
Expand Down Expand Up @@ -131,8 +132,8 @@ class IncomingVerificationRequestHandler @Inject constructor(
// XXX this is a bit hard coded :/
popupAlertManager.cancelAlert("review_login")
}
val user = session?.userService()?.getUser(pr.otherUserId)?.toMatrixItem()
val name = user?.getBestName() ?: pr.otherUserId
val user = session.getUserOrDefault(pr.otherUserId).toMatrixItem()
val name = user.getBestName()
val description = if (name == pr.otherUserId) {
name
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,29 +152,25 @@ class VerificationBottomSheet : VectorBaseBottomSheetDialogFragment<BottomSheetV
}

override fun invalidate() = withState(viewModel) { state ->
state.otherUserMxItem?.let { matrixItem ->
if (state.isMe) {
avatarRenderer.render(matrixItem, views.otherUserAvatarImageView)
if (state.sasTransactionState == VerificationTxState.Verified ||
state.qrTransactionState == VerificationTxState.Verified ||
state.verifiedFromPrivateKeys) {
views.otherUserShield.render(RoomEncryptionTrustLevel.Trusted)
} else {
views.otherUserShield.render(RoomEncryptionTrustLevel.Warning)
}
views.otherUserNameText.text = getString(
if (state.selfVerificationMode) R.string.crosssigning_verify_this_session else R.string.crosssigning_verify_session
)
avatarRenderer.render(state.otherUserMxItem, views.otherUserAvatarImageView)
if (state.isMe) {
if (state.sasTransactionState == VerificationTxState.Verified ||
state.qrTransactionState == VerificationTxState.Verified ||
state.verifiedFromPrivateKeys) {
views.otherUserShield.render(RoomEncryptionTrustLevel.Trusted)
} else {
avatarRenderer.render(matrixItem, views.otherUserAvatarImageView)

if (state.sasTransactionState == VerificationTxState.Verified || state.qrTransactionState == VerificationTxState.Verified) {
views.otherUserNameText.text = getString(R.string.verification_verified_user, matrixItem.getBestName())
views.otherUserShield.render(RoomEncryptionTrustLevel.Trusted)
} else {
views.otherUserNameText.text = getString(R.string.verification_verify_user, matrixItem.getBestName())
views.otherUserShield.render(null)
}
views.otherUserShield.render(RoomEncryptionTrustLevel.Warning)
}
views.otherUserNameText.text = getString(
if (state.selfVerificationMode) R.string.crosssigning_verify_this_session else R.string.crosssigning_verify_session
)
} else {
if (state.sasTransactionState == VerificationTxState.Verified || state.qrTransactionState == VerificationTxState.Verified) {
views.otherUserNameText.text = getString(R.string.verification_verified_user, state.otherUserMxItem.getBestName())
views.otherUserShield.render(RoomEncryptionTrustLevel.Trusted)
} else {
views.otherUserNameText.text = getString(R.string.verification_verify_user, state.otherUserMxItem.getBestName())
views.otherUserShield.render(null)
}
}

Expand Down Expand Up @@ -235,7 +231,7 @@ class VerificationBottomSheet : VectorBaseBottomSheetDialogFragment<BottomSheetV
showFragment(
VerificationEmojiCodeFragment::class,
VerificationArgs(
state.otherUserMxItem?.id ?: "",
state.otherUserId,
// If it was outgoing it.transaction id would be null, but the pending request
// would be updated (from localId to txId)
state.pendingRequest.invoke()?.transactionId ?: state.transactionId
Expand Down Expand Up @@ -271,7 +267,7 @@ class VerificationBottomSheet : VectorBaseBottomSheetDialogFragment<BottomSheetV
VerificationQRWaitingFragment::class,
VerificationQRWaitingFragment.Args(
isMe = state.isMe,
otherUserName = state.otherUserMxItem?.getBestName() ?: ""
otherUserName = state.otherUserMxItem.getBestName()
)
)
return@withState
Expand Down Expand Up @@ -319,7 +315,7 @@ class VerificationBottomSheet : VectorBaseBottomSheetDialogFragment<BottomSheetV
showFragment(
VerificationChooseMethodFragment::class,
VerificationArgs(
otherUserId = state.otherUserMxItem?.id ?: "",
otherUserId = state.otherUserId,
verificationId = state.pendingRequest.invoke()?.transactionId
)
)
Expand All @@ -328,7 +324,7 @@ class VerificationBottomSheet : VectorBaseBottomSheetDialogFragment<BottomSheetV
showFragment(
VerificationRequestFragment::class,
VerificationArgs(
otherUserId = state.otherUserMxItem?.id ?: "",
otherUserId = state.otherUserId,
verificationId = state.pendingRequest.invoke()?.transactionId,
verificationLocalId = state.roomId
)
Expand All @@ -340,7 +336,7 @@ class VerificationBottomSheet : VectorBaseBottomSheetDialogFragment<BottomSheetV
showFragment(
VerificationChooseMethodFragment::class,
VerificationArgs(
otherUserId = state.otherUserMxItem?.id ?: "",
otherUserId = state.otherUserId,
verificationId = state.pendingRequest.invoke()?.transactionId
)
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import kotlinx.coroutines.launch
import org.matrix.android.sdk.api.Matrix
import org.matrix.android.sdk.api.MatrixCallback
import org.matrix.android.sdk.api.extensions.orFalse
import org.matrix.android.sdk.api.extensions.tryOrNull
import org.matrix.android.sdk.api.raw.RawService
import org.matrix.android.sdk.api.session.Session
import org.matrix.android.sdk.api.session.crypto.crosssigning.KEYBACKUP_SECRET_SSSS_NAME
Expand Down Expand Up @@ -70,7 +71,7 @@ data class VerificationBottomSheetViewState(
val roomId: String?,
// true when we display the loading and we wait for the other (incoming request)
val selfVerificationMode: Boolean,
val otherUserMxItem: MatrixItem? = null,
val otherUserMxItem: MatrixItem,
val pendingRequest: Async<PendingVerificationRequest> = Uninitialized,
val pendingLocalId: String? = null,
val sasTransactionState: VerificationTxState? = null,
Expand All @@ -92,7 +93,8 @@ data class VerificationBottomSheetViewState(
otherUserId = args.otherUserId,
verificationId = args.verificationId,
roomId = args.roomId,
selfVerificationMode = args.selfVerificationMode
selfVerificationMode = args.selfVerificationMode,
otherUserMxItem = MatrixItem.UserItem(args.otherUserId),
)
}

Expand Down Expand Up @@ -126,7 +128,7 @@ class VerificationBottomSheetViewModel @AssistedInject constructor(
}
}

val userItem = session.getUser(initialState.otherUserId)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A null userItem was the source of the issue.

fetchOtherUserProfile(initialState.otherUserId)

var autoReady = false
val pr = if (initialState.selfVerificationMode) {
Expand Down Expand Up @@ -160,7 +162,6 @@ class VerificationBottomSheetViewModel @AssistedInject constructor(

setState {
copy(
otherUserMxItem = userItem?.toMatrixItem(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is set in the constructor and updated with full data in fetchOtherUserProfile

sasTransactionState = sasTx?.state,
qrTransactionState = qrTx?.state,
transactionId = pr?.transactionId ?: initialState.verificationId,
Expand All @@ -183,6 +184,28 @@ class VerificationBottomSheetViewModel @AssistedInject constructor(
}
}

private fun fetchOtherUserProfile(otherUserId: String) {
session.getUser(otherUserId)?.toMatrixItem()?.let {
setState {
copy(
otherUserMxItem = it
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First change to set known data

)
}
}
// Always fetch the latest User data
viewModelScope.launch {
tryOrNull { session.userService().resolveUser(otherUserId) }
?.toMatrixItem()
?.let {
setState {
copy(
otherUserMxItem = it
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

second change to set up to date data, if possible (network available + API is returning data)

)
}
}
}
}

override fun onCleared() {
session.cryptoService().verificationService().removeListener(this)
super.onCleared()
Expand Down Expand Up @@ -216,12 +239,12 @@ class VerificationBottomSheetViewModel @AssistedInject constructor(

private fun cancelAllPendingVerifications(state: VerificationBottomSheetViewState) {
session.cryptoService()
.verificationService().getExistingVerificationRequest(state.otherUserMxItem?.id ?: "", state.transactionId)?.let {
Copy link
Member Author

@bmarty bmarty Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defaulting to "" everywhere was pretty bad...

.verificationService().getExistingVerificationRequest(state.otherUserId, state.transactionId)?.let {
session.cryptoService().verificationService().cancelVerificationRequest(it)
}
session.cryptoService()
.verificationService()
.getExistingTransaction(state.otherUserMxItem?.id ?: "", state.transactionId ?: "")
.getExistingTransaction(state.otherUserId, state.transactionId ?: "")
?.cancel(CancelCode.User)
}

Expand Down Expand Up @@ -249,7 +272,7 @@ class VerificationBottomSheetViewModel @AssistedInject constructor(
}

override fun handle(action: VerificationAction) = withState { state ->
val otherUserId = state.otherUserMxItem?.id ?: return@withState
val otherUserId = state.otherUserId
val roomId = state.roomId
?: session.roomService().getExistingDirectRoomWithUser(otherUserId)

Expand Down Expand Up @@ -514,7 +537,7 @@ class VerificationBottomSheetViewModel @AssistedInject constructor(
override fun transactionUpdated(tx: VerificationTransaction) = withState { state ->
if (state.selfVerificationMode && state.transactionId == null) {
// is this an incoming with that user
if (tx.isIncoming && tx.otherUserId == state.otherUserMxItem?.id) {
if (tx.isIncoming && tx.otherUserId == state.otherUserId) {
// Also auto accept incoming if needed!
if (tx is IncomingSasVerificationTransaction) {
if (tx.uxState == IncomingSasVerificationTransaction.UxState.SHOW_ACCEPT) {
Expand Down Expand Up @@ -564,7 +587,7 @@ class VerificationBottomSheetViewModel @AssistedInject constructor(

if (state.selfVerificationMode && state.pendingRequest.invoke() == null && state.transactionId == null) {
// is this an incoming with that user
if (pr.isIncoming && pr.otherUserId == state.otherUserMxItem?.id) {
if (pr.isIncoming && pr.otherUserId == state.otherUserId) {
if (!pr.isReady) {
// auto ready in this case, as we are waiting
// TODO, can I be here in DM mode? in this case should test if roomID is null?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import im.vector.app.core.utils.colorizeMatchingText
import im.vector.app.features.crypto.verification.VerificationBottomSheetViewState
import im.vector.app.features.crypto.verification.epoxy.bottomSheetVerificationActionItem
import im.vector.app.features.crypto.verification.epoxy.bottomSheetVerificationNoticeItem
import im.vector.app.features.displayname.getBestName
import im.vector.lib.core.utils.epoxy.charsequence.EpoxyCharSequence
import im.vector.lib.core.utils.epoxy.charsequence.toEpoxyCharSequence
import javax.inject.Inject
Expand Down Expand Up @@ -60,8 +61,8 @@ class VerificationCancelController @Inject constructor(
}
}
} else {
val otherUserID = state.otherUserMxItem?.id ?: ""
val otherDisplayName = state.otherUserMxItem?.displayName ?: ""
val otherUserID = state.otherUserId
val otherDisplayName = state.otherUserMxItem.getBestName()
bottomSheetVerificationNoticeItem {
id("notice")
notice(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ class VerificationChooseMethodFragment :
override fun doVerifyBySas() = withState(sharedViewModel) { state ->
sharedViewModel.handle(
VerificationAction.StartSASVerification(
state.otherUserMxItem?.id ?: "",
state.otherUserId,
state.pendingRequest.invoke()?.transactionId ?: ""
)
)
Expand Down Expand Up @@ -130,7 +130,7 @@ class VerificationChooseMethodFragment :
private fun onRemoteQrCodeScanned(remoteQrCode: String) = withState(sharedViewModel) { state ->
sharedViewModel.handle(
VerificationAction.RemoteQrCodeScanned(
state.otherUserMxItem?.id ?: "",
state.otherUserId,
state.pendingRequest.invoke()?.transactionId ?: "",
remoteQrCode
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ class VerificationEmojiCodeController @Inject constructor(
if (state.isWaitingFromOther) {
bottomSheetVerificationWaitingItem {
id("waiting")
title(host.stringProvider.getString(R.string.verification_request_waiting_for, state.otherUser?.getBestName() ?: ""))
title(host.stringProvider.getString(R.string.verification_request_waiting_for, state.otherUser.getBestName()))
}
} else {
bottomSheetVerificationActionItem {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,13 @@ class VerificationEmojiCodeFragment :
}

override fun onMatchButtonTapped() = withState(viewModel) { state ->
val otherUserId = state.otherUser?.id ?: return@withState
val otherUserId = state.otherUser.id
val txId = state.transactionId ?: return@withState
sharedViewModel.handle(VerificationAction.SASMatchAction(otherUserId, txId))
}

override fun onDoNotMatchButtonTapped() = withState(viewModel) { state ->
val otherUserId = state.otherUser?.id ?: return@withState
val otherUserId = state.otherUser.id
val txId = state.transactionId ?: return@withState
sharedViewModel.handle(VerificationAction.SASDoNotMatchAction(otherUserId, txId))
}
Expand Down
Loading