feature(crypto): verification violation handling and block sending #4126

BillCarsonFr · 2025-01-08T09:43:42Z

Draft as it depends on matrix-org/matrix-rust-sdk#4478

Android part of element-hq/element-meta#2492 (still partial as there is no way to verify other users yet on EX, so you can only withdraw the verification requirement).

Content

When a previously verified user is not anymore there will now be a banner on top of the composer. The composer will be in a disabled state, as anyhow sending will fail (as per element-hq/element-meta#2488)

Motivation and context

Screenshots / GIFs

Dark:

Tests

Step 1
Step 2
Step ...

Tested devices

Physical
Emulator
OS version(s):

Checklist

Changes have been tested on an Android device or Android emulator with API 24
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request title will be used in the release note, it clearly define what will change for the user
Pull request includes screenshots or videos if containing UI changes
You've made a self review of your PR

github-actions · 2025-01-20T08:06:17Z

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/xWozr2

codecov · 2025-01-20T12:33:19Z

Codecov Report

Attention: Patch coverage is 93.69369% with 7 lines in your changes missing coverage. Please review.

Project coverage is 83.37%. Comparing base (3668e86) to head (68a3902).

Files with missing lines	Patch %	Lines
...pl/crypto/identity/IdentityChangeStatePresenter.kt	72.72%	2 Missing and 1 partial ⚠️
...sages/impl/messagecomposer/MessageComposerUtils.kt	90.00%	0 Missing and 2 partials ⚠️
...ssages/impl/messagecomposer/MessageComposerView.kt	60.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4126      +/-   ##
===========================================
+ Coverage    83.35%   83.37%   +0.01%     
===========================================
  Files         1886     1888       +2     
  Lines        49403    49474      +71     
  Branches      5804     5812       +8     
===========================================
+ Hits         41180    41249      +69     
- Misses        6134     6137       +3     
+ Partials      2089     2088       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ion_violation_banner' into feature/valere/support_verification_violation_banner

bmarty

Thanks! Just need some renaming and cleanup, let me know if I can do anything else.

bmarty · 2025-01-21T15:01:40Z

.../kotlin/io/element/android/features/messages/impl/crypto/identity/IdentityChangeStateView.kt

@@ -32,7 +33,8 @@ fun IdentityChangeStateView(
    // Pick the first identity change to PinViolation
    val pinViolationIdentityChange = state.roomMemberIdentityStateChanges.firstOrNull {
        // For now only render PinViolation
-        it.identityState == IdentityState.PinViolation
+        it.identityState == IdentityState.PinViolation ||
+            it.identityState == IdentityState.VerificationViolation


Can you remove the 2 comments above please? Looks like there are not true anymore :)

Also probably pinViolationIdentityChange could be rename to something more generic like firstRoomMemberIdentityStateChange.

bmarty · 2025-01-21T15:02:49Z

...main/kotlin/io/element/android/features/messages/impl/crypto/identity/IdentityChangeEvent.kt

@@ -10,5 +10,6 @@ package io.element.android.features.messages.impl.crypto.identity
 import io.element.android.libraries.matrix.api.core.UserId

 sealed interface IdentityChangeEvent {
-    data class Submit(val userId: UserId) : IdentityChangeEvent
+    data class PinViolation(val userId: UserId) : IdentityChangeEvent


Should be renamed to something more related to the action that will be performed. I propose PinUserIdentity.

bmarty · 2025-01-21T15:03:29Z

...main/kotlin/io/element/android/features/messages/impl/crypto/identity/IdentityChangeEvent.kt

@@ -10,5 +10,6 @@ package io.element.android.features.messages.impl.crypto.identity
 import io.element.android.libraries.matrix.api.core.UserId

 sealed interface IdentityChangeEvent {
-    data class Submit(val userId: UserId) : IdentityChangeEvent
+    data class PinViolation(val userId: UserId) : IdentityChangeEvent
+    data class VerificationViolation(val userId: UserId) : IdentityChangeEvent


Should be renamed to something more related to the action that will be performed. I propose WithdrawVerification.

bmarty · 2025-01-21T15:04:04Z

...in/io/element/android/features/messages/impl/crypto/identity/IdentityChangeStatePresenter.kt

@@ -94,15 +99,22 @@ class IdentityChangeStatePresenter @Inject constructor(
                Timber.e(it, "Failed to pin identity for user $userId")
            }
    }
+
+    private fun CoroutineScope.withdrawVerificationRequirement(userId: UserId) = launch {


Rename to withdrawVerification?

bmarty · 2025-01-21T15:04:29Z

.../api/src/main/kotlin/io/element/android/libraries/matrix/api/encryption/EncryptionService.kt

@@ -66,6 +66,7 @@ interface EncryptionService {
     * Remember this identity, ensuring it does not result in a pin violation.
     */
    suspend fun pinUserIdentity(userId: UserId): Result<Unit>
+    suspend fun withdrawVerificationRequirement(userId: UserId): Result<Unit>


Rename to withdrawVerification?

bmarty · 2025-01-21T15:12:18Z

...ain/kotlin/io/element/android/features/messages/impl/messagecomposer/DisabledComposerView.kt

+import io.element.android.libraries.textcomposer.R
+
+@Composable
+internal fun DisabledComposerView(


I understand the aim of this Composable, but, as @jmartinesp mentionned, we take the risk that if a change in the real composer UI is done, this component is not updated.

I have seen that you have added screenshot test in the same view than the real composer, so this may mitigate this risk, thanks.

So it's OK for me!

ok. not ideal but enough for now. We need anyhow a way to have the composer disabled, and properly support transition to disabled in the middle of editing/recording ..

bmarty · 2025-01-21T15:15:35Z

...kotlin/io/element/android/features/messages/impl/messagecomposer/MessageComposerPresenter.kt

@@ -705,4 +723,33 @@ class MessageComposerPresenter @AssistedInject constructor(
            }
        }
    }
+
+    @OptIn(ExperimentalCoroutinesApi::class)
+    private fun ProduceStateScope<PersistentList<RoomMemberIdentityStateChange>>.observeRoomMemberIdentityStateChange() {


This method already exists in IdentityChangeStatePresenter, we should avoid copy pasting full block of code like this. Can you extract it please?

Moved to a Utils class

...main/kotlin/io/element/android/features/messages/impl/messagecomposer/MessageComposerView.kt

bmarty · 2025-01-21T15:17:30Z

...n/kotlin/io/element/android/libraries/designsystem/atomic/molecules/ComposerAlertMolecule.kt

@@ -78,7 +78,11 @@ fun ComposerAlertMolecule(
                        text = content,
                        modifier = Modifier.weight(1f),
                        style = ElementTheme.typography.fontBodyMdRegular,
-                        color = ElementTheme.colors.textPrimary,
+                        color = if (isCritical) {
+                            ElementTheme.colors.textCriticalPrimary


It's a bit "red on red" in dark theme, is it the expected design?

There is no dark version in figma. I am color blind so not sure if it's strange or not ^^

bmarty · 2025-01-21T15:22:04Z

...est/snapshots/images/features.messages.impl.messagecomposer_MessageComposerView_Day_0_en.png

The composer box does not have the same width:

I guess a user with hardly notices that, but maybe improve it?

a bit better, but the paddings/margins are a bit strange on the composer de44045

…tion_banner

…ion_violation_banner' into feature/valere/support_verification_violation_banner

sonarqubecloud · 2025-01-24T14:26:07Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

BillCarsonFr added the PR-Feature For a new feature label Jan 8, 2025

BillCarsonFr mentioned this pull request Jan 15, 2025

Warn and block sending on verification violation element-hq/element-x-ios#3679

Merged

BillCarsonFr force-pushed the feature/valere/support_verification_violation_banner branch from 27093ef to 0566d90 Compare January 20, 2025 07:57

BillCarsonFr added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jan 20, 2025

github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jan 20, 2025

BillCarsonFr and others added 3 commits January 20, 2025 11:39

feature(crypto): verification violation handling and block sending

d6000e3

Quick test for verification violation event

4092604

Update screenshots

c012fa1

BillCarsonFr force-pushed the feature/valere/support_verification_violation_banner branch from 159fd2a to c012fa1 Compare January 20, 2025 10:39

Fix konsist warnings

f731dee

BillCarsonFr added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jan 20, 2025

github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jan 20, 2025

Update screenshots

82302e5

BillCarsonFr added 2 commits January 20, 2025 14:54

test: Add new IdentityChangeStateViewTest

22cfdeb

Merge remote-tracking branch 'origin/feature/valere/support_verificat…

fe83acb

…ion_violation_banner' into feature/valere/support_verification_violation_banner

BillCarsonFr marked this pull request as ready for review January 20, 2025 14:43

BillCarsonFr requested a review from a team as a code owner January 20, 2025 14:43

BillCarsonFr requested review from bmarty and removed request for a team January 20, 2025 14:43

richvdh assigned BillCarsonFr Jan 21, 2025

bmarty reviewed Jan 21, 2025

View reviewed changes

BillCarsonFr added 3 commits January 24, 2025 12:00

code review: renaming, comments, extract common code

caf2662

fix disabled composer padding/margin diff with composer

3933054

fixup: ktlint

355b6d5

BillCarsonFr added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jan 24, 2025

github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jan 24, 2025

ElementBot and others added 2 commits January 24, 2025 12:47

Update screenshots

de44045

Merge branch 'develop' into feature/valere/support_verification_viola…

1d98874

…tion_banner

BillCarsonFr added 2 commits January 24, 2025 14:47

Merge remote-tracking branch 'origin/feature/valere/support_verificat…

520eedc

…ion_violation_banner' into feature/valere/support_verification_violation_banner

fixup: remove dead code (refactored method)

68a3902

BillCarsonFr requested a review from bmarty January 27, 2025 08:55

BillCarsonFr mentioned this pull request Feb 3, 2025

Invisible crypto: Verified Identity change notice on composer: part 1. element-hq/element-meta#2715

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(crypto): verification violation handling and block sending #4126

feature(crypto): verification violation handling and block sending #4126

BillCarsonFr commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 20, 2025 •

edited

Loading

codecov bot commented Jan 20, 2025 •

edited

Loading

bmarty left a comment

bmarty Jan 21, 2025

BillCarsonFr Jan 24, 2025

bmarty Jan 21, 2025

bmarty Jan 21, 2025

BillCarsonFr Jan 24, 2025

bmarty Jan 21, 2025

BillCarsonFr Jan 24, 2025

bmarty Jan 21, 2025

BillCarsonFr Jan 24, 2025

bmarty Jan 21, 2025

BillCarsonFr Jan 24, 2025

bmarty Jan 21, 2025

BillCarsonFr Jan 24, 2025

bmarty Jan 21, 2025

BillCarsonFr Jan 24, 2025

bmarty Jan 21, 2025

BillCarsonFr Jan 24, 2025

sonarqubecloud bot commented Jan 24, 2025

feature(crypto): verification violation handling and block sending #4126

Are you sure you want to change the base?

feature(crypto): verification violation handling and block sending #4126

Conversation

BillCarsonFr commented Jan 8, 2025 • edited Loading

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

github-actions bot commented Jan 20, 2025 • edited Loading

codecov bot commented Jan 20, 2025 • edited Loading

Codecov Report

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 24, 2025

Quality Gate passed

BillCarsonFr commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 20, 2025 •

edited

Loading

codecov bot commented Jan 20, 2025 •

edited

Loading