From 1ef8f257b0120fbef5f22e1828e2facea9a9913e Mon Sep 17 00:00:00 2001 From: Alex <55413326+AlexRubik@users.noreply.github.com> Date: Wed, 18 Feb 2026 09:00:36 -0700 Subject: [PATCH] [PM-31803] Fix Password Manager reports not displaying items with limited collection access (#18956) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When "Owners and admins can manage all collections and items" is OFF, Password Manager reports incorrectly filter out items from collections where the user has "Can view", "Can view except passwords", or "Can edit except passwords" access. The root cause is that all five PM report components filter ciphers using `(!this.organization && !edit) || !viewPassword`. Since PM reports run without an organization context (this.organization is undefined), this condition excludes any item where edit=false or viewPassword=false. These permission checks are unnecessary for PM reports because: 1. Personal vault items always have edit=true and viewPassword=true, so the checks never applied to them. 2. Organization items should appear in reports regardless of permission level — the user has collection access, and edit restrictions should only affect the item dialog, not report visibility. 3. Admin Console reports (which work correctly) skip this filtering because this.organization is always set, making the condition always false. This also explains why "Can edit except passwords" items only appeared in the Unsecured Websites report — it was the only report that didn't check !viewPassword. Removed the edit/viewPassword filter conditions from all five PM report components: - exposed-passwords-report - weak-passwords-report - reused-passwords-report - inactive-two-factor-report - unsecured-websites-report --- ...exposed-passwords-report.component.spec.ts | 15 +++++------- .../exposed-passwords-report.component.ts | 6 ++--- ...active-two-factor-report.component.spec.ts | 23 ++++++++----------- .../inactive-two-factor-report.component.ts | 6 ++--- .../reused-passwords-report.component.spec.ts | 14 +++++------ .../reused-passwords-report.component.ts | 6 ++--- ...nsecured-websites-report.component.spec.ts | 11 ++++----- .../unsecured-websites-report.component.ts | 7 +----- .../weak-passwords-report.component.spec.ts | 15 +++++------- .../pages/weak-passwords-report.component.ts | 11 ++------- 10 files changed, 41 insertions(+), 73 deletions(-) diff --git a/apps/web/src/app/dirt/reports/pages/exposed-passwords-report.component.spec.ts b/apps/web/src/app/dirt/reports/pages/exposed-passwords-report.component.spec.ts index e056ec44af5..81e4a78b491 100644 --- a/apps/web/src/app/dirt/reports/pages/exposed-passwords-report.component.spec.ts +++ b/apps/web/src/app/dirt/reports/pages/exposed-passwords-report.component.spec.ts @@ -122,19 +122,16 @@ describe("ExposedPasswordsReportComponent", () => { expect(component).toBeTruthy(); }); - it('should get only ciphers with exposed passwords that the user has "Can Edit" access to', async () => { - const expectedIdOne: any = "cbea34a8-bde4-46ad-9d19-b05001228ab2"; - const expectedIdTwo = "cbea34a8-bde4-46ad-9d19-b05001228cd3"; - + it("should get ciphers with exposed passwords regardless of edit access", async () => { jest.spyOn(auditService, "passwordLeaked").mockReturnValue(Promise.resolve(1234)); jest.spyOn(component as any, "getAllCiphers").mockReturnValue(Promise.resolve(cipherData)); await component.setCiphers(); - expect(component.ciphers.length).toEqual(2); - expect(component.ciphers[0].id).toEqual(expectedIdOne); - expect(component.ciphers[0].edit).toEqual(true); - expect(component.ciphers[1].id).toEqual(expectedIdTwo); - expect(component.ciphers[1].edit).toEqual(true); + const cipherIds = component.ciphers.map((c) => c.id); + expect(cipherIds).toContain("cbea34a8-bde4-46ad-9d19-b05001228ab1"); + expect(cipherIds).toContain("cbea34a8-bde4-46ad-9d19-b05001228ab2"); + expect(cipherIds).toContain("cbea34a8-bde4-46ad-9d19-b05001228cd3"); + expect(component.ciphers.length).toEqual(3); }); it("should call fullSync method of syncService", () => { diff --git a/apps/web/src/app/dirt/reports/pages/exposed-passwords-report.component.ts b/apps/web/src/app/dirt/reports/pages/exposed-passwords-report.component.ts index 51bdde3eda8..e39ef811d66 100644 --- a/apps/web/src/app/dirt/reports/pages/exposed-passwords-report.component.ts +++ b/apps/web/src/app/dirt/reports/pages/exposed-passwords-report.component.ts @@ -64,14 +64,12 @@ export class ExposedPasswordsReportComponent extends CipherReportComponent imple this.filterStatus = [0]; allCiphers.forEach((ciph) => { - const { type, login, isDeleted, edit, viewPassword } = ciph; + const { type, login, isDeleted } = ciph; if ( type !== CipherType.Login || login.password == null || login.password === "" || - isDeleted || - (!this.organization && !edit) || - !viewPassword + isDeleted ) { return; } diff --git a/apps/web/src/app/dirt/reports/pages/inactive-two-factor-report.component.spec.ts b/apps/web/src/app/dirt/reports/pages/inactive-two-factor-report.component.spec.ts index 12453ea3b88..07a772755f5 100644 --- a/apps/web/src/app/dirt/reports/pages/inactive-two-factor-report.component.spec.ts +++ b/apps/web/src/app/dirt/reports/pages/inactive-two-factor-report.component.spec.ts @@ -95,9 +95,7 @@ describe("InactiveTwoFactorReportComponent", () => { expect(component).toBeTruthy(); }); - it('should get only ciphers with domains in the 2fa directory that they have "Can Edit" access to', async () => { - const expectedIdOne: any = "cbea34a8-bde4-46ad-9d19-b05001228xy4"; - const expectedIdTwo: any = "cbea34a8-bde4-46ad-9d19-b05001227nm5"; + it("should get ciphers with domains in the 2fa directory regardless of edit access", async () => { component.services.set( "101domain.com", "https://help.101domain.com/account-management/account-security/enabling-disabling-two-factor-verification", @@ -110,11 +108,10 @@ describe("InactiveTwoFactorReportComponent", () => { jest.spyOn(component as any, "getAllCiphers").mockReturnValue(Promise.resolve(cipherData)); await component.setCiphers(); + const cipherIds = component.ciphers.map((c) => c.id); + expect(cipherIds).toContain("cbea34a8-bde4-46ad-9d19-b05001228xy4"); + expect(cipherIds).toContain("cbea34a8-bde4-46ad-9d19-b05001227nm5"); expect(component.ciphers.length).toEqual(2); - expect(component.ciphers[0].id).toEqual(expectedIdOne); - expect(component.ciphers[0].edit).toEqual(true); - expect(component.ciphers[1].id).toEqual(expectedIdTwo); - expect(component.ciphers[1].edit).toEqual(true); }); it("should call fullSync method of syncService", () => { @@ -197,7 +194,7 @@ describe("InactiveTwoFactorReportComponent", () => { expect(doc).toBe(""); }); - it("should return false if cipher does not have edit access and no organization", () => { + it("should return true for cipher without edit access", () => { component.organization = null; const cipher = createCipherView({ edit: false, @@ -206,11 +203,11 @@ describe("InactiveTwoFactorReportComponent", () => { }, }); const [doc, isInactive] = (component as any).isInactive2faCipher(cipher); - expect(isInactive).toBe(false); - expect(doc).toBe(""); + expect(isInactive).toBe(true); + expect(doc).toBe("https://example.com/2fa-doc"); }); - it("should return false if cipher does not have viewPassword", () => { + it("should return true for cipher without viewPassword", () => { const cipher = createCipherView({ viewPassword: false, login: { @@ -218,8 +215,8 @@ describe("InactiveTwoFactorReportComponent", () => { }, }); const [doc, isInactive] = (component as any).isInactive2faCipher(cipher); - expect(isInactive).toBe(false); - expect(doc).toBe(""); + expect(isInactive).toBe(true); + expect(doc).toBe("https://example.com/2fa-doc"); }); it("should check all uris and return true if any matches domain or host", () => { diff --git a/apps/web/src/app/dirt/reports/pages/inactive-two-factor-report.component.ts b/apps/web/src/app/dirt/reports/pages/inactive-two-factor-report.component.ts index 9d7de688f3e..cd892130518 100644 --- a/apps/web/src/app/dirt/reports/pages/inactive-two-factor-report.component.ts +++ b/apps/web/src/app/dirt/reports/pages/inactive-two-factor-report.component.ts @@ -92,14 +92,12 @@ export class InactiveTwoFactorReportComponent extends CipherReportComponent impl let docFor2fa: string = ""; let isInactive2faCipher: boolean = false; - const { type, login, isDeleted, edit, viewPassword } = cipher; + const { type, login, isDeleted } = cipher; if ( type !== CipherType.Login || (login.totp != null && login.totp !== "") || !login.hasUris || - isDeleted || - (!this.organization && !edit) || - !viewPassword + isDeleted ) { return [docFor2fa, isInactive2faCipher]; } diff --git a/apps/web/src/app/dirt/reports/pages/reused-passwords-report.component.spec.ts b/apps/web/src/app/dirt/reports/pages/reused-passwords-report.component.spec.ts index 1b7006d0c68..8f08d06e27b 100644 --- a/apps/web/src/app/dirt/reports/pages/reused-passwords-report.component.spec.ts +++ b/apps/web/src/app/dirt/reports/pages/reused-passwords-report.component.spec.ts @@ -109,17 +109,15 @@ describe("ReusedPasswordsReportComponent", () => { expect(component).toBeTruthy(); }); - it('should get ciphers with reused passwords that the user has "Can Edit" access to', async () => { - const expectedIdOne: any = "cbea34a8-bde4-46ad-9d19-b05001228ab2"; - const expectedIdTwo = "cbea34a8-bde4-46ad-9d19-b05001228cd3"; + it("should get ciphers with reused passwords regardless of edit access", async () => { jest.spyOn(component as any, "getAllCiphers").mockReturnValue(Promise.resolve(cipherData)); await component.setCiphers(); - expect(component.ciphers.length).toEqual(2); - expect(component.ciphers[0].id).toEqual(expectedIdOne); - expect(component.ciphers[0].edit).toEqual(true); - expect(component.ciphers[1].id).toEqual(expectedIdTwo); - expect(component.ciphers[1].edit).toEqual(true); + const cipherIds = component.ciphers.map((c) => c.id); + expect(cipherIds).toContain("cbea34a8-bde4-46ad-9d19-b05001228ab1"); + expect(cipherIds).toContain("cbea34a8-bde4-46ad-9d19-b05001228ab2"); + expect(cipherIds).toContain("cbea34a8-bde4-46ad-9d19-b05001228cd3"); + expect(component.ciphers.length).toEqual(3); }); it("should call fullSync method of syncService", () => { diff --git a/apps/web/src/app/dirt/reports/pages/reused-passwords-report.component.ts b/apps/web/src/app/dirt/reports/pages/reused-passwords-report.component.ts index 0a81b19d4ff..7d24e61f276 100644 --- a/apps/web/src/app/dirt/reports/pages/reused-passwords-report.component.ts +++ b/apps/web/src/app/dirt/reports/pages/reused-passwords-report.component.ts @@ -71,14 +71,12 @@ export class ReusedPasswordsReportComponent extends CipherReportComponent implem this.filterStatus = [0]; ciphers.forEach((ciph) => { - const { type, login, isDeleted, edit, viewPassword } = ciph; + const { type, login, isDeleted } = ciph; if ( type !== CipherType.Login || login.password == null || login.password === "" || - isDeleted || - (!this.organization && !edit) || - !viewPassword + isDeleted ) { return; } diff --git a/apps/web/src/app/dirt/reports/pages/unsecured-websites-report.component.spec.ts b/apps/web/src/app/dirt/reports/pages/unsecured-websites-report.component.spec.ts index 2107e0c8df7..f116faf114f 100644 --- a/apps/web/src/app/dirt/reports/pages/unsecured-websites-report.component.spec.ts +++ b/apps/web/src/app/dirt/reports/pages/unsecured-websites-report.component.spec.ts @@ -118,17 +118,14 @@ describe("UnsecuredWebsitesReportComponent", () => { expect(component).toBeTruthy(); }); - it('should get only unsecured ciphers that the user has "Can Edit" access to', async () => { - const expectedIdOne: any = "cbea34a8-bde4-46ad-9d19-b05001228ab2"; - const expectedIdTwo = "cbea34a8-bde4-46ad-9d19-b05001228cd3"; + it("should get unsecured ciphers regardless of edit access", async () => { jest.spyOn(component as any, "getAllCiphers").mockReturnValue(Promise.resolve(cipherData)); await component.setCiphers(); + const cipherIds = component.ciphers.map((c) => c.id); + expect(cipherIds).toContain("cbea34a8-bde4-46ad-9d19-b05001228ab2"); + expect(cipherIds).toContain("cbea34a8-bde4-46ad-9d19-b05001228cd3"); expect(component.ciphers.length).toEqual(2); - expect(component.ciphers[0].id).toEqual(expectedIdOne); - expect(component.ciphers[0].edit).toEqual(true); - expect(component.ciphers[1].id).toEqual(expectedIdTwo); - expect(component.ciphers[1].edit).toEqual(true); }); it("should call fullSync method of syncService", () => { diff --git a/apps/web/src/app/dirt/reports/pages/unsecured-websites-report.component.ts b/apps/web/src/app/dirt/reports/pages/unsecured-websites-report.component.ts index 4a2c0677574..8399395d273 100644 --- a/apps/web/src/app/dirt/reports/pages/unsecured-websites-report.component.ts +++ b/apps/web/src/app/dirt/reports/pages/unsecured-websites-report.component.ts @@ -71,12 +71,7 @@ export class UnsecuredWebsitesReportComponent extends CipherReportComponent impl * @param cipher Current cipher with unsecured uri */ private cipherContainsUnsecured(cipher: CipherView): boolean { - if ( - cipher.type !== CipherType.Login || - !cipher.login.hasUris || - cipher.isDeleted || - (!this.organization && !cipher.edit) - ) { + if (cipher.type !== CipherType.Login || !cipher.login.hasUris || cipher.isDeleted) { return false; } diff --git a/apps/web/src/app/dirt/reports/pages/weak-passwords-report.component.spec.ts b/apps/web/src/app/dirt/reports/pages/weak-passwords-report.component.spec.ts index a63723dc688..f9aca0aa378 100644 --- a/apps/web/src/app/dirt/reports/pages/weak-passwords-report.component.spec.ts +++ b/apps/web/src/app/dirt/reports/pages/weak-passwords-report.component.spec.ts @@ -114,10 +114,7 @@ describe("WeakPasswordsReportComponent", () => { expect(component).toBeTruthy(); }); - it('should get only ciphers with weak passwords that the user has "Can Edit" access to', async () => { - const expectedIdOne: any = "cbea34a8-bde4-46ad-9d19-b05001228ab2"; - const expectedIdTwo = "cbea34a8-bde4-46ad-9d19-b05001228cd3"; - + it("should get ciphers with weak passwords regardless of edit access", async () => { jest.spyOn(passwordStrengthService, "getPasswordStrength").mockReturnValue({ password: "123", score: 0, @@ -125,11 +122,11 @@ describe("WeakPasswordsReportComponent", () => { jest.spyOn(component as any, "getAllCiphers").mockReturnValue(Promise.resolve(cipherData)); await component.setCiphers(); - expect(component.ciphers.length).toEqual(2); - expect(component.ciphers[0].id).toEqual(expectedIdOne); - expect(component.ciphers[0].edit).toEqual(true); - expect(component.ciphers[1].id).toEqual(expectedIdTwo); - expect(component.ciphers[1].edit).toEqual(true); + const cipherIds = component.ciphers.map((c) => c.id); + expect(cipherIds).toContain("cbea34a8-bde4-46ad-9d19-b05001228ab1"); + expect(cipherIds).toContain("cbea34a8-bde4-46ad-9d19-b05001228ab2"); + expect(cipherIds).toContain("cbea34a8-bde4-46ad-9d19-b05001228cd3"); + expect(component.ciphers.length).toEqual(3); }); it("should call fullSync method of syncService", () => { diff --git a/apps/web/src/app/dirt/reports/pages/weak-passwords-report.component.ts b/apps/web/src/app/dirt/reports/pages/weak-passwords-report.component.ts index bb5400346fd..6cde01f2d92 100644 --- a/apps/web/src/app/dirt/reports/pages/weak-passwords-report.component.ts +++ b/apps/web/src/app/dirt/reports/pages/weak-passwords-report.component.ts @@ -103,15 +103,8 @@ export class WeakPasswordsReportComponent extends CipherReportComponent implemen } protected determineWeakPasswordScore(ciph: CipherView): ReportResult | null { - const { type, login, isDeleted, edit, viewPassword } = ciph; - if ( - type !== CipherType.Login || - login.password == null || - login.password === "" || - isDeleted || - (!this.organization && !edit) || - !viewPassword - ) { + const { type, login, isDeleted } = ciph; + if (type !== CipherType.Login || login.password == null || login.password === "" || isDeleted) { return; }