clients icon indicating copy to clipboard operation
clients copied to clipboard

[PM-18721][PM-21272] Integrate InputPasswordComponent in AccountRecoveryDialogComponent

Open rr-bw opened this issue 8 months ago â€ĸ 7 comments

đŸŽŸī¸ Tracking

PM-18721 PM-21272

This PR stacks on top of these two PRs:

  • https://github.com/bitwarden/clients/pull/14636
  • https://github.com/bitwarden/clients/pull/14226

📔 Objective

This PR integrates the InputPasswordComponent within the AccountRecoveryDialogComponent (formerly called ResetPasswordComponent).

The result is that:

  • The InputPasswordComponent will now handle form field UI display and validation that is common to all of our set/change password flows
  • The AccountRecoveryDialogComponent will now just be responsible for displaying the dialog and calling resetPasswordService.resetMasterPassword() to perform the password reset operation.

The showing of the new dialog component is behind the PM16117_ChangeExistingPasswordRefactor feature flag.

📸 Screenshots

PM16117_ChangeExistingPasswordRefactor flag ON ✅

https://github.com/user-attachments/assets/45a82faf-668a-45d7-a6ea-131f13df03f3

PM16117_ChangeExistingPasswordRefactor flag OFF ❌

https://github.com/user-attachments/assets/4fe07143-0608-4fe9-8cb5-cf8ec88b1cc8

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

đŸĻŽ Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or â„šī¸ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or âš ī¸ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or â™ģī¸ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

rr-bw avatar May 07 '25 06:05 rr-bw

Codecov Report

Attention: Patch coverage is 0% with 54 lines in your changes missing coverage. Please review.

Project coverage is 36.84%. Comparing base (9ba3cc0) to head (185c300). Report is 4 commits behind head on main.

:white_check_mark: All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ount-recovery/account-recovery-dialog.component.ts 0.00% 36 Missing :warning:
...console/organizations/members/members.component.ts 0.00% 12 Missing :warning:
...angular/input-password/input-password.component.ts 0.00% 4 Missing :warning:
...app/admin-console/common/base-members.component.ts 0.00% 1 Missing :warning:
...tions/members/components/account-recovery/index.ts 0.00% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14662      +/-   ##
==========================================
- Coverage   36.85%   36.84%   -0.02%     
==========================================
  Files        3228     3230       +2     
  Lines       93388    93438      +50     
  Branches    14056    14062       +6     
==========================================
+ Hits        34419    34426       +7     
- Misses      57543    57586      +43     
  Partials     1426     1426              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar May 07 '25 07:05 codecov[bot]

Logo Checkmarx One – Scan Summary & Details – e5c5651f-ba1e-4854-9a5b-9a9f277fd526

Great job, no security vulnerabilities found in this Pull Request

github-actions[bot] avatar May 07 '25 07:05 github-actions[bot]

Mere mortal user here again. - I don't know if it's intentional, but the password generator component in the "Recover account" dialog (shown in the first 20 seconds of the video) doesn't contain a "Check for Data breaches"-button, like it is shown and available in the browser extension after a password was generated:

2025-05-07--09-59-38-vivaldi_EhbU414Rs6

pamperer562580892423 avatar May 07 '25 08:05 pamperer562580892423

@eliykat

What's the plan for managing your branches - are they being merged into main one-at-a-time in order, or are they all getting merged down into a single feature branch? I ask because usually feature flags remove the need for large feature branches.

  • https://github.com/bitwarden/clients/pull/14662 (This PR)
    • Will be merged to main directly, after PM-14636 has been merged, because it doesn't have meaningful changes outside of the feature flag
  • https://github.com/bitwarden/clients/pull/14636
    • Will be tested on a feature branch because it has meaningful changes that leak outside the feature flag
  • https://github.com/bitwarden/clients/pull/14226
    • Has been merged to main and fully tested

rr-bw avatar May 30 '25 23:05 rr-bw

Mere mortal user here again. - I don't know if it's intentional, but the password generator component in the "Recover account" dialog (shown in the first 20 seconds of the video) doesn't contain a "Check for Data breaches"-button, like it is shown and available in the browser extension after a password was generated:

2025-05-07--09-59-38-vivaldi_EhbU414Rs6

Thanks again! @pamperer562580892423

I've raised the question to our Product team.

rr-bw avatar Jun 04 '25 21:06 rr-bw

Mere mortal user here again. - I don't know if it's intentional, but the password generator component in the "Recover account" dialog (shown in the first 20 seconds of the video) doesn't contain a "Check for Data breaches"-button, like it is shown and available in the browser extension after a password was generated: 2025-05-07--09-59-38-vivaldi_EhbU414Rs6

Thanks again! @pamperer562580892423

I've raised the question to our Product team.

Thanks for that "update"! - And I think you have also seen by now, that the "Data breach checker" is also missing in the "Update Master Password" dialog (second part of the video).

pamperer562580892423 avatar Jun 05 '25 01:06 pamperer562580892423