From 386903f5a9ef42fed709813d40e550ba91c0ee40 Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Tue, 16 Nov 2021 07:53:57 +1000 Subject: [PATCH] [Key Connector] QA fixes for CLI and Desktop (#544) * Make UserVerificationService compatible with CLI * Refactor error handling * Fix i18n key name * Add apiUseKeyConnector flag to TokenResponse * Always require keyConnectorUrl to be passed in * Throw errors in userVerificationService * Use requestOTP in UserVerificationService * Remove unused deps * Fix linting --- .../verify-master-password.component.ts | 6 ++- .../abstractions/userVerification.service.ts | 1 + .../models/response/identityTokenResponse.ts | 2 + common/src/services/auth.service.ts | 5 ++- common/src/services/keyConnector.service.ts | 14 +------ .../src/services/userVerification.service.ts | 39 +++++++++---------- 6 files changed, 31 insertions(+), 36 deletions(-) diff --git a/angular/src/components/verify-master-password.component.ts b/angular/src/components/verify-master-password.component.ts index 01c511c916..658f7f59ae 100644 --- a/angular/src/components/verify-master-password.component.ts +++ b/angular/src/components/verify-master-password.component.ts @@ -10,6 +10,7 @@ import { import { ApiService } from 'jslib-common/abstractions/api.service'; import { KeyConnectorService } from 'jslib-common/abstractions/keyConnector.service'; +import { UserVerificationService } from 'jslib-common/abstractions/userVerification.service'; import { VerificationType } from 'jslib-common/enums/verificationType'; @@ -34,7 +35,8 @@ export class VerifyMasterPasswordComponent implements ControlValueAccessor, OnIn private onChange: (value: Verification) => void; - constructor(private keyConnectorService: KeyConnectorService, private apiService: ApiService) { } + constructor(private keyConnectorService: KeyConnectorService, + private userVerificationService: UserVerificationService) { } async ngOnInit() { this.usesKeyConnector = await this.keyConnectorService.getUsesKeyConnector(); @@ -54,7 +56,7 @@ export class VerifyMasterPasswordComponent implements ControlValueAccessor, OnIn async requestOTP() { if (this.usesKeyConnector) { this.disableRequestOTP = true; - await this.apiService.postAccountRequestOTP(); + await this.userVerificationService.requestOTP(); } } diff --git a/common/src/abstractions/userVerification.service.ts b/common/src/abstractions/userVerification.service.ts index 05c7a0903f..7dcf1d10bf 100644 --- a/common/src/abstractions/userVerification.service.ts +++ b/common/src/abstractions/userVerification.service.ts @@ -6,4 +6,5 @@ export abstract class UserVerificationService { buildRequest: (verification: Verification, requestClass?: new () => T, alreadyHashed?: boolean) => Promise; verifyUser: (verification: Verification) => Promise; + requestOTP: () => Promise; } diff --git a/common/src/models/response/identityTokenResponse.ts b/common/src/models/response/identityTokenResponse.ts index 7b128f44c4..c2283956fb 100644 --- a/common/src/models/response/identityTokenResponse.ts +++ b/common/src/models/response/identityTokenResponse.ts @@ -15,6 +15,7 @@ export class IdentityTokenResponse extends BaseResponse { kdf: KdfType; kdfIterations: number; forcePasswordReset: boolean; + apiUseKeyConnector: boolean; keyConnectorUrl: string; constructor(response: any) { @@ -31,6 +32,7 @@ export class IdentityTokenResponse extends BaseResponse { this.kdf = this.getResponseProperty('Kdf'); this.kdfIterations = this.getResponseProperty('KdfIterations'); this.forcePasswordReset = this.getResponseProperty('ForcePasswordReset'); + this.apiUseKeyConnector = this.getResponseProperty('ApiUseKeyConnector'); this.keyConnectorUrl = this.getResponseProperty('KeyConnectorUrl'); } } diff --git a/common/src/services/auth.service.ts b/common/src/services/auth.service.ts index 5c5ca225f0..e4f670d7b7 100644 --- a/common/src/services/auth.service.ts +++ b/common/src/services/auth.service.ts @@ -370,8 +370,9 @@ export class AuthService implements AuthServiceAbstraction { if (tokenResponse.keyConnectorUrl != null) { await this.keyConnectorService.getAndSetKey(tokenResponse.keyConnectorUrl); - } else if (this.environmentService.getKeyConnectorUrl() != null) { - await this.keyConnectorService.getAndSetKey(); + } else if (tokenResponse.apiUseKeyConnector) { + const keyConnectorUrl = this.environmentService.getKeyConnectorUrl(); + await this.keyConnectorService.getAndSetKey(keyConnectorUrl); } await this.cryptoService.setEncKey(tokenResponse.key); diff --git a/common/src/services/keyConnector.service.ts b/common/src/services/keyConnector.service.ts index 68aac9a21e..d0b8e04007 100644 --- a/common/src/services/keyConnector.service.ts +++ b/common/src/services/keyConnector.service.ts @@ -1,6 +1,5 @@ import { ApiService } from '../abstractions/api.service'; import { CryptoService } from '../abstractions/crypto.service'; -import { EnvironmentService } from '../abstractions/environment.service'; import { KeyConnectorService as KeyConnectorServiceAbstraction } from '../abstractions/keyConnector.service'; import { LogService } from '../abstractions/log.service'; import { StorageService } from '../abstractions/storage.service'; @@ -25,8 +24,7 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { constructor(private storageService: StorageService, private userService: UserService, private cryptoService: CryptoService, private apiService: ApiService, - private environmentService: EnvironmentService, private tokenService: TokenService, - private logService: LogService) { } + private tokenService: TokenService, private logService: LogService) { } setUsesKeyConnector(usesKeyConnector: boolean) { this.usesKeyConnector = usesKeyConnector; @@ -59,15 +57,7 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { await this.apiService.postConvertToKeyConnector(); } - async getAndSetKey(url?: string) { - if (url == null) { - url = this.environmentService.getKeyConnectorUrl(); - } - - if (url == null) { - throw new Error('No Key Connector URL found.'); - } - + async getAndSetKey(url: string) { try { const userKeyResponse = await this.apiService.getUserKeyFromKeyConnector(url); const keyArr = Utils.fromB64ToArray(userKeyResponse.key); diff --git a/common/src/services/userVerification.service.ts b/common/src/services/userVerification.service.ts index 7910f44a16..2b94ffaec3 100644 --- a/common/src/services/userVerification.service.ts +++ b/common/src/services/userVerification.service.ts @@ -1,12 +1,8 @@ -import { Injectable } from '@angular/core'; - import { UserVerificationService as UserVerificationServiceAbstraction } from '../abstractions/userVerification.service'; import { ApiService } from '../abstractions/api.service'; import { CryptoService } from '../abstractions/crypto.service'; import { I18nService } from '../abstractions/i18n.service'; -import { LogService } from '../abstractions/log.service'; -import { PlatformUtilsService } from '../abstractions/platformUtils.service'; import { VerificationType } from '../enums/verificationType'; @@ -15,17 +11,13 @@ import { SecretVerificationRequest } from '../models/request/secretVerificationR import { Verification } from '../types/verification'; -@Injectable() export class UserVerificationService implements UserVerificationServiceAbstraction { constructor(private cryptoService: CryptoService, private i18nService: I18nService, - private platformUtilsService: PlatformUtilsService, private apiService: ApiService, - private logService: LogService) { } + private apiService: ApiService) { } async buildRequest(verification: Verification, requestClass?: new () => T, alreadyHashed?: boolean) { - if (verification?.secret == null || verification.secret === '') { - throw new Error('No secret provided for verification.'); - } + this.validateInput(verification); const request = requestClass != null ? new requestClass() @@ -43,28 +35,35 @@ export class UserVerificationService implements UserVerificationServiceAbstracti } async verifyUser(verification: Verification): Promise { - if (verification?.secret == null || verification.secret === '') { - throw new Error('No secret provided for verification.'); - } + this.validateInput(verification); if (verification.type === VerificationType.OTP) { const request = new VerifyOTPRequest(verification.secret); try { await this.apiService.postAccountVerifyOTP(request); } catch (e) { - this.logService.error(e); - this.platformUtilsService.showToast('error', this.i18nService.t('errorOccurred'), - this.i18nService.t('invalidVerificationCode')); - return false; + throw new Error(this.i18nService.t('invalidVerificationCode')); } } else { const passwordValid = await this.cryptoService.compareAndUpdateKeyHash(verification.secret, null); if (!passwordValid) { - this.platformUtilsService.showToast('error', this.i18nService.t('errorOccurred'), - this.i18nService.t('invalidMasterPassword')); - return false; + throw new Error(this.i18nService.t('invalidMasterPassword')); } } return true; } + + async requestOTP() { + await this.apiService.postAccountRequestOTP(); + } + + private validateInput(verification: Verification) { + if (verification?.secret == null || verification.secret === '') { + if (verification.type === VerificationType.OTP) { + throw new Error(this.i18nService.t('verificationCodeRequired')); + } else { + throw new Error(this.i18nService.t('masterPassRequired')); + } + } + } }