Upstream/bitwarden-browser
1
0
Fork 0
mirror of https://github.com/bitwarden/browser.git synced 2024-11-25 12:15:18 +01:00
Code Issues Projects Releases Wiki Activity

Removing hanging promises, and adding a guard to projects routing (#8891)

Browse Source 
* Removing hanging promises, and adding a guard to projects routing

* Additional logging

* adding tests

* Trying to get Jest tests working

* coltons suggested changes
This commit is contained in:
cd-bitwarden 2024-06-04 10:59:43 -04:00 committed by GitHub
parent ed7a57810e
commit 6fa12fea49
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
15 changed files with 321 additions and 63 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
bitwarden_license/bit-web/src/app/secrets-manager/overview/overview.component.ts
View File

@ -17,6 +17,7 @@ import {
import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
import { DialogService } from "@bitwarden/components";
@ -94,6 +95,7 @@ export class OverviewComponent implements OnInit, OnDestroy {
private platformUtilsService: PlatformUtilsService,
private i18nService: I18nService,
private smOnboardingTasksService: SMOnboardingTasksService,
private logService: LogService,
) {}
ngOnInit() {
@ -297,12 +299,13 @@ export class OverviewComponent implements OnInit, OnDestroy {
SecretsListComponent.copySecretName(name, this.platformUtilsService, this.i18nService);
}
copySecretValue(id: string) {
SecretsListComponent.copySecretValue(
async copySecretValue(id: string) {
await SecretsListComponent.copySecretValue(
id,
this.platformUtilsService,
this.i18nService,
this.secretService,
this.logService,
);
}
@ -310,11 +313,9 @@ export class OverviewComponent implements OnInit, OnDestroy {
SecretsListComponent.copySecretUuid(id, this.platformUtilsService, this.i18nService);
}
protected hideOnboarding() {
protected async hideOnboarding() {
this.showOnboarding = false;
// FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
// eslint-disable-next-line @typescript-eslint/no-floating-promises
this.saveCompletedTasks(this.organizationId, {
await this.saveCompletedTasks(this.organizationId, {
importSecrets: true,
createSecret: true,
createProject: true,

4
bitwarden_license/bit-web/src/app/secrets-manager/projects/dialog/project-dialog.component.ts
View File

@ -82,9 +82,7 @@ export class ProjectDialogComponent implements OnInit {
const projectView = this.getProjectView();
if (this.data.operation === OperationType.Add) {
const newProject = await this.createProject(projectView);
// FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
// eslint-disable-next-line @typescript-eslint/no-floating-promises
this.router.navigate(["sm", this.data.organizationId, "projects", newProject.id]);
await this.router.navigate(["sm", this.data.organizationId, "projects", newProject.id]);
} else {
projectView.id = this.data.projectId;
await this.updateProject(projectView);

120
bitwarden_license/bit-web/src/app/secrets-manager/projects/guards/project-access.guard.spec.ts Normal file
View File

@ -0,0 +1,120 @@
import { Component } from "@angular/core";
import { TestBed } from "@angular/core/testing";
import { Router } from "@angular/router";
import { RouterTestingModule } from "@angular/router/testing";
import { MockProxy, mock } from "jest-mock-extended";
import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction";
import { Organization } from "@bitwarden/common/admin-console/models/domain/organization";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
import { RouterService } from "../../../../../../../apps/web/src/app/core/router.service";
import { ProjectView } from "../../models/view/project.view";
import { ProjectService } from "../project.service";
import { projectAccessGuard } from "./project-access.guard";
@Component({
template: "",
})
export class GuardedRouteTestComponent {}
@Component({
template: "",
})
export class RedirectTestComponent {}
describe("Project Redirect Guard", () => {
let organizationService: MockProxy<OrganizationService>;
let routerService: MockProxy<RouterService>;
let projectServiceMock: MockProxy<ProjectService>;
let i18nServiceMock: MockProxy<I18nService>;
let platformUtilsService: MockProxy<PlatformUtilsService>;
let router: Router;
const smOrg1 = { id: "123", canAccessSecretsManager: true } as Organization;
const projectView = {
id: "123",
organizationId: "123",
name: "project-name",
creationDate: Date.now.toString(),
revisionDate: Date.now.toString(),
read: true,
write: true,
} as ProjectView;
beforeEach(async () => {
organizationService = mock<OrganizationService>();
routerService = mock<RouterService>();
projectServiceMock = mock<ProjectService>();
i18nServiceMock = mock<I18nService>();
platformUtilsService = mock<PlatformUtilsService>();
TestBed.configureTestingModule({
imports: [
RouterTestingModule.withRoutes([
{
path: "sm/:organizationId/projects/:projectId",
component: GuardedRouteTestComponent,
canActivate: [projectAccessGuard],
},
{
path: "sm",
component: RedirectTestComponent,
},
{
path: "sm/:organizationId/projects",
component: RedirectTestComponent,
},
]),
],
providers: [
{ provide: OrganizationService, useValue: organizationService },
{ provide: RouterService, useValue: routerService },
{ provide: ProjectService, useValue: projectServiceMock },
{ provide: I18nService, useValue: i18nServiceMock },
{ provide: PlatformUtilsService, useValue: platformUtilsService },
],
});
router = TestBed.inject(Router);
});
it("redirects to sm/{orgId}/projects/{projectId} if project exists", async () => {
// Arrange
organizationService.getAll.mockResolvedValue([smOrg1]);
projectServiceMock.getByProjectId.mockReturnValue(Promise.resolve(projectView));
// Act
await router.navigateByUrl("sm/123/projects/123");
// Assert
expect(router.url).toBe("/sm/123/projects/123");
});
it("redirects to sm/projects if project does not exist", async () => {
// Arrange
organizationService.getAll.mockResolvedValue([smOrg1]);
// Act
await router.navigateByUrl("sm/123/projects/124");
// Assert
expect(router.url).toBe("/sm/123/projects");
});
it("redirects to sm/123/projects if exception occurs while looking for Project", async () => {
// Arrange
jest.spyOn(projectServiceMock, "getByProjectId").mockImplementation(() => {
throw new Error("Test error");
});
jest.spyOn(i18nServiceMock, "t").mockReturnValue("Project not found");
// Act
await router.navigateByUrl("sm/123/projects/123");
// Assert
expect(platformUtilsService.showToast).toHaveBeenCalledWith("error", null, "Project not found");
expect(router.url).toBe("/sm/123/projects");
});
});

31
bitwarden_license/bit-web/src/app/secrets-manager/projects/guards/project-access.guard.ts Normal file
View File

@ -0,0 +1,31 @@
import { inject } from "@angular/core";
import { ActivatedRouteSnapshot, CanActivateFn, createUrlTreeFromSnapshot } from "@angular/router";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
import { ProjectService } from "../project.service";
/**
* Redirects to projects list if the user doesn't have access to project.
*/
export const projectAccessGuard: CanActivateFn = async (route: ActivatedRouteSnapshot) => {
const projectService = inject(ProjectService);
const platformUtilsService = inject(PlatformUtilsService);
const i18nService = inject(I18nService);
try {
const project = await projectService.getByProjectId(route.params.projectId);
if (project) {
return true;
}
} catch {
platformUtilsService.showToast(
"error",
null,
i18nService.t("notFound", i18nService.t("project")),
);
return createUrlTreeFromSnapshot(route, ["/sm", route.params.organizationId, "projects"]);
}
return createUrlTreeFromSnapshot(route, ["/sm", route.params.organizationId, "projects"]);
};

5
bitwarden_license/bit-web/src/app/secrets-manager/projects/project/project-people.component.ts
View File

@ -4,6 +4,7 @@ import { ActivatedRoute, Router } from "@angular/router";
import { combineLatest, Subject, switchMap, takeUntil, catchError } from "rxjs";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
import { ValidationService } from "@bitwarden/common/platform/abstractions/validation.service";
import { DialogService } from "@bitwarden/components";
@ -38,8 +39,9 @@ export class ProjectPeopleComponent implements OnInit, OnDestroy {
}),
),
catchError(async () => {
this.logService.info("Error fetching project people access policies.");
await this.router.navigate(["/sm", this.organizationId, "projects"]);
return [];
return undefined;
}),
);
@ -70,6 +72,7 @@ export class ProjectPeopleComponent implements OnInit, OnDestroy {
private platformUtilsService: PlatformUtilsService,
private i18nService: I18nService,
private accessPolicySelectorService: AccessPolicySelectorService,
private logService: LogService,
) {}
ngOnInit(): void {

7
bitwarden_license/bit-web/src/app/secrets-manager/projects/project/project-secrets.component.ts
View File

@ -4,6 +4,7 @@ import { combineLatest, combineLatestWith, filter, Observable, startWith, switch
import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
import { DialogService } from "@bitwarden/components";
@ -42,6 +43,7 @@ export class ProjectSecretsComponent {
private platformUtilsService: PlatformUtilsService,
private i18nService: I18nService,
private organizationService: OrganizationService,
private logService: LogService,
) {}
ngOnInit() {
@ -109,12 +111,13 @@ export class ProjectSecretsComponent {
SecretsListComponent.copySecretName(name, this.platformUtilsService, this.i18nService);
}
copySecretValue(id: string) {
SecretsListComponent.copySecretValue(
async copySecretValue(id: string) {
await SecretsListComponent.copySecretValue(
id,
this.platformUtilsService,
this.i18nService,
this.secretService,
this.logService,
);
}

14
bitwarden_license/bit-web/src/app/secrets-manager/projects/project/project.component.ts
View File

@ -1,9 +1,7 @@
import { Component, OnDestroy, OnInit } from "@angular/core";
import { ActivatedRoute, Router } from "@angular/router";
import {
catchError,
combineLatest,
EMPTY,
filter,
Observable,
startWith,
@ -58,18 +56,6 @@ export class ProjectComponent implements OnInit, OnDestroy {
this.project$ = combineLatest([this.route.params, currentProjectEdited]).pipe(
switchMap(([params, _]) => this.projectService.getByProjectId(params.projectId)),
catchError(() => {
// FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
// eslint-disable-next-line @typescript-eslint/no-floating-promises
this.router.navigate(["/sm", this.organizationId, "projects"]).then(() => {
this.platformUtilsService.showToast(
"error",
null,
this.i18nService.t("notFound", this.i18nService.t("project")),
);
});
return EMPTY;
}),
);
const projectId$ = this.route.params.pipe(map((p) => p.projectId));

2
bitwarden_license/bit-web/src/app/secrets-manager/projects/projects-routing.module.ts
View File

@ -1,6 +1,7 @@
import { NgModule } from "@angular/core";
import { RouterModule, Routes } from "@angular/router";
import { projectAccessGuard } from "./guards/project-access.guard";
import { ProjectPeopleComponent } from "./project/project-people.component";
import { ProjectSecretsComponent } from "./project/project-secrets.component";
import { ProjectServiceAccountsComponent } from "./project/project-service-accounts.component";
@ -15,6 +16,7 @@ const routes: Routes = [
{
path: ":projectId",
component: ProjectComponent,
canActivate: [projectAccessGuard],
children: [
{
path: "",

6
bitwarden_license/bit-web/src/app/secrets-manager/secrets/dialog/secret-dialog.component.ts
View File

@ -199,7 +199,7 @@ export class SecretDialogComponent implements OnInit {
return await this.projectService.create(this.data.organizationId, projectView);
}
protected openDeleteSecretDialog() {
protected async openDeleteSecretDialog() {
const secretListView: SecretListView[] = this.getSecretListView();
const dialogRef = this.dialogService.open<unknown, SecretDeleteOperation>(
@ -212,9 +212,7 @@ export class SecretDialogComponent implements OnInit {
);
// If the secret is deleted, chain close this dialog after the delete dialog
// FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
// eslint-disable-next-line @typescript-eslint/no-floating-promises
lastValueFrom(dialogRef.closed).then(
await lastValueFrom(dialogRef.closed).then(
(closeData) => closeData !== undefined && this.dialogRef.close(),
);
}

7
bitwarden_license/bit-web/src/app/secrets-manager/secrets/secrets.component.ts
View File

@ -4,6 +4,7 @@ import { combineLatestWith, Observable, startWith, switchMap } from "rxjs";
import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
import { DialogService } from "@bitwarden/components";
@ -39,6 +40,7 @@ export class SecretsComponent implements OnInit {
private platformUtilsService: PlatformUtilsService,
private i18nService: I18nService,
private organizationService: OrganizationService,
private logService: LogService,
) {}
ngOnInit() {
@ -97,12 +99,13 @@ export class SecretsComponent implements OnInit {
SecretsListComponent.copySecretName(name, this.platformUtilsService, this.i18nService);
}
copySecretValue(id: string) {
SecretsListComponent.copySecretValue(
async copySecretValue(id: string) {
await SecretsListComponent.copySecretValue(
id,
this.platformUtilsService,
this.i18nService,
this.secretService,
this.logService,
);
}

4
bitwarden_license/bit-web/src/app/secrets-manager/service-accounts/dialog/service-account-dialog.component.ts
View File

@ -47,9 +47,7 @@ export class ServiceAccountDialogComponent {
async ngOnInit() {
if (this.data.operation == OperationType.Edit) {
// FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
// eslint-disable-next-line @typescript-eslint/no-floating-promises
this.loadData();
await this.loadData();
}
}

122
bitwarden_license/bit-web/src/app/secrets-manager/service-accounts/guards/service-account-access.guard.spec.ts Normal file
View File

@ -0,0 +1,122 @@
import { Component } from "@angular/core";
import { TestBed } from "@angular/core/testing";
import { Router } from "@angular/router";
import { RouterTestingModule } from "@angular/router/testing";
import { MockProxy, mock } from "jest-mock-extended";
import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction";
import { Organization } from "@bitwarden/common/admin-console/models/domain/organization";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
import { RouterService } from "../../../../../../../../clients/apps/web/src/app/core/router.service";
import { ServiceAccountView } from "../../models/view/service-account.view";
import { ServiceAccountService } from "../service-account.service";
import { serviceAccountAccessGuard } from "./service-account-access.guard";
@Component({
template: "",
})
export class GuardedRouteTestComponent {}
@Component({
template: "",
})
export class RedirectTestComponent {}
describe("Service account Redirect Guard", () => {
let organizationService: MockProxy<OrganizationService>;
let routerService: MockProxy<RouterService>;
let serviceAccountServiceMock: MockProxy<ServiceAccountService>;
let i18nServiceMock: MockProxy<I18nService>;
let platformUtilsService: MockProxy<PlatformUtilsService>;
let router: Router;
const smOrg1 = { id: "123", canAccessSecretsManager: true } as Organization;
const serviceAccountView = {
id: "123",
organizationId: "123",
name: "service-account-name",
} as ServiceAccountView;
beforeEach(async () => {
organizationService = mock<OrganizationService>();
routerService = mock<RouterService>();
serviceAccountServiceMock = mock<ServiceAccountService>();
i18nServiceMock = mock<I18nService>();
platformUtilsService = mock<PlatformUtilsService>();
TestBed.configureTestingModule({
imports: [
RouterTestingModule.withRoutes([
{
path: "sm/:organizationId/machine-accounts/:serviceAccountId",
component: GuardedRouteTestComponent,
canActivate: [serviceAccountAccessGuard],
},
{
path: "sm",
component: RedirectTestComponent,
},
{
path: "sm/:organizationId/machine-accounts",
component: RedirectTestComponent,
},
]),
],
providers: [
{ provide: OrganizationService, useValue: organizationService },
{ provide: RouterService, useValue: routerService },
{ provide: ServiceAccountService, useValue: serviceAccountServiceMock },
{ provide: I18nService, useValue: i18nServiceMock },
{ provide: PlatformUtilsService, useValue: platformUtilsService },
],
});
router = TestBed.inject(Router);
});
it("redirects to sm/{orgId}/machine-accounts/{serviceAccountId} if machine account exists", async () => {
// Arrange
organizationService.getAll.mockResolvedValue([smOrg1]);
serviceAccountServiceMock.getByServiceAccountId.mockReturnValue(
Promise.resolve(serviceAccountView),
);
// Act
await router.navigateByUrl("sm/123/machine-accounts/123");
// Assert
expect(router.url).toBe("/sm/123/machine-accounts/123");
});
it("redirects to sm/machine-accounts if machine account does not exist", async () => {
// Arrange
organizationService.getAll.mockResolvedValue([smOrg1]);
// Act
await router.navigateByUrl("sm/123/machine-accounts/124");
// Assert
expect(router.url).toBe("/sm/123/machine-accounts");
});
it("redirects to sm/123/machine-accounts if exception occurs while looking for service account", async () => {
// Arrange
jest.spyOn(serviceAccountServiceMock, "getByServiceAccountId").mockImplementation(() => {
throw new Error("Test error");
});
jest.spyOn(i18nServiceMock, "t").mockReturnValue("Service account not found");
// Act
await router.navigateByUrl("sm/123/machine-accounts/123");
// Assert
expect(platformUtilsService.showToast).toHaveBeenCalledWith(
"error",
null,
"Service account not found",
);
expect(router.url).toBe("/sm/123/machine-accounts");
});
});

11
bitwarden_license/bit-web/src/app/secrets-manager/service-accounts/guards/service-account-access.guard.ts
View File

@ -1,6 +1,9 @@
import { inject } from "@angular/core";
import { ActivatedRouteSnapshot, CanActivateFn, createUrlTreeFromSnapshot } from "@angular/router";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
import { ServiceAccountService } from "../service-account.service";
/**
@ -8,6 +11,8 @@ import { ServiceAccountService } from "../service-account.service";
*/
export const serviceAccountAccessGuard: CanActivateFn = async (route: ActivatedRouteSnapshot) => {
const serviceAccountService = inject(ServiceAccountService);
const platformUtilsService = inject(PlatformUtilsService);
const i18nService = inject(I18nService);
try {
const serviceAccount = await serviceAccountService.getByServiceAccountId(
@ -18,6 +23,12 @@ export const serviceAccountAccessGuard: CanActivateFn = async (route: ActivatedR
return true;
}
} catch {
platformUtilsService.showToast(
"error",
null,
i18nService.t("notFound", i18nService.t("machineAccount")),
);
return createUrlTreeFromSnapshot(route, [
"/sm",
route.params.organizationId,

23
bitwarden_license/bit-web/src/app/secrets-manager/service-accounts/service-account.component.ts
View File

@ -1,15 +1,6 @@
import { Component, OnDestroy, OnInit } from "@angular/core";
import { ActivatedRoute, Router } from "@angular/router";
import {
EMPTY,
Subject,
catchError,
combineLatest,
filter,
startWith,
switchMap,
takeUntil,
} from "rxjs";
import { Subject, combineLatest, filter, startWith, switchMap, takeUntil } from "rxjs";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
@ -42,18 +33,6 @@ export class ServiceAccountComponent implements OnInit, OnDestroy {
params.organizationId,
),
),
catchError(() => {
// FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
// eslint-disable-next-line @typescript-eslint/no-floating-promises
this.router.navigate(["/sm", this.organizationId, "machine-accounts"]).then(() => {
this.platformUtilsService.showToast(
"error",
null,
this.i18nService.t("notFound", this.i18nService.t("machineAccount")),
);
});
return EMPTY;
}),
);
constructor(

15
bitwarden_license/bit-web/src/app/secrets-manager/shared/secrets-list.component.ts
View File

@ -3,6 +3,7 @@ import { Component, EventEmitter, Input, OnDestroy, Output } from "@angular/core
import { Subject, takeUntil } from "rxjs";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
import { TableDataSource } from "@bitwarden/components";
@ -134,22 +135,24 @@ export class SecretsListComponent implements OnDestroy {
/**
* TODO: Refactor to smart component and remove
*/
static copySecretValue(
static async copySecretValue(
id: string,
platformUtilsService: PlatformUtilsService,
i18nService: I18nService,
secretService: SecretService,
logService: LogService,
) {
const value = secretService.getBySecretId(id).then((secret) => secret.value);
// FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
// eslint-disable-next-line @typescript-eslint/no-floating-promises
SecretsListComponent.copyToClipboardAsync(value, platformUtilsService).then(() => {
try {
const value = await secretService.getBySecretId(id).then((secret) => secret.value);
platformUtilsService.copyToClipboard(value);
platformUtilsService.showToast(
"success",
null,
i18nService.t("valueCopied", i18nService.t("value")),
);
});
} catch {
logService.info("Error fetching secret value.");
}
}
static copySecretUuid(