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

rr-bw · 2025-05-07T06:57:05Z

🎟️ Tracking

This PR stacks on top of these two PRs:

📔 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 ✅

account-recovery-flag-on.mov

PM16117_ChangeExistingPasswordRefactor flag OFF ❌

account-recovery-flag-off.mov

⏰ 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

codecov · 2025-05-07T07:01:11Z

Codecov Report

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

Project coverage is 36.79%. Comparing base (18b7d46) to head (d562acc).
Report is 34 commits behind head on auth/pm-18721/integrate-input-password-component-in-dialogs.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...ount-recovery/account-recovery-dialog.component.ts	0.00%	47 Missing ⚠️
...console/organizations/members/members.component.ts	0.00%	12 Missing ⚠️
...app/admin-console/common/base-members.component.ts	0.00%	1 Missing ⚠️
...tions/members/components/account-recovery/index.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@                                       Coverage Diff                                       @@
##           auth/pm-18721/integrate-input-password-component-in-dialogs   #14662      +/-   ##
===============================================================================================
+ Coverage                                                        36.26%   36.79%   +0.52%     
===============================================================================================
  Files                                                             3203     3202       -1     
  Lines                                                            93461    92906     -555     
  Branches                                                         16864    13950    -2914     
===============================================================================================
+ Hits                                                             33897    34184     +287     
- Misses                                                           57115    57316     +201     
+ Partials                                                          2449     1406    -1043

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

github-actions · 2025-05-07T07:06:27Z

Checkmarx One – Scan Summary & Details – bf9609a8-db1b-4021-9bab-cbf6a84a3036

New Issues (3)

Checkmarx found the following issues in this Pull Request

Issue	Source File / Package	Checkmarx Insight
Cxbb85e86c-2fac	Npm-esbuild-0.23.0	details Recommended version: 0.25.0 Description: esbuild is an extremely fast bundler for the web, allowing any website to send any request to the development server and read the response due to d... Attack Vector: NETWORK Attack Complexity: HIGH `ID: F%2Fgr9uMAqxJyLibeUtBDNowYER4I%2B3OAMHUxeFYuTvI%3D` Vulnerable Package
Cxbb85e86c-2fac	Npm-esbuild-0.21.5	details Recommended version: 0.25.0 Description: esbuild is an extremely fast bundler for the web, allowing any website to send any request to the development server and read the response due to d... Attack Vector: NETWORK Attack Complexity: HIGH `ID: GA1sDg9c%2B7CNB%2BshyjM1MqbpTP6R5aa%2Fjurbt6HYTzU%3D` Vulnerable Package
Cxbb85e86c-2fac	Npm-esbuild-wasm-0.23.0	details Recommended version: 0.25.0 Description: esbuild is an extremely fast bundler for the web, allowing any website to send any request to the development server and read the response due to d... Attack Vector: NETWORK Attack Complexity: HIGH `ID: Xw%2FLg2nMZISvbniEoXOWe5IvnyW%2BNqqWzgrq2DQfkZE%3D` Vulnerable Package

pamperer562580892423 · 2025-05-07T08:03:49Z

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:

...nsole/organizations/members/components/account-recovery/account-recovery-dialog.component.ts

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.

EDIT: also, please review CI failures.

eliykat · 2025-05-14T22:56:18Z

...nsole/organizations/members/components/account-recovery/account-recovery-dialog.component.ts

+  inputPasswordComponent!: InputPasswordComponent;
+
+  inputPasswordFlow = InputPasswordFlow.ChangePasswordDelegation;
+  masterPasswordPolicyOptions?: MasterPasswordPolicyOptions;


masterPasswordPolicyOptions is now unused.

eliykat · 2025-05-14T23:11:09Z

...nsole/organizations/members/components/account-recovery/account-recovery-dialog.component.ts

+  handlePrimaryButtonClick = async () => {
+    try {
+      this.submitting = true;
+      await this.inputPasswordComponent.submit();
+    } catch {
+      // Flip to false if submit() throws an error
+      this.submitting = false;
+    }
+
+    // Flip to false if submit() returns early without a PasswordInputResult
+    // emission due to form validation errors or new password doesn't meet org policy reqs.
+    if (!this.receivedPasswordInputResult) {
+      this.submitting = false;
+    }
+  };


There is a potential race condition here: inputPasswordComponent.submit() ends by emitting an event that calls handlePasswordFormSubmit, not by calling it directly. That's good design, but it means that await this.inputPasswordComponent.submit() is not going to await the execution of handlePasswordFormSubmit as well. The race condition is whether or not receivedPasswordInputResult is updated by handlePasswordFormSubmit before it's read by handlePrimaryButtonClick.

This also means that this.submitting is not going to be accurate, although it's possible that it's near enough at the moment (especially running locally with zero latency) that it's not noticeable.

This all seems to be in service of the this.submitting behaviour, but we avoid this pattern these days in favour of the Async Actions tools in the component library. So I actually think you can avoid this problem altogether by using the CL.

Your parent-child situation is slightly unusual so if the CL isn't helping, I recommend reaching out to the UI Foundations team to see how to adapt it to your situation.

I posted on the UIF team channel and it looks like Async Actions won't handle this. You can see that thread here if you'd like.

So our solution is to handle parent/child submit state reactively, like so: 1fccdde

eliykat · 2025-05-14T23:12:42Z

...ole/organizations/members/components/account-recovery/account-recovery-dialog.component.html

+      <span
+        [ngClass]="{ 'tw-invisible': !submitting }"
+        class="tw-absolute tw-inset-0 tw-flex tw-items-center tw-justify-center"
+      >
+        <i class="bwi bwi-spinner bwi-lg bwi-spin" aria-hidden="true"></i>
+      </span>
+      <span [ngClass]="{ 'tw-invisible': submitting }">
+        {{ "save" | i18n }}
+      </span>


Similarly, I think the CL should replace all of this manual handling around the loading spinner.

See my comment above: #14662 (comment)

eliykat · 2025-05-14T23:13:58Z

apps/web/src/app/admin-console/organizations/members/members.component.ts

+      if (result === AccountRecoveryDialogResultTypes.Ok) {
+        await this.load();
+      }
+    } else {


Nit: please use an early return rather than nesting with else.

53d4a14?diff=split&w=1 (whitespace hidden)

…L and TS

…swordcomponent

…e observable first

… in InputPasswordComponent HTML

rr-bw · 2025-05-30T23:11:57Z

@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.

[PM-18721][PM-21272] Integrate InputPasswordComponent in AccountRecoveryDialogComponent #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
[PM-18721][PM-21271] Integrate InputPasswordComponent in EmergencyAccessTakeoverDialogComponent #14636
- Will be tested on a feature branch because it has meaningful changes that leak outside the feature flag
[PM-18458] Create new ChangePasswordComponent #14226
- Has been merged to main and fully tested

eliykat · 2025-06-03T02:02:28Z

...nsole/organizations/members/components/account-recovery/account-recovery-dialog.component.ts

+  private parentSubmittingBehaviorSubject = new BehaviorSubject(false);
+  parentSubmitting$ = this.parentSubmittingBehaviorSubject.asObservable();
+
+  private childSubmittingBehaviorSubject = new BehaviorSubject(false);
+  childSubmitting$ = this.childSubmittingBehaviorSubject.asObservable();
+
+  submitting$ = combineLatest([this.parentSubmitting$, this.childSubmitting$]).pipe(
+    map(([parentIsSubmitting, childIsSubmitting]) => parentIsSubmitting || childIsSubmitting),
+  );


You've fixed the race condition, but I think this is still a lot of boilerplate just to handle a form submission. I assume this would have to be repeated in every parent component that wants to use this child component.

Does the child need to know about the form submit state at all? The submission process starts when handlePrimaryButtonClick is called, and it ends when handlePasswordFormSubmit is finished, or if the child emits an error. If that's the case, the parent should be able to handle all of that by itself, which is also better design because the parent owns the form.

💭 I also like the suggestion in Slack:

Directly referencing a component is highly discouraged. Could you instead invert the relationship so that the action happens in the parent?

It does feel a bit unusual that the child is handling the parent's submit action. There's quite a bit of logic in that child component, what if it were extracted to a service that the parent could call (thereby handling its own form submission), and the child is more presentational, simply emitting form values? That's a fairly large change so it doesn't need to be done in this PR (if at all), just mentioning it as feedback.

The submission process starts when handlePrimaryButtonClick is called, and it ends when handlePasswordFormSubmit is finished, or if the child emits an error. If that's the case, the parent should be able to handle all of that by itself

The submit process can also end if the child returns early, without emitting. That means handlePasswordFormSubmit would never get called in the parent, leaving is in a perpetual "submitting" state.

That is, if we merely set submitting to true in handlePrimaryButtonClick, and then set it to false in handlePasswordFormSubmit, then that false set would never get called in a case where InputPasswordComponent.submit() returns early without emitting. That's the case I was trying to solve in my previous implementation here (but had the race condition).

cc: @JaredSnider-Bitwarden let me know if you have any other thoughts on this.

There's quite a bit of logic in that child component, what if it were extracted to a service that the parent could call (thereby handling its own form submission), and the child is more presentational, simply emitting form values? That's a fairly large change so it doesn't need to be done in this PR (if at all), just mentioning it as feedback.

Yea the InputPasswordComponent has grown in scope over time. I will do a retro soon to investigate if/how we could potentially refactor things.

…ert changes that leaked outside of flag

sonarqubecloud · 2025-06-04T17:34:03Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

rr-bw · 2025-06-04T21:16:08Z

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:

Thanks again! @pamperer562580892423

I've raised the question to our Product team.

rr-bw · 2025-06-04T21:30:07Z

...embers/services/organization-user-reset-password/organization-user-reset-password.service.ts

@@ -96,7 +96,7 @@ export class OrganizationUserResetPasswordService
    newMasterPassword: string,
    email: string,
    orgUserId: string,
-    orgId: string,
+    orgId: OrganizationId,


@eliykat

When I updated the organizationId property type from string to OrganizationId in the new AccountRecoveryDialogComponent (here), it meant I have to update the param type here in the resetPassword() method as well. Since that method is also used by the old ResetPasswordComponent, it means I had to make some changes to the old code (here and here).

Since I'd rather changes not leak outside the feature flag, would it be better for me to set the param type in resetPassword() to be orgId: string | OrganizationId -- and then cleanup when the flag is removed?

pamperer562580892423 · 2025-06-05T01:27:23Z

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:

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).

rr-bw changed the base branch from main to auth/pm-18721/integrate-input-password-component-in-dialogs May 7, 2025 07:04

rr-bw changed the title ~~[PM-18721] Integrate InputPasswordComponent in AccountRecoveryDialogComponent~~ [PM-18721][PM-21272] Integrate InputPasswordComponent in AccountRecoveryDialogComponent May 7, 2025

rr-bw force-pushed the auth/pm-18721/integrate-input-password-component-in-dialogs branch from 9f03aca to 22c813d Compare May 7, 2025 22:14

eliykat requested changes May 8, 2025

View reviewed changes

rr-bw force-pushed the auth/pm-18721/integrate-input-password-component-in-dialogs branch from 2d30ad6 to 8c557cc Compare May 13, 2025 20:52

rr-bw force-pushed the auth/pm-18721/integrate-input-password-component-in-dialogs-part-2 branch 2 times, most recently from bbd56db to 64afc78 Compare May 14, 2025 09:56

rr-bw marked this pull request as ready for review May 14, 2025 11:39

rr-bw requested review from a team as code owners May 14, 2025 11:39

rr-bw requested review from jrmccannon, JaredSnider-Bitwarden and eliykat May 14, 2025 11:39

eliykat requested changes May 14, 2025

View reviewed changes

rr-bw marked this pull request as draft May 21, 2025 20:13

rr-bw force-pushed the auth/pm-18721/integrate-input-password-component-in-dialogs-part-2 branch from 63418d0 to c4985a9 Compare May 22, 2025 17:51

rr-bw force-pushed the auth/pm-18721/integrate-input-password-component-in-dialogs branch from 6fcd9f6 to e8cbcb0 Compare May 28, 2025 08:46

rr-bw force-pushed the auth/pm-18721/integrate-input-password-component-in-dialogs-part-2 branch from c4985a9 to 1629daf Compare May 28, 2025 16:00

rr-bw added 9 commits May 30, 2025 14:24

[Account Recovery][PM-18721] setup AccountRecoveryDialogComponent HTM…

9606947

…L and TS

[Account Recovery][PM-18721] add generatePassword() logic to InputPas…

0c17789

…swordcomponent

[Account Recovery][PM-18721] add copy() logic to InputPasswordComponent

5c80984

[Account Recovery][PM-18721] feature flag AccountRecoveryDialogComponent

787fddf

[Account Recovery][PM-18721] convert enum to as const

aa64267

[Account Recovery][PM-18721] pass message as @input

7ca7918

[Account Recovery][PM-18721] disable button during submit process

42e56d0

[Account Recovery][PM-18721] fix type errors

db7a401

[Account Recovery][PM-18721] update property name/type

9360bb0

rr-bw added 7 commits May 30, 2025 14:24

[Account Recovery][PM-18721] convert retreival of policy options to b…

9b13ff5

…e observable first

[Account Recovery][PM-18721] remove unused prop

5d7ceac

[Account Recovery][PM-18721] use early return

53d4a14

[Account Recovery][PM-18721] update typing on open method

09128e2

[Account Recovery][PM-18721] handle cross-component submit process

1fccdde

[Account Recovery][PM-18721] add type check

60e2bdc

[Account Recovery][PM-18721] update as const / type

801f5b1

rr-bw force-pushed the auth/pm-18721/integrate-input-password-component-in-dialogs-part-2 branch from 4806bf4 to 801f5b1 Compare May 30, 2025 21:25

rr-bw added 2 commits May 30, 2025 14:37

[Account Recovery][PM-18721] remove message Input as it's now handled…

03415ea

… in InputPasswordComponent HTML

[Account Recovery][PM-18721] update type for organizationId

45f8c19

rr-bw marked this pull request as ready for review May 30, 2025 22:52

rr-bw requested a review from eliykat May 30, 2025 22:52

eliykat requested changes Jun 3, 2025

View reviewed changes

rr-bw added 2 commits June 3, 2025 13:33

[Account Recovery][PM-18721] catch missing user details client-side

1ae47e4

[Account Recovery][PM-18721] move error handling within flag, and rev…

d562acc

…ert changes that leaked outside of flag

rr-bw commented Jun 4, 2025

View reviewed changes

rr-bw requested a review from eliykat June 4, 2025 21:31

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

Are you sure you want to change the base?

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

Conversation

rr-bw commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

Uh oh!

codecov bot commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pamperer562580892423 commented May 7, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eliykat left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rr-bw May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rr-bw commented May 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rr-bw Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jun 4, 2025

Quality Gate passed

Uh oh!

rr-bw commented Jun 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pamperer562580892423 commented Jun 5, 2025

Uh oh!

Uh oh!

rr-bw commented May 7, 2025 •

edited

Loading

codecov bot commented May 7, 2025 •

edited

Loading

github-actions bot commented May 7, 2025 •

edited

Loading

eliykat left a comment •

edited

Loading

rr-bw May 30, 2025 •

edited

Loading

rr-bw Jun 3, 2025 •

edited

Loading