Skip to content
This repository was archived by the owner on May 14, 2025. It is now read-only.

[AC-1070] Enforce master password policy on login/unlock#2410

Merged
shane-melton merged 35 commits intomasterfrom
AC-1070-expand-master-pass-reqs
Apr 17, 2023
Merged

[AC-1070] Enforce master password policy on login/unlock#2410
shane-melton merged 35 commits intomasterfrom
AC-1070-expand-master-pass-reqs

Conversation

@shane-melton
Copy link
Mannequin

@shane-melton shane-melton mannequin commented Mar 9, 2023

Type of change

  • Bug fix
  • New feature development
  • Tech debt (refactoring, code cleanup, dependency upgrades, etc)
  • Build/deploy pipeline (DevOps)
  • Other

Objective

Implement new functionality to evaluate a master password on login if the new option to enforce on login is enabled. If enabled, whenever a user logs in, their master password will be evaluated against the organization's password requirements. If it fails the requirements, the user will be redirected to the update password page and forced to update to a new master password that meets the requirements. The same requirements are also checked during an a vault unlock with master password to ensure existing SSO members are also compliant with the organization's policy.

Related PRs

Required server PR can be found here: bitwarden/server#2714
Clients repo PR with similar functionality for other clients: bitwarden/clients#4795

Code changes

Forcing Password Reset

  • src/App/App.xaml.cs: Listen for new forceUpdatePassword command to navigate to the update password page. Similar to the convertAccountToKeyConnector command.

  • src/App/Pages/TabsPage.cs: On page load, check if there is a forceResetPasswordReason saved to the account's state. If so, fire the forceUpdatePassword command to navigate to the update password page. This is to prevent users from skipping the update password page by restarting the app.

Model Changes

  • src/Core/Models/Domain/MasterPasswordPolicyOptions.cs: Add the new EnforceOnLogin flag that is now available on the master password policy data.

  • src/Core/Models/Response/IdentityTokenResponse.cs: Add MasterPasswordPolicy field that is now returned in the response.

  • src/Core/Models/Response/IdentityTwoFactorResponse.cs: Same as above.

  • src/Core/Models/Response/VerifyMasterPasswordResponse.cs: New response model that is returned when a user verifies their master password with the server that includes the master password policy for that user (during a vault unlock).

Service Changes

  • src/Core/Abstractions/IStateService.cs: Add methods to get/set an accounts forcePasswordResetReason. The auth service will set the value after successful authentication and the password is "weak". It can then be fetched in the TabsPage.cs to check if the user should be forced to update their password.

  • src/Core/Services/AuthService.cs: Bulk of the logic change. During password login, the master password is evaluated using the master password policy returned by the Identity response. If the password does not meet requirements, save the reason to the account state to force a password update. Additional logic is in place to support 2FA login flow.

  • src/Core/Abstractions/IApiService.cs: Add method to update the user's password

Page Changes

  • src/App/Pages/Accounts/UpdateTempPasswordPage.xaml: Update the warning text to depend on the reason for the password reset. Conditionally show a new "Current master password" field if updating a weak master password (not required for admin password resets)

  • src/App/Pages/Accounts/UpdateTempPasswordPageViewModel.cs: Add logic to support updating both admin reset passwords and weak master passwords depending on the reason the user is on the page.

Screenshots

image

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

…assword is found during login

- Additionally, save the AdminForcePasswordReset reason if the identity result indicates an admin password reset is in effect.
Allow the UpdateTempPasswordPage to override the InitAsync method to check for a reset password reason in the state service
- Load the force password reset reason from the state service
- Make warning text dynamic based on force password reason
- Conditionally show the Current master password field if updating a weak master password
- Check the Reason to use the appropriate request/endpoint when submitting.
- Verify the users current password locally using the user verification service.
Keep track of the reset password reason after a password login requires 2FA. During 2FA submission, check if there is a saved reason, and if so, force the user to update their password.
@shane-melton shane-melton mannequin marked this pull request as ready for review March 9, 2023 01:19
Copy link
Member

@fedemkr fedemkr left a comment

Choose a reason for hiding this comment

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

Great work on this Shane, just a few comments. Also does this also work on extensions, like Autofill/Share?
Oh and for the warnings at the top, what do you think about talking this with product/design to change it a bit cause on small mobile devices the user might not even see the first field depending on the length of the warning (maybe in some language is even longer).
So maybe a general warning with an info button or something expandable could fix this potential UX issue

@shane-melton
Copy link
Mannequin Author

shane-melton mannequin commented Mar 13, 2023

@fedemkr Thanks for the review! I believe I have addressed all of your individual comments.

Also does this also work on extensions, like Autofill/Share?

I believe this should only apply during log in to main mobile application and not necessarily the extensions on their own, but I'll double check with product.

I'll also reach out to them about condensing/collapsing the warning messages in some way so they don't hide fields on smaller devices.

@shane-melton shane-melton mannequin added the needs-qa label Mar 13, 2023
@shane-melton shane-melton mannequin requested a review from fedemkr March 13, 2023 21:54
@shane-melton shane-melton mannequin dismissed a stale review via d05e958 April 13, 2023 18:26
@shane-melton shane-melton mannequin requested a review from fedemkr April 13, 2023 18:28
@shane-melton shane-melton mannequin removed the needs-qa label Apr 16, 2023
@shane-melton shane-melton mannequin merged commit b108b4e into master Apr 17, 2023
@shane-melton shane-melton mannequin deleted the AC-1070-expand-master-pass-reqs branch April 17, 2023 14:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants