From 2e2849b4def0534f3f72b7a84c3183ab0b1589f2 Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Fri, 29 Apr 2022 21:33:38 +1000 Subject: [PATCH] [PS-74] Fix user authentication state checks (#721) * Create authService.authStatus, refactor isLocked checks * Rename authStatus -> getAuthStatus --- angular/src/services/auth-guard.service.ts | 17 +++++----- angular/src/services/jslib-services.module.ts | 3 +- angular/src/services/lock-guard.service.ts | 19 +++++------ angular/src/services/unauth-guard.service.ts | 28 ++++++++-------- common/src/abstractions/auth.service.ts | 2 ++ .../src/abstractions/vaultTimeout.service.ts | 1 - common/src/enums/authenticationStatus.ts | 7 ++-- common/src/services/auth.service.ts | 27 ++++++++++++++++ common/src/services/notifications.service.ts | 14 ++++---- common/src/services/vaultTimeout.service.ts | 32 +++++-------------- 10 files changed, 77 insertions(+), 73 deletions(-) diff --git a/angular/src/services/auth-guard.service.ts b/angular/src/services/auth-guard.service.ts index fafcbfdfda..b9f8ebfdcf 100644 --- a/angular/src/services/auth-guard.service.ts +++ b/angular/src/services/auth-guard.service.ts @@ -1,30 +1,29 @@ import { Injectable } from "@angular/core"; import { ActivatedRouteSnapshot, CanActivate, Router, RouterStateSnapshot } from "@angular/router"; +import { AuthService } from "jslib-common/abstractions/auth.service"; import { KeyConnectorService } from "jslib-common/abstractions/keyConnector.service"; import { MessagingService } from "jslib-common/abstractions/messaging.service"; -import { StateService } from "jslib-common/abstractions/state.service"; -import { VaultTimeoutService } from "jslib-common/abstractions/vaultTimeout.service"; +import { AuthenticationStatus } from "jslib-common/enums/authenticationStatus"; @Injectable() export class AuthGuardService implements CanActivate { constructor( - private vaultTimeoutService: VaultTimeoutService, + private authService: AuthService, private router: Router, private messagingService: MessagingService, - private keyConnectorService: KeyConnectorService, - private stateService: StateService + private keyConnectorService: KeyConnectorService ) {} async canActivate(route: ActivatedRouteSnapshot, routerState: RouterStateSnapshot) { - const isAuthed = await this.stateService.getIsAuthenticated(); - if (!isAuthed) { + const authStatus = await this.authService.getAuthStatus(); + + if (authStatus === AuthenticationStatus.LoggedOut) { this.messagingService.send("authBlocked", { url: routerState.url }); return false; } - const locked = await this.vaultTimeoutService.isLocked(); - if (locked) { + if (authStatus === AuthenticationStatus.Locked) { if (routerState != null) { this.messagingService.send("lockedUrl", { url: routerState.url }); } diff --git a/angular/src/services/jslib-services.module.ts b/angular/src/services/jslib-services.module.ts index e0bc046830..3cd97d6336 100644 --- a/angular/src/services/jslib-services.module.ts +++ b/angular/src/services/jslib-services.module.ts @@ -303,6 +303,7 @@ export const SYSTEM_LANGUAGE = new InjectionToken("SYSTEM_LANGUAGE"); PolicyServiceAbstraction, KeyConnectorServiceAbstraction, StateServiceAbstraction, + AuthServiceAbstraction, LOCKED_CALLBACK, LOGOUT_CALLBACK, ], @@ -346,11 +347,11 @@ export const SYSTEM_LANGUAGE = new InjectionToken("SYSTEM_LANGUAGE"); SyncServiceAbstraction, AppIdServiceAbstraction, ApiServiceAbstraction, - VaultTimeoutServiceAbstraction, EnvironmentServiceAbstraction, LOGOUT_CALLBACK, LogService, StateServiceAbstraction, + AuthServiceAbstraction, ], }, { diff --git a/angular/src/services/lock-guard.service.ts b/angular/src/services/lock-guard.service.ts index 992c6e5d67..2a44db7203 100644 --- a/angular/src/services/lock-guard.service.ts +++ b/angular/src/services/lock-guard.service.ts @@ -1,27 +1,24 @@ import { Injectable } from "@angular/core"; import { CanActivate, Router } from "@angular/router"; -import { StateService } from "jslib-common/abstractions/state.service"; -import { VaultTimeoutService } from "jslib-common/abstractions/vaultTimeout.service"; +import { AuthService } from "jslib-common/abstractions/auth.service"; +import { AuthenticationStatus } from "jslib-common/enums/authenticationStatus"; @Injectable() export class LockGuardService implements CanActivate { protected homepage = "vault"; protected loginpage = "login"; - constructor( - private vaultTimeoutService: VaultTimeoutService, - private router: Router, - private stateService: StateService - ) {} + constructor(private authService: AuthService, private router: Router) {} async canActivate() { - if (await this.vaultTimeoutService.isLocked()) { + const authStatus = await this.authService.getAuthStatus(); + + if (authStatus === AuthenticationStatus.Locked) { return true; } - const redirectUrl = (await this.stateService.getIsAuthenticated()) - ? [this.homepage] - : [this.loginpage]; + const redirectUrl = + authStatus === AuthenticationStatus.LoggedOut ? [this.loginpage] : [this.homepage]; this.router.navigate(redirectUrl); return false; diff --git a/angular/src/services/unauth-guard.service.ts b/angular/src/services/unauth-guard.service.ts index 89c08b195e..9c309d9afd 100644 --- a/angular/src/services/unauth-guard.service.ts +++ b/angular/src/services/unauth-guard.service.ts @@ -1,27 +1,25 @@ import { Injectable } from "@angular/core"; import { CanActivate, Router } from "@angular/router"; -import { StateService } from "jslib-common/abstractions/state.service"; -import { VaultTimeoutService } from "jslib-common/abstractions/vaultTimeout.service"; +import { AuthService } from "jslib-common/abstractions/auth.service"; +import { AuthenticationStatus } from "jslib-common/enums/authenticationStatus"; @Injectable() export class UnauthGuardService implements CanActivate { protected homepage = "vault"; - constructor( - private vaultTimeoutService: VaultTimeoutService, - private router: Router, - private stateService: StateService - ) {} + constructor(private authService: AuthService, private router: Router) {} async canActivate() { - const isAuthed = await this.stateService.getIsAuthenticated(); - if (isAuthed) { - const locked = await this.vaultTimeoutService.isLocked(); - if (locked) { - return this.router.createUrlTree(["lock"]); - } - return this.router.createUrlTree([this.homepage]); + const authStatus = await this.authService.getAuthStatus(); + + if (authStatus === AuthenticationStatus.LoggedOut) { + return true; } - return true; + + if (authStatus === AuthenticationStatus.Locked) { + return this.router.createUrlTree(["lock"]); + } + + return this.router.createUrlTree([this.homepage]); } } diff --git a/common/src/abstractions/auth.service.ts b/common/src/abstractions/auth.service.ts index bebac7300b..4947f21708 100644 --- a/common/src/abstractions/auth.service.ts +++ b/common/src/abstractions/auth.service.ts @@ -1,3 +1,4 @@ +import { AuthenticationStatus } from "../enums/authenticationStatus"; import { AuthResult } from "../models/domain/authResult"; import { ApiLogInCredentials, @@ -22,4 +23,5 @@ export abstract class AuthService { authingWithApiKey: () => boolean; authingWithSso: () => boolean; authingWithPassword: () => boolean; + getAuthStatus: (userId?: string) => Promise; } diff --git a/common/src/abstractions/vaultTimeout.service.ts b/common/src/abstractions/vaultTimeout.service.ts index 2c2b2521d6..317fbca86d 100644 --- a/common/src/abstractions/vaultTimeout.service.ts +++ b/common/src/abstractions/vaultTimeout.service.ts @@ -1,5 +1,4 @@ export abstract class VaultTimeoutService { - isLocked: (userId?: string) => Promise; checkVaultTimeout: () => Promise; lock: (allowSoftLock?: boolean, userId?: string) => Promise; logOut: (userId?: string) => Promise; diff --git a/common/src/enums/authenticationStatus.ts b/common/src/enums/authenticationStatus.ts index 8e1db548ce..17b4f1f21e 100644 --- a/common/src/enums/authenticationStatus.ts +++ b/common/src/enums/authenticationStatus.ts @@ -1,6 +1,5 @@ export enum AuthenticationStatus { - Locked = "locked", - Unlocked = "unlocked", - LoggedOut = "loggedOut", - Active = "active", + LoggedOut = 0, + Locked = 1, + Unlocked = 2, } diff --git a/common/src/services/auth.service.ts b/common/src/services/auth.service.ts index fe4542b8e2..6f77bca20b 100644 --- a/common/src/services/auth.service.ts +++ b/common/src/services/auth.service.ts @@ -11,8 +11,10 @@ import { PlatformUtilsService } from "../abstractions/platformUtils.service"; import { StateService } from "../abstractions/state.service"; import { TokenService } from "../abstractions/token.service"; import { TwoFactorService } from "../abstractions/twoFactor.service"; +import { AuthenticationStatus } from "../enums/authenticationStatus"; import { AuthenticationType } from "../enums/authenticationType"; import { KdfType } from "../enums/kdfType"; +import { KeySuffixOptions } from "../enums/keySuffixOptions"; import { ApiLogInStrategy } from "../misc/logInStrategies/apiLogin.strategy"; import { PasswordLogInStrategy } from "../misc/logInStrategies/passwordLogin.strategy"; import { SsoLogInStrategy } from "../misc/logInStrategies/ssoLogin.strategy"; @@ -157,6 +159,31 @@ export class AuthService implements AuthServiceAbstraction { return this.logInStrategy instanceof PasswordLogInStrategy; } + async getAuthStatus(userId?: string): Promise { + const isAuthenticated = await this.stateService.getIsAuthenticated({ userId: userId }); + if (!isAuthenticated) { + return AuthenticationStatus.LoggedOut; + } + + // Keys aren't stored for a device that is locked or logged out + // Make sure we're logged in before checking this, otherwise we could mix up those states + const neverLock = + (await this.cryptoService.hasKeyStored(KeySuffixOptions.Auto, userId)) && + !(await this.stateService.getEverBeenUnlocked({ userId: userId })); + if (neverLock) { + // TODO: This also _sets_ the key so when we check memory in the next line it finds a key. + // We should refactor here. + await this.cryptoService.getKey(KeySuffixOptions.Auto, userId); + } + + const hasKeyInMemory = await this.cryptoService.hasKeyInMemory(userId); + if (!hasKeyInMemory) { + return AuthenticationStatus.Locked; + } + + return AuthenticationStatus.Unlocked; + } + async makePreloginKey(masterPassword: string, email: string): Promise { email = email.trim().toLowerCase(); let kdf: KdfType = null; diff --git a/common/src/services/notifications.service.ts b/common/src/services/notifications.service.ts index f3d8411587..39e2df541f 100644 --- a/common/src/services/notifications.service.ts +++ b/common/src/services/notifications.service.ts @@ -3,12 +3,13 @@ import * as signalRMsgPack from "@microsoft/signalr-protocol-msgpack"; import { ApiService } from "../abstractions/api.service"; import { AppIdService } from "../abstractions/appId.service"; +import { AuthService } from "../abstractions/auth.service"; import { EnvironmentService } from "../abstractions/environment.service"; import { LogService } from "../abstractions/log.service"; import { NotificationsService as NotificationsServiceAbstraction } from "../abstractions/notifications.service"; import { StateService } from "../abstractions/state.service"; import { SyncService } from "../abstractions/sync.service"; -import { VaultTimeoutService } from "../abstractions/vaultTimeout.service"; +import { AuthenticationStatus } from "../enums/authenticationStatus"; import { NotificationType } from "../enums/notificationType"; import { NotificationResponse, @@ -29,11 +30,11 @@ export class NotificationsService implements NotificationsServiceAbstraction { private syncService: SyncService, private appIdService: AppIdService, private apiService: ApiService, - private vaultTimeoutService: VaultTimeoutService, private environmentService: EnvironmentService, private logoutCallback: (expired: boolean) => Promise, private logService: LogService, - private stateService: StateService + private stateService: StateService, + private authService: AuthService ) { this.environmentService.urls.subscribe(() => { if (!this.inited) { @@ -216,11 +217,8 @@ export class NotificationsService implements NotificationsServiceAbstraction { } private async isAuthedAndUnlocked() { - if (await this.stateService.getIsAuthenticated()) { - const locked = await this.vaultTimeoutService.isLocked(); - return !locked; - } - return false; + const authStatus = await this.authService.getAuthStatus(); + return authStatus >= AuthenticationStatus.Unlocked; } private random(min: number, max: number) { diff --git a/common/src/services/vaultTimeout.service.ts b/common/src/services/vaultTimeout.service.ts index 0247c2264c..935c55d8e6 100644 --- a/common/src/services/vaultTimeout.service.ts +++ b/common/src/services/vaultTimeout.service.ts @@ -1,3 +1,4 @@ +import { AuthService } from "../abstractions/auth.service"; import { CipherService } from "../abstractions/cipher.service"; import { CollectionService } from "../abstractions/collection.service"; import { CryptoService } from "../abstractions/crypto.service"; @@ -10,7 +11,7 @@ import { SearchService } from "../abstractions/search.service"; import { StateService } from "../abstractions/state.service"; import { TokenService } from "../abstractions/token.service"; import { VaultTimeoutService as VaultTimeoutServiceAbstraction } from "../abstractions/vaultTimeout.service"; -import { KeySuffixOptions } from "../enums/keySuffixOptions"; +import { AuthenticationStatus } from "../enums/authenticationStatus"; import { PolicyType } from "../enums/policyType"; export class VaultTimeoutService implements VaultTimeoutServiceAbstraction { @@ -28,6 +29,7 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction { private policyService: PolicyService, private keyConnectorService: KeyConnectorService, private stateService: StateService, + private authService: AuthService, private lockedCallback: (userId?: string) => Promise = null, private loggedOutCallback: (expired: boolean, userId?: string) => Promise = null ) {} @@ -48,20 +50,6 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction { setInterval(() => this.checkVaultTimeout(), 10 * 1000); // check every 10 seconds } - // Keys aren't stored for a device that is locked or logged out. - async isLocked(userId?: string): Promise { - const neverLock = - (await this.cryptoService.hasKeyStored(KeySuffixOptions.Auto, userId)) && - !(await this.stateService.getEverBeenUnlocked({ userId: userId })); - if (neverLock) { - // TODO: This also _sets_ the key so when we check memory in the next line it finds a key. - // We should refactor here. - await this.cryptoService.getKey(KeySuffixOptions.Auto, userId); - } - - return !(await this.cryptoService.hasKeyInMemory(userId)); - } - async checkVaultTimeout(): Promise { if (await this.platformUtilsService.isViewOpen()) { return; @@ -187,16 +175,12 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction { await this.stateService.setProtectedPin(null, { userId: userId }); } - private async isLoggedOut(userId?: string): Promise { - return !(await this.stateService.getIsAuthenticated({ userId: userId })); - } - private async shouldLock(userId: string): Promise { - if (await this.isLoggedOut(userId)) { - return false; - } - - if (await this.isLocked(userId)) { + const authStatus = await this.authService.getAuthStatus(userId); + if ( + authStatus === AuthenticationStatus.Locked || + authStatus === AuthenticationStatus.LoggedOut + ) { return false; }