Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Jun 7, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e3d46cf
introducing a reset state for holding onto the reset inputs
ouchadam May 26, 2022
35163f7
allow passing the new password when resetting passwords either upfron…
ouchadam May 27, 2022
cc8f17b
moving the reset password new password to the reset confirmation step
ouchadam May 30, 2022
93a247e
converting if/else and try/catch to when and runCatching
ouchadam May 30, 2022
32389a9
removing impossible case
ouchadam May 30, 2022
fa5b7c6
adding dedicated fail event in the legacy onboarding flow when the re…
ouchadam May 30, 2022
edfabb0
adding missing loading state when confirming password reset
ouchadam May 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/6169.sdk
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Allows new passwords to be passed at the point of confirmation when resetting a password
10 changes: 4 additions & 6 deletions matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/auth/login/LoginWizard.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,16 +65,14 @@ 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
*/
suspend fun resetPassword(
email: String,
newPassword: String
)
suspend fun resetPassword(email: String)

/**
* Confirm the new password, once the user has checked their email
* When this method succeed, tha account password will be effectively modified.
*
* @param newPassword the desired new password
*/
suspend fun resetPasswordMailConfirmed()
suspend fun resetPasswordMailConfirmed(newPassword: String)
}
14 changes: 6 additions & 8 deletions ...dk-android/src/main/java/org/matrix/android/sdk/internal/auth/login/DefaultLoginWizard.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ internal class DefaultLoginWizard(
return sessionCreator.createSession(credentials, pendingSessionData.homeServerConnectionConfig)
}

override suspend fun resetPassword(email: String, newPassword: String) {
override suspend fun resetPassword(email: String) {
val param = RegisterAddThreePidTask.Params(
RegisterThreePid.Email(email),
pendingSessionData.clientSecret,
Expand All @@ -117,18 +117,16 @@ internal class DefaultLoginWizard(
authAPI.resetPassword(AddThreePidRegistrationParams.from(param))
}

pendingSessionData = pendingSessionData.copy(resetPasswordData = ResetPasswordData(newPassword, result))
pendingSessionData = pendingSessionData.copy(resetPasswordData = ResetPasswordData(result))
.also { pendingSessionStore.savePendingSessionData(it) }
}

override suspend fun resetPasswordMailConfirmed() {
val safeResetPasswordData = pendingSessionData.resetPasswordData
?: throw IllegalStateException("developer error, no reset password in progress")

override suspend fun resetPasswordMailConfirmed(newPassword: String) {
val resetPasswordData = pendingSessionData.resetPasswordData ?: throw IllegalStateException("Developer error - Must call resetPassword first")
val param = ResetPasswordMailConfirmed.create(
pendingSessionData.clientSecret,
safeResetPasswordData.addThreePidRegistrationResponse.sid,
safeResetPasswordData.newPassword
resetPasswordData.addThreePidRegistrationResponse.sid,
newPassword
)

executeRequest(null) {
Expand Down
1 change: 0 additions & 1 deletion ...sdk-android/src/main/java/org/matrix/android/sdk/internal/auth/login/ResetPasswordData.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,5 @@ import org.matrix.android.sdk.internal.auth.registration.AddThreePidRegistration
*/
@JsonClass(generateAdapter = true)
internal data class ResetPasswordData(
val newPassword: String,
val addThreePidRegistrationResponse: AddThreePidRegistrationResponse
)
45 changes: 29 additions & 16 deletions vector/src/main/java/im/vector/app/features/login/LoginViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,8 @@ class LoginViewModel @AssistedInject constructor(
copy(
asyncResetPassword = Uninitialized,
asyncResetMailConfirmed = Uninitialized,
resetPasswordEmail = null
resetPasswordEmail = null,
resetPasswordNewPassword = null
)
}
}
Expand Down Expand Up @@ -488,7 +489,7 @@ class LoginViewModel @AssistedInject constructor(

currentJob = viewModelScope.launch {
try {
safeLoginWizard.resetPassword(action.email, action.newPassword)
safeLoginWizard.resetPassword(action.email)
} catch (failure: Throwable) {
setState {
copy(
Expand All @@ -501,7 +502,8 @@ class LoginViewModel @AssistedInject constructor(
setState {
copy(
asyncResetPassword = Success(Unit),
resetPasswordEmail = action.email
resetPasswordEmail = action.email,
resetPasswordNewPassword = action.newPassword
)
}

Expand Down Expand Up @@ -529,24 +531,35 @@ class LoginViewModel @AssistedInject constructor(
}

currentJob = viewModelScope.launch {
try {
safeLoginWizard.resetPasswordMailConfirmed()
} catch (failure: Throwable) {
val state = awaitState()

if (state.resetPasswordNewPassword == null) {
setState {
copy(
asyncResetMailConfirmed = Fail(failure)
asyncResetPassword = Uninitialized,
asyncResetMailConfirmed = Fail(Throwable("Developer error - New password not set"))
)
}
return@launch
}
setState {
copy(
asyncResetMailConfirmed = Success(Unit),
resetPasswordEmail = null
)
} else {
try {
safeLoginWizard.resetPasswordMailConfirmed(state.resetPasswordNewPassword)
} catch (failure: Throwable) {
setState {
copy(
asyncResetMailConfirmed = Fail(failure)
)
}
return@launch
}
setState {
copy(
asyncResetMailConfirmed = Success(Unit),
resetPasswordEmail = null,
resetPasswordNewPassword = null
)
}
_viewEvents.post(LoginViewEvents.OnResetPasswordMailConfirmationSuccess)
}

_viewEvents.post(LoginViewEvents.OnResetPasswordMailConfirmationSuccess)
}
}
}
Expand Down
2 changes: 2 additions & 0 deletions vector/src/main/java/im/vector/app/features/login/LoginViewState.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ data class LoginViewState(
@PersistState
val resetPasswordEmail: String? = null,
@PersistState
val resetPasswordNewPassword: String? = null,
@PersistState
val homeServerUrlFromUser: String? = null,

// Can be modified after a Wellknown request
Expand Down
14 changes: 9 additions & 5 deletions vector/src/main/java/im/vector/app/features/login2/LoginViewModel2.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,8 @@ class LoginViewModel2 @AssistedInject constructor(
LoginAction2.ResetResetPassword -> {
setState {
copy(
resetPasswordEmail = null
resetPasswordEmail = null,
resetPasswordNewPassword = null
)
}
}
Expand Down Expand Up @@ -443,7 +444,7 @@ class LoginViewModel2 @AssistedInject constructor(

currentJob = viewModelScope.launch {
try {
safeLoginWizard.resetPassword(action.email, action.newPassword)
safeLoginWizard.resetPassword(action.email)
} catch (failure: Throwable) {
_viewEvents.post(LoginViewEvents2.Failure(failure))
setState { copy(isLoading = false) }
Expand All @@ -453,7 +454,8 @@ class LoginViewModel2 @AssistedInject constructor(
setState {
copy(
isLoading = false,
resetPasswordEmail = action.email
resetPasswordEmail = action.email,
resetPasswordNewPassword = action.newPassword
)
}

Expand All @@ -472,7 +474,8 @@ class LoginViewModel2 @AssistedInject constructor(

currentJob = viewModelScope.launch {
try {
safeLoginWizard.resetPasswordMailConfirmed()
val state = awaitState()
safeLoginWizard.resetPasswordMailConfirmed(state.resetPasswordNewPassword!!)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@ouchadam ouchadam May 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

} catch (failure: Throwable) {
_viewEvents.post(LoginViewEvents2.Failure(failure))
setState { copy(isLoading = false) }
Expand All @@ -481,7 +484,8 @@ class LoginViewModel2 @AssistedInject constructor(
setState {
copy(
isLoading = false,
resetPasswordEmail = null
resetPasswordEmail = null,
resetPasswordNewPassword = null
)
}

Expand Down
2 changes: 2 additions & 0 deletions vector/src/main/java/im/vector/app/features/login2/LoginViewState2.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ data class LoginViewState2(
@PersistState
val resetPasswordEmail: String? = null,
@PersistState
val resetPasswordNewPassword: String? = null,
@PersistState
val homeServerUrlFromUser: String? = null,

// Can be modified after a Wellknown request
Expand Down
141 changes: 61 additions & 80 deletions vector/src/main/java/im/vector/app/features/onboarding/OnboardingViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ class OnboardingViewModel @AssistedInject constructor(
val isRegistrationStarted: Boolean
get() = authenticationService.isRegistrationStarted()

private val loginWizard: LoginWizard?
private val loginWizard: LoginWizard
get() = authenticationService.getLoginWizard()

private var loginConfig: LoginConfig? = null
Expand Down Expand Up @@ -246,21 +246,15 @@ class OnboardingViewModel @AssistedInject constructor(

private fun handleLoginWithToken(action: OnboardingAction.LoginWithToken) {
val safeLoginWizard = loginWizard
setState { copy(isLoading = true) }

if (safeLoginWizard == null) {
setState { copy(isLoading = false) }
Copy link
Contributor Author

@ouchadam ouchadam May 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

_viewEvents.post(OnboardingViewEvents.Failure(Throwable("Bad configuration")))
} else {
setState { copy(isLoading = true) }

currentJob = viewModelScope.launch {
try {
val result = safeLoginWizard.loginWithToken(action.loginToken)
onSessionCreated(result, authenticationDescription = AuthenticationDescription.Login)
} catch (failure: Throwable) {
setState { copy(isLoading = false) }
_viewEvents.post(OnboardingViewEvents.Failure(failure))
}
currentJob = viewModelScope.launch {
try {
val result = safeLoginWizard.loginWithToken(action.loginToken)
onSessionCreated(result, authenticationDescription = AuthenticationDescription.Login)
} catch (failure: Throwable) {
setState { copy(isLoading = false) }
_viewEvents.post(OnboardingViewEvents.Failure(failure))
}
}
}
Expand Down Expand Up @@ -377,7 +371,7 @@ class OnboardingViewModel @AssistedInject constructor(
setState {
copy(
isLoading = false,
resetPasswordEmail = null
resetState = ResetState()
)
}
}
Expand Down Expand Up @@ -447,59 +441,52 @@ class OnboardingViewModel @AssistedInject constructor(

private fun handleResetPassword(action: OnboardingAction.ResetPassword) {
val safeLoginWizard = loginWizard

if (safeLoginWizard == null) {
setState { copy(isLoading = false) }
_viewEvents.post(OnboardingViewEvents.Failure(Throwable("Bad configuration")))
} else {
setState { copy(isLoading = true) }

currentJob = viewModelScope.launch {
try {
safeLoginWizard.resetPassword(action.email, action.newPassword)
} catch (failure: Throwable) {
setState { copy(isLoading = false) }
_viewEvents.post(OnboardingViewEvents.Failure(failure))
return@launch
}

setState {
copy(
isLoading = false,
resetPasswordEmail = action.email
)
}

_viewEvents.post(OnboardingViewEvents.OnResetPasswordSendThreePidDone)
}
setState { copy(isLoading = true) }
currentJob = viewModelScope.launch {
runCatching { safeLoginWizard.resetPassword(action.email) }.fold(
onSuccess = {
setState {
copy(
isLoading = false,
resetState = ResetState(email = action.email, newPassword = action.newPassword)
)
}
_viewEvents.post(OnboardingViewEvents.OnResetPasswordSendThreePidDone)
},
onFailure = {
setState { copy(isLoading = false) }
_viewEvents.post(OnboardingViewEvents.Failure(it))
}
)
}
}

private fun handleResetPasswordMailConfirmed() {
val safeLoginWizard = loginWizard

if (safeLoginWizard == null) {
setState { copy(isLoading = false) }
_viewEvents.post(OnboardingViewEvents.Failure(Throwable("Bad configuration")))
} else {
setState { copy(isLoading = false) }

currentJob = viewModelScope.launch {
try {
safeLoginWizard.resetPasswordMailConfirmed()
} catch (failure: Throwable) {
setState { copy(isLoading = true) }
currentJob = viewModelScope.launch {
val resetState = awaitState().resetState
when (val newPassword = resetState.newPassword) {
null -> {
setState { copy(isLoading = false) }
_viewEvents.post(OnboardingViewEvents.Failure(failure))
return@launch
_viewEvents.post(OnboardingViewEvents.Failure(IllegalStateException("Developer error - No new password has been set")))
}
setState {
copy(
isLoading = false,
resetPasswordEmail = null
else -> {
runCatching { loginWizard.resetPasswordMailConfirmed(newPassword) }.fold(
onSuccess = {
setState {
copy(
isLoading = false,
resetState = ResetState()
)
}
_viewEvents.post(OnboardingViewEvents.OnResetPasswordMailConfirmationSuccess)
},
onFailure = {
setState { copy(isLoading = false) }
_viewEvents.post(OnboardingViewEvents.Failure(it))
}
)
}

_viewEvents.post(OnboardingViewEvents.OnResetPasswordMailConfirmationSuccess)
}
}
}
Expand All @@ -519,25 +506,19 @@ class OnboardingViewModel @AssistedInject constructor(

private fun handleLogin(action: AuthenticateAction.Login) {
val safeLoginWizard = loginWizard

if (safeLoginWizard == null) {
setState { copy(isLoading = false) }
_viewEvents.post(OnboardingViewEvents.Failure(Throwable("Bad configuration")))
} else {
setState { copy(isLoading = true) }
currentJob = viewModelScope.launch {
try {
val result = safeLoginWizard.login(
action.username,
action.password,
action.initialDeviceName
)
reAuthHelper.data = action.password
onSessionCreated(result, authenticationDescription = AuthenticationDescription.Login)
} catch (failure: Throwable) {
setState { copy(isLoading = false) }
_viewEvents.post(OnboardingViewEvents.Failure(failure))
}
setState { copy(isLoading = true) }
currentJob = viewModelScope.launch {
try {
val result = safeLoginWizard.login(
action.username,
action.password,
action.initialDeviceName
)
reAuthHelper.data = action.password
onSessionCreated(result, authenticationDescription = AuthenticationDescription.Login)
} catch (failure: Throwable) {
setState { copy(isLoading = false) }
_viewEvents.post(OnboardingViewEvents.Failure(failure))
}
}
}
Expand Down
Loading