Fix reviews not saving in new applications review. Skip assign page if no at risk passwords are to be assigned. Fix bug in password change widget

Author: Banrion

apps/web/src/locales/en/messages.json

@@ -12152,5 +12152,11 @@
         "example": "5"
       }
     }
+  },
+  "confirmNoSelectedCriticalApplicationsTitle" : {
+    "message": "No critical applications are selected"
+  },
+  "confirmNoSelectedCriticalApplicationsDesc" : {
+    "message": "Are you sure you want to continue?"
   }
 }

bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/activity-cards/password-change-metric.component.html

@@ -43,7 +43,7 @@
       </div>
 
       <div class="tw-items-baseline tw-gap-2">
-        <span bitTypography="body2">{{ "newPasswordsAtRisk" | i18n: atRiskPasswordCount() }}</span>
+        <span bitTypography="body2">{{ "newPasswordsAtRisk" | i18n: unassignedCipherIds() }}</span>
       </div>
 
       <div class="tw-mt-4">

bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/activity-cards/password-change-metric.component.ts

@@ -66,37 +66,42 @@ export class PasswordChangeMetricComponent implements OnInit {
   readonly completedTasksCount = computed(
     () => this._tasks().filter((task) => task.status === SecurityTaskStatus.Completed).length,
   );
-  readonly uncompletedTasksCount = computed(
-    () => this._tasks().filter((task) => task.status == SecurityTaskStatus.Pending).length,
-  );
   readonly completedTasksPercent = computed(() => {
     const total = this.tasksCount();
     // Account for case where there are no tasks to avoid NaN
     return total > 0 ? Math.round((this.completedTasksCount() / total) * 100) : 0;
   });
 
-  readonly atRiskPasswordCount = computed<number>(() => {
+  readonly unassignedCipherIds = computed<number>(() => {
     const atRiskIds = this._atRiskCipherIds();
     const tasks = this._tasks();
 
     if (tasks.length === 0) {
       return atRiskIds.length;
     }
 
-    const assignedIdSet = new Set(tasks.map((task) => task.cipherId));
+    const inProgressTasks = tasks.filter((task) => task.status === SecurityTaskStatus.Pending);
+    const assignedIdSet = new Set(inProgressTasks.map((task) => task.cipherId));
     const unassignedIds = atRiskIds.filter((id) => !assignedIdSet.has(id));
 
     return unassignedIds.length;
   });
 
+  readonly atRiskPasswordCount = computed<number>(() => {
+    const atRiskIds = this._atRiskCipherIds();
+    const atRiskIdsSet = new Set(atRiskIds);
+
+    return atRiskIdsSet.size;
+  });
+
   readonly currentView = computed<PasswordChangeView>(() => {
     if (!this._hasCriticalApplications()) {
       return PasswordChangeView.EMPTY;
     }
     if (this.tasksCount() === 0) {
       return PasswordChangeView.NO_TASKS_ASSIGNED;
     }
-    if (this.atRiskPasswordCount() > 0) {
+    if (this.unassignedCipherIds() > 0) {
       return PasswordChangeView.NEW_TASKS_AVAILABLE;
     }
     return PasswordChangeView.PROGRESS;

bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/application-review-dialog/new-applications-dialog.component.html

@@ -38,8 +38,8 @@
 
     @if (currentView() === DialogView.AssignTasks) {
       <dirt-assign-tasks-view
-        [criticalApplicationsCount]="atRiskCriticalApplicationsCount()"
-        [totalApplicationsCount]="totalCriticalApplicationsCount()"
+        [criticalApplicationsCount]="newAtRiskCriticalApplications().length"
+        [totalApplicationsCount]="newCriticalApplications().length"
         [atRiskCriticalMembersCount]="atRiskCriticalMembersCount()"
       >
       </dirt-assign-tasks-view>

bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/application-review-dialog/new-applications-dialog.component.ts

@@ -2,13 +2,16 @@ import { CommonModule } from "@angular/common";
 import {
   ChangeDetectionStrategy,
   Component,
+  computed,
   DestroyRef,
   Inject,
   inject,
+  Injector,
+  Signal,
   signal,
 } from "@angular/core";
-import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
-import { from, switchMap, take } from "rxjs";
+import { takeUntilDestroyed, toSignal } from "@angular/core/rxjs-interop";
+import { from, of, switchMap, take } from "rxjs";
 
 import {
   ApplicationHealthReportDetail,
@@ -17,7 +20,8 @@ import {
 import { getUniqueMembers } from "@bitwarden/bit-common/dirt/reports/risk-insights/helpers";
 import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
 import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
-import { OrganizationId } from "@bitwarden/common/types/guid";
+import { CipherId, OrganizationId } from "@bitwarden/common/types/guid";
+import { SecurityTask, SecurityTaskStatus } from "@bitwarden/common/vault/tasks";
 import {
   ButtonModule,
   DIALOG_DATA,
@@ -70,9 +74,9 @@ export type NewApplicationsDialogResultType =
   (typeof NewApplicationsDialogResultType)[keyof typeof NewApplicationsDialogResultType];
 
 @Component({
+  changeDetection: ChangeDetectionStrategy.OnPush,
   selector: "dirt-new-applications-dialog",
   templateUrl: "./new-applications-dialog.component.html",
-  changeDetection: ChangeDetectionStrategy.OnPush,
   imports: [
     CommonModule,
     ButtonModule,
@@ -95,24 +99,63 @@ export class NewApplicationsDialogComponent {
   // Applications selected to save as critical applications
   protected readonly selectedApplications = signal<Set<string>>(new Set());
 
-  // Assign tasks variables
-  readonly atRiskCriticalApplicationsCount = signal<number>(0);
-  readonly totalCriticalApplicationsCount = signal<number>(0);
-  readonly atRiskCriticalMembersCount = signal<number>(0);
+  // Used to determine if there are unassigned at-risk cipher IDs
+  private readonly _tasks!: Signal<SecurityTask[]>;
+
+  // Computed properties for selected applications
+  protected readonly newCriticalApplications = computed(() => {
+    return this.dialogParams.newApplications.filter((newApp) =>
+      this.selectedApplications().has(newApp.applicationName),
+    );
+  });
+
+  // New at risk critical applications
+  protected readonly newAtRiskCriticalApplications = computed(() => {
+    return this.newCriticalApplications().filter((app) => app.atRiskPasswordCount > 0);
+  });
+
+  // Count of unique members with at-risk passwords in newly marked critical applications
+  protected readonly atRiskCriticalMembersCount = computed(() => {
+    return getUniqueMembers(this.newCriticalApplications().flatMap((x) => x.atRiskMemberDetails))
+      .length;
+  });
+
+  protected readonly newUnassignedAtRiskCipherIds = computed<CipherId[]>(() => {
+    const newAtRiskCipherIds = this.newCriticalApplications().flatMap((app) => app.atRiskCipherIds);
+    const tasks = this._tasks();
+
+    if (tasks.length === 0) {
+      return newAtRiskCipherIds;
+    }
+
+    const inProgressTasks = tasks.filter((task) => task.status === SecurityTaskStatus.Pending);
+    const assignedIdSet = new Set(inProgressTasks.map((task) => task.cipherId));
+    const unassignedIds = newAtRiskCipherIds.filter((id) => !assignedIdSet.has(id));
+    return unassignedIds;
+  });
+
   readonly saving = signal<boolean>(false);
 
   // Loading states
   protected readonly markingAsCritical = signal<boolean>(false);
 
   constructor(
     @Inject(DIALOG_DATA) protected dialogParams: NewApplicationsDialogData,
-    private dialogRef: DialogRef<NewApplicationsDialogResultType>,
     private dataService: RiskInsightsDataService,
-    private toastService: ToastService,
+    private dialogRef: DialogRef<NewApplicationsDialogResultType>,
+    private dialogService: DialogService,
     private i18nService: I18nService,
-    private accessIntelligenceSecurityTasksService: AccessIntelligenceSecurityTasksService,
+    private injector: Injector,
     private logService: LogService,
-  ) {}
+    private securityTasksService: AccessIntelligenceSecurityTasksService,
+    private toastService: ToastService,
+  ) {
+    // Setup the _tasks signal by manually passing in the injector
+    this._tasks = toSignal(this.securityTasksService.tasks$, {
+      initialValue: [],
+      injector: this.injector,
+    });
+  }
 
   /**
    * Opens the new applications dialog
@@ -170,54 +213,63 @@ export class NewApplicationsDialogComponent {
     });
   }
 
-  handleMarkAsCritical() {
-    if (this.markingAsCritical() || this.saving()) {
-      return; // Prevent action if already processing
-    }
-    this.markingAsCritical.set(true);
-
-    const onlyNewCriticalApplications = this.dialogParams.newApplications.filter((newApp) =>
-      this.selectedApplications().has(newApp.applicationName),
-    );
-
-    // Count only critical applications that have at-risk passwords
-    const atRiskCriticalApplicationsCount = onlyNewCriticalApplications.filter(
-      (app) => app.atRiskPasswordCount > 0,
-    ).length;
-    this.atRiskCriticalApplicationsCount.set(atRiskCriticalApplicationsCount);
-
-    // Total number of selected critical applications
-    this.totalCriticalApplicationsCount.set(onlyNewCriticalApplications.length);
+  // Checks if there are selected applications and proceeds to assign tasks
+  async handleMarkAsCritical() {
+    if (this.selectedApplications().size === 0) {
+      const confirmed = await this.dialogService.openSimpleDialog({
+        title: { key: "confirmNoSelectedCriticalApplicationsTitle" },
+        content: { key: "confirmNoSelectedCriticalApplicationsDesc" },
+        type: "warning",
+      });
 
-    const atRiskCriticalMembersCount = getUniqueMembers(
-      onlyNewCriticalApplications.flatMap((x) => x.atRiskMemberDetails),
-    ).length;
-    this.atRiskCriticalMembersCount.set(atRiskCriticalMembersCount);
+      if (!confirmed) {
+        return;
+      }
+    }
 
-    this.currentView.set(DialogView.AssignTasks);
-    this.markingAsCritical.set(false);
+    // Skip the assign tasks view if there are no new unassigned at-risk cipher IDs
+    if (this.newUnassignedAtRiskCipherIds().length === 0) {
+      this.handleAssignTasks();
+    } else {
+      this.currentView.set(DialogView.AssignTasks);
+    }
   }
 
-  /**
-   * Handles the assign tasks button click
-   */
+  // Saves the application review and assigns tasks for unassigned at-risk ciphers
   protected handleAssignTasks() {
     if (this.saving()) {
       return; // Prevent double-click
     }
     this.saving.set(true);
 
+    const reviewedDate = new Date();
+    const updatedApplications = this.dialogParams.newApplications.map((app) => {
+      const isCritical = this.selectedApplications().has(app.applicationName);
+      return {
+        applicationName: app.applicationName,
+        isCritical,
+        reviewedDate,
+      };
+    });
+
     // Save the application review dates and critical markings
-    this.dataService.criticalApplicationAtRiskCipherIds$
+    this.dataService
+      .saveApplicationReviewStatus(updatedApplications)
       .pipe(
-        takeUntilDestroyed(this.destroyRef), // Satisfy eslint rule
-        take(1), // Handle unsubscribe for one off operation
-        switchMap((criticalApplicationAtRiskCipherIds) => {
-          return from(
-            this.accessIntelligenceSecurityTasksService.requestPasswordChangeForCriticalApplications(
-              this.dialogParams.organizationId,
-              criticalApplicationAtRiskCipherIds,
-            ),
+        takeUntilDestroyed(this.destroyRef), // Satisfy eslint rule,
+        take(1),
+        switchMap(() => {
+          // Assign password change tasks for unassigned at-risk ciphers for critical applications
+          return of(this.newUnassignedAtRiskCipherIds()).pipe(
+            takeUntilDestroyed(this.destroyRef), // Satisfy eslint rule
+            switchMap((criticalApplicationAtRiskCipherIds) => {
+              return from(
+                this.securityTasksService.requestPasswordChangeForCriticalApplications(
+                  this.dialogParams.organizationId,
+                  criticalApplicationAtRiskCipherIds,
+                ),
+              );
+            }),
           );
         }),
       )