From 9f5dab05a265749b3b4338c7c8addce09078da3c Mon Sep 17 00:00:00 2001 From: Shane Melton Date: Mon, 8 Dec 2025 09:14:41 -0800 Subject: [PATCH] [PM-22750] Reimplement fix old attachment logic (#17689) * [PM-22750] Add upgradeOldCipherAttachment method to CipherService * [PM-22750] Refactor download attachment component to use signals * [PM-22750] Better download url handling * [PM-22750] Cleanup upgradeOldCipherAttachments method * [PM-22750] Refactor cipher-attachments.component to use Signals and OnPush * [PM-22750] Use the correct legacy decryption key for attachments without their own content encryption key * [PM-22750] Add fix attachment button back to attachments component * [PM-22750] Fix newly added output signals * [PM-22750] Fix failing test due to signal refactor * [PM-22750] Update copy --- apps/browser/src/_locales/en/messages.json | 9 + .../attachments-v2.component.spec.ts | 22 +- apps/desktop/src/locales/en/messages.json | 9 + apps/web/src/locales/en/messages.json | 9 + .../src/vault/abstractions/cipher.service.ts | 13 +- .../src/vault/services/cipher.service.ts | 103 ++++++++- .../cipher-attachments.component.html | 79 ++++--- .../cipher-attachments.component.spec.ts | 170 ++++++++------ .../cipher-attachments.component.ts | 216 ++++++++++-------- .../download-attachment.component.html | 19 +- .../download-attachment.component.spec.ts | 9 +- .../download-attachment.component.ts | 93 ++++---- 12 files changed, 491 insertions(+), 260 deletions(-) diff --git a/apps/browser/src/_locales/en/messages.json b/apps/browser/src/_locales/en/messages.json index cb858930650..e3b7c69e163 100644 --- a/apps/browser/src/_locales/en/messages.json +++ b/apps/browser/src/_locales/en/messages.json @@ -1457,6 +1457,15 @@ "attachmentSaved": { "message": "Attachment saved" }, + "fixEncryption": { + "message": "Fix encryption" + }, + "fixEncryptionTooltip": { + "message": "This file is using an outdated encryption method." + }, + "attachmentUpdated": { + "message": "Attachment updated" + }, "file": { "message": "File" }, diff --git a/apps/browser/src/vault/popup/components/vault-v2/attachments/attachments-v2.component.spec.ts b/apps/browser/src/vault/popup/components/vault-v2/attachments/attachments-v2.component.spec.ts index 871163ac80b..1da2d352c14 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/attachments/attachments-v2.component.spec.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/attachments/attachments-v2.component.spec.ts @@ -1,4 +1,4 @@ -import { Component, Input } from "@angular/core"; +import { Component, input, ChangeDetectionStrategy } from "@angular/core"; import { ComponentFixture, TestBed, fakeAsync, tick } from "@angular/core/testing"; import { By } from "@angular/platform-browser"; import { ActivatedRoute, Router } from "@angular/router"; @@ -25,31 +25,23 @@ import { PopupRouterCacheService } from "../../../../../platform/popup/view-cach import { AttachmentsV2Component } from "./attachments-v2.component"; -// FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush -// eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection @Component({ selector: "popup-header", template: ``, + changeDetection: ChangeDetectionStrategy.OnPush, }) class MockPopupHeaderComponent { - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-signals - @Input() pageTitle: string; - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-signals - @Input() backAction: () => void; + readonly pageTitle = input(); + readonly backAction = input<() => void>(); } -// FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush -// eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection @Component({ selector: "popup-footer", template: ``, + changeDetection: ChangeDetectionStrategy.OnPush, }) class MockPopupFooterComponent { - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-signals - @Input() pageTitle: string; + readonly pageTitle = input(); } describe("AttachmentsV2Component", () => { @@ -120,7 +112,7 @@ describe("AttachmentsV2Component", () => { const submitBtn = fixture.debugElement.queryAll(By.directive(ButtonComponent))[1] .componentInstance; - expect(cipherAttachment.submitBtn).toEqual(submitBtn); + expect(cipherAttachment.submitBtn()).toEqual(submitBtn); }); it("navigates the user to the edit view `onUploadSuccess`", fakeAsync(() => { diff --git a/apps/desktop/src/locales/en/messages.json b/apps/desktop/src/locales/en/messages.json index 33582c857aa..92e350fab90 100644 --- a/apps/desktop/src/locales/en/messages.json +++ b/apps/desktop/src/locales/en/messages.json @@ -708,6 +708,15 @@ "addAttachment": { "message": "Add attachment" }, + "fixEncryption": { + "message": "Fix encryption" + }, + "fixEncryptionTooltip": { + "message": "This file is using an outdated encryption method." + }, + "attachmentUpdated": { + "message": "Attachment updated" + }, "maxFileSizeSansPunctuation": { "message": "Maximum file size is 500 MB" }, diff --git a/apps/web/src/locales/en/messages.json b/apps/web/src/locales/en/messages.json index 3d5303c8e82..4be70b102d1 100644 --- a/apps/web/src/locales/en/messages.json +++ b/apps/web/src/locales/en/messages.json @@ -5173,6 +5173,15 @@ "message": "Fix", "description": "This is a verb. ex. 'Fix The Car'" }, + "fixEncryption": { + "message": "Fix encryption" + }, + "fixEncryptionTooltip": { + "message": "This file is using an outdated encryption method." + }, + "attachmentUpdated": { + "message": "Attachment updated" + }, "oldAttachmentsNeedFixDesc": { "message": "There are old file attachments in your vault that need to be fixed before you can rotate your account's encryption key." }, diff --git a/libs/common/src/vault/abstractions/cipher.service.ts b/libs/common/src/vault/abstractions/cipher.service.ts index b95d9023a7c..8472a359c51 100644 --- a/libs/common/src/vault/abstractions/cipher.service.ts +++ b/libs/common/src/vault/abstractions/cipher.service.ts @@ -161,6 +161,17 @@ export abstract class CipherService implements UserKeyRotationDataProvider; + /** + * Upgrade all old attachments for a cipher by downloading, decrypting, re-uploading with new key, and deleting old. + * @param cipher - The cipher with old attachments to upgrade + * @param userId - The user ID + * @param attachmentId - If provided, only upgrade the attachment with this ID + */ + abstract upgradeOldCipherAttachments( + cipher: CipherView, + userId: UserId, + attachmentId?: string, + ): Promise; /** * Save the collections for a cipher with the server * @@ -274,7 +285,7 @@ export abstract class CipherService implements UserKeyRotationDataProvider; + ): Promise; /** * Decrypts the full `CipherView` for a given `CipherViewLike`. diff --git a/libs/common/src/vault/services/cipher.service.ts b/libs/common/src/vault/services/cipher.service.ts index 7eebe960a7f..402b8ed1030 100644 --- a/libs/common/src/vault/services/cipher.service.ts +++ b/libs/common/src/vault/services/cipher.service.ts @@ -1656,12 +1656,14 @@ export class CipherService implements CipherServiceAbstraction { const key = attachment.key != null ? attachment.key - : await firstValueFrom( - this.keyService.orgKeys$(userId).pipe( - filterOutNullish(), - map((orgKeys) => orgKeys[cipherDomain.organizationId as OrganizationId] as OrgKey), - ), - ); + : cipherDomain.organizationId + ? await firstValueFrom( + this.keyService.orgKeys$(userId).pipe( + filterOutNullish(), + map((orgKeys) => orgKeys[cipherDomain.organizationId as OrganizationId] as OrgKey), + ), + ) + : await firstValueFrom(this.keyService.userKey$(userId).pipe(filterOutNullish())); return await this.encryptService.decryptFileData(encBuf, key); } @@ -1829,6 +1831,95 @@ export class CipherService implements CipherServiceAbstraction { } } + /** + * Upgrade all old attachments for a cipher by downloading, decrypting, re-uploading with new key, and deleting old. + * @param cipher + * @param userId + * @param attachmentId Optional specific attachment ID to upgrade. If not provided, all old attachments will be upgraded. + */ + async upgradeOldCipherAttachments( + cipher: CipherView, + userId: UserId, + attachmentId?: string, + ): Promise { + if (!cipher.hasOldAttachments) { + return cipher; + } + + let cipherDomain = await this.get(cipher.id, userId); + + for (const attachmentView of cipher.attachments) { + if ( + attachmentView.key != null || + (attachmentId != null && attachmentView.id !== attachmentId) + ) { + continue; + } + + try { + // 1. Get download URL + const downloadUrl = await this.getAttachmentDownloadUrl(cipher.id, attachmentView); + + // 2. Download attachment data + const dataResponse = await this.apiService.nativeFetch( + new Request(downloadUrl, { cache: "no-store" }), + ); + + if (dataResponse.status !== 200) { + throw new Error(`Failed to download attachment. Status: ${dataResponse.status}`); + } + + // 3. Decrypt the attachment + const decryptedBuffer = await this.getDecryptedAttachmentBuffer( + cipher.id as CipherId, + attachmentView, + dataResponse, + userId, + ); + + // 4. Re-upload with attachment key + cipherDomain = await this.saveAttachmentRawWithServer( + cipherDomain, + attachmentView.fileName, + decryptedBuffer, + userId, + ); + + // 5. Delete the old attachment + const cipherData = await this.deleteAttachmentWithServer( + cipher.id, + attachmentView.id, + userId, + ); + cipherDomain = new Cipher(cipherData); + } catch (e) { + this.logService.error(`Failed to upgrade attachment ${attachmentView.id}`, e); + throw e; + } + } + + return await this.decrypt(cipherDomain, userId); + } + + private async getAttachmentDownloadUrl( + cipherId: string, + attachmentView: AttachmentView, + ): Promise { + try { + const attachmentResponse = await this.apiService.getAttachmentData( + cipherId, + attachmentView.id, + ); + return attachmentResponse.url; + } catch (e) { + // Fall back to the attachment's stored URL + if (e instanceof ErrorResponse && e.statusCode === 404 && attachmentView.url) { + return attachmentView.url; + } + throw new Error(`Failed to get download URL for attachment ${attachmentView.id}`); + } + } + private async encryptObjProperty( model: V, obj: D, diff --git a/libs/vault/src/cipher-form/components/attachments/cipher-attachments.component.html b/libs/vault/src/cipher-form/components/attachments/cipher-attachments.component.html index 83e5956a067..855c37ecab5 100644 --- a/libs/vault/src/cipher-form/components/attachments/cipher-attachments.component.html +++ b/libs/vault/src/cipher-form/components/attachments/cipher-attachments.component.html @@ -1,32 +1,57 @@

{{ "attachments" | i18n }}

-
    -
  • - - - {{ attachment.fileName }} - {{ attachment.sizeName }} - - - - - - - - - - -
    • -
+@if (cipher()?.attachments; as attachments) { +
    + @for (attachment of attachments; track attachment.id) { +
  • + + + {{ + attachment.fileName + }} + {{ attachment.sizeName }} + + + + + + @if (attachment.key != null) { + + } @else { + + } + + + + + + +
    • + } +
+}
diff --git a/libs/vault/src/cipher-form/components/attachments/cipher-attachments.component.spec.ts b/libs/vault/src/cipher-form/components/attachments/cipher-attachments.component.spec.ts index 06f62976548..2e54d3b539a 100644 --- a/libs/vault/src/cipher-form/components/attachments/cipher-attachments.component.spec.ts +++ b/libs/vault/src/cipher-form/components/attachments/cipher-attachments.component.spec.ts @@ -1,7 +1,8 @@ -import { Component, Input } from "@angular/core"; +import { ChangeDetectionStrategy, Component, input } from "@angular/core"; import { ComponentFixture, TestBed } from "@angular/core/testing"; import { By } from "@angular/platform-browser"; import { mock } from "jest-mock-extended"; +import { BehaviorSubject } from "rxjs"; import { ApiService } from "@bitwarden/common/abstractions/api.service"; import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; @@ -13,7 +14,7 @@ import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.servic import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { Utils } from "@bitwarden/common/platform/misc/utils"; -import { CipherId, UserId } from "@bitwarden/common/types/guid"; +import { CipherId, OrganizationId, UserId } from "@bitwarden/common/types/guid"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; import { CipherType } from "@bitwarden/common/vault/enums"; import { AttachmentView } from "@bitwarden/common/vault/models/view/attachment.view"; @@ -26,27 +27,21 @@ import { FakeAccountService, mockAccountServiceWith } from "../../../../../commo import { CipherAttachmentsComponent } from "./cipher-attachments.component"; import { DeleteAttachmentComponent } from "./delete-attachment/delete-attachment.component"; -// FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush -// eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection @Component({ selector: "app-download-attachment", template: "", + changeDetection: ChangeDetectionStrategy.OnPush, }) class MockDownloadAttachmentComponent { - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-signals - @Input() attachment: AttachmentView; - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-signals - @Input() cipher: CipherView; - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-signals - @Input() admin: boolean = false; + readonly attachment = input(); + readonly cipher = input(); + readonly admin = input(false); } describe("CipherAttachmentsComponent", () => { let component: CipherAttachmentsComponent; let fixture: ComponentFixture; + let submitBtnFixture: ComponentFixture; const showToast = jest.fn(); const cipherView = { id: "5555-444-3333", @@ -63,17 +58,21 @@ describe("CipherAttachmentsComponent", () => { }; const organization = new Organization(); + organization.id = "org-123" as OrganizationId; organization.type = OrganizationUserType.Admin; organization.allowAdminAccessToAllCollectionItems = true; const cipherServiceGet = jest.fn().mockResolvedValue(cipherDomain); + const cipherServiceDecrypt = jest.fn().mockResolvedValue(cipherView); const saveAttachmentWithServer = jest.fn().mockResolvedValue(cipherDomain); const mockUserId = Utils.newGuid() as UserId; const accountService: FakeAccountService = mockAccountServiceWith(mockUserId); + const organizations$ = new BehaviorSubject([organization]); beforeEach(async () => { cipherServiceGet.mockClear(); + cipherServiceDecrypt.mockClear().mockResolvedValue(cipherView); showToast.mockClear(); saveAttachmentWithServer.mockClear().mockResolvedValue(cipherDomain); @@ -87,7 +86,7 @@ describe("CipherAttachmentsComponent", () => { get: cipherServiceGet, saveAttachmentWithServer, getKeyForCipherKeyDecryption: () => Promise.resolve(null), - decrypt: jest.fn().mockResolvedValue(cipherView), + decrypt: cipherServiceDecrypt, }, }, { @@ -110,7 +109,9 @@ describe("CipherAttachmentsComponent", () => { }, { provide: OrganizationService, - useValue: mock(), + useValue: { + organizations$: () => organizations$.asObservable(), + }, }, ], }) @@ -128,70 +129,67 @@ describe("CipherAttachmentsComponent", () => { beforeEach(() => { fixture = TestBed.createComponent(CipherAttachmentsComponent); component = fixture.componentInstance; - component.cipherId = "5555-444-3333" as CipherId; - component.submitBtn = TestBed.createComponent(ButtonComponent).componentInstance; + submitBtnFixture = TestBed.createComponent(ButtonComponent); + + fixture.componentRef.setInput("cipherId", "5555-444-3333" as CipherId); + fixture.componentRef.setInput("submitBtn", submitBtnFixture.componentInstance); fixture.detectChanges(); }); + /** + * Helper to wait for the async initialization effect to complete + */ + async function waitForInitialization(): Promise { + await fixture.whenStable(); + fixture.detectChanges(); + } + it("fetches cipherView using `cipherId`", async () => { - await component.ngOnInit(); + await waitForInitialization(); expect(cipherServiceGet).toHaveBeenCalledWith("5555-444-3333", mockUserId); - expect(component.cipher).toEqual(cipherView); }); - it("sets testids for automation testing", () => { + it("sets testids for automation testing", async () => { const attachment = { id: "1234-5678", fileName: "test file.txt", sizeName: "244.2 KB", } as AttachmentView; - component.cipher.attachments = [attachment]; + const cipherWithAttachments = { ...cipherView, attachments: [attachment] }; + cipherServiceDecrypt.mockResolvedValue(cipherWithAttachments); + // Create fresh fixture to pick up the mock + fixture = TestBed.createComponent(CipherAttachmentsComponent); + component = fixture.componentInstance; + fixture.componentRef.setInput("cipherId", "5555-444-3333" as CipherId); fixture.detectChanges(); + await waitForInitialization(); + const fileName = fixture.debugElement.query(By.css('[data-testid="file-name"]')); const fileSize = fixture.debugElement.query(By.css('[data-testid="file-size"]')); - expect(fileName.nativeElement.textContent).toEqual(attachment.fileName); + expect(fileName.nativeElement.textContent.trim()).toEqual(attachment.fileName); expect(fileSize.nativeElement.textContent).toEqual(attachment.sizeName); }); describe("bitSubmit", () => { - beforeEach(() => { - component.submitBtn.disabled.set(undefined); - component.submitBtn.loading.set(undefined); - }); - it("updates sets initial state of the submit button", async () => { - await component.ngOnInit(); + // Create fresh fixture to properly test initial state + submitBtnFixture = TestBed.createComponent(ButtonComponent); + submitBtnFixture.componentInstance.disabled.set(undefined as unknown as boolean); - expect(component.submitBtn.disabled()).toBe(true); - }); + fixture = TestBed.createComponent(CipherAttachmentsComponent); + component = fixture.componentInstance; + fixture.componentRef.setInput("submitBtn", submitBtnFixture.componentInstance); + fixture.componentRef.setInput("cipherId", "5555-444-3333" as CipherId); + fixture.detectChanges(); - it("sets submitBtn loading state", () => { - jest.useFakeTimers(); + await waitForInitialization(); - component.bitSubmit.loading = true; - - jest.runAllTimers(); - - expect(component.submitBtn.loading()).toBe(true); - - component.bitSubmit.loading = false; - - expect(component.submitBtn.loading()).toBe(false); - }); - - it("sets submitBtn disabled state", () => { - component.bitSubmit.disabled = true; - - expect(component.submitBtn.disabled()).toBe(true); - - component.bitSubmit.disabled = false; - - expect(component.submitBtn.disabled()).toBe(false); + expect(submitBtnFixture.componentInstance.disabled()).toBe(true); }); }); @@ -199,7 +197,7 @@ describe("CipherAttachmentsComponent", () => { let file: File; beforeEach(() => { - component.submitBtn.disabled.set(undefined); + submitBtnFixture.componentInstance.disabled.set(undefined as unknown as boolean); file = new File([""], "attachment.txt", { type: "text/plain" }); const inputElement = fixture.debugElement.query(By.css("input[type=file]")); @@ -215,11 +213,11 @@ describe("CipherAttachmentsComponent", () => { }); it("sets value of `file` control when input changes", () => { - expect(component.attachmentForm.controls.file.value.name).toEqual(file.name); + expect(component.attachmentForm.controls.file.value?.name).toEqual(file.name); }); it("updates disabled state of submit button", () => { - expect(component.submitBtn.disabled()).toBe(false); + expect(submitBtnFixture.componentInstance.disabled()).toBe(false); }); }); @@ -250,6 +248,8 @@ describe("CipherAttachmentsComponent", () => { }); it("shows error toast with server message when saveAttachmentWithServer fails", async () => { + await waitForInitialization(); + const file = { size: 100 } as File; component.attachmentForm.controls.file.setValue(file); @@ -265,6 +265,8 @@ describe("CipherAttachmentsComponent", () => { }); it("shows error toast with fallback message when error has no message property", async () => { + await waitForInitialization(); + const file = { size: 100 } as File; component.attachmentForm.controls.file.setValue(file); @@ -279,6 +281,8 @@ describe("CipherAttachmentsComponent", () => { }); it("shows error toast with string error message", async () => { + await waitForInitialization(); + const file = { size: 100 } as File; component.attachmentForm.controls.file.setValue(file); @@ -296,13 +300,27 @@ describe("CipherAttachmentsComponent", () => { describe("success", () => { const file = { size: 524287999 } as File; - beforeEach(() => { + async function setupWithOrganization(adminAccess: boolean): Promise { + // Create fresh fixture with organization set before cipherId + organization.allowAdminAccessToAllCollectionItems = adminAccess; + + fixture = TestBed.createComponent(CipherAttachmentsComponent); + component = fixture.componentInstance; + submitBtnFixture = TestBed.createComponent(ButtonComponent); + + // Set organizationId BEFORE cipherId so the effect picks it up + fixture.componentRef.setInput("organizationId", organization.id); + fixture.componentRef.setInput("submitBtn", submitBtnFixture.componentInstance); + fixture.componentRef.setInput("cipherId", "5555-444-3333" as CipherId); + fixture.detectChanges(); + + await waitForInitialization(); component.attachmentForm.controls.file.setValue(file); - component.organization = organization; - }); + } it("calls `saveAttachmentWithServer` with admin=false when admin permission is false for organization", async () => { - component.organization.allowAdminAccessToAllCollectionItems = false; + await setupWithOrganization(false); + await component.submit(); expect(saveAttachmentWithServer).toHaveBeenCalledWith( @@ -314,13 +332,16 @@ describe("CipherAttachmentsComponent", () => { }); it("calls `saveAttachmentWithServer` with admin=true when using admin API", async () => { - component.organization.allowAdminAccessToAllCollectionItems = true; + await setupWithOrganization(true); + await component.submit(); expect(saveAttachmentWithServer).toHaveBeenCalledWith(cipherDomain, file, mockUserId, true); }); it("resets form and input values", async () => { + await setupWithOrganization(true); + await component.submit(); const fileInput = fixture.debugElement.query(By.css("input[type=file]")); @@ -330,16 +351,19 @@ describe("CipherAttachmentsComponent", () => { }); it("shows success toast", async () => { + await setupWithOrganization(true); + await component.submit(); expect(showToast).toHaveBeenCalledWith({ variant: "success", - title: null, message: "attachmentSaved", }); }); it('emits "onUploadSuccess"', async () => { + await setupWithOrganization(true); + const emitSpy = jest.spyOn(component.onUploadSuccess, "emit"); await component.submit(); @@ -350,22 +374,36 @@ describe("CipherAttachmentsComponent", () => { }); describe("removeAttachment", () => { - const attachment = { id: "1234-5678" } as AttachmentView; + const attachment = { id: "1234-5678", fileName: "test.txt" } as AttachmentView; - beforeEach(() => { - component.cipher.attachments = [attachment]; + it("removes attachment from cipher", async () => { + // Create a new fixture with cipher that has attachments + const cipherWithAttachments = { ...cipherView, attachments: [attachment] }; + cipherServiceDecrypt.mockResolvedValue(cipherWithAttachments); + // Create fresh fixture + fixture = TestBed.createComponent(CipherAttachmentsComponent); + component = fixture.componentInstance; + fixture.componentRef.setInput("cipherId", "5555-444-3333" as CipherId); fixture.detectChanges(); - }); - it("removes attachment from cipher", () => { + await waitForInitialization(); + + // Verify attachment is rendered + const attachmentsBefore = fixture.debugElement.queryAll(By.css('[data-testid="file-name"]')); + expect(attachmentsBefore.length).toEqual(1); + const deleteAttachmentComponent = fixture.debugElement.query( By.directive(DeleteAttachmentComponent), ).componentInstance as DeleteAttachmentComponent; deleteAttachmentComponent.onDeletionSuccess.emit(); - expect(component.cipher.attachments).toEqual([]); + fixture.detectChanges(); + + // After removal, there should be no attachments displayed + const attachmentItems = fixture.debugElement.queryAll(By.css('[data-testid="file-name"]')); + expect(attachmentItems.length).toEqual(0); }); }); }); diff --git a/libs/vault/src/cipher-form/components/attachments/cipher-attachments.component.ts b/libs/vault/src/cipher-form/components/attachments/cipher-attachments.component.ts index a5306606199..f75611b995e 100644 --- a/libs/vault/src/cipher-form/components/attachments/cipher-attachments.component.ts +++ b/libs/vault/src/cipher-form/components/attachments/cipher-attachments.component.ts @@ -1,17 +1,15 @@ -// FIXME: Update this file to be type safe and remove this and next line -// @ts-strict-ignore import { CommonModule } from "@angular/common"; import { - AfterViewInit, + ChangeDetectionStrategy, Component, DestroyRef, ElementRef, - EventEmitter, - Input, - OnInit, - Output, - ViewChild, + effect, inject, + input, + output, + signal, + viewChild, } from "@angular/core"; import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; import { @@ -56,11 +54,10 @@ type CipherAttachmentForm = FormGroup<{ file: FormControl; }>; -// FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush -// eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection @Component({ selector: "app-cipher-attachments", templateUrl: "./cipher-attachments.component.html", + changeDetection: ChangeDetectionStrategy.OnPush, imports: [ AsyncActionsModule, ButtonModule, @@ -74,70 +71,50 @@ type CipherAttachmentForm = FormGroup<{ DownloadAttachmentComponent, ], }) -export class CipherAttachmentsComponent implements OnInit, AfterViewInit { +export class CipherAttachmentsComponent { /** `id` associated with the form element */ static attachmentFormID = "attachmentForm"; /** Reference to the file HTMLInputElement */ - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-signals - @ViewChild("fileInput", { read: ElementRef }) private fileInput: ElementRef; + private readonly fileInput = viewChild("fileInput", { read: ElementRef }); /** Reference to the BitSubmitDirective */ - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-signals - @ViewChild(BitSubmitDirective) bitSubmit: BitSubmitDirective; + readonly bitSubmit = viewChild(BitSubmitDirective); /** The `id` of the cipher in context */ - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-signals - @Input({ required: true }) cipherId: CipherId; + readonly cipherId = input.required(); /** The organization ID if this cipher belongs to an organization */ - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-signals - @Input() organizationId?: OrganizationId; + readonly organizationId = input(); /** Denotes if the action is occurring from within the admin console */ - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-signals - @Input() admin: boolean = false; + readonly admin = input(false); /** An optional submit button, whose loading/disabled state will be tied to the form state. */ - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-signals - @Input() submitBtn?: ButtonComponent; + readonly submitBtn = input(); /** Emits when a file upload is started */ - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-output-emitter-ref - @Output() onUploadStarted = new EventEmitter(); + readonly onUploadStarted = output(); /** Emits after a file has been successfully uploaded */ - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-output-emitter-ref - @Output() onUploadSuccess = new EventEmitter(); + readonly onUploadSuccess = output(); /** Emits when a file upload fails */ - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-output-emitter-ref - @Output() onUploadFailed = new EventEmitter(); + readonly onUploadFailed = output(); /** Emits after a file has been successfully removed */ - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-output-emitter-ref - @Output() onRemoveSuccess = new EventEmitter(); + readonly onRemoveSuccess = output(); - organization: Organization; - cipher: CipherView; + protected readonly organization = signal(null); + protected readonly cipher = signal(null); attachmentForm: CipherAttachmentForm = this.formBuilder.group({ - file: new FormControl(null, [Validators.required]), + file: new FormControl(null, [Validators.required]), }); - private cipherDomain: Cipher; - private activeUserId: UserId; - private destroy$ = inject(DestroyRef); + private cipherDomain: Cipher | null = null; + private activeUserId: UserId | null = null; + private readonly destroyRef = inject(DestroyRef); constructor( private cipherService: CipherService, @@ -150,43 +127,52 @@ export class CipherAttachmentsComponent implements OnInit, AfterViewInit { private organizationService: OrganizationService, ) { this.attachmentForm.statusChanges.pipe(takeUntilDestroyed()).subscribe((status) => { - if (!this.submitBtn) { + const btn = this.submitBtn(); + if (!btn) { return; } - this.submitBtn.disabled.set(status !== "VALID"); - }); - } - - async ngOnInit(): Promise { - this.activeUserId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId)); - // Get the organization to check admin permissions - this.organization = await this.getOrganization(); - this.cipherDomain = await this.getCipher(this.cipherId); - - this.cipher = await this.cipherService.decrypt(this.cipherDomain, this.activeUserId); - - // Update the initial state of the submit button - if (this.submitBtn) { - this.submitBtn.disabled.set(!this.attachmentForm.valid); - } - } - - ngAfterViewInit(): void { - this.bitSubmit.loading$.pipe(takeUntilDestroyed(this.destroy$)).subscribe((loading) => { - if (!this.submitBtn) { - return; - } - - this.submitBtn.loading.set(loading); + btn.disabled.set(status !== "VALID"); }); - this.bitSubmit.disabled$.pipe(takeUntilDestroyed(this.destroy$)).subscribe((disabled) => { - if (!this.submitBtn) { + // Initialize data when cipherId input is available + effect(async () => { + const cipherId = this.cipherId(); + if (!cipherId) { return; } - this.submitBtn.disabled.set(disabled); + this.activeUserId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId)); + // Get the organization to check admin permissions + this.organization.set(await this.getOrganization()); + this.cipherDomain = await this.getCipher(cipherId); + + if (this.cipherDomain && this.activeUserId) { + this.cipher.set(await this.cipherService.decrypt(this.cipherDomain, this.activeUserId)); + } + + // Update the initial state of the submit button + const btn = this.submitBtn(); + if (btn) { + btn.disabled.set(!this.attachmentForm.valid); + } + }); + + // Sync bitSubmit loading/disabled state with submitBtn + effect(() => { + const bitSubmit = this.bitSubmit(); + const btn = this.submitBtn(); + if (!bitSubmit || !btn) { + return; + } + + bitSubmit.loading$.pipe(takeUntilDestroyed(this.destroyRef)).subscribe((loading) => { + btn.loading.set(loading); + }); + + bitSubmit.disabled$.pipe(takeUntilDestroyed(this.destroyRef)).subscribe((disabled) => { + btn.disabled.set(disabled); + }); }); } @@ -209,7 +195,7 @@ export class CipherAttachmentsComponent implements OnInit, AfterViewInit { this.onUploadStarted.emit(); const file = this.attachmentForm.value.file; - if (file === null) { + if (file == null) { this.toastService.showToast({ variant: "error", title: this.i18nService.t("errorOccurred"), @@ -228,24 +214,30 @@ export class CipherAttachmentsComponent implements OnInit, AfterViewInit { return; } + if (!this.cipherDomain || !this.activeUserId) { + return; + } + try { this.cipherDomain = await this.cipherService.saveAttachmentWithServer( this.cipherDomain, file, this.activeUserId, - this.organization?.canEditAllCiphers, + this.organization()?.canEditAllCiphers, ); // re-decrypt the cipher to update the attachments - this.cipher = await this.cipherService.decrypt(this.cipherDomain, this.activeUserId); + this.cipher.set(await this.cipherService.decrypt(this.cipherDomain, this.activeUserId)); // Reset reactive form and input element - this.fileInput.nativeElement.value = ""; + const fileInputEl = this.fileInput(); + if (fileInputEl) { + fileInputEl.nativeElement.value = ""; + } this.attachmentForm.controls.file.setValue(null); this.toastService.showToast({ variant: "success", - title: null, message: this.i18nService.t("attachmentSaved"), }); @@ -257,7 +249,7 @@ export class CipherAttachmentsComponent implements OnInit, AfterViewInit { let errorMessage = this.i18nService.t("unexpectedError"); if (typeof e === "string") { errorMessage = e; - } else if (e?.message) { + } else if (e instanceof Error && e?.message) { errorMessage = e.message; } @@ -271,10 +263,19 @@ export class CipherAttachmentsComponent implements OnInit, AfterViewInit { /** Removes the attachment from the cipher */ removeAttachment(attachment: AttachmentView) { - const index = this.cipher.attachments.indexOf(attachment); + const currentCipher = this.cipher(); + if (!currentCipher?.attachments) { + return; + } + + const index = currentCipher.attachments.indexOf(attachment); if (index > -1) { - this.cipher.attachments.splice(index, 1); + currentCipher.attachments.splice(index, 1); + // Trigger signal update by creating a new reference + this.cipher.set( + Object.assign(Object.create(Object.getPrototypeOf(currentCipher)), currentCipher), + ); } this.onRemoveSuccess.emit(); @@ -286,7 +287,7 @@ export class CipherAttachmentsComponent implements OnInit, AfterViewInit { * it will retrieve the cipher using the admin endpoint. */ private async getCipher(id: CipherId): Promise { - if (id == null) { + if (id == null || !this.activeUserId) { return null; } @@ -294,12 +295,13 @@ export class CipherAttachmentsComponent implements OnInit, AfterViewInit { const localCipher = await this.cipherService.get(id, this.activeUserId); // If we got the cipher or there's no organization context, return the result - if (localCipher != null || !this.organizationId) { + if (localCipher != null || !this.organizationId()) { return localCipher; } // Only try the admin API if the user has admin permissions - if (this.organization != null && this.organization.canEditAllCiphers) { + const org = this.organization(); + if (org != null && org.canEditAllCiphers) { const cipherResponse = await this.apiService.getCipherAdmin(id); const cipherData = new CipherData(cipherResponse); return new Cipher(cipherData); @@ -312,7 +314,8 @@ export class CipherAttachmentsComponent implements OnInit, AfterViewInit { * Gets the organization for the given organization ID */ private async getOrganization(): Promise { - if (!this.organizationId) { + const orgId = this.organizationId(); + if (!orgId || !this.activeUserId) { return null; } @@ -320,6 +323,41 @@ export class CipherAttachmentsComponent implements OnInit, AfterViewInit { this.organizationService.organizations$(this.activeUserId), ); - return organizations.find((o) => o.id === this.organizationId) || null; + return organizations.find((o) => o.id === orgId) || null; } + + protected fixOldAttachment = (attachment: AttachmentView) => { + return async () => { + const cipher = this.cipher(); + const userId = this.activeUserId; + + if (!attachment.id || !userId || !cipher) { + this.toastService.showToast({ + variant: "error", + message: this.i18nService.t("errorOccurred"), + }); + return; + } + + try { + const updatedCipher = await this.cipherService.upgradeOldCipherAttachments( + cipher, + userId, + attachment.id, + ); + + this.cipher.set(updatedCipher); + this.toastService.showToast({ + variant: "success", + message: this.i18nService.t("attachmentUpdated"), + }); + this.onUploadSuccess.emit(); + } catch { + this.toastService.showToast({ + variant: "error", + message: this.i18nService.t("errorOccurred"), + }); + } + }; + }; } diff --git a/libs/vault/src/components/download-attachment/download-attachment.component.html b/libs/vault/src/components/download-attachment/download-attachment.component.html index c2c2f1d4ebd..9d80f36818a 100644 --- a/libs/vault/src/components/download-attachment/download-attachment.component.html +++ b/libs/vault/src/components/download-attachment/download-attachment.component.html @@ -1,9 +1,10 @@ - +@if (!isDecryptionFailure()) { + +} diff --git a/libs/vault/src/components/download-attachment/download-attachment.component.spec.ts b/libs/vault/src/components/download-attachment/download-attachment.component.spec.ts index ec5a9ce96fd..3bbc375fdfc 100644 --- a/libs/vault/src/components/download-attachment/download-attachment.component.spec.ts +++ b/libs/vault/src/components/download-attachment/download-attachment.component.spec.ts @@ -100,8 +100,8 @@ describe("DownloadAttachmentComponent", () => { beforeEach(() => { fixture = TestBed.createComponent(DownloadAttachmentComponent); component = fixture.componentInstance; - component.attachment = attachment; - component.cipher = cipherView; + fixture.componentRef.setInput("attachment", attachment); + fixture.componentRef.setInput("cipher", cipherView); fixture.detectChanges(); }); @@ -123,7 +123,8 @@ describe("DownloadAttachmentComponent", () => { }); it("hides download button when the attachment has decryption failure", () => { - component.attachment.fileName = DECRYPT_ERROR; + const decryptFailureAttachment = { ...attachment, fileName: DECRYPT_ERROR }; + fixture.componentRef.setInput("attachment", decryptFailureAttachment); fixture.detectChanges(); expect(fixture.debugElement.query(By.css("button"))).toBeNull(); @@ -156,7 +157,6 @@ describe("DownloadAttachmentComponent", () => { expect(showToast).toHaveBeenCalledWith({ message: "errorOccurred", - title: null, variant: "error", }); }); @@ -172,7 +172,6 @@ describe("DownloadAttachmentComponent", () => { expect(showToast).toHaveBeenCalledWith({ message: "errorOccurred", - title: null, variant: "error", }); }); diff --git a/libs/vault/src/components/download-attachment/download-attachment.component.ts b/libs/vault/src/components/download-attachment/download-attachment.component.ts index 2f9cd528990..31ed609637c 100644 --- a/libs/vault/src/components/download-attachment/download-attachment.component.ts +++ b/libs/vault/src/components/download-attachment/download-attachment.component.ts @@ -1,7 +1,5 @@ -// FIXME: Update this file to be type safe and remove this and next line -// @ts-strict-ignore import { CommonModule } from "@angular/common"; -import { Component, Input } from "@angular/core"; +import { ChangeDetectionStrategy, Component, computed, input } from "@angular/core"; import { firstValueFrom } from "rxjs"; import { JslibModule } from "@bitwarden/angular/jslib.module"; @@ -17,38 +15,27 @@ import { AttachmentView } from "@bitwarden/common/vault/models/view/attachment.v import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; import { AsyncActionsModule, IconButtonModule, ToastService } from "@bitwarden/components"; -// FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush -// eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection @Component({ selector: "app-download-attachment", templateUrl: "./download-attachment.component.html", imports: [AsyncActionsModule, CommonModule, JslibModule, IconButtonModule], + changeDetection: ChangeDetectionStrategy.OnPush, }) export class DownloadAttachmentComponent { /** Attachment to download */ - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-signals - @Input({ required: true }) attachment: AttachmentView; + readonly attachment = input.required(); /** The cipher associated with the attachment */ - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-signals - @Input({ required: true }) cipher: CipherView; + readonly cipher = input.required(); - // When in view mode, we will want to check for the master password reprompt - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-signals - @Input() checkPwReprompt?: boolean = false; + /** When in view mode, we will want to check for the master password reprompt */ + readonly checkPwReprompt = input(false); - // Required for fetching attachment data when viewed from cipher via emergency access - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-signals - @Input() emergencyAccessId?: EmergencyAccessId; + /** Required for fetching attachment data when viewed from cipher via emergency access */ + readonly emergencyAccessId = input(); - /** When owners/admins can mange all items and when accessing from the admin console, use the admin endpoint */ - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-signals - @Input() admin?: boolean = false; + /** When owners/admins can manage all items and when accessing from the admin console, use the admin endpoint */ + readonly admin = input(false); constructor( private i18nService: I18nService, @@ -59,26 +46,36 @@ export class DownloadAttachmentComponent { private cipherService: CipherService, ) {} - protected get isDecryptionFailure(): boolean { - return this.attachment.fileName === DECRYPT_ERROR; - } + protected readonly isDecryptionFailure = computed( + () => this.attachment().fileName === DECRYPT_ERROR, + ); /** Download the attachment */ download = async () => { - let url: string; + const attachment = this.attachment(); + const cipher = this.cipher(); + let url: string | undefined; + + if (!attachment.id) { + this.toastService.showToast({ + variant: "error", + message: this.i18nService.t("errorOccurred"), + }); + return; + } try { - const attachmentDownloadResponse = this.admin - ? await this.apiService.getAttachmentDataAdmin(this.cipher.id, this.attachment.id) + const attachmentDownloadResponse = this.admin() + ? await this.apiService.getAttachmentDataAdmin(cipher.id, attachment.id) : await this.apiService.getAttachmentData( - this.cipher.id, - this.attachment.id, - this.emergencyAccessId, + cipher.id, + attachment.id, + this.emergencyAccessId(), ); url = attachmentDownloadResponse.url; } catch (e) { if (e instanceof ErrorResponse && (e as ErrorResponse).statusCode === 404) { - url = this.attachment.url; + url = attachment.url; } else if (e instanceof ErrorResponse) { throw new Error((e as ErrorResponse).getSingleMessage()); } else { @@ -86,11 +83,18 @@ export class DownloadAttachmentComponent { } } + if (!url) { + this.toastService.showToast({ + variant: "error", + message: this.i18nService.t("errorOccurred"), + }); + return; + } + const response = await fetch(new Request(url, { cache: "no-store" })); if (response.status !== 200) { this.toastService.showToast({ variant: "error", - title: null, message: this.i18nService.t("errorOccurred"), }); return; @@ -99,26 +103,31 @@ export class DownloadAttachmentComponent { try { const userId = await firstValueFrom(this.stateProvider.activeUserId$); + if (!userId || !attachment.fileName) { + this.toastService.showToast({ + variant: "error", + message: this.i18nService.t("errorOccurred"), + }); + return; + } + const decBuf = await this.cipherService.getDecryptedAttachmentBuffer( - this.cipher.id as CipherId, - this.attachment, + cipher.id as CipherId, + attachment, response, userId, // When the emergency access ID is present, the cipher is being viewed via emergency access. // Force legacy decryption in these cases. - this.emergencyAccessId ? true : false, + Boolean(this.emergencyAccessId()), ); this.fileDownloadService.download({ - fileName: this.attachment.fileName, + fileName: attachment.fileName, blobData: decBuf, }); - // FIXME: Remove when updating file. Eslint update - // eslint-disable-next-line @typescript-eslint/no-unused-vars - } catch (e) { + } catch { this.toastService.showToast({ variant: "error", - title: null, message: this.i18nService.t("errorOccurred"), }); }