Fix verification failed issue, simplify verification logic #3830

jmartinesp · 2024-11-07T14:45:47Z

Content

Avoid unnecessary initVerificationControllerIfNeeded call which caused the automatic verification flow failure.
Reuse Rust Client instances created on the login process so we don't need to restore one right before the session verification.
Remove unnecessary sources of verification state updates.
Add an intermediate FTUE flow step which will display an indeterminate progress indicator instead of a blank screen.

Motivation and context

This should fix the issues we've seen where the authentication failed automatically due to a race condition.

Keeping the Client instances created on login would also make our setup more similar to iOS and avoid unnecessary work.

Tests

Login and verify your device (self-verification).
Try verifying some other device from this one.

Tested devices

Physical
Emulator
OS version(s): 14

Checklist

Changes have been tested on an Android device or Android emulator with API 23
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
Pull request includes a sign off
You've made a self review of your PR

- Reuse Rust `Client` instances created on the login process so we don't need to restore one right before the session verification. - Remove unnecessary sources of verification state updates. - Add an intermediate FTUE flow step which will display an indeterminate progress indicator instead of a blank screen.

jmartinesp · 2024-11-07T14:51:08Z

...tlin/io/element/android/libraries/matrix/impl/verification/RustSessionVerificationService.kt

-            // Otherwise, just check the current verification status from the session verification controller instead
-            Timber.d("Updating verification status: flow is pending or was finished some time ago")
-            runCatching {
-                initVerificationControllerIfNeeded()


This line here is what caused the automatic verification failure if it was called before the verification service was ready.

github-actions · 2024-11-07T14:51:22Z

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

bmarty

A first few remarks.

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

.../ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/state/DefaultFtueService.kt

…vice usage.

...src/main/kotlin/io/element/android/libraries/matrix/api/auth/ClientAuthenticationObserver.kt

codecov · 2024-11-07T15:58:09Z

Codecov Report

Attention: Patch coverage is 81.25000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 82.98%. Comparing base (cf9dbc6) to head (0b00150).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
...atrix/test/auth/FakeMatrixAuthenticationService.kt	63.63%	0 Missing and 4 partials ⚠️
...oid/features/ftue/impl/state/DefaultFtueService.kt	86.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3830      +/-   ##
===========================================
- Coverage    82.98%   82.98%   -0.01%     
===========================================
  Files         1783     1783              
  Lines        44971    44980       +9     
  Branches      5289     5293       +4     
===========================================
+ Hits         37320    37327       +7     
+ Misses        5805     5803       -2     
- Partials      1846     1850       +4

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

bmarty

Some remarks, still testing on my side

...main/kotlin/io/element/android/libraries/matrix/impl/auth/RustMatrixAuthenticationService.kt

...src/main/kotlin/io/element/android/libraries/matrix/api/auth/ClientAuthenticationObserver.kt

bmarty

I'll merge develop on it and do more test, but it looks OK!

bmarty · 2024-11-08T13:15:19Z

appnav/src/test/kotlin/io/element/android/appnav/di/MatrixClientsHolderTest.kt

@@ -81,4 +81,17 @@ class MatrixClientsHolderTest {
        matrixClientsHolder.restoreWithSavedState(savedStateMap)
        assertThat(matrixClientsHolder.getOrNull(A_SESSION_ID)).isEqualTo(fakeMatrixClient)
    }
+
+    @Test
+    fun `test ClientAuthenticationObserver emits a value and we save it`() = runTest {


Maybe rename this test now that ClientAuthenticationObserver has been deleted.

bmarty · 2024-11-08T13:18:47Z

.../ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/state/DefaultFtueService.kt

-            }
-            .first()
-        return readyVerifiedSessionStatus == SessionVerifiedStatus.NotVerified && !canSkipVerification()
+        // Wait until the session verification status is known


Neat: the @OptIn can be removed.

bmarty · 2024-11-08T13:19:20Z

.../ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/state/DefaultFtueService.kt

+     * In this case, the FTUE flow is ready when the session verification status is known.
+     */
+    @OptIn(FlowPreview::class)
+    val isVerificationStatusKnown = sessionVerificationService.sessionVerifiedStatus


Neat: the @OptIn can be removed (twice in this file).

bmarty · 2024-11-08T13:21:36Z

...es/ftue/impl/src/test/kotlin/io/element/android/features/ftue/impl/DefaultFtueServiceTest.kt

+            analyticsService = analyticsService,
+            permissionStateProvider = permissionStateProvider,
+            lockScreenService = lockScreenService,
+        )


Thanks for this new test!

sonarcloud · 2024-11-08T14:55:55Z

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 SonarCloud

bmarty

Thanks for the update.
I have done more tests and this seems to be fine.
We will need to handle the problem when Element Web is accepting an incoming verification request even if it does not know the private key. In this case, the Element X session will not become verified.

This is tracked by element-hq/element-web#27655

jmartinesp added the PR-Bugfix For bug fix label Nov 7, 2024

jmartinesp requested a review from a team as a code owner November 7, 2024 14:45

jmartinesp requested review from bmarty and removed request for a team November 7, 2024 14:45

jmartinesp added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Nov 7, 2024

Remove unnecessary workaround: the SDK should already handle this

c6a7031

jmartinesp commented Nov 7, 2024

View reviewed changes

bmarty reviewed Nov 7, 2024

View reviewed changes

features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/FtueFlowNode.kt Outdated Show resolved Hide resolved

.../ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/state/DefaultFtueService.kt Show resolved Hide resolved

Fix first review commits, add regression tests for noop analytics ser…

536f712

…vice usage.

ganfra reviewed Nov 7, 2024

View reviewed changes

...src/main/kotlin/io/element/android/libraries/matrix/api/auth/ClientAuthenticationObserver.kt Outdated Show resolved Hide resolved

jmartinesp added 2 commits November 7, 2024 16:15

Remove unused imports

2e0dfef

Add services.analytics.noop module to the test dependencies

bb51db1

bmarty reviewed Nov 8, 2024

View reviewed changes

Apply patch and review suggestions

d36d779

bmarty approved these changes Nov 8, 2024

View reviewed changes

bmarty and others added 2 commits November 8, 2024 14:32

Merge branch 'develop' into fix/jme/simplify-verification-service

3693d34

Fix nits

0b00150

bmarty approved these changes Nov 8, 2024

View reviewed changes

bmarty added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Nov 8, 2024

github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Nov 8, 2024

jmartinesp merged commit 2206e94 into develop Nov 8, 2024
29 of 30 checks passed

jmartinesp deleted the fix/jme/simplify-verification-service branch November 8, 2024 15:42

jmartinesp mentioned this pull request Nov 8, 2024

Try waiting for the encryption service to be ready before getting the… #3807

Closed

10 tasks

bmarty mentioned this pull request Nov 12, 2024

EXA briefly displays passphrase / authentication screen on application launch as of 0.7.2 #3783

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix verification failed issue, simplify verification logic #3830

Fix verification failed issue, simplify verification logic #3830

jmartinesp commented Nov 7, 2024 •

edited

Loading

jmartinesp Nov 7, 2024

github-actions bot commented Nov 7, 2024 •

edited

Loading

bmarty left a comment

codecov bot commented Nov 7, 2024 •

edited

Loading

bmarty left a comment

bmarty left a comment

bmarty Nov 8, 2024

bmarty Nov 8, 2024

bmarty Nov 8, 2024

bmarty Nov 8, 2024

sonarcloud bot commented Nov 8, 2024

bmarty left a comment

Fix verification failed issue, simplify verification logic #3830

Fix verification failed issue, simplify verification logic #3830

Conversation

jmartinesp commented Nov 7, 2024 • edited Loading

Content

Motivation and context

Tests

Tested devices

Checklist

jmartinesp Nov 7, 2024

Choose a reason for hiding this comment

github-actions bot commented Nov 7, 2024 • edited Loading

bmarty left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 7, 2024 • edited Loading

Codecov Report

bmarty left a comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

bmarty Nov 8, 2024

Choose a reason for hiding this comment

bmarty Nov 8, 2024

Choose a reason for hiding this comment

bmarty Nov 8, 2024

Choose a reason for hiding this comment

bmarty Nov 8, 2024

Choose a reason for hiding this comment

sonarcloud bot commented Nov 8, 2024

Quality Gate passed

bmarty left a comment

Choose a reason for hiding this comment

jmartinesp commented Nov 7, 2024 •

edited

Loading

github-actions bot commented Nov 7, 2024 •

edited

Loading

codecov bot commented Nov 7, 2024 •

edited

Loading