From fe08980a8946e5d8cc36b92bb6e07a96809fa20b Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Thu, 30 Jan 2025 01:01:01 +1000 Subject: [PATCH] [PM-17692] Fix AC navigation permissions bug (#13116) * Add failing tests * Fix bug --- .../guards/org-permissions.guard.spec.ts | 122 ++++++++++-------- .../guards/org-permissions.guard.ts | 14 +- 2 files changed, 73 insertions(+), 63 deletions(-) diff --git a/apps/web/src/app/admin-console/organizations/guards/org-permissions.guard.spec.ts b/apps/web/src/app/admin-console/organizations/guards/org-permissions.guard.spec.ts index 619b9cc4424..9afd34ca149 100644 --- a/apps/web/src/app/admin-console/organizations/guards/org-permissions.guard.spec.ts +++ b/apps/web/src/app/admin-console/organizations/guards/org-permissions.guard.spec.ts @@ -23,17 +23,27 @@ import { ToastService } from "@bitwarden/components"; import { organizationPermissionsGuard } from "./org-permissions.guard"; +// Returns a test organization with the specified props. const orgFactory = (props: Partial = {}) => Object.assign( new Organization(), { - id: "myOrgId", enabled: true, type: OrganizationUserType.Admin, }, props, ); +const targetOrgId = "myOrgId"; + +// Returns an array of test organizations with the target organization in the middle. +// This more accurately tests the return value of OrganizationService. +const orgStateFactory = (targetOrgProps: Partial = {}) => [ + orgFactory({ id: "anotherOrg" }), + orgFactory({ id: targetOrgId, ...targetOrgProps }), // target org intentionally nestled in the middle + orgFactory({ id: "andAnotherOrg" }), +]; + describe("Organization Permissions Guard", () => { let router: MockProxy; let organizationService: MockProxy; @@ -49,7 +59,7 @@ describe("Organization Permissions Guard", () => { state = mock(); route = mock({ params: { - organizationId: orgFactory().id, + organizationId: targetOrgId, }, }); @@ -75,82 +85,79 @@ describe("Organization Permissions Guard", () => { expect(actual).not.toBe(true); }); - it("permits navigation if no permissions are specified", async () => { - const org = orgFactory(); - organizationService.organizations$.calledWith(userId).mockReturnValue(of([org])); + describe("given an enabled organization", () => { + beforeEach(() => { + organizationService.organizations$.calledWith(userId).mockReturnValue(of(orgStateFactory())); + }); - const actual = await TestBed.runInInjectionContext(async () => - organizationPermissionsGuard()(route, state), - ); + it("permits navigation if no permissions are specified", async () => { + const actual = await TestBed.runInInjectionContext(async () => + organizationPermissionsGuard()(route, state), + ); - expect(actual).toBe(true); - }); + expect(actual).toBe(true); + }); - it("permits navigation if the user has permissions", async () => { - const permissionsCallback = jest.fn(); - permissionsCallback.mockImplementation((_org) => true); - const org = orgFactory(); - organizationService.organizations$.calledWith(userId).mockReturnValue(of([org])); - - const actual = await TestBed.runInInjectionContext( - async () => await organizationPermissionsGuard(permissionsCallback)(route, state), - ); - - expect(permissionsCallback).toHaveBeenCalled(); - expect(actual).toBe(true); - }); - - describe("if the user does not have permissions", () => { - it("and there is no Item ID, block navigation", async () => { + it("permits navigation if the user has permissions", async () => { const permissionsCallback = jest.fn(); - permissionsCallback.mockImplementation((_org) => false); - - state = mock({ - root: mock({ - queryParamMap: convertToParamMap({}), - }), - }); - - const org = orgFactory(); - organizationService.organizations$.calledWith(userId).mockReturnValue(of([org])); + permissionsCallback.mockImplementation((_org) => true); const actual = await TestBed.runInInjectionContext( async () => await organizationPermissionsGuard(permissionsCallback)(route, state), ); - expect(permissionsCallback).toHaveBeenCalled(); - expect(actual).not.toBe(true); + expect(permissionsCallback).toHaveBeenCalledWith(orgFactory({ id: targetOrgId })); + expect(actual).toBe(true); }); - it("and there is an Item ID, redirect to the item in the individual vault", async () => { - state = mock({ - root: mock({ - queryParamMap: convertToParamMap({ - itemId: "myItemId", + describe("if the user does not have permissions", () => { + it("and there is no Item ID, block navigation", async () => { + const permissionsCallback = jest.fn(); + permissionsCallback.mockImplementation((_org) => false); + + state = mock({ + root: mock({ + queryParamMap: convertToParamMap({}), }), - }), - }); - const org = orgFactory(); - organizationService.organizations$.calledWith(userId).mockReturnValue(of([org])); + }); - const actual = await TestBed.runInInjectionContext( - async () => await organizationPermissionsGuard((_org: Organization) => false)(route, state), - ); + const actual = await TestBed.runInInjectionContext( + async () => await organizationPermissionsGuard(permissionsCallback)(route, state), + ); - expect(router.createUrlTree).toHaveBeenCalledWith(["/vault"], { - queryParams: { itemId: "myItemId" }, + expect(permissionsCallback).toHaveBeenCalledWith(orgFactory({ id: targetOrgId })); + expect(actual).not.toBe(true); + }); + + it("and there is an Item ID, redirect to the item in the individual vault", async () => { + state = mock({ + root: mock({ + queryParamMap: convertToParamMap({ + itemId: "myItemId", + }), + }), + }); + + const actual = await TestBed.runInInjectionContext( + async () => + await organizationPermissionsGuard((_org: Organization) => false)(route, state), + ); + + expect(router.createUrlTree).toHaveBeenCalledWith(["/vault"], { + queryParams: { itemId: "myItemId" }, + }); + expect(actual).not.toBe(true); }); - expect(actual).not.toBe(true); }); }); describe("given a disabled organization", () => { it("blocks navigation if user is not an owner", async () => { - const org = orgFactory({ + const orgs = orgStateFactory({ type: OrganizationUserType.Admin, enabled: false, }); - organizationService.organizations$.calledWith(userId).mockReturnValue(of([org])); + organizationService.organizations$.calledWith(userId).mockReturnValue(of(orgs)); const actual = await TestBed.runInInjectionContext( async () => await organizationPermissionsGuard()(route, state), @@ -160,11 +167,12 @@ describe("Organization Permissions Guard", () => { }); it("permits navigation if user is an owner", async () => { - const org = orgFactory({ + const orgs = orgStateFactory({ type: OrganizationUserType.Owner, enabled: false, }); - organizationService.organizations$.calledWith(userId).mockReturnValue(of([org])); + + organizationService.organizations$.calledWith(userId).mockReturnValue(of(orgs)); const actual = await TestBed.runInInjectionContext( async () => await organizationPermissionsGuard()(route, state), diff --git a/apps/web/src/app/admin-console/organizations/guards/org-permissions.guard.ts b/apps/web/src/app/admin-console/organizations/guards/org-permissions.guard.ts index ea9bfad3893..d399f9c9c05 100644 --- a/apps/web/src/app/admin-console/organizations/guards/org-permissions.guard.ts +++ b/apps/web/src/app/admin-console/organizations/guards/org-permissions.guard.ts @@ -7,7 +7,7 @@ import { Router, RouterStateSnapshot, } from "@angular/router"; -import { firstValueFrom, map } from "rxjs"; +import { firstValueFrom, switchMap } from "rxjs"; import { canAccessOrgAdmin, @@ -15,7 +15,9 @@ import { } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { getUserId } from "@bitwarden/common/auth/services/account.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { getById } from "@bitwarden/common/platform/misc"; import { SyncService } from "@bitwarden/common/vault/abstractions/sync/sync.service.abstraction"; import { ToastService } from "@bitwarden/components"; @@ -55,12 +57,12 @@ export function organizationPermissionsGuard( await syncService.fullSync(false); } - const userId = await firstValueFrom(accountService.activeAccount$.pipe(map((a) => a?.id))); - const org = await firstValueFrom( - organizationService - .organizations$(userId) - .pipe(map((organizations) => organizations.find((org) => route.params.organizationId))), + accountService.activeAccount$.pipe( + getUserId, + switchMap((userId) => organizationService.organizations$(userId)), + getById(route.params.organizationId), + ), ); if (org == null) {