diff --git a/bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/all-activity.component.ts b/bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/all-activity.component.ts index cca4b1c7a06..0a344472b61 100644 --- a/bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/all-activity.component.ts +++ b/bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/all-activity.component.ts @@ -91,8 +91,19 @@ export class AllActivityComponent implements OnInit { * Opens a dialog showing the list of new applications that can be marked as critical. */ onReviewNewApplications = async () => { + const organizationId = this.activatedRoute.snapshot.paramMap.get("organizationId"); + + if (!organizationId) { + return; + } + + // Pass organizationId via dialog data instead of having the dialog retrieve it from route. + // This ensures organizationId is immediately available when dialog opens, preventing + // timing issues where the dialog's checkForTasksToAssign() method runs before + // organizationId is populated via async route subscription. const dialogRef = NewApplicationsDialogComponent.open(this.dialogService, { newApplications: this.newApplications, + organizationId: organizationId as any, }); await firstValueFrom(dialogRef.closed); diff --git a/bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/assign-tasks-view.component.ts b/bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/assign-tasks-view.component.ts index f7b4629252d..723ebba6c09 100644 --- a/bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/assign-tasks-view.component.ts +++ b/bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/assign-tasks-view.component.ts @@ -1,9 +1,6 @@ import { CommonModule } from "@angular/common"; -import { Component, DestroyRef, OnInit, inject, input, output } from "@angular/core"; -import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; -import { ActivatedRoute } from "@angular/router"; +import { Component, OnInit, inject, input, output } from "@angular/core"; import { firstValueFrom } from "rxjs"; -import { map, filter } from "rxjs/operators"; import { AllActivitiesService, @@ -15,11 +12,18 @@ import { OrganizationId } from "@bitwarden/common/types/guid"; import { ButtonModule, ToastService, TypographyModule } from "@bitwarden/components"; import { I18nPipe } from "@bitwarden/ui-common"; +import { DefaultAdminTaskService } from "../../../vault/services/default-admin-task.service"; import { AccessIntelligenceSecurityTasksService } from "../shared/security-tasks.service"; /** * Embedded component for displaying task assignment UI. * Not a dialog - intended to be embedded within a parent dialog. + * + * Important: This component provides its own instances of AccessIntelligenceSecurityTasksService + * and DefaultAdminTaskService. These services are scoped to this component to ensure proper + * dependency injection when the component is dynamically rendered within the dialog. + * Without these providers, Angular would throw NullInjectorError when trying to inject + * DefaultAdminTaskService, which is required by AccessIntelligenceSecurityTasksService. */ // FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush // eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection @@ -27,6 +31,7 @@ import { AccessIntelligenceSecurityTasksService } from "../shared/security-tasks selector: "dirt-assign-tasks-view", templateUrl: "./assign-tasks-view.component.html", imports: [CommonModule, ButtonModule, TypographyModule, I18nPipe], + providers: [AccessIntelligenceSecurityTasksService, DefaultAdminTaskService], }) export class AssignTasksViewComponent implements OnInit { /** @@ -34,6 +39,12 @@ export class AssignTasksViewComponent implements OnInit { */ readonly selectedApplicationsCount = input.required(); + /** + * Organization ID - passed from parent instead of reading from route + * because this component is embedded in a dialog and doesn't have direct route access + */ + readonly organizationId = input.required(); + /** * Emitted when tasks have been successfully assigned */ @@ -47,40 +58,26 @@ export class AssignTasksViewComponent implements OnInit { protected totalTasksToAssign = 0; protected isAssigning = false; - private destroyRef = inject(DestroyRef); private allActivitiesService = inject(AllActivitiesService); private securityTasksApiService = inject(SecurityTasksApiService); private accessIntelligenceSecurityTasksService = inject(AccessIntelligenceSecurityTasksService); private toastService = inject(ToastService); private i18nService = inject(I18nService); private logService = inject(LogService); - private activatedRoute = inject(ActivatedRoute); - - private organizationId: OrganizationId = "" as OrganizationId; async ngOnInit(): Promise { - // Get organization ID from route params - this.activatedRoute.paramMap - .pipe( - map((params) => params.get("organizationId")), - filter((orgId): orgId is string => !!orgId), - takeUntilDestroyed(this.destroyRef), - ) - .subscribe((orgId) => { - this.organizationId = orgId as OrganizationId; - }); - - // Calculate tasks to assign + // Calculate tasks to assign using organizationId passed from parent await this.calculateTasksToAssign(); } /** - * Calculates the number of tasks that will be assigned + * Calculates the number of tasks that will be assigned. + * Uses the same logic as password-change-metric.component.ts */ private async calculateTasksToAssign(): Promise { try { const taskMetrics = await firstValueFrom( - this.securityTasksApiService.getTaskMetrics(this.organizationId), + this.securityTasksApiService.getTaskMetrics(this.organizationId()), ); const atRiskPasswordsCount = await firstValueFrom( this.allActivitiesService.atRiskPasswordsCount$, @@ -114,8 +111,9 @@ export class AssignTasksViewComponent implements OnInit { const criticalApps = allApplicationsDetails.filter((app) => app.isMarkedAsCritical); // Assign tasks using the security tasks service + // Uses the same logic as password-change-metric.component.ts await this.accessIntelligenceSecurityTasksService.assignTasks( - this.organizationId, + this.organizationId(), criticalApps, ); diff --git a/bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/new-applications-dialog.component.html b/bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/new-applications-dialog.component.html index 65fd2d07131..14cb2558834 100644 --- a/bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/new-applications-dialog.component.html +++ b/bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/new-applications-dialog.component.html @@ -62,6 +62,7 @@ @if (currentView === DialogView.AssignTasks) { diff --git a/bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/new-applications-dialog.component.ts b/bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/new-applications-dialog.component.ts index b1fe4062e69..1e13015e24f 100644 --- a/bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/new-applications-dialog.component.ts +++ b/bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/new-applications-dialog.component.ts @@ -1,9 +1,6 @@ import { CommonModule } from "@angular/common"; -import { Component, DestroyRef, inject, OnInit } from "@angular/core"; -import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; -import { ActivatedRoute } from "@angular/router"; +import { Component, inject } from "@angular/core"; import { firstValueFrom } from "rxjs"; -import { filter, map } from "rxjs/operators"; import { AllActivitiesService, @@ -27,6 +24,13 @@ import { AssignTasksViewComponent } from "./assign-tasks-view.component"; export interface NewApplicationsDialogData { newApplications: string[]; + /** + * Organization ID is passed via dialog data instead of being retrieved from route params. + * This ensures organizationId is available immediately when the dialog opens, + * preventing async timing issues where user clicks "Mark as critical" before + * the route subscription has fired. + */ + organizationId: OrganizationId; } /** @@ -53,7 +57,7 @@ export type DialogView = (typeof DialogView)[keyof typeof DialogView]; AssignTasksViewComponent, ], }) -export class NewApplicationsDialogComponent implements OnInit { +export class NewApplicationsDialogComponent { protected newApplications: string[] = []; protected selectedApplications: Set = new Set(); @@ -65,7 +69,6 @@ export class NewApplicationsDialogComponent implements OnInit { // Loading states protected isCalculatingTasks = false; - private destroyRef = inject(DestroyRef); private dialogRef = inject(DialogRef); private dataService = inject(RiskInsightsDataService); private toastService = inject(ToastService); @@ -73,27 +76,19 @@ export class NewApplicationsDialogComponent implements OnInit { private logService = inject(LogService); private allActivitiesService = inject(AllActivitiesService); private securityTasksApiService = inject(SecurityTasksApiService); - private activatedRoute = inject(ActivatedRoute); - private organizationId: OrganizationId = "" as OrganizationId; - - async ngOnInit(): Promise { - // Get organization ID from route params - this.activatedRoute.paramMap - .pipe( - map((params) => params.get("organizationId")), - filter((orgId): orgId is string => !!orgId), - takeUntilDestroyed(this.destroyRef), - ) - .subscribe((orgId) => { - this.organizationId = orgId as OrganizationId; - }); - } + /** + * Organization ID set synchronously by static open() method from dialog data. + * Must be available immediately (not async) because checkForTasksToAssign() + * needs it when user clicks "Mark as critical" button. + * Previous implementation using route subscription had timing issues. + */ + organizationId: OrganizationId = "" as OrganizationId; /** * Opens the new applications dialog * @param dialogService The dialog service instance - * @param data Dialog data containing the list of new applications + * @param data Dialog data containing the list of new applications and organizationId * @returns Dialog reference */ static open(dialogService: DialogService, data: NewApplicationsDialogData) { @@ -105,9 +100,13 @@ export class NewApplicationsDialogComponent implements OnInit { ); // Set the component's data after opening + // Important: organizationId is set synchronously here, not via async route subscription. + // This prevents race conditions where user clicks "Mark as critical" before + // organizationId is populated, which would cause checkForTasksToAssign() to fail. const instance = ref.componentInstance as NewApplicationsDialogComponent; if (instance) { instance.newApplications = data.newApplications; + instance.organizationId = data.organizationId; } return ref; @@ -189,16 +188,34 @@ export class NewApplicationsDialogComponent implements OnInit { */ private async checkForTasksToAssign(): Promise { try { + this.logService.info( + `[NewApplicationsDialog] checkForTasksToAssign - organizationId: ${this.organizationId}`, + ); + + if (!this.organizationId) { + this.logService.warning("[NewApplicationsDialog] organizationId is not set yet"); + return false; + } + const taskMetrics = await firstValueFrom( this.securityTasksApiService.getTaskMetrics(this.organizationId), ); + this.logService.info( + `[NewApplicationsDialog] taskMetrics: totalTasks=${taskMetrics.totalTasks}, completedTasks=${taskMetrics.completedTasks}`, + ); + const atRiskPasswordsCount = await firstValueFrom( this.allActivitiesService.atRiskPasswordsCount$, ); + this.logService.info(`[NewApplicationsDialog] atRiskPasswordsCount: ${atRiskPasswordsCount}`); const canAssignTasks = atRiskPasswordsCount > taskMetrics.totalTasks; const newTasksCount = canAssignTasks ? atRiskPasswordsCount - taskMetrics.totalTasks : 0; + this.logService.info( + `[NewApplicationsDialog] canAssignTasks: ${canAssignTasks}, newTasksCount: ${newTasksCount}, returning: ${canAssignTasks && newTasksCount > 0}`, + ); + return canAssignTasks && newTasksCount > 0; } catch (error) { this.logService.error("[NewApplicationsDialog] Failed to check for tasks", error);