[SDK] Allow passwords to be set at the point of reset confirmation #6171

ouchadam · 2022-05-27T14:59:32Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Updates the LoginWizard reset api to allow passing the new password at the point of confirmation

Removes the newPassword parameter from LoginWizard.resetPassword in favour of only supply it as part of resetPasswordMailConfirmed
Moves the reset password new password to the ViewModel state, emitting an error if the password has not been set
Also introduces a ResetState to the OnboardingViewModel in preparation for the flow changes

Motivation and context

To enable the FTUE reset password journey which involves supplying a password after the reset email has been confirmed instead of the current flow which involves passing the email and password upfront.

Screenshots / GIFs

No UI changes

Tests

No production changes

Tested devices

Physical
Emulator
OS version(s): 31

github-actions · 2022-05-27T15:30:17Z

Unit Test Results

146 files 146 suites 2m 51s ⏱️
238 tests 238 ✔️ 0 💤 0 ❌
796 runs 796 ✔️ 0 💤 0 ❌

Results for commit edfabb0.

♻️ This comment has been updated with latest results.

bmarty

LGTM;
But having two ways to do the same thing is maybe not ideal from a SDK user POV.

Have you considered fully remove newPassword parameter in resetPassword?

I would vote to do so, no matter if there is an API break.

bmarty · 2022-05-30T08:52:26Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/auth/login/LoginWizard.kt

     */
-    suspend fun resetPassword(email: String,
-                              newPassword: String)
+    suspend fun resetPassword(email: String, newPassword: String? = null)


When I created this API, I was following the UI, when the form was asking for the email and the new password. I have checked and it's fine to provide the Pwd later.

bmarty · 2022-05-30T08:56:54Z

...x-sdk-android/src/main/java/org/matrix/android/sdk/internal/auth/login/DefaultLoginWizard.kt

+        return when (resetPasswordData) {
+            null -> throw IllegalStateException("developer error, no reset password in progress")
+            else -> {
+                val password = newPassword ?: resetPasswordData.newPassword ?: throw IllegalStateException("developer error, no new password set")


Do we want to enforce that newPassword and resetPasswordData.newPassword are not not null simultaneously? Or are the same?

bmarty · 2022-05-30T08:58:48Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/auth/login/LoginWizard.kt

@@ -63,14 +63,15 @@ interface LoginWizard {
     * [resetPasswordMailConfirmed] is successfully called.
     *
     * @param email an email previously associated to the account the user wants the password to be reset.
-     * @param newPassword the desired new password
+     * @param newPassword the desired new password, can be optionally set here or as part of [resetPasswordMailConfirmed]


Maybe add the point that if the pwd is provided here, it will be temporarily stored in the SDK encrypted DB.
To avoid that consider passing the pwd in [resetPasswordMailConfirmed]

ouchadam · 2022-05-30T09:04:40Z

Have you considered fully remove newPassword parameter in resetPassword?

I originally had this but wasn't sure of the consequences of no longer persisting the password within the pendingState in the database

I'm happy to update and remove!

ouchadam · 2022-05-30T09:58:02Z

will close and rework to provide a single way to reset the password

ouchadam · 2022-05-30T13:02:25Z

vector/src/main/java/im/vector/app/features/onboarding/OnboardingViewModel.kt


-        if (safeLoginWizard == null) {
-            setState { copy(isLoading = false) }


by making the loginWizard non null (which the compiler is warning is impossible to be null) we can simplify the branching

I have noticed this warning. I think this is relatively new, probably since a Kotlin version upgrade. I am also wondering why the compiler is not failing, since warnings are errors on our project. So looks more like a IDE inspection warning.

bmarty

LGTM!

Claire1817 · 2022-05-31T13:49:07Z

vector/src/main/java/im/vector/app/features/login2/LoginViewModel2.kt

@@ -470,7 +472,8 @@ class LoginViewModel2 @AssistedInject constructor(

            currentJob = viewModelScope.launch {
                try {
-                    safeLoginWizard.resetPasswordMailConfirmed()
+                    val state = awaitState()
+                    safeLoginWizard.resetPasswordMailConfirmed(state.resetPasswordNewPassword!!)


is it possible to avoid the not-null assertion operator ?

it's possible (we're avoiding it in the OnboardingViewModel) but I've taken a shortcut here in the LoginViewModel2 as this entire package will be removed once the FTUE is finished

for context, Login2 is the proof of concept which has lead to the FTUE project

I'd prefer to avoid making tooo many changes to this package, but would be happy to make the change if you feel it's worth doing 👍

…t or as part of the confirmation step

- the new password is moved to the in memory view model state

…set password new password is not set

- adds reset test cases to the onboarding view model

Claire1817

Tested: Ok :)

ouchadam added Z-FTUE Issue is relevant to the first time use project or experience matrix-sdk PR-Small PR with less than 20 updated lines labels May 27, 2022

bmarty reviewed May 30, 2022

View reviewed changes

ouchadam closed this May 30, 2022

ouchadam reopened this May 30, 2022

ouchadam commented May 30, 2022

View reviewed changes

ouchadam requested review from a team, Claire1817, ariskotsomitopoulos and bmarty and removed request for a team May 30, 2022 13:17

ouchadam mentioned this pull request May 30, 2022

Mobile FTUE: Password recovery (Android) #5284

Closed

bmarty approved these changes May 30, 2022

View reviewed changes

ouchadam mentioned this pull request May 31, 2022

SDK - Logout all devices #6207

Merged

6 tasks

Claire1817 reviewed May 31, 2022

View reviewed changes

ouchadam force-pushed the feature/adm/sdk-new-password-on-confirmation branch from 3ae103c to 2f51280 Compare June 1, 2022 16:19

ouchadam removed the request for review from ariskotsomitopoulos June 1, 2022 16:19

ouchadam added 7 commits June 6, 2022 13:57

introducing a reset state for holding onto the reset inputs

e3d46cf

allow passing the new password when resetting passwords either upfron…

35163f7

…t or as part of the confirmation step

moving the reset password new password to the reset confirmation step

cc8f17b

- the new password is moved to the in memory view model state

converting if/else and try/catch to when and runCatching

93a247e

removing impossible case

32389a9

adding dedicated fail event in the legacy onboarding flow when the re…

fa5b7c6

…set password new password is not set

adding missing loading state when confirming password reset

edfabb0

- adds reset test cases to the onboarding view model

ouchadam force-pushed the feature/adm/sdk-new-password-on-confirmation branch from 2f51280 to edfabb0 Compare June 6, 2022 12:59

ouchadam requested a review from Claire1817 June 7, 2022 09:00

Claire1817 approved these changes Jun 7, 2022

View reviewed changes

ouchadam merged commit 462d307 into develop Jun 7, 2022

ouchadam deleted the feature/adm/sdk-new-password-on-confirmation branch June 7, 2022 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SDK] Allow passwords to be set at the point of reset confirmation #6171

[SDK] Allow passwords to be set at the point of reset confirmation #6171

ouchadam commented May 27, 2022 •

edited

Loading

github-actions bot commented May 27, 2022 •

edited

Loading

bmarty left a comment •

edited

Loading

bmarty May 30, 2022

bmarty May 30, 2022

bmarty May 30, 2022

ouchadam commented May 30, 2022

ouchadam commented May 30, 2022

ouchadam May 30, 2022 •

edited

Loading

bmarty May 30, 2022

bmarty left a comment

Claire1817 May 31, 2022

ouchadam May 31, 2022 •

edited

Loading

Claire1817 left a comment


		if (safeLoginWizard == null) {
		setState { copy(isLoading = false) }

[SDK] Allow passwords to be set at the point of reset confirmation #6171

[SDK] Allow passwords to be set at the point of reset confirmation #6171

Conversation

ouchadam commented May 27, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

github-actions bot commented May 27, 2022 • edited Loading

Unit Test Results

bmarty left a comment • edited Loading

Choose a reason for hiding this comment

bmarty May 30, 2022

Choose a reason for hiding this comment

bmarty May 30, 2022

Choose a reason for hiding this comment

bmarty May 30, 2022

Choose a reason for hiding this comment

ouchadam commented May 30, 2022

ouchadam commented May 30, 2022

ouchadam May 30, 2022 • edited Loading

Choose a reason for hiding this comment

bmarty May 30, 2022

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

Claire1817 May 31, 2022

Choose a reason for hiding this comment

ouchadam May 31, 2022 • edited Loading

Choose a reason for hiding this comment

Claire1817 left a comment

Choose a reason for hiding this comment

ouchadam commented May 27, 2022 •

edited

Loading

github-actions bot commented May 27, 2022 •

edited

Loading

bmarty left a comment •

edited

Loading

ouchadam May 30, 2022 •

edited

Loading

ouchadam May 31, 2022 •

edited

Loading