From b09171974841a3baf9df3ec44308266da8af29fb Mon Sep 17 00:00:00 2001 From: Daniel James Smith <2670567+djsmith85@users.noreply.github.com> Date: Thu, 18 Sep 2025 22:02:49 +0200 Subject: [PATCH] Introduce a stricter use of the OrganizationId type on org-vault exports (#15836) Co-authored-by: Daniel James Smith --- .../org-vault-export.component.ts | 18 ++-- libs/common/src/types/guid.spec.ts | 35 ++++++++ libs/common/src/types/guid.ts | 22 +++++ .../org-vault-export.service.abstraction.ts | 6 +- .../src/services/org-vault-export.service.ts | 89 +++++++++---------- .../vault-export.service.abstraction.ts | 4 +- .../src/services/vault-export.service.ts | 3 +- .../src/components/export.component.ts | 32 ++++--- 8 files changed, 139 insertions(+), 70 deletions(-) create mode 100644 libs/common/src/types/guid.spec.ts diff --git a/apps/web/src/app/tools/vault-export/org-vault-export.component.ts b/apps/web/src/app/tools/vault-export/org-vault-export.component.ts index a1de4814a1..69dcce5282 100644 --- a/apps/web/src/app/tools/vault-export/org-vault-export.component.ts +++ b/apps/web/src/app/tools/vault-export/org-vault-export.component.ts @@ -1,8 +1,7 @@ -// FIXME: Update this file to be type safe and remove this and next line -// @ts-strict-ignore import { Component, OnInit } from "@angular/core"; import { ActivatedRoute } from "@angular/router"; +import { isId, OrganizationId } from "@bitwarden/common/types/guid"; import { ExportComponent } from "@bitwarden/vault-export-ui"; import { HeaderModule } from "../../layouts/header/header.module"; @@ -13,18 +12,27 @@ import { SharedModule } from "../../shared"; imports: [SharedModule, ExportComponent, HeaderModule], }) export class OrganizationVaultExportComponent implements OnInit { - protected routeOrgId: string = null; + protected routeOrgId: OrganizationId | undefined = undefined; protected loading = false; protected disabled = false; constructor(private route: ActivatedRoute) {} async ngOnInit() { - this.routeOrgId = this.route.snapshot.paramMap.get("organizationId"); + const orgIdParam = this.route.snapshot.paramMap.get("organizationId"); + if (orgIdParam === undefined) { + throw new Error("`organizationId` is a required route parameter"); + } + + if (!isId(orgIdParam)) { + throw new Error("Invalid OrganizationId provided in route parameter `organizationId`"); + } + + this.routeOrgId = orgIdParam; } /** * Callback that is called after a successful export. */ - protected async onSuccessfulExport(organizationId: string): Promise {} + protected async onSuccessfulExport(organizationId: OrganizationId): Promise {} } diff --git a/libs/common/src/types/guid.spec.ts b/libs/common/src/types/guid.spec.ts new file mode 100644 index 0000000000..bb0966bcbd --- /dev/null +++ b/libs/common/src/types/guid.spec.ts @@ -0,0 +1,35 @@ +import { isId, emptyGuid, OrganizationId } from "./guid"; + +describe("isId tests", () => { + it("should return true for a valid guid string", () => { + // Example valid GUID + const validGuid = "12345678-1234-1234-1234-123456789abc"; + expect(isId(validGuid)).toBe(true); + }); + + it("should return false for an invalid guid string", () => { + // Example invalid GUID + const invalidGuid = "not-a-guid"; + expect(isId(invalidGuid)).toBe(false); + }); + + it("should return false for non-string values", () => { + expect(isId(undefined)).toBe(false); + expect(isId(null)).toBe(false); + expect(isId(123)).toBe(false); + expect(isId({})).toBe(false); + expect(isId([])).toBe(false); + }); + + it("should return true for the emptyGuid constant if it is a valid guid", () => { + expect(isId(emptyGuid)).toBe(true); + }); + + it("should infer type OrganizationId when using isId", () => { + const orgId: string = "12345678-1234-1234-1234-123456789abc"; + if (isId(orgId)) { + return; + } + throw new Error("Type guard failed, orgId is not a valid OrganizationId"); + }); +}); diff --git a/libs/common/src/types/guid.ts b/libs/common/src/types/guid.ts index 94aa44d64b..f09b87fef8 100644 --- a/libs/common/src/types/guid.ts +++ b/libs/common/src/types/guid.ts @@ -1,5 +1,7 @@ import { Opaque } from "type-fest"; +import { isGuid } from "@bitwarden/guid"; + export type Guid = Opaque; // Convenience re-export of UserId from it's original location, any library that @@ -26,3 +28,23 @@ export type OrganizationReportId = Opaque; * A string representation of an empty guid. */ export const emptyGuid = "00000000-0000-0000-0000-000000000000"; + +/** + * Determines if the provided value is a valid GUID string. + * + * @typeParam SomeGuid - The input type, defaults to `Guid`. + * @typeParam Output - The output type, resolves to `SomeGuid` if it is an opaque string, otherwise to `Guid` if `SomeGuid` is a string, or `never`. + * @param id - The value to check. + * @returns `true` if `id` is a string and a valid GUID, otherwise `false`. + */ +export function isId< + SomeGuid extends string = Guid, + // eslint-disable-next-line @typescript-eslint/no-unused-vars + Output = SomeGuid extends Opaque + ? SomeGuid + : SomeGuid extends string + ? Guid + : never, +>(id: unknown): id is Output { + return typeof id === "string" && isGuid(id); +} diff --git a/libs/tools/export/vault-export/vault-export-core/src/services/org-vault-export.service.abstraction.ts b/libs/tools/export/vault-export/vault-export-core/src/services/org-vault-export.service.abstraction.ts index 002b1c2d5d..3b3b3aaf90 100644 --- a/libs/tools/export/vault-export/vault-export-core/src/services/org-vault-export.service.abstraction.ts +++ b/libs/tools/export/vault-export/vault-export-core/src/services/org-vault-export.service.abstraction.ts @@ -1,15 +1,17 @@ +import { OrganizationId } from "@bitwarden/common/types/guid"; + import { ExportedVaultAsString } from "../types"; import { ExportFormat } from "./vault-export.service.abstraction"; export abstract class OrganizationVaultExportServiceAbstraction { abstract getPasswordProtectedExport: ( - organizationId: string, + organizationId: OrganizationId, password: string, onlyManagedCollections: boolean, ) => Promise; abstract getOrganizationExport: ( - organizationId: string, + organizationId: OrganizationId, format: ExportFormat, onlyManagedCollections: boolean, ) => Promise; diff --git a/libs/tools/export/vault-export/vault-export-core/src/services/org-vault-export.service.ts b/libs/tools/export/vault-export/vault-export-core/src/services/org-vault-export.service.ts index a0a05f29ff..bf00d1a350 100644 --- a/libs/tools/export/vault-export/vault-export-core/src/services/org-vault-export.service.ts +++ b/libs/tools/export/vault-export/vault-export-core/src/services/org-vault-export.service.ts @@ -65,7 +65,7 @@ export class OrganizationVaultExportService * @returns The exported vault */ async getPasswordProtectedExport( - organizationId: string, + organizationId: OrganizationId, password: string, onlyManagedCollections: boolean, ): Promise { @@ -94,7 +94,7 @@ export class OrganizationVaultExportService * @throws Error if the organization policies prevent the export */ async getOrganizationExport( - organizationId: string, + organizationId: OrganizationId, format: ExportFormat = "csv", onlyManagedCollections: boolean, ): Promise { @@ -128,7 +128,7 @@ export class OrganizationVaultExportService private async getOrganizationDecryptedExport( activeUserId: UserId, - organizationId: string, + organizationId: OrganizationId, format: "json" | "csv", ): Promise { const decCollections: CollectionView[] = []; @@ -138,49 +138,42 @@ export class OrganizationVaultExportService const restrictions = await firstValueFrom(this.restrictedItemTypesService.restricted$); promises.push( - this.vaultExportApiService - .getOrganizationExport(organizationId as OrganizationId) - .then((exportData) => { - const exportPromises: Promise[] = []; - if (exportData != null) { - if (exportData.collections != null && exportData.collections.length > 0) { - exportData.collections.forEach((c) => { - const collection = Collection.fromCollectionData( - new CollectionData(c as CollectionDetailsResponse), - ); + this.vaultExportApiService.getOrganizationExport(organizationId).then((exportData) => { + const exportPromises: Promise[] = []; + if (exportData != null) { + if (exportData.collections != null && exportData.collections.length > 0) { + exportData.collections.forEach((c) => { + const collection = Collection.fromCollectionData( + new CollectionData(c as CollectionDetailsResponse), + ); + exportPromises.push( + firstValueFrom(this.keyService.activeUserOrgKeys$) + .then((keys) => collection.decrypt(keys[organizationId], this.encryptService)) + .then((decCol) => { + decCollections.push(decCol); + }), + ); + }); + } + if (exportData.ciphers != null && exportData.ciphers.length > 0) { + exportData.ciphers + .filter((c) => c.deletedDate === null) + .forEach(async (c) => { + const cipher = new Cipher(new CipherData(c)); exportPromises.push( - firstValueFrom(this.keyService.activeUserOrgKeys$) - .then((keys) => - collection.decrypt( - keys[organizationId as OrganizationId], - this.encryptService, - ), - ) - .then((decCol) => { - decCollections.push(decCol); - }), + this.cipherService.decrypt(cipher, activeUserId).then((decCipher) => { + if ( + !this.restrictedItemTypesService.isCipherRestricted(decCipher, restrictions) + ) { + decCiphers.push(decCipher); + } + }), ); }); - } - if (exportData.ciphers != null && exportData.ciphers.length > 0) { - exportData.ciphers - .filter((c) => c.deletedDate === null) - .forEach(async (c) => { - const cipher = new Cipher(new CipherData(c)); - exportPromises.push( - this.cipherService.decrypt(cipher, activeUserId).then((decCipher) => { - if ( - !this.restrictedItemTypesService.isCipherRestricted(decCipher, restrictions) - ) { - decCiphers.push(decCipher); - } - }), - ); - }); - } } - return Promise.all(exportPromises); - }), + } + return Promise.all(exportPromises); + }), ); await Promise.all(promises); @@ -191,15 +184,13 @@ export class OrganizationVaultExportService return this.buildJsonExport(decCollections, decCiphers); } - private async getOrganizationEncryptedExport(organizationId: string): Promise { + private async getOrganizationEncryptedExport(organizationId: OrganizationId): Promise { const collections: Collection[] = []; const ciphers: Cipher[] = []; const restrictions = await firstValueFrom(this.restrictedItemTypesService.restricted$); - const exportData = await this.vaultExportApiService.getOrganizationExport( - organizationId as OrganizationId, - ); + const exportData = await this.vaultExportApiService.getOrganizationExport(organizationId); if (exportData == null) { return; @@ -229,7 +220,7 @@ export class OrganizationVaultExportService private async getDecryptedManagedExport( activeUserId: UserId, - organizationId: string, + organizationId: OrganizationId, format: "json" | "csv", ): Promise { let decCiphers: CipherView[] = []; @@ -271,7 +262,7 @@ export class OrganizationVaultExportService private async getEncryptedManagedExport( activeUserId: UserId, - organizationId: string, + organizationId: OrganizationId, ): Promise { let encCiphers: Cipher[] = []; let allCiphers: Cipher[] = []; @@ -308,7 +299,7 @@ export class OrganizationVaultExportService } private async BuildEncryptedExport( - organizationId: string, + organizationId: OrganizationId, collections: Collection[], ciphers: Cipher[], ): Promise { diff --git a/libs/tools/export/vault-export/vault-export-core/src/services/vault-export.service.abstraction.ts b/libs/tools/export/vault-export/vault-export-core/src/services/vault-export.service.abstraction.ts index d41f1aa0a1..98eca52b3c 100644 --- a/libs/tools/export/vault-export/vault-export-core/src/services/vault-export.service.abstraction.ts +++ b/libs/tools/export/vault-export/vault-export-core/src/services/vault-export.service.abstraction.ts @@ -1,3 +1,5 @@ +import { OrganizationId } from "@bitwarden/common/types/guid"; + import { ExportedVault } from "../types"; export const EXPORT_FORMATS = ["csv", "json", "encrypted_json", "zip"] as const; @@ -6,7 +8,7 @@ export type ExportFormat = (typeof EXPORT_FORMATS)[number]; export abstract class VaultExportServiceAbstraction { abstract getExport: (format: ExportFormat, password: string) => Promise; abstract getOrganizationExport: ( - organizationId: string, + organizationId: OrganizationId, format: ExportFormat, password: string, onlyManagedCollections?: boolean, diff --git a/libs/tools/export/vault-export/vault-export-core/src/services/vault-export.service.ts b/libs/tools/export/vault-export/vault-export-core/src/services/vault-export.service.ts index 8af961e2be..9209587f4e 100644 --- a/libs/tools/export/vault-export/vault-export-core/src/services/vault-export.service.ts +++ b/libs/tools/export/vault-export/vault-export-core/src/services/vault-export.service.ts @@ -1,4 +1,5 @@ import { Utils } from "@bitwarden/common/platform/misc/utils"; +import { OrganizationId } from "@bitwarden/common/types/guid"; import { ExportedVault } from "../types"; @@ -42,7 +43,7 @@ export class VaultExportService implements VaultExportServiceAbstraction { * @throws Error if the organization policies prevent the export */ async getOrganizationExport( - organizationId: string, + organizationId: OrganizationId, format: ExportFormat, password: string, onlyManagedCollections = false, diff --git a/libs/tools/export/vault-export/vault-export-ui/src/components/export.component.ts b/libs/tools/export/vault-export/vault-export-ui/src/components/export.component.ts index 0b5d3d7083..550b421d27 100644 --- a/libs/tools/export/vault-export/vault-export-ui/src/components/export.component.ts +++ b/libs/tools/export/vault-export/vault-export-ui/src/components/export.component.ts @@ -29,10 +29,7 @@ import { JslibModule } from "@bitwarden/angular/jslib.module"; import { PasswordStrengthV2Component } from "@bitwarden/angular/tools/password-strength/password-strength-v2.component"; import { UserVerificationDialogComponent } from "@bitwarden/auth/angular"; import { EventCollectionService } from "@bitwarden/common/abstractions/event/event-collection.service"; -import { - getOrganizationById, - OrganizationService, -} from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; +import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; import { PolicyType } from "@bitwarden/common/admin-console/enums"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; @@ -42,8 +39,10 @@ import { EventType } from "@bitwarden/common/enums"; import { FileDownloadService } from "@bitwarden/common/platform/abstractions/file-download/file-download.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; +import { getById } from "@bitwarden/common/platform/misc"; import { Utils } from "@bitwarden/common/platform/misc/utils"; import { pin } from "@bitwarden/common/tools/rx"; +import { isId, OrganizationId } from "@bitwarden/common/types/guid"; import { AsyncActionsModule, BitSubmitDirective, @@ -84,9 +83,9 @@ import { ExportScopeCalloutComponent } from "./export-scope-callout.component"; ], }) export class ExportComponent implements OnInit, OnDestroy, AfterViewInit { - private _organizationId: string; + private _organizationId: OrganizationId | undefined; - get organizationId(): string { + get organizationId(): OrganizationId | undefined { return this._organizationId; } @@ -94,14 +93,23 @@ export class ExportComponent implements OnInit, OnDestroy, AfterViewInit { * Enables the hosting control to pass in an organizationId * If a organizationId is provided, the organization selection is disabled. */ - @Input() set organizationId(value: string) { + @Input() set organizationId(value: OrganizationId | string | undefined) { + if (Utils.isNullOrEmpty(value)) { + this._organizationId = undefined; + return; + } + + if (!isId(value)) { + this._organizationId = undefined; + return; + } + this._organizationId = value; + getUserId(this.accountService.activeAccount$) .pipe( switchMap((userId) => - this.organizationService - .organizations$(userId) - .pipe(getOrganizationById(this._organizationId)), + this.organizationService.organizations$(userId).pipe(getById(this._organizationId)), ), ) .pipe(takeUntil(this.destroy$)) @@ -133,11 +141,11 @@ export class ExportComponent implements OnInit, OnDestroy, AfterViewInit { /** * Emits when the creation and download of the export-file have succeeded - * - Emits an null/empty string when exporting from an individual vault + * - Emits an undefined when exporting from an individual vault * - Emits the organizationId when exporting from an organizationl vault * */ @Output() - onSuccessfulExport = new EventEmitter(); + onSuccessfulExport = new EventEmitter(); @ViewChild(PasswordStrengthV2Component) passwordStrengthComponent: PasswordStrengthV2Component;