bitwarden/browser
1
0
Fork 0
mirror of https://github.com/bitwarden/browser synced 2025-12-14 23:33:31 +00:00
Code Issues Releases Wiki Activity

[PM-17692] Fix AC navigation permissions bug (#13116)

Browse Source 
* Add failing tests

* Fix bug
This commit is contained in:
Thomas Rittson
2025-01-30 01:01:01 +10:00
committed by GitHub
parent 60e569ed9d
commit fe08980a89
2 changed files with 73 additions and 63 deletions
Download Patch File Download Diff File Expand all files Collapse all files

122
apps/web/src/app/admin-console/organizations/guards/org-permissions.guard.spec.ts
View File

@@ -23,17 +23,27 @@ import { ToastService } from "@bitwarden/components";
import { organizationPermissionsGuard } from "./org-permissions.guard"; import { organizationPermissionsGuard } from "./org-permissions.guard";
// Returns a test organization with the specified props.
const orgFactory = (props: Partial<Organization> = {}) => const orgFactory = (props: Partial<Organization> = {}) =>
Object.assign( Object.assign(
new Organization(), new Organization(),
{ {
id: "myOrgId",
enabled: true, enabled: true,
type: OrganizationUserType.Admin, type: OrganizationUserType.Admin,
}, },
props, 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<Organization> = {}) => [
orgFactory({ id: "anotherOrg" }),
orgFactory({ id: targetOrgId, ...targetOrgProps }), // target org intentionally nestled in the middle
orgFactory({ id: "andAnotherOrg" }),
];
describe("Organization Permissions Guard", () => { describe("Organization Permissions Guard", () => {
let router: MockProxy<Router>; let router: MockProxy<Router>;
let organizationService: MockProxy<OrganizationService>; let organizationService: MockProxy<OrganizationService>;
@@ -49,7 +59,7 @@ describe("Organization Permissions Guard", () => {
state = mock<RouterStateSnapshot>(); state = mock<RouterStateSnapshot>();
route = mock<ActivatedRouteSnapshot>({ route = mock<ActivatedRouteSnapshot>({
params: { params: {
organizationId: orgFactory().id, organizationId: targetOrgId,
}, },
}); });
@@ -75,82 +85,79 @@ describe("Organization Permissions Guard", () => {
expect(actual).not.toBe(true); expect(actual).not.toBe(true);
}); });
it("permits navigation if no permissions are specified", async () => { describe("given an enabled organization", () => {
const org = orgFactory(); beforeEach(() => {
organizationService.organizations$.calledWith(userId).mockReturnValue(of([org])); organizationService.organizations$.calledWith(userId).mockReturnValue(of(orgStateFactory()));
});
const actual = await TestBed.runInInjectionContext(async () => it("permits navigation if no permissions are specified", async () => {
organizationPermissionsGuard()(route, state), 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 () => { 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 () => {
const permissionsCallback = jest.fn(); const permissionsCallback = jest.fn();
permissionsCallback.mockImplementation((_org) => false); permissionsCallback.mockImplementation((_org) => true);
state = mock<RouterStateSnapshot>({
root: mock<ActivatedRouteSnapshot>({
queryParamMap: convertToParamMap({}),
}),
});
const org = orgFactory();
organizationService.organizations$.calledWith(userId).mockReturnValue(of([org]));
const actual = await TestBed.runInInjectionContext( const actual = await TestBed.runInInjectionContext(
async () => await organizationPermissionsGuard(permissionsCallback)(route, state), async () => await organizationPermissionsGuard(permissionsCallback)(route, state),
); );
expect(permissionsCallback).toHaveBeenCalled(); expect(permissionsCallback).toHaveBeenCalledWith(orgFactory({ id: targetOrgId }));
expect(actual).not.toBe(true); expect(actual).toBe(true);
}); });
it("and there is an Item ID, redirect to the item in the individual vault", async () => { describe("if the user does not have permissions", () => {
state = mock<RouterStateSnapshot>({ it("and there is no Item ID, block navigation", async () => {
root: mock<ActivatedRouteSnapshot>({ const permissionsCallback = jest.fn();
queryParamMap: convertToParamMap({ permissionsCallback.mockImplementation((_org) => false);
itemId: "myItemId",
state = mock<RouterStateSnapshot>({
root: mock<ActivatedRouteSnapshot>({
queryParamMap: convertToParamMap({}),
}), }),
}), });
});
const org = orgFactory();
organizationService.organizations$.calledWith(userId).mockReturnValue(of([org]));
const actual = await TestBed.runInInjectionContext( const actual = await TestBed.runInInjectionContext(
async () => await organizationPermissionsGuard((_org: Organization) => false)(route, state), async () => await organizationPermissionsGuard(permissionsCallback)(route, state),
); );
expect(router.createUrlTree).toHaveBeenCalledWith(["/vault"], { expect(permissionsCallback).toHaveBeenCalledWith(orgFactory({ id: targetOrgId }));
queryParams: { itemId: "myItemId" }, expect(actual).not.toBe(true);
});
it("and there is an Item ID, redirect to the item in the individual vault", async () => {
state = mock<RouterStateSnapshot>({
root: mock<ActivatedRouteSnapshot>({
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", () => { describe("given a disabled organization", () => {
it("blocks navigation if user is not an owner", async () => { it("blocks navigation if user is not an owner", async () => {
const org = orgFactory({ const orgs = orgStateFactory({
type: OrganizationUserType.Admin, type: OrganizationUserType.Admin,
enabled: false, enabled: false,
}); });
organizationService.organizations$.calledWith(userId).mockReturnValue(of([org])); organizationService.organizations$.calledWith(userId).mockReturnValue(of(orgs));
const actual = await TestBed.runInInjectionContext( const actual = await TestBed.runInInjectionContext(
async () => await organizationPermissionsGuard()(route, state), async () => await organizationPermissionsGuard()(route, state),
@@ -160,11 +167,12 @@ describe("Organization Permissions Guard", () => {
}); });
it("permits navigation if user is an owner", async () => { it("permits navigation if user is an owner", async () => {
const org = orgFactory({ const orgs = orgStateFactory({
type: OrganizationUserType.Owner, type: OrganizationUserType.Owner,
enabled: false, enabled: false,
}); });
organizationService.organizations$.calledWith(userId).mockReturnValue(of([org]));
organizationService.organizations$.calledWith(userId).mockReturnValue(of(orgs));
const actual = await TestBed.runInInjectionContext( const actual = await TestBed.runInInjectionContext(
async () => await organizationPermissionsGuard()(route, state), async () => await organizationPermissionsGuard()(route, state),

14
apps/web/src/app/admin-console/organizations/guards/org-permissions.guard.ts
View File

@@ -7,7 +7,7 @@ import {
Router, Router,
RouterStateSnapshot, RouterStateSnapshot,
} from "@angular/router"; } from "@angular/router";
import { firstValueFrom, map } from "rxjs"; import { firstValueFrom, switchMap } from "rxjs";
import { import {
canAccessOrgAdmin, canAccessOrgAdmin,
@@ -15,7 +15,9 @@ import {
} from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction";
import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization";
import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; 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 { 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 { SyncService } from "@bitwarden/common/vault/abstractions/sync/sync.service.abstraction";
import { ToastService } from "@bitwarden/components"; import { ToastService } from "@bitwarden/components";
@@ -55,12 +57,12 @@ export function organizationPermissionsGuard(
await syncService.fullSync(false); await syncService.fullSync(false);
} }
const userId = await firstValueFrom(accountService.activeAccount$.pipe(map((a) => a?.id)));
const org = await firstValueFrom( const org = await firstValueFrom(
organizationService accountService.activeAccount$.pipe(
.organizations$(userId) getUserId,
.pipe(map((organizations) => organizations.find((org) => route.params.organizationId))), switchMap((userId) => organizationService.organizations$(userId)),
getById(route.params.organizationId),
),
); );
if (org == null) { if (org == null) {