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.

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

Improve password change scenario in lockscreen #1018

Merged
merged 1 commit into from
Nov 28, 2023

Conversation

acezard
Copy link
Contributor

@acezard acezard commented Nov 16, 2023

This pull request includes changes to the validatePassword function in the lockscreen feature. The function now handles several scenarios, including successful validation with cached password, successful validation with user input password, invalid cached password and invalid user input, unexpected server errors, errors in the success handler, and network or communication errors. These changes improve the overall user experience and ensure that the lockscreen feature is more reliable and secure.

@acezard acezard force-pushed the fix--Improve-new-password-scenario-in-lockscreen branch 2 times, most recently from 798bc73 to 676fa01 Compare November 17, 2023 08:36
@acezard acezard marked this pull request as ready for review November 17, 2023 08:36
@acezard acezard requested review from Crash-- and Ldoppea and removed request for Ldoppea and zatteo November 17, 2023 08:36
@acezard acezard force-pushed the fix--Improve-new-password-scenario-in-lockscreen branch from 676fa01 to 1924a64 Compare November 17, 2023 08:43
@acezard acezard changed the title Improve new password scenario in lockscreen Improve password change scenario in lockscreen Nov 17, 2023
Copy link
Member

@Ldoppea Ldoppea left a comment

Choose a reason for hiding this comment

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

Sorry I didn't realize that during dailies, but although this should work, after reading the code I'm not sure what's the advantage of doing this instead of directly doing the checkInputPassword part?

My comprehension is the following:

Seems like all "good" scenario will call /settings/passphrase/check with the password input

If entered password is good, then /settings/passphrase/check is called with the cached password, but as it must be the same as the entered password, this is equivalent to calling it with the entered password. Then onSuccess() is called

If the entered password is good but cache is obsolete, then we end up calling /settings/passphrase/check with entered password. Then onSuccess() is called

If the entered password is bad, then badUnlockPassword is returned whatever if the cached password is bad, or if the input password is bad. So there seems to be no distinction for the user. The only one is that if bad password is entered then 2 http requests are made instead of one.

So would it be simpler if you directly call /settings/passphrase/check only 1 time with input password? And then:

  • if 200 and hashedInputPassword.passwordHash === storedHash -> onSuccess()
  • if 200 and hashedInputPassword.passwordHash !== storedHash -> updatCache + onSuccess()
  • if 403 -> onFailure(errors.badUnlockPassword)
  • else -> onFailure(errors.serverError) or onFailure(errors.unknown_error)

I think this would output the same result so unit tests would still be green without any change.

However this PR should work so feel free to merge it

Comment on lines 65 to 66
await removeVaultInformation('passwordHash')
await saveVaultInformation('passwordHash', hashedInputPassword.passwordHash)
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be redundant with the updateCachedPassword() content that is called just after this method on line 84

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@acezard acezard force-pushed the fix--Improve-new-password-scenario-in-lockscreen branch from 1924a64 to 1c2b0df Compare November 28, 2023 09:44
@acezard acezard merged commit 0064bcd into master Nov 28, 2023
1 check passed
@acezard acezard deleted the fix--Improve-new-password-scenario-in-lockscreen branch November 28, 2023 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants