From bb0a65f6d61f79fa1cf0629f784c27602badea6c Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Tue, 30 Apr 2024 02:49:10 +1000 Subject: [PATCH 1/8] [AC-2523] Fix broken members dialog for Manage Users custom permission (#8968) * Let Manage Users permission edit collection access * Remove unused comment --- .../member-dialog/member-dialog.component.ts | 5 +++-- .../src/app/vault/core/views/collection-admin.view.ts | 10 ++++++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.ts b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.ts index 3cccd6e28f..a67bea39c0 100644 --- a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.ts +++ b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.ts @@ -619,7 +619,7 @@ export class MemberDialogComponent implements OnDestroy { } function mapCollectionToAccessItemView( - collection: CollectionView, + collection: CollectionAdminView, organization: Organization, flexibleCollectionsV1Enabled: boolean, accessSelection?: CollectionAccessSelectionView, @@ -631,7 +631,8 @@ function mapCollectionToAccessItemView( labelName: collection.name, listName: collection.name, readonly: - group !== undefined || !collection.canEdit(organization, flexibleCollectionsV1Enabled), + group !== undefined || + !collection.canEditUserAccess(organization, flexibleCollectionsV1Enabled), readonlyPermission: accessSelection ? convertToPermission(accessSelection) : undefined, viaGroupName: group?.name, }; diff --git a/apps/web/src/app/vault/core/views/collection-admin.view.ts b/apps/web/src/app/vault/core/views/collection-admin.view.ts index d942d42fb8..369c78636c 100644 --- a/apps/web/src/app/vault/core/views/collection-admin.view.ts +++ b/apps/web/src/app/vault/core/views/collection-admin.view.ts @@ -31,6 +31,9 @@ export class CollectionAdminView extends CollectionView { this.assigned = response.assigned; } + /** + * Whether the current user can edit the collection, including user and group access + */ override canEdit(org: Organization, flexibleCollectionsV1Enabled: boolean): boolean { return org?.flexibleCollections ? org?.canEditAnyCollection(flexibleCollectionsV1Enabled) || this.manage @@ -43,4 +46,11 @@ export class CollectionAdminView extends CollectionView { ? org?.canDeleteAnyCollection || (!org?.limitCollectionCreationDeletion && this.manage) : org?.canDeleteAnyCollection || (org?.canDeleteAssignedCollections && this.assigned); } + + /** + * Whether the user can modify user access to this collection + */ + canEditUserAccess(org: Organization, flexibleCollectionsV1Enabled: boolean): boolean { + return this.canEdit(org, flexibleCollectionsV1Enabled) || org.canManageUsers; + } } From 443da7f62db23ab1ff762e0d8ac68946451db95e Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Mon, 29 Apr 2024 13:40:44 -0400 Subject: [PATCH 2/8] Fix Beta Version (#8944) * Fix Beta Version * Add Comments * Fix Function Reference --- apps/browser/gulpfile.js | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/apps/browser/gulpfile.js b/apps/browser/gulpfile.js index d5b29ffc38..3fe2c44dd1 100644 --- a/apps/browser/gulpfile.js +++ b/apps/browser/gulpfile.js @@ -30,6 +30,19 @@ const filters = { safari: ["!build/safari/**/*"], }; +/** + * Converts a number to a tuple containing two Uint16's + * @param num {number} This number is expected to be a integer style number with no decimals + * + * @returns {number[]} A tuple containing two elements that are both numbers. + */ +function numToUint16s(num) { + var arr = new ArrayBuffer(4); + var view = new DataView(arr); + view.setUint32(0, num, false); + return [view.getUint16(0), view.getUint16(2)]; +} + function buildString() { var build = ""; if (process.env.MANIFEST_VERSION) { @@ -258,8 +271,19 @@ function applyBetaLabels(manifest) { manifest.short_name = "Bitwarden BETA"; manifest.description = "THIS EXTENSION IS FOR BETA TESTING BITWARDEN."; if (process.env.GITHUB_RUN_ID) { - manifest.version_name = `${manifest.version} beta - ${process.env.GITHUB_SHA.slice(0, 8)}`; - manifest.version = `${manifest.version}.${parseInt(process.env.GITHUB_RUN_ID.slice(-4))}`; + const existingVersionParts = manifest.version.split("."); // 3 parts expected 2024.4.0 + + // GITHUB_RUN_ID is a number like: 8853654662 + // which will convert to [ 4024, 3206 ] + // and a single incremented id of 8853654663 will become [ 4024, 3207 ] + const runIdParts = numToUint16s(parseInt(process.env.GITHUB_RUN_ID)); + + // Only use the first 2 parts from the given version number and base the other 2 numbers from the GITHUB_RUN_ID + // Example: 2024.4.4024.3206 + const betaVersion = `${existingVersionParts[0]}.${existingVersionParts[1]}.${runIdParts[0]}.${runIdParts[1]}`; + + manifest.version_name = `${betaVersion} beta - ${process.env.GITHUB_SHA.slice(0, 8)}`; + manifest.version = betaVersion; } else { manifest.version = `${manifest.version}.0`; } From 82ae2fe62cc4a82e59dc594d810be51ab78fa19d Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Mon, 29 Apr 2024 15:57:37 -0400 Subject: [PATCH 3/8] [deps] Platform (CL): Update react monorepo (#8734) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --- package-lock.json | 36 ++++++++++++++++++------------------ package.json | 8 ++++---- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/package-lock.json b/package-lock.json index 4c652b7419..ba0119ca63 100644 --- a/package-lock.json +++ b/package-lock.json @@ -114,7 +114,7 @@ "@types/node-ipc": "9.2.3", "@types/papaparse": "5.3.14", "@types/proper-lockfile": "4.1.4", - "@types/react": "16.14.57", + "@types/react": "16.14.60", "@types/retry": "0.12.5", "@types/zxcvbn": "4.4.4", "@typescript-eslint/eslint-plugin": "7.7.1", @@ -163,8 +163,8 @@ "prettier": "3.2.2", "prettier-plugin-tailwindcss": "0.5.14", "process": "0.11.10", - "react": "18.2.0", - "react-dom": "18.2.0", + "react": "18.3.1", + "react-dom": "18.3.1", "regedit": "^3.0.3", "remark-gfm": "3.0.1", "rimraf": "5.0.5", @@ -10618,13 +10618,13 @@ "dev": true }, "node_modules/@types/react": { - "version": "16.14.57", - "resolved": "https://registry.npmjs.org/@types/react/-/react-16.14.57.tgz", - "integrity": "sha512-fuNq/GV1a6GgqSuVuC457vYeTbm4E1CUBQVZwSPxqYnRhIzSXCJ1gGqyv+PKhqLyfbKCga9dXHJDzv+4XE41fw==", + "version": "16.14.60", + "resolved": "https://registry.npmjs.org/@types/react/-/react-16.14.60.tgz", + "integrity": "sha512-wIFmnczGsTcgwCBeIYOuy2mdXEiKZ5znU/jNOnMZPQyCcIxauMGWlX0TNG4lZ7NxRKj7YUIZRneJQSSdB2jKgg==", "dev": true, "dependencies": { "@types/prop-types": "*", - "@types/scheduler": "*", + "@types/scheduler": "^0.16", "csstype": "^3.0.2" } }, @@ -32085,9 +32085,9 @@ } }, "node_modules/react": { - "version": "18.2.0", - "resolved": "https://registry.npmjs.org/react/-/react-18.2.0.tgz", - "integrity": "sha512-/3IjMdb2L9QbBdWiW5e3P2/npwMBaU9mHCSCUzNln0ZCYbcfTsGbTJrU/kGemdH2IWmB2ioZ+zkxtmq6g09fGQ==", + "version": "18.3.1", + "resolved": "https://registry.npmjs.org/react/-/react-18.3.1.tgz", + "integrity": "sha512-wS+hAgJShR0KhEvPJArfuPVN1+Hz1t0Y6n5jLrGQbkb4urgPE/0Rve+1kMB1v/oWgHgm4WIcV+i7F2pTVj+2iQ==", "dev": true, "dependencies": { "loose-envify": "^1.1.0" @@ -32107,16 +32107,16 @@ } }, "node_modules/react-dom": { - "version": "18.2.0", - "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-18.2.0.tgz", - "integrity": "sha512-6IMTriUmvsjHUjNtEDudZfuDQUoWXVxKHhlEGSk81n4YFS+r/Kl99wXiwlVXtPBtJenozv2P+hxDsw9eA7Xo6g==", + "version": "18.3.1", + "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-18.3.1.tgz", + "integrity": "sha512-5m4nQKp+rZRb09LNH59GM4BxTh9251/ylbKIbpe7TpGxfJ+9kv6BLkLBXIjjspbgbnIBNqlI23tRnTWT0snUIw==", "dev": true, "dependencies": { "loose-envify": "^1.1.0", - "scheduler": "^0.23.0" + "scheduler": "^0.23.2" }, "peerDependencies": { - "react": "^18.2.0" + "react": "^18.3.1" } }, "node_modules/react-is": { @@ -33604,9 +33604,9 @@ } }, "node_modules/scheduler": { - "version": "0.23.0", - "resolved": "https://registry.npmjs.org/scheduler/-/scheduler-0.23.0.tgz", - "integrity": "sha512-CtuThmgHNg7zIZWAXi3AsyIzA3n4xx7aNyjwC2VJldO2LMVDhFK+63xGqq6CsJH4rTAt6/M+N4GhZiDYPx9eUw==", + "version": "0.23.2", + "resolved": "https://registry.npmjs.org/scheduler/-/scheduler-0.23.2.tgz", + "integrity": "sha512-UOShsPwz7NrMUqhR6t0hWjFduvOzbtv7toDH1/hIrfRNIDBnnBWd0CwJTGvTpngVlmwGCdP9/Zl/tVrDqcuYzQ==", "dev": true, "dependencies": { "loose-envify": "^1.1.0" diff --git a/package.json b/package.json index 6c1fac2e54..c73ae492f5 100644 --- a/package.json +++ b/package.json @@ -75,7 +75,7 @@ "@types/node-ipc": "9.2.3", "@types/papaparse": "5.3.14", "@types/proper-lockfile": "4.1.4", - "@types/react": "16.14.57", + "@types/react": "16.14.60", "@types/retry": "0.12.5", "@types/zxcvbn": "4.4.4", "@typescript-eslint/eslint-plugin": "7.7.1", @@ -124,8 +124,8 @@ "prettier": "3.2.2", "prettier-plugin-tailwindcss": "0.5.14", "process": "0.11.10", - "react": "18.2.0", - "react-dom": "18.2.0", + "react": "18.3.1", + "react-dom": "18.3.1", "regedit": "^3.0.3", "remark-gfm": "3.0.1", "rimraf": "5.0.5", @@ -213,7 +213,7 @@ "replacestream": "4.0.3" }, "resolutions": { - "@types/react": "16.14.57" + "@types/react": "16.14.60" }, "lint-staged": { "*": "prettier --cache --ignore-unknown --write", From 20de053770a9411c46e1a675d0ed53b551297ac0 Mon Sep 17 00:00:00 2001 From: Jared Snider <116684653+JaredSnider-Bitwarden@users.noreply.github.com> Date: Mon, 29 Apr 2024 17:43:14 -0400 Subject: [PATCH 4/8] Auth/PM-7811 - Refactor User Auto Unlock Key Hydration Process To Remove Race Conditions (#8979) * PM-7811 - Refactor UserKeyInitService to UserAutoUnlockKeyService - remove active account listening logic as it introduced race conditions with user key memory retrieval happening before the user auto unlock key was set into memory. * PM-7811 - CLI - (1) Fix deps (2) On CLI init (pre command execution), if there is an active account, then set the user key in memory from the user auto unlock key. * PM-7811 - Browser Extension / desktop - (1) Update deps (2) Sets user key in memory if the auto unlock key is set on account switch and background init (must act on all accounts so that account switcher displays unlock status properly). * PM-7811 - Web - (1) Update deps (2) Sets user key in memory if the auto unlock key is set on init * PM-7811 - Fix account switcher service changes not being necessary. --- .../browser/src/background/main.background.ts | 23 ++- apps/cli/src/bw.ts | 17 +- apps/desktop/src/app/services/init.service.ts | 21 ++- apps/web/src/app/core/init.service.ts | 15 +- .../src/services/jslib-services.module.ts | 9 +- .../user-auto-unlock-key.service.spec.ts | 71 ++++++++ .../services/user-auto-unlock-key.service.ts | 36 ++++ .../services/user-key-init.service.spec.ts | 162 ------------------ .../services/user-key-init.service.ts | 57 ------ 9 files changed, 165 insertions(+), 246 deletions(-) create mode 100644 libs/common/src/platform/services/user-auto-unlock-key.service.spec.ts create mode 100644 libs/common/src/platform/services/user-auto-unlock-key.service.ts delete mode 100644 libs/common/src/platform/services/user-key-init.service.spec.ts delete mode 100644 libs/common/src/platform/services/user-key-init.service.ts diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 5aec6e01a4..2dc229e402 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -113,7 +113,7 @@ import { MemoryStorageService } from "@bitwarden/common/platform/services/memory import { MigrationBuilderService } from "@bitwarden/common/platform/services/migration-builder.service"; import { MigrationRunner } from "@bitwarden/common/platform/services/migration-runner"; import { SystemService } from "@bitwarden/common/platform/services/system.service"; -import { UserKeyInitService } from "@bitwarden/common/platform/services/user-key-init.service"; +import { UserAutoUnlockKeyService } from "@bitwarden/common/platform/services/user-auto-unlock-key.service"; import { WebCryptoFunctionService } from "@bitwarden/common/platform/services/web-crypto-function.service"; import { ActiveUserStateProvider, @@ -334,7 +334,7 @@ export default class MainBackground { billingAccountProfileStateService: BillingAccountProfileStateService; // eslint-disable-next-line rxjs/no-exposed-subjects -- Needed to give access to services module intraprocessMessagingSubject: Subject>; - userKeyInitService: UserKeyInitService; + userAutoUnlockKeyService: UserAutoUnlockKeyService; scriptInjectorService: BrowserScriptInjectorService; kdfConfigService: kdfConfigServiceAbstraction; @@ -1064,11 +1064,7 @@ export default class MainBackground { } } - this.userKeyInitService = new UserKeyInitService( - this.accountService, - this.cryptoService, - this.logService, - ); + this.userAutoUnlockKeyService = new UserAutoUnlockKeyService(this.cryptoService); } async bootstrap() { @@ -1079,7 +1075,18 @@ export default class MainBackground { // This is here instead of in in the InitService b/c we don't plan for // side effects to run in the Browser InitService. - this.userKeyInitService.listenForActiveUserChangesToSetUserKey(); + const accounts = await firstValueFrom(this.accountService.accounts$); + + const setUserKeyInMemoryPromises = []; + for (const userId of Object.keys(accounts) as UserId[]) { + // For each acct, we must await the process of setting the user key in memory + // if the auto user key is set to avoid race conditions of any code trying to access + // the user key from mem. + setUserKeyInMemoryPromises.push( + this.userAutoUnlockKeyService.setUserKeyInMemoryIfAutoUserKeySet(userId), + ); + } + await Promise.all(setUserKeyInMemoryPromises); await (this.i18nService as I18nService).init(); (this.eventUploadService as EventUploadService).init(true); diff --git a/apps/cli/src/bw.ts b/apps/cli/src/bw.ts index b3fb68fe63..4c2066dbf1 100644 --- a/apps/cli/src/bw.ts +++ b/apps/cli/src/bw.ts @@ -3,6 +3,7 @@ import * as path from "path"; import { program } from "commander"; import * as jsdom from "jsdom"; +import { firstValueFrom } from "rxjs"; import { InternalUserDecryptionOptionsServiceAbstraction, @@ -79,7 +80,7 @@ import { MigrationBuilderService } from "@bitwarden/common/platform/services/mig import { MigrationRunner } from "@bitwarden/common/platform/services/migration-runner"; import { StateService } from "@bitwarden/common/platform/services/state.service"; import { StorageServiceProvider } from "@bitwarden/common/platform/services/storage-service.provider"; -import { UserKeyInitService } from "@bitwarden/common/platform/services/user-key-init.service"; +import { UserAutoUnlockKeyService } from "@bitwarden/common/platform/services/user-auto-unlock-key.service"; import { ActiveUserStateProvider, DerivedStateProvider, @@ -236,7 +237,7 @@ export class Main { biometricStateService: BiometricStateService; billingAccountProfileStateService: BillingAccountProfileStateService; providerApiService: ProviderApiServiceAbstraction; - userKeyInitService: UserKeyInitService; + userAutoUnlockKeyService: UserAutoUnlockKeyService; kdfConfigService: KdfConfigServiceAbstraction; constructor() { @@ -709,11 +710,7 @@ export class Main { this.providerApiService = new ProviderApiService(this.apiService); - this.userKeyInitService = new UserKeyInitService( - this.accountService, - this.cryptoService, - this.logService, - ); + this.userAutoUnlockKeyService = new UserAutoUnlockKeyService(this.cryptoService); } async run() { @@ -757,7 +754,11 @@ export class Main { this.containerService.attachToGlobal(global); await this.i18nService.init(); this.twoFactorService.init(); - this.userKeyInitService.listenForActiveUserChangesToSetUserKey(); + + const activeAccount = await firstValueFrom(this.accountService.activeAccount$); + if (activeAccount) { + await this.userAutoUnlockKeyService.setUserKeyInMemoryIfAutoUserKeySet(activeAccount.id); + } } } diff --git a/apps/desktop/src/app/services/init.service.ts b/apps/desktop/src/app/services/init.service.ts index ae2e1ba97c..0452e9be83 100644 --- a/apps/desktop/src/app/services/init.service.ts +++ b/apps/desktop/src/app/services/init.service.ts @@ -1,10 +1,12 @@ import { DOCUMENT } from "@angular/common"; import { Inject, Injectable } from "@angular/core"; +import { firstValueFrom } from "rxjs"; import { AbstractThemingService } from "@bitwarden/angular/platform/services/theming/theming.service.abstraction"; import { WINDOW } from "@bitwarden/angular/services/injection-tokens"; import { EventUploadService as EventUploadServiceAbstraction } from "@bitwarden/common/abstractions/event/event-upload.service"; import { NotificationsService as NotificationsServiceAbstraction } from "@bitwarden/common/abstractions/notifications.service"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { TwoFactorService as TwoFactorServiceAbstraction } from "@bitwarden/common/auth/abstractions/two-factor.service"; import { CryptoService as CryptoServiceAbstraction } from "@bitwarden/common/platform/abstractions/crypto.service"; import { EncryptService } from "@bitwarden/common/platform/abstractions/encrypt.service"; @@ -12,9 +14,10 @@ import { I18nService as I18nServiceAbstraction } from "@bitwarden/common/platfor import { PlatformUtilsService as PlatformUtilsServiceAbstraction } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { StateService as StateServiceAbstraction } from "@bitwarden/common/platform/abstractions/state.service"; import { ContainerService } from "@bitwarden/common/platform/services/container.service"; -import { UserKeyInitService } from "@bitwarden/common/platform/services/user-key-init.service"; +import { UserAutoUnlockKeyService } from "@bitwarden/common/platform/services/user-auto-unlock-key.service"; import { EventUploadService } from "@bitwarden/common/services/event/event-upload.service"; import { VaultTimeoutService } from "@bitwarden/common/services/vault-timeout/vault-timeout.service"; +import { UserId } from "@bitwarden/common/types/guid"; import { SyncService as SyncServiceAbstraction } from "@bitwarden/common/vault/abstractions/sync/sync.service.abstraction"; import { I18nRendererService } from "../../platform/services/i18n.renderer.service"; @@ -36,7 +39,8 @@ export class InitService { private nativeMessagingService: NativeMessagingService, private themingService: AbstractThemingService, private encryptService: EncryptService, - private userKeyInitService: UserKeyInitService, + private userAutoUnlockKeyService: UserAutoUnlockKeyService, + private accountService: AccountService, @Inject(DOCUMENT) private document: Document, ) {} @@ -44,7 +48,18 @@ export class InitService { return async () => { this.nativeMessagingService.init(); await this.stateService.init({ runMigrations: false }); // Desktop will run them in main process - this.userKeyInitService.listenForActiveUserChangesToSetUserKey(); + + const accounts = await firstValueFrom(this.accountService.accounts$); + const setUserKeyInMemoryPromises = []; + for (const userId of Object.keys(accounts) as UserId[]) { + // For each acct, we must await the process of setting the user key in memory + // if the auto user key is set to avoid race conditions of any code trying to access + // the user key from mem. + setUserKeyInMemoryPromises.push( + this.userAutoUnlockKeyService.setUserKeyInMemoryIfAutoUserKeySet(userId), + ); + } + await Promise.all(setUserKeyInMemoryPromises); // 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 diff --git a/apps/web/src/app/core/init.service.ts b/apps/web/src/app/core/init.service.ts index dab6ed5e3d..55dc1544ff 100644 --- a/apps/web/src/app/core/init.service.ts +++ b/apps/web/src/app/core/init.service.ts @@ -1,17 +1,19 @@ import { DOCUMENT } from "@angular/common"; import { Inject, Injectable } from "@angular/core"; +import { firstValueFrom } from "rxjs"; import { AbstractThemingService } from "@bitwarden/angular/platform/services/theming/theming.service.abstraction"; import { WINDOW } from "@bitwarden/angular/services/injection-tokens"; import { EventUploadService as EventUploadServiceAbstraction } from "@bitwarden/common/abstractions/event/event-upload.service"; import { NotificationsService as NotificationsServiceAbstraction } from "@bitwarden/common/abstractions/notifications.service"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { TwoFactorService as TwoFactorServiceAbstraction } from "@bitwarden/common/auth/abstractions/two-factor.service"; import { CryptoService as CryptoServiceAbstraction } from "@bitwarden/common/platform/abstractions/crypto.service"; import { EncryptService } from "@bitwarden/common/platform/abstractions/encrypt.service"; import { I18nService as I18nServiceAbstraction } from "@bitwarden/common/platform/abstractions/i18n.service"; import { StateService as StateServiceAbstraction } from "@bitwarden/common/platform/abstractions/state.service"; import { ContainerService } from "@bitwarden/common/platform/services/container.service"; -import { UserKeyInitService } from "@bitwarden/common/platform/services/user-key-init.service"; +import { UserAutoUnlockKeyService } from "@bitwarden/common/platform/services/user-auto-unlock-key.service"; import { EventUploadService } from "@bitwarden/common/services/event/event-upload.service"; import { VaultTimeoutService } from "@bitwarden/common/services/vault-timeout/vault-timeout.service"; @@ -28,14 +30,21 @@ export class InitService { private cryptoService: CryptoServiceAbstraction, private themingService: AbstractThemingService, private encryptService: EncryptService, - private userKeyInitService: UserKeyInitService, + private userAutoUnlockKeyService: UserAutoUnlockKeyService, + private accountService: AccountService, @Inject(DOCUMENT) private document: Document, ) {} init() { return async () => { await this.stateService.init(); - this.userKeyInitService.listenForActiveUserChangesToSetUserKey(); + + const activeAccount = await firstValueFrom(this.accountService.activeAccount$); + if (activeAccount) { + // If there is an active account, we must await the process of setting the user key in memory + // if the auto user key is set to avoid race conditions of any code trying to access the user key from mem. + await this.userAutoUnlockKeyService.setUserKeyInMemoryIfAutoUserKeySet(activeAccount.id); + } setTimeout(() => this.notificationsService.init(), 3000); await this.vaultTimeoutService.init(true); diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index 2f6167d676..46d739d0f5 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -53,7 +53,6 @@ import { ProviderApiService } from "@bitwarden/common/admin-console/services/pro import { ProviderService } from "@bitwarden/common/admin-console/services/provider.service"; import { AccountApiService as AccountApiServiceAbstraction } from "@bitwarden/common/auth/abstractions/account-api.service"; import { - AccountService, AccountService as AccountServiceAbstraction, InternalAccountService, } from "@bitwarden/common/auth/abstractions/account.service"; @@ -162,7 +161,7 @@ import { MigrationRunner } from "@bitwarden/common/platform/services/migration-r import { NoopNotificationsService } from "@bitwarden/common/platform/services/noop-notifications.service"; import { StateService } from "@bitwarden/common/platform/services/state.service"; import { StorageServiceProvider } from "@bitwarden/common/platform/services/storage-service.provider"; -import { UserKeyInitService } from "@bitwarden/common/platform/services/user-key-init.service"; +import { UserAutoUnlockKeyService } from "@bitwarden/common/platform/services/user-auto-unlock-key.service"; import { ValidationService } from "@bitwarden/common/platform/services/validation.service"; import { WebCryptoFunctionService } from "@bitwarden/common/platform/services/web-crypto-function.service"; import { @@ -1128,9 +1127,9 @@ const safeProviders: SafeProvider[] = [ deps: [StateProvider], }), safeProvider({ - provide: UserKeyInitService, - useClass: UserKeyInitService, - deps: [AccountService, CryptoServiceAbstraction, LogService], + provide: UserAutoUnlockKeyService, + useClass: UserAutoUnlockKeyService, + deps: [CryptoServiceAbstraction], }), safeProvider({ provide: ErrorHandler, diff --git a/libs/common/src/platform/services/user-auto-unlock-key.service.spec.ts b/libs/common/src/platform/services/user-auto-unlock-key.service.spec.ts new file mode 100644 index 0000000000..f0d60158c1 --- /dev/null +++ b/libs/common/src/platform/services/user-auto-unlock-key.service.spec.ts @@ -0,0 +1,71 @@ +import { mock } from "jest-mock-extended"; + +import { CsprngArray } from "../../types/csprng"; +import { UserId } from "../../types/guid"; +import { UserKey } from "../../types/key"; +import { KeySuffixOptions } from "../enums"; +import { Utils } from "../misc/utils"; +import { SymmetricCryptoKey } from "../models/domain/symmetric-crypto-key"; + +import { CryptoService } from "./crypto.service"; +import { UserAutoUnlockKeyService } from "./user-auto-unlock-key.service"; + +describe("UserAutoUnlockKeyService", () => { + let userAutoUnlockKeyService: UserAutoUnlockKeyService; + + const mockUserId = Utils.newGuid() as UserId; + + const cryptoService = mock(); + + beforeEach(() => { + userAutoUnlockKeyService = new UserAutoUnlockKeyService(cryptoService); + }); + + describe("setUserKeyInMemoryIfAutoUserKeySet", () => { + it("does nothing if the userId is null", async () => { + // Act + await (userAutoUnlockKeyService as any).setUserKeyInMemoryIfAutoUserKeySet(null); + + // Assert + expect(cryptoService.getUserKeyFromStorage).not.toHaveBeenCalled(); + expect(cryptoService.setUserKey).not.toHaveBeenCalled(); + }); + + it("does nothing if the autoUserKey is null", async () => { + // Arrange + const userId = mockUserId; + + cryptoService.getUserKeyFromStorage.mockResolvedValue(null); + + // Act + await (userAutoUnlockKeyService as any).setUserKeyInMemoryIfAutoUserKeySet(userId); + + // Assert + expect(cryptoService.getUserKeyFromStorage).toHaveBeenCalledWith( + KeySuffixOptions.Auto, + userId, + ); + expect(cryptoService.setUserKey).not.toHaveBeenCalled(); + }); + + it("sets the user key in memory if the autoUserKey is not null", async () => { + // Arrange + const userId = mockUserId; + + const mockRandomBytes = new Uint8Array(64) as CsprngArray; + const mockAutoUserKey: UserKey = new SymmetricCryptoKey(mockRandomBytes) as UserKey; + + cryptoService.getUserKeyFromStorage.mockResolvedValue(mockAutoUserKey); + + // Act + await (userAutoUnlockKeyService as any).setUserKeyInMemoryIfAutoUserKeySet(userId); + + // Assert + expect(cryptoService.getUserKeyFromStorage).toHaveBeenCalledWith( + KeySuffixOptions.Auto, + userId, + ); + expect(cryptoService.setUserKey).toHaveBeenCalledWith(mockAutoUserKey, userId); + }); + }); +}); diff --git a/libs/common/src/platform/services/user-auto-unlock-key.service.ts b/libs/common/src/platform/services/user-auto-unlock-key.service.ts new file mode 100644 index 0000000000..6c8ce3f048 --- /dev/null +++ b/libs/common/src/platform/services/user-auto-unlock-key.service.ts @@ -0,0 +1,36 @@ +import { UserId } from "../../types/guid"; +import { CryptoService } from "../abstractions/crypto.service"; +import { KeySuffixOptions } from "../enums"; + +// TODO: this is a half measure improvement which allows us to reduce some side effects today (cryptoService.getUserKey setting user key in memory if auto key exists) +// but ideally, in the future, we would be able to put this logic into the cryptoService +// after the vault timeout settings service is transitioned to state provider so that +// the getUserKey logic can simply go to the correct location based on the vault timeout settings +// similar to the TokenService (it would either go to secure storage for the auto user key or memory for the user key) + +export class UserAutoUnlockKeyService { + constructor(private cryptoService: CryptoService) {} + + /** + * The presence of the user key in memory dictates whether the user's vault is locked or unlocked. + * However, for users that have the auto unlock user key set, we need to set the user key in memory + * on application bootstrap and on active account changes so that the user's vault loads unlocked. + * @param userId - The user id to check for an auto user key. + */ + async setUserKeyInMemoryIfAutoUserKeySet(userId: UserId): Promise { + if (userId == null) { + return; + } + + const autoUserKey = await this.cryptoService.getUserKeyFromStorage( + KeySuffixOptions.Auto, + userId, + ); + + if (autoUserKey == null) { + return; + } + + await this.cryptoService.setUserKey(autoUserKey, userId); + } +} diff --git a/libs/common/src/platform/services/user-key-init.service.spec.ts b/libs/common/src/platform/services/user-key-init.service.spec.ts deleted file mode 100644 index 567320ded6..0000000000 --- a/libs/common/src/platform/services/user-key-init.service.spec.ts +++ /dev/null @@ -1,162 +0,0 @@ -import { mock } from "jest-mock-extended"; - -import { FakeAccountService, mockAccountServiceWith } from "../../../spec"; -import { CsprngArray } from "../../types/csprng"; -import { UserId } from "../../types/guid"; -import { UserKey } from "../../types/key"; -import { LogService } from "../abstractions/log.service"; -import { KeySuffixOptions } from "../enums"; -import { Utils } from "../misc/utils"; -import { SymmetricCryptoKey } from "../models/domain/symmetric-crypto-key"; - -import { CryptoService } from "./crypto.service"; -import { UserKeyInitService } from "./user-key-init.service"; - -describe("UserKeyInitService", () => { - let userKeyInitService: UserKeyInitService; - - const mockUserId = Utils.newGuid() as UserId; - - const accountService: FakeAccountService = mockAccountServiceWith(mockUserId); - - const cryptoService = mock(); - const logService = mock(); - - beforeEach(() => { - userKeyInitService = new UserKeyInitService(accountService, cryptoService, logService); - }); - - describe("listenForActiveUserChangesToSetUserKey()", () => { - it("calls setUserKeyInMemoryIfAutoUserKeySet if there is an active user", () => { - // Arrange - accountService.activeAccountSubject.next({ - id: mockUserId, - name: "name", - email: "email", - }); - - (userKeyInitService as any).setUserKeyInMemoryIfAutoUserKeySet = jest.fn(); - - // Act - - const subscription = userKeyInitService.listenForActiveUserChangesToSetUserKey(); - - // Assert - - expect(subscription).not.toBeFalsy(); - - expect((userKeyInitService as any).setUserKeyInMemoryIfAutoUserKeySet).toHaveBeenCalledWith( - mockUserId, - ); - }); - - it("calls setUserKeyInMemoryIfAutoUserKeySet if there is an active user and tracks subsequent emissions", () => { - // Arrange - accountService.activeAccountSubject.next({ - id: mockUserId, - name: "name", - email: "email", - }); - - const mockUser2Id = Utils.newGuid() as UserId; - - jest - .spyOn(userKeyInitService as any, "setUserKeyInMemoryIfAutoUserKeySet") - .mockImplementation(() => Promise.resolve()); - - // Act - - const subscription = userKeyInitService.listenForActiveUserChangesToSetUserKey(); - - accountService.activeAccountSubject.next({ - id: mockUser2Id, - name: "name", - email: "email", - }); - - // Assert - - expect(subscription).not.toBeFalsy(); - - expect((userKeyInitService as any).setUserKeyInMemoryIfAutoUserKeySet).toHaveBeenCalledTimes( - 2, - ); - - expect( - (userKeyInitService as any).setUserKeyInMemoryIfAutoUserKeySet, - ).toHaveBeenNthCalledWith(1, mockUserId); - expect( - (userKeyInitService as any).setUserKeyInMemoryIfAutoUserKeySet, - ).toHaveBeenNthCalledWith(2, mockUser2Id); - - subscription.unsubscribe(); - }); - - it("does not call setUserKeyInMemoryIfAutoUserKeySet if there is not an active user", () => { - // Arrange - accountService.activeAccountSubject.next(null); - - (userKeyInitService as any).setUserKeyInMemoryIfAutoUserKeySet = jest.fn(); - - // Act - - const subscription = userKeyInitService.listenForActiveUserChangesToSetUserKey(); - - // Assert - - expect(subscription).not.toBeFalsy(); - - expect( - (userKeyInitService as any).setUserKeyInMemoryIfAutoUserKeySet, - ).not.toHaveBeenCalledWith(mockUserId); - }); - }); - - describe("setUserKeyInMemoryIfAutoUserKeySet", () => { - it("does nothing if the userId is null", async () => { - // Act - await (userKeyInitService as any).setUserKeyInMemoryIfAutoUserKeySet(null); - - // Assert - expect(cryptoService.getUserKeyFromStorage).not.toHaveBeenCalled(); - expect(cryptoService.setUserKey).not.toHaveBeenCalled(); - }); - - it("does nothing if the autoUserKey is null", async () => { - // Arrange - const userId = mockUserId; - - cryptoService.getUserKeyFromStorage.mockResolvedValue(null); - - // Act - await (userKeyInitService as any).setUserKeyInMemoryIfAutoUserKeySet(userId); - - // Assert - expect(cryptoService.getUserKeyFromStorage).toHaveBeenCalledWith( - KeySuffixOptions.Auto, - userId, - ); - expect(cryptoService.setUserKey).not.toHaveBeenCalled(); - }); - - it("sets the user key in memory if the autoUserKey is not null", async () => { - // Arrange - const userId = mockUserId; - - const mockRandomBytes = new Uint8Array(64) as CsprngArray; - const mockAutoUserKey: UserKey = new SymmetricCryptoKey(mockRandomBytes) as UserKey; - - cryptoService.getUserKeyFromStorage.mockResolvedValue(mockAutoUserKey); - - // Act - await (userKeyInitService as any).setUserKeyInMemoryIfAutoUserKeySet(userId); - - // Assert - expect(cryptoService.getUserKeyFromStorage).toHaveBeenCalledWith( - KeySuffixOptions.Auto, - userId, - ); - expect(cryptoService.setUserKey).toHaveBeenCalledWith(mockAutoUserKey, userId); - }); - }); -}); diff --git a/libs/common/src/platform/services/user-key-init.service.ts b/libs/common/src/platform/services/user-key-init.service.ts deleted file mode 100644 index 1f6aacce8f..0000000000 --- a/libs/common/src/platform/services/user-key-init.service.ts +++ /dev/null @@ -1,57 +0,0 @@ -import { EMPTY, Subscription, catchError, filter, from, switchMap } from "rxjs"; - -import { AccountService } from "../../auth/abstractions/account.service"; -import { UserId } from "../../types/guid"; -import { CryptoService } from "../abstractions/crypto.service"; -import { LogService } from "../abstractions/log.service"; -import { KeySuffixOptions } from "../enums"; - -// TODO: this is a half measure improvement which allows us to reduce some side effects today (cryptoService.getUserKey setting user key in memory if auto key exists) -// but ideally, in the future, we would be able to put this logic into the cryptoService -// after the vault timeout settings service is transitioned to state provider so that -// the getUserKey logic can simply go to the correct location based on the vault timeout settings -// similar to the TokenService (it would either go to secure storage for the auto user key or memory for the user key) - -export class UserKeyInitService { - constructor( - private accountService: AccountService, - private cryptoService: CryptoService, - private logService: LogService, - ) {} - - // Note: must listen for changes to support account switching - listenForActiveUserChangesToSetUserKey(): Subscription { - return this.accountService.activeAccount$ - .pipe( - filter((activeAccount) => activeAccount != null), - switchMap((activeAccount) => - from(this.setUserKeyInMemoryIfAutoUserKeySet(activeAccount?.id)).pipe( - catchError((err: unknown) => { - this.logService.warning( - `setUserKeyInMemoryIfAutoUserKeySet failed with error: ${err}`, - ); - // Returning EMPTY to protect observable chain from cancellation in case of error - return EMPTY; - }), - ), - ), - ) - .subscribe(); - } - - private async setUserKeyInMemoryIfAutoUserKeySet(userId: UserId) { - if (userId == null) { - return; - } - - const autoUserKey = await this.cryptoService.getUserKeyFromStorage( - KeySuffixOptions.Auto, - userId, - ); - if (autoUserKey == null) { - return; - } - - await this.cryptoService.setUserKey(autoUserKey, userId); - } -} From 61d079cc3485799e1396eef75b94b90a27967114 Mon Sep 17 00:00:00 2001 From: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com> Date: Mon, 29 Apr 2024 17:30:18 -0500 Subject: [PATCH 5/8] [SM-1168] Update access policy selector to disable on submit (#8519) * Add loading and disabled on all inputs * Add proper spinner and form disable on submit --- .../project/project-people.component.html | 8 ++++++- .../project/project-people.component.ts | 22 +++++++++---------- .../service-account-people.component.html | 8 ++++++- .../service-account-people.component.ts | 22 ++++++++----------- .../access-policy-selector.component.html | 10 ++++++++- 5 files changed, 43 insertions(+), 27 deletions(-) diff --git a/bitwarden_license/bit-web/src/app/secrets-manager/projects/project/project-people.component.html b/bitwarden_license/bit-web/src/app/secrets-manager/projects/project/project-people.component.html index 9a47f42686..3f107486e2 100644 --- a/bitwarden_license/bit-web/src/app/secrets-manager/projects/project/project-people.component.html +++ b/bitwarden_license/bit-web/src/app/secrets-manager/projects/project/project-people.component.html @@ -1,4 +1,4 @@ -
+

{{ "projectPeopleDescription" | i18n }} @@ -19,3 +19,9 @@

+ + +
+ +
+ diff --git a/bitwarden_license/bit-web/src/app/secrets-manager/projects/project/project-people.component.ts b/bitwarden_license/bit-web/src/app/secrets-manager/projects/project/project-people.component.ts index b52a8938b4..835d3825a0 100644 --- a/bitwarden_license/bit-web/src/app/secrets-manager/projects/project/project-people.component.ts +++ b/bitwarden_license/bit-web/src/app/secrets-manager/projects/project/project-people.component.ts @@ -1,7 +1,7 @@ import { ChangeDetectorRef, Component, OnDestroy, OnInit } from "@angular/core"; import { FormControl, FormGroup } from "@angular/forms"; import { ActivatedRoute, Router } from "@angular/router"; -import { combineLatest, Subject, switchMap, takeUntil, catchError, EMPTY } from "rxjs"; +import { combineLatest, Subject, switchMap, takeUntil, catchError } from "rxjs"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; @@ -37,11 +37,9 @@ export class ProjectPeopleComponent implements OnInit, OnDestroy { return convertToAccessPolicyItemViews(policies); }), ), - 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"]); - return EMPTY; + catchError(async () => { + await this.router.navigate(["/sm", this.organizationId, "projects"]); + return []; }), ); @@ -99,17 +97,20 @@ export class ProjectPeopleComponent implements OnInit, OnDestroy { if (this.formGroup.invalid) { return; } + const formValues = this.formGroup.value.accessPolicies; + this.formGroup.disable(); const showAccessRemovalWarning = await this.accessPolicySelectorService.showAccessRemovalWarning( this.organizationId, - this.formGroup.value.accessPolicies, + formValues, ); if (showAccessRemovalWarning) { const confirmed = await this.showWarning(); if (!confirmed) { this.setSelected(this.currentAccessPolicies); + this.formGroup.enable(); return; } } @@ -117,7 +118,7 @@ export class ProjectPeopleComponent implements OnInit, OnDestroy { try { const projectPeopleView = convertToProjectPeopleAccessPoliciesView( this.projectId, - this.formGroup.value.accessPolicies, + formValues, ); const peoplePoliciesViews = await this.accessPolicyService.putProjectPeopleAccessPolicies( this.projectId, @@ -126,9 +127,7 @@ export class ProjectPeopleComponent implements OnInit, OnDestroy { this.currentAccessPolicies = convertToAccessPolicyItemViews(peoplePoliciesViews); if (showAccessRemovalWarning) { - // 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"]); + await this.router.navigate(["sm", this.organizationId, "projects"]); } this.platformUtilsService.showToast( "success", @@ -139,6 +138,7 @@ export class ProjectPeopleComponent implements OnInit, OnDestroy { this.validationService.showError(e); this.setSelected(this.currentAccessPolicies); } + this.formGroup.enable(); }; private setSelected(policiesToSelect: ApItemViewType[]) { diff --git a/bitwarden_license/bit-web/src/app/secrets-manager/service-accounts/people/service-account-people.component.html b/bitwarden_license/bit-web/src/app/secrets-manager/service-accounts/people/service-account-people.component.html index 074fa8ca00..96f7ae4d2b 100644 --- a/bitwarden_license/bit-web/src/app/secrets-manager/service-accounts/people/service-account-people.component.html +++ b/bitwarden_license/bit-web/src/app/secrets-manager/service-accounts/people/service-account-people.component.html @@ -1,4 +1,4 @@ -
+

{{ "machineAccountPeopleDescription" | i18n }} @@ -20,3 +20,9 @@

+ + +
+ +
+ diff --git a/bitwarden_license/bit-web/src/app/secrets-manager/service-accounts/people/service-account-people.component.ts b/bitwarden_license/bit-web/src/app/secrets-manager/service-accounts/people/service-account-people.component.ts index aeb124aa6a..a3d3984ea8 100644 --- a/bitwarden_license/bit-web/src/app/secrets-manager/service-accounts/people/service-account-people.component.ts +++ b/bitwarden_license/bit-web/src/app/secrets-manager/service-accounts/people/service-account-people.component.ts @@ -1,7 +1,7 @@ import { ChangeDetectorRef, Component, OnDestroy, OnInit } from "@angular/core"; import { FormControl, FormGroup } from "@angular/forms"; import { ActivatedRoute, Router } from "@angular/router"; -import { catchError, combineLatest, EMPTY, Subject, switchMap, takeUntil } from "rxjs"; +import { combineLatest, Subject, switchMap, takeUntil } from "rxjs"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; @@ -40,12 +40,6 @@ export class ServiceAccountPeopleComponent implements OnInit, OnDestroy { return convertToAccessPolicyItemViews(policies); }), ), - 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"]); - return EMPTY; - }), ); private potentialGrantees$ = combineLatest([this.route.params]).pipe( @@ -101,29 +95,32 @@ export class ServiceAccountPeopleComponent implements OnInit, OnDestroy { if (this.isFormInvalid()) { return; } + const formValues = this.formGroup.value.accessPolicies; + this.formGroup.disable(); const showAccessRemovalWarning = await this.accessPolicySelectorService.showAccessRemovalWarning( this.organizationId, - this.formGroup.value.accessPolicies, + formValues, ); if ( await this.handleAccessRemovalWarning(showAccessRemovalWarning, this.currentAccessPolicies) ) { + this.formGroup.enable(); return; } try { const peoplePoliciesViews = await this.updateServiceAccountPeopleAccessPolicies( this.serviceAccountId, - this.formGroup.value.accessPolicies, + formValues, ); await this.handleAccessTokenAvailableWarning( showAccessRemovalWarning, this.currentAccessPolicies, - this.formGroup.value.accessPolicies, + formValues, ); this.currentAccessPolicies = convertToAccessPolicyItemViews(peoplePoliciesViews); @@ -137,6 +134,7 @@ export class ServiceAccountPeopleComponent implements OnInit, OnDestroy { this.validationService.showError(e); this.setSelected(this.currentAccessPolicies); } + this.formGroup.enable(); }; private setSelected(policiesToSelect: ApItemViewType[]) { @@ -198,9 +196,7 @@ export class ServiceAccountPeopleComponent implements OnInit, OnDestroy { selectedPolicies: ApItemValueType[], ): Promise { if (showAccessRemovalWarning) { - // 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"]); + await this.router.navigate(["sm", this.organizationId, "machine-accounts"]); } else if ( this.accessPolicySelectorService.isAccessRemoval(currentAccessPolicies, selectedPolicies) ) { diff --git a/bitwarden_license/bit-web/src/app/secrets-manager/shared/access-policies/access-policy-selector/access-policy-selector.component.html b/bitwarden_license/bit-web/src/app/secrets-manager/shared/access-policies/access-policy-selector/access-policy-selector.component.html index 4b3c839264..e1faf2a185 100644 --- a/bitwarden_license/bit-web/src/app/secrets-manager/shared/access-policies/access-policy-selector/access-policy-selector.component.html +++ b/bitwarden_license/bit-web/src/app/secrets-manager/shared/access-policies/access-policy-selector/access-policy-selector.component.html @@ -55,6 +55,7 @@ bitIconButton="bwi-close" buttonType="main" size="default" + [disabled]="disabled" [attr.title]="'remove' | i18n" [attr.aria-label]="'remove' | i18n" (click)="selectionList.deselectItem(item.id); handleBlur()" @@ -84,7 +85,14 @@
-
From c70a5aa02457e515c6a94854da71e1200ea1b6e2 Mon Sep 17 00:00:00 2001 From: Matt Gibson Date: Tue, 30 Apr 2024 09:13:02 -0400 Subject: [PATCH 6/8] [PM-6688] Use AccountService as account source (#8893) * Use account service to track accounts and active account * Remove state service active account Observables. * Add email verified to account service * Do not store account info on logged out accounts * Add account activity tracking to account service * Use last account activity from account service * migrate or replicate account service data * Add `AccountActivityService` that handles storing account last active data * Move active and next active user to account service * Remove authenticated accounts from state object * Fold account activity into account service * Fix builds * Fix desktop app switch * Fix logging out non active user * Expand helper to handle new authenticated accounts location * Prefer view observable to tons of async pipes * Fix `npm run test:types` * Correct user activity sorting test * Be more precise about log out messaging * Fix dev compare errors All stored values are serializable, the next step wasn't necessary and was erroring on some types that lack `toString`. * If the account in unlocked on load of lock component, navigate away from lock screen * Handle no users case for auth service statuses * Specify account to switch to * Filter active account out of inactive accounts * Prefer constructor init * Improve comparator * Use helper methods internally * Fixup component tests * Clarify name * Ensure accounts object has only valid userIds * Capitalize const values * Prefer descriptive, single-responsibility guards * Update libs/common/src/state-migrations/migrate.ts Co-authored-by: Justin Baur <19896123+justindbaur@users.noreply.github.com> * Fix merge * Add user Id validation activity for undefined was being set, which was resulting in requests for the auth status of `"undefined"` (string) userId, due to key enumeration. These changes stop that at both locations, as well as account add for good measure. --------- Co-authored-by: Justin Baur <19896123+justindbaur@users.noreply.github.com> --- .../account-switcher.component.html | 4 +- .../account-switcher.component.ts | 9 +- .../services/account-switcher.service.spec.ts | 3 + apps/browser/src/auth/popup/lock.component.ts | 3 +- .../context-menu-clicked-handler.spec.ts | 9 +- .../browser/context-menu-clicked-handler.ts | 17 +- .../browser/src/background/main.background.ts | 20 +- .../src/background/runtime.background.ts | 13 +- .../src/platform/popup/header.component.ts | 17 +- .../services/browser-state.service.spec.ts | 16 +- .../services/default-browser-state.service.ts | 2 - .../local-backed-session-storage.service.ts | 41 +-- apps/browser/src/popup/app.component.ts | 23 +- .../popup/send/send-add-edit.component.ts | 3 + apps/cli/src/bw.ts | 5 +- apps/desktop/src/app/app-routing.module.ts | 4 +- apps/desktop/src/app/app.component.ts | 65 ++--- .../layout/account-switcher.component.html | 214 ++++++++-------- .../app/layout/account-switcher.component.ts | 130 ++++++---- .../src/app/layout/search/search.component.ts | 6 +- .../src/app/services/services.module.ts | 3 +- .../src/app/tools/generator.component.spec.ts | 5 + .../src/app/tools/send/add-edit.component.ts | 3 + apps/desktop/src/auth/guards/login.guard.ts | 29 --- .../src/auth/guards/max-accounts.guard.ts | 38 +++ apps/desktop/src/auth/lock.component.spec.ts | 9 +- apps/desktop/src/auth/lock.component.ts | 3 + apps/desktop/src/main/menu/menubar.ts | 5 +- apps/web/src/app/app.component.ts | 17 +- .../layouts/header/web-header.component.html | 4 +- .../layouts/header/web-header.component.ts | 20 +- .../src/app/tools/send/add-edit.component.ts | 3 + .../vault-items/vault-items.stories.ts | 1 - .../src/auth/components/lock.component.ts | 38 ++- libs/angular/src/pipes/user-name.pipe.ts | 2 +- .../src/tools/send/add-edit.component.ts | 6 +- .../auth-request-login.strategy.spec.ts | 1 + .../login-strategies/login.strategy.spec.ts | 5 +- .../common/login-strategies/login.strategy.ts | 8 + .../password-login.strategy.spec.ts | 3 + .../user-decryption-options.service.spec.ts | 1 + libs/common/spec/fake-account-service.ts | 57 ++++- .../src/auth/abstractions/account.service.ts | 47 +++- .../src/auth/services/account.service.spec.ts | 203 ++++++++++++++- .../src/auth/services/account.service.ts | 90 ++++++- .../src/auth/services/auth.service.spec.ts | 21 +- libs/common/src/auth/services/auth.service.ts | 28 +-- ...-enrollment.service.implementation.spec.ts | 1 + .../platform/abstractions/state.service.ts | 10 +- libs/common/src/platform/misc/utils.spec.ts | 27 ++ .../src/platform/models/domain/state.ts | 3 - .../default-environment.service.spec.ts | 3 + .../src/platform/services/state.service.ts | 234 +++++------------- .../src/platform/services/system.service.ts | 26 +- ...default-active-user-state.provider.spec.ts | 3 +- .../default-active-user-state.spec.ts | 1 + .../default-state.provider.spec.ts | 14 +- .../src/platform/state/state-definitions.ts | 1 + .../vault-timeout.service.spec.ts | 49 ++-- .../vault-timeout/vault-timeout.service.ts | 33 ++- libs/common/src/state-migrations/migrate.ts | 6 +- .../state-migrations/migration-helper.spec.ts | 43 ++++ .../src/state-migrations/migration-helper.ts | 28 ++- .../migrations/60-known-accounts.spec.ts | 145 +++++++++++ .../migrations/60-known-accounts.ts | 111 +++++++++ .../tools/send/services/send.service.spec.ts | 1 + .../src/vault/services/sync/sync.service.ts | 5 +- 67 files changed, 1380 insertions(+), 618 deletions(-) delete mode 100644 apps/desktop/src/auth/guards/login.guard.ts create mode 100644 apps/desktop/src/auth/guards/max-accounts.guard.ts create mode 100644 libs/common/src/state-migrations/migrations/60-known-accounts.spec.ts create mode 100644 libs/common/src/state-migrations/migrations/60-known-accounts.ts diff --git a/apps/browser/src/auth/popup/account-switching/account-switcher.component.html b/apps/browser/src/auth/popup/account-switching/account-switcher.component.html index aebf2219ff..806dae084d 100644 --- a/apps/browser/src/auth/popup/account-switching/account-switcher.component.html +++ b/apps/browser/src/auth/popup/account-switching/account-switcher.component.html @@ -49,7 +49,7 @@ - - - - -
- - - - - {{ "accountSwitcherLimitReached" | i18n }} - + + + - - + + {{ "switchAccount" | i18n }} + + + + + + + + diff --git a/apps/desktop/src/app/layout/account-switcher.component.ts b/apps/desktop/src/app/layout/account-switcher.component.ts index 4e39ab0029..c8a26065c1 100644 --- a/apps/desktop/src/app/layout/account-switcher.component.ts +++ b/apps/desktop/src/app/layout/account-switcher.component.ts @@ -1,19 +1,17 @@ import { animate, state, style, transition, trigger } from "@angular/animations"; import { ConnectedPosition } from "@angular/cdk/overlay"; -import { Component, OnDestroy, OnInit } from "@angular/core"; +import { Component } from "@angular/core"; import { Router } from "@angular/router"; -import { concatMap, firstValueFrom, Subject, takeUntil } from "rxjs"; +import { combineLatest, firstValueFrom, map, Observable, switchMap } from "rxjs"; import { LoginEmailServiceAbstraction } from "@bitwarden/auth/common"; +import { AccountInfo, AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { AvatarService } from "@bitwarden/common/auth/abstractions/avatar.service"; -import { TokenService } from "@bitwarden/common/auth/abstractions/token.service"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; -import { Utils } from "@bitwarden/common/platform/misc/utils"; -import { Account } from "@bitwarden/common/platform/models/domain/account"; import { UserId } from "@bitwarden/common/types/guid"; type ActiveAccount = { @@ -52,12 +50,18 @@ type InactiveAccount = ActiveAccount & { ]), ], }) -export class AccountSwitcherComponent implements OnInit, OnDestroy { - activeAccount?: ActiveAccount; - inactiveAccounts: { [userId: string]: InactiveAccount } = {}; - +export class AccountSwitcherComponent { + activeAccount$: Observable; + inactiveAccounts$: Observable<{ [userId: string]: InactiveAccount }>; authStatus = AuthenticationStatus; + view$: Observable<{ + activeAccount: ActiveAccount | null; + inactiveAccounts: { [userId: string]: InactiveAccount }; + numberOfAccounts: number; + showSwitcher: boolean; + }>; + isOpen = false; overlayPosition: ConnectedPosition[] = [ { @@ -68,21 +72,9 @@ export class AccountSwitcherComponent implements OnInit, OnDestroy { }, ]; - private destroy$ = new Subject(); + showSwitcher$: Observable; - get showSwitcher() { - const userIsInAVault = !Utils.isNullOrWhitespace(this.activeAccount?.email); - const userIsAddingAnAdditionalAccount = Object.keys(this.inactiveAccounts).length > 0; - return userIsInAVault || userIsAddingAnAdditionalAccount; - } - - get numberOfAccounts() { - if (this.inactiveAccounts == null) { - this.isOpen = false; - return 0; - } - return Object.keys(this.inactiveAccounts).length; - } + numberOfAccounts$: Observable; constructor( private stateService: StateService, @@ -90,37 +82,65 @@ export class AccountSwitcherComponent implements OnInit, OnDestroy { private avatarService: AvatarService, private messagingService: MessagingService, private router: Router, - private tokenService: TokenService, private environmentService: EnvironmentService, private loginEmailService: LoginEmailServiceAbstraction, - ) {} + private accountService: AccountService, + ) { + this.activeAccount$ = this.accountService.activeAccount$.pipe( + switchMap(async (active) => { + if (active == null) { + return null; + } - async ngOnInit(): Promise { - this.stateService.accounts$ - .pipe( - concatMap(async (accounts: { [userId: string]: Account }) => { - this.inactiveAccounts = await this.createInactiveAccounts(accounts); + return { + id: active.id, + name: active.name, + email: active.email, + avatarColor: await firstValueFrom(this.avatarService.avatarColor$), + server: (await this.environmentService.getEnvironment())?.getHostname(), + }; + }), + ); + this.inactiveAccounts$ = combineLatest([ + this.activeAccount$, + this.accountService.accounts$, + this.authService.authStatuses$, + ]).pipe( + switchMap(async ([activeAccount, accounts, accountStatuses]) => { + // Filter out logged out accounts and active account + accounts = Object.fromEntries( + Object.entries(accounts).filter( + ([id]: [UserId, AccountInfo]) => + accountStatuses[id] !== AuthenticationStatus.LoggedOut || id === activeAccount?.id, + ), + ); + return this.createInactiveAccounts(accounts); + }), + ); + this.showSwitcher$ = combineLatest([this.activeAccount$, this.inactiveAccounts$]).pipe( + map(([activeAccount, inactiveAccounts]) => { + const hasActiveUser = activeAccount != null; + const userIsAddingAnAdditionalAccount = Object.keys(inactiveAccounts).length > 0; + return hasActiveUser || userIsAddingAnAdditionalAccount; + }), + ); + this.numberOfAccounts$ = this.inactiveAccounts$.pipe( + map((accounts) => Object.keys(accounts).length), + ); - try { - this.activeAccount = { - id: await this.tokenService.getUserId(), - name: (await this.tokenService.getName()) ?? (await this.tokenService.getEmail()), - email: await this.tokenService.getEmail(), - avatarColor: await firstValueFrom(this.avatarService.avatarColor$), - server: (await this.environmentService.getEnvironment())?.getHostname(), - }; - } catch { - this.activeAccount = undefined; - } - }), - takeUntil(this.destroy$), - ) - .subscribe(); - } - - ngOnDestroy(): void { - this.destroy$.next(); - this.destroy$.complete(); + this.view$ = combineLatest([ + this.activeAccount$, + this.inactiveAccounts$, + this.numberOfAccounts$, + this.showSwitcher$, + ]).pipe( + map(([activeAccount, inactiveAccounts, numberOfAccounts, showSwitcher]) => ({ + activeAccount, + inactiveAccounts, + numberOfAccounts, + showSwitcher, + })), + ); } toggle() { @@ -144,11 +164,13 @@ export class AccountSwitcherComponent implements OnInit, OnDestroy { await this.loginEmailService.saveEmailSettings(); await this.router.navigate(["/login"]); - await this.stateService.setActiveUser(null); + const activeAccount = await firstValueFrom(this.accountService.activeAccount$); + await this.stateService.clearDecryptedData(activeAccount?.id as UserId); + await this.accountService.switchAccount(null); } private async createInactiveAccounts(baseAccounts: { - [userId: string]: Account; + [userId: string]: AccountInfo; }): Promise<{ [userId: string]: InactiveAccount }> { const inactiveAccounts: { [userId: string]: InactiveAccount } = {}; @@ -159,8 +181,8 @@ export class AccountSwitcherComponent implements OnInit, OnDestroy { inactiveAccounts[userId] = { id: userId, - name: baseAccounts[userId].profile.name, - email: baseAccounts[userId].profile.email, + name: baseAccounts[userId].name, + email: baseAccounts[userId].email, authenticationStatus: await this.authService.getAuthStatus(userId), avatarColor: await firstValueFrom(this.avatarService.getUserAvatarColor$(userId as UserId)), server: (await this.environmentService.getEnvironment(userId))?.getHostname(), diff --git a/apps/desktop/src/app/layout/search/search.component.ts b/apps/desktop/src/app/layout/search/search.component.ts index 9a7226218a..06c67d8af2 100644 --- a/apps/desktop/src/app/layout/search/search.component.ts +++ b/apps/desktop/src/app/layout/search/search.component.ts @@ -2,7 +2,7 @@ import { Component, OnDestroy, OnInit } from "@angular/core"; import { UntypedFormControl } from "@angular/forms"; import { Subscription } from "rxjs"; -import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { SearchBarService, SearchBarState } from "./search-bar.service"; @@ -18,7 +18,7 @@ export class SearchComponent implements OnInit, OnDestroy { constructor( private searchBarService: SearchBarService, - private stateService: StateService, + private accountService: AccountService, ) { // eslint-disable-next-line rxjs-angular/prefer-takeuntil this.searchBarService.state$.subscribe((state) => { @@ -33,7 +33,7 @@ export class SearchComponent implements OnInit, OnDestroy { ngOnInit() { // eslint-disable-next-line rxjs-angular/prefer-takeuntil - this.activeAccountSubscription = this.stateService.activeAccount$.subscribe((value) => { + this.activeAccountSubscription = this.accountService.activeAccount$.subscribe((_) => { this.searchBarService.setSearchText(""); this.searchText.patchValue(""); }); diff --git a/apps/desktop/src/app/services/services.module.ts b/apps/desktop/src/app/services/services.module.ts index 1e3a7fdfa5..a485b925ba 100644 --- a/apps/desktop/src/app/services/services.module.ts +++ b/apps/desktop/src/app/services/services.module.ts @@ -59,7 +59,6 @@ import { PasswordGenerationServiceAbstraction } from "@bitwarden/common/tools/ge import { CipherService as CipherServiceAbstraction } from "@bitwarden/common/vault/abstractions/cipher.service"; import { DialogService } from "@bitwarden/components"; -import { LoginGuard } from "../../auth/guards/login.guard"; import { DesktopAutofillSettingsService } from "../../autofill/services/desktop-autofill-settings.service"; import { Account } from "../../models/account"; import { DesktopSettingsService } from "../../platform/services/desktop-settings.service"; @@ -102,7 +101,6 @@ const safeProviders: SafeProvider[] = [ safeProvider(InitService), safeProvider(NativeMessagingService), safeProvider(SearchBarService), - safeProvider(LoginGuard), safeProvider(DialogService), safeProvider({ provide: APP_INITIALIZER as SafeInjectionToken<() => void>, @@ -192,6 +190,7 @@ const safeProviders: SafeProvider[] = [ AutofillSettingsServiceAbstraction, VaultTimeoutSettingsService, BiometricStateService, + AccountServiceAbstraction, ], }), safeProvider({ diff --git a/apps/desktop/src/app/tools/generator.component.spec.ts b/apps/desktop/src/app/tools/generator.component.spec.ts index 51b5bf93a2..d908de8ef7 100644 --- a/apps/desktop/src/app/tools/generator.component.spec.ts +++ b/apps/desktop/src/app/tools/generator.component.spec.ts @@ -4,6 +4,7 @@ import { ActivatedRoute } from "@angular/router"; import { mock, MockProxy } from "jest-mock-extended"; import { I18nPipe } from "@bitwarden/angular/platform/pipes/i18n.pipe"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; 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"; @@ -59,6 +60,10 @@ describe("GeneratorComponent", () => { provide: CipherService, useValue: mock(), }, + { + provide: AccountService, + useValue: mock(), + }, ], schemas: [NO_ERRORS_SCHEMA], }).compileComponents(); diff --git a/apps/desktop/src/app/tools/send/add-edit.component.ts b/apps/desktop/src/app/tools/send/add-edit.component.ts index 7bdd5efbba..804a390438 100644 --- a/apps/desktop/src/app/tools/send/add-edit.component.ts +++ b/apps/desktop/src/app/tools/send/add-edit.component.ts @@ -4,6 +4,7 @@ import { FormBuilder } from "@angular/forms"; import { AddEditComponent as BaseAddEditComponent } from "@bitwarden/angular/tools/send/add-edit.component"; import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { BillingAccountProfileStateService } from "@bitwarden/common/billing/abstractions/account/billing-account-profile-state.service"; import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; @@ -34,6 +35,7 @@ export class AddEditComponent extends BaseAddEditComponent { dialogService: DialogService, formBuilder: FormBuilder, billingAccountProfileStateService: BillingAccountProfileStateService, + accountService: AccountService, ) { super( i18nService, @@ -49,6 +51,7 @@ export class AddEditComponent extends BaseAddEditComponent { dialogService, formBuilder, billingAccountProfileStateService, + accountService, ); } diff --git a/apps/desktop/src/auth/guards/login.guard.ts b/apps/desktop/src/auth/guards/login.guard.ts deleted file mode 100644 index f6c67d5af9..0000000000 --- a/apps/desktop/src/auth/guards/login.guard.ts +++ /dev/null @@ -1,29 +0,0 @@ -import { Injectable } from "@angular/core"; -import { CanActivate } from "@angular/router"; -import { firstValueFrom } from "rxjs"; - -import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; -import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; -import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; - -const maxAllowedAccounts = 5; - -@Injectable() -export class LoginGuard implements CanActivate { - protected homepage = "vault"; - constructor( - private stateService: StateService, - private platformUtilsService: PlatformUtilsService, - private i18nService: I18nService, - ) {} - - async canActivate() { - const accounts = await firstValueFrom(this.stateService.accounts$); - if (accounts != null && Object.keys(accounts).length >= maxAllowedAccounts) { - this.platformUtilsService.showToast("error", null, this.i18nService.t("accountLimitReached")); - return false; - } - - return true; - } -} diff --git a/apps/desktop/src/auth/guards/max-accounts.guard.ts b/apps/desktop/src/auth/guards/max-accounts.guard.ts new file mode 100644 index 0000000000..65c4ac99d0 --- /dev/null +++ b/apps/desktop/src/auth/guards/max-accounts.guard.ts @@ -0,0 +1,38 @@ +import { inject } from "@angular/core"; +import { CanActivateFn } from "@angular/router"; +import { Observable, map } from "rxjs"; + +import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; +import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; +import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { ToastService } from "@bitwarden/components"; + +const maxAllowedAccounts = 5; + +function maxAccountsGuard(): Observable { + const authService = inject(AuthService); + const toastService = inject(ToastService); + const i18nService = inject(I18nService); + + return authService.authStatuses$.pipe( + map((statuses) => + Object.values(statuses).filter((status) => status != AuthenticationStatus.LoggedOut), + ), + map((accounts) => { + if (accounts != null && Object.keys(accounts).length >= maxAllowedAccounts) { + toastService.showToast({ + variant: "error", + title: null, + message: i18nService.t("accountLimitReached"), + }); + return false; + } + + return true; + }), + ); +} + +export function maxAccountsGuardFn(): CanActivateFn { + return () => maxAccountsGuard(); +} diff --git a/apps/desktop/src/auth/lock.component.spec.ts b/apps/desktop/src/auth/lock.component.spec.ts index f998e75d7a..2137b707f6 100644 --- a/apps/desktop/src/auth/lock.component.spec.ts +++ b/apps/desktop/src/auth/lock.component.spec.ts @@ -13,6 +13,7 @@ import { VaultTimeoutService } from "@bitwarden/common/abstractions/vault-timeou import { PolicyApiServiceAbstraction } from "@bitwarden/common/admin-console/abstractions/policy/policy-api.service.abstraction"; import { InternalPolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { DeviceTrustServiceAbstraction } from "@bitwarden/common/auth/abstractions/device-trust.service.abstraction"; import { KdfConfigService } from "@bitwarden/common/auth/abstractions/kdf-config.service"; import { InternalMasterPasswordServiceAbstraction } from "@bitwarden/common/auth/abstractions/master-password.service.abstraction"; @@ -50,7 +51,7 @@ describe("LockComponent", () => { let component: LockComponent; let fixture: ComponentFixture; let stateServiceMock: MockProxy; - const biometricStateService = mock(); + let biometricStateService: MockProxy; let messagingServiceMock: MockProxy; let broadcasterServiceMock: MockProxy; let platformUtilsServiceMock: MockProxy; @@ -62,7 +63,6 @@ describe("LockComponent", () => { beforeEach(async () => { stateServiceMock = mock(); - stateServiceMock.activeAccount$ = of(null); messagingServiceMock = mock(); broadcasterServiceMock = mock(); @@ -73,6 +73,7 @@ describe("LockComponent", () => { mockMasterPasswordService = new FakeMasterPasswordService(); + biometricStateService = mock(); biometricStateService.dismissedRequirePasswordOnStartCallout$ = of(false); biometricStateService.promptAutomatically$ = of(false); biometricStateService.promptCancelled$ = of(false); @@ -165,6 +166,10 @@ describe("LockComponent", () => { provide: AccountService, useValue: accountService, }, + { + provide: AuthService, + useValue: mock(), + }, { provide: KdfConfigService, useValue: mock(), diff --git a/apps/desktop/src/auth/lock.component.ts b/apps/desktop/src/auth/lock.component.ts index 8e87b6663f..d95df419e1 100644 --- a/apps/desktop/src/auth/lock.component.ts +++ b/apps/desktop/src/auth/lock.component.ts @@ -10,6 +10,7 @@ import { VaultTimeoutService } from "@bitwarden/common/abstractions/vault-timeou import { PolicyApiServiceAbstraction } from "@bitwarden/common/admin-console/abstractions/policy/policy-api.service.abstraction"; import { InternalPolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { DeviceTrustServiceAbstraction } from "@bitwarden/common/auth/abstractions/device-trust.service.abstraction"; import { KdfConfigService } from "@bitwarden/common/auth/abstractions/kdf-config.service"; import { InternalMasterPasswordServiceAbstraction } from "@bitwarden/common/auth/abstractions/master-password.service.abstraction"; @@ -64,6 +65,7 @@ export class LockComponent extends BaseLockComponent { pinCryptoService: PinCryptoServiceAbstraction, biometricStateService: BiometricStateService, accountService: AccountService, + authService: AuthService, kdfConfigService: KdfConfigService, ) { super( @@ -89,6 +91,7 @@ export class LockComponent extends BaseLockComponent { pinCryptoService, biometricStateService, accountService, + authService, kdfConfigService, ); } diff --git a/apps/desktop/src/main/menu/menubar.ts b/apps/desktop/src/main/menu/menubar.ts index eb1dacf825..b71774c5af 100644 --- a/apps/desktop/src/main/menu/menubar.ts +++ b/apps/desktop/src/main/menu/menubar.ts @@ -65,9 +65,10 @@ export class Menubar { isLocked = updateRequest.accounts[updateRequest.activeUserId]?.isLocked ?? true; } - const isLockable = !isLocked && updateRequest?.accounts[updateRequest.activeUserId]?.isLockable; + const isLockable = + !isLocked && updateRequest?.accounts?.[updateRequest.activeUserId]?.isLockable; const hasMasterPassword = - updateRequest?.accounts[updateRequest.activeUserId]?.hasMasterPassword ?? false; + updateRequest?.accounts?.[updateRequest.activeUserId]?.hasMasterPassword ?? false; this.items = [ new FileMenu( diff --git a/apps/web/src/app/app.component.ts b/apps/web/src/app/app.component.ts index 1da2d94c15..1939bb11f5 100644 --- a/apps/web/src/app/app.component.ts +++ b/apps/web/src/app/app.component.ts @@ -2,7 +2,7 @@ import { DOCUMENT } from "@angular/common"; import { Component, Inject, NgZone, OnDestroy, OnInit } from "@angular/core"; import { NavigationEnd, Router } from "@angular/router"; import * as jq from "jquery"; -import { Subject, switchMap, takeUntil, timer } from "rxjs"; +import { Subject, firstValueFrom, map, switchMap, takeUntil, timer } from "rxjs"; import { EventUploadService } from "@bitwarden/common/abstractions/event/event-upload.service"; import { NotificationsService } from "@bitwarden/common/abstractions/notifications.service"; @@ -10,6 +10,7 @@ import { SearchService } from "@bitwarden/common/abstractions/search.service"; import { VaultTimeoutService } from "@bitwarden/common/abstractions/vault-timeout/vault-timeout.service"; import { InternalOrganizationServiceAbstraction } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { InternalPolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { KeyConnectorService } from "@bitwarden/common/auth/abstractions/key-connector.service"; import { PaymentMethodWarningsServiceAbstraction as PaymentMethodWarningService } from "@bitwarden/common/billing/abstractions/payment-method-warnings-service.abstraction"; @@ -51,7 +52,7 @@ const PaymentMethodWarningsRefresh = 60000; // 1 Minute templateUrl: "app.component.html", }) export class AppComponent implements OnDestroy, OnInit { - private lastActivity: number = null; + private lastActivity: Date = null; private idleTimer: number = null; private isIdle = false; private destroy$ = new Subject(); @@ -86,6 +87,7 @@ export class AppComponent implements OnDestroy, OnInit { private stateEventRunnerService: StateEventRunnerService, private paymentMethodWarningService: PaymentMethodWarningService, private organizationService: InternalOrganizationServiceAbstraction, + private accountService: AccountService, ) {} ngOnInit() { @@ -298,15 +300,16 @@ export class AppComponent implements OnDestroy, OnInit { } private async recordActivity() { - const now = new Date().getTime(); - if (this.lastActivity != null && now - this.lastActivity < 250) { + const activeUserId = await firstValueFrom( + this.accountService.activeAccount$.pipe(map((a) => a?.id)), + ); + const now = new Date(); + if (this.lastActivity != null && now.getTime() - this.lastActivity.getTime() < 250) { return; } this.lastActivity = now; - // 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.stateService.setLastActive(now); + await this.accountService.setAccountActivity(activeUserId, now); // Idle states if (this.isIdle) { this.isIdle = false; diff --git a/apps/web/src/app/layouts/header/web-header.component.html b/apps/web/src/app/layouts/header/web-header.component.html index e24013de6f..e2b3e7910a 100644 --- a/apps/web/src/app/layouts/header/web-header.component.html +++ b/apps/web/src/app/layouts/header/web-header.component.html @@ -58,7 +58,7 @@ [bitMenuTriggerFor]="accountMenu" class="tw-border-0 tw-bg-transparent tw-p-0" > - + @@ -67,7 +67,7 @@ class="tw-flex tw-items-center tw-px-4 tw-py-1 tw-leading-tight tw-text-info" appStopProp > - +
{{ "loggedInAs" | i18n }} diff --git a/apps/web/src/app/layouts/header/web-header.component.ts b/apps/web/src/app/layouts/header/web-header.component.ts index 1f012e52dd..9906bd53ba 100644 --- a/apps/web/src/app/layouts/header/web-header.component.ts +++ b/apps/web/src/app/layouts/header/web-header.component.ts @@ -1,16 +1,17 @@ import { Component, Input } from "@angular/core"; import { ActivatedRoute } from "@angular/router"; -import { combineLatest, map, Observable } from "rxjs"; +import { map, Observable } from "rxjs"; +import { User } from "@bitwarden/angular/pipes/user-name.pipe"; import { UnassignedItemsBannerService } from "@bitwarden/angular/services/unassigned-items-banner.service"; import { VaultTimeoutSettingsService } from "@bitwarden/common/abstractions/vault-timeout/vault-timeout-settings.service"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; import { VaultTimeoutAction } from "@bitwarden/common/enums/vault-timeout-action.enum"; import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; -import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; -import { AccountProfile } from "@bitwarden/common/platform/models/domain/account"; +import { UserId } from "@bitwarden/common/types/guid"; @Component({ selector: "app-header", @@ -28,7 +29,7 @@ export class WebHeaderComponent { @Input() icon: string; protected routeData$: Observable<{ titleId: string }>; - protected account$: Observable; + protected account$: Observable; protected canLock$: Observable; protected selfHosted: boolean; protected hostname = location.hostname; @@ -38,12 +39,12 @@ export class WebHeaderComponent { constructor( private route: ActivatedRoute, - private stateService: StateService, private platformUtilsService: PlatformUtilsService, private vaultTimeoutSettingsService: VaultTimeoutSettingsService, private messagingService: MessagingService, protected unassignedItemsBannerService: UnassignedItemsBannerService, private configService: ConfigService, + private accountService: AccountService, ) { this.routeData$ = this.route.data.pipe( map((params) => { @@ -55,14 +56,7 @@ export class WebHeaderComponent { this.selfHosted = this.platformUtilsService.isSelfHost(); - this.account$ = combineLatest([ - this.stateService.activeAccount$, - this.stateService.accounts$, - ]).pipe( - map(([activeAccount, accounts]) => { - return accounts[activeAccount]?.profile; - }), - ); + this.account$ = this.accountService.activeAccount$; this.canLock$ = this.vaultTimeoutSettingsService .availableVaultTimeoutActions$() .pipe(map((actions) => actions.includes(VaultTimeoutAction.Lock))); diff --git a/apps/web/src/app/tools/send/add-edit.component.ts b/apps/web/src/app/tools/send/add-edit.component.ts index ee4be41488..cca416db9c 100644 --- a/apps/web/src/app/tools/send/add-edit.component.ts +++ b/apps/web/src/app/tools/send/add-edit.component.ts @@ -5,6 +5,7 @@ import { FormBuilder } from "@angular/forms"; import { AddEditComponent as BaseAddEditComponent } from "@bitwarden/angular/tools/send/add-edit.component"; import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { BillingAccountProfileStateService } from "@bitwarden/common/billing/abstractions/account/billing-account-profile-state.service"; import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; @@ -40,6 +41,7 @@ export class AddEditComponent extends BaseAddEditComponent { billingAccountProfileStateService: BillingAccountProfileStateService, protected dialogRef: DialogRef, @Inject(DIALOG_DATA) params: { sendId: string }, + accountService: AccountService, ) { super( i18nService, @@ -55,6 +57,7 @@ export class AddEditComponent extends BaseAddEditComponent { dialogService, formBuilder, billingAccountProfileStateService, + accountService, ); this.sendId = params.sendId; diff --git a/apps/web/src/app/vault/components/vault-items/vault-items.stories.ts b/apps/web/src/app/vault/components/vault-items/vault-items.stories.ts index ad80c9f4e5..41aa766e3a 100644 --- a/apps/web/src/app/vault/components/vault-items/vault-items.stories.ts +++ b/apps/web/src/app/vault/components/vault-items/vault-items.stories.ts @@ -55,7 +55,6 @@ export default { { provide: StateService, useValue: { - activeAccount$: new BehaviorSubject("1").asObservable(), accounts$: new BehaviorSubject({ "1": { profile: { name: "Foo" } } }).asObservable(), async getShowFavicon() { return true; diff --git a/libs/angular/src/auth/components/lock.component.ts b/libs/angular/src/auth/components/lock.component.ts index 89af31da81..7eb30d759a 100644 --- a/libs/angular/src/auth/components/lock.component.ts +++ b/libs/angular/src/auth/components/lock.component.ts @@ -1,7 +1,7 @@ import { Directive, NgZone, OnDestroy, OnInit } from "@angular/core"; import { Router } from "@angular/router"; import { firstValueFrom, Subject } from "rxjs"; -import { concatMap, take, takeUntil } from "rxjs/operators"; +import { concatMap, map, take, takeUntil } from "rxjs/operators"; import { PinCryptoServiceAbstraction } from "@bitwarden/auth/common"; import { ApiService } from "@bitwarden/common/abstractions/api.service"; @@ -11,10 +11,12 @@ import { PolicyApiServiceAbstraction } from "@bitwarden/common/admin-console/abs import { InternalPolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; import { MasterPasswordPolicyOptions } from "@bitwarden/common/admin-console/models/domain/master-password-policy-options"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { DeviceTrustServiceAbstraction } from "@bitwarden/common/auth/abstractions/device-trust.service.abstraction"; import { KdfConfigService } from "@bitwarden/common/auth/abstractions/kdf-config.service"; import { InternalMasterPasswordServiceAbstraction } from "@bitwarden/common/auth/abstractions/master-password.service.abstraction"; import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction"; +import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; import { ForceSetPasswordReason } from "@bitwarden/common/auth/models/domain/force-set-password-reason"; import { SecretVerificationRequest } from "@bitwarden/common/auth/models/request/secret-verification.request"; import { MasterPasswordPolicyResponse } from "@bitwarden/common/auth/models/response/master-password-policy.response"; @@ -30,6 +32,7 @@ import { BiometricStateService } from "@bitwarden/common/platform/biometrics/bio import { HashPurpose, KeySuffixOptions } from "@bitwarden/common/platform/enums"; import { PinLockType } from "@bitwarden/common/services/vault-timeout/vault-timeout-settings.service"; import { PasswordStrengthServiceAbstraction } from "@bitwarden/common/tools/password-strength"; +import { UserId } from "@bitwarden/common/types/guid"; import { UserKey } from "@bitwarden/common/types/key"; import { DialogService } from "@bitwarden/components"; @@ -46,6 +49,7 @@ export class LockComponent implements OnInit, OnDestroy { supportsBiometric: boolean; biometricLock: boolean; + private activeUserId: UserId; protected successRoute = "vault"; protected forcePasswordResetRoute = "update-temp-password"; protected onSuccessfulSubmit: () => Promise; @@ -80,14 +84,16 @@ export class LockComponent implements OnInit, OnDestroy { protected pinCryptoService: PinCryptoServiceAbstraction, protected biometricStateService: BiometricStateService, protected accountService: AccountService, + protected authService: AuthService, protected kdfConfigService: KdfConfigService, ) {} async ngOnInit() { - this.stateService.activeAccount$ + this.accountService.activeAccount$ .pipe( - concatMap(async () => { - await this.load(); + concatMap(async (account) => { + this.activeUserId = account?.id; + await this.load(account?.id); }), takeUntil(this.destroy$), ) @@ -116,7 +122,7 @@ export class LockComponent implements OnInit, OnDestroy { }); if (confirmed) { - this.messagingService.send("logout"); + this.messagingService.send("logout", { userId: this.activeUserId }); } } @@ -321,23 +327,35 @@ export class LockComponent implements OnInit, OnDestroy { } } - private async load() { + private async load(userId: UserId) { // TODO: Investigate PM-3515 // The loading of the lock component works as follows: - // 1. First, is locking a valid timeout action? If not, we will log the user out. - // 2. If locking IS a valid timeout action, we proceed to show the user the lock screen. + // 1. If the user is unlocked, we're here in error so we navigate to the home page + // 2. First, is locking a valid timeout action? If not, we will log the user out. + // 3. If locking IS a valid timeout action, we proceed to show the user the lock screen. // The user will be able to unlock as follows: // - If they have a PIN set, they will be presented with the PIN input // - If they have a master password and no PIN, they will be presented with the master password input // - If they have biometrics enabled, they will be presented with the biometric prompt + const isUnlocked = await firstValueFrom( + this.authService + .authStatusFor$(userId) + .pipe(map((status) => status === AuthenticationStatus.Unlocked)), + ); + if (isUnlocked) { + // navigate to home + await this.router.navigate(["/"]); + return; + } + const availableVaultTimeoutActions = await firstValueFrom( - this.vaultTimeoutSettingsService.availableVaultTimeoutActions$(), + this.vaultTimeoutSettingsService.availableVaultTimeoutActions$(userId), ); const supportsLock = availableVaultTimeoutActions.includes(VaultTimeoutAction.Lock); if (!supportsLock) { - return await this.vaultTimeoutService.logOut(); + return await this.vaultTimeoutService.logOut(userId); } this.pinStatus = await this.vaultTimeoutSettingsService.isPinLockSet(); diff --git a/libs/angular/src/pipes/user-name.pipe.ts b/libs/angular/src/pipes/user-name.pipe.ts index 88b088a7e2..f007f4ad87 100644 --- a/libs/angular/src/pipes/user-name.pipe.ts +++ b/libs/angular/src/pipes/user-name.pipe.ts @@ -1,6 +1,6 @@ import { Pipe, PipeTransform } from "@angular/core"; -interface User { +export interface User { name?: string; email?: string; } diff --git a/libs/angular/src/tools/send/add-edit.component.ts b/libs/angular/src/tools/send/add-edit.component.ts index da859e50bf..b4f7ec171a 100644 --- a/libs/angular/src/tools/send/add-edit.component.ts +++ b/libs/angular/src/tools/send/add-edit.component.ts @@ -5,6 +5,7 @@ import { Subject, firstValueFrom, takeUntil, map, BehaviorSubject, concatMap } f import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; import { PolicyType } from "@bitwarden/common/admin-console/enums"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { BillingAccountProfileStateService } from "@bitwarden/common/billing/abstractions/account/billing-account-profile-state.service"; import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; @@ -118,6 +119,7 @@ export class AddEditComponent implements OnInit, OnDestroy { protected dialogService: DialogService, protected formBuilder: FormBuilder, protected billingAccountProfileStateService: BillingAccountProfileStateService, + protected accountService: AccountService, ) { this.typeOptions = [ { name: i18nService.t("sendTypeFile"), value: SendType.File, premium: true }, @@ -215,7 +217,9 @@ export class AddEditComponent implements OnInit, OnDestroy { } async load() { - this.emailVerified = await this.stateService.getEmailVerified(); + this.emailVerified = await firstValueFrom( + this.accountService.activeAccount$.pipe(map((a) => a?.emailVerified ?? false)), + ); this.type = !this.canAccessPremium || !this.emailVerified ? SendType.Text : SendType.File; if (this.send == null) { diff --git a/libs/auth/src/common/login-strategies/auth-request-login.strategy.spec.ts b/libs/auth/src/common/login-strategies/auth-request-login.strategy.spec.ts index a123e30053..0efb9569eb 100644 --- a/libs/auth/src/common/login-strategies/auth-request-login.strategy.spec.ts +++ b/libs/auth/src/common/login-strategies/auth-request-login.strategy.spec.ts @@ -128,6 +128,7 @@ describe("AuthRequestLoginStrategy", () => { masterPasswordService.masterKeySubject.next(masterKey); cryptoService.decryptUserKeyWithMasterKey.mockResolvedValue(userKey); + tokenService.decodeAccessToken.mockResolvedValue({ sub: mockUserId }); await authRequestLoginStrategy.logIn(credentials); diff --git a/libs/auth/src/common/login-strategies/login.strategy.spec.ts b/libs/auth/src/common/login-strategies/login.strategy.spec.ts index 3284f6e947..c3a8f61d78 100644 --- a/libs/auth/src/common/login-strategies/login.strategy.spec.ts +++ b/libs/auth/src/common/login-strategies/login.strategy.spec.ts @@ -218,7 +218,7 @@ describe("LoginStrategy", () => { expect(messagingService.send).toHaveBeenCalledWith("loggedIn"); }); - it("throws if active account isn't found after being initialized", async () => { + it("throws if new account isn't active after being initialized", async () => { const idTokenResponse = identityTokenResponseFactory(); apiService.postIdentityToken.mockResolvedValue(idTokenResponse); @@ -228,7 +228,8 @@ describe("LoginStrategy", () => { stateService.getVaultTimeoutAction.mockResolvedValue(mockVaultTimeoutAction); stateService.getVaultTimeout.mockResolvedValue(mockVaultTimeout); - accountService.activeAccountSubject.next(null); + accountService.switchAccount = jest.fn(); // block internal switch to new account + accountService.activeAccountSubject.next(null); // simulate no active account await expect(async () => await passwordLoginStrategy.logIn(credentials)).rejects.toThrow(); }); diff --git a/libs/auth/src/common/login-strategies/login.strategy.ts b/libs/auth/src/common/login-strategies/login.strategy.ts index fd268d955e..3a3109349e 100644 --- a/libs/auth/src/common/login-strategies/login.strategy.ts +++ b/libs/auth/src/common/login-strategies/login.strategy.ts @@ -169,6 +169,12 @@ export abstract class LoginStrategy { const vaultTimeoutAction = await this.stateService.getVaultTimeoutAction({ userId }); const vaultTimeout = await this.stateService.getVaultTimeout({ userId }); + await this.accountService.addAccount(userId, { + name: accountInformation.name, + email: accountInformation.email, + emailVerified: accountInformation.email_verified, + }); + // set access token and refresh token before account initialization so authN status can be accurate // User id will be derived from the access token. await this.tokenService.setTokens( @@ -178,6 +184,8 @@ export abstract class LoginStrategy { tokenResponse.refreshToken, // Note: CLI login via API key sends undefined for refresh token. ); + await this.accountService.switchAccount(userId); + await this.stateService.addAccount( new Account({ profile: { diff --git a/libs/auth/src/common/login-strategies/password-login.strategy.spec.ts b/libs/auth/src/common/login-strategies/password-login.strategy.spec.ts index 5c1fe9b1fe..c97639f102 100644 --- a/libs/auth/src/common/login-strategies/password-login.strategy.spec.ts +++ b/libs/auth/src/common/login-strategies/password-login.strategy.spec.ts @@ -164,6 +164,7 @@ describe("PasswordLoginStrategy", () => { masterPasswordService.masterKeySubject.next(masterKey); cryptoService.decryptUserKeyWithMasterKey.mockResolvedValue(userKey); + tokenService.decodeAccessToken.mockResolvedValue({ sub: userId }); await passwordLoginStrategy.logIn(credentials); @@ -199,6 +200,7 @@ describe("PasswordLoginStrategy", () => { it("forces the user to update their master password on successful login when it does not meet master password policy requirements", async () => { passwordStrengthService.getPasswordStrength.mockReturnValue({ score: 0 } as any); policyService.evaluateMasterPassword.mockReturnValue(false); + tokenService.decodeAccessToken.mockResolvedValue({ sub: userId }); const result = await passwordLoginStrategy.logIn(credentials); @@ -213,6 +215,7 @@ describe("PasswordLoginStrategy", () => { it("forces the user to update their master password on successful 2FA login when it does not meet master password policy requirements", async () => { passwordStrengthService.getPasswordStrength.mockReturnValue({ score: 0 } as any); policyService.evaluateMasterPassword.mockReturnValue(false); + tokenService.decodeAccessToken.mockResolvedValue({ sub: userId }); const token2FAResponse = new IdentityTwoFactorResponse({ TwoFactorProviders: ["0"], diff --git a/libs/auth/src/common/services/user-decryption-options/user-decryption-options.service.spec.ts b/libs/auth/src/common/services/user-decryption-options/user-decryption-options.service.spec.ts index 16479f19ea..ae1813d3d7 100644 --- a/libs/auth/src/common/services/user-decryption-options/user-decryption-options.service.spec.ts +++ b/libs/auth/src/common/services/user-decryption-options/user-decryption-options.service.spec.ts @@ -65,6 +65,7 @@ describe("UserDecryptionOptionsService", () => { await fakeAccountService.addAccount(givenUser, { name: "Test User 1", email: "test1@email.com", + emailVerified: false, }); await fakeStateProvider.setUserState( USER_DECRYPTION_OPTIONS, diff --git a/libs/common/spec/fake-account-service.ts b/libs/common/spec/fake-account-service.ts index a8b09b7417..649a158d75 100644 --- a/libs/common/spec/fake-account-service.ts +++ b/libs/common/spec/fake-account-service.ts @@ -1,5 +1,5 @@ import { mock } from "jest-mock-extended"; -import { ReplaySubject } from "rxjs"; +import { ReplaySubject, combineLatest, map } from "rxjs"; import { AccountInfo, AccountService } from "../src/auth/abstractions/account.service"; import { UserId } from "../src/types/guid"; @@ -7,15 +7,20 @@ import { UserId } from "../src/types/guid"; export function mockAccountServiceWith( userId: UserId, info: Partial = {}, + activity: Record = {}, ): FakeAccountService { const fullInfo: AccountInfo = { ...info, ...{ name: "name", email: "email", + emailVerified: true, }, }; - const service = new FakeAccountService({ [userId]: fullInfo }); + + const fullActivity = { [userId]: new Date(), ...activity }; + + const service = new FakeAccountService({ [userId]: fullInfo }, fullActivity); service.activeAccountSubject.next({ id: userId, ...fullInfo }); return service; } @@ -26,17 +31,46 @@ export class FakeAccountService implements AccountService { accountsSubject = new ReplaySubject>(1); // eslint-disable-next-line rxjs/no-exposed-subjects -- test class activeAccountSubject = new ReplaySubject<{ id: UserId } & AccountInfo>(1); + // eslint-disable-next-line rxjs/no-exposed-subjects -- test class + accountActivitySubject = new ReplaySubject>(1); private _activeUserId: UserId; get activeUserId() { return this._activeUserId; } accounts$ = this.accountsSubject.asObservable(); activeAccount$ = this.activeAccountSubject.asObservable(); + accountActivity$ = this.accountActivitySubject.asObservable(); + get sortedUserIds$() { + return this.accountActivity$.pipe( + map((activity) => { + return Object.entries(activity) + .map(([userId, lastActive]: [UserId, Date]) => ({ userId, lastActive })) + .sort((a, b) => a.lastActive.getTime() - b.lastActive.getTime()) + .map((a) => a.userId); + }), + ); + } + get nextUpAccount$() { + return combineLatest([this.accounts$, this.activeAccount$, this.sortedUserIds$]).pipe( + map(([accounts, activeAccount, sortedUserIds]) => { + const nextId = sortedUserIds.find((id) => id !== activeAccount?.id && accounts[id] != null); + return nextId ? { id: nextId, ...accounts[nextId] } : null; + }), + ); + } - constructor(initialData: Record) { + constructor(initialData: Record, accountActivity?: Record) { this.accountsSubject.next(initialData); this.activeAccountSubject.subscribe((data) => (this._activeUserId = data?.id)); this.activeAccountSubject.next(null); + this.accountActivitySubject.next(accountActivity); + } + setAccountActivity(userId: UserId, lastActivity: Date): Promise { + this.accountActivitySubject.next({ + ...this.accountActivitySubject["_buffer"][0], + [userId]: lastActivity, + }); + return this.mock.setAccountActivity(userId, lastActivity); } async addAccount(userId: UserId, accountData: AccountInfo): Promise { @@ -53,10 +87,27 @@ export class FakeAccountService implements AccountService { await this.mock.setAccountEmail(userId, email); } + async setAccountEmailVerified(userId: UserId, emailVerified: boolean): Promise { + await this.mock.setAccountEmailVerified(userId, emailVerified); + } + async switchAccount(userId: UserId): Promise { const next = userId == null ? null : { id: userId, ...this.accountsSubject["_buffer"]?.[0]?.[userId] }; this.activeAccountSubject.next(next); await this.mock.switchAccount(userId); } + + async clean(userId: UserId): Promise { + const current = this.accountsSubject["_buffer"][0] ?? {}; + const updated = { ...current, [userId]: loggedOutInfo }; + this.accountsSubject.next(updated); + await this.mock.clean(userId); + } } + +const loggedOutInfo: AccountInfo = { + name: undefined, + email: "", + emailVerified: false, +}; diff --git a/libs/common/src/auth/abstractions/account.service.ts b/libs/common/src/auth/abstractions/account.service.ts index fa9ad36378..b7fd6d9bb9 100644 --- a/libs/common/src/auth/abstractions/account.service.ts +++ b/libs/common/src/auth/abstractions/account.service.ts @@ -8,18 +8,44 @@ import { UserId } from "../../types/guid"; */ export type AccountInfo = { email: string; + emailVerified: boolean; name: string | undefined; }; export function accountInfoEqual(a: AccountInfo, b: AccountInfo) { - return a?.email === b?.email && a?.name === b?.name; + if (a == null && b == null) { + return true; + } + + if (a == null || b == null) { + return false; + } + + const keys = new Set([...Object.keys(a), ...Object.keys(b)]) as Set; + for (const key of keys) { + if (a[key] !== b[key]) { + return false; + } + } + return true; } export abstract class AccountService { accounts$: Observable>; activeAccount$: Observable<{ id: UserId | undefined } & AccountInfo>; + + /** + * Observable of the last activity time for each account. + */ + accountActivity$: Observable>; + /** Account list in order of descending recency */ + sortedUserIds$: Observable; + /** Next account that is not the current active account */ + nextUpAccount$: Observable<{ id: UserId } & AccountInfo>; /** * Updates the `accounts$` observable with the new account data. + * + * @note Also sets the last active date of the account to `now`. * @param userId * @param accountData */ @@ -36,11 +62,30 @@ export abstract class AccountService { * @param email */ abstract setAccountEmail(userId: UserId, email: string): Promise; + /** + * updates the `accounts$` observable with the new email verification status for the account. + * @param userId + * @param emailVerified + */ + abstract setAccountEmailVerified(userId: UserId, emailVerified: boolean): Promise; /** * Updates the `activeAccount$` observable with the new active account. * @param userId */ abstract switchAccount(userId: UserId): Promise; + /** + * Cleans personal information for the given account from the `accounts$` observable. Does not remove the userId from the observable. + * + * @note Also sets the last active date of the account to `null`. + * @param userId + */ + abstract clean(userId: UserId): Promise; + /** + * Updates the given user's last activity time. + * @param userId + * @param lastActivity + */ + abstract setAccountActivity(userId: UserId, lastActivity: Date): Promise; } export abstract class InternalAccountService extends AccountService { diff --git a/libs/common/src/auth/services/account.service.spec.ts b/libs/common/src/auth/services/account.service.spec.ts index a9cec82c51..0ae14b0cc1 100644 --- a/libs/common/src/auth/services/account.service.spec.ts +++ b/libs/common/src/auth/services/account.service.spec.ts @@ -1,3 +1,8 @@ +/** + * need to update test environment so structuredClone works appropriately + * @jest-environment ../../libs/shared/test.environment.ts + */ + import { MockProxy, mock } from "jest-mock-extended"; import { firstValueFrom } from "rxjs"; @@ -6,15 +11,57 @@ import { FakeGlobalStateProvider } from "../../../spec/fake-state-provider"; import { trackEmissions } from "../../../spec/utils"; import { LogService } from "../../platform/abstractions/log.service"; import { MessagingService } from "../../platform/abstractions/messaging.service"; +import { Utils } from "../../platform/misc/utils"; import { UserId } from "../../types/guid"; -import { AccountInfo } from "../abstractions/account.service"; +import { AccountInfo, accountInfoEqual } from "../abstractions/account.service"; import { ACCOUNT_ACCOUNTS, ACCOUNT_ACTIVE_ACCOUNT_ID, + ACCOUNT_ACTIVITY, AccountServiceImplementation, } from "./account.service"; +describe("accountInfoEqual", () => { + const accountInfo: AccountInfo = { name: "name", email: "email", emailVerified: true }; + + it("compares nulls", () => { + expect(accountInfoEqual(null, null)).toBe(true); + expect(accountInfoEqual(null, accountInfo)).toBe(false); + expect(accountInfoEqual(accountInfo, null)).toBe(false); + }); + + it("compares all keys, not just those defined in AccountInfo", () => { + const different = { ...accountInfo, extra: "extra" }; + + expect(accountInfoEqual(accountInfo, different)).toBe(false); + }); + + it("compares name", () => { + const same = { ...accountInfo }; + const different = { ...accountInfo, name: "name2" }; + + expect(accountInfoEqual(accountInfo, same)).toBe(true); + expect(accountInfoEqual(accountInfo, different)).toBe(false); + }); + + it("compares email", () => { + const same = { ...accountInfo }; + const different = { ...accountInfo, email: "email2" }; + + expect(accountInfoEqual(accountInfo, same)).toBe(true); + expect(accountInfoEqual(accountInfo, different)).toBe(false); + }); + + it("compares emailVerified", () => { + const same = { ...accountInfo }; + const different = { ...accountInfo, emailVerified: false }; + + expect(accountInfoEqual(accountInfo, same)).toBe(true); + expect(accountInfoEqual(accountInfo, different)).toBe(false); + }); +}); + describe("accountService", () => { let messagingService: MockProxy; let logService: MockProxy; @@ -22,8 +69,8 @@ describe("accountService", () => { let sut: AccountServiceImplementation; let accountsState: FakeGlobalState>; let activeAccountIdState: FakeGlobalState; - const userId = "userId" as UserId; - const userInfo = { email: "email", name: "name" }; + const userId = Utils.newGuid() as UserId; + const userInfo = { email: "email", name: "name", emailVerified: true }; beforeEach(() => { messagingService = mock(); @@ -86,6 +133,25 @@ describe("accountService", () => { expect(currentValue).toEqual({ [userId]: userInfo }); }); + + it("sets the last active date of the account to now", async () => { + const state = globalStateProvider.getFake(ACCOUNT_ACTIVITY); + state.stateSubject.next({}); + await sut.addAccount(userId, userInfo); + + expect(state.nextMock).toHaveBeenCalledWith({ [userId]: expect.any(Date) }); + }); + + it.each([null, undefined, 123, "not a guid"])( + "does not set last active if the userId is not a valid guid", + async (userId) => { + const state = globalStateProvider.getFake(ACCOUNT_ACTIVITY); + state.stateSubject.next({}); + await expect(sut.addAccount(userId as UserId, userInfo)).rejects.toThrow( + "userId is required", + ); + }, + ); }); describe("setAccountName", () => { @@ -134,6 +200,58 @@ describe("accountService", () => { }); }); + describe("setAccountEmailVerified", () => { + const initialState = { [userId]: userInfo }; + initialState[userId].emailVerified = false; + beforeEach(() => { + accountsState.stateSubject.next(initialState); + }); + + it("should update the account", async () => { + await sut.setAccountEmailVerified(userId, true); + const currentState = await firstValueFrom(accountsState.state$); + + expect(currentState).toEqual({ + [userId]: { ...userInfo, emailVerified: true }, + }); + }); + + it("should not update if the email is the same", async () => { + await sut.setAccountEmailVerified(userId, false); + const currentState = await firstValueFrom(accountsState.state$); + + expect(currentState).toEqual(initialState); + }); + }); + + describe("clean", () => { + beforeEach(() => { + accountsState.stateSubject.next({ [userId]: userInfo }); + }); + + it("removes account info of the given user", async () => { + await sut.clean(userId); + const currentState = await firstValueFrom(accountsState.state$); + + expect(currentState).toEqual({ + [userId]: { + email: "", + emailVerified: false, + name: undefined, + }, + }); + }); + + it("removes account activity of the given user", async () => { + const state = globalStateProvider.getFake(ACCOUNT_ACTIVITY); + state.stateSubject.next({ [userId]: new Date() }); + + await sut.clean(userId); + + expect(state.nextMock).toHaveBeenCalledWith({}); + }); + }); + describe("switchAccount", () => { beforeEach(() => { accountsState.stateSubject.next({ [userId]: userInfo }); @@ -152,4 +270,83 @@ describe("accountService", () => { expect(sut.switchAccount("unknown" as UserId)).rejects.toThrowError("Account does not exist"); }); }); + + describe("account activity", () => { + let state: FakeGlobalState>; + + beforeEach(() => { + state = globalStateProvider.getFake(ACCOUNT_ACTIVITY); + }); + describe("accountActivity$", () => { + it("returns the account activity state", async () => { + state.stateSubject.next({ + [toId("user1")]: new Date(1), + [toId("user2")]: new Date(2), + }); + + await expect(firstValueFrom(sut.accountActivity$)).resolves.toEqual({ + [toId("user1")]: new Date(1), + [toId("user2")]: new Date(2), + }); + }); + + it("returns an empty object when account activity is null", async () => { + state.stateSubject.next(null); + + await expect(firstValueFrom(sut.accountActivity$)).resolves.toEqual({}); + }); + }); + + describe("sortedUserIds$", () => { + it("returns the sorted user ids by date with most recent first", async () => { + state.stateSubject.next({ + [toId("user1")]: new Date(3), + [toId("user2")]: new Date(2), + [toId("user3")]: new Date(1), + }); + + await expect(firstValueFrom(sut.sortedUserIds$)).resolves.toEqual([ + "user1" as UserId, + "user2" as UserId, + "user3" as UserId, + ]); + }); + + it("returns an empty array when account activity is null", async () => { + state.stateSubject.next(null); + + await expect(firstValueFrom(sut.sortedUserIds$)).resolves.toEqual([]); + }); + }); + + describe("setAccountActivity", () => { + const userId = Utils.newGuid() as UserId; + it("sets the account activity", async () => { + await sut.setAccountActivity(userId, new Date(1)); + + expect(state.nextMock).toHaveBeenCalledWith({ [userId]: new Date(1) }); + }); + + it("does not update if the activity is the same", async () => { + state.stateSubject.next({ [userId]: new Date(1) }); + + await sut.setAccountActivity(userId, new Date(1)); + + expect(state.nextMock).not.toHaveBeenCalled(); + }); + + it.each([null, undefined, 123, "not a guid"])( + "does not set last active if the userId is not a valid guid", + async (userId) => { + await sut.setAccountActivity(userId as UserId, new Date(1)); + + expect(state.nextMock).not.toHaveBeenCalled(); + }, + ); + }); + }); }); + +function toId(userId: string) { + return userId as UserId; +} diff --git a/libs/common/src/auth/services/account.service.ts b/libs/common/src/auth/services/account.service.ts index 77d61fae91..6740387ded 100644 --- a/libs/common/src/auth/services/account.service.ts +++ b/libs/common/src/auth/services/account.service.ts @@ -1,4 +1,4 @@ -import { Subject, combineLatestWith, map, distinctUntilChanged, shareReplay } from "rxjs"; +import { combineLatestWith, map, distinctUntilChanged, shareReplay, combineLatest } from "rxjs"; import { AccountInfo, @@ -7,8 +7,9 @@ import { } from "../../auth/abstractions/account.service"; import { LogService } from "../../platform/abstractions/log.service"; import { MessagingService } from "../../platform/abstractions/messaging.service"; +import { Utils } from "../../platform/misc/utils"; import { - ACCOUNT_MEMORY, + ACCOUNT_DISK, GlobalState, GlobalStateProvider, KeyDefinition, @@ -16,25 +17,36 @@ import { import { UserId } from "../../types/guid"; export const ACCOUNT_ACCOUNTS = KeyDefinition.record( - ACCOUNT_MEMORY, + ACCOUNT_DISK, "accounts", { deserializer: (accountInfo) => accountInfo, }, ); -export const ACCOUNT_ACTIVE_ACCOUNT_ID = new KeyDefinition(ACCOUNT_MEMORY, "activeAccountId", { +export const ACCOUNT_ACTIVE_ACCOUNT_ID = new KeyDefinition(ACCOUNT_DISK, "activeAccountId", { deserializer: (id: UserId) => id, }); +export const ACCOUNT_ACTIVITY = KeyDefinition.record(ACCOUNT_DISK, "activity", { + deserializer: (activity) => new Date(activity), +}); + +const LOGGED_OUT_INFO: AccountInfo = { + email: "", + emailVerified: false, + name: undefined, +}; + export class AccountServiceImplementation implements InternalAccountService { - private lock = new Subject(); - private logout = new Subject(); private accountsState: GlobalState>; private activeAccountIdState: GlobalState; accounts$; activeAccount$; + accountActivity$; + sortedUserIds$; + nextUpAccount$; constructor( private messagingService: MessagingService, @@ -53,14 +65,40 @@ export class AccountServiceImplementation implements InternalAccountService { distinctUntilChanged((a, b) => a?.id === b?.id && accountInfoEqual(a, b)), shareReplay({ bufferSize: 1, refCount: false }), ); + this.accountActivity$ = this.globalStateProvider + .get(ACCOUNT_ACTIVITY) + .state$.pipe(map((activity) => activity ?? {})); + this.sortedUserIds$ = this.accountActivity$.pipe( + map((activity) => { + return Object.entries(activity) + .map(([userId, lastActive]: [UserId, Date]) => ({ userId, lastActive })) + .sort((a, b) => b.lastActive.getTime() - a.lastActive.getTime()) // later dates first + .map((a) => a.userId); + }), + ); + this.nextUpAccount$ = combineLatest([ + this.accounts$, + this.activeAccount$, + this.sortedUserIds$, + ]).pipe( + map(([accounts, activeAccount, sortedUserIds]) => { + const nextId = sortedUserIds.find((id) => id !== activeAccount?.id && accounts[id] != null); + return nextId ? { id: nextId, ...accounts[nextId] } : null; + }), + ); } async addAccount(userId: UserId, accountData: AccountInfo): Promise { + if (!Utils.isGuid(userId)) { + throw new Error("userId is required"); + } + await this.accountsState.update((accounts) => { accounts ||= {}; accounts[userId] = accountData; return accounts; }); + await this.setAccountActivity(userId, new Date()); } async setAccountName(userId: UserId, name: string): Promise { @@ -71,6 +109,15 @@ export class AccountServiceImplementation implements InternalAccountService { await this.setAccountInfo(userId, { email }); } + async setAccountEmailVerified(userId: UserId, emailVerified: boolean): Promise { + await this.setAccountInfo(userId, { emailVerified }); + } + + async clean(userId: UserId) { + await this.setAccountInfo(userId, LOGGED_OUT_INFO); + await this.removeAccountActivity(userId); + } + async switchAccount(userId: UserId): Promise { await this.activeAccountIdState.update( (_, accounts) => { @@ -94,6 +141,37 @@ export class AccountServiceImplementation implements InternalAccountService { ); } + async setAccountActivity(userId: UserId, lastActivity: Date): Promise { + if (!Utils.isGuid(userId)) { + // only store for valid userIds + return; + } + + await this.globalStateProvider.get(ACCOUNT_ACTIVITY).update( + (activity) => { + activity ||= {}; + activity[userId] = lastActivity; + return activity; + }, + { + shouldUpdate: (oldActivity) => oldActivity?.[userId]?.getTime() !== lastActivity?.getTime(), + }, + ); + } + + async removeAccountActivity(userId: UserId): Promise { + await this.globalStateProvider.get(ACCOUNT_ACTIVITY).update( + (activity) => { + if (activity == null) { + return activity; + } + delete activity[userId]; + return activity; + }, + { shouldUpdate: (oldActivity) => oldActivity?.[userId] != null }, + ); + } + // TODO: update to use our own account status settings. Requires inverting direction of state service accounts flow async delete(): Promise { try { diff --git a/libs/common/src/auth/services/auth.service.spec.ts b/libs/common/src/auth/services/auth.service.spec.ts index 3bdf85d3e1..9a93a4207b 100644 --- a/libs/common/src/auth/services/auth.service.spec.ts +++ b/libs/common/src/auth/services/auth.service.spec.ts @@ -56,6 +56,7 @@ describe("AuthService", () => { status: AuthenticationStatus.Unlocked, id: userId, email: "email", + emailVerified: false, name: "name", }; @@ -109,6 +110,7 @@ describe("AuthService", () => { status: AuthenticationStatus.Unlocked, id: Utils.newGuid() as UserId, email: "email2", + emailVerified: false, name: "name2", }; @@ -126,7 +128,11 @@ describe("AuthService", () => { it("requests auth status for all known users", async () => { const userId2 = Utils.newGuid() as UserId; - await accountService.addAccount(userId2, { email: "email2", name: "name2" }); + await accountService.addAccount(userId2, { + email: "email2", + emailVerified: false, + name: "name2", + }); const mockFn = jest.fn().mockReturnValue(of(AuthenticationStatus.Locked)); sut.authStatusFor$ = mockFn; @@ -147,11 +153,14 @@ describe("AuthService", () => { cryptoService.getInMemoryUserKeyFor$.mockReturnValue(of(undefined)); }); - it("emits LoggedOut when userId is null", async () => { - expect(await firstValueFrom(sut.authStatusFor$(null))).toEqual( - AuthenticationStatus.LoggedOut, - ); - }); + it.each([null, undefined, "not a userId"])( + "emits LoggedOut when userId is invalid (%s)", + async () => { + expect(await firstValueFrom(sut.authStatusFor$(null))).toEqual( + AuthenticationStatus.LoggedOut, + ); + }, + ); it("emits LoggedOut when there is no access token", async () => { tokenService.hasAccessToken$.mockReturnValue(of(false)); diff --git a/libs/common/src/auth/services/auth.service.ts b/libs/common/src/auth/services/auth.service.ts index c9e711b4cc..a4529084a2 100644 --- a/libs/common/src/auth/services/auth.service.ts +++ b/libs/common/src/auth/services/auth.service.ts @@ -2,6 +2,7 @@ import { Observable, combineLatest, distinctUntilChanged, + firstValueFrom, map, of, shareReplay, @@ -12,6 +13,7 @@ import { ApiService } from "../../abstractions/api.service"; import { CryptoService } from "../../platform/abstractions/crypto.service"; import { MessagingService } from "../../platform/abstractions/messaging.service"; import { StateService } from "../../platform/abstractions/state.service"; +import { Utils } from "../../platform/misc/utils"; import { UserId } from "../../types/guid"; import { AccountService } from "../abstractions/account.service"; import { AuthService as AuthServiceAbstraction } from "../abstractions/auth.service"; @@ -39,13 +41,16 @@ export class AuthService implements AuthServiceAbstraction { this.authStatuses$ = this.accountService.accounts$.pipe( map((accounts) => Object.keys(accounts) as UserId[]), - switchMap((entries) => - combineLatest( + switchMap((entries) => { + if (entries.length === 0) { + return of([] as { userId: UserId; status: AuthenticationStatus }[]); + } + return combineLatest( entries.map((userId) => this.authStatusFor$(userId).pipe(map((status) => ({ userId, status }))), ), - ), - ), + ); + }), map((statuses) => { return statuses.reduce( (acc, { userId, status }) => { @@ -59,7 +64,7 @@ export class AuthService implements AuthServiceAbstraction { } authStatusFor$(userId: UserId): Observable { - if (userId == null) { + if (!Utils.isGuid(userId)) { return of(AuthenticationStatus.LoggedOut); } @@ -84,17 +89,8 @@ export class AuthService implements AuthServiceAbstraction { } async getAuthStatus(userId?: string): Promise { - // If we don't have an access token or userId, we're logged out - const isAuthenticated = await this.stateService.getIsAuthenticated({ userId: userId }); - if (!isAuthenticated) { - return AuthenticationStatus.LoggedOut; - } - - // Note: since we aggresively set the auto user key to memory if it exists on app init (see InitService) - // we only need to check if the user key is in memory. - const hasUserKey = await this.cryptoService.hasUserKeyInMemory(userId as UserId); - - return hasUserKey ? AuthenticationStatus.Unlocked : AuthenticationStatus.Locked; + userId ??= await firstValueFrom(this.accountService.activeAccount$.pipe(map((a) => a?.id))); + return await firstValueFrom(this.authStatusFor$(userId as UserId)); } logOut(callback: () => void) { diff --git a/libs/common/src/auth/services/password-reset-enrollment.service.implementation.spec.ts b/libs/common/src/auth/services/password-reset-enrollment.service.implementation.spec.ts index fc5060af5f..19b29f0593 100644 --- a/libs/common/src/auth/services/password-reset-enrollment.service.implementation.spec.ts +++ b/libs/common/src/auth/services/password-reset-enrollment.service.implementation.spec.ts @@ -90,6 +90,7 @@ describe("PasswordResetEnrollmentServiceImplementation", () => { const user1AccountInfo: AccountInfo = { name: "Test User 1", email: "test1@email.com", + emailVerified: true, }; activeAccountSubject.next(Object.assign(user1AccountInfo, { id: "userId" as UserId })); diff --git a/libs/common/src/platform/abstractions/state.service.ts b/libs/common/src/platform/abstractions/state.service.ts index 13c33305d1..5ca604b526 100644 --- a/libs/common/src/platform/abstractions/state.service.ts +++ b/libs/common/src/platform/abstractions/state.service.ts @@ -25,11 +25,10 @@ export type InitOptions = { export abstract class StateService { accounts$: Observable<{ [userId: string]: T }>; - activeAccount$: Observable; addAccount: (account: T) => Promise; - setActiveUser: (userId: string) => Promise; - clean: (options?: StorageOptions) => Promise; + clearDecryptedData: (userId: UserId) => Promise; + clean: (options?: StorageOptions) => Promise; init: (initOptions?: InitOptions) => Promise; /** @@ -122,8 +121,6 @@ export abstract class StateService { setDuckDuckGoSharedKey: (value: string, options?: StorageOptions) => Promise; getEmail: (options?: StorageOptions) => Promise; setEmail: (value: string, options?: StorageOptions) => Promise; - getEmailVerified: (options?: StorageOptions) => Promise; - setEmailVerified: (value: boolean, options?: StorageOptions) => Promise; getEnableBrowserIntegration: (options?: StorageOptions) => Promise; setEnableBrowserIntegration: (value: boolean, options?: StorageOptions) => Promise; getEnableBrowserIntegrationFingerprint: (options?: StorageOptions) => Promise; @@ -147,8 +144,6 @@ export abstract class StateService { */ setEncryptedPinProtected: (value: string, options?: StorageOptions) => Promise; getIsAuthenticated: (options?: StorageOptions) => Promise; - getLastActive: (options?: StorageOptions) => Promise; - setLastActive: (value: number, options?: StorageOptions) => Promise; getLastSync: (options?: StorageOptions) => Promise; setLastSync: (value: string, options?: StorageOptions) => Promise; getMinimizeOnCopyToClipboard: (options?: StorageOptions) => Promise; @@ -180,5 +175,4 @@ export abstract class StateService { setVaultTimeout: (value: number, options?: StorageOptions) => Promise; getVaultTimeoutAction: (options?: StorageOptions) => Promise; setVaultTimeoutAction: (value: string, options?: StorageOptions) => Promise; - nextUpActiveUser: () => Promise; } diff --git a/libs/common/src/platform/misc/utils.spec.ts b/libs/common/src/platform/misc/utils.spec.ts index a7a520a77c..964a2a1941 100644 --- a/libs/common/src/platform/misc/utils.spec.ts +++ b/libs/common/src/platform/misc/utils.spec.ts @@ -3,6 +3,33 @@ import * as path from "path"; import { Utils } from "./utils"; describe("Utils Service", () => { + describe("isGuid", () => { + it("is false when null", () => { + expect(Utils.isGuid(null)).toBe(false); + }); + + it("is false when undefined", () => { + expect(Utils.isGuid(undefined)).toBe(false); + }); + + it("is false when empty", () => { + expect(Utils.isGuid("")).toBe(false); + }); + + it("is false when not a string", () => { + expect(Utils.isGuid(123 as any)).toBe(false); + }); + + it("is false when not a guid", () => { + expect(Utils.isGuid("not a guid")).toBe(false); + }); + + it("is true when a guid", () => { + // we use a limited guid scope in which all zeroes is invalid + expect(Utils.isGuid("00000000-0000-1000-8000-000000000000")).toBe(true); + }); + }); + describe("getDomain", () => { it("should fail for invalid urls", () => { expect(Utils.getDomain(null)).toBeNull(); diff --git a/libs/common/src/platform/models/domain/state.ts b/libs/common/src/platform/models/domain/state.ts index 95557e082a..5dde49f99d 100644 --- a/libs/common/src/platform/models/domain/state.ts +++ b/libs/common/src/platform/models/domain/state.ts @@ -9,9 +9,6 @@ export class State< > { accounts: { [userId: string]: TAccount } = {}; globals: TGlobalState; - activeUserId: string; - authenticatedAccounts: string[] = []; - accountActivity: { [userId: string]: number } = {}; constructor(globals: TGlobalState) { this.globals = globals; diff --git a/libs/common/src/platform/services/default-environment.service.spec.ts b/libs/common/src/platform/services/default-environment.service.spec.ts index dd504dc302..7d266e93fc 100644 --- a/libs/common/src/platform/services/default-environment.service.spec.ts +++ b/libs/common/src/platform/services/default-environment.service.spec.ts @@ -31,10 +31,12 @@ describe("EnvironmentService", () => { [testUser]: { name: "name", email: "email", + emailVerified: false, }, [alternateTestUser]: { name: "name", email: "email", + emailVerified: false, }, }); stateProvider = new FakeStateProvider(accountService); @@ -47,6 +49,7 @@ describe("EnvironmentService", () => { id: userId, email: "test@example.com", name: `Test Name ${userId}`, + emailVerified: false, }); await awaitAsync(); }; diff --git a/libs/common/src/platform/services/state.service.ts b/libs/common/src/platform/services/state.service.ts index cab5768d2a..9479d64710 100644 --- a/libs/common/src/platform/services/state.service.ts +++ b/libs/common/src/platform/services/state.service.ts @@ -1,4 +1,4 @@ -import { BehaviorSubject } from "rxjs"; +import { BehaviorSubject, firstValueFrom, map } from "rxjs"; import { Jsonify, JsonValue } from "type-fest"; import { AccountService } from "../../auth/abstractions/account.service"; @@ -33,10 +33,7 @@ const keys = { state: "state", stateVersion: "stateVersion", global: "global", - authenticatedAccounts: "authenticatedAccounts", - activeUserId: "activeUserId", tempAccountSettings: "tempAccountSettings", // used to hold account specific settings (i.e clear clipboard) between initial migration and first account authentication - accountActivity: "accountActivity", }; const partialKeys = { @@ -58,9 +55,6 @@ export class StateService< protected accountsSubject = new BehaviorSubject<{ [userId: string]: TAccount }>({}); accounts$ = this.accountsSubject.asObservable(); - protected activeAccountSubject = new BehaviorSubject(null); - activeAccount$ = this.activeAccountSubject.asObservable(); - private hasBeenInited = false; protected isRecoveredSession = false; @@ -112,36 +106,16 @@ export class StateService< } // Get all likely authenticated accounts - const authenticatedAccounts = ( - (await this.storageService.get(keys.authenticatedAccounts)) ?? [] - ).filter((account) => account != null); + const authenticatedAccounts = await firstValueFrom( + this.accountService.accounts$.pipe(map((accounts) => Object.keys(accounts))), + ); await this.updateState(async (state) => { for (const i in authenticatedAccounts) { state = await this.syncAccountFromDisk(authenticatedAccounts[i]); } - // After all individual accounts have been added - state.authenticatedAccounts = authenticatedAccounts; - - const storedActiveUser = await this.storageService.get(keys.activeUserId); - if (storedActiveUser != null) { - state.activeUserId = storedActiveUser; - } await this.pushAccounts(); - this.activeAccountSubject.next(state.activeUserId); - // TODO: Temporary update to avoid routing all account status changes through account service for now. - // account service tracks logged out accounts, but State service does not, so we need to add the active account - // if it's not in the accounts list. - if (state.activeUserId != null && this.accountsSubject.value[state.activeUserId] == null) { - const activeDiskAccount = await this.getAccountFromDisk({ userId: state.activeUserId }); - await this.accountService.addAccount(state.activeUserId as UserId, { - name: activeDiskAccount.profile.name, - email: activeDiskAccount.profile.email, - }); - } - await this.accountService.switchAccount(state.activeUserId as UserId); - // End TODO return state; }); @@ -161,61 +135,25 @@ export class StateService< return state; }); - // TODO: Temporary update to avoid routing all account status changes through account service for now. - // The determination of state should be handled by the various services that control those values. - await this.accountService.addAccount(userId as UserId, { - name: diskAccount.profile.name, - email: diskAccount.profile.email, - }); - return state; } async addAccount(account: TAccount) { await this.environmentService.seedUserEnvironment(account.profile.userId as UserId); await this.updateState(async (state) => { - state.authenticatedAccounts.push(account.profile.userId); - await this.storageService.save(keys.authenticatedAccounts, state.authenticatedAccounts); state.accounts[account.profile.userId] = account; return state; }); await this.scaffoldNewAccountStorage(account); - await this.setLastActive(new Date().getTime(), { userId: account.profile.userId }); - // TODO: Temporary update to avoid routing all account status changes through account service for now. - await this.accountService.addAccount(account.profile.userId as UserId, { - name: account.profile.name, - email: account.profile.email, - }); - await this.setActiveUser(account.profile.userId); } - async setActiveUser(userId: string): Promise { - await this.clearDecryptedDataForActiveUser(); - await this.updateState(async (state) => { - state.activeUserId = userId; - await this.storageService.save(keys.activeUserId, userId); - this.activeAccountSubject.next(state.activeUserId); - // TODO: temporary update to avoid routing all account status changes through account service for now. - await this.accountService.switchAccount(userId as UserId); - - return state; - }); - - await this.pushAccounts(); - } - - async clean(options?: StorageOptions): Promise { + async clean(options?: StorageOptions): Promise { options = this.reconcileOptions(options, await this.defaultInMemoryOptions()); await this.deAuthenticateAccount(options.userId); - let currentUser = (await this.state())?.activeUserId; - if (options.userId === currentUser) { - currentUser = await this.dynamicallySetActiveUser(); - } await this.removeAccountFromDisk(options?.userId); await this.removeAccountFromMemory(options?.userId); await this.pushAccounts(); - return currentUser as UserId; } /** @@ -515,24 +453,6 @@ export class StateService< ); } - async getEmailVerified(options?: StorageOptions): Promise { - return ( - (await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskOptions()))) - ?.profile.emailVerified ?? false - ); - } - - async setEmailVerified(value: boolean, options?: StorageOptions): Promise { - const account = await this.getAccount( - this.reconcileOptions(options, await this.defaultOnDiskOptions()), - ); - account.profile.emailVerified = value; - await this.saveAccount( - account, - this.reconcileOptions(options, await this.defaultOnDiskOptions()), - ); - } - async getEnableBrowserIntegration(options?: StorageOptions): Promise { return ( (await this.getGlobals(this.reconcileOptions(options, await this.defaultOnDiskOptions()))) @@ -642,35 +562,6 @@ export class StateService< ); } - async getLastActive(options?: StorageOptions): Promise { - options = this.reconcileOptions(options, await this.defaultOnDiskOptions()); - - const accountActivity = await this.storageService.get<{ [userId: string]: number }>( - keys.accountActivity, - options, - ); - - if (accountActivity == null || Object.keys(accountActivity).length < 1) { - return null; - } - - return accountActivity[options.userId]; - } - - async setLastActive(value: number, options?: StorageOptions): Promise { - options = this.reconcileOptions(options, await this.defaultOnDiskOptions()); - if (options.userId == null) { - return; - } - const accountActivity = - (await this.storageService.get<{ [userId: string]: number }>( - keys.accountActivity, - options, - )) ?? {}; - accountActivity[options.userId] = value; - await this.storageService.save(keys.accountActivity, accountActivity, options); - } - async getLastSync(options?: StorageOptions): Promise { return ( await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskMemoryOptions())) @@ -910,24 +801,28 @@ export class StateService< } protected async getAccountFromMemory(options: StorageOptions): Promise { + const userId = + options.userId ?? + (await firstValueFrom( + this.accountService.activeAccount$.pipe(map((account) => account?.id)), + )); + return await this.state().then(async (state) => { if (state.accounts == null) { return null; } - return state.accounts[await this.getUserIdFromMemory(options)]; - }); - } - - protected async getUserIdFromMemory(options: StorageOptions): Promise { - return await this.state().then((state) => { - return options?.userId != null - ? state.accounts[options.userId]?.profile?.userId - : state.activeUserId; + return state.accounts[userId]; }); } protected async getAccountFromDisk(options: StorageOptions): Promise { - if (options?.userId == null && (await this.state())?.activeUserId == null) { + const userId = + options.userId ?? + (await firstValueFrom( + this.accountService.activeAccount$.pipe(map((account) => account?.id)), + )); + + if (userId == null) { return null; } @@ -1086,53 +981,76 @@ export class StateService< } protected async defaultInMemoryOptions(): Promise { + const userId = await firstValueFrom( + this.accountService.activeAccount$.pipe(map((account) => account?.id)), + ); + return { storageLocation: StorageLocation.Memory, - userId: (await this.state()).activeUserId, + userId, }; } protected async defaultOnDiskOptions(): Promise { + const userId = await firstValueFrom( + this.accountService.activeAccount$.pipe(map((account) => account?.id)), + ); + return { storageLocation: StorageLocation.Disk, htmlStorageLocation: HtmlStorageLocation.Session, - userId: (await this.state())?.activeUserId ?? (await this.getActiveUserIdFromStorage()), + userId, useSecureStorage: false, }; } protected async defaultOnDiskLocalOptions(): Promise { + const userId = await firstValueFrom( + this.accountService.activeAccount$.pipe(map((account) => account?.id)), + ); + return { storageLocation: StorageLocation.Disk, htmlStorageLocation: HtmlStorageLocation.Local, - userId: (await this.state())?.activeUserId ?? (await this.getActiveUserIdFromStorage()), + userId, useSecureStorage: false, }; } protected async defaultOnDiskMemoryOptions(): Promise { + const userId = await firstValueFrom( + this.accountService.activeAccount$.pipe(map((account) => account?.id)), + ); + return { storageLocation: StorageLocation.Disk, htmlStorageLocation: HtmlStorageLocation.Memory, - userId: (await this.state())?.activeUserId ?? (await this.getUserId()), + userId, useSecureStorage: false, }; } protected async defaultSecureStorageOptions(): Promise { + const userId = await firstValueFrom( + this.accountService.activeAccount$.pipe(map((account) => account?.id)), + ); + return { storageLocation: StorageLocation.Disk, useSecureStorage: true, - userId: (await this.state())?.activeUserId ?? (await this.getActiveUserIdFromStorage()), + userId, }; } protected async getActiveUserIdFromStorage(): Promise { - return await this.storageService.get(keys.activeUserId); + return await firstValueFrom(this.accountService.activeAccount$.pipe(map((a) => a?.id))); } protected async removeAccountFromLocalStorage(userId: string = null): Promise { - userId = userId ?? (await this.state())?.activeUserId; + userId ??= await firstValueFrom( + this.accountService.activeAccount$.pipe(map((account) => account?.id)), + ); + const storedAccount = await this.getAccount( this.reconcileOptions({ userId: userId }, await this.defaultOnDiskLocalOptions()), ); @@ -1143,7 +1061,10 @@ export class StateService< } protected async removeAccountFromSessionStorage(userId: string = null): Promise { - userId = userId ?? (await this.state())?.activeUserId; + userId ??= await firstValueFrom( + this.accountService.activeAccount$.pipe(map((account) => account?.id)), + ); + const storedAccount = await this.getAccount( this.reconcileOptions({ userId: userId }, await this.defaultOnDiskOptions()), ); @@ -1154,7 +1075,10 @@ export class StateService< } protected async removeAccountFromSecureStorage(userId: string = null): Promise { - userId = userId ?? (await this.state())?.activeUserId; + userId ??= await firstValueFrom( + this.accountService.activeAccount$.pipe(map((account) => account?.id)), + ); + await this.setUserKeyAutoUnlock(null, { userId: userId }); await this.setUserKeyBiometric(null, { userId: userId }); await this.setCryptoMasterKeyAuto(null, { userId: userId }); @@ -1163,8 +1087,11 @@ export class StateService< } protected async removeAccountFromMemory(userId: string = null): Promise { + userId ??= await firstValueFrom( + this.accountService.activeAccount$.pipe(map((account) => account?.id)), + ); + await this.updateState(async (state) => { - userId = userId ?? state.activeUserId; delete state.accounts[userId]; return state; }); @@ -1178,15 +1105,16 @@ export class StateService< return Object.assign(this.createAccount(), persistentAccountInformation); } - protected async clearDecryptedDataForActiveUser(): Promise { + async clearDecryptedData(userId: UserId): Promise { await this.updateState(async (state) => { - const userId = state?.activeUserId; if (userId != null && state?.accounts[userId]?.data != null) { state.accounts[userId].data = new AccountData(); } return state; }); + + await this.pushAccounts(); } protected createAccount(init: Partial = null): TAccount { @@ -1201,14 +1129,6 @@ export class StateService< // We must have a manual call to clear tokens as we can't leverage state provider to clean // up our data as we have secure storage in the mix. await this.tokenService.clearTokens(userId as UserId); - await this.setLastActive(null, { userId: userId }); - await this.updateState(async (state) => { - state.authenticatedAccounts = state.authenticatedAccounts.filter((id) => id !== userId); - - await this.storageService.save(keys.authenticatedAccounts, state.authenticatedAccounts); - - return state; - }); } protected async removeAccountFromDisk(userId: string) { @@ -1217,32 +1137,6 @@ export class StateService< await this.removeAccountFromSecureStorage(userId); } - async nextUpActiveUser() { - const accounts = (await this.state())?.accounts; - if (accounts == null || Object.keys(accounts).length < 1) { - return null; - } - - let newActiveUser; - for (const userId in accounts) { - if (userId == null) { - continue; - } - if (await this.getIsAuthenticated({ userId: userId })) { - newActiveUser = userId; - break; - } - newActiveUser = null; - } - return newActiveUser as UserId; - } - - protected async dynamicallySetActiveUser() { - const newActiveUser = await this.nextUpActiveUser(); - await this.setActiveUser(newActiveUser); - return newActiveUser; - } - protected async saveSecureStorageKey( key: string, value: T, diff --git a/libs/common/src/platform/services/system.service.ts b/libs/common/src/platform/services/system.service.ts index d19390c45e..80053673d8 100644 --- a/libs/common/src/platform/services/system.service.ts +++ b/libs/common/src/platform/services/system.service.ts @@ -1,10 +1,12 @@ -import { firstValueFrom, timeout } from "rxjs"; +import { firstValueFrom, map, timeout } from "rxjs"; import { VaultTimeoutSettingsService } from "../../abstractions/vault-timeout/vault-timeout-settings.service"; +import { AccountService } from "../../auth/abstractions/account.service"; import { AuthService } from "../../auth/abstractions/auth.service"; import { AuthenticationStatus } from "../../auth/enums/authentication-status"; import { AutofillSettingsServiceAbstraction } from "../../autofill/services/autofill-settings.service"; import { VaultTimeoutAction } from "../../enums/vault-timeout-action.enum"; +import { UserId } from "../../types/guid"; import { MessagingService } from "../abstractions/messaging.service"; import { PlatformUtilsService } from "../abstractions/platform-utils.service"; import { StateService } from "../abstractions/state.service"; @@ -25,15 +27,18 @@ export class SystemService implements SystemServiceAbstraction { private autofillSettingsService: AutofillSettingsServiceAbstraction, private vaultTimeoutSettingsService: VaultTimeoutSettingsService, private biometricStateService: BiometricStateService, + private accountService: AccountService, ) {} async startProcessReload(authService: AuthService): Promise { - const accounts = await firstValueFrom(this.stateService.accounts$); + const accounts = await firstValueFrom(this.accountService.accounts$); if (accounts != null) { const keys = Object.keys(accounts); if (keys.length > 0) { for (const userId of keys) { - if ((await authService.getAuthStatus(userId)) === AuthenticationStatus.Unlocked) { + let status = await firstValueFrom(authService.authStatusFor$(userId as UserId)); + status = await authService.getAuthStatus(userId); + if (status === AuthenticationStatus.Unlocked) { return; } } @@ -63,15 +68,24 @@ export class SystemService implements SystemServiceAbstraction { clearInterval(this.reloadInterval); this.reloadInterval = null; - const currentUser = await firstValueFrom(this.stateService.activeAccount$.pipe(timeout(500))); + const currentUser = await firstValueFrom( + this.accountService.activeAccount$.pipe( + map((a) => a?.id), + timeout(500), + ), + ); // Replace current active user if they will be logged out on reload if (currentUser != null) { const timeoutAction = await firstValueFrom( this.vaultTimeoutSettingsService.vaultTimeoutAction$().pipe(timeout(500)), ); if (timeoutAction === VaultTimeoutAction.LogOut) { - const nextUser = await this.stateService.nextUpActiveUser(); - await this.stateService.setActiveUser(nextUser); + const nextUser = await firstValueFrom( + this.accountService.nextUpAccount$.pipe(map((account) => account?.id ?? null)), + ); + // Can be removed once we migrate password generation history to state providers + await this.stateService.clearDecryptedData(currentUser); + await this.accountService.switchAccount(nextUser); } } diff --git a/libs/common/src/platform/state/implementations/default-active-user-state.provider.spec.ts b/libs/common/src/platform/state/implementations/default-active-user-state.provider.spec.ts index c1cc15a176..681963f823 100644 --- a/libs/common/src/platform/state/implementations/default-active-user-state.provider.spec.ts +++ b/libs/common/src/platform/state/implementations/default-active-user-state.provider.spec.ts @@ -1,7 +1,6 @@ import { mock } from "jest-mock-extended"; import { mockAccountServiceWith, trackEmissions } from "../../../../spec"; -import { AuthenticationStatus } from "../../../auth/enums/authentication-status"; import { UserId } from "../../../types/guid"; import { SingleUserStateProvider } from "../user-state.provider"; @@ -14,7 +13,7 @@ describe("DefaultActiveUserStateProvider", () => { id: userId, name: "name", email: "email", - status: AuthenticationStatus.Locked, + emailVerified: false, }; const accountService = mockAccountServiceWith(userId, accountInfo); let sut: DefaultActiveUserStateProvider; diff --git a/libs/common/src/platform/state/implementations/default-active-user-state.spec.ts b/libs/common/src/platform/state/implementations/default-active-user-state.spec.ts index 51a972a9dc..c652136a0d 100644 --- a/libs/common/src/platform/state/implementations/default-active-user-state.spec.ts +++ b/libs/common/src/platform/state/implementations/default-active-user-state.spec.ts @@ -82,6 +82,7 @@ describe("DefaultActiveUserState", () => { activeAccountSubject.next({ id: userId, email: `test${id}@example.com`, + emailVerified: false, name: `Test User ${id}`, }); await awaitAsync(); diff --git a/libs/common/src/platform/state/implementations/default-state.provider.spec.ts b/libs/common/src/platform/state/implementations/default-state.provider.spec.ts index 3243b53d67..98d423cf48 100644 --- a/libs/common/src/platform/state/implementations/default-state.provider.spec.ts +++ b/libs/common/src/platform/state/implementations/default-state.provider.spec.ts @@ -69,7 +69,12 @@ describe("DefaultStateProvider", () => { userId?: UserId, ) => Observable, ) => { - const accountInfo = { email: "email", name: "name", status: AuthenticationStatus.LoggedOut }; + const accountInfo = { + email: "email", + emailVerified: false, + name: "name", + status: AuthenticationStatus.LoggedOut, + }; const keyDefinition = new KeyDefinition(new StateDefinition("test", "disk"), "test", { deserializer: (s) => s, }); @@ -114,7 +119,12 @@ describe("DefaultStateProvider", () => { ); describe("getUserState$", () => { - const accountInfo = { email: "email", name: "name", status: AuthenticationStatus.LoggedOut }; + const accountInfo = { + email: "email", + emailVerified: false, + name: "name", + status: AuthenticationStatus.LoggedOut, + }; const keyDefinition = new KeyDefinition(new StateDefinition("test", "disk"), "test", { deserializer: (s) => s, }); diff --git a/libs/common/src/platform/state/state-definitions.ts b/libs/common/src/platform/state/state-definitions.ts index ee5005202f..6b309ecfb9 100644 --- a/libs/common/src/platform/state/state-definitions.ts +++ b/libs/common/src/platform/state/state-definitions.ts @@ -38,6 +38,7 @@ export const BILLING_DISK = new StateDefinition("billing", "disk"); export const KDF_CONFIG_DISK = new StateDefinition("kdfConfig", "disk"); export const KEY_CONNECTOR_DISK = new StateDefinition("keyConnector", "disk"); export const ACCOUNT_MEMORY = new StateDefinition("account", "memory"); +export const ACCOUNT_DISK = new StateDefinition("account", "disk"); export const MASTER_PASSWORD_MEMORY = new StateDefinition("masterPassword", "memory"); export const MASTER_PASSWORD_DISK = new StateDefinition("masterPassword", "disk"); export const TWO_FACTOR_MEMORY = new StateDefinition("twoFactor", "memory"); diff --git a/libs/common/src/services/vault-timeout/vault-timeout.service.spec.ts b/libs/common/src/services/vault-timeout/vault-timeout.service.spec.ts index 5344093a25..12c24dcdef 100644 --- a/libs/common/src/services/vault-timeout/vault-timeout.service.spec.ts +++ b/libs/common/src/services/vault-timeout/vault-timeout.service.spec.ts @@ -1,9 +1,10 @@ import { MockProxy, any, mock } from "jest-mock-extended"; -import { BehaviorSubject } from "rxjs"; +import { BehaviorSubject, of } from "rxjs"; import { FakeAccountService, mockAccountServiceWith } from "../../../spec/fake-account-service"; import { SearchService } from "../../abstractions/search.service"; import { VaultTimeoutSettingsService } from "../../abstractions/vault-timeout/vault-timeout-settings.service"; +import { AccountInfo } from "../../auth/abstractions/account.service"; import { AuthService } from "../../auth/abstractions/auth.service"; import { AuthenticationStatus } from "../../auth/enums/authentication-status"; import { FakeMasterPasswordService } from "../../auth/services/master-password/fake-master-password.service"; @@ -13,7 +14,6 @@ import { MessagingService } from "../../platform/abstractions/messaging.service" import { PlatformUtilsService } from "../../platform/abstractions/platform-utils.service"; import { StateService } from "../../platform/abstractions/state.service"; import { Utils } from "../../platform/misc/utils"; -import { Account } from "../../platform/models/domain/account"; import { StateEventRunnerService } from "../../platform/state"; import { UserId } from "../../types/guid"; import { CipherService } from "../../vault/abstractions/cipher.service"; @@ -39,7 +39,6 @@ describe("VaultTimeoutService", () => { let lockedCallback: jest.Mock, [userId: string]>; let loggedOutCallback: jest.Mock, [expired: boolean, userId?: string]>; - let accountsSubject: BehaviorSubject>; let vaultTimeoutActionSubject: BehaviorSubject; let availableVaultTimeoutActionsSubject: BehaviorSubject; @@ -65,10 +64,6 @@ describe("VaultTimeoutService", () => { lockedCallback = jest.fn(); loggedOutCallback = jest.fn(); - accountsSubject = new BehaviorSubject(null); - - stateService.accounts$ = accountsSubject; - vaultTimeoutActionSubject = new BehaviorSubject(VaultTimeoutAction.Lock); vaultTimeoutSettingsService.vaultTimeoutAction$.mockReturnValue(vaultTimeoutActionSubject); @@ -127,21 +122,39 @@ describe("VaultTimeoutService", () => { return Promise.resolve(accounts[userId]?.vaultTimeout); }); - stateService.getLastActive.mockImplementation((options) => { - return Promise.resolve(accounts[options.userId]?.lastActive); - }); - stateService.getUserId.mockResolvedValue(globalSetups?.userId); - stateService.activeAccount$ = new BehaviorSubject(globalSetups?.userId); - + // Set desired user active and known users on accounts service : note the only thing that matters here is that the ID are set if (globalSetups?.userId) { accountService.activeAccountSubject.next({ id: globalSetups.userId as UserId, email: null, + emailVerified: false, name: null, }); } + accountService.accounts$ = of( + Object.entries(accounts).reduce( + (agg, [id]) => { + agg[id] = { + email: "", + emailVerified: true, + name: "", + }; + return agg; + }, + {} as Record, + ), + ); + accountService.accountActivity$ = of( + Object.entries(accounts).reduce( + (agg, [id, info]) => { + agg[id] = info.lastActive ? new Date(info.lastActive) : null; + return agg; + }, + {} as Record, + ), + ); platformUtilsService.isViewOpen.mockResolvedValue(globalSetups?.isViewOpen ?? false); @@ -158,16 +171,6 @@ describe("VaultTimeoutService", () => { ], ); }); - - const accountsSubjectValue: Record = Object.keys(accounts).reduce( - (agg, key) => { - const newPartial: Record = {}; - newPartial[key] = null; // No values actually matter on this other than the key - return Object.assign(agg, newPartial); - }, - {} as Record, - ); - accountsSubject.next(accountsSubjectValue); }; const expectUserToHaveLocked = (userId: string) => { diff --git a/libs/common/src/services/vault-timeout/vault-timeout.service.ts b/libs/common/src/services/vault-timeout/vault-timeout.service.ts index 8baf6c04c4..8e0978d07d 100644 --- a/libs/common/src/services/vault-timeout/vault-timeout.service.ts +++ b/libs/common/src/services/vault-timeout/vault-timeout.service.ts @@ -1,4 +1,4 @@ -import { firstValueFrom, timeout } from "rxjs"; +import { combineLatest, firstValueFrom, switchMap } from "rxjs"; import { SearchService } from "../../abstractions/search.service"; import { VaultTimeoutSettingsService } from "../../abstractions/vault-timeout/vault-timeout-settings.service"; @@ -64,14 +64,25 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction { // Get whether or not the view is open a single time so it can be compared for each user const isViewOpen = await this.platformUtilsService.isViewOpen(); - const activeUserId = await firstValueFrom(this.stateService.activeAccount$.pipe(timeout(500))); - - const accounts = await firstValueFrom(this.stateService.accounts$); - for (const userId in accounts) { - if (userId != null && (await this.shouldLock(userId, activeUserId, isViewOpen))) { - await this.executeTimeoutAction(userId); - } - } + await firstValueFrom( + combineLatest([ + this.accountService.activeAccount$, + this.accountService.accountActivity$, + ]).pipe( + switchMap(async ([activeAccount, accountActivity]) => { + const activeUserId = activeAccount?.id; + for (const userIdString in accountActivity) { + const userId = userIdString as UserId; + if ( + userId != null && + (await this.shouldLock(userId, accountActivity[userId], activeUserId, isViewOpen)) + ) { + await this.executeTimeoutAction(userId); + } + } + }), + ), + ); } async lock(userId?: string): Promise { @@ -123,6 +134,7 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction { private async shouldLock( userId: string, + lastActive: Date, activeUserId: string, isViewOpen: boolean, ): Promise { @@ -146,13 +158,12 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction { return false; } - const lastActive = await this.stateService.getLastActive({ userId: userId }); if (lastActive == null) { return false; } const vaultTimeoutSeconds = vaultTimeout * 60; - const diffSeconds = (new Date().getTime() - lastActive) / 1000; + const diffSeconds = (new Date().getTime() - lastActive.getTime()) / 1000; return diffSeconds >= vaultTimeoutSeconds; } diff --git a/libs/common/src/state-migrations/migrate.ts b/libs/common/src/state-migrations/migrate.ts index 31bc5460b4..0a1f4b1d11 100644 --- a/libs/common/src/state-migrations/migrate.ts +++ b/libs/common/src/state-migrations/migrate.ts @@ -57,13 +57,14 @@ import { CipherServiceMigrator } from "./migrations/57-move-cipher-service-to-st import { RemoveRefreshTokenMigratedFlagMigrator } from "./migrations/58-remove-refresh-token-migrated-state-provider-flag"; import { KdfConfigMigrator } from "./migrations/59-move-kdf-config-to-state-provider"; import { RemoveLegacyEtmKeyMigrator } from "./migrations/6-remove-legacy-etm-key"; +import { KnownAccountsMigrator } from "./migrations/60-known-accounts"; import { MoveBiometricAutoPromptToAccount } from "./migrations/7-move-biometric-auto-prompt-to-account"; import { MoveStateVersionMigrator } from "./migrations/8-move-state-version"; import { MoveBrowserSettingsToGlobal } from "./migrations/9-move-browser-settings-to-global"; import { MinVersionMigrator } from "./migrations/min-version"; export const MIN_VERSION = 3; -export const CURRENT_VERSION = 59; +export const CURRENT_VERSION = 60; export type MinVersion = typeof MIN_VERSION; export function createMigrationBuilder() { @@ -124,7 +125,8 @@ export function createMigrationBuilder() { .with(AuthRequestMigrator, 55, 56) .with(CipherServiceMigrator, 56, 57) .with(RemoveRefreshTokenMigratedFlagMigrator, 57, 58) - .with(KdfConfigMigrator, 58, CURRENT_VERSION); + .with(KdfConfigMigrator, 58, 59) + .with(KnownAccountsMigrator, 59, CURRENT_VERSION); } export async function currentVersion( diff --git a/libs/common/src/state-migrations/migration-helper.spec.ts b/libs/common/src/state-migrations/migration-helper.spec.ts index 5f366f2597..162fac2fab 100644 --- a/libs/common/src/state-migrations/migration-helper.spec.ts +++ b/libs/common/src/state-migrations/migration-helper.spec.ts @@ -27,6 +27,14 @@ const exampleJSON = { }, global_serviceName_key: "global_serviceName_key", user_userId_serviceName_key: "user_userId_serviceName_key", + global_account_accounts: { + "c493ed01-4e08-4e88-abc7-332f380ca760": { + otherStuff: "otherStuff3", + }, + "23e61a5f-2ece-4f5e-b499-f0bc489482a9": { + otherStuff: "otherStuff4", + }, + }, }; describe("RemoveLegacyEtmKeyMigrator", () => { @@ -81,6 +89,41 @@ describe("RemoveLegacyEtmKeyMigrator", () => { const accounts = await sut.getAccounts(); expect(accounts).toEqual([]); }); + + it("handles global scoped known accounts for version 60 and after", async () => { + sut.currentVersion = 60; + const accounts = await sut.getAccounts(); + expect(accounts).toEqual([ + // Note, still gets values stored in state service objects, just grabs user ids from global + { + userId: "c493ed01-4e08-4e88-abc7-332f380ca760", + account: { otherStuff: "otherStuff1" }, + }, + { + userId: "23e61a5f-2ece-4f5e-b499-f0bc489482a9", + account: { otherStuff: "otherStuff2" }, + }, + ]); + }); + }); + + describe("getKnownUserIds", () => { + it("returns all user ids", async () => { + const userIds = await sut.getKnownUserIds(); + expect(userIds).toEqual([ + "c493ed01-4e08-4e88-abc7-332f380ca760", + "23e61a5f-2ece-4f5e-b499-f0bc489482a9", + ]); + }); + + it("returns all user ids when version is 60 or greater", async () => { + sut.currentVersion = 60; + const userIds = await sut.getKnownUserIds(); + expect(userIds).toEqual([ + "c493ed01-4e08-4e88-abc7-332f380ca760", + "23e61a5f-2ece-4f5e-b499-f0bc489482a9", + ]); + }); }); describe("getFromGlobal", () => { diff --git a/libs/common/src/state-migrations/migration-helper.ts b/libs/common/src/state-migrations/migration-helper.ts index 2505e2b264..5d1de8dd49 100644 --- a/libs/common/src/state-migrations/migration-helper.ts +++ b/libs/common/src/state-migrations/migration-helper.ts @@ -162,7 +162,7 @@ export class MigrationHelper { async getAccounts(): Promise< { userId: string; account: ExpectedAccountType }[] > { - const userIds = (await this.get("authenticatedAccounts")) ?? []; + const userIds = await this.getKnownUserIds(); return Promise.all( userIds.map(async (userId) => ({ userId, @@ -171,6 +171,17 @@ export class MigrationHelper { ); } + /** + * Helper method to read known users ids. + */ + async getKnownUserIds(): Promise { + if (this.currentVersion < 61) { + return knownAccountUserIdsBuilderPre61(this.storageService); + } else { + return knownAccountUserIdsBuilder(this.storageService); + } + } + /** * Builds a user storage key appropriate for the current version. * @@ -233,3 +244,18 @@ function globalKeyBuilder(keyDefinition: KeyDefinitionLike): string { function globalKeyBuilderPre9(): string { throw Error("No key builder should be used for versions prior to 9."); } + +async function knownAccountUserIdsBuilderPre61( + storageService: AbstractStorageService, +): Promise { + return (await storageService.get("authenticatedAccounts")) ?? []; +} + +async function knownAccountUserIdsBuilder( + storageService: AbstractStorageService, +): Promise { + const accounts = await storageService.get>( + globalKeyBuilder({ stateDefinition: { name: "account" }, key: "accounts" }), + ); + return Object.keys(accounts ?? {}); +} diff --git a/libs/common/src/state-migrations/migrations/60-known-accounts.spec.ts b/libs/common/src/state-migrations/migrations/60-known-accounts.spec.ts new file mode 100644 index 0000000000..28dedb3c39 --- /dev/null +++ b/libs/common/src/state-migrations/migrations/60-known-accounts.spec.ts @@ -0,0 +1,145 @@ +import { MockProxy } from "jest-mock-extended"; + +import { MigrationHelper } from "../migration-helper"; +import { mockMigrationHelper } from "../migration-helper.spec"; + +import { + ACCOUNT_ACCOUNTS, + ACCOUNT_ACTIVE_ACCOUNT_ID, + ACCOUNT_ACTIVITY, + KnownAccountsMigrator, +} from "./60-known-accounts"; + +const migrateJson = () => { + return { + authenticatedAccounts: ["user1", "user2"], + activeUserId: "user1", + user1: { + profile: { + email: "user1", + name: "User 1", + emailVerified: true, + }, + }, + user2: { + profile: { + email: "", + emailVerified: false, + }, + }, + accountActivity: { + user1: 1609459200000, // 2021-01-01 + user2: 1609545600000, // 2021-01-02 + }, + }; +}; + +const rollbackJson = () => { + return { + user1: { + profile: { + email: "user1", + name: "User 1", + emailVerified: true, + }, + }, + user2: { + profile: { + email: "", + emailVerified: false, + }, + }, + global_account_accounts: { + user1: { + profile: { + email: "user1", + name: "User 1", + emailVerified: true, + }, + }, + user2: { + profile: { + email: "", + emailVerified: false, + }, + }, + }, + global_account_activeAccountId: "user1", + global_account_activity: { + user1: "2021-01-01T00:00:00.000Z", + user2: "2021-01-02T00:00:00.000Z", + }, + }; +}; + +describe("ReplicateKnownAccounts", () => { + let helper: MockProxy; + let sut: KnownAccountsMigrator; + + describe("migrate", () => { + beforeEach(() => { + helper = mockMigrationHelper(migrateJson(), 59); + sut = new KnownAccountsMigrator(59, 60); + }); + + it("migrates accounts", async () => { + await sut.migrate(helper); + expect(helper.setToGlobal).toHaveBeenCalledWith(ACCOUNT_ACCOUNTS, { + user1: { + email: "user1", + name: "User 1", + emailVerified: true, + }, + user2: { + email: "", + emailVerified: false, + name: undefined, + }, + }); + expect(helper.remove).toHaveBeenCalledWith("authenticatedAccounts"); + }); + + it("migrates active account it", async () => { + await sut.migrate(helper); + expect(helper.setToGlobal).toHaveBeenCalledWith(ACCOUNT_ACTIVE_ACCOUNT_ID, "user1"); + expect(helper.remove).toHaveBeenCalledWith("activeUserId"); + }); + + it("migrates account activity", async () => { + await sut.migrate(helper); + expect(helper.setToGlobal).toHaveBeenCalledWith(ACCOUNT_ACTIVITY, { + user1: '"2021-01-01T00:00:00.000Z"', + user2: '"2021-01-02T00:00:00.000Z"', + }); + expect(helper.remove).toHaveBeenCalledWith("accountActivity"); + }); + }); + + describe("rollback", () => { + beforeEach(() => { + helper = mockMigrationHelper(rollbackJson(), 60); + sut = new KnownAccountsMigrator(59, 60); + }); + + it("rolls back authenticated accounts", async () => { + await sut.rollback(helper); + expect(helper.set).toHaveBeenCalledWith("authenticatedAccounts", ["user1", "user2"]); + expect(helper.removeFromGlobal).toHaveBeenCalledWith(ACCOUNT_ACCOUNTS); + }); + + it("rolls back active account id", async () => { + await sut.rollback(helper); + expect(helper.set).toHaveBeenCalledWith("activeUserId", "user1"); + expect(helper.removeFromGlobal).toHaveBeenCalledWith(ACCOUNT_ACTIVE_ACCOUNT_ID); + }); + + it("rolls back account activity", async () => { + await sut.rollback(helper); + expect(helper.set).toHaveBeenCalledWith("accountActivity", { + user1: 1609459200000, + user2: 1609545600000, + }); + expect(helper.removeFromGlobal).toHaveBeenCalledWith(ACCOUNT_ACTIVITY); + }); + }); +}); diff --git a/libs/common/src/state-migrations/migrations/60-known-accounts.ts b/libs/common/src/state-migrations/migrations/60-known-accounts.ts new file mode 100644 index 0000000000..75117da5b4 --- /dev/null +++ b/libs/common/src/state-migrations/migrations/60-known-accounts.ts @@ -0,0 +1,111 @@ +import { KeyDefinitionLike, MigrationHelper } from "../migration-helper"; +import { Migrator } from "../migrator"; + +export const ACCOUNT_ACCOUNTS: KeyDefinitionLike = { + stateDefinition: { + name: "account", + }, + key: "accounts", +}; + +export const ACCOUNT_ACTIVE_ACCOUNT_ID: KeyDefinitionLike = { + stateDefinition: { + name: "account", + }, + key: "activeAccountId", +}; + +export const ACCOUNT_ACTIVITY: KeyDefinitionLike = { + stateDefinition: { + name: "account", + }, + key: "activity", +}; + +type ExpectedAccountType = { + profile?: { + email?: string; + name?: string; + emailVerified?: boolean; + }; +}; + +export class KnownAccountsMigrator extends Migrator<59, 60> { + async migrate(helper: MigrationHelper): Promise { + await this.migrateAuthenticatedAccounts(helper); + await this.migrateActiveAccountId(helper); + await this.migrateAccountActivity(helper); + } + async rollback(helper: MigrationHelper): Promise { + // authenticated account are removed, but the accounts record also contains logged out accounts. Best we can do is to add them all back + const accounts = (await helper.getFromGlobal>(ACCOUNT_ACCOUNTS)) ?? {}; + await helper.set("authenticatedAccounts", Object.keys(accounts)); + await helper.removeFromGlobal(ACCOUNT_ACCOUNTS); + + // Active Account Id + const activeAccountId = await helper.getFromGlobal(ACCOUNT_ACTIVE_ACCOUNT_ID); + if (activeAccountId) { + await helper.set("activeUserId", activeAccountId); + } + await helper.removeFromGlobal(ACCOUNT_ACTIVE_ACCOUNT_ID); + + // Account Activity + const accountActivity = await helper.getFromGlobal>(ACCOUNT_ACTIVITY); + if (accountActivity) { + const toStore = Object.entries(accountActivity).reduce( + (agg, [userId, dateString]) => { + agg[userId] = new Date(dateString).getTime(); + return agg; + }, + {} as Record, + ); + await helper.set("accountActivity", toStore); + } + await helper.removeFromGlobal(ACCOUNT_ACTIVITY); + } + + private async migrateAuthenticatedAccounts(helper: MigrationHelper) { + const authenticatedAccounts = (await helper.get("authenticatedAccounts")) ?? []; + const accounts = await Promise.all( + authenticatedAccounts.map(async (userId) => { + const account = await helper.get(userId); + return { userId, account }; + }), + ); + const accountsToStore = accounts.reduce( + (agg, { userId, account }) => { + if (account?.profile) { + agg[userId] = { + email: account.profile.email ?? "", + emailVerified: account.profile.emailVerified ?? false, + name: account.profile.name, + }; + } + return agg; + }, + {} as Record, + ); + + await helper.setToGlobal(ACCOUNT_ACCOUNTS, accountsToStore); + await helper.remove("authenticatedAccounts"); + } + + private async migrateAccountActivity(helper: MigrationHelper) { + const stored = await helper.get>("accountActivity"); + const accountActivity = Object.entries(stored ?? {}).reduce( + (agg, [userId, dateMs]) => { + agg[userId] = JSON.stringify(new Date(dateMs)); + return agg; + }, + {} as Record, + ); + await helper.setToGlobal(ACCOUNT_ACTIVITY, accountActivity); + await helper.remove("accountActivity"); + } + + private async migrateActiveAccountId(helper: MigrationHelper) { + const activeAccountId = await helper.get("activeUserId"); + await helper.setToGlobal(ACCOUNT_ACTIVE_ACCOUNT_ID, activeAccountId); + await helper.remove("activeUserId"); + } +} diff --git a/libs/common/src/tools/send/services/send.service.spec.ts b/libs/common/src/tools/send/services/send.service.spec.ts index 41183c42af..2f0f50c616 100644 --- a/libs/common/src/tools/send/services/send.service.spec.ts +++ b/libs/common/src/tools/send/services/send.service.spec.ts @@ -62,6 +62,7 @@ describe("SendService", () => { accountService.activeAccountSubject.next({ id: mockUserId, email: "email", + emailVerified: false, name: "name", }); diff --git a/libs/common/src/vault/services/sync/sync.service.ts b/libs/common/src/vault/services/sync/sync.service.ts index 73869ff488..995ab7319b 100644 --- a/libs/common/src/vault/services/sync/sync.service.ts +++ b/libs/common/src/vault/services/sync/sync.service.ts @@ -326,7 +326,10 @@ export class SyncService implements SyncServiceAbstraction { await this.cryptoService.setOrgKeys(response.organizations, response.providerOrganizations); await this.avatarService.setSyncAvatarColor(response.id as UserId, response.avatarColor); await this.tokenService.setSecurityStamp(response.securityStamp, response.id as UserId); - await this.stateService.setEmailVerified(response.emailVerified); + await this.accountService.setAccountEmailVerified( + response.id as UserId, + response.emailVerified, + ); await this.billingAccountProfileStateService.setHasPremium( response.premiumPersonally, From e7416384dcb1b1b242a4672366bf5a6e81d1ee09 Mon Sep 17 00:00:00 2001 From: Will Martin Date: Tue, 30 Apr 2024 10:27:47 -0400 Subject: [PATCH 7/8] [CL-220] item components (#8870) --- .../popup/layout/popup-layout.stories.ts | 41 ++- .../src/a11y/a11y-cell.directive.ts | 33 ++ .../src/a11y/a11y-grid.directive.ts | 145 ++++++++ .../components/src/a11y/a11y-row.directive.ts | 31 ++ libs/components/src/badge/badge.directive.ts | 9 +- .../src/icon-button/icon-button.component.ts | 16 +- libs/components/src/index.ts | 1 + .../src/input/autofocus.directive.ts | 9 +- libs/components/src/item/index.ts | 1 + .../src/item/item-action.component.ts | 12 + .../src/item/item-content.component.html | 16 + .../src/item/item-content.component.ts | 15 + .../src/item/item-group.component.ts | 13 + libs/components/src/item/item.component.html | 21 ++ libs/components/src/item/item.component.ts | 29 ++ libs/components/src/item/item.mdx | 141 ++++++++ libs/components/src/item/item.module.ts | 12 + libs/components/src/item/item.stories.ts | 326 ++++++++++++++++++ .../components/src/search/search.component.ts | 6 +- .../src/shared/focusable-element.ts | 8 + libs/components/src/styles.scss | 2 +- 21 files changed, 858 insertions(+), 29 deletions(-) create mode 100644 libs/components/src/a11y/a11y-cell.directive.ts create mode 100644 libs/components/src/a11y/a11y-grid.directive.ts create mode 100644 libs/components/src/a11y/a11y-row.directive.ts create mode 100644 libs/components/src/item/index.ts create mode 100644 libs/components/src/item/item-action.component.ts create mode 100644 libs/components/src/item/item-content.component.html create mode 100644 libs/components/src/item/item-content.component.ts create mode 100644 libs/components/src/item/item-group.component.ts create mode 100644 libs/components/src/item/item.component.html create mode 100644 libs/components/src/item/item.component.ts create mode 100644 libs/components/src/item/item.mdx create mode 100644 libs/components/src/item/item.module.ts create mode 100644 libs/components/src/item/item.stories.ts create mode 100644 libs/components/src/shared/focusable-element.ts diff --git a/apps/browser/src/platform/popup/layout/popup-layout.stories.ts b/apps/browser/src/platform/popup/layout/popup-layout.stories.ts index 1b10e50c0c..77530d06e5 100644 --- a/apps/browser/src/platform/popup/layout/popup-layout.stories.ts +++ b/apps/browser/src/platform/popup/layout/popup-layout.stories.ts @@ -6,9 +6,11 @@ import { Meta, StoryObj, applicationConfig, moduleMetadata } from "@storybook/an import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { AvatarModule, + BadgeModule, ButtonModule, I18nMockService, IconButtonModule, + ItemModule, } from "@bitwarden/components"; import { PopupFooterComponent } from "./popup-footer.component"; @@ -30,23 +32,34 @@ class ExtensionContainerComponent {} @Component({ selector: "vault-placeholder", template: ` -
vault item
-
vault item
-
vault item
-
vault item
-
vault item
-
vault item
-
vault item
-
vault item
-
vault item
-
vault item
-
vault item
-
vault item
-
vault item last item
+ + + + + + + + + + + + + + + + + `, standalone: true, + imports: [CommonModule, ItemModule, BadgeModule, IconButtonModule], }) -class VaultComponent {} +class VaultComponent { + protected data = Array.from(Array(20).keys()); +} @Component({ selector: "generator-placeholder", diff --git a/libs/components/src/a11y/a11y-cell.directive.ts b/libs/components/src/a11y/a11y-cell.directive.ts new file mode 100644 index 0000000000..fdd75c076f --- /dev/null +++ b/libs/components/src/a11y/a11y-cell.directive.ts @@ -0,0 +1,33 @@ +import { ContentChild, Directive, ElementRef, HostBinding } from "@angular/core"; + +import { FocusableElement } from "../shared/focusable-element"; + +@Directive({ + selector: "bitA11yCell", + standalone: true, + providers: [{ provide: FocusableElement, useExisting: A11yCellDirective }], +}) +export class A11yCellDirective implements FocusableElement { + @HostBinding("attr.role") + role: "gridcell" | null; + + @ContentChild(FocusableElement) + private focusableChild: FocusableElement; + + getFocusTarget() { + let focusTarget: HTMLElement; + if (this.focusableChild) { + focusTarget = this.focusableChild.getFocusTarget(); + } else { + focusTarget = this.elementRef.nativeElement.querySelector("button, a"); + } + + if (!focusTarget) { + return this.elementRef.nativeElement; + } + + return focusTarget; + } + + constructor(private elementRef: ElementRef) {} +} diff --git a/libs/components/src/a11y/a11y-grid.directive.ts b/libs/components/src/a11y/a11y-grid.directive.ts new file mode 100644 index 0000000000..c632376f4f --- /dev/null +++ b/libs/components/src/a11y/a11y-grid.directive.ts @@ -0,0 +1,145 @@ +import { + AfterViewInit, + ContentChildren, + Directive, + HostBinding, + HostListener, + Input, + QueryList, +} from "@angular/core"; + +import type { A11yCellDirective } from "./a11y-cell.directive"; +import { A11yRowDirective } from "./a11y-row.directive"; + +@Directive({ + selector: "bitA11yGrid", + standalone: true, +}) +export class A11yGridDirective implements AfterViewInit { + @HostBinding("attr.role") + role = "grid"; + + @ContentChildren(A11yRowDirective) + rows: QueryList; + + /** The number of pages to navigate on `PageUp` and `PageDown` */ + @Input() pageSize = 5; + + private grid: A11yCellDirective[][]; + + /** The row that currently has focus */ + private activeRow = 0; + + /** The cell that currently has focus */ + private activeCol = 0; + + @HostListener("keydown", ["$event"]) + onKeyDown(event: KeyboardEvent) { + switch (event.code) { + case "ArrowUp": + this.updateCellFocusByDelta(-1, 0); + break; + case "ArrowRight": + this.updateCellFocusByDelta(0, 1); + break; + case "ArrowDown": + this.updateCellFocusByDelta(1, 0); + break; + case "ArrowLeft": + this.updateCellFocusByDelta(0, -1); + break; + case "Home": + this.updateCellFocusByDelta(-this.activeRow, -this.activeCol); + break; + case "End": + this.updateCellFocusByDelta(this.grid.length, this.grid[this.grid.length - 1].length); + break; + case "PageUp": + this.updateCellFocusByDelta(-this.pageSize, 0); + break; + case "PageDown": + this.updateCellFocusByDelta(this.pageSize, 0); + break; + default: + return; + } + + /** Prevent default scrolling behavior */ + event.preventDefault(); + } + + ngAfterViewInit(): void { + this.initializeGrid(); + } + + private initializeGrid(): void { + try { + this.grid = this.rows.map((listItem) => { + listItem.role = "row"; + return [...listItem.cells]; + }); + this.grid.flat().forEach((cell) => { + cell.role = "gridcell"; + cell.getFocusTarget().tabIndex = -1; + }); + + this.getActiveCellContent().tabIndex = 0; + } catch (error) { + // eslint-disable-next-line no-console + console.error("Unable to initialize grid"); + } + } + + /** Get the focusable content of the active cell */ + private getActiveCellContent(): HTMLElement { + return this.grid[this.activeRow][this.activeCol].getFocusTarget(); + } + + /** Move focus via a delta against the currently active gridcell */ + private updateCellFocusByDelta(rowDelta: number, colDelta: number) { + const prevActive = this.getActiveCellContent(); + + this.activeCol += colDelta; + this.activeRow += rowDelta; + + // Row upper bound + if (this.activeRow >= this.grid.length) { + this.activeRow = this.grid.length - 1; + } + + // Row lower bound + if (this.activeRow < 0) { + this.activeRow = 0; + } + + // Column upper bound + if (this.activeCol >= this.grid[this.activeRow].length) { + if (this.activeRow < this.grid.length - 1) { + // Wrap to next row on right arrow + this.activeCol = 0; + this.activeRow += 1; + } else { + this.activeCol = this.grid[this.activeRow].length - 1; + } + } + + // Column lower bound + if (this.activeCol < 0) { + if (this.activeRow > 0) { + // Wrap to prev row on left arrow + this.activeRow -= 1; + this.activeCol = this.grid[this.activeRow].length - 1; + } else { + this.activeCol = 0; + } + } + + const nextActive = this.getActiveCellContent(); + nextActive.tabIndex = 0; + nextActive.focus(); + + if (nextActive !== prevActive) { + prevActive.tabIndex = -1; + } + } +} diff --git a/libs/components/src/a11y/a11y-row.directive.ts b/libs/components/src/a11y/a11y-row.directive.ts new file mode 100644 index 0000000000..e062eb2b5a --- /dev/null +++ b/libs/components/src/a11y/a11y-row.directive.ts @@ -0,0 +1,31 @@ +import { + AfterViewInit, + ContentChildren, + Directive, + HostBinding, + QueryList, + ViewChildren, +} from "@angular/core"; + +import { A11yCellDirective } from "./a11y-cell.directive"; + +@Directive({ + selector: "bitA11yRow", + standalone: true, +}) +export class A11yRowDirective implements AfterViewInit { + @HostBinding("attr.role") + role: "row" | null; + + cells: A11yCellDirective[]; + + @ViewChildren(A11yCellDirective) + private viewCells: QueryList; + + @ContentChildren(A11yCellDirective) + private contentCells: QueryList; + + ngAfterViewInit(): void { + this.cells = [...this.viewCells, ...this.contentCells]; + } +} diff --git a/libs/components/src/badge/badge.directive.ts b/libs/components/src/badge/badge.directive.ts index b81b9f80e2..acce4a18aa 100644 --- a/libs/components/src/badge/badge.directive.ts +++ b/libs/components/src/badge/badge.directive.ts @@ -1,5 +1,7 @@ import { Directive, ElementRef, HostBinding, Input } from "@angular/core"; +import { FocusableElement } from "../shared/focusable-element"; + export type BadgeVariant = "primary" | "secondary" | "success" | "danger" | "warning" | "info"; const styles: Record = { @@ -22,8 +24,9 @@ const hoverStyles: Record = { @Directive({ selector: "span[bitBadge], a[bitBadge], button[bitBadge]", + providers: [{ provide: FocusableElement, useExisting: BadgeDirective }], }) -export class BadgeDirective { +export class BadgeDirective implements FocusableElement { @HostBinding("class") get classList() { return [ "tw-inline-block", @@ -62,6 +65,10 @@ export class BadgeDirective { */ @Input() truncate = true; + getFocusTarget() { + return this.el.nativeElement; + } + private hasHoverEffects = false; constructor(private el: ElementRef) { diff --git a/libs/components/src/icon-button/icon-button.component.ts b/libs/components/src/icon-button/icon-button.component.ts index 53e8032795..54f6dfda96 100644 --- a/libs/components/src/icon-button/icon-button.component.ts +++ b/libs/components/src/icon-button/icon-button.component.ts @@ -1,6 +1,7 @@ -import { Component, HostBinding, Input } from "@angular/core"; +import { Component, ElementRef, HostBinding, Input } from "@angular/core"; import { ButtonLikeAbstraction, ButtonType } from "../shared/button-like.abstraction"; +import { FocusableElement } from "../shared/focusable-element"; export type IconButtonType = ButtonType | "contrast" | "main" | "muted" | "light"; @@ -123,9 +124,12 @@ const sizes: Record = { @Component({ selector: "button[bitIconButton]:not(button[bitButton])", templateUrl: "icon-button.component.html", - providers: [{ provide: ButtonLikeAbstraction, useExisting: BitIconButtonComponent }], + providers: [ + { provide: ButtonLikeAbstraction, useExisting: BitIconButtonComponent }, + { provide: FocusableElement, useExisting: BitIconButtonComponent }, + ], }) -export class BitIconButtonComponent implements ButtonLikeAbstraction { +export class BitIconButtonComponent implements ButtonLikeAbstraction, FocusableElement { @Input("bitIconButton") icon: string; @Input() buttonType: IconButtonType; @@ -162,4 +166,10 @@ export class BitIconButtonComponent implements ButtonLikeAbstraction { setButtonType(value: "primary" | "secondary" | "danger" | "unstyled") { this.buttonType = value; } + + getFocusTarget() { + return this.elementRef.nativeElement; + } + + constructor(private elementRef: ElementRef) {} } diff --git a/libs/components/src/index.ts b/libs/components/src/index.ts index 36185911a6..1e4a3a86ff 100644 --- a/libs/components/src/index.ts +++ b/libs/components/src/index.ts @@ -16,6 +16,7 @@ export * from "./form-field"; export * from "./icon-button"; export * from "./icon"; export * from "./input"; +export * from "./item"; export * from "./layout"; export * from "./link"; export * from "./menu"; diff --git a/libs/components/src/input/autofocus.directive.ts b/libs/components/src/input/autofocus.directive.ts index f8161ee6e0..625e7fbc92 100644 --- a/libs/components/src/input/autofocus.directive.ts +++ b/libs/components/src/input/autofocus.directive.ts @@ -3,12 +3,7 @@ import { take } from "rxjs/operators"; import { Utils } from "@bitwarden/common/platform/misc/utils"; -/** - * Interface for implementing focusable components. Used by the AutofocusDirective. - */ -export abstract class FocusableElement { - focus: () => void; -} +import { FocusableElement } from "../shared/focusable-element"; /** * Directive to focus an element. @@ -46,7 +41,7 @@ export class AutofocusDirective { private focus() { if (this.focusableElement) { - this.focusableElement.focus(); + this.focusableElement.getFocusTarget().focus(); } else { this.el.nativeElement.focus(); } diff --git a/libs/components/src/item/index.ts b/libs/components/src/item/index.ts new file mode 100644 index 0000000000..56896cdc3c --- /dev/null +++ b/libs/components/src/item/index.ts @@ -0,0 +1 @@ +export * from "./item.module"; diff --git a/libs/components/src/item/item-action.component.ts b/libs/components/src/item/item-action.component.ts new file mode 100644 index 0000000000..8cabf5c5c2 --- /dev/null +++ b/libs/components/src/item/item-action.component.ts @@ -0,0 +1,12 @@ +import { Component } from "@angular/core"; + +import { A11yCellDirective } from "../a11y/a11y-cell.directive"; + +@Component({ + selector: "bit-item-action", + standalone: true, + imports: [], + template: ``, + providers: [{ provide: A11yCellDirective, useExisting: ItemActionComponent }], +}) +export class ItemActionComponent extends A11yCellDirective {} diff --git a/libs/components/src/item/item-content.component.html b/libs/components/src/item/item-content.component.html new file mode 100644 index 0000000000..d034a4a001 --- /dev/null +++ b/libs/components/src/item/item-content.component.html @@ -0,0 +1,16 @@ +
+ + +
+
+ +
+
+ +
+
+
+ +
+ +
diff --git a/libs/components/src/item/item-content.component.ts b/libs/components/src/item/item-content.component.ts new file mode 100644 index 0000000000..58a1198512 --- /dev/null +++ b/libs/components/src/item/item-content.component.ts @@ -0,0 +1,15 @@ +import { CommonModule } from "@angular/common"; +import { ChangeDetectionStrategy, Component } from "@angular/core"; + +@Component({ + selector: "bit-item-content, [bit-item-content]", + standalone: true, + imports: [CommonModule], + templateUrl: `item-content.component.html`, + host: { + class: + "fvw-target tw-outline-none tw-text-main hover:tw-text-main hover:tw-no-underline tw-text-base tw-py-2 tw-px-4 tw-bg-transparent tw-w-full tw-border-none tw-flex tw-gap-4 tw-items-center tw-justify-between", + }, + changeDetection: ChangeDetectionStrategy.OnPush, +}) +export class ItemContentComponent {} diff --git a/libs/components/src/item/item-group.component.ts b/libs/components/src/item/item-group.component.ts new file mode 100644 index 0000000000..2a9a8275cc --- /dev/null +++ b/libs/components/src/item/item-group.component.ts @@ -0,0 +1,13 @@ +import { ChangeDetectionStrategy, Component } from "@angular/core"; + +@Component({ + selector: "bit-item-group", + standalone: true, + imports: [], + template: ``, + changeDetection: ChangeDetectionStrategy.OnPush, + host: { + class: "tw-block", + }, +}) +export class ItemGroupComponent {} diff --git a/libs/components/src/item/item.component.html b/libs/components/src/item/item.component.html new file mode 100644 index 0000000000..0c91c6848e --- /dev/null +++ b/libs/components/src/item/item.component.html @@ -0,0 +1,21 @@ + +
+ + + + +
+ +
+
diff --git a/libs/components/src/item/item.component.ts b/libs/components/src/item/item.component.ts new file mode 100644 index 0000000000..4b7b57fa9f --- /dev/null +++ b/libs/components/src/item/item.component.ts @@ -0,0 +1,29 @@ +import { CommonModule } from "@angular/common"; +import { ChangeDetectionStrategy, Component, HostListener, signal } from "@angular/core"; + +import { A11yRowDirective } from "../a11y/a11y-row.directive"; + +import { ItemActionComponent } from "./item-action.component"; + +@Component({ + selector: "bit-item", + standalone: true, + imports: [CommonModule, ItemActionComponent], + changeDetection: ChangeDetectionStrategy.OnPush, + templateUrl: "item.component.html", + providers: [{ provide: A11yRowDirective, useExisting: ItemComponent }], +}) +export class ItemComponent extends A11yRowDirective { + /** + * We have `:focus-within` and `:focus-visible` but no `:focus-visible-within` + */ + protected focusVisibleWithin = signal(false); + @HostListener("focusin", ["$event.target"]) + onFocusIn(target: HTMLElement) { + this.focusVisibleWithin.set(target.matches(".fvw-target:focus-visible")); + } + @HostListener("focusout") + onFocusOut() { + this.focusVisibleWithin.set(false); + } +} diff --git a/libs/components/src/item/item.mdx b/libs/components/src/item/item.mdx new file mode 100644 index 0000000000..8506de72bb --- /dev/null +++ b/libs/components/src/item/item.mdx @@ -0,0 +1,141 @@ +import { Meta, Story, Primary, Controls, Canvas } from "@storybook/addon-docs"; + +import * as stories from "./item.stories"; + + + +```ts +import { ItemModule } from "@bitwarden/components"; +``` + +# Item + +`` is a horizontal card that contains one or more interactive actions. + +It is a generic container that can be used for either standalone content, an alternative to tables, +or to list nav links. + + + + + +## Primary Content + +The primary content of an item is supplied by `bit-item-content`. + +### Content Types + +The content can be a button, anchor, or static container. + +```html + + Hi, I am a link. + + + + + + + + I'm just static :( + +``` + + + + + +### Content Slots + +`bit-item-content` contains the following slots to help position the content: + +| Slot | Description | +| ------------------ | --------------------------------------------------- | +| default | primary text or arbitrary content; fan favorite | +| `slot="secondary"` | supporting text; under the default slot | +| `slot="start"` | commonly an icon or avatar; before the default slot | +| `slot="end"` | commonly an icon; after the default slot | + +- Note: There is also an `end` slot within `bit-item` itself. Place + [interactive secondary actions](#secondary-actions) there, and place non-interactive content (such + as icons) in `bit-item-content` + +```html + + + +``` + + + + + +## Secondary Actions + +Secondary interactive actions can be placed in the item through the `"end"` slot, outside of +`bit-item-content`. + +Each action must be wrapped by ``. + +Actions are commonly icon buttons or badge buttons. + +```html + + + + + + + + + + + + + + + +``` + +## Item Groups + +Groups of items can be associated by wrapping them in the ``. + + + + + + + + + +### A11y + +Keyboard nav is currently disabled due to a bug when used within a virtual scroll viewport. + +Item groups utilize arrow-based keyboard navigation +([further reading here](https://www.w3.org/WAI/ARIA/apg/patterns/grid/examples/layout-grids/#kbd_label)). + +Use `aria-label` or `aria-labelledby` to give groups an accessible name. + +```html + + ... + ... + ... + +``` + +### Virtual Scrolling + + + + diff --git a/libs/components/src/item/item.module.ts b/libs/components/src/item/item.module.ts new file mode 100644 index 0000000000..226fed11d8 --- /dev/null +++ b/libs/components/src/item/item.module.ts @@ -0,0 +1,12 @@ +import { NgModule } from "@angular/core"; + +import { ItemActionComponent } from "./item-action.component"; +import { ItemContentComponent } from "./item-content.component"; +import { ItemGroupComponent } from "./item-group.component"; +import { ItemComponent } from "./item.component"; + +@NgModule({ + imports: [ItemComponent, ItemContentComponent, ItemActionComponent, ItemGroupComponent], + exports: [ItemComponent, ItemContentComponent, ItemActionComponent, ItemGroupComponent], +}) +export class ItemModule {} diff --git a/libs/components/src/item/item.stories.ts b/libs/components/src/item/item.stories.ts new file mode 100644 index 0000000000..b9d8d6cc2e --- /dev/null +++ b/libs/components/src/item/item.stories.ts @@ -0,0 +1,326 @@ +import { ScrollingModule } from "@angular/cdk/scrolling"; +import { CommonModule } from "@angular/common"; +import { Meta, StoryObj, componentWrapperDecorator, moduleMetadata } from "@storybook/angular"; + +import { A11yGridDirective } from "../a11y/a11y-grid.directive"; +import { AvatarModule } from "../avatar"; +import { BadgeModule } from "../badge"; +import { IconButtonModule } from "../icon-button"; +import { TypographyModule } from "../typography"; + +import { ItemActionComponent } from "./item-action.component"; +import { ItemContentComponent } from "./item-content.component"; +import { ItemGroupComponent } from "./item-group.component"; +import { ItemComponent } from "./item.component"; + +export default { + title: "Component Library/Item", + component: ItemComponent, + decorators: [ + moduleMetadata({ + imports: [ + CommonModule, + ItemGroupComponent, + AvatarModule, + IconButtonModule, + BadgeModule, + TypographyModule, + ItemActionComponent, + ItemContentComponent, + A11yGridDirective, + ScrollingModule, + ], + }), + componentWrapperDecorator((story) => `
${story}
`), + ], +} as Meta; + +type Story = StoryObj; + +export const Default: Story = { + render: (args) => ({ + props: args, + template: /*html*/ ` + + + + + + + + + + + + + + + + `, + }), +}; + +export const ContentSlots: Story = { + render: (args) => ({ + props: args, + template: /*html*/ ` + + + + `, + }), +}; + +export const ContentTypes: Story = { + render: (args) => ({ + props: args, + template: /*html*/ ` + + + Hi, I am a link. + + + + + + + + I'm just static :( + + + `, + }), +}; + +export const TextOverflow: Story = { + render: (args) => ({ + props: args, + template: /*html*/ ` +
TODO: Fix truncation
+ + + Helloooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo + + + `, + }), +}; + +export const MultipleActionList: Story = { + render: (args) => ({ + props: args, + template: /*html*/ ` + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + `, + }), +}; + +export const SingleActionList: Story = { + render: (args) => ({ + props: args, + template: /*html*/ ` + + + + Foobar + + + + + + Foobar + + + + + + Foobar + + + + + + Foobar + + + + + + Foobar + + + + + + Foobar + + + + + `, + }), +}; + +export const VirtualScrolling: Story = { + render: (_args) => ({ + props: { + data: Array.from(Array(100000).keys()), + }, + template: /*html*/ ` + + + + + + + + + + + + + + + + + + + + `, + }), +}; diff --git a/libs/components/src/search/search.component.ts b/libs/components/src/search/search.component.ts index a0f3eb363f..27170d5d7b 100644 --- a/libs/components/src/search/search.component.ts +++ b/libs/components/src/search/search.component.ts @@ -1,7 +1,7 @@ import { Component, ElementRef, Input, ViewChild } from "@angular/core"; import { ControlValueAccessor, NG_VALUE_ACCESSOR } from "@angular/forms"; -import { FocusableElement } from "../input/autofocus.directive"; +import { FocusableElement } from "../shared/focusable-element"; let nextId = 0; @@ -32,8 +32,8 @@ export class SearchComponent implements ControlValueAccessor, FocusableElement { @Input() disabled: boolean; @Input() placeholder: string; - focus() { - this.input.nativeElement.focus(); + getFocusTarget() { + return this.input.nativeElement; } onChange(searchText: string) { diff --git a/libs/components/src/shared/focusable-element.ts b/libs/components/src/shared/focusable-element.ts new file mode 100644 index 0000000000..1ea422aa6f --- /dev/null +++ b/libs/components/src/shared/focusable-element.ts @@ -0,0 +1,8 @@ +/** + * Interface for implementing focusable components. + * + * Used by the `AutofocusDirective` and `A11yGridDirective`. + */ +export abstract class FocusableElement { + getFocusTarget: () => HTMLElement; +} diff --git a/libs/components/src/styles.scss b/libs/components/src/styles.scss index ae97838e09..7ddcb1b64b 100644 --- a/libs/components/src/styles.scss +++ b/libs/components/src/styles.scss @@ -49,6 +49,6 @@ $card-icons-base: "../../src/billing/images/cards/"; @import "multi-select/scss/bw.theme.scss"; // Workaround for https://bitwarden.atlassian.net/browse/CL-110 -#storybook-docs pre.prismjs { +.sbdocs-preview pre.prismjs { color: white; } From 418d4642da81e09de6a4b445042a26592a017c98 Mon Sep 17 00:00:00 2001 From: Alex Morask <144709477+amorask-bitwarden@users.noreply.github.com> Date: Tue, 30 Apr 2024 10:55:00 -0400 Subject: [PATCH 8/8] Hide grace period note when in self-serve trial (#8768) --- ...organization-subscription-selfhost.component.html | 5 ++++- .../self-hosted-organization-subscription.view.ts | 12 ++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/apps/web/src/app/billing/organizations/organization-subscription-selfhost.component.html b/apps/web/src/app/billing/organizations/organization-subscription-selfhost.component.html index 6d6691f336..b4c1224db9 100644 --- a/apps/web/src/app/billing/organizations/organization-subscription-selfhost.component.html +++ b/apps/web/src/app/billing/organizations/organization-subscription-selfhost.component.html @@ -42,7 +42,10 @@ : subscription.expirationWithGracePeriod ) | date: "mediumDate" }} -
+
{{ "selfHostGracePeriodHelp" | i18n: (subscription.expirationWithGracePeriod | date: "mediumDate") diff --git a/libs/common/src/billing/models/view/self-hosted-organization-subscription.view.ts b/libs/common/src/billing/models/view/self-hosted-organization-subscription.view.ts index c1f5640207..7b49688294 100644 --- a/libs/common/src/billing/models/view/self-hosted-organization-subscription.view.ts +++ b/libs/common/src/billing/models/view/self-hosted-organization-subscription.view.ts @@ -58,4 +58,16 @@ export class SelfHostedOrganizationSubscriptionView implements View { get isExpiredAndOutsideGracePeriod() { return this.hasExpiration && this.expirationWithGracePeriod < new Date(); } + + /** + * In the case of a trial, where there is no grace period, the expirationWithGracePeriod and expirationWithoutGracePeriod will + * be exactly the same. This can be used to hide the grace period note. + */ + get isInTrial() { + return ( + this.expirationWithGracePeriod && + this.expirationWithoutGracePeriod && + this.expirationWithGracePeriod.getTime() === this.expirationWithoutGracePeriod.getTime() + ); + } }