From 9e3d1caee4c8f986fb6acff0cff7fc2375c53491 Mon Sep 17 00:00:00 2001 From: Addison Beck Date: Thu, 20 Jan 2022 16:31:46 -0500 Subject: [PATCH] [chore] Update jslib & state services to match (#212) * [chore] Update jslib & state services to match * [bug] Save userId when migrating state This is used to check for authentication, so if not present on boot of the app authenticated users will still have to log in again * [bug] Save added accounts with userId Currently we are passing in an account object, resulting in a null key. We should be passing in a userId * [bug] Ensure configs and settings are not cleared on logout We need to persist directoryConfigruations on logout so that logging out and back in doesn't require folks to need to reconfig their settings * Remove unneeded LoginSyncService * Run prettier * [style] Remove commented lines Co-authored-by: Thomas Rittson --- jslib | 2 +- src/app/services/services.module.ts | 19 +++- src/bwdc.ts | 28 ++---- src/main.ts | 13 ++- src/models/account.ts | 4 +- src/program.ts | 4 +- src/services/state.service.ts | 64 +++++++++--- src/services/stateMigration.service.ts | 129 ++++++++++++------------- 8 files changed, 155 insertions(+), 108 deletions(-) diff --git a/jslib b/jslib index b0dcc235..cf1e483c 160000 --- a/jslib +++ b/jslib @@ -1 +1 @@ -Subproject commit b0dcc2353f9a6028d64d40215b61b6c651c21684 +Subproject commit cf1e483c7fa4c3ea16d1981eb833e286f034806d diff --git a/src/app/services/services.module.ts b/src/app/services/services.module.ts index a7204767..58ebdba1 100644 --- a/src/app/services/services.module.ts +++ b/src/app/services/services.module.ts @@ -43,6 +43,10 @@ import { AuthService } from "../../services/auth.service"; import { StateService } from "../../services/state.service"; import { StateMigrationService } from "../../services/stateMigration.service"; +import { Account } from "../../models/account"; + +import { AccountFactory } from "jslib-common/models/domain/account"; + function refreshTokenCallback(injector: Injector) { return () => { const stateService = injector.get(StateServiceAbstraction); @@ -197,7 +201,20 @@ export function initFactory( }, { provide: StateServiceAbstraction, - useClass: StateService, + useFactory: ( + storageService: StorageServiceAbstraction, + secureStorageService: StorageServiceAbstraction, + logService: LogServiceAbstraction, + stateMigrationService: StateMigrationServiceAbstraction + ) => + new StateService( + storageService, + secureStorageService, + logService, + stateMigrationService, + true, // TODO: It seems like we aren't applying this from settings anywhere. Is toggling secure storage working? + new AccountFactory(Account) + ), deps: [ StorageServiceAbstraction, "SECURE_STORAGE", diff --git a/src/bwdc.ts b/src/bwdc.ts index 669d0c49..8978daa9 100644 --- a/src/bwdc.ts +++ b/src/bwdc.ts @@ -34,13 +34,15 @@ import { ProviderService } from "jslib-common/services/provider.service"; import { SearchService } from "jslib-common/services/search.service"; import { SendService } from "jslib-common/services/send.service"; import { SettingsService } from "jslib-common/services/settings.service"; -import { SyncService as LoginSyncService } from "jslib-common/services/sync.service"; import { TokenService } from "jslib-common/services/token.service"; import { StorageService as StorageServiceAbstraction } from "jslib-common/abstractions/storage.service"; import { Program } from "./program"; -import { refreshToken } from "./services/api.service"; + +import { AccountFactory } from "jslib-common/models/domain/account"; + +import { Account } from "./models/account"; // tslint:disable-next-line const packageJson = require("./package.json"); @@ -72,7 +74,6 @@ export class Main { syncService: SyncService; passwordGenerationService: PasswordGenerationService; policyService: PolicyService; - loginSyncService: LoginSyncService; keyConnectorService: KeyConnectorService; program: Program; stateService: StateService; @@ -130,7 +131,8 @@ export class Main { this.secureStorageService, this.logService, this.stateMigrationService, - process.env.BITWARDENCLI_CONNECTOR_PLAINTEXT_SECRETS !== "true" + process.env.BITWARDENCLI_CONNECTOR_PLAINTEXT_SECRETS !== "true", + new AccountFactory(Account) ); this.cryptoService = new CryptoService( @@ -249,24 +251,6 @@ export class Main { this.providerService = new ProviderService(this.stateService); - this.loginSyncService = new LoginSyncService( - this.apiService, - this.settingsService, - this.folderService, - this.cipherService, - this.cryptoService, - this.collectionService, - this.messagingService, - this.policyService, - this.sendService, - this.logService, - this.keyConnectorService, - this.stateService, - this.organizationService, - this.providerService, - async (expired: boolean) => this.messagingService.send("logout", { expired: expired }) - ); - this.program = new Program(this); } diff --git a/src/main.ts b/src/main.ts index 19df477d..df31d76a 100644 --- a/src/main.ts +++ b/src/main.ts @@ -15,6 +15,10 @@ import { WindowMain } from "jslib-electron/window.main"; import { StateService } from "./services/state.service"; +import { AccountFactory } from "jslib-common/models/domain/account"; + +import { Account } from "./models/account"; + export class Main { logService: ElectronLogService; i18nService: I18nService; @@ -54,7 +58,14 @@ export class Main { this.logService = new ElectronLogService(null, app.getPath("userData")); this.i18nService = new I18nService("en", "./locales/"); this.storageService = new ElectronStorageService(app.getPath("userData")); - this.stateService = new StateService(this.storageService, null, this.logService, null); + this.stateService = new StateService( + this.storageService, + null, + this.logService, + null, + true, + new AccountFactory(Account) + ); this.windowMain = new WindowMain( this.stateService, diff --git a/src/models/account.ts b/src/models/account.ts index c63e750b..c80e687d 100644 --- a/src/models/account.ts +++ b/src/models/account.ts @@ -16,8 +16,8 @@ export class Account extends BaseAccount { constructor(init: Partial) { super(init); - this.directoryConfigurations = init.directoryConfigurations ?? new DirectoryConfigurations(); - this.directorySettings = init.directorySettings ?? new DirectorySettings(); + this.directoryConfigurations = init?.directoryConfigurations ?? new DirectoryConfigurations(); + this.directorySettings = init?.directorySettings ?? new DirectorySettings(); } } diff --git a/src/program.ts b/src/program.ts index 484bb632..90b6c44b 100644 --- a/src/program.ts +++ b/src/program.ts @@ -106,9 +106,7 @@ export class Program extends BaseProgram { this.main.stateService, this.main.cryptoService, this.main.policyService, - "connector", - this.main.loginSyncService, - this.main.keyConnectorService + "connector" ); if (!Utils.isNullOrWhitespace(clientId)) { diff --git a/src/services/state.service.ts b/src/services/state.service.ts index 87bb346c..4edefeee 100644 --- a/src/services/state.service.ts +++ b/src/services/state.service.ts @@ -1,6 +1,6 @@ import { StateService as BaseStateService } from "jslib-common/services/state.service"; -import { State } from "jslib-common/models/domain/state"; +import { AccountFactory } from "jslib-common/models/domain/account"; import { StorageOptions } from "jslib-common/models/domain/storageOptions"; import { Account } from "src/models/account"; @@ -10,13 +10,15 @@ import { IConfiguration } from "src/models/IConfiguration"; import { LdapConfiguration } from "src/models/ldapConfiguration"; import { OktaConfiguration } from "src/models/oktaConfiguration"; import { OneLoginConfiguration } from "src/models/oneLoginConfiguration"; +import { SyncConfiguration } from "src/models/syncConfiguration"; import { LogService } from "jslib-common/abstractions/log.service"; +import { StateMigrationService } from "jslib-common/abstractions/stateMigration.service"; import { StorageService } from "jslib-common/abstractions/storage.service"; + import { StateService as StateServiceAbstraction } from "src/abstractions/state.service"; + import { DirectoryType } from "src/enums/directoryType"; -import { SyncConfiguration } from "src/models/syncConfiguration"; -import { StateMigrationService } from "./stateMigration.service"; const SecureStorageKeys = { ldap: "ldapPassword", @@ -31,6 +33,12 @@ const SecureStorageKeys = { lastSyncHash: "lastSyncHash", }; +const keys = { + tempAccountSettings: "tempAccountSettings", + tempDirectoryConfigs: "tempDirectoryConfigs", + tempDirectorySettings: "tempDirectorySettings", +}; + const StoredSecurely = "[STORED SECURELY]"; export class StateService extends BaseStateService implements StateServiceAbstraction { @@ -39,9 +47,10 @@ export class StateService extends BaseStateService implements StateServ protected secureStorageService: StorageService, protected logService: LogService, protected stateMigrationService: StateMigrationService, - private useSecureStorageForSecrets = true + private useSecureStorageForSecrets = true, + protected accountFactory: AccountFactory ) { - super(storageService, secureStorageService, logService, stateMigrationService); + super(storageService, secureStorageService, logService, stateMigrationService, accountFactory); } async getDirectory(type: DirectoryType): Promise { @@ -520,19 +529,33 @@ export class StateService extends BaseStateService implements StateServ } protected async scaffoldNewAccountDiskStorage(account: Account): Promise { - const storedState = - (await this.storageService.get>( - "state", + const storedAccount = await this.getAccount( + this.reconcileOptions( + { userId: account.profile.userId }, await this.defaultOnDiskLocalOptions() - )) ?? new State(); - const storedAccount = storedState.accounts[account.profile.userId]; + ) + ); if (storedAccount != null) { account.settings = storedAccount.settings; account.directorySettings = storedAccount.directorySettings; account.directoryConfigurations = storedAccount.directoryConfigurations; + } else if (await this.hasTemporaryStorage()) { + // If migrating to state V2 with an no actively authed account we store temporary data to be copied on auth - this will only be run once. + account.settings = await this.storageService.get(keys.tempAccountSettings); + account.directorySettings = await this.storageService.get(keys.tempDirectorySettings); + account.directoryConfigurations = await this.storageService.get( + keys.tempDirectoryConfigs + ); + await this.storageService.remove(keys.tempAccountSettings); + await this.storageService.remove(keys.tempDirectorySettings); + await this.storageService.remove(keys.tempDirectoryConfigs); } - storedState.accounts[account.profile.userId] = account; - await this.saveStateToStorage(storedState, await this.defaultOnDiskLocalOptions()); + + await this.storageService.save( + account.profile.userId, + account, + await this.defaultOnDiskLocalOptions() + ); } protected async pushAccounts(): Promise { @@ -542,4 +565,21 @@ export class StateService extends BaseStateService implements StateServ } this.accounts.next(this.state.accounts); } + + protected async hasTemporaryStorage(): Promise { + return ( + (await this.storageService.has(keys.tempAccountSettings)) || + (await this.storageService.has(keys.tempDirectorySettings)) || + (await this.storageService.has(keys.tempDirectoryConfigs)) + ); + } + + protected resetAccount(account: Account) { + const persistentAccountInformation = { + settings: account.settings, + directorySettings: account.directorySettings, + directoryConfigurations: account.directoryConfigurations, + }; + return Object.assign(this.createAccount(), persistentAccountInformation); + } } diff --git a/src/services/stateMigration.service.ts b/src/services/stateMigration.service.ts index fe06eac0..1d92b49e 100644 --- a/src/services/stateMigration.service.ts +++ b/src/services/stateMigration.service.ts @@ -1,6 +1,3 @@ -import { HtmlStorageLocation } from "jslib-common/enums/htmlStorageLocation"; -import { State } from "jslib-common/models/domain/state"; - import { StateMigrationService as BaseStateMigrationService } from "jslib-common/services/stateMigration.service"; import { StateVersion } from "jslib-common/enums/stateVersion"; @@ -30,7 +27,6 @@ const SecureStorageKeys: { [key: string]: any } = { }; const Keys: { [key: string]: any } = { - state: "state", entityId: "entityId", directoryType: "directoryType", organizationId: "organizationId", @@ -39,6 +35,8 @@ const Keys: { [key: string]: any } = { lastSyncHash: "lastSyncHash", syncingDir: "syncingDir", syncConfig: "syncConfig", + tempDirectoryConfigs: "tempDirectoryConfigs", + tempDirectorySettings: "tempDirectorySettings", }; const ClientKeys: { [key: string]: any } = { @@ -50,8 +48,7 @@ const ClientKeys: { [key: string]: any } = { export class StateMigrationService extends BaseStateMigrationService { async migrate(): Promise { - let currentStateVersion = - (await this.storageService.get>("state"))?.globals?.stateVersion ?? StateVersion.One; + let currentStateVersion = await this.getCurrentStateVersion(); while (currentStateVersion < StateVersion.Latest) { switch (currentStateVersion) { case StateVersion.One: @@ -80,73 +77,73 @@ export class StateMigrationService extends BaseStateMigrationService { } protected async migrateStateFrom1To2(useSecureStorageForSecrets: boolean = true): Promise { + // Grabbing a couple of key settings before they get cleared by the base migration + const userId = await this.get(Keys.entityId); + const clientId = await this.get(ClientKeys.clientId); + const clientSecret = await this.get(ClientKeys.clientSecret); + await super.migrateStateFrom1To2(); - const state = await this.storageService.get>(Keys.state); - const userId = await this.storageService.get(Keys.entityId); - if (userId != null) { - state.accounts[userId] = new Account({ - directorySettings: { - directoryType: await this.storageService.get(Keys.directoryType), - organizationId: await this.storageService.get(Keys.organizationId), - lastUserSync: await this.storageService.get(Keys.lastUserSync), - lastGroupSync: await this.storageService.get(Keys.lastGroupSync), - lastSyncHash: await this.storageService.get(Keys.lastSyncHash), - syncingDir: await this.storageService.get(Keys.syncingDir), - sync: await this.storageService.get(Keys.syncConfig), - }, - profile: { - entityId: await this.storageService.get(Keys.entityId), - }, - directoryConfigurations: new DirectoryConfigurations(), - clientKeys: { - clientId: await this.storageService.get(ClientKeys.clientId), - clientSecret: await this.storageService.get(ClientKeys.clientSecret), - }, - }); - } - - for (const key in DirectoryType) { - if (await this.storageService.has(SecureStorageKeys.directoryConfigPrefix + key)) { - switch (+key) { - case DirectoryType.Ldap: - state.accounts[userId].directoryConfigurations.ldap = - await this.storageService.get( - SecureStorageKeys.directoryConfigPrefix + key - ); - break; - case DirectoryType.GSuite: - state.accounts[userId].directoryConfigurations.gsuite = - await this.storageService.get( - SecureStorageKeys.directoryConfigPrefix + key - ); - break; - case DirectoryType.AzureActiveDirectory: - state.accounts[userId].directoryConfigurations.azure = - await this.storageService.get( - SecureStorageKeys.directoryConfigPrefix + key - ); - break; - case DirectoryType.Okta: - state.accounts[userId].directoryConfigurations.okta = - await this.storageService.get( - SecureStorageKeys.directoryConfigPrefix + key - ); - break; - case DirectoryType.OneLogin: - state.accounts[userId].directoryConfigurations.oneLogin = - await this.storageService.get( - SecureStorageKeys.directoryConfigPrefix + key - ); - break; + // Setup reusable method for clearing keys since we will want to do that regardless of if there is an active authenticated session + const clearDirectoryConnectorV1Keys = async () => { + for (const key in Keys) { + if (key == null) { + continue; + } + for (const directoryType in DirectoryType) { + if (directoryType == null) { + continue; + } + await this.set(SecureStorageKeys.directoryConfigPrefix + directoryType, null); } - await this.storageService.remove(SecureStorageKeys.directoryConfigPrefix + key); } + }; + + // Initilize typed objects from key/value pairs in storage to either be saved temporarily until an account is authed or applied to the active account + const getDirectoryConfig = async (type: DirectoryType) => + await this.get(SecureStorageKeys.directoryConfigPrefix + type); + const directoryConfigs: DirectoryConfigurations = { + ldap: await getDirectoryConfig(DirectoryType.Ldap), + gsuite: await getDirectoryConfig(DirectoryType.GSuite), + azure: await getDirectoryConfig(DirectoryType.AzureActiveDirectory), + okta: await getDirectoryConfig(DirectoryType.Okta), + oneLogin: await getDirectoryConfig(DirectoryType.OneLogin), + }; + + const directorySettings: DirectorySettings = { + directoryType: await this.get(Keys.directoryType), + organizationId: await this.get(Keys.organizationId), + lastUserSync: await this.get(Keys.lastUserSync), + lastGroupSync: await this.get(Keys.lastGroupSync), + lastSyncHash: await this.get(Keys.lastSyncHash), + syncingDir: await this.get(Keys.syncingDir), + sync: await this.get(Keys.syncConfig), + }; + + // (userId == null) = no authed account, stored data temporarily to be applied and cleared on next auth + // (userId != null) = authed account known, applied stored data to it and do not save temp data + if (userId == null) { + await this.set(Keys.tempDirectoryConfigs, directoryConfigs); + await this.set(Keys.tempDirectorySettings, directorySettings); + await clearDirectoryConnectorV1Keys(); + return; } - state.globals.environmentUrls = await this.storageService.get("environmentUrls"); + const account = await this.get(userId); + account.directoryConfigurations = directoryConfigs; + account.directorySettings = directorySettings; + account.profile = { + userId: userId, + entityId: userId, + apiKeyClientId: clientId, + }; + account.clientKeys = { + clientId: clientId, + clientSecret: clientSecret, + }; - await this.storageService.save("state", state); + await this.set(userId, account); + await clearDirectoryConnectorV1Keys(); if (useSecureStorageForSecrets) { for (const key in SecureStorageKeys) {