From 06d15e96811f709f285299978424ae537abe4f6a Mon Sep 17 00:00:00 2001 From: Shane Melton Date: Tue, 16 Dec 2025 15:03:48 -0800 Subject: [PATCH 01/99] [PM-27675] Browser item transfer integration (#17918) * [PM-27675] Integrate dialogs into VaultItemTransferService * [PM-27675] Update tests for new dialogs * [PM-27675] Center dialogs and prevent closing with escape or pointer events * [PM-27675] Add transferInProgress$ observable to VaultItemsTransferService * [PM-27675] Hook vault item transfer service into browser vault component * [PM-27675] Move defaultUserCollection$ to collection service * [PM-27675] Cleanup dialog styles * [PM-27675] Introduce readySubject to popup vault component to keep prevent flashing content while item transfer is in progress * [PM-27675] Fix vault-v2 tests --- .../vault-v2/vault-v2.component.html | 2 +- .../vault-v2/vault-v2.component.spec.ts | 18 +- .../components/vault-v2/vault-v2.component.ts | 39 +- .../abstractions/collection.service.ts | 8 + .../default-collection.service.spec.ts | 80 +++- .../services/default-collection.service.ts | 11 + .../vault-items-transfer.service.ts | 5 + .../leave-confirmation-dialog.component.html | 4 +- .../leave-confirmation-dialog.component.ts | 3 + .../transfer-items-dialog.component.html | 2 +- .../transfer-items-dialog.component.ts | 3 + ...fault-vault-items-transfer.service.spec.ts | 342 +++++++++++++----- .../default-vault-items-transfer.service.ts | 84 +++-- 13 files changed, 464 insertions(+), 137 deletions(-) diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.html b/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.html index 347c5fe6286..6382b5fee0e 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.html +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.html @@ -108,7 +108,7 @@ - + { stop: jest.fn(), } as Partial; + const vaultItemsTransferSvc = { + transferInProgress$: new BehaviorSubject(false), + enforceOrganizationDataOwnership: jest.fn().mockResolvedValue(undefined), + } as Partial; + function getObs(cmp: any, key: string): Observable { return cmp[key] as Observable; } @@ -283,6 +292,9 @@ describe("VaultV2Component", () => { AutofillVaultListItemsComponent, VaultListItemsContainerComponent, ], + providers: [ + { provide: VaultItemsTransferService, useValue: DefaultVaultItemsTransferService }, + ], }, add: { imports: [ @@ -296,6 +308,7 @@ describe("VaultV2Component", () => { AutofillVaultListItemsStubComponent, VaultListItemsContainerStubComponent, ], + providers: [{ provide: VaultItemsTransferService, useValue: vaultItemsTransferSvc }], }, }); @@ -344,6 +357,7 @@ describe("VaultV2Component", () => { it("loading$ is true when items loading or filters missing; false when both ready", () => { const itemsLoading$ = itemsSvc.loading$ as unknown as BehaviorSubject; const allFilters$ = filtersSvc.allFilters$ as unknown as Subject; + const readySubject$ = component["readySubject"] as unknown as BehaviorSubject; const values: boolean[] = []; getObs(component, "loading$").subscribe((v) => values.push(!!v)); @@ -354,6 +368,8 @@ describe("VaultV2Component", () => { itemsLoading$.next(false); + readySubject$.next(true); + expect(values[values.length - 1]).toBe(false); }); diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.ts index 63d971081df..30d1d21abfb 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.ts @@ -16,6 +16,7 @@ import { switchMap, take, tap, + BehaviorSubject, } from "rxjs"; import { PremiumUpgradeDialogComponent } from "@bitwarden/angular/billing/components"; @@ -42,7 +43,11 @@ import { NoItemsModule, TypographyModule, } from "@bitwarden/components"; -import { DecryptionFailureDialogComponent } from "@bitwarden/vault"; +import { + DecryptionFailureDialogComponent, + VaultItemsTransferService, + DefaultVaultItemsTransferService, +} from "@bitwarden/vault"; import { CurrentAccountComponent } from "../../../../auth/popup/account-switching/current-account.component"; import { BrowserApi } from "../../../../platform/browser/browser-api"; @@ -105,6 +110,7 @@ type VaultState = UnionOfValues; VaultFadeInOutSkeletonComponent, VaultFadeInOutComponent, ], + providers: [{ provide: VaultItemsTransferService, useClass: DefaultVaultItemsTransferService }], }) export class VaultV2Component implements OnInit, AfterViewInit, OnDestroy { // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals @@ -125,7 +131,22 @@ export class VaultV2Component implements OnInit, AfterViewInit, OnDestroy { activeUserId: UserId | null = null; - private loading$ = this.vaultPopupLoadingService.loading$.pipe( + /** + * Subject that indicates whether the vault is ready to render + * and that all initialization tasks have been completed (ngOnInit). + * @private + */ + private readySubject = new BehaviorSubject(false); + + /** + * Indicates whether the vault is loading and not yet ready to be displayed. + * @protected + */ + protected loading$ = combineLatest([ + this.vaultPopupLoadingService.loading$, + this.readySubject.asObservable(), + ]).pipe( + map(([loading, ready]) => loading || !ready), distinctUntilChanged(), tap((loading) => { const key = loading ? "loadingVault" : "vaultLoaded"; @@ -200,14 +221,15 @@ export class VaultV2Component implements OnInit, AfterViewInit, OnDestroy { protected showSkeletonsLoaders$ = combineLatest([ this.loading$, this.searchService.isCipherSearching$, + this.vaultItemsTransferService.transferInProgress$, this.skeletonFeatureFlag$, ]).pipe( - map( - ([loading, cipherSearching, skeletonsEnabled]) => - (loading || cipherSearching) && skeletonsEnabled, - ), + map(([loading, cipherSearching, transferInProgress, skeletonsEnabled]) => { + return (loading || cipherSearching || transferInProgress) && skeletonsEnabled; + }), distinctUntilChanged(), skeletonLoadingDelay(), + shareReplay({ bufferSize: 1, refCount: true }), ); protected newItemItemValues$: Observable = @@ -251,6 +273,7 @@ export class VaultV2Component implements OnInit, AfterViewInit, OnDestroy { private i18nService: I18nService, private configService: ConfigService, private searchService: SearchService, + private vaultItemsTransferService: VaultItemsTransferService, ) { combineLatest([ this.vaultPopupItemsService.emptyVault$, @@ -305,6 +328,10 @@ export class VaultV2Component implements OnInit, AfterViewInit, OnDestroy { cipherIds: ciphers.map((c) => c.id as CipherId), }); }); + + await this.vaultItemsTransferService.enforceOrganizationDataOwnership(this.activeUserId); + + this.readySubject.next(true); } ngOnDestroy() { diff --git a/libs/admin-console/src/common/collections/abstractions/collection.service.ts b/libs/admin-console/src/common/collections/abstractions/collection.service.ts index f879831324d..f0f02ee377e 100644 --- a/libs/admin-console/src/common/collections/abstractions/collection.service.ts +++ b/libs/admin-console/src/common/collections/abstractions/collection.service.ts @@ -9,6 +9,14 @@ import { CollectionData, Collection, CollectionView } from "../models"; export abstract class CollectionService { abstract encryptedCollections$(userId: UserId): Observable; abstract decryptedCollections$(userId: UserId): Observable; + + /** + * Gets the default collection for a user in a given organization, if it exists. + */ + abstract defaultUserCollection$( + userId: UserId, + orgId: OrganizationId, + ): Observable; abstract upsert(collection: CollectionData, userId: UserId): Promise; abstract replace(collections: { [id: string]: CollectionData }, userId: UserId): Promise; /** diff --git a/libs/admin-console/src/common/collections/services/default-collection.service.spec.ts b/libs/admin-console/src/common/collections/services/default-collection.service.spec.ts index ced3b720e3c..2eaaa48594e 100644 --- a/libs/admin-console/src/common/collections/services/default-collection.service.spec.ts +++ b/libs/admin-console/src/common/collections/services/default-collection.service.spec.ts @@ -15,9 +15,10 @@ import { } from "@bitwarden/common/spec"; import { CollectionId, OrganizationId, UserId } from "@bitwarden/common/types/guid"; import { OrgKey } from "@bitwarden/common/types/key"; +import { newGuid } from "@bitwarden/guid"; import { KeyService } from "@bitwarden/key-management"; -import { CollectionData, CollectionView } from "../models"; +import { CollectionData, CollectionTypes, CollectionView } from "../models"; import { DECRYPTED_COLLECTION_DATA_KEY, ENCRYPTED_COLLECTION_DATA_KEY } from "./collection.state"; import { DefaultCollectionService } from "./default-collection.service"; @@ -389,6 +390,83 @@ describe("DefaultCollectionService", () => { }); }); + describe("defaultUserCollection$", () => { + it("returns the default collection when one exists matching the org", async () => { + const orgId = newGuid() as OrganizationId; + const defaultCollection = collectionViewDataFactory(orgId); + defaultCollection.type = CollectionTypes.DefaultUserCollection; + + const regularCollection = collectionViewDataFactory(orgId); + regularCollection.type = CollectionTypes.SharedCollection; + + await setDecryptedState([defaultCollection, regularCollection]); + + const result = await firstValueFrom(collectionService.defaultUserCollection$(userId, orgId)); + + expect(result).toBeDefined(); + expect(result?.id).toBe(defaultCollection.id); + expect(result?.isDefaultCollection).toBe(true); + }); + + it("returns undefined when no default collection exists", async () => { + const orgId = newGuid() as OrganizationId; + const collection1 = collectionViewDataFactory(orgId); + collection1.type = CollectionTypes.SharedCollection; + + const collection2 = collectionViewDataFactory(orgId); + collection2.type = CollectionTypes.SharedCollection; + + await setDecryptedState([collection1, collection2]); + + const result = await firstValueFrom(collectionService.defaultUserCollection$(userId, orgId)); + + expect(result).toBeUndefined(); + }); + + it("returns undefined when default collection exists but for different org", async () => { + const orgA = newGuid() as OrganizationId; + const orgB = newGuid() as OrganizationId; + + const defaultCollectionForOrgA = collectionViewDataFactory(orgA); + defaultCollectionForOrgA.type = CollectionTypes.DefaultUserCollection; + + await setDecryptedState([defaultCollectionForOrgA]); + + const result = await firstValueFrom(collectionService.defaultUserCollection$(userId, orgB)); + + expect(result).toBeUndefined(); + }); + + it("returns undefined when collections array is empty", async () => { + const orgId = newGuid() as OrganizationId; + + await setDecryptedState([]); + + const result = await firstValueFrom(collectionService.defaultUserCollection$(userId, orgId)); + + expect(result).toBeUndefined(); + }); + + it("returns correct collection when multiple orgs have default collections", async () => { + const orgA = newGuid() as OrganizationId; + const orgB = newGuid() as OrganizationId; + + const defaultCollectionForOrgA = collectionViewDataFactory(orgA); + defaultCollectionForOrgA.type = CollectionTypes.DefaultUserCollection; + + const defaultCollectionForOrgB = collectionViewDataFactory(orgB); + defaultCollectionForOrgB.type = CollectionTypes.DefaultUserCollection; + + await setDecryptedState([defaultCollectionForOrgA, defaultCollectionForOrgB]); + + const result = await firstValueFrom(collectionService.defaultUserCollection$(userId, orgB)); + + expect(result).toBeDefined(); + expect(result?.id).toBe(defaultCollectionForOrgB.id); + expect(result?.organizationId).toBe(orgB); + }); + }); + const setEncryptedState = (collectionData: CollectionData[] | null) => stateProvider.setUserState( ENCRYPTED_COLLECTION_DATA_KEY, diff --git a/libs/admin-console/src/common/collections/services/default-collection.service.ts b/libs/admin-console/src/common/collections/services/default-collection.service.ts index 0511b692b38..ccc2e6f0de5 100644 --- a/libs/admin-console/src/common/collections/services/default-collection.service.ts +++ b/libs/admin-console/src/common/collections/services/default-collection.service.ts @@ -87,6 +87,17 @@ export class DefaultCollectionService implements CollectionService { return result$; } + defaultUserCollection$( + userId: UserId, + orgId: OrganizationId, + ): Observable { + return this.decryptedCollections$(userId).pipe( + map((collections) => { + return collections.find((c) => c.isDefaultCollection && c.organizationId === orgId); + }), + ); + } + private initializeDecryptedState(userId: UserId): Observable { return combineLatest([ this.encryptedCollections$(userId), diff --git a/libs/vault/src/abstractions/vault-items-transfer.service.ts b/libs/vault/src/abstractions/vault-items-transfer.service.ts index ced9f71eb83..0fc19cc0e79 100644 --- a/libs/vault/src/abstractions/vault-items-transfer.service.ts +++ b/libs/vault/src/abstractions/vault-items-transfer.service.ts @@ -31,6 +31,11 @@ export type UserMigrationInfo = }; export abstract class VaultItemsTransferService { + /** + * Indicates whether a vault items transfer is currently in progress. + */ + abstract transferInProgress$: Observable; + /** * Gets information about whether the given user requires migration of their vault items * from My Vault to a My Items collection, and whether they are capable of performing that migration. diff --git a/libs/vault/src/components/vault-items-transfer/leave-confirmation-dialog.component.html b/libs/vault/src/components/vault-items-transfer/leave-confirmation-dialog.component.html index f0d644fecff..6d1045e1a86 100644 --- a/libs/vault/src/components/vault-items-transfer/leave-confirmation-dialog.component.html +++ b/libs/vault/src/components/vault-items-transfer/leave-confirmation-dialog.component.html @@ -11,7 +11,7 @@

{{ "leaveConfirmationDialogContentOne" | i18n }}

-

+

{{ "leaveConfirmationDialogContentTwo" | i18n }}

@@ -25,7 +25,7 @@ {{ "goBack" | i18n }} - + {{ "howToManageMyVault" | i18n }} diff --git a/libs/vault/src/components/vault-items-transfer/leave-confirmation-dialog.component.ts b/libs/vault/src/components/vault-items-transfer/leave-confirmation-dialog.component.ts index bd32a1ea6dd..af106376a79 100644 --- a/libs/vault/src/components/vault-items-transfer/leave-confirmation-dialog.component.ts +++ b/libs/vault/src/components/vault-items-transfer/leave-confirmation-dialog.component.ts @@ -12,6 +12,7 @@ import { DialogModule, LinkModule, TypographyModule, + CenterPositionStrategy, } from "@bitwarden/components"; export interface LeaveConfirmationDialogParams { @@ -58,6 +59,8 @@ export class LeaveConfirmationDialogComponent { static open(dialogService: DialogService, config: DialogConfig) { return dialogService.open(LeaveConfirmationDialogComponent, { + positionStrategy: new CenterPositionStrategy(), + disableClose: true, ...config, }); } diff --git a/libs/vault/src/components/vault-items-transfer/transfer-items-dialog.component.html b/libs/vault/src/components/vault-items-transfer/transfer-items-dialog.component.html index 0b77d4ba7d8..3cf626baaf7 100644 --- a/libs/vault/src/components/vault-items-transfer/transfer-items-dialog.component.html +++ b/libs/vault/src/components/vault-items-transfer/transfer-items-dialog.component.html @@ -14,7 +14,7 @@ {{ "declineAndLeave" | i18n }} - + {{ "whyAmISeeingThis" | i18n }} diff --git a/libs/vault/src/components/vault-items-transfer/transfer-items-dialog.component.ts b/libs/vault/src/components/vault-items-transfer/transfer-items-dialog.component.ts index f28ad2ab3ec..619181f37fc 100644 --- a/libs/vault/src/components/vault-items-transfer/transfer-items-dialog.component.ts +++ b/libs/vault/src/components/vault-items-transfer/transfer-items-dialog.component.ts @@ -12,6 +12,7 @@ import { DialogModule, LinkModule, TypographyModule, + CenterPositionStrategy, } from "@bitwarden/components"; export interface TransferItemsDialogParams { @@ -58,6 +59,8 @@ export class TransferItemsDialogComponent { static open(dialogService: DialogService, config: DialogConfig) { return dialogService.open(TransferItemsDialogComponent, { + positionStrategy: new CenterPositionStrategy(), + disableClose: true, ...config, }); } diff --git a/libs/vault/src/services/default-vault-items-transfer.service.spec.ts b/libs/vault/src/services/default-vault-items-transfer.service.spec.ts index d85fe2ffd43..d78cf95ebf2 100644 --- a/libs/vault/src/services/default-vault-items-transfer.service.spec.ts +++ b/libs/vault/src/services/default-vault-items-transfer.service.spec.ts @@ -1,5 +1,5 @@ import { mock, MockProxy } from "jest-mock-extended"; -import { firstValueFrom, of } from "rxjs"; +import { firstValueFrom, of, Subject } from "rxjs"; // eslint-disable-next-line no-restricted-imports import { CollectionService, CollectionView } from "@bitwarden/admin-console/common"; @@ -14,14 +14,20 @@ import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.servic import { OrganizationId, CollectionId } from "@bitwarden/common/types/guid"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; -import { DialogService, ToastService } from "@bitwarden/components"; +import { DialogRef, DialogService, ToastService } from "@bitwarden/components"; import { LogService } from "@bitwarden/logging"; import { UserId } from "@bitwarden/user-core"; +import { + LeaveConfirmationDialogResult, + TransferItemsDialogResult, +} from "../components/vault-items-transfer"; + import { DefaultVaultItemsTransferService } from "./default-vault-items-transfer.service"; describe("DefaultVaultItemsTransferService", () => { let service: DefaultVaultItemsTransferService; + let transferInProgressValues: boolean[]; let mockCipherService: MockProxy; let mockPolicyService: MockProxy; @@ -37,6 +43,25 @@ describe("DefaultVaultItemsTransferService", () => { const organizationId = "org-id" as OrganizationId; const collectionId = "collection-id" as CollectionId; + /** + * Creates a mock DialogRef that emits the provided result when closed + */ + function createMockDialogRef(result: T): DialogRef { + const closed$ = new Subject(); + const dialogRef = { + closed: closed$.asObservable(), + close: jest.fn(), + } as unknown as DialogRef; + + // Emit the result asynchronously to simulate dialog closing + setTimeout(() => { + closed$.next(result); + closed$.complete(); + }, 0); + + return dialogRef; + } + beforeEach(() => { mockCipherService = mock(); mockPolicyService = mock(); @@ -49,6 +74,7 @@ describe("DefaultVaultItemsTransferService", () => { mockConfigService = mock(); mockI18nService.t.mockImplementation((key) => key); + transferInProgressValues = []; service = new DefaultVaultItemsTransferService( mockCipherService, @@ -69,12 +95,12 @@ describe("DefaultVaultItemsTransferService", () => { policies?: Policy[]; organizations?: Organization[]; ciphers?: CipherView[]; - collections?: CollectionView[]; + defaultCollection?: CollectionView; }): void { mockPolicyService.policiesByType$.mockReturnValue(of(options.policies ?? [])); mockOrganizationService.organizations$.mockReturnValue(of(options.organizations ?? [])); mockCipherService.cipherViews$.mockReturnValue(of(options.ciphers ?? [])); - mockCollectionService.decryptedCollections$.mockReturnValue(of(options.collections ?? [])); + mockCollectionService.defaultUserCollection$.mockReturnValue(of(options.defaultCollection)); } it("calls policiesByType$ with correct PolicyType", async () => { @@ -151,39 +177,12 @@ describe("DefaultVaultItemsTransferService", () => { }); it("includes defaultCollectionId when a default collection exists", async () => { - mockCollectionService.decryptedCollections$.mockReturnValue( - of([ - { - id: collectionId, - organizationId: organizationId, - isDefaultCollection: true, - } as CollectionView, - ]), - ); - - const result = await firstValueFrom(service.userMigrationInfo$(userId)); - - expect(result).toEqual({ - requiresMigration: true, - enforcingOrganization: organization, - defaultCollectionId: collectionId, - }); - }); - - it("returns default collection only for the enforcing organization", async () => { - mockCollectionService.decryptedCollections$.mockReturnValue( - of([ - { - id: "wrong-collection-id" as CollectionId, - organizationId: "wrong-org-id" as OrganizationId, - isDefaultCollection: true, - } as CollectionView, - { - id: collectionId, - organizationId: organizationId, - isDefaultCollection: true, - } as CollectionView, - ]), + mockCollectionService.defaultUserCollection$.mockReturnValue( + of({ + id: collectionId, + organizationId: organizationId, + isDefaultCollection: true, + } as CollectionView), ); const result = await firstValueFrom(service.userMigrationInfo$(userId)); @@ -542,13 +541,13 @@ describe("DefaultVaultItemsTransferService", () => { policies?: Policy[]; organizations?: Organization[]; ciphers?: CipherView[]; - collections?: CollectionView[]; + defaultCollection?: CollectionView; }): void { mockConfigService.getFeatureFlag.mockResolvedValue(options.featureEnabled ?? true); mockPolicyService.policiesByType$.mockReturnValue(of(options.policies ?? [])); mockOrganizationService.organizations$.mockReturnValue(of(options.organizations ?? [])); mockCipherService.cipherViews$.mockReturnValue(of(options.ciphers ?? [])); - mockCollectionService.decryptedCollections$.mockReturnValue(of(options.collections ?? [])); + mockCollectionService.defaultUserCollection$.mockReturnValue(of(options.defaultCollection)); } it("does nothing when feature flag is disabled", async () => { @@ -557,13 +556,11 @@ describe("DefaultVaultItemsTransferService", () => { policies: [policy], organizations: [organization], ciphers: [{ id: "cipher-1" } as CipherView], - collections: [ - { - id: collectionId, - organizationId: organizationId, - isDefaultCollection: true, - } as CollectionView, - ], + defaultCollection: { + id: collectionId, + organizationId: organizationId, + isDefaultCollection: true, + } as CollectionView, }); await service.enforceOrganizationDataOwnership(userId); @@ -571,7 +568,7 @@ describe("DefaultVaultItemsTransferService", () => { expect(mockConfigService.getFeatureFlag).toHaveBeenCalledWith( FeatureFlag.MigrateMyVaultToMyItems, ); - expect(mockDialogService.openSimpleDialog).not.toHaveBeenCalled(); + expect(mockDialogService.open).not.toHaveBeenCalled(); expect(mockCipherService.shareManyWithServer).not.toHaveBeenCalled(); }); @@ -580,7 +577,7 @@ describe("DefaultVaultItemsTransferService", () => { await service.enforceOrganizationDataOwnership(userId); - expect(mockDialogService.openSimpleDialog).not.toHaveBeenCalled(); + expect(mockDialogService.open).not.toHaveBeenCalled(); expect(mockCipherService.shareManyWithServer).not.toHaveBeenCalled(); }); @@ -593,7 +590,7 @@ describe("DefaultVaultItemsTransferService", () => { await service.enforceOrganizationDataOwnership(userId); - expect(mockDialogService.openSimpleDialog).not.toHaveBeenCalled(); + expect(mockDialogService.open).not.toHaveBeenCalled(); expect(mockCipherService.shareManyWithServer).not.toHaveBeenCalled(); }); @@ -602,7 +599,6 @@ describe("DefaultVaultItemsTransferService", () => { policies: [policy], organizations: [organization], ciphers: [{ id: "cipher-1" } as CipherView], - collections: [], }); await service.enforceOrganizationDataOwnership(userId); @@ -610,69 +606,48 @@ describe("DefaultVaultItemsTransferService", () => { expect(mockLogService.warning).toHaveBeenCalledWith( "Default collection is missing for user during organization data ownership enforcement", ); - expect(mockDialogService.openSimpleDialog).not.toHaveBeenCalled(); + expect(mockDialogService.open).not.toHaveBeenCalled(); expect(mockCipherService.shareManyWithServer).not.toHaveBeenCalled(); }); - it("shows confirmation dialog when migration is required", async () => { + it("does not transfer items when user declines and confirms leaving", async () => { setupMocksForEnforcementScenario({ policies: [policy], organizations: [organization], ciphers: [{ id: "cipher-1" } as CipherView], - collections: [ - { - id: collectionId, - organizationId: organizationId, - isDefaultCollection: true, - } as CollectionView, - ], + defaultCollection: { + id: collectionId, + organizationId: organizationId, + isDefaultCollection: true, + } as CollectionView, }); - mockDialogService.openSimpleDialog.mockResolvedValue(false); - await service.enforceOrganizationDataOwnership(userId); - - expect(mockDialogService.openSimpleDialog).toHaveBeenCalledWith({ - title: "Requires migration", - content: "Your vault requires migration of personal items to your organization.", - type: "warning", - }); - }); - - it("does not transfer items when user declines confirmation", async () => { - setupMocksForEnforcementScenario({ - policies: [policy], - organizations: [organization], - ciphers: [{ id: "cipher-1" } as CipherView], - collections: [ - { - id: collectionId, - organizationId: organizationId, - isDefaultCollection: true, - } as CollectionView, - ], - }); - mockDialogService.openSimpleDialog.mockResolvedValue(false); + // User declines transfer, then confirms leaving + mockDialogService.open + .mockReturnValueOnce(createMockDialogRef(TransferItemsDialogResult.Declined)) + .mockReturnValueOnce(createMockDialogRef(LeaveConfirmationDialogResult.Confirmed)); await service.enforceOrganizationDataOwnership(userId); expect(mockCipherService.shareManyWithServer).not.toHaveBeenCalled(); }); - it("transfers items and shows success toast when user confirms", async () => { + it("transfers items and shows success toast when user accepts transfer", async () => { const personalCiphers = [{ id: "cipher-1" } as CipherView]; setupMocksForEnforcementScenario({ policies: [policy], organizations: [organization], ciphers: personalCiphers, - collections: [ - { - id: collectionId, - organizationId: organizationId, - isDefaultCollection: true, - } as CollectionView, - ], + defaultCollection: { + id: collectionId, + organizationId: organizationId, + isDefaultCollection: true, + } as CollectionView, }); - mockDialogService.openSimpleDialog.mockResolvedValue(true); + + mockDialogService.open.mockReturnValueOnce( + createMockDialogRef(TransferItemsDialogResult.Accepted), + ); mockCipherService.shareManyWithServer.mockResolvedValue(undefined); await service.enforceOrganizationDataOwnership(userId); @@ -695,15 +670,16 @@ describe("DefaultVaultItemsTransferService", () => { policies: [policy], organizations: [organization], ciphers: personalCiphers, - collections: [ - { - id: collectionId, - organizationId: organizationId, - isDefaultCollection: true, - } as CollectionView, - ], + defaultCollection: { + id: collectionId, + organizationId: organizationId, + isDefaultCollection: true, + } as CollectionView, }); - mockDialogService.openSimpleDialog.mockResolvedValue(true); + + mockDialogService.open.mockReturnValueOnce( + createMockDialogRef(TransferItemsDialogResult.Accepted), + ); mockCipherService.shareManyWithServer.mockRejectedValue(new Error("Transfer failed")); await service.enforceOrganizationDataOwnership(userId); @@ -717,5 +693,171 @@ describe("DefaultVaultItemsTransferService", () => { message: "errorOccurred", }); }); + + it("re-shows transfer dialog when user goes back from leave confirmation", async () => { + const personalCiphers = [{ id: "cipher-1" } as CipherView]; + setupMocksForEnforcementScenario({ + policies: [policy], + organizations: [organization], + ciphers: personalCiphers, + defaultCollection: { + id: collectionId, + organizationId: organizationId, + isDefaultCollection: true, + } as CollectionView, + }); + + // User declines, goes back, then accepts + mockDialogService.open + .mockReturnValueOnce(createMockDialogRef(TransferItemsDialogResult.Declined)) + .mockReturnValueOnce(createMockDialogRef(LeaveConfirmationDialogResult.Back)) + .mockReturnValueOnce(createMockDialogRef(TransferItemsDialogResult.Accepted)); + mockCipherService.shareManyWithServer.mockResolvedValue(undefined); + + await service.enforceOrganizationDataOwnership(userId); + + // Dialog should have been opened 3 times: transfer -> leave -> transfer (after going back) + expect(mockDialogService.open).toHaveBeenCalledTimes(3); + expect(mockCipherService.shareManyWithServer).toHaveBeenCalled(); + }); + + it("allows multiple back navigations before accepting transfer", async () => { + const personalCiphers = [{ id: "cipher-1" } as CipherView]; + setupMocksForEnforcementScenario({ + policies: [policy], + organizations: [organization], + ciphers: personalCiphers, + defaultCollection: { + id: collectionId, + organizationId: organizationId, + isDefaultCollection: true, + } as CollectionView, + }); + + // User declines, goes back, declines again, goes back again, then accepts + mockDialogService.open + .mockReturnValueOnce(createMockDialogRef(TransferItemsDialogResult.Declined)) + .mockReturnValueOnce(createMockDialogRef(LeaveConfirmationDialogResult.Back)) + .mockReturnValueOnce(createMockDialogRef(TransferItemsDialogResult.Declined)) + .mockReturnValueOnce(createMockDialogRef(LeaveConfirmationDialogResult.Back)) + .mockReturnValueOnce(createMockDialogRef(TransferItemsDialogResult.Accepted)); + mockCipherService.shareManyWithServer.mockResolvedValue(undefined); + + await service.enforceOrganizationDataOwnership(userId); + + // Dialog should have been opened 5 times + expect(mockDialogService.open).toHaveBeenCalledTimes(5); + expect(mockCipherService.shareManyWithServer).toHaveBeenCalled(); + }); + + it("allows user to go back and then confirm leaving", async () => { + setupMocksForEnforcementScenario({ + policies: [policy], + organizations: [organization], + ciphers: [{ id: "cipher-1" } as CipherView], + defaultCollection: { + id: collectionId, + organizationId: organizationId, + isDefaultCollection: true, + } as CollectionView, + }); + + // User declines, goes back, declines again, then confirms leaving + mockDialogService.open + .mockReturnValueOnce(createMockDialogRef(TransferItemsDialogResult.Declined)) + .mockReturnValueOnce(createMockDialogRef(LeaveConfirmationDialogResult.Back)) + .mockReturnValueOnce(createMockDialogRef(TransferItemsDialogResult.Declined)) + .mockReturnValueOnce(createMockDialogRef(LeaveConfirmationDialogResult.Confirmed)); + + await service.enforceOrganizationDataOwnership(userId); + + expect(mockDialogService.open).toHaveBeenCalledTimes(4); + expect(mockCipherService.shareManyWithServer).not.toHaveBeenCalled(); + }); + }); + + describe("transferInProgress$", () => { + const policy = { + organizationId: organizationId, + revisionDate: new Date("2024-01-01"), + } as Policy; + const organization = { + id: organizationId, + name: "Test Org", + } as Organization; + + function setupMocksForTransferScenario(options: { + featureEnabled?: boolean; + policies?: Policy[]; + organizations?: Organization[]; + ciphers?: CipherView[]; + defaultCollection?: CollectionView; + }): void { + mockConfigService.getFeatureFlag.mockResolvedValue(options.featureEnabled ?? true); + mockPolicyService.policiesByType$.mockReturnValue(of(options.policies ?? [])); + mockOrganizationService.organizations$.mockReturnValue(of(options.organizations ?? [])); + mockCipherService.cipherViews$.mockReturnValue(of(options.ciphers ?? [])); + mockCollectionService.defaultUserCollection$.mockReturnValue(of(options.defaultCollection)); + } + + it("emits false initially", async () => { + const result = await firstValueFrom(service.transferInProgress$); + + expect(result).toBe(false); + }); + + it("emits true during transfer and false after successful completion", async () => { + const personalCiphers = [{ id: "cipher-1" } as CipherView]; + setupMocksForTransferScenario({ + policies: [policy], + organizations: [organization], + ciphers: personalCiphers, + defaultCollection: { + id: collectionId, + organizationId: organizationId, + isDefaultCollection: true, + } as CollectionView, + }); + + mockDialogService.open.mockReturnValueOnce( + createMockDialogRef(TransferItemsDialogResult.Accepted), + ); + mockCipherService.shareManyWithServer.mockResolvedValue(undefined); + + // Subscribe to track all emitted values + service.transferInProgress$.subscribe((value) => transferInProgressValues.push(value)); + + await service.enforceOrganizationDataOwnership(userId); + + // Should have emitted: false (initial), true (transfer started), false (transfer completed) + expect(transferInProgressValues).toEqual([false, true, false]); + }); + + it("emits false after transfer fails with error", async () => { + const personalCiphers = [{ id: "cipher-1" } as CipherView]; + setupMocksForTransferScenario({ + policies: [policy], + organizations: [organization], + ciphers: personalCiphers, + defaultCollection: { + id: collectionId, + organizationId: organizationId, + isDefaultCollection: true, + } as CollectionView, + }); + + mockDialogService.open.mockReturnValueOnce( + createMockDialogRef(TransferItemsDialogResult.Accepted), + ); + mockCipherService.shareManyWithServer.mockRejectedValue(new Error("Transfer failed")); + + // Subscribe to track all emitted values + service.transferInProgress$.subscribe((value) => transferInProgressValues.push(value)); + + await service.enforceOrganizationDataOwnership(userId); + + // Should have emitted: false (initial), true (transfer started), false (transfer failed) + expect(transferInProgressValues).toEqual([false, true, false]); + }); }); }); diff --git a/libs/vault/src/services/default-vault-items-transfer.service.ts b/libs/vault/src/services/default-vault-items-transfer.service.ts index d9c490f870e..d7088873071 100644 --- a/libs/vault/src/services/default-vault-items-transfer.service.ts +++ b/libs/vault/src/services/default-vault-items-transfer.service.ts @@ -1,5 +1,13 @@ import { Injectable } from "@angular/core"; -import { firstValueFrom, switchMap, map, of, Observable, combineLatest } from "rxjs"; +import { + firstValueFrom, + switchMap, + map, + of, + Observable, + combineLatest, + BehaviorSubject, +} from "rxjs"; // eslint-disable-next-line no-restricted-imports import { CollectionService } from "@bitwarden/admin-console/common"; @@ -23,6 +31,12 @@ import { VaultItemsTransferService, UserMigrationInfo, } from "../abstractions/vault-items-transfer.service"; +import { + TransferItemsDialogComponent, + TransferItemsDialogResult, + LeaveConfirmationDialogComponent, + LeaveConfirmationDialogResult, +} from "../components/vault-items-transfer"; @Injectable() export class DefaultVaultItemsTransferService implements VaultItemsTransferService { @@ -38,6 +52,10 @@ export class DefaultVaultItemsTransferService implements VaultItemsTransferServi private configService: ConfigService, ) {} + private _transferInProgressSubject = new BehaviorSubject(false); + + transferInProgress$ = this._transferInProgressSubject.asObservable(); + private enforcingOrganization$(userId: UserId): Observable { return this.policyService.policiesByType$(PolicyType.OrganizationDataOwnership, userId).pipe( map( @@ -60,18 +78,6 @@ export class DefaultVaultItemsTransferService implements VaultItemsTransferServi ); } - private defaultUserCollection$( - userId: UserId, - organizationId: OrganizationId, - ): Observable { - return this.collectionService.decryptedCollections$(userId).pipe( - map((collections) => { - return collections.find((c) => c.isDefaultCollection && c.organizationId === organizationId) - ?.id; - }), - ); - } - userMigrationInfo$(userId: UserId): Observable { return this.enforcingOrganization$(userId).pipe( switchMap((enforcingOrganization) => { @@ -82,13 +88,13 @@ export class DefaultVaultItemsTransferService implements VaultItemsTransferServi } return combineLatest([ this.personalCiphers$(userId), - this.defaultUserCollection$(userId, enforcingOrganization.id), + this.collectionService.defaultUserCollection$(userId, enforcingOrganization.id), ]).pipe( - map(([personalCiphers, defaultCollectionId]): UserMigrationInfo => { + map(([personalCiphers, defaultCollection]): UserMigrationInfo => { return { requiresMigration: personalCiphers.length > 0, enforcingOrganization, - defaultCollectionId, + defaultCollectionId: defaultCollection?.id, }; }), ); @@ -96,6 +102,35 @@ export class DefaultVaultItemsTransferService implements VaultItemsTransferServi ); } + /** + * Prompts the user to accept or decline the vault items transfer. + * If declined, shows a leave confirmation dialog with option to go back. + * @returns true if user accepts transfer, false if user confirms leaving + */ + private async promptUserForTransfer(organizationName: string): Promise { + const confirmDialogRef = TransferItemsDialogComponent.open(this.dialogService, { + data: { organizationName }, + }); + + const confirmResult = await firstValueFrom(confirmDialogRef.closed); + + if (confirmResult === TransferItemsDialogResult.Accepted) { + return true; + } + + const leaveDialogRef = LeaveConfirmationDialogComponent.open(this.dialogService, { + data: { organizationName }, + }); + + const leaveResult = await firstValueFrom(leaveDialogRef.closed); + + if (leaveResult === LeaveConfirmationDialogResult.Back) { + return this.promptUserForTransfer(organizationName); + } + + return false; + } + async enforceOrganizationDataOwnership(userId: UserId): Promise { const featureEnabled = await this.configService.getFeatureFlag( FeatureFlag.MigrateMyVaultToMyItems, @@ -119,30 +154,29 @@ export class DefaultVaultItemsTransferService implements VaultItemsTransferServi return; } - // Temporary confirmation dialog. Full implementation in PM-27663 - const confirmMigration = await this.dialogService.openSimpleDialog({ - title: "Requires migration", - content: "Your vault requires migration of personal items to your organization.", - type: "warning", - }); + const userAcceptedTransfer = await this.promptUserForTransfer( + migrationInfo.enforcingOrganization.name, + ); - if (!confirmMigration) { - // TODO: Show secondary confirmation dialog in PM-27663, for now we just exit - // TODO: Revoke user from organization if they decline migration PM-29465 + if (!userAcceptedTransfer) { + // TODO: Revoke user from organization if they decline migration and show toast PM-29465 return; } try { + this._transferInProgressSubject.next(true); await this.transferPersonalItems( userId, migrationInfo.enforcingOrganization.id, migrationInfo.defaultCollectionId, ); + this._transferInProgressSubject.next(false); this.toastService.showToast({ variant: "success", message: this.i18nService.t("itemsTransferred"), }); } catch (error) { + this._transferInProgressSubject.next(false); this.logService.error("Error transferring personal items to organization", error); this.toastService.showToast({ variant: "error", From ba1c74b9052a79806e1f159547f4e5cc442227e7 Mon Sep 17 00:00:00 2001 From: Jason Ng Date: Tue, 16 Dec 2025 18:04:54 -0500 Subject: [PATCH 02/99] [PM-29286] clear selected items when filter is changed (#17929) * check filter inside vault-items and clear on change --- .../vault-items/vault-items.component.spec.ts | 31 ++++++++++++++++++- .../vault-items/vault-items.component.ts | 28 ++++++++++++++++- .../vault-items/vault-items.stories.ts | 12 +++++++ 3 files changed, 69 insertions(+), 2 deletions(-) diff --git a/apps/web/src/app/vault/components/vault-items/vault-items.component.spec.ts b/apps/web/src/app/vault/components/vault-items/vault-items.component.spec.ts index 1eccb4c49ce..c1c25c625da 100644 --- a/apps/web/src/app/vault/components/vault-items/vault-items.component.spec.ts +++ b/apps/web/src/app/vault/components/vault-items/vault-items.component.spec.ts @@ -1,6 +1,6 @@ import { ScrollingModule } from "@angular/cdk/scrolling"; import { TestBed } from "@angular/core/testing"; -import { of } from "rxjs"; +import { of, Subject } from "rxjs"; import { CollectionView } from "@bitwarden/admin-console/common"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; @@ -11,12 +11,15 @@ import { RestrictedItemTypesService } from "@bitwarden/common/vault/services/res import { CipherViewLike } from "@bitwarden/common/vault/utils/cipher-view-like-utils"; import { MenuModule, TableModule } from "@bitwarden/components"; import { I18nPipe } from "@bitwarden/ui-common"; +import { RoutedVaultFilterService } from "@bitwarden/web-vault/app/vault/individual-vault/vault-filter/services/routed-vault-filter.service"; +import { RoutedVaultFilterModel } from "@bitwarden/web-vault/app/vault/individual-vault/vault-filter/shared/models/routed-vault-filter.model"; import { VaultItem } from "./vault-item"; import { VaultItemsComponent } from "./vault-items.component"; describe("VaultItemsComponent", () => { let component: VaultItemsComponent; + let filterSelect: Subject; const cipher1: Partial = { id: "cipher-1", @@ -31,6 +34,8 @@ describe("VaultItemsComponent", () => { }; beforeEach(async () => { + filterSelect = new Subject(); + await TestBed.configureTestingModule({ declarations: [VaultItemsComponent], imports: [ScrollingModule, TableModule, I18nPipe, MenuModule], @@ -61,6 +66,12 @@ describe("VaultItemsComponent", () => { hasArchiveFlagEnabled$: of(true), }, }, + { + provide: RoutedVaultFilterService, + useValue: { + filter$: filterSelect, + }, + }, ], }); @@ -143,4 +154,22 @@ describe("VaultItemsComponent", () => { expect(component.bulkUnarchiveAllowed).toBe(false); }); }); + + describe("filter change handling", () => { + it("clears selection when routed filter changes", () => { + const items: VaultItem[] = [ + { cipher: cipher1 as CipherView }, + { cipher: cipher2 as CipherView }, + ]; + + component["selection"].select(...items); + expect(component["selection"].selected.length).toBeGreaterThan(0); + + filterSelect.next({ + folderId: "folderId", + }); + + expect(component["selection"].selected.length).toBe(0); + }); + }); }); diff --git a/apps/web/src/app/vault/components/vault-items/vault-items.component.ts b/apps/web/src/app/vault/components/vault-items/vault-items.component.ts index a935314eb3a..a51009a1e5b 100644 --- a/apps/web/src/app/vault/components/vault-items/vault-items.component.ts +++ b/apps/web/src/app/vault/components/vault-items/vault-items.component.ts @@ -3,7 +3,15 @@ import { SelectionModel } from "@angular/cdk/collections"; import { Component, EventEmitter, Input, Output } from "@angular/core"; import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; -import { Observable, combineLatest, map, of, startWith, switchMap } from "rxjs"; +import { + Observable, + combineLatest, + distinctUntilChanged, + map, + of, + startWith, + switchMap, +} from "rxjs"; import { CollectionView, Unassigned, CollectionAdminView } from "@bitwarden/admin-console/common"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; @@ -19,6 +27,7 @@ import { } from "@bitwarden/common/vault/utils/cipher-view-like-utils"; import { SortDirection, TableDataSource } from "@bitwarden/components"; import { OrganizationId } from "@bitwarden/sdk-internal"; +import { RoutedVaultFilterService } from "@bitwarden/web-vault/app/vault/individual-vault/vault-filter/services/routed-vault-filter.service"; import { GroupView } from "../../../admin-console/organizations/core"; @@ -152,6 +161,7 @@ export class VaultItemsComponent { protected cipherAuthorizationService: CipherAuthorizationService, protected restrictedItemTypesService: RestrictedItemTypesService, protected cipherArchiveService: CipherArchiveService, + protected routedVaultFilterService: RoutedVaultFilterService, ) { this.canDeleteSelected$ = this.selection.changed.pipe( startWith(null), @@ -219,6 +229,22 @@ export class VaultItemsComponent { ); }), ); + + this.routedVaultFilterService.filter$ + .pipe( + distinctUntilChanged( + (prev, curr) => + prev.organizationId === curr.organizationId && + prev.collectionId === curr.collectionId && + prev.folderId === curr.folderId && + prev.type === curr.type && + prev.organizationIdParamType === curr.organizationIdParamType, + ), + takeUntilDestroyed(), + ) + .subscribe(() => { + this.clearSelection(); + }); } clearSelection() { diff --git a/apps/web/src/app/vault/components/vault-items/vault-items.stories.ts b/apps/web/src/app/vault/components/vault-items/vault-items.stories.ts index d973fbcbbc7..a71427cf475 100644 --- a/apps/web/src/app/vault/components/vault-items/vault-items.stories.ts +++ b/apps/web/src/app/vault/components/vault-items/vault-items.stories.ts @@ -40,6 +40,7 @@ import { CipherAuthorizationService } from "@bitwarden/common/vault/services/cip import { RestrictedItemTypesService } from "@bitwarden/common/vault/services/restricted-item-types.service"; import { CipherViewLike } from "@bitwarden/common/vault/utils/cipher-view-like-utils"; import { LayoutComponent } from "@bitwarden/components"; +import { RoutedVaultFilterService } from "@bitwarden/web-vault/app/vault/individual-vault/vault-filter/services/routed-vault-filter.service"; import { GroupView } from "../../../admin-console/organizations/core"; import { PreloadedEnglishI18nModule } from "../../../core/tests"; @@ -150,6 +151,17 @@ export default { hasArchiveFlagEnabled$: of(true), }, }, + { + provide: RoutedVaultFilterService, + useValue: { + filter$: of({ + organizationId: null, + collectionId: null, + folderId: null, + type: null, + }), + }, + }, ], }), applicationConfig({ From ced97a4467de3f2eb8d0fb4a902fce9daea1ae33 Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk <167752252+mzieniukbw@users.noreply.github.com> Date: Wed, 17 Dec 2025 10:19:11 +0100 Subject: [PATCH 03/99] cli status command shows locked status when unlocked (#17708) --- apps/cli/src/commands/status.command.ts | 17 ++++++++++++----- apps/cli/src/oss-serve-configurator.ts | 1 + apps/cli/src/program.ts | 1 + 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/apps/cli/src/commands/status.command.ts b/apps/cli/src/commands/status.command.ts index f7fc8541a5f..7ae1e657630 100644 --- a/apps/cli/src/commands/status.command.ts +++ b/apps/cli/src/commands/status.command.ts @@ -1,12 +1,12 @@ -// FIXME: Update this file to be type safe and remove this and next line -// @ts-strict-ignore import { firstValueFrom, map } from "rxjs"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; +import { UserAutoUnlockKeyService } from "@bitwarden/common/platform/services/user-auto-unlock-key.service"; import { SyncService } from "@bitwarden/common/vault/abstractions/sync/sync.service.abstraction"; +import { UserId } from "@bitwarden/user-core"; import { Response } from "../models/response"; import { TemplateResponse } from "../models/response/template.response"; @@ -17,16 +17,17 @@ export class StatusCommand { private syncService: SyncService, private accountService: AccountService, private authService: AuthService, + private userAutoUnlockKeyService: UserAutoUnlockKeyService, ) {} async run(): Promise { try { const baseUrl = await this.baseUrl(); - const status = await this.status(); const lastSync = await this.syncService.getLastSync(); const [userId, email] = await firstValueFrom( this.accountService.activeAccount$.pipe(map((a) => [a?.id, a?.email])), ); + const status = await this.status(userId); return Response.success( new TemplateResponse({ @@ -42,12 +43,18 @@ export class StatusCommand { } } - private async baseUrl(): Promise { + private async baseUrl(): Promise { const env = await firstValueFrom(this.envService.environment$); return env.getUrls().base; } - private async status(): Promise<"unauthenticated" | "locked" | "unlocked"> { + private async status( + userId: UserId | undefined, + ): Promise<"unauthenticated" | "locked" | "unlocked"> { + if (userId != null) { + await this.userAutoUnlockKeyService.setUserKeyInMemoryIfAutoUserKeySet(userId); + } + const authStatus = await this.authService.getAuthStatus(); if (authStatus === AuthenticationStatus.Unlocked) { return "unlocked"; diff --git a/apps/cli/src/oss-serve-configurator.ts b/apps/cli/src/oss-serve-configurator.ts index dbe17224d07..e8f5e6acd9a 100644 --- a/apps/cli/src/oss-serve-configurator.ts +++ b/apps/cli/src/oss-serve-configurator.ts @@ -122,6 +122,7 @@ export class OssServeConfigurator { this.serviceContainer.syncService, this.serviceContainer.accountService, this.serviceContainer.authService, + this.serviceContainer.userAutoUnlockKeyService, ); this.deleteCommand = new DeleteCommand( this.serviceContainer.cipherService, diff --git a/apps/cli/src/program.ts b/apps/cli/src/program.ts index 3e5b5678629..b0c94b19ae9 100644 --- a/apps/cli/src/program.ts +++ b/apps/cli/src/program.ts @@ -524,6 +524,7 @@ export class Program extends BaseProgram { this.serviceContainer.syncService, this.serviceContainer.accountService, this.serviceContainer.authService, + this.serviceContainer.userAutoUnlockKeyService, ); const response = await command.run(); this.processResponse(response); From 4846d217a93b4b284ee428946a3239381aaf3e83 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Wed, 17 Dec 2025 10:57:24 +0100 Subject: [PATCH 04/99] [PM-28901] Fix master key not being set to state after kdf update (#17990) * Fix master key not being set to state after kdf update * Fix cli build * Fix test error * Fix hash purpose * Add test for master key being set * Fix incorrect variable name --- .../service-container/service-container.ts | 7 ++++- .../src/services/jslib-services.module.ts | 4 +-- .../kdf/change-kdf.service.spec.ts | 26 +++++++++++++++++-- .../key-management/kdf/change-kdf.service.ts | 20 +++++++++++++- 4 files changed, 51 insertions(+), 6 deletions(-) diff --git a/apps/cli/src/service-container/service-container.ts b/apps/cli/src/service-container/service-container.ts index 2d4ea7d00b5..f22f7cb6a00 100644 --- a/apps/cli/src/service-container/service-container.ts +++ b/apps/cli/src/service-container/service-container.ts @@ -982,7 +982,12 @@ export class ServiceContainer { this.masterPasswordApiService = new MasterPasswordApiService(this.apiService, this.logService); const changeKdfApiService = new DefaultChangeKdfApiService(this.apiService); - const changeKdfService = new DefaultChangeKdfService(changeKdfApiService, this.sdkService); + const changeKdfService = new DefaultChangeKdfService( + changeKdfApiService, + this.sdkService, + this.keyService, + this.masterPasswordService, + ); this.encryptedMigrator = new DefaultEncryptedMigrator( this.kdfConfigService, changeKdfService, diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index 816e09fd45d..6881862615d 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -528,7 +528,7 @@ const safeProviders: SafeProvider[] = [ safeProvider({ provide: ChangeKdfService, useClass: DefaultChangeKdfService, - deps: [ChangeKdfApiService, SdkService], + deps: [ChangeKdfApiService, SdkService, KeyService, InternalMasterPasswordServiceAbstraction], }), safeProvider({ provide: EncryptedMigrator, @@ -1333,7 +1333,7 @@ const safeProviders: SafeProvider[] = [ safeProvider({ provide: ChangeKdfService, useClass: DefaultChangeKdfService, - deps: [ChangeKdfApiService, SdkService], + deps: [ChangeKdfApiService, SdkService, KeyService, InternalMasterPasswordServiceAbstraction], }), safeProvider({ provide: AuthRequestServiceAbstraction, diff --git a/libs/common/src/key-management/kdf/change-kdf.service.spec.ts b/libs/common/src/key-management/kdf/change-kdf.service.spec.ts index 12096155641..e9f1ddca4e5 100644 --- a/libs/common/src/key-management/kdf/change-kdf.service.spec.ts +++ b/libs/common/src/key-management/kdf/change-kdf.service.spec.ts @@ -2,13 +2,14 @@ import { mock } from "jest-mock-extended"; import { of } from "rxjs"; // eslint-disable-next-line no-restricted-imports -import { PBKDF2KdfConfig } from "@bitwarden/key-management"; +import { KeyService, PBKDF2KdfConfig } from "@bitwarden/key-management"; import { makeEncString } from "../../../spec"; import { KdfRequest } from "../../models/request/kdf.request"; import { SdkService } from "../../platform/abstractions/sdk/sdk.service"; import { UserId } from "../../types/guid"; import { EncString } from "../crypto/models/enc-string"; +import { InternalMasterPasswordServiceAbstraction } from "../master-password/abstractions/master-password.service.abstraction"; import { MasterKeyWrappedUserKey, MasterPasswordAuthenticationHash, @@ -22,6 +23,8 @@ import { DefaultChangeKdfService } from "./change-kdf.service"; describe("ChangeKdfService", () => { const changeKdfApiService = mock(); const sdkService = mock(); + const keyService = mock(); + const masterPasswordService = mock(); let sut: DefaultChangeKdfService; @@ -48,7 +51,12 @@ describe("ChangeKdfService", () => { beforeEach(() => { sdkService.userClient$ = jest.fn((userId: UserId) => of(mockSdk)) as any; - sut = new DefaultChangeKdfService(changeKdfApiService, sdkService); + sut = new DefaultChangeKdfService( + changeKdfApiService, + sdkService, + keyService, + masterPasswordService, + ); }); afterEach(() => { @@ -163,6 +171,20 @@ describe("ChangeKdfService", () => { expect(changeKdfApiService.updateUserKdfParams).toHaveBeenCalledWith(expectedRequest); }); + it("should set master key and hash after KDF update", async () => { + const masterPassword = "masterPassword"; + const mockMasterKey = {} as any; + const mockHash = "localHash"; + + keyService.makeMasterKey.mockResolvedValue(mockMasterKey); + keyService.hashMasterKey.mockResolvedValue(mockHash); + + await sut.updateUserKdfParams(masterPassword, mockNewKdfConfig, mockUserId); + + expect(masterPasswordService.setMasterKey).toHaveBeenCalledWith(mockMasterKey, mockUserId); + expect(masterPasswordService.setMasterKeyHash).toHaveBeenCalledWith(mockHash, mockUserId); + }); + it("should properly dispose of SDK resources", async () => { const masterPassword = "masterPassword"; jest.spyOn(mockNewKdfConfig, "toSdkConfig").mockReturnValue({} as any); diff --git a/libs/common/src/key-management/kdf/change-kdf.service.ts b/libs/common/src/key-management/kdf/change-kdf.service.ts index 89d97e6704f..61842e7354b 100644 --- a/libs/common/src/key-management/kdf/change-kdf.service.ts +++ b/libs/common/src/key-management/kdf/change-kdf.service.ts @@ -1,12 +1,14 @@ import { firstValueFrom, map } from "rxjs"; import { assertNonNullish } from "@bitwarden/common/auth/utils"; +import { HashPurpose } from "@bitwarden/common/platform/enums"; import { UserId } from "@bitwarden/common/types/guid"; // eslint-disable-next-line no-restricted-imports -import { KdfConfig } from "@bitwarden/key-management"; +import { KdfConfig, KeyService } from "@bitwarden/key-management"; import { KdfRequest } from "../../models/request/kdf.request"; import { SdkService } from "../../platform/abstractions/sdk/sdk.service"; +import { InternalMasterPasswordServiceAbstraction } from "../master-password/abstractions/master-password.service.abstraction"; import { fromSdkAuthenticationData, MasterPasswordAuthenticationData, @@ -20,6 +22,8 @@ export class DefaultChangeKdfService implements ChangeKdfService { constructor( private changeKdfApiService: ChangeKdfApiService, private sdkService: SdkService, + private keyService: KeyService, + private masterPasswordService: InternalMasterPasswordServiceAbstraction, ) {} async updateUserKdfParams(masterPassword: string, kdf: KdfConfig, userId: UserId): Promise { @@ -56,5 +60,19 @@ export class DefaultChangeKdfService implements ChangeKdfService { const request = new KdfRequest(authenticationData, unlockData); request.authenticateWith(oldAuthenticationData); await this.changeKdfApiService.updateUserKdfParams(request); + + // Update the locally stored master key and hash, so that UV, etc. still works + const masterKey = await this.keyService.makeMasterKey( + masterPassword, + unlockData.salt, + unlockData.kdf, + ); + const localMasterKeyHash = await this.keyService.hashMasterKey( + masterPassword, + masterKey, + HashPurpose.LocalAuthorization, + ); + await this.masterPasswordService.setMasterKeyHash(localMasterKeyHash, userId); + await this.masterPasswordService.setMasterKey(masterKey, userId); } } From 3114b319204ef0c1948dc220c8a14e63cf6de83a Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Wed, 17 Dec 2025 11:59:40 +0100 Subject: [PATCH 05/99] Fix slow agent operations (#17867) --- .../desktop_native/core/src/ssh_agent/peerinfo/gather.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/apps/desktop/desktop_native/core/src/ssh_agent/peerinfo/gather.rs b/apps/desktop/desktop_native/core/src/ssh_agent/peerinfo/gather.rs index 699203d613d..bf8e24dd79c 100644 --- a/apps/desktop/desktop_native/core/src/ssh_agent/peerinfo/gather.rs +++ b/apps/desktop/desktop_native/core/src/ssh_agent/peerinfo/gather.rs @@ -3,8 +3,12 @@ use sysinfo::{Pid, System}; use super::models::PeerInfo; pub fn get_peer_info(peer_pid: u32) -> Result { - let s = System::new_all(); - if let Some(process) = s.process(Pid::from_u32(peer_pid)) { + let mut system = System::new(); + system.refresh_processes( + sysinfo::ProcessesToUpdate::Some(&[Pid::from_u32(peer_pid)]), + true, + ); + if let Some(process) = system.process(Pid::from_u32(peer_pid)) { let peer_process_name = match process.name().to_str() { Some(name) => name.to_string(), None => { From 24dcbb48c61309da382a6a33b96bdee9785e1295 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Wed, 17 Dec 2025 12:00:13 +0100 Subject: [PATCH 06/99] [PM-29418] Fix SSH list not working while locked (#17866) * Fix SSH list not working while locked * Add tests * Update private key to SDK test key * Cleanup --- .../desktop_native/core/src/ssh_agent/mod.rs | 86 ++++++++++++++++++- 1 file changed, 85 insertions(+), 1 deletion(-) diff --git a/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs b/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs index 8ba64618ffa..16cf778b575 100644 --- a/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs +++ b/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs @@ -226,7 +226,7 @@ impl BitwardenDesktopAgent { keystore.0.write().expect("RwLock is not poisoned").clear(); self.needs_unlock - .store(true, std::sync::atomic::Ordering::Relaxed); + .store(false, std::sync::atomic::Ordering::Relaxed); for (key, name, cipher_id) in new_keys.iter() { match parse_key_safe(key) { @@ -307,3 +307,87 @@ fn parse_key_safe(pem: &str) -> Result Err(anyhow::Error::msg(format!("Failed to parse key: {e}"))), } } + +#[cfg(test)] +mod tests { + use super::*; + + fn create_test_agent() -> ( + BitwardenDesktopAgent, + tokio::sync::mpsc::Receiver, + tokio::sync::broadcast::Sender<(u32, bool)>, + ) { + let (tx, rx) = tokio::sync::mpsc::channel(10); + let (response_tx, response_rx) = tokio::sync::broadcast::channel(10); + let agent = BitwardenDesktopAgent::new(tx, Arc::new(Mutex::new(response_rx))); + (agent, rx, response_tx) + } + + const TEST_ED25519_KEY: &str = "-----BEGIN OPENSSH PRIVATE KEY----- +b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAMwAAAAtzc2gtZW +QyNTUxOQAAACCWETEIh/JX+ZaK0Xlg5xZ9QIfjiKD2Qs57PjhRY45trwAAAIhqmvSbapr0 +mwAAAAtzc2gtZWQyNTUxOQAAACCWETEIh/JX+ZaK0Xlg5xZ9QIfjiKD2Qs57PjhRY45trw +AAAEAHVflTgR/OEl8mg9UEKcO7SeB0FH4AiaUurhVfBWT4eZYRMQiH8lf5lorReWDnFn1A +h+OIoPZCzns+OFFjjm2vAAAAAAECAwQF +-----END OPENSSH PRIVATE KEY-----"; + + #[tokio::test] + async fn test_needs_unlock_initial_state() { + let (agent, _rx, _response_tx) = create_test_agent(); + + // Initially, needs_unlock should be true + assert!(agent + .needs_unlock + .load(std::sync::atomic::Ordering::Relaxed)); + } + + #[tokio::test] + async fn test_needs_unlock_after_set_keys() { + let (mut agent, _rx, _response_tx) = create_test_agent(); + agent + .is_running + .store(true, std::sync::atomic::Ordering::Relaxed); + + // Set keys should set needs_unlock to false + let keys = vec![( + TEST_ED25519_KEY.to_string(), + "test_key".to_string(), + "cipher_id".to_string(), + )]; + + agent.set_keys(keys).unwrap(); + + assert!(!agent + .needs_unlock + .load(std::sync::atomic::Ordering::Relaxed)); + } + + #[tokio::test] + async fn test_needs_unlock_after_clear_keys() { + let (mut agent, _rx, _response_tx) = create_test_agent(); + agent + .is_running + .store(true, std::sync::atomic::Ordering::Relaxed); + + // Set keys first + let keys = vec![( + TEST_ED25519_KEY.to_string(), + "test_key".to_string(), + "cipher_id".to_string(), + )]; + agent.set_keys(keys).unwrap(); + + // Verify needs_unlock is false + assert!(!agent + .needs_unlock + .load(std::sync::atomic::Ordering::Relaxed)); + + // Clear keys should set needs_unlock back to true + agent.clear_keys().unwrap(); + + // Verify needs_unlock is true + assert!(agent + .needs_unlock + .load(std::sync::atomic::Ordering::Relaxed)); + } +} From e6062ec84e09c713cdac60b428bddba58cb4a9d0 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Wed, 17 Dec 2025 15:16:02 +0100 Subject: [PATCH 07/99] Fix agent crashing when account switching (#17868) --- .../autofill/services/ssh-agent.service.ts | 25 +++++-------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/apps/desktop/src/autofill/services/ssh-agent.service.ts b/apps/desktop/src/autofill/services/ssh-agent.service.ts index d5aed7f3289..a61903a5c82 100644 --- a/apps/desktop/src/autofill/services/ssh-agent.service.ts +++ b/apps/desktop/src/autofill/services/ssh-agent.service.ts @@ -46,8 +46,6 @@ export class SshAgentService implements OnDestroy { private authorizedSshKeys: Record = {}; - private isFeatureFlagEnabled = false; - private destroy$ = new Subject(); constructor( @@ -91,6 +89,7 @@ export class SshAgentService implements OnDestroy { filter(({ enabled }) => enabled), map(({ message }) => message), withLatestFrom(this.authService.activeAccountStatus$, this.accountService.activeAccount$), + filter(([, , account]) => account != null), // This switchMap handles unlocking the vault if it is locked: // - If the vault is locked, we will wait for it to be unlocked. // - If the vault is not unlocked within the timeout, we will abort the flow. @@ -127,7 +126,11 @@ export class SshAgentService implements OnDestroy { throw error; }), - map(() => [message, account.id]), + concatMap(async () => { + // The active account may have switched with account switching during unlock + const updatedAccount = await firstValueFrom(this.accountService.activeAccount$); + return [message, updatedAccount.id] as const; + }), ); } @@ -200,10 +203,6 @@ export class SshAgentService implements OnDestroy { this.accountService.activeAccount$.pipe(skip(1), takeUntil(this.destroy$)).subscribe({ next: (account) => { - if (!this.isFeatureFlagEnabled) { - return; - } - this.authorizedSshKeys = {}; this.logService.info("Active account changed, clearing SSH keys"); ipc.platform.sshAgent @@ -211,20 +210,12 @@ export class SshAgentService implements OnDestroy { .catch((e) => this.logService.error("Failed to clear SSH keys", e)); }, error: (e: unknown) => { - if (!this.isFeatureFlagEnabled) { - return; - } - this.logService.error("Error in active account observable", e); ipc.platform.sshAgent .clearKeys() .catch((e) => this.logService.error("Failed to clear SSH keys", e)); }, complete: () => { - if (!this.isFeatureFlagEnabled) { - return; - } - this.logService.info("Active account observable completed, clearing SSH keys"); this.authorizedSshKeys = {}; ipc.platform.sshAgent @@ -239,10 +230,6 @@ export class SshAgentService implements OnDestroy { ]) .pipe( concatMap(async ([, enabled]) => { - if (!this.isFeatureFlagEnabled) { - return; - } - if (!enabled) { await ipc.platform.sshAgent.clearKeys(); return; From 78f4947d006bc5d0d009ab6eaaba688d7b15a7c8 Mon Sep 17 00:00:00 2001 From: Leslie Tilton <23057410+Banrion@users.noreply.github.com> Date: Wed, 17 Dec 2025 09:23:09 -0600 Subject: [PATCH 08/99] [PM-25884] Disable phishing detection if safari is detected (#17655) * Disable phishing detection if safari is detected * Apply suggestion from @claude[bot] Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> * Move order of safari vs account checks --------- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> --- .../services/phishing-detection.service.spec.ts | 5 +++++ .../services/phishing-detection.service.ts | 13 ++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.spec.ts b/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.spec.ts index ceb18bd1573..06a37f12faa 100644 --- a/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.spec.ts +++ b/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.spec.ts @@ -79,4 +79,9 @@ describe("PhishingDetectionService", () => { // phishingDetectionSettingsService, // ); // }); + + // TODO + // it("should not enable phishing detection for safari", () => { + // + // }); }); diff --git a/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts b/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts index e04d08559ab..501dfbf7a50 100644 --- a/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts +++ b/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts @@ -5,6 +5,7 @@ import { filter, map, merge, + of, Subject, switchMap, tap, @@ -111,7 +112,17 @@ export class PhishingDetectionService { .messages$(PHISHING_DETECTION_CANCEL_COMMAND) .pipe(switchMap((message) => BrowserApi.closeTab(message.tabId))); - const phishingDetectionActive$ = phishingDetectionSettingsService.on$; + // Phishing detection is unavailable on Safari due to platform limitations + if (BrowserApi.isSafariApi) { + logService.debug( + "[PhishingDetectionService] Disabling phishing detection service for Safari.", + ); + } + + // Watching for settings changes to enable/disable phishing detection + const phishingDetectionActive$ = BrowserApi.isSafariApi + ? of(false) + : phishingDetectionSettingsService.on$; const initSub = phishingDetectionActive$ .pipe( From b0fcd92f35515144d04da0b8381b3f876c9b386e Mon Sep 17 00:00:00 2001 From: Alex <55413326+AlexRubik@users.noreply.github.com> Date: Wed, 17 Dec 2025 10:23:33 -0500 Subject: [PATCH 09/99] add padding between name and icons in health reports (#18002) Added tw-ml-1 class to shared (bwi-collection-shared) and attachment (bwi-paperclip) icons in report tables to add spacing between the item name and icons. Affected reports: - Weak passwords - Exposed passwords - Reused passwords - Unsecured websites - Inactive two-factor - Emergency access view (PM-29488) --- .../view/emergency-access-view.component.html | 4 ++-- .../reports/pages/exposed-passwords-report.component.html | 4 ++-- .../reports/pages/inactive-two-factor-report.component.html | 4 ++-- .../dirt/reports/pages/reused-passwords-report.component.html | 4 ++-- .../reports/pages/unsecured-websites-report.component.html | 4 ++-- .../dirt/reports/pages/weak-passwords-report.component.html | 4 ++-- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/apps/web/src/app/auth/settings/emergency-access/view/emergency-access-view.component.html b/apps/web/src/app/auth/settings/emergency-access/view/emergency-access-view.component.html index 20cc50c4d59..4aaac6aaa52 100644 --- a/apps/web/src/app/auth/settings/emergency-access/view/emergency-access-view.component.html +++ b/apps/web/src/app/auth/settings/emergency-access/view/emergency-access-view.component.html @@ -19,7 +19,7 @@ >