Implement user verification #4294

jmartinesp · 2025-02-21T15:40:17Z

Content

Moves the 'select key recovery method' screen out of the VerifySelfSessionNode to its own navigation Node so we can have the same Initial state for both session verification and user verification.
Add 'Profile' list item for DMs so you can access the other user's profile.
Make the 'verify' list item in the user profile actually trigger a user verification.
Implement user verification request and the whole verification flow.

Motivation and context

Close #4241.

Screenshots / GIFs

There are a few new screenshots in the PR.

Tests

You'll need 2 accounts that haven't verified each other yet.

With one of the accounts, go to the other user's profile (maybe through a DM -> Settings -> Profile, to test this works) and start the verification.
The other user should receive a verification request and it should be displayed. Verification requests received while the app was in background should also be displayed soon after re-opening the app.
If you start a user verification request, you should be able to verify each other. Also, the verification should be cancellable and restartable.
Once the users have verified each other, opening each others' profiles should display a 'verified' badge.

Tested devices

Physical
Emulator
OS version(s): 14, 15.

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-02-21T15:50:55Z

📱 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/c1Qex6

codecov · 2025-02-21T16:26:58Z

Codecov Report

Attention: Patch coverage is 84.88121% with 70 lines in your changes missing coverage. Please review.

Project coverage is 80.12%. Comparing base (9f43135) to head (e1b03bc).
Report is 19 commits behind head on develop.

Files with missing lines	Patch %	Lines
...mpl/verification/RustSessionVerificationService.kt	0.00%	13 Missing ⚠️
...d/libraries/designsystem/utils/OpenUrlInTabView.kt	0.00%	9 Missing ⚠️
.../verification/SessionVerificationRequestDetails.kt	0.00%	9 Missing ⚠️
...droid/libraries/designsystem/components/BigIcon.kt	85.00%	0 Missing and 6 partials ⚠️
...ion/impl/incoming/IncomingVerificationPresenter.kt	80.76%	1 Missing and 4 partials ⚠️
...rifysession/impl/outgoing/VerifySelfSessionView.kt	91.66%	0 Missing and 4 partials ⚠️
...ies/matrix/api/verification/VerificationRequest.kt	50.00%	4 Missing ⚠️
...tures/verifysession/api/VerifySessionEntryPoint.kt	0.00%	3 Missing ⚠️
.../impl/incoming/IncomingVerificationStateMachine.kt	75.00%	3 Missing ⚠️
.../verification/SessionVerificationRequestDetails.kt	62.50%	3 Missing ⚠️
... and 10 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4294      +/-   ##
===========================================
- Coverage    80.12%   80.12%   -0.01%     
===========================================
  Files         2058     2065       +7     
  Lines        54795    54953     +158     
  Branches      6713     6739      +26     
===========================================
+ Hits         43907    44029     +122     
- Misses        8592     8625      +33     
- Partials      2296     2299       +3

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

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

bmarty

Some first remarks on the UI. I'll continue the review.

I need to click twice on "Start verification" for something to happen. It seems that the screen is added twice on the backstack since at the end I need to press twice on the back arrow. See below:

StartVerification_press_twice.mp4

One question, the "Verified" decoration (green dot with check) for verified user in timeline top bar and room list item will come later?

bmarty · 2025-03-06T13:35:03Z

...uitests/src/test/snapshots/images/features.userprofile.shared_UserProfileView_Night_1_en.png

Figma says that the wording should be "Verify user"

bmarty · 2025-03-06T13:41:00Z

.../snapshots/images/features.verifysession.impl.incoming_IncomingVerificationView_Day_5_en.png

This does not match Figma, the top icon should be "Reaction solid"

bmarty · 2025-03-06T13:42:00Z

...st/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Day_13_en.png

Empty screen?

Yes, this is the 'Exit' state, it shouldn't display any UI.

bmarty · 2025-03-06T13:42:52Z

...est/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Day_1_en.png

The top icon should be "lock solid"

bmarty · 2025-03-06T13:53:35Z

...n/kotlin/io/element/android/features/verifysession/impl/ui/VerificationUserProfileContent.kt

+            Text(text = displayName ?: userId.value, style = ElementTheme.typography.fontBodyLgMedium, color = ElementTheme.colors.textPrimary)
+
+            if (displayName != null) {
+                Text(text = userId.value, style = ElementTheme.typography.fontBodyMdRegular, color = ElementTheme.colors.textSecondary)


In Figma there is a space of 2 with the text above. Add a verticalArrangement = Arrangement.spacedBy(2.dp) to the Column?

bmarty

More remarks. Doing more test now.

bmarty · 2025-03-06T14:03:42Z

appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt

+                // Wait for the UI to be ready
+                delay(500.milliseconds)
+
+                backstack.singleTop(NavTarget.IncomingVerificationRequest(verificationRequest))


We should probably ignore new incoming request if there is already a verification in progress. But maybe this is handled by the SDK?

Yep, I can't remember the behaviour, but it was either new requests cancelled previous ones or were ignored, according to the docs.

bmarty · 2025-03-06T14:06:55Z

appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt

+        override fun onIncomingSessionRequest(verificationRequest: VerificationRequest.Incoming) {
+            // Without this launch the rendering and actual state of this Appyx node's children gets out of sync, resulting in a crash.
+            // This might be because this method is called back from Rust in a background thread.
+            MainScope().launch {


Can RustSessionVerificationService ensure that onIncomingSessionRequest is invoked on the same thread that was used when registering the listener?

I don't think so, this is due to how JNA works, it'll create a separate thread for this callback and use it when receiving data from Rust. Maybe we could somehow retain the thread instance and force the callback to somehow run some runnable in it, but it seems like an overkill.

retain the thread instance and force the callback to somehow run some runnable in it

This is what I had in mind, but OK to keep it like this :)

bmarty · 2025-03-06T14:07:06Z

appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt

+            // This might be because this method is called back from Rust in a background thread.
+            MainScope().launch {
+                // Wait until the app is in foreground to display the incoming verification request
+                appNavigationStateService.appNavigationState.first { it.isInForeground }


Maybe add a timeout?

Maybe, but I'm not sure what a sensible value would be. Maybe instead of the timeout we should try to check if the request hasn't been cancelled.

bmarty · 2025-03-06T14:09:21Z

features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/di/FtueModule.kt

+@Module
+interface FtueModule {
+    @Binds
+    fun bindChooseVerificationMethodPresenter(presenter: ChooseSelfVerificationModePresenter): Presenter<ChooseSelfVerificationModeState>


Not a big deal, but the name does not match :)

Suggested change

fun bindChooseVerificationMethodPresenter(presenter: ChooseSelfVerificationModePresenter): Presenter<ChooseSelfVerificationModeState>

fun bindChooseSelfVerificationModePresenter(presenter: ChooseSelfVerificationModePresenter): Presenter<ChooseSelfVerificationModeState>

bmarty · 2025-03-06T14:11:44Z

...io/element/android/features/ftue/impl/sessionverification/FtueSessionVerificationFlowNode.kt

+        data object Root : NavTarget
+
+        @Parcelize
+        data class UseAnotherDevice(val showDeviceVerifiedScreen: Boolean) : NavTarget


The value for showDeviceVerifiedScreen is always true, so I guess this parameter can be removed.

bmarty · 2025-03-06T14:15:00Z

...oid/features/ftue/impl/sessionverification/choosemode/ChooseSelfVerificationModePresenter.kt

+)
+
+sealed interface ChooseSelfVerificationModeEvent {
+    object SignOut : ChooseSelfVerificationModeEvent


Suggested change

object SignOut : ChooseSelfVerificationModeEvent

data object SignOut : ChooseSelfVerificationModeEvent

bmarty · 2025-03-06T14:15:10Z

...oid/features/ftue/impl/sessionverification/choosemode/ChooseSelfVerificationModePresenter.kt

+    val eventSink: (ChooseSelfVerificationModeEvent) -> Unit,
+)
+
+sealed interface ChooseSelfVerificationModeEvent {


Extract to own file?

bmarty · 2025-03-06T14:15:13Z

...oid/features/ftue/impl/sessionverification/choosemode/ChooseSelfVerificationModePresenter.kt

+    }
+}
+
+data class ChooseSelfVerificationModeState(


Extract to own file?

bmarty · 2025-03-06T14:16:44Z

.../android/features/ftue/impl/sessionverification/choosemode/ChooseSelfVerificationModeView.kt

+) {
+    val activity = LocalActivity.current
+    BackHandler {
+        activity?.finish()


That seems a bit brutal to close the app on a back press. Maybe later we will add a dialog or something?

This screen is displayed after logging in, or if you haven't verified your session and close and reopen the app. It can't go to a previous state, so I think closing the app is ok, but I can check why I did it like this because just pressing back should have the same effect through the usual navigation flow. Something tells me there was something weird with said navigation flow 😅 .

I see, it's because the NoOpBackstackHandlerStrategy used in FtueFlowNode prevents the user from going back. I think we can remove that instead: AFAICT, pressing back here will close the app, but then again, the state should be restored when reopened.

I understand that the user cannot go back, but a dialog would help to understand that verification is mandatory to go further maybe. Here when clicking back, user may think that the application just crashed.

Not blocking the PR though.

I confirm that the state is restored. I guess it's OK then!

bmarty · 2025-03-06T14:18:14Z

...tures/ftue/impl/sessionverification/choosemode/ChooseSessionVerificationModePresenterTest.kt

+        }
+    }
+
+    fun createPresenter(


Suggested change

fun createPresenter(

private fun createPresenter(

jmartinesp · 2025-03-06T14:57:49Z

I need to click twice on "Start verification" for something to happen. It seems that the screen is added twice on the backstack since at the end I need to press twice on the back arrow.

I can't reproduce it 🫤 :

navigation.mp4

One question, the "Verified" decoration (green dot with check) for verified user in timeline top bar and room list item will come later?

All the status badges will come in a different PR. I'm not sure if the room list one is feasible though.

jmartinesp · 2025-03-06T15:10:26Z

I can't reproduce it 🫤

There is a weird flash in your recording though, it seems like some other state is being displayed for a short while. Could you maybe add some logs or attach the debugger to try to understand what's happening?

bmarty · 2025-03-06T15:21:08Z

I can't reproduce it 🫤

There is a weird flash in your recording though, it seems like some other state is being displayed for a short while. Could you maybe add some logs or attach the debugger to try to understand what's happening?

I got this log on first click on "Verify"

org.matrix.rustcomponents.sdk.ClientException$Generic: msg=encryption failed due to an error collecting the recipient devices: one or more verified users have unsigned devices
...
elementx: Session verification failed with an unknown error

On the second attempt I got the same log, but the state machine goes further.

Full logs are in https://github.com/element-hq/element-x-android-rageshakes/issues/4613

jmartinesp · 2025-03-06T15:37:37Z

Ok, so it is caused by the error you mentioned. This line moves the state back to initial, maybe it should go to failed.

It seems like it's cause by your account having some unverified session, which prevents sending the request verification in the room, AFAICT. I'm asking product/design what we can do about this issue. Either we prevent the user from starting the verification at all or we should display some helpful error messages so they know what's wrong.

bmarty · 2025-03-07T10:00:49Z

Thanks. It's strange that on second click I can go further and actually do the verification.

jmartinesp · 2025-03-07T10:15:26Z

Thanks. It's strange that on second click I can go further and actually do the verification.

Yeah, I think it's because we need the change I mentioned:

This line moves the state back to initial, maybe it should go to failed.

Otherwise the UI and the state machine states differ and we have that issue.

…ModeView`

…est screen

…rDevice`

…their own files

…tate will be restored when the app is reopened.

Then, when resetting the state machine state also reset the verification service.

sonarqubecloud · 2025-03-07T11:49:49Z

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

jmartinesp · 2025-03-10T08:07:20Z

If the rest of the issues are fixed, can we move this PR forward even if it has the issue with the unverified sessions canceling the flow? iOS has the same problem, and the feature was delivered weeks ago. Also, it needs product/design feedback so it'll take a while, blocking other PRs related to user verification.

EDIT: apparently, there's even some bug report so it can be fixed in the Rust SDK.

bmarty

I guess it's fine, thanks!

jmartinesp force-pushed the feat/jme/add-user-verification branch from 680f231 to 4ad8fa1 Compare March 6, 2025 08:25

jmartinesp marked this pull request as ready for review March 6, 2025 10:20

jmartinesp requested a review from a team as a code owner March 6, 2025 10:20

jmartinesp requested review from bmarty and removed request for a team March 6, 2025 10:20

bmarty reviewed Mar 6, 2025

View reviewed changes

jmartinesp and others added 15 commits March 7, 2025 11:37

Add support for starting verification of a user

73454b6

Add support for replying to incoming user verification requests

3480d44

Add reset recovery key button and previews to `ChooseSelfVerification…

87f32b4

…ModeView`

Add and fix tests, fix lint issues

e2f0302

Add 'Profile' item in room details screen

13d8f66

Delay displaying the incoming user verification until the UI is ready

88befce

Fix issues after rebase

320323f

Update screenshots

53bf1a7

Use right copy for 'verify user' action

34fcd79

Add padding between text items in the incoming user verification requ…

a653a54

…est screen

Fix icons in the outgoing user verification

f95748d

Fix icons in the incoming user verification screens

a15ece9

Remove showDeviceVerifiedScreen parameter from `NavTarget.UseAnothe…

a973dd9

…rDevice`

Fix minor naming/visibility issues

2b6a3e8

Split event and state for ChooseSelfVerificationModePresenter into …

3f87591

…their own files

jmartinesp added 3 commits March 7, 2025 11:53

Allow exiting the FTUE flow, which will close the app. The previous s…

18b8a36

…tate will be restored when the app is reopened.

When outgoing verification fails, move to the Canceled state.

54babec

Then, when resetting the state machine state also reset the verification service.

Remove some dead code

995a122

jmartinesp force-pushed the feat/jme/add-user-verification branch from 7a918e8 to 995a122 Compare March 7, 2025 10:58

jmartinesp added the PR-Feature For a new feature label Mar 7, 2025

Update screenshots

e1b03bc

jmartinesp requested a review from bmarty March 10, 2025 08:05

bmarty approved these changes Mar 10, 2025

View reviewed changes

jmartinesp merged commit f73c0e4 into develop Mar 10, 2025
28 checks passed

jmartinesp deleted the feat/jme/add-user-verification branch March 10, 2025 10:20

	fun bindChooseVerificationMethodPresenter(presenter: ChooseSelfVerificationModePresenter): Presenter<ChooseSelfVerificationModeState>
	fun bindChooseSelfVerificationModePresenter(presenter: ChooseSelfVerificationModePresenter): Presenter<ChooseSelfVerificationModeState>

	object SignOut : ChooseSelfVerificationModeEvent
	data object SignOut : ChooseSelfVerificationModeEvent

Implement user verification #4294

Implement user verification #4294

Conversation

jmartinesp commented Feb 21, 2025 • edited Loading

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

github-actions bot commented Feb 21, 2025 • edited Loading

codecov bot commented Feb 21, 2025 • edited Loading

Codecov Report

bmarty left a comment • edited Loading

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

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

jmartinesp Mar 6, 2025 • edited Loading

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

jmartinesp commented Mar 6, 2025

jmartinesp commented Mar 6, 2025 • edited Loading

bmarty commented Mar 6, 2025

jmartinesp commented Mar 6, 2025 • edited Loading

bmarty commented Mar 7, 2025

jmartinesp commented Mar 7, 2025 • edited Loading

sonarqubecloud bot commented Mar 7, 2025

Quality Gate passed

jmartinesp commented Mar 10, 2025 • edited Loading

bmarty left a comment

Choose a reason for hiding this comment

jmartinesp commented Feb 21, 2025 •

edited

Loading

github-actions bot commented Feb 21, 2025 •

edited

Loading

codecov bot commented Feb 21, 2025 •

edited

Loading

bmarty left a comment •

edited

Loading

jmartinesp Mar 6, 2025 •

edited

Loading

jmartinesp commented Mar 6, 2025 •

edited

Loading

jmartinesp commented Mar 6, 2025 •

edited

Loading

jmartinesp commented Mar 7, 2025 •

edited

Loading

jmartinesp commented Mar 10, 2025 •

edited

Loading