Feature: identity reset #3298

jmartinesp · 2024-08-12T11:51:18Z

Content

Moves OIDC related code to their own modules: libraries/oidc.
Improves the custom chrome tab implementation and fixes issues with the fallback webview one (it was freezing after signing in).
Creates OidcEntryPoint which allows opening any of the OIDC flow options (custom tab, webview).
Adds new flows for identity reset: root screen with confirmation, OIDC one which uses the extracted OIDC logic, user+password one.
Create IdentityResetHandle wrappers for both OIDC and password variants.

Motivation and context

Closes #3268.

Screenshots / GIFs

In the PR.

Tests

With a user + password based account whose data you don't mind losing:

Log in.
Select 'Can't confirm?'.
Continue, add your password and confirm.
You should be fully logged in now and the system will ask you to add key backup.

With an OIDC one (maybe using synapse-oidc.element.dev?):

Log in.
Select 'Can't confirm?'.
Continue, confirm the reset in the custom tab/webview, then go back.
You should be fully logged in now and the system will ask you to add key backup.

Tested devices

Physical
Emulator
OS version(s): 13

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

github-actions · 2024-08-12T12:01:18Z

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

codecov · 2024-08-12T12:40:25Z

Codecov Report

Attention: Patch coverage is 84.92647% with 41 lines in your changes missing coverage. Please review.

Project coverage is 82.56%. Comparing base (45775d7) to head (99efdb6).
Report is 17 commits behind head on develop.

Files	Patch %	Lines
...nt/android/libraries/oidc/impl/webview/OidcView.kt	52.63%	8 Missing and 1 partial ⚠️
...droid/libraries/oidc/impl/DefaultOidcEntryPoint.kt	0.00%	8 Missing ⚠️
.../matrix/test/encryption/FakeIdentityResetHandle.kt	44.44%	5 Missing ⚠️
...p/impl/reset/password/ResetIdentityPasswordView.kt	91.30%	0 Missing and 4 partials ⚠️
.../libraries/androidutils/browser/ChromeCustomTab.kt	0.00%	4 Missing ⚠️
...ecurebackup/impl/reset/ResetIdentityFlowManager.kt	87.50%	0 Missing and 3 partials ⚠️
...l/reset/password/ResetIdentityPasswordPresenter.kt	87.50%	1 Missing and 1 partial ⚠️
.../impl/reset/root/ResetIdentityRootStateProvider.kt	75.00%	2 Missing ⚠️
...eatures/securebackup/api/SecureBackupEntryPoint.kt	0.00%	1 Missing ⚠️
...ckup/impl/reset/root/ResetIdentityRootPresenter.kt	88.88%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3298      +/-   ##
===========================================
- Coverage    82.56%   82.56%   -0.01%     
===========================================
  Files         1662     1675      +13     
  Lines        38992    39204     +212     
  Branches      4724     4745      +21     
===========================================
+ Hits         32193    32367     +174     
- Misses        5133     5163      +30     
- Partials      1666     1674       +8

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

bmarty

A few first remarks.

I have tested the reset using password, it's working as expected.
Also tested using Oidc, working as expected.

bmarty · 2024-08-14T09:30:01Z

...in/io/element/android/features/securebackup/impl/reset/password/ResetIdentityPasswordView.kt

+    } else if (state.resetAction.isFailure()) {
+        ErrorDialog(
+            content = stringResource(CommonStrings.error_unknown),
+            onDismiss = { state.eventSink(ResetIdentityPasswordEvent.DismissError) }


Can we inline the error in the password field when the password is not correct?
We may also want to improve the error message in this case:

Note that the error in this case is not very explicit: the server returned an error: User-Interactive Authentication required.

bmarty · 2024-08-14T09:31:16Z

...in/io/element/android/features/securebackup/impl/reset/password/ResetIdentityPasswordView.kt

+                modifier = Modifier.fillMaxWidth(),
+                text = stringResource(CommonStrings.action_reset_identity),
+                onClick = { state.eventSink(ResetIdentityPasswordEvent.Reset(passwordState.value)) },
+                destructive = true,


Maybe disable the button when the password is empty?

bmarty · 2024-08-14T09:35:06Z

...ain/kotlin/io/element/android/features/securebackup/impl/reset/root/ResetIdentityRootView.kt

+            },
+            destructiveSubmit = true,
+            onDismiss = { state.eventSink(ResetIdentityRootEvent.DismissDialog) }
+        )


There is a wording inconsistency: will the user reset encryption or identity (whatever this may mean for a non-technical user...)

Note: on OIDC web page this is yet another wording:

The latest agreed terminology is "Reset identity". See https://github.com/matrix-org/matrix-spec-proposals/blob/andybalaam/crypto-terminology/proposals/4161-crypto-terminology.md#identity as well as the Figma designs. The MAS screen will be updated with the next release.

FWIW, your first screen is outdated. Figma designs have been updated since.

You mean this one?

You mean this one?

I meant the first one of this post.

But the one you mentioned is actually outdated in the designs, yes. It should say "go to your account to reset your identity".

I have requested to access the Figma file (again)

bmarty · 2024-08-14T09:36:29Z

...src/main/kotlin/io/element/android/features/securebackup/impl/reset/ResetIdentityFlowNode.kt

+        lifecycle.addObserver(object : DefaultLifecycleObserver {
+            override fun onStart(owner: LifecycleOwner) {
+                // If the custom tab was opened, we need to cancel the reset job
+                // when we come back to the node if it the reset wasn't successful


Suggested change

// when we come back to the node if it the reset wasn't successful

// when we come back to the node if the reset wasn't successful

bmarty · 2024-08-14T09:45:30Z

...src/main/kotlin/io/element/android/features/securebackup/impl/reset/ResetIdentityFlowNode.kt

+        val handle = resetIdentityFlowManager.getResetHandle()
+            .filterIsInstance<AsyncData.Success<IdentityResetHandle>>()
+            .first()
+            .data


This will ignore any Failure coming in the flow. Can it be a problem?
Maybe add a log, just in case...

bmarty · 2024-08-14T10:00:18Z

libraries/oidc/api/build.gradle.kts

@@ -0,0 +1,27 @@
+/*
+ * Copyright (c) 2023 New Vector Ltd


Suggested change

* Copyright (c) 2023 New Vector Ltd

* Copyright (c) 2024 New Vector Ltd

bmarty · 2024-08-14T10:01:23Z

libraries/oidc/impl/build.gradle.kts

@@ -0,0 +1,71 @@
+/*
+ * Copyright (c) 2022 New Vector Ltd


Suggested change

* Copyright (c) 2022 New Vector Ltd

* Copyright (c) 2024 New Vector Ltd

bmarty · 2024-08-14T10:01:43Z

libraries/oidc/impl/build.gradle.kts

+    testImplementation(projects.libraries.matrix.test)
+    testImplementation(projects.libraries.permissions.test)
+    testImplementation(projects.tests.testutils)
+    testReleaseImplementation(libs.androidx.compose.ui.test.manifest)


I think some unused dependencies can be removed here.

bmarty · 2024-08-14T10:03:10Z

...hots/images/features.securebackup.impl.reset.password_ResetIdentityPasswordView_Day_0_en.png

Wording: reset encryption or reset identity?

bmarty · 2024-08-14T10:14:15Z

.../main/kotlin/io/element/android/features/securebackup/impl/reset/ResetIdentityFlowManager.kt

+    val currentHandleFlow: StateFlow<AsyncData<IdentityResetHandle>> = resetHandleFlow
+
+    fun whenResetIsDone(block: () -> Unit) {
+        sessionCoroutineScope.launch {


We need to cancel this job if the user click go back instead of going to the end of the flow (or use the presenter / node scope)

sonarqubecloud · 2024-08-14T15:44:33Z

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!

jmartinesp added the PR-Feature For a new feature label Aug 12, 2024

jmartinesp marked this pull request as ready for review August 13, 2024 06:47

jmartinesp requested a review from a team as a code owner August 13, 2024 06:47

jmartinesp requested review from bmarty and removed request for a team August 13, 2024 06:47

jmartinesp and others added 13 commits August 13, 2024 08:55

Initial implementation of the reset identity feature

4ab0b10

Extract OIDC to its own module

ee7a31c

Use extracted OIDC flow to display the reset identity screen

abb9959

Improve existing APIs

175bada

Improve APIs, add tests

7fd0ad0

Fix lint issues

5239e5c

Fix OidcView preview

ae18efd

Update screenshots

300df0c

Fix vertical position for the primary button in VerifySelfSessionView

c02f013

Fix Konsist failures

5e3e127

Update screenshots

d40f5bd

Remove unused import

f41e42d

Add an extra test

d5eb71a

jmartinesp force-pushed the feat/jme/3268-crypto-identity-reset branch from de7f819 to d5eb71a Compare August 13, 2024 06:55

bmarty reviewed Aug 14, 2024

View reviewed changes

jmartinesp and others added 2 commits August 14, 2024 16:53

Fix most review comments

8a1e333

Update screenshots

99efdb6

bmarty approved these changes Aug 14, 2024

View reviewed changes

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

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

bmarty merged commit b62348f into develop Aug 16, 2024
28 of 29 checks passed

bmarty deleted the feat/jme/3268-crypto-identity-reset branch August 16, 2024 07:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: identity reset #3298

Feature: identity reset #3298

jmartinesp commented Aug 12, 2024 •

edited

Loading

github-actions bot commented Aug 12, 2024 •

edited

Loading

codecov bot commented Aug 12, 2024 •

edited

Loading

bmarty left a comment •

edited

Loading

bmarty Aug 14, 2024

bmarty Aug 14, 2024

bmarty Aug 14, 2024 •

edited

Loading

pmaier1 Aug 22, 2024

pmaier1 Aug 22, 2024

jmartinesp Aug 22, 2024

pmaier1 Aug 22, 2024

bmarty Aug 22, 2024

bmarty Aug 14, 2024

bmarty Aug 14, 2024

bmarty Aug 14, 2024

bmarty Aug 14, 2024

bmarty Aug 14, 2024

bmarty Aug 14, 2024

bmarty Aug 14, 2024 •

edited

Loading

sonarqubecloud bot commented Aug 14, 2024

bmarty left a comment

	// when we come back to the node if it the reset wasn't successful
	// when we come back to the node if the reset wasn't successful

	* Copyright (c) 2023 New Vector Ltd
	* Copyright (c) 2024 New Vector Ltd

	* Copyright (c) 2022 New Vector Ltd
	* Copyright (c) 2024 New Vector Ltd

Feature: identity reset #3298

Feature: identity reset #3298

Conversation

jmartinesp commented Aug 12, 2024 • edited Loading

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

github-actions bot commented Aug 12, 2024 • edited Loading

codecov bot commented Aug 12, 2024 • 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

bmarty Aug 14, 2024 • 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

Choose a reason for hiding this comment

bmarty Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

sonarqubecloud bot commented Aug 14, 2024

Quality Gate passed

bmarty left a comment

Choose a reason for hiding this comment

jmartinesp commented Aug 12, 2024 •

edited

Loading

github-actions bot commented Aug 12, 2024 •

edited

Loading

codecov bot commented Aug 12, 2024 •

edited

Loading

bmarty left a comment •

edited

Loading

bmarty Aug 14, 2024 •

edited

Loading

bmarty Aug 14, 2024 •

edited

Loading