From 076e605f101646523387ce620bac11c932b66ab7 Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Fri, 18 Nov 2022 16:38:28 -0500 Subject: [PATCH] [PS-1879] Fix Key Connector Migration Flow (#4080) * Move OrganizationService to fullSync * Add Tech Debt Tracking Link * Remove Commented out code * Add InternalOrganizationService * Use InternalOrganization in services that get to update state --- .../browser/src/background/main.background.ts | 8 ++-- .../organization-service.factory.ts | 11 +---- apps/cli/src/bw.ts | 4 +- apps/web/webpack.config.js | 1 + .../src/services/jslib-services.module.ts | 15 ++++-- .../organization/organization.service.spec.ts | 48 ++----------------- .../organization.service.abstraction.ts | 5 ++ .../organization/organization.service.ts | 45 ++++------------- libs/common/src/services/sync/sync.service.ts | 23 ++++++--- 9 files changed, 54 insertions(+), 106 deletions(-) diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index cc973a8c82..581da587e4 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -17,7 +17,7 @@ import { KeyConnectorService as KeyConnectorServiceAbstraction } from "@bitwarde import { LogService as LogServiceAbstraction } from "@bitwarden/common/abstractions/log.service"; import { MessagingService as MessagingServiceAbstraction } from "@bitwarden/common/abstractions/messaging.service"; import { NotificationsService as NotificationsServiceAbstraction } from "@bitwarden/common/abstractions/notifications.service"; -import { OrganizationService as OrganizationServiceAbstraction } from "@bitwarden/common/abstractions/organization/organization.service.abstraction"; +import { InternalOrganizationService as InternalOrganizationServiceAbstraction } from "@bitwarden/common/abstractions/organization/organization.service.abstraction"; import { PasswordGenerationService as PasswordGenerationServiceAbstraction } from "@bitwarden/common/abstractions/passwordGeneration.service"; import { PlatformUtilsService as PlatformUtilsServiceAbstraction } from "@bitwarden/common/abstractions/platformUtils.service"; import { PolicyApiServiceAbstraction } from "@bitwarden/common/abstractions/policy/policy-api.service.abstraction"; @@ -153,7 +153,7 @@ export default class MainBackground { popupUtilsService: PopupUtilsService; sendService: SendServiceAbstraction; fileUploadService: FileUploadServiceAbstraction; - organizationService: OrganizationServiceAbstraction; + organizationService: InternalOrganizationServiceAbstraction; providerService: ProviderServiceAbstraction; keyConnectorService: KeyConnectorServiceAbstraction; userVerificationService: UserVerificationServiceAbstraction; @@ -317,7 +317,7 @@ export default class MainBackground { this.stateService ); this.syncNotifierService = new SyncNotifierService(); - this.organizationService = new OrganizationService(this.stateService, this.syncNotifierService); + this.organizationService = new OrganizationService(this.stateService); this.policyService = new PolicyService(this.stateService, this.organizationService); this.policyApiService = new PolicyApiService( this.policyService, @@ -409,7 +409,7 @@ export default class MainBackground { this.stateService, this.providerService, this.folderApiService, - this.syncNotifierService, + this.organizationService, logoutCallback ); this.eventService = new EventService( diff --git a/apps/browser/src/background/service_factories/organization-service.factory.ts b/apps/browser/src/background/service_factories/organization-service.factory.ts index 2e7c31b596..ea11d32e26 100644 --- a/apps/browser/src/background/service_factories/organization-service.factory.ts +++ b/apps/browser/src/background/service_factories/organization-service.factory.ts @@ -3,15 +3,10 @@ import { OrganizationService } from "@bitwarden/common/services/organization/org import { FactoryOptions, CachedServices, factory } from "./factory-options"; import { stateServiceFactory, StateServiceInitOptions } from "./state-service.factory"; -import { - syncNotifierServiceFactory, - SyncNotifierServiceInitOptions, -} from "./sync-notifier-service.factory"; type OrganizationServiceFactoryOptions = FactoryOptions; export type OrganizationServiceInitOptions = OrganizationServiceFactoryOptions & - SyncNotifierServiceInitOptions & StateServiceInitOptions; export function organizationServiceFactory( @@ -22,10 +17,6 @@ export function organizationServiceFactory( cache, "organizationService", opts, - async () => - new OrganizationService( - await stateServiceFactory(cache, opts), - await syncNotifierServiceFactory(cache, opts) - ) + async () => new OrganizationService(await stateServiceFactory(cache, opts)) ); } diff --git a/apps/cli/src/bw.ts b/apps/cli/src/bw.ts index 8f877e7fbf..1cb03db67b 100644 --- a/apps/cli/src/bw.ts +++ b/apps/cli/src/bw.ts @@ -240,7 +240,7 @@ export class Main { this.providerService = new ProviderService(this.stateService); - this.organizationService = new OrganizationService(this.stateService, this.syncNotifierService); + this.organizationService = new OrganizationService(this.stateService); this.policyService = new PolicyService(this.stateService, this.organizationService); @@ -322,7 +322,7 @@ export class Main { this.stateService, this.providerService, this.folderApiService, - this.syncNotifierService, + this.organizationService, async (expired: boolean) => await this.logout() ); diff --git a/apps/web/webpack.config.js b/apps/web/webpack.config.js index cecfbea399..2000fe877a 100644 --- a/apps/web/webpack.config.js +++ b/apps/web/webpack.config.js @@ -273,6 +273,7 @@ const devServer = https://quack.duckduckgo.com/api/email/addresses https://app.anonaddy.com/api/v1/aliases https://api.fastmail.com + http://localhost:5000 ;object-src 'self' blob: diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index 5f49041bd7..5399c90e63 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -35,7 +35,10 @@ import { LoginService as LoginServiceAbstraction } from "@bitwarden/common/abstr import { MessagingService as MessagingServiceAbstraction } from "@bitwarden/common/abstractions/messaging.service"; import { NotificationsService as NotificationsServiceAbstraction } from "@bitwarden/common/abstractions/notifications.service"; import { OrganizationApiServiceAbstraction } from "@bitwarden/common/abstractions/organization/organization-api.service.abstraction"; -import { OrganizationService as OrganizationServiceAbstraction } from "@bitwarden/common/abstractions/organization/organization.service.abstraction"; +import { + InternalOrganizationService, + OrganizationService as OrganizationServiceAbstraction, +} from "@bitwarden/common/abstractions/organization/organization.service.abstraction"; import { PasswordGenerationService as PasswordGenerationServiceAbstraction } from "@bitwarden/common/abstractions/passwordGeneration.service"; import { PasswordRepromptService as PasswordRepromptServiceAbstraction } from "@bitwarden/common/abstractions/passwordReprompt.service"; import { PlatformUtilsService as PlatformUtilsServiceAbstraction } from "@bitwarden/common/abstractions/platformUtils.service"; @@ -356,7 +359,7 @@ import { AbstractThemingService } from "./theming/theming.service.abstraction"; StateServiceAbstraction, ProviderServiceAbstraction, FolderApiServiceAbstraction, - SyncNotifierServiceAbstraction, + OrganizationServiceAbstraction, LOGOUT_CALLBACK, ], }, @@ -506,6 +509,8 @@ import { AbstractThemingService } from "./theming/theming.service.abstraction"; LogService, OrganizationServiceAbstraction, CryptoFunctionServiceAbstraction, + SyncNotifierServiceAbstraction, + MessagingServiceAbstraction, LOGOUT_CALLBACK, ], }, @@ -522,7 +527,11 @@ import { AbstractThemingService } from "./theming/theming.service.abstraction"; { provide: OrganizationServiceAbstraction, useClass: OrganizationService, - deps: [StateServiceAbstraction, SyncNotifierServiceAbstraction], + deps: [StateServiceAbstraction], + }, + { + provide: InternalOrganizationService, + useExisting: OrganizationServiceAbstraction, }, { provide: ProviderServiceAbstraction, diff --git a/libs/common/spec/services/organization/organization.service.spec.ts b/libs/common/spec/services/organization/organization.service.spec.ts index 8722673571..42a327c18f 100644 --- a/libs/common/spec/services/organization/organization.service.spec.ts +++ b/libs/common/spec/services/organization/organization.service.spec.ts @@ -1,12 +1,9 @@ -import { MockProxy, mock, any, mockClear, matches } from "jest-mock-extended"; -import { BehaviorSubject, firstValueFrom, Subject } from "rxjs"; +import { MockProxy, mock, any, mockClear } from "jest-mock-extended"; +import { BehaviorSubject, firstValueFrom } from "rxjs"; import { StateService } from "@bitwarden/common/abstractions/state.service"; -import { SyncNotifierService } from "@bitwarden/common/abstractions/sync/syncNotifier.service.abstraction"; import { OrganizationData } from "@bitwarden/common/models/data/organization.data"; -import { SyncResponse } from "@bitwarden/common/models/response/sync.response"; import { OrganizationService } from "@bitwarden/common/services/organization/organization.service"; -import { SyncEventArgs } from "@bitwarden/common/types/syncEventArgs"; describe("Organization Service", () => { let organizationService: OrganizationService; @@ -14,8 +11,6 @@ describe("Organization Service", () => { let stateService: MockProxy; let activeAccount: BehaviorSubject; let activeAccountUnlocked: BehaviorSubject; - let syncNotifierService: MockProxy; - let sync: Subject; const resetStateService = async ( customizeStateService: (stateService: MockProxy) => void @@ -25,7 +20,7 @@ describe("Organization Service", () => { stateService.activeAccount$ = activeAccount; stateService.activeAccountUnlocked$ = activeAccountUnlocked; customizeStateService(stateService); - organizationService = new OrganizationService(stateService, syncNotifierService); + organizationService = new OrganizationService(stateService); await new Promise((r) => setTimeout(r, 50)); }; @@ -41,12 +36,7 @@ describe("Organization Service", () => { "1": organizationData("1", "Test Org"), }); - sync = new Subject(); - - syncNotifierService = mock(); - syncNotifierService.sync$ = sync; - - organizationService = new OrganizationService(stateService, syncNotifierService); + organizationService = new OrganizationService(stateService); }); afterEach(() => { @@ -169,36 +159,6 @@ describe("Organization Service", () => { }); }); - describe("syncEvent works", () => { - it("Complete event updates data", async () => { - sync.next({ - status: "Completed", - successfully: true, - data: new SyncResponse({ - profile: { - organizations: [ - { - id: "1", - name: "Updated Name", - }, - ], - }, - }), - }); - - await new Promise((r) => setTimeout(r, 500)); - - expect(stateService.setOrganizations).toHaveBeenCalledTimes(1); - - expect(stateService.setOrganizations).toHaveBeenLastCalledWith( - matches((organizationData: { [id: string]: OrganizationData }) => { - const organization = organizationData["1"]; - return organization?.name === "Updated Name"; - }) - ); - }); - }); - function organizationData(id: string, name: string) { const data = new OrganizationData({} as any); data.id = id; diff --git a/libs/common/src/abstractions/organization/organization.service.abstraction.ts b/libs/common/src/abstractions/organization/organization.service.abstraction.ts index 3dfeb90cb0..68c64654d5 100644 --- a/libs/common/src/abstractions/organization/organization.service.abstraction.ts +++ b/libs/common/src/abstractions/organization/organization.service.abstraction.ts @@ -1,6 +1,7 @@ import { map, Observable } from "rxjs"; import { Utils } from "../../misc/utils"; +import { OrganizationData } from "../../models/data/organization.data"; import { Organization } from "../../models/domain/organization"; import { I18nService } from "../i18n.service"; @@ -83,3 +84,7 @@ export abstract class OrganizationService { canManageSponsorships: () => Promise; hasOrganizations: () => boolean; } + +export abstract class InternalOrganizationService extends OrganizationService { + replace: (organizations: { [id: string]: OrganizationData }) => Promise; +} diff --git a/libs/common/src/services/organization/organization.service.ts b/libs/common/src/services/organization/organization.service.ts index 101069adad..5d936c53ae 100644 --- a/libs/common/src/services/organization/organization.service.ts +++ b/libs/common/src/services/organization/organization.service.ts @@ -1,21 +1,16 @@ -import { BehaviorSubject, concatMap, filter } from "rxjs"; +import { BehaviorSubject, concatMap } from "rxjs"; -import { OrganizationService as OrganizationServiceAbstraction } from "../../abstractions/organization/organization.service.abstraction"; +import { InternalOrganizationService as InternalOrganizationServiceAbstraction } from "../../abstractions/organization/organization.service.abstraction"; import { StateService } from "../../abstractions/state.service"; -import { SyncNotifierService } from "../../abstractions/sync/syncNotifier.service.abstraction"; import { OrganizationData } from "../../models/data/organization.data"; import { Organization } from "../../models/domain/organization"; -import { isSuccessfullyCompleted } from "../../types/syncEventArgs"; -export class OrganizationService implements OrganizationServiceAbstraction { +export class OrganizationService implements InternalOrganizationServiceAbstraction { private _organizations = new BehaviorSubject([]); organizations$ = this._organizations.asObservable(); - constructor( - private stateService: StateService, - private syncNotifierService: SyncNotifierService - ) { + constructor(private stateService: StateService) { this.stateService.activeAccountUnlocked$ .pipe( concatMap(async (unlocked) => { @@ -29,28 +24,6 @@ export class OrganizationService implements OrganizationServiceAbstraction { }) ) .subscribe(); - - this.syncNotifierService.sync$ - .pipe( - filter(isSuccessfullyCompleted), - concatMap(async ({ data }) => { - const { profile } = data; - const organizations: { [id: string]: OrganizationData } = {}; - profile.organizations.forEach((o) => { - organizations[o.id] = new OrganizationData(o); - }); - - profile.providerOrganizations.forEach((o) => { - if (organizations[o.id] == null) { - organizations[o.id] = new OrganizationData(o); - organizations[o.id].isProviderUser = true; - } - }); - - await this.updateStateAndObservables(organizations); - }) - ) - .subscribe(); } async getAll(userId?: string): Promise { @@ -78,7 +51,7 @@ export class OrganizationService implements OrganizationServiceAbstraction { organizations[organization.id] = organization; - await this.updateStateAndObservables(organizations); + await this.replace(organizations); } async delete(id: string): Promise { @@ -92,7 +65,7 @@ export class OrganizationService implements OrganizationServiceAbstraction { } delete organizations[id]; - await this.updateStateAndObservables(organizations); + await this.replace(organizations); } get(id: string): Organization { @@ -121,9 +94,9 @@ export class OrganizationService implements OrganizationServiceAbstraction { return organizations.find((organization) => organization.identifier === identifier); } - private async updateStateAndObservables(organizationsMap: { [id: string]: OrganizationData }) { - await this.stateService.setOrganizations(organizationsMap); - this.updateObservables(organizationsMap); + async replace(organizations: { [id: string]: OrganizationData }) { + await this.stateService.setOrganizations(organizations); + this.updateObservables(organizations); } private updateObservables(organizationsMap: { [id: string]: OrganizationData }) { diff --git a/libs/common/src/services/sync/sync.service.ts b/libs/common/src/services/sync/sync.service.ts index 2513871636..cd3f8e40ff 100644 --- a/libs/common/src/services/sync/sync.service.ts +++ b/libs/common/src/services/sync/sync.service.ts @@ -7,17 +7,18 @@ import { InternalFolderService } from "../../abstractions/folder/folder.service. import { KeyConnectorService } from "../../abstractions/keyConnector.service"; import { LogService } from "../../abstractions/log.service"; import { MessagingService } from "../../abstractions/messaging.service"; +import { InternalOrganizationService } from "../../abstractions/organization/organization.service.abstraction"; import { InternalPolicyService } from "../../abstractions/policy/policy.service.abstraction"; import { ProviderService } from "../../abstractions/provider.service"; import { SendService } from "../../abstractions/send.service"; import { SettingsService } from "../../abstractions/settings.service"; import { StateService } from "../../abstractions/state.service"; import { SyncService as SyncServiceAbstraction } from "../../abstractions/sync/sync.service.abstraction"; -import { SyncNotifierService } from "../../abstractions/sync/syncNotifier.service.abstraction"; import { sequentialize } from "../../misc/sequentialize"; import { CipherData } from "../../models/data/cipher.data"; import { CollectionData } from "../../models/data/collection.data"; import { FolderData } from "../../models/data/folder.data"; +import { OrganizationData } from "../../models/data/organization.data"; import { PolicyData } from "../../models/data/policy.data"; import { ProviderData } from "../../models/data/provider.data"; import { SendData } from "../../models/data/send.data"; @@ -52,7 +53,7 @@ export class SyncService implements SyncServiceAbstraction { private stateService: StateService, private providerService: ProviderService, private folderApiService: FolderApiServiceAbstraction, - private syncNotifierService: SyncNotifierService, + private organizationService: InternalOrganizationService, private logoutCallback: (expired: boolean) => Promise ) {} @@ -76,10 +77,8 @@ export class SyncService implements SyncServiceAbstraction { @sequentialize(() => "fullSync") async fullSync(forceSync: boolean, allowThrowOnError = false): Promise { this.syncStarted(); - this.syncNotifierService.next({ status: "Started" }); const isAuthenticated = await this.stateService.getIsAuthenticated(); if (!isAuthenticated) { - this.syncNotifierService.next({ status: "Completed", successfully: false }); return this.syncCompleted(false); } @@ -95,7 +94,6 @@ export class SyncService implements SyncServiceAbstraction { if (!needsSync) { await this.setLastSync(now); - this.syncNotifierService.next({ status: "Completed", successfully: false }); return this.syncCompleted(false); } @@ -112,13 +110,11 @@ export class SyncService implements SyncServiceAbstraction { await this.syncPolicies(response.policies); await this.setLastSync(now); - this.syncNotifierService.next({ status: "Completed", successfully: true, data: response }); return this.syncCompleted(true); } catch (e) { if (allowThrowOnError) { throw e; } else { - this.syncNotifierService.next({ status: "Completed", successfully: false }); return this.syncCompleted(false); } } @@ -315,11 +311,24 @@ export class SyncService implements SyncServiceAbstraction { await this.stateService.setForcePasswordReset(response.forcePasswordReset); await this.keyConnectorService.setUsesKeyConnector(response.usesKeyConnector); + const organizations: { [id: string]: OrganizationData } = {}; + response.organizations.forEach((o) => { + organizations[o.id] = new OrganizationData(o); + }); + const providers: { [id: string]: ProviderData } = {}; response.providers.forEach((p) => { providers[p.id] = new ProviderData(p); }); + response.providerOrganizations.forEach((o) => { + if (organizations[o.id] == null) { + organizations[o.id] = new OrganizationData(o); + organizations[o.id].isProviderUser = true; + } + }); + + await this.organizationService.replace(organizations); await this.providerService.save(providers); if (await this.keyConnectorService.userNeedsMigration()) {