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

Feature/bca/verif resist no age #6328

Merged
merged 5 commits into from
Jun 28, 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/6328.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix | Some user verification requests couldn't be accepted/declined
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ internal class VerificationMessageProcessor @Inject constructor(
// If the request is in the future by more than 5 minutes or more than 10 minutes in the past,
// the message should be ignored by the receiver.

if (event.ageLocalTs != null && !VerificationService.isValidRequest(event.ageLocalTs, clock.epochMillis())) return Unit.also {
if (!VerificationService.isValidRequest(event.ageLocalTs, clock.epochMillis())) return Unit.also {
Timber.d("## SAS Verification live observer: msgId: ${event.eventId} is outdated age:$event.ageLocalTs ms")
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ private fun HashMap<String, RoomMemberContent?>.addSenderState(realm: Realm, roo
* Create an EventEntity for the root thread event or get an existing one.
*/
private fun createEventEntity(realm: Realm, roomId: String, event: Event, currentTimeMillis: Long): EventEntity {
val ageLocalTs = event.unsignedData?.age?.let { currentTimeMillis - it }
val ageLocalTs = currentTimeMillis - (event.unsignedData?.age ?: 0)
return event.toEntity(roomId, SendState.SYNCED, ageLocalTs).copyToRealmOrIgnore(realm, EventInsertType.PAGINATION)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ internal fun EventEntity.asDomain(castJsonNumbers: Boolean = false): Event {
internal fun Event.toEntity(
roomId: String,
sendState: SendState,
ageLocalTs: Long?,
ageLocalTs: Long,
contentToInject: String? = null
): EventEntity {
return EventMapper.map(this, roomId).apply {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ internal class DefaultLoadRoomMembersTask @Inject constructor(
if (roomMemberEvent.eventId == null || roomMemberEvent.stateKey == null || roomMemberEvent.type == null) {
continue
}
val ageLocalTs = roomMemberEvent.unsignedData?.age?.let { now - it }
val ageLocalTs = now - (roomMemberEvent.unsignedData?.age ?: 0)
val eventEntity = roomMemberEvent.toEntity(roomId, SendState.SYNCED, ageLocalTs).copyToRealmOrIgnore(realm, EventInsertType.PAGINATION)
CurrentStateEventEntity.getOrCreate(
realm,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,8 @@ internal class DefaultFetchThreadTimelineTask @Inject constructor(
* Create an EventEntity to be added in the TimelineEventEntity.
*/
private fun createEventEntity(roomId: String, event: Event, realm: Realm): EventEntity {
val ageLocalTs = event.unsignedData?.age?.let { clock.epochMillis() - it }
val now = clock.epochMillis()
val ageLocalTs = now - (event.unsignedData?.age ?: 0)
return event.toEntity(roomId, SendState.SYNCED, ageLocalTs).copyToRealmOrIgnore(realm, EventInsertType.PAGINATION)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ internal class DefaultGetEventTask @Inject constructor(
}
}

event.ageLocalTs = event.unsignedData?.age?.let { clock.epochMillis() - it }
event.ageLocalTs = clock.epochMillis() - (event.unsignedData?.age ?: 0)

return event
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ internal class TokenChunkEventPersistor @Inject constructor(
val now = clock.epochMillis()

stateEvents?.forEach { stateEvent ->
val ageLocalTs = stateEvent.unsignedData?.age?.let { now - it }
val ageLocalTs = now - (stateEvent.unsignedData?.age ?: 0)
val stateEventEntity = stateEvent.toEntity(roomId, SendState.SYNCED, ageLocalTs).copyToRealmOrIgnore(realm, EventInsertType.PAGINATION)
currentChunk.addStateEvent(roomId, stateEventEntity, direction)
if (stateEvent.type == EventType.STATE_ROOM_MEMBER && stateEvent.stateKey != null) {
Expand All @@ -155,7 +155,7 @@ internal class TokenChunkEventPersistor @Inject constructor(
if (event.eventId == null || event.senderId == null) {
return@forEach
}
val ageLocalTs = event.unsignedData?.age?.let { now - it }
val ageLocalTs = now - (event.unsignedData?.age ?: 0)
val eventEntity = event.toEntity(roomId, SendState.SYNCED, ageLocalTs).copyToRealmOrIgnore(realm, EventInsertType.PAGINATION)
if (event.type == EventType.STATE_ROOM_MEMBER && event.stateKey != null) {
val contentToUse = if (direction == PaginationDirection.BACKWARDS) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ internal class RoomSyncHandler @Inject constructor(
if (event.eventId == null || event.stateKey == null || event.type == null) {
continue
}
val ageLocalTs = event.unsignedData?.age?.let { syncLocalTimestampMillis - it }
val ageLocalTs = syncLocalTimestampMillis - (event.unsignedData?.age ?: 0)
val eventEntity = event.toEntity(roomId, SendState.SYNCED, ageLocalTs).copyToRealmOrIgnore(realm, insertType)
Timber.v("## received state event ${event.type} and key ${event.stateKey}")
CurrentStateEventEntity.getOrCreate(realm, roomId, event.stateKey, event.type).apply {
Expand Down Expand Up @@ -306,7 +306,7 @@ internal class RoomSyncHandler @Inject constructor(
if (event.stateKey == null || event.type == null) {
return@forEach
}
val ageLocalTs = event.unsignedData?.age?.let { syncLocalTimestampMillis - it }
val ageLocalTs = syncLocalTimestampMillis - (event.unsignedData?.age ?: 0)
val eventEntity = event.toEntity(roomId, SendState.SYNCED, ageLocalTs).copyToRealmOrIgnore(realm, insertType)
CurrentStateEventEntity.getOrCreate(realm, roomId, event.stateKey, event.type).apply {
eventId = eventEntity.eventId
Expand Down Expand Up @@ -336,7 +336,7 @@ internal class RoomSyncHandler @Inject constructor(
if (event.eventId == null || event.stateKey == null || event.type == null) {
continue
}
val ageLocalTs = event.unsignedData?.age?.let { syncLocalTimestampMillis - it }
val ageLocalTs = syncLocalTimestampMillis - (event.unsignedData?.age ?: 0)
val eventEntity = event.toEntity(roomId, SendState.SYNCED, ageLocalTs).copyToRealmOrIgnore(realm, insertType)
CurrentStateEventEntity.getOrCreate(realm, roomId, event.stateKey, event.type).apply {
eventId = event.eventId
Expand All @@ -348,7 +348,7 @@ internal class RoomSyncHandler @Inject constructor(
if (event.eventId == null || event.senderId == null || event.type == null) {
continue
}
val ageLocalTs = event.unsignedData?.age?.let { syncLocalTimestampMillis - it }
val ageLocalTs = syncLocalTimestampMillis - (event.unsignedData?.age ?: 0)
val eventEntity = event.toEntity(roomId, SendState.SYNCED, ageLocalTs).copyToRealmOrIgnore(realm, insertType)
if (event.stateKey != null) {
CurrentStateEventEntity.getOrCreate(realm, roomId, event.stateKey, event.type).apply {
Expand Down Expand Up @@ -401,7 +401,10 @@ internal class RoomSyncHandler @Inject constructor(
for (rawEvent in eventList) {
// It's annoying roomId is not there, but lot of code rely on it.
// And had to do it now as copy would delete all decryption results..
val event = rawEvent.copy(roomId = roomId)
val ageLocalTs = syncLocalTimestampMillis - (rawEvent.unsignedData?.age ?: 0)
val event = rawEvent.copy(roomId = roomId).also {
it.ageLocalTs = ageLocalTs
}
if (event.eventId == null || event.senderId == null || event.type == null) {
continue
}
Expand All @@ -423,7 +426,6 @@ internal class RoomSyncHandler @Inject constructor(
contentToInject = threadsAwarenessHandler.makeEventThreadAware(realm, roomId, event)
}

val ageLocalTs = event.unsignedData?.age?.let { syncLocalTimestampMillis - it }
val eventEntity = event.toEntity(roomId, SendState.SYNCED, ageLocalTs, contentToInject).copyToRealmOrIgnore(realm, insertType)
if (event.stateKey != null) {
CurrentStateEventEntity.getOrCreate(realm, roomId, event.stateKey, event.type).apply {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import org.matrix.android.sdk.internal.session.permalinks.PermalinkFactory
import org.matrix.android.sdk.internal.session.room.send.LocalEchoEventFactory
import org.matrix.android.sdk.internal.session.room.timeline.GetEventTask
import org.matrix.android.sdk.internal.util.awaitTransaction
import org.matrix.android.sdk.internal.util.time.Clock
import javax.inject.Inject

/**
Expand All @@ -64,7 +65,8 @@ internal class ThreadsAwarenessHandler @Inject constructor(
private val permalinkFactory: PermalinkFactory,
@SessionDatabase private val monarchy: Monarchy,
private val lightweightSettingsStorage: LightweightSettingsStorage,
private val getEventTask: GetEventTask
private val getEventTask: GetEventTask,
private val clock: Clock,
) {

// This caching is responsible to improve the performance when we receive a root event
Expand Down Expand Up @@ -120,7 +122,7 @@ internal class ThreadsAwarenessHandler @Inject constructor(
private suspend fun fetchThreadsEvents(threadsToFetch: Map<String, String>) {
val eventEntityList = threadsToFetch.mapNotNull { (eventId, roomId) ->
fetchEvent(eventId, roomId)?.let {
it.toEntity(roomId, SendState.SYNCED, it.ageLocalTs)
it.toEntity(roomId, SendState.SYNCED, it.ageLocalTs ?: clock.epochMillis())
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* Copyright (c) 2020 New Vector Ltd
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package im.vector.app.core.ui.list

import com.airbnb.epoxy.EpoxyAttribute
import com.airbnb.epoxy.EpoxyModelClass
import com.google.android.material.button.MaterialButton
import im.vector.app.R
import im.vector.app.core.epoxy.ClickListener
import im.vector.app.core.epoxy.VectorEpoxyHolder
import im.vector.app.core.epoxy.VectorEpoxyModel
import im.vector.app.core.epoxy.onClick
import im.vector.lib.core.utils.epoxy.charsequence.EpoxyCharSequence

/**
* A generic button list item.
*/
@EpoxyModelClass(layout = R.layout.item_positive_destrutive_buttons)
abstract class ButtonPositiveDestructiveButtonBarItem : VectorEpoxyModel<ButtonPositiveDestructiveButtonBarItem.Holder>() {

@EpoxyAttribute
var positiveText: EpoxyCharSequence? = null

@EpoxyAttribute
var destructiveText: EpoxyCharSequence? = null

@EpoxyAttribute(EpoxyAttribute.Option.DoNotHash)
var positiveButtonClickAction: ClickListener? = null

@EpoxyAttribute(EpoxyAttribute.Option.DoNotHash)
var destructiveButtonClickAction: ClickListener? = null

override fun bind(holder: Holder) {
super.bind(holder)
positiveText?.charSequence?.let { holder.positiveButton.text = it }
destructiveText?.charSequence?.let { holder.destructiveButton.text = it }

holder.positiveButton.onClick(positiveButtonClickAction)
holder.destructiveButton.onClick(destructiveButtonClickAction)
}

class Holder : VectorEpoxyHolder() {
val destructiveButton by bind<MaterialButton>(R.id.destructive_button)
val positiveButton by bind<MaterialButton>(R.id.positive_button)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ sealed class VerificationAction : VectorViewModelAction {
data class GotItConclusion(val verified: Boolean) : VerificationAction()
object SkipVerification : VerificationAction()
object VerifyFromPassphrase : VerificationAction()
object ReadyPendingVerification : VerificationAction()
object CancelPendingVerification : VerificationAction()
data class GotResultFromSsss(val cypherData: String, val alias: String) : VerificationAction()
object CancelledFromSsss : VerificationAction()
object SecuredStorageHasBeenReset : VerificationAction()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,27 @@ class VerificationBottomSheetViewModel @AssistedInject constructor(
as? SasVerificationTransaction)
?.shortCodeDoesNotMatch()
}
is VerificationAction.ReadyPendingVerification -> {
state.pendingRequest.invoke()?.let { request ->
// will only be there for dm verif
if (state.roomId != null) {
session.cryptoService().verificationService()
.readyPendingVerificationInDMs(
supportedVerificationMethodsProvider.provide(),
state.otherUserId,
state.roomId,
request.transactionId ?: ""
)
}
}
}
is VerificationAction.CancelPendingVerification -> {
state.pendingRequest.invoke()?.let {
session.cryptoService().verificationService()
.cancelVerificationRequest(it)
}
_viewEvents.post(VerificationBottomSheetViewEvents.Dismiss)
}
is VerificationAction.GotItConclusion -> {
if (state.isVerificationRequired && !action.verified) {
// we should go back to first screen
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import im.vector.app.R
import im.vector.app.core.epoxy.bottomSheetDividerItem
import im.vector.app.core.resources.ColorProvider
import im.vector.app.core.resources.StringProvider
import im.vector.app.core.ui.list.buttonPositiveDestructiveButtonBarItem
import im.vector.app.features.crypto.verification.epoxy.bottomSheetVerificationActionItem
import im.vector.app.features.crypto.verification.epoxy.bottomSheetVerificationNoticeItem
import im.vector.app.features.crypto.verification.epoxy.bottomSheetVerificationQrCodeItem
Expand Down Expand Up @@ -108,6 +109,15 @@ class VerificationChooseMethodController @Inject constructor(
iconColor(host.colorProvider.getColorFromAttribute(R.attr.vctr_content_primary))
listener { host.listener?.doVerifyBySas() }
}
} else if (!state.isReadySent) {
// a bit of a special case, if you tapped on the timeline cell but not on a button
buttonPositiveDestructiveButtonBarItem {
id("accept_decline")
positiveText(host.stringProvider.getString(R.string.action_accept).toEpoxyCharSequence())
destructiveText(host.stringProvider.getString(R.string.action_decline).toEpoxyCharSequence())
positiveButtonClickAction { host.listener?.acceptRequest() }
destructiveButtonClickAction { host.listener?.declineRequest() }
}
}

if (state.isMe && state.canCrossSign) {
Expand All @@ -131,5 +141,7 @@ class VerificationChooseMethodController @Inject constructor(
fun openCamera()
fun doVerifyBySas()
fun onClickOnWasNotMe()
fun acceptRequest()
fun declineRequest()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,14 @@ class VerificationChooseMethodFragment @Inject constructor(
sharedViewModel.itWasNotMe()
}

override fun acceptRequest() {
sharedViewModel.handle(VerificationAction.ReadyPendingVerification)
}

override fun declineRequest() {
sharedViewModel.handle(VerificationAction.CancelPendingVerification)
}

private fun doOpenQRCodeScanner() {
QrCodeScannerActivity.startForResult(requireActivity(), scanActivityResultLauncher)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ data class VerificationChooseMethodViewState(
val qrCodeText: String? = null,
val sasModeAvailable: Boolean = false,
val isMe: Boolean = false,
val canCrossSign: Boolean = false
val canCrossSign: Boolean = false,
val isReadySent: Boolean = false
) : MavericksState

class VerificationChooseMethodViewModel @AssistedInject constructor(
Expand Down Expand Up @@ -81,7 +82,8 @@ class VerificationChooseMethodViewModel @AssistedInject constructor(
copy(
otherCanShowQrCode = pvr?.otherCanShowQrCode().orFalse(),
otherCanScanQrCode = pvr?.otherCanScanQrCode().orFalse(),
sasModeAvailable = pvr?.isSasSupported().orFalse()
sasModeAvailable = pvr?.isSasSupported().orFalse(),
isReadySent = pvr?.isReady.orFalse(),
)
}
}
Expand Down
24 changes: 24 additions & 0 deletions vector/src/main/res/layout/item_positive_destrutive_buttons.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?xml version="1.0" encoding="utf-8"?>
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:tools="http://schemas.android.com/tools"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:gravity="center_horizontal"
android:orientation="horizontal">

<Button
android:id="@+id/destructive_button"
style="@style/Widget.Vector.Button.Destructive"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_marginEnd="16dp"
tools:text="@string/action_decline" />
Copy link
Member

Choose a reason for hiding this comment

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

OK to use tools but in this case, I think that positiveText and destructiveText should not be nullable. (see https://github.com/vector-im/element-android/pull/6328/files#diff-e03d15165cd3cc55c597e31b0494b0088b8ef56da1edb7468b870973d2948265R35)

This is not a blocker though.


<Button
android:id="@+id/positive_button"
style="@style/Widget.Vector.Button.Positive"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
tools:text="@string/action_accept" />

</LinearLayout>