From 6bd5d579af3f26046e83931bc3a51cbe473da309 Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Mon, 2 May 2022 07:57:40 +1000 Subject: [PATCH] [PS-74] Fix user authentication state checks (#2481) * Replace vaultTimeoutService.isLocked --- jslib | 2 +- src/background/commands.background.ts | 7 +- src/background/contextMenus.background.ts | 7 +- src/background/main.background.ts | 84 ++++++++++---------- src/background/nativeMessaging.background.ts | 7 +- src/background/notification.background.ts | 14 ++-- src/background/webRequest.background.ts | 7 +- src/popup/accounts/lock.component.ts | 16 ++-- src/popup/accounts/sso.component.ts | 6 +- src/popup/accounts/two-factor.component.ts | 7 +- src/popup/generator/generator.component.ts | 3 + 11 files changed, 93 insertions(+), 67 deletions(-) diff --git a/jslib b/jslib index e40e7de808..2e2849b4de 160000 --- a/jslib +++ b/jslib @@ -1 +1 @@ -Subproject commit e40e7de8083ecfe220ef7f94f7447d560f822272 +Subproject commit 2e2849b4def0534f3f72b7a84c3183ab0b1589f2 diff --git a/src/background/commands.background.ts b/src/background/commands.background.ts index 580f691665..97b367e1d4 100644 --- a/src/background/commands.background.ts +++ b/src/background/commands.background.ts @@ -1,6 +1,8 @@ +import { AuthService } from "jslib-common/abstractions/auth.service"; import { PasswordGenerationService } from "jslib-common/abstractions/passwordGeneration.service"; import { PlatformUtilsService } from "jslib-common/abstractions/platformUtils.service"; import { VaultTimeoutService } from "jslib-common/abstractions/vaultTimeout.service"; +import { AuthenticationStatus } from "jslib-common/enums/authenticationStatus"; import { BrowserApi } from "../browser/browserApi"; @@ -15,7 +17,8 @@ export default class CommandsBackground { private main: MainBackground, private passwordGenerationService: PasswordGenerationService, private platformUtilsService: PlatformUtilsService, - private vaultTimeoutService: VaultTimeoutService + private vaultTimeoutService: VaultTimeoutService, + private authService: AuthService ) { this.isSafari = this.platformUtilsService.isSafari(); this.isVivaldi = this.platformUtilsService.isVivaldi(); @@ -80,7 +83,7 @@ export default class CommandsBackground { return; } - if (await this.vaultTimeoutService.isLocked()) { + if ((await this.authService.getAuthStatus()) < AuthenticationStatus.Unlocked) { const retryMessage: LockedVaultPendingNotificationsItem = { commandToRetry: { msg: { command: "autofill_login" }, diff --git a/src/background/contextMenus.background.ts b/src/background/contextMenus.background.ts index 90b052fc11..29bc5254d1 100644 --- a/src/background/contextMenus.background.ts +++ b/src/background/contextMenus.background.ts @@ -1,9 +1,10 @@ +import { AuthService } from "jslib-common/abstractions/auth.service"; import { CipherService } from "jslib-common/abstractions/cipher.service"; import { EventService } from "jslib-common/abstractions/event.service"; import { PasswordGenerationService } from "jslib-common/abstractions/passwordGeneration.service"; import { PlatformUtilsService } from "jslib-common/abstractions/platformUtils.service"; import { TotpService } from "jslib-common/abstractions/totp.service"; -import { VaultTimeoutService } from "jslib-common/abstractions/vaultTimeout.service"; +import { AuthenticationStatus } from "jslib-common/enums/authenticationStatus"; import { CipherRepromptType } from "jslib-common/enums/cipherRepromptType"; import { EventType } from "jslib-common/enums/eventType"; import { CipherView } from "jslib-common/models/view/cipherView"; @@ -22,7 +23,7 @@ export default class ContextMenusBackground { private cipherService: CipherService, private passwordGenerationService: PasswordGenerationService, private platformUtilsService: PlatformUtilsService, - private vaultTimeoutService: VaultTimeoutService, + private authService: AuthService, private eventService: EventService, private totpService: TotpService ) { @@ -82,7 +83,7 @@ export default class ContextMenusBackground { private async cipherAction(tab: chrome.tabs.Tab, info: chrome.contextMenus.OnClickData) { const id = info.menuItemId.split("_")[1]; - if (await this.vaultTimeoutService.isLocked()) { + if ((await this.authService.getAuthStatus()) < AuthenticationStatus.Unlocked) { const retryMessage: LockedVaultPendingNotificationsItem = { commandToRetry: { msg: { command: this.noopCommandSuffix, data: info }, diff --git a/src/background/main.background.ts b/src/background/main.background.ts index 4910a53a71..8bcc195db0 100644 --- a/src/background/main.background.ts +++ b/src/background/main.background.ts @@ -33,6 +33,7 @@ import { TwoFactorService as TwoFactorServiceAbstraction } from "jslib-common/ab import { UserVerificationService as UserVerificationServiceAbstraction } from "jslib-common/abstractions/userVerification.service"; import { UsernameGenerationService as UsernameGenerationServiceAbstraction } from "jslib-common/abstractions/usernameGeneration.service"; import { VaultTimeoutService as VaultTimeoutServiceAbstraction } from "jslib-common/abstractions/vaultTimeout.service"; +import { AuthenticationStatus } from "jslib-common/enums/authenticationStatus"; import { CipherRepromptType } from "jslib-common/enums/cipherRepromptType"; import { CipherType } from "jslib-common/enums/cipherType"; import { StateFactory } from "jslib-common/factories/stateFactory"; @@ -267,6 +268,32 @@ export default class MainBackground { this.cryptoFunctionService ); + this.twoFactorService = new TwoFactorService(this.i18nService, this.platformUtilsService); + + // eslint-disable-next-line + const that = this; + const backgroundMessagingService = new (class extends MessagingServiceAbstraction { + // AuthService should send the messages to the background not popup. + send = (subscriber: string, arg: any = {}) => { + const message = Object.assign({}, { command: subscriber }, arg); + that.runtimeBackground.processMessage(message, that, null); + }; + })(); + this.authService = new AuthService( + this.cryptoService, + this.apiService, + this.tokenService, + this.appIdService, + this.platformUtilsService, + backgroundMessagingService, + this.logService, + this.keyConnectorService, + this.environmentService, + this.stateService, + this.twoFactorService, + this.i18nService + ); + const lockedCallback = async (userId?: string) => { if (this.notificationsService != null) { this.notificationsService.updateConnection(false); @@ -294,6 +321,7 @@ export default class MainBackground { this.policyService, this.keyConnectorService, this.stateService, + this.authService, lockedCallback, logoutCallback ); @@ -352,11 +380,11 @@ export default class MainBackground { this.syncService, this.appIdService, this.apiService, - this.vaultTimeoutService, this.environmentService, logoutCallback, this.logService, - this.stateService + this.stateService, + this.authService ); this.popupUtilsService = new PopupUtilsService(isPrivateMode); @@ -412,18 +440,19 @@ export default class MainBackground { this.platformUtilsService, this.stateService, this.logService, - this.vaultTimeoutService + this.authService ); this.commandsBackground = new CommandsBackground( this, this.passwordGenerationService, this.platformUtilsService, - this.vaultTimeoutService + this.vaultTimeoutService, + this.authService ); this.notificationBackground = new NotificationBackground( this.autofillService, this.cipherService, - this.vaultTimeoutService, + this.authService, this.policyService, this.folderService, this.stateService @@ -435,7 +464,7 @@ export default class MainBackground { this.cipherService, this.passwordGenerationService, this.platformUtilsService, - this.vaultTimeoutService, + this.authService, this.eventService, this.totpService ); @@ -447,37 +476,13 @@ export default class MainBackground { this.webRequestBackground = new WebRequestBackground( this.platformUtilsService, this.cipherService, - this.vaultTimeoutService + this.authService ); - this.twoFactorService = new TwoFactorService(this.i18nService, this.platformUtilsService); - - // eslint-disable-next-line - const that = this; - const backgroundMessagingService = new (class extends MessagingServiceAbstraction { - // AuthService should send the messages to the background not popup. - send = (subscriber: string, arg: any = {}) => { - const message = Object.assign({}, { command: subscriber }, arg); - that.runtimeBackground.processMessage(message, that, null); - }; - })(); - this.authService = new AuthService( - this.cryptoService, - this.apiService, - this.tokenService, - this.appIdService, - this.platformUtilsService, - backgroundMessagingService, - this.logService, - this.keyConnectorService, - this.environmentService, - this.stateService, - this.twoFactorService, - this.i18nService - ); this.usernameGenerationService = new UsernameGenerationService( this.cryptoService, - this.stateService + this.stateService, + this.apiService ); } @@ -532,13 +537,12 @@ export default class MainBackground { return; } - const isAuthenticated = await this.stateService.getIsAuthenticated(); - const locked = await this.vaultTimeoutService.isLocked(); + const authStatus = await this.authService.getAuthStatus(); let suffix = ""; - if (!isAuthenticated) { + if (authStatus === AuthenticationStatus.LoggedOut) { suffix = "_gray"; - } else if (locked) { + } else if (authStatus === AuthenticationStatus.Locked) { suffix = "_locked"; } @@ -756,8 +760,8 @@ export default class MainBackground { this.actionSetBadgeBackgroundColor(this.sidebarAction); this.menuOptionsLoaded = []; - const locked = await this.vaultTimeoutService.isLocked(); - if (!locked) { + const authStatus = await this.authService.getAuthStatus(); + if (authStatus === AuthenticationStatus.Unlocked) { try { const ciphers = await this.cipherService.getAllDecryptedForUrl(url); ciphers.sort((a, b) => this.cipherService.sortCiphersByLastUsedThenName(a, b)); @@ -1011,7 +1015,7 @@ export default class MainBackground { const accounts = this.stateService.accounts.getValue(); if (accounts != null) { for (const userId of Object.keys(accounts)) { - if (!(await this.vaultTimeoutService.isLocked(userId))) { + if ((await this.authService.getAuthStatus(userId)) === AuthenticationStatus.Unlocked) { return; } } diff --git a/src/background/nativeMessaging.background.ts b/src/background/nativeMessaging.background.ts index eb03d805c1..e48345be11 100644 --- a/src/background/nativeMessaging.background.ts +++ b/src/background/nativeMessaging.background.ts @@ -1,4 +1,5 @@ import { AppIdService } from "jslib-common/abstractions/appId.service"; +import { AuthService } from "jslib-common/abstractions/auth.service"; import { CryptoService } from "jslib-common/abstractions/crypto.service"; import { CryptoFunctionService } from "jslib-common/abstractions/cryptoFunction.service"; import { I18nService } from "jslib-common/abstractions/i18n.service"; @@ -6,7 +7,7 @@ import { LogService } from "jslib-common/abstractions/log.service"; import { MessagingService } from "jslib-common/abstractions/messaging.service"; import { PlatformUtilsService } from "jslib-common/abstractions/platformUtils.service"; import { StateService } from "jslib-common/abstractions/state.service"; -import { VaultTimeoutService } from "jslib-common/abstractions/vaultTimeout.service"; +import { AuthenticationStatus } from "jslib-common/enums/authenticationStatus"; import { Utils } from "jslib-common/misc/utils"; import { EncString } from "jslib-common/models/domain/encString"; import { SymmetricCryptoKey } from "jslib-common/models/domain/symmetricCryptoKey"; @@ -75,7 +76,7 @@ export class NativeMessagingBackground { private platformUtilsService: PlatformUtilsService, private stateService: StateService, private logService: LogService, - private vaultTimeoutService: VaultTimeoutService + private authService: AuthService ) { this.stateService.setBiometricFingerprintValidated(false); @@ -298,7 +299,7 @@ export class NativeMessagingBackground { } // Ignore unlock if already unlocked - if (!(await this.vaultTimeoutService.isLocked())) { + if ((await this.authService.getAuthStatus()) === AuthenticationStatus.Unlocked) { break; } diff --git a/src/background/notification.background.ts b/src/background/notification.background.ts index 22620fb80f..366eddc61a 100644 --- a/src/background/notification.background.ts +++ b/src/background/notification.background.ts @@ -1,7 +1,8 @@ +import { AuthService } from "jslib-common/abstractions/auth.service"; import { CipherService } from "jslib-common/abstractions/cipher.service"; import { FolderService } from "jslib-common/abstractions/folder.service"; import { PolicyService } from "jslib-common/abstractions/policy.service"; -import { VaultTimeoutService } from "jslib-common/abstractions/vaultTimeout.service"; +import { AuthenticationStatus } from "jslib-common/enums/authenticationStatus"; import { CipherType } from "jslib-common/enums/cipherType"; import { PolicyType } from "jslib-common/enums/policyType"; import { Utils } from "jslib-common/misc/utils"; @@ -26,7 +27,7 @@ export default class NotificationBackground { constructor( private autofillService: AutofillService, private cipherService: CipherService, - private vaultTimeoutService: VaultTimeoutService, + private authService: AuthService, private policyService: PolicyService, private folderService: FolderService, private stateService: StateService @@ -76,7 +77,7 @@ export default class NotificationBackground { break; case "bgAddSave": case "bgChangeSave": - if (await this.vaultTimeoutService.isLocked()) { + if ((await this.authService.getAuthStatus()) < AuthenticationStatus.Unlocked) { const retryMessage: LockedVaultPendingNotificationsItem = { commandToRetry: { msg: msg, @@ -187,7 +188,8 @@ export default class NotificationBackground { } private async addLogin(loginInfo: AddLoginRuntimeMessage, tab: chrome.tabs.Tab) { - if (!(await this.stateService.getIsAuthenticated())) { + const authStatus = await this.authService.getAuthStatus(); + if (authStatus === AuthenticationStatus.LoggedOut) { return; } @@ -202,7 +204,7 @@ export default class NotificationBackground { } const disabledAddLogin = await this.stateService.getDisableAddLoginNotification(); - if (await this.vaultTimeoutService.isLocked()) { + if (authStatus === AuthenticationStatus.Locked) { if (disabledAddLogin) { return; } @@ -270,7 +272,7 @@ export default class NotificationBackground { return; } - if (await this.vaultTimeoutService.isLocked()) { + if ((await this.authService.getAuthStatus()) < AuthenticationStatus.Unlocked) { this.pushChangePasswordToQueue(null, loginDomain, changeData.newPassword, tab, true); return; } diff --git a/src/background/webRequest.background.ts b/src/background/webRequest.background.ts index ac088b75b2..ad78ae7085 100644 --- a/src/background/webRequest.background.ts +++ b/src/background/webRequest.background.ts @@ -1,6 +1,7 @@ +import { AuthService } from "jslib-common/abstractions/auth.service"; import { CipherService } from "jslib-common/abstractions/cipher.service"; import { PlatformUtilsService } from "jslib-common/abstractions/platformUtils.service"; -import { VaultTimeoutService } from "jslib-common/abstractions/vaultTimeout.service"; +import { AuthenticationStatus } from "jslib-common/enums/authenticationStatus"; import { UriMatchType } from "jslib-common/enums/uriMatchType"; export default class WebRequestBackground { @@ -11,7 +12,7 @@ export default class WebRequestBackground { constructor( platformUtilsService: PlatformUtilsService, private cipherService: CipherService, - private vaultTimeoutService: VaultTimeoutService + private authService: AuthService ) { this.webRequest = (window as any).chrome.webRequest; this.isFirefox = platformUtilsService.isFirefox(); @@ -59,7 +60,7 @@ export default class WebRequestBackground { // eslint-disable-next-line private async resolveAuthCredentials(domain: string, success: Function, error: Function) { - if (await this.vaultTimeoutService.isLocked()) { + if ((await this.authService.getAuthStatus()) < AuthenticationStatus.Unlocked) { error(); return; } diff --git a/src/popup/accounts/lock.component.ts b/src/popup/accounts/lock.component.ts index 52f0f9b1d9..e1bee7fc78 100644 --- a/src/popup/accounts/lock.component.ts +++ b/src/popup/accounts/lock.component.ts @@ -3,6 +3,7 @@ import { Router } from "@angular/router"; import { LockComponent as BaseLockComponent } from "jslib-angular/components/lock.component"; import { ApiService } from "jslib-common/abstractions/api.service"; +import { AuthService } from "jslib-common/abstractions/auth.service"; import { CryptoService } from "jslib-common/abstractions/crypto.service"; import { EnvironmentService } from "jslib-common/abstractions/environment.service"; import { I18nService } from "jslib-common/abstractions/i18n.service"; @@ -12,6 +13,7 @@ import { MessagingService } from "jslib-common/abstractions/messaging.service"; import { PlatformUtilsService } from "jslib-common/abstractions/platformUtils.service"; import { StateService } from "jslib-common/abstractions/state.service"; import { VaultTimeoutService } from "jslib-common/abstractions/vaultTimeout.service"; +import { AuthenticationStatus } from "jslib-common/enums/authenticationStatus"; import { BiometricErrors, BiometricErrorTypes } from "../../models/biometricErrors"; @@ -37,7 +39,8 @@ export class LockComponent extends BaseLockComponent { apiService: ApiService, logService: LogService, keyConnectorService: KeyConnectorService, - ngZone: NgZone + ngZone: NgZone, + private authService: AuthService ) { super( router, @@ -64,10 +67,13 @@ export class LockComponent extends BaseLockComponent { window.setTimeout(async () => { document.getElementById(this.pinLock ? "pin" : "masterPassword").focus(); - if (this.biometricLock && !disableAutoBiometricsPrompt && this.isInitialLockScreen) { - if (await this.vaultTimeoutService.isLocked()) { - await this.unlockBiometric(); - } + if ( + this.biometricLock && + !disableAutoBiometricsPrompt && + this.isInitialLockScreen && + (await this.authService.getAuthStatus()) === AuthenticationStatus.Locked + ) { + await this.unlockBiometric(); } }, 100); } diff --git a/src/popup/accounts/sso.component.ts b/src/popup/accounts/sso.component.ts index 1faca54656..29af3db6ae 100644 --- a/src/popup/accounts/sso.component.ts +++ b/src/popup/accounts/sso.component.ts @@ -13,6 +13,7 @@ import { PlatformUtilsService } from "jslib-common/abstractions/platformUtils.se import { StateService } from "jslib-common/abstractions/state.service"; import { SyncService } from "jslib-common/abstractions/sync.service"; import { VaultTimeoutService } from "jslib-common/abstractions/vaultTimeout.service"; +import { AuthenticationStatus } from "jslib-common/enums/authenticationStatus"; import { BrowserApi } from "../../browser/browserApi"; @@ -57,8 +58,9 @@ export class SsoComponent extends BaseSsoComponent { super.onSuccessfulLogin = async () => { await syncService.fullSync(true); - if (await this.vaultTimeoutService.isLocked()) { - // If the vault is unlocked then this will clear keys from memory, which we don't want to do + + // If the vault is unlocked then this will clear keys from memory, which we don't want to do + if ((await this.authService.getAuthStatus()) !== AuthenticationStatus.Unlocked) { BrowserApi.reloadOpenWindows(); } diff --git a/src/popup/accounts/two-factor.component.ts b/src/popup/accounts/two-factor.component.ts index 37827ab266..d7e2d3771b 100644 --- a/src/popup/accounts/two-factor.component.ts +++ b/src/popup/accounts/two-factor.component.ts @@ -4,6 +4,7 @@ import { first } from "rxjs/operators"; import { TwoFactorComponent as BaseTwoFactorComponent } from "jslib-angular/components/two-factor.component"; import { ApiService } from "jslib-common/abstractions/api.service"; +import { AppIdService } from "jslib-common/abstractions/appId.service"; import { AuthService } from "jslib-common/abstractions/auth.service"; import { BroadcasterService } from "jslib-common/abstractions/broadcaster.service"; import { EnvironmentService } from "jslib-common/abstractions/environment.service"; @@ -42,7 +43,8 @@ export class TwoFactorComponent extends BaseTwoFactorComponent { route: ActivatedRoute, private messagingService: MessagingService, logService: LogService, - twoFactorService: TwoFactorService + twoFactorService: TwoFactorService, + appIdService: AppIdService ) { super( authService, @@ -55,7 +57,8 @@ export class TwoFactorComponent extends BaseTwoFactorComponent { stateService, route, logService, - twoFactorService + twoFactorService, + appIdService ); super.onSuccessfulLogin = () => { return syncService.fullSync(true); diff --git a/src/popup/generator/generator.component.ts b/src/popup/generator/generator.component.ts index d2a3e71eec..4e9f710c5b 100644 --- a/src/popup/generator/generator.component.ts +++ b/src/popup/generator/generator.component.ts @@ -4,6 +4,7 @@ import { ActivatedRoute } from "@angular/router"; import { GeneratorComponent as BaseGeneratorComponent } from "jslib-angular/components/generator.component"; import { I18nService } from "jslib-common/abstractions/i18n.service"; +import { LogService } from "jslib-common/abstractions/log.service"; import { PasswordGenerationService } from "jslib-common/abstractions/passwordGeneration.service"; import { PlatformUtilsService } from "jslib-common/abstractions/platformUtils.service"; import { StateService } from "jslib-common/abstractions/state.service"; @@ -25,6 +26,7 @@ export class GeneratorComponent extends BaseGeneratorComponent { i18nService: I18nService, stateService: StateService, route: ActivatedRoute, + logService: LogService, private location: Location ) { super( @@ -33,6 +35,7 @@ export class GeneratorComponent extends BaseGeneratorComponent { platformUtilsService, stateService, i18nService, + logService, route, window );