From 869fa29da6f586826437816d53164f160893447d Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Sat, 4 May 2024 02:04:56 -0400 Subject: [PATCH] Encourage The Use of UserId in CryptoService (#9033) --- .../auth/components/set-password.component.ts | 4 +-- .../auth-request-login.strategy.spec.ts | 4 +-- .../auth-request-login.strategy.ts | 8 ++++-- .../common/login-strategies/login.strategy.ts | 9 ++++--- .../password-login.strategy.spec.ts | 2 +- .../password-login.strategy.ts | 8 ++++-- .../login-strategies/sso-login.strategy.ts | 8 ++++-- .../user-api-login.strategy.spec.ts | 2 +- .../user-api-login.strategy.ts | 8 ++++-- .../webauthn-login.strategy.spec.ts | 2 +- .../webauthn-login.strategy.ts | 8 ++++-- .../auth/services/key-connector.service.ts | 2 +- .../src/models/response/profile.response.ts | 3 ++- .../platform/abstractions/crypto.service.ts | 27 ++++++++++++++++--- .../src/platform/services/crypto.service.ts | 26 +++++++++--------- .../src/vault/services/sync/sync.service.ts | 20 +++++++------- 16 files changed, 92 insertions(+), 49 deletions(-) diff --git a/libs/angular/src/auth/components/set-password.component.ts b/libs/angular/src/auth/components/set-password.component.ts index 00a36434b0..7ddc76d6c1 100644 --- a/libs/angular/src/auth/components/set-password.component.ts +++ b/libs/angular/src/auth/components/set-password.component.ts @@ -244,7 +244,7 @@ export class SetPasswordComponent extends BaseChangePasswordComponent { await this.userDecryptionOptionsService.setUserDecryptionOptions(userDecryptionOpts); await this.kdfConfigService.setKdfConfig(this.userId, this.kdfConfig); await this.masterPasswordService.setMasterKey(masterKey, this.userId); - await this.cryptoService.setUserKey(userKey[0]); + await this.cryptoService.setUserKey(userKey[0], this.userId); // Set private key only for new JIT provisioned users in MP encryption orgs // Existing TDE users will have private key set on sync or on login @@ -253,7 +253,7 @@ export class SetPasswordComponent extends BaseChangePasswordComponent { this.forceSetPasswordReason != ForceSetPasswordReason.TdeUserWithoutPasswordHasPasswordResetPermission ) { - await this.cryptoService.setPrivateKey(keyPair[1].encryptedString); + await this.cryptoService.setPrivateKey(keyPair[1].encryptedString, this.userId); } const localMasterKeyHash = await this.cryptoService.hashMasterKey( diff --git a/libs/auth/src/common/login-strategies/auth-request-login.strategy.spec.ts b/libs/auth/src/common/login-strategies/auth-request-login.strategy.spec.ts index 0efb9569eb..cde64f0477 100644 --- a/libs/auth/src/common/login-strategies/auth-request-login.strategy.spec.ts +++ b/libs/auth/src/common/login-strategies/auth-request-login.strategy.spec.ts @@ -140,7 +140,7 @@ describe("AuthRequestLoginStrategy", () => { expect(cryptoService.setMasterKeyEncryptedUserKey).toHaveBeenCalledWith(tokenResponse.key); expect(cryptoService.setUserKey).toHaveBeenCalledWith(userKey); expect(deviceTrustService.trustDeviceIfRequired).toHaveBeenCalled(); - expect(cryptoService.setPrivateKey).toHaveBeenCalledWith(tokenResponse.privateKey); + expect(cryptoService.setPrivateKey).toHaveBeenCalledWith(tokenResponse.privateKey, mockUserId); }); it("sets keys after a successful authentication when only userKey provided in login credentials", async () => { @@ -164,7 +164,7 @@ describe("AuthRequestLoginStrategy", () => { // setMasterKeyEncryptedUserKey, setUserKey, and setPrivateKey should still be called expect(cryptoService.setMasterKeyEncryptedUserKey).toHaveBeenCalledWith(tokenResponse.key); expect(cryptoService.setUserKey).toHaveBeenCalledWith(decUserKey); - expect(cryptoService.setPrivateKey).toHaveBeenCalledWith(tokenResponse.privateKey); + expect(cryptoService.setPrivateKey).toHaveBeenCalledWith(tokenResponse.privateKey, mockUserId); // trustDeviceIfRequired should be called expect(deviceTrustService.trustDeviceIfRequired).not.toHaveBeenCalled(); diff --git a/libs/auth/src/common/login-strategies/auth-request-login.strategy.ts b/libs/auth/src/common/login-strategies/auth-request-login.strategy.ts index c8acc6c24b..e815d8f3ba 100644 --- a/libs/auth/src/common/login-strategies/auth-request-login.strategy.ts +++ b/libs/auth/src/common/login-strategies/auth-request-login.strategy.ts @@ -161,9 +161,13 @@ export class AuthRequestLoginStrategy extends LoginStrategy { } } - protected override async setPrivateKey(response: IdentityTokenResponse): Promise { + protected override async setPrivateKey( + response: IdentityTokenResponse, + userId: UserId, + ): Promise { await this.cryptoService.setPrivateKey( - response.privateKey ?? (await this.createKeyPairForOldAccount()), + response.privateKey ?? (await this.createKeyPairForOldAccount(userId)), + userId, ); } diff --git a/libs/auth/src/common/login-strategies/login.strategy.ts b/libs/auth/src/common/login-strategies/login.strategy.ts index 96f7b73cab..adcf753325 100644 --- a/libs/auth/src/common/login-strategies/login.strategy.ts +++ b/libs/auth/src/common/login-strategies/login.strategy.ts @@ -252,7 +252,7 @@ export abstract class LoginStrategy { await this.setMasterKey(response, userId); await this.setUserKey(response, userId); - await this.setPrivateKey(response); + await this.setPrivateKey(response, userId); this.messagingService.send("loggedIn"); @@ -262,7 +262,7 @@ export abstract class LoginStrategy { // The keys comes from different sources depending on the login strategy protected abstract setMasterKey(response: IdentityTokenResponse, userId: UserId): Promise; protected abstract setUserKey(response: IdentityTokenResponse, userId: UserId): Promise; - protected abstract setPrivateKey(response: IdentityTokenResponse): Promise; + protected abstract setPrivateKey(response: IdentityTokenResponse, userId: UserId): Promise; // Old accounts used master key for encryption. We are forcing migrations but only need to // check on password logins @@ -270,9 +270,10 @@ export abstract class LoginStrategy { return false; } - protected async createKeyPairForOldAccount() { + protected async createKeyPairForOldAccount(userId: UserId) { try { - const [publicKey, privateKey] = await this.cryptoService.makeKeyPair(); + const userKey = await this.cryptoService.getUserKeyWithLegacySupport(userId); + const [publicKey, privateKey] = await this.cryptoService.makeKeyPair(userKey); await this.apiService.postAccountKeys(new KeysRequest(publicKey, privateKey.encryptedString)); return privateKey.encryptedString; } catch (e) { diff --git a/libs/auth/src/common/login-strategies/password-login.strategy.spec.ts b/libs/auth/src/common/login-strategies/password-login.strategy.spec.ts index e85d01c5d7..f887f047a9 100644 --- a/libs/auth/src/common/login-strategies/password-login.strategy.spec.ts +++ b/libs/auth/src/common/login-strategies/password-login.strategy.spec.ts @@ -178,7 +178,7 @@ describe("PasswordLoginStrategy", () => { userId, ); expect(cryptoService.setUserKey).toHaveBeenCalledWith(userKey, userId); - expect(cryptoService.setPrivateKey).toHaveBeenCalledWith(tokenResponse.privateKey); + expect(cryptoService.setPrivateKey).toHaveBeenCalledWith(tokenResponse.privateKey, userId); }); it("does not force the user to update their master password when there are no requirements", async () => { diff --git a/libs/auth/src/common/login-strategies/password-login.strategy.ts b/libs/auth/src/common/login-strategies/password-login.strategy.ts index ec2a4850fe..80048d6e10 100644 --- a/libs/auth/src/common/login-strategies/password-login.strategy.ts +++ b/libs/auth/src/common/login-strategies/password-login.strategy.ts @@ -233,9 +233,13 @@ export class PasswordLoginStrategy extends LoginStrategy { } } - protected override async setPrivateKey(response: IdentityTokenResponse): Promise { + protected override async setPrivateKey( + response: IdentityTokenResponse, + userId: UserId, + ): Promise { await this.cryptoService.setPrivateKey( - response.privateKey ?? (await this.createKeyPairForOldAccount()), + response.privateKey ?? (await this.createKeyPairForOldAccount(userId)), + userId, ); } diff --git a/libs/auth/src/common/login-strategies/sso-login.strategy.ts b/libs/auth/src/common/login-strategies/sso-login.strategy.ts index 37f616c98a..f328547772 100644 --- a/libs/auth/src/common/login-strategies/sso-login.strategy.ts +++ b/libs/auth/src/common/login-strategies/sso-login.strategy.ts @@ -354,12 +354,16 @@ export class SsoLoginStrategy extends LoginStrategy { await this.cryptoService.setUserKey(userKey); } - protected override async setPrivateKey(tokenResponse: IdentityTokenResponse): Promise { + protected override async setPrivateKey( + tokenResponse: IdentityTokenResponse, + userId: UserId, + ): Promise { const newSsoUser = tokenResponse.key == null; if (!newSsoUser) { await this.cryptoService.setPrivateKey( - tokenResponse.privateKey ?? (await this.createKeyPairForOldAccount()), + tokenResponse.privateKey ?? (await this.createKeyPairForOldAccount(userId)), + userId, ); } } diff --git a/libs/auth/src/common/login-strategies/user-api-login.strategy.spec.ts b/libs/auth/src/common/login-strategies/user-api-login.strategy.spec.ts index 03dfa4f5fa..673fadd5b0 100644 --- a/libs/auth/src/common/login-strategies/user-api-login.strategy.spec.ts +++ b/libs/auth/src/common/login-strategies/user-api-login.strategy.spec.ts @@ -159,7 +159,7 @@ describe("UserApiLoginStrategy", () => { await apiLogInStrategy.logIn(credentials); expect(cryptoService.setMasterKeyEncryptedUserKey).toHaveBeenCalledWith(tokenResponse.key); - expect(cryptoService.setPrivateKey).toHaveBeenCalledWith(tokenResponse.privateKey); + expect(cryptoService.setPrivateKey).toHaveBeenCalledWith(tokenResponse.privateKey, userId); }); it("gets and sets the master key if Key Connector is enabled", async () => { diff --git a/libs/auth/src/common/login-strategies/user-api-login.strategy.ts b/libs/auth/src/common/login-strategies/user-api-login.strategy.ts index b9d0c9e588..440dccd12e 100644 --- a/libs/auth/src/common/login-strategies/user-api-login.strategy.ts +++ b/libs/auth/src/common/login-strategies/user-api-login.strategy.ts @@ -116,9 +116,13 @@ export class UserApiLoginStrategy extends LoginStrategy { } } - protected override async setPrivateKey(response: IdentityTokenResponse): Promise { + protected override async setPrivateKey( + response: IdentityTokenResponse, + userId: UserId, + ): Promise { await this.cryptoService.setPrivateKey( - response.privateKey ?? (await this.createKeyPairForOldAccount()), + response.privateKey ?? (await this.createKeyPairForOldAccount(userId)), + userId, ); } diff --git a/libs/auth/src/common/login-strategies/webauthn-login.strategy.spec.ts b/libs/auth/src/common/login-strategies/webauthn-login.strategy.spec.ts index 9e977193dd..afac2c2e6a 100644 --- a/libs/auth/src/common/login-strategies/webauthn-login.strategy.spec.ts +++ b/libs/auth/src/common/login-strategies/webauthn-login.strategy.spec.ts @@ -224,7 +224,7 @@ describe("WebAuthnLoginStrategy", () => { mockPrfPrivateKey, ); expect(cryptoService.setUserKey).toHaveBeenCalledWith(mockUserKey, userId); - expect(cryptoService.setPrivateKey).toHaveBeenCalledWith(idTokenResponse.privateKey); + expect(cryptoService.setPrivateKey).toHaveBeenCalledWith(idTokenResponse.privateKey, userId); // Master key and private key should not be set expect(masterPasswordService.mock.setMasterKey).not.toHaveBeenCalled(); diff --git a/libs/auth/src/common/login-strategies/webauthn-login.strategy.ts b/libs/auth/src/common/login-strategies/webauthn-login.strategy.ts index 714edabc1e..4b5441d00a 100644 --- a/libs/auth/src/common/login-strategies/webauthn-login.strategy.ts +++ b/libs/auth/src/common/login-strategies/webauthn-login.strategy.ts @@ -139,9 +139,13 @@ export class WebAuthnLoginStrategy extends LoginStrategy { } } - protected override async setPrivateKey(response: IdentityTokenResponse): Promise { + protected override async setPrivateKey( + response: IdentityTokenResponse, + userId: UserId, + ): Promise { await this.cryptoService.setPrivateKey( - response.privateKey ?? (await this.createKeyPairForOldAccount()), + response.privateKey ?? (await this.createKeyPairForOldAccount(userId)), + userId, ); } diff --git a/libs/common/src/auth/services/key-connector.service.ts b/libs/common/src/auth/services/key-connector.service.ts index 5065e58c76..65d1030bd3 100644 --- a/libs/common/src/auth/services/key-connector.service.ts +++ b/libs/common/src/auth/services/key-connector.service.ts @@ -155,7 +155,7 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { await this.cryptoService.setUserKey(userKey[0], userId); await this.cryptoService.setMasterKeyEncryptedUserKey(userKey[1].encryptedString, userId); - const [pubKey, privKey] = await this.cryptoService.makeKeyPair(); + const [pubKey, privKey] = await this.cryptoService.makeKeyPair(userKey[0]); try { const keyConnectorUrl = diff --git a/libs/common/src/models/response/profile.response.ts b/libs/common/src/models/response/profile.response.ts index fbaa4f84ef..3d5fde9ac4 100644 --- a/libs/common/src/models/response/profile.response.ts +++ b/libs/common/src/models/response/profile.response.ts @@ -1,11 +1,12 @@ import { ProfileOrganizationResponse } from "../../admin-console/models/response/profile-organization.response"; import { ProfileProviderOrganizationResponse } from "../../admin-console/models/response/profile-provider-organization.response"; import { ProfileProviderResponse } from "../../admin-console/models/response/profile-provider.response"; +import { UserId } from "../../types/guid"; import { BaseResponse } from "./base.response"; export class ProfileResponse extends BaseResponse { - id: string; + id: UserId; name: string; email: string; emailVerified: boolean; diff --git a/libs/common/src/platform/abstractions/crypto.service.ts b/libs/common/src/platform/abstractions/crypto.service.ts index 0a210e6709..43f47b0a3d 100644 --- a/libs/common/src/platform/abstractions/crypto.service.ts +++ b/libs/common/src/platform/abstractions/crypto.service.ts @@ -54,13 +54,23 @@ export abstract class CryptoService { * for encryption of data instead of the user key. */ abstract isLegacyUser(masterKey?: MasterKey, userId?: string): Promise; + + /** + * Use for encryption/decryption of data in order to support legacy + * encryption models. It will return the user key if available, + * if not it will return the master key. + * + * @deprecated Please provide the userId of the user you want the user key for. + */ + abstract getUserKeyWithLegacySupport(): Promise; + /** * Use for encryption/decryption of data in order to support legacy * encryption models. It will return the user key if available, * if not it will return the master key. * @param userId The desired user */ - abstract getUserKeyWithLegacySupport(userId?: string): Promise; + abstract getUserKeyWithLegacySupport(userId: UserId): Promise; /** * Retrieves the user key from storage * @param keySuffix The desired version of the user's key to retrieve @@ -169,10 +179,12 @@ export abstract class CryptoService { * organization keys currently in memory * @param orgs The organizations to set keys for * @param providerOrgs The provider organizations to set keys for + * @param userId The user id of the user to set the org keys for */ abstract setOrgKeys( orgs: ProfileOrganizationResponse[], providerOrgs: ProfileProviderOrganizationResponse[], + userId: UserId, ): Promise; abstract activeUserOrgKeys$: Observable>; /** @@ -200,7 +212,13 @@ export abstract class CryptoService { * @param providers The providers to set keys for */ abstract activeUserProviderKeys$: Observable>; - abstract setProviderKeys(orgs: ProfileProviderResponse[]): Promise; + + /** + * Stores the provider keys for a given user. + * @param orgs The provider orgs for which to save the keys from. + * @param userId The user id of the user for which to store the keys for. + */ + abstract setProviderKeys(orgs: ProfileProviderResponse[], userId: UserId): Promise; /** * @param providerId The desired provider * @returns The provider's symmetric key @@ -228,7 +246,7 @@ export abstract class CryptoService { * Note: does not clear the private key if null is provided * @param encPrivateKey An encrypted private key */ - abstract setPrivateKey(encPrivateKey: string): Promise; + abstract setPrivateKey(encPrivateKey: string, userId: UserId): Promise; /** * Returns the private key from memory. If not available, decrypts it * from storage and stores it in memory @@ -247,8 +265,9 @@ export abstract class CryptoService { * @param key A key to encrypt the private key with. If not provided, * defaults to the user key * @returns A new keypair: [publicKey in Base64, encrypted privateKey] + * @throws If the provided key is a null-ish value. */ - abstract makeKeyPair(key?: SymmetricCryptoKey): Promise<[string, EncString]>; + abstract makeKeyPair(key: SymmetricCryptoKey): Promise<[string, EncString]>; /** * @param pin The user's pin * @param salt The user's salt diff --git a/libs/common/src/platform/services/crypto.service.ts b/libs/common/src/platform/services/crypto.service.ts index 6d9574ef38..873f3ab9a7 100644 --- a/libs/common/src/platform/services/crypto.service.ts +++ b/libs/common/src/platform/services/crypto.service.ts @@ -395,12 +395,11 @@ export class CryptoService implements CryptoServiceAbstraction { } async setOrgKeys( - orgs: ProfileOrganizationResponse[] = [], - providerOrgs: ProfileProviderOrganizationResponse[] = [], + orgs: ProfileOrganizationResponse[], + providerOrgs: ProfileProviderOrganizationResponse[], + userId: UserId, ): Promise { - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - this.activeUserEncryptedOrgKeysState.update((_) => { + await this.stateProvider.getUser(userId, USER_ENCRYPTED_ORGANIZATION_KEYS).update(() => { const encOrgKeyData: { [orgId: string]: EncryptedOrganizationKeyData } = {}; orgs.forEach((org) => { @@ -450,8 +449,8 @@ export class CryptoService implements CryptoServiceAbstraction { await this.stateProvider.setUserState(USER_ENCRYPTED_ORGANIZATION_KEYS, null, userId); } - async setProviderKeys(providers: ProfileProviderResponse[]): Promise { - await this.activeUserEncryptedProviderKeysState.update((_) => { + async setProviderKeys(providers: ProfileProviderResponse[], userId: UserId): Promise { + await this.stateProvider.getUser(userId, USER_ENCRYPTED_PROVIDER_KEYS).update(() => { const encProviderKeys: { [providerId: ProviderId]: EncryptedString } = {}; providers.forEach((provider) => { @@ -494,12 +493,14 @@ export class CryptoService implements CryptoServiceAbstraction { return [encShareKey, shareKey as T]; } - async setPrivateKey(encPrivateKey: EncryptedString): Promise { + async setPrivateKey(encPrivateKey: EncryptedString, userId: UserId): Promise { if (encPrivateKey == null) { return; } - await this.activeUserEncryptedPrivateKeyState.update(() => encPrivateKey); + await this.stateProvider + .getUser(userId, USER_ENCRYPTED_PRIVATE_KEY) + .update(() => encPrivateKey); } async getPrivateKey(): Promise { @@ -523,9 +524,10 @@ export class CryptoService implements CryptoServiceAbstraction { return this.hashPhrase(userFingerprint); } - async makeKeyPair(key?: SymmetricCryptoKey): Promise<[string, EncString]> { - // Default to user key - key ||= await this.getUserKeyWithLegacySupport(); + async makeKeyPair(key: SymmetricCryptoKey): Promise<[string, EncString]> { + if (key == null) { + throw new Error("'key' is a required parameter and must be non-null."); + } const keyPair = await this.cryptoFunctionService.rsaGenerateKeyPair(2048); const publicB64 = Utils.fromBufferToB64(keyPair[0]); diff --git a/libs/common/src/vault/services/sync/sync.service.ts b/libs/common/src/vault/services/sync/sync.service.ts index 995ab7319b..793bcf2437 100644 --- a/libs/common/src/vault/services/sync/sync.service.ts +++ b/libs/common/src/vault/services/sync/sync.service.ts @@ -35,7 +35,6 @@ import { SendData } from "../../../tools/send/models/data/send.data"; import { SendResponse } from "../../../tools/send/models/response/send.response"; import { SendApiService } from "../../../tools/send/services/send-api.service.abstraction"; import { InternalSendService } from "../../../tools/send/services/send.service.abstraction"; -import { UserId } from "../../../types/guid"; import { CipherService } from "../../../vault/abstractions/cipher.service"; import { FolderApiServiceAbstraction } from "../../../vault/abstractions/folder/folder-api.service.abstraction"; import { InternalFolderService } from "../../../vault/abstractions/folder/folder.service.abstraction"; @@ -311,7 +310,7 @@ export class SyncService implements SyncServiceAbstraction { } private async syncProfile(response: ProfileResponse) { - const stamp = await this.tokenService.getSecurityStamp(response.id as UserId); + const stamp = await this.tokenService.getSecurityStamp(response.id); if (stamp != null && stamp !== response.securityStamp) { if (this.logoutCallback != null) { await this.logoutCallback(true); @@ -321,15 +320,16 @@ export class SyncService implements SyncServiceAbstraction { } await this.cryptoService.setMasterKeyEncryptedUserKey(response.key); - await this.cryptoService.setPrivateKey(response.privateKey); - await this.cryptoService.setProviderKeys(response.providers); - await this.cryptoService.setOrgKeys(response.organizations, response.providerOrganizations); - await this.avatarService.setSyncAvatarColor(response.id as UserId, response.avatarColor); - await this.tokenService.setSecurityStamp(response.securityStamp, response.id as UserId); - await this.accountService.setAccountEmailVerified( - response.id as UserId, - response.emailVerified, + await this.cryptoService.setPrivateKey(response.privateKey, response.id); + await this.cryptoService.setProviderKeys(response.providers, response.id); + await this.cryptoService.setOrgKeys( + response.organizations, + response.providerOrganizations, + response.id, ); + await this.avatarService.setSyncAvatarColor(response.id, response.avatarColor); + await this.tokenService.setSecurityStamp(response.securityStamp, response.id); + await this.accountService.setAccountEmailVerified(response.id, response.emailVerified); await this.billingAccountProfileStateService.setHasPremium( response.premiumPersonally,