[PM-6727] Part 1: pass userId in login strategies (#9030)

* add validation to initAccount

* pass userId to setMasterKey

* fix key connector tests
This commit is contained in:
Jake Fink 2024-05-03 11:54:29 -04:00 committed by GitHub
parent debfe914c2
commit 69ed6ce1f5
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
16 changed files with 101 additions and 65 deletions

View File

@ -117,13 +117,12 @@ export class AuthRequestLoginStrategy extends LoginStrategy {
return super.logInTwoFactor(twoFactor);
}
protected override async setMasterKey(response: IdentityTokenResponse) {
protected override async setMasterKey(response: IdentityTokenResponse, userId: UserId) {
const authRequestCredentials = this.cache.value.authRequestCredentials;
if (
authRequestCredentials.decryptedMasterKey &&
authRequestCredentials.decryptedMasterKeyHash
) {
const userId = (await firstValueFrom(this.accountService.activeAccount$))?.id;
await this.masterPasswordService.setMasterKey(
authRequestCredentials.decryptedMasterKey,
userId,
@ -147,15 +146,14 @@ export class AuthRequestLoginStrategy extends LoginStrategy {
if (authRequestCredentials.decryptedUserKey) {
await this.cryptoService.setUserKey(authRequestCredentials.decryptedUserKey);
} else {
await this.trySetUserKeyWithMasterKey();
await this.trySetUserKeyWithMasterKey(userId);
// Establish trust if required after setting user key
await this.deviceTrustService.trustDeviceIfRequired(userId);
}
}
private async trySetUserKeyWithMasterKey(): Promise<void> {
const userId = (await firstValueFrom(this.accountService.activeAccount$))?.id;
private async trySetUserKeyWithMasterKey(userId: UserId): Promise<void> {
const masterKey = await firstValueFrom(this.masterPasswordService.masterKey$(userId));
if (masterKey) {
const userKey = await this.cryptoService.decryptUserKeyWithMasterKey(masterKey);

View File

@ -244,6 +244,7 @@ describe("LoginStrategy", () => {
const result = await passwordLoginStrategy.logIn(credentials);
expect(result).toEqual({
userId: userId,
forcePasswordReset: ForceSetPasswordReason.AdminForcePasswordReset,
resetMasterPassword: true,
twoFactorProviders: null,

View File

@ -241,6 +241,7 @@ export abstract class LoginStrategy {
// Must come before setting keys, user key needs email to update additional keys
const userId = await this.saveAccountInformation(response);
result.userId = userId;
if (response.twoFactorToken != null) {
// note: we can read email from access token b/c it was saved in saveAccountInformation
@ -249,7 +250,7 @@ export abstract class LoginStrategy {
await this.tokenService.setTwoFactorToken(userEmail, response.twoFactorToken);
}
await this.setMasterKey(response);
await this.setMasterKey(response, userId);
await this.setUserKey(response, userId);
await this.setPrivateKey(response);
@ -259,7 +260,7 @@ export abstract class LoginStrategy {
}
// The keys comes from different sources depending on the login strategy
protected abstract setMasterKey(response: IdentityTokenResponse): Promise<void>;
protected abstract setMasterKey(response: IdentityTokenResponse, userId: UserId): Promise<void>;
protected abstract setUserKey(response: IdentityTokenResponse, userId: UserId): Promise<void>;
protected abstract setPrivateKey(response: IdentityTokenResponse): Promise<void>;

View File

@ -173,8 +173,11 @@ describe("PasswordLoginStrategy", () => {
localHashedPassword,
userId,
);
expect(cryptoService.setMasterKeyEncryptedUserKey).toHaveBeenCalledWith(tokenResponse.key);
expect(cryptoService.setUserKey).toHaveBeenCalledWith(userKey);
expect(cryptoService.setMasterKeyEncryptedUserKey).toHaveBeenCalledWith(
tokenResponse.key,
userId,
);
expect(cryptoService.setUserKey).toHaveBeenCalledWith(userKey, userId);
expect(cryptoService.setPrivateKey).toHaveBeenCalledWith(tokenResponse.privateKey);
});

View File

@ -147,6 +147,10 @@ export class PasswordLoginStrategy extends LoginStrategy {
const [authResult, identityResponse] = await this.startLogIn();
if (identityResponse instanceof IdentityCaptchaResponse) {
return authResult;
}
const masterPasswordPolicyOptions =
this.getMasterPasswordPolicyOptionsFromResponse(identityResponse);
@ -157,23 +161,23 @@ export class PasswordLoginStrategy extends LoginStrategy {
credentials,
masterPasswordPolicyOptions,
);
if (meetsRequirements) {
return authResult;
}
if (!meetsRequirements) {
if (authResult.requiresCaptcha || authResult.requiresTwoFactor) {
// Save the flag to this strategy for later use as the master password is about to pass out of scope
this.cache.next({
...this.cache.value,
forcePasswordResetReason: ForceSetPasswordReason.WeakMasterPassword,
});
} else {
// Authentication was successful, save the force update password options with the state service
const userId = (await firstValueFrom(this.accountService.activeAccount$))?.id;
await this.masterPasswordService.setForceSetPasswordReason(
ForceSetPasswordReason.WeakMasterPassword,
userId,
);
authResult.forcePasswordReset = ForceSetPasswordReason.WeakMasterPassword;
}
if (identityResponse instanceof IdentityTwoFactorResponse) {
// Save the flag to this strategy for use in 2fa login as the master password is about to pass out of scope
this.cache.next({
...this.cache.value,
forcePasswordResetReason: ForceSetPasswordReason.WeakMasterPassword,
});
} else {
// Authentication was successful, save the force update password options with the state service
await this.masterPasswordService.setForceSetPasswordReason(
ForceSetPasswordReason.WeakMasterPassword,
authResult.userId, // userId is only available on successful login
);
authResult.forcePasswordReset = ForceSetPasswordReason.WeakMasterPassword;
}
}
return authResult;
@ -196,17 +200,18 @@ export class PasswordLoginStrategy extends LoginStrategy {
!result.requiresCaptcha &&
forcePasswordResetReason != ForceSetPasswordReason.None
) {
const userId = (await firstValueFrom(this.accountService.activeAccount$))?.id;
await this.masterPasswordService.setForceSetPasswordReason(forcePasswordResetReason, userId);
await this.masterPasswordService.setForceSetPasswordReason(
forcePasswordResetReason,
result.userId,
);
result.forcePasswordReset = forcePasswordResetReason;
}
return result;
}
protected override async setMasterKey(response: IdentityTokenResponse) {
protected override async setMasterKey(response: IdentityTokenResponse, userId: UserId) {
const { masterKey, localMasterKeyHash } = this.cache.value;
const userId = (await firstValueFrom(this.accountService.activeAccount$))?.id;
await this.masterPasswordService.setMasterKey(masterKey, userId);
await this.masterPasswordService.setMasterKeyHash(localMasterKeyHash, userId);
}
@ -219,12 +224,12 @@ export class PasswordLoginStrategy extends LoginStrategy {
if (this.encryptionKeyMigrationRequired(response)) {
return;
}
await this.cryptoService.setMasterKeyEncryptedUserKey(response.key);
await this.cryptoService.setMasterKeyEncryptedUserKey(response.key, userId);
const masterKey = await firstValueFrom(this.masterPasswordService.masterKey$(userId));
if (masterKey) {
const userKey = await this.cryptoService.decryptUserKeyWithMasterKey(masterKey);
await this.cryptoService.setUserKey(userKey);
await this.cryptoService.setUserKey(userKey, userId);
}
}
@ -239,9 +244,9 @@ export class PasswordLoginStrategy extends LoginStrategy {
}
private getMasterPasswordPolicyOptionsFromResponse(
response: IdentityTokenResponse | IdentityTwoFactorResponse | IdentityCaptchaResponse,
response: IdentityTokenResponse | IdentityTwoFactorResponse,
): MasterPasswordPolicyOptions {
if (response == null || response instanceof IdentityCaptchaResponse) {
if (response == null) {
return null;
}
return MasterPasswordPolicyOptions.fromResponse(response.masterPasswordPolicy);

View File

@ -163,7 +163,10 @@ describe("SsoLoginStrategy", () => {
// Assert
expect(cryptoService.setMasterKeyEncryptedUserKey).toHaveBeenCalledTimes(1);
expect(cryptoService.setMasterKeyEncryptedUserKey).toHaveBeenCalledWith(tokenResponse.key);
expect(cryptoService.setMasterKeyEncryptedUserKey).toHaveBeenCalledWith(
tokenResponse.key,
userId,
);
});
describe("Trusted Device Decryption", () => {
@ -417,7 +420,7 @@ describe("SsoLoginStrategy", () => {
await ssoLoginStrategy.logIn(credentials);
expect(keyConnectorService.setMasterKeyFromUrl).toHaveBeenCalledWith(keyConnectorUrl);
expect(keyConnectorService.setMasterKeyFromUrl).toHaveBeenCalledWith(keyConnectorUrl, userId);
});
it("converts new SSO user with no master password to Key Connector on first login", async () => {
@ -430,6 +433,7 @@ describe("SsoLoginStrategy", () => {
expect(keyConnectorService.convertNewSsoUserToKeyConnector).toHaveBeenCalledWith(
tokenResponse,
ssoOrgId,
userId,
);
});
@ -468,7 +472,7 @@ describe("SsoLoginStrategy", () => {
await ssoLoginStrategy.logIn(credentials);
expect(keyConnectorService.setMasterKeyFromUrl).toHaveBeenCalledWith(keyConnectorUrl);
expect(keyConnectorService.setMasterKeyFromUrl).toHaveBeenCalledWith(keyConnectorUrl, userId);
});
it("converts new SSO user with no master password to Key Connector on first login", async () => {
@ -481,6 +485,7 @@ describe("SsoLoginStrategy", () => {
expect(keyConnectorService.convertNewSsoUserToKeyConnector).toHaveBeenCalledWith(
tokenResponse,
ssoOrgId,
userId,
);
});

View File

@ -8,6 +8,7 @@ import { KeyConnectorService } from "@bitwarden/common/auth/abstractions/key-con
import { InternalMasterPasswordServiceAbstraction } from "@bitwarden/common/auth/abstractions/master-password.service.abstraction";
import { TokenService } from "@bitwarden/common/auth/abstractions/token.service";
import { TwoFactorService } from "@bitwarden/common/auth/abstractions/two-factor.service";
import { AuthResult } from "@bitwarden/common/auth/models/domain/auth-result";
import { ForceSetPasswordReason } from "@bitwarden/common/auth/models/domain/force-set-password-reason";
import { SsoTokenRequest } from "@bitwarden/common/auth/models/request/identity-token/sso-token.request";
import { AuthRequestResponse } from "@bitwarden/common/auth/models/response/auth-request.response";
@ -124,7 +125,7 @@ export class SsoLoginStrategy extends LoginStrategy {
this.ssoEmail2FaSessionToken$ = this.cache.pipe(map((state) => state.ssoEmail2FaSessionToken));
}
async logIn(credentials: SsoLoginCredentials) {
async logIn(credentials: SsoLoginCredentials): Promise<AuthResult> {
const data = new SsoLoginStrategyData();
data.orgId = credentials.orgId;
@ -147,10 +148,9 @@ export class SsoLoginStrategy extends LoginStrategy {
// Auth guard currently handles redirects for this.
if (ssoAuthResult.forcePasswordReset == ForceSetPasswordReason.AdminForcePasswordReset) {
const userId = (await firstValueFrom(this.accountService.activeAccount$))?.id;
await this.masterPasswordService.setForceSetPasswordReason(
ssoAuthResult.forcePasswordReset,
userId,
ssoAuthResult.userId,
);
}
@ -163,7 +163,7 @@ export class SsoLoginStrategy extends LoginStrategy {
return ssoAuthResult;
}
protected override async setMasterKey(tokenResponse: IdentityTokenResponse) {
protected override async setMasterKey(tokenResponse: IdentityTokenResponse, userId: UserId) {
// The only way we can be setting a master key at this point is if we are using Key Connector.
// First, check to make sure that we should do so based on the token response.
if (this.shouldSetMasterKeyFromKeyConnector(tokenResponse)) {
@ -175,10 +175,11 @@ export class SsoLoginStrategy extends LoginStrategy {
await this.keyConnectorService.convertNewSsoUserToKeyConnector(
tokenResponse,
this.cache.value.orgId,
userId,
);
} else {
const keyConnectorUrl = this.getKeyConnectorUrl(tokenResponse);
await this.keyConnectorService.setMasterKeyFromUrl(keyConnectorUrl);
await this.keyConnectorService.setMasterKeyFromUrl(keyConnectorUrl, userId);
}
}
}
@ -231,7 +232,7 @@ export class SsoLoginStrategy extends LoginStrategy {
if (masterKeyEncryptedUserKey) {
// set the master key encrypted user key if it exists
await this.cryptoService.setMasterKeyEncryptedUserKey(masterKeyEncryptedUserKey);
await this.cryptoService.setMasterKeyEncryptedUserKey(masterKeyEncryptedUserKey, userId);
}
const userDecryptionOptions = tokenResponse?.userDecryptionOptions;
@ -251,7 +252,7 @@ export class SsoLoginStrategy extends LoginStrategy {
this.getKeyConnectorUrl(tokenResponse) != null
) {
// Key connector enabled for user
await this.trySetUserKeyWithMasterKey();
await this.trySetUserKeyWithMasterKey(userId);
}
// Note: In the traditional SSO flow with MP without key connector, the lock component
@ -338,8 +339,7 @@ export class SsoLoginStrategy extends LoginStrategy {
}
}
private async trySetUserKeyWithMasterKey(): Promise<void> {
const userId = (await firstValueFrom(this.accountService.activeAccount$))?.id;
private async trySetUserKeyWithMasterKey(userId: UserId): Promise<void> {
const masterKey = await firstValueFrom(this.masterPasswordService.masterKey$(userId));
// There is a scenario in which the master key is not set here. That will occur if the user

View File

@ -174,7 +174,7 @@ describe("UserApiLoginStrategy", () => {
await apiLogInStrategy.logIn(credentials);
expect(keyConnectorService.setMasterKeyFromUrl).toHaveBeenCalledWith(keyConnectorUrl);
expect(keyConnectorService.setMasterKeyFromUrl).toHaveBeenCalledWith(keyConnectorUrl, userId);
});
it("decrypts and sets the user key if Key Connector is enabled", async () => {
@ -195,6 +195,6 @@ describe("UserApiLoginStrategy", () => {
await apiLogInStrategy.logIn(credentials);
expect(cryptoService.decryptUserKeyWithMasterKey).toHaveBeenCalledWith(masterKey);
expect(cryptoService.setUserKey).toHaveBeenCalledWith(userKey);
expect(cryptoService.setUserKey).toHaveBeenCalledWith(userKey, userId);
});
});

View File

@ -93,11 +93,11 @@ export class UserApiLoginStrategy extends LoginStrategy {
return authResult;
}
protected override async setMasterKey(response: IdentityTokenResponse) {
protected override async setMasterKey(response: IdentityTokenResponse, userId: UserId) {
if (response.apiUseKeyConnector) {
const env = await firstValueFrom(this.environmentService.environment$);
const keyConnectorUrl = env.getKeyConnectorUrl();
await this.keyConnectorService.setMasterKeyFromUrl(keyConnectorUrl);
await this.keyConnectorService.setMasterKeyFromUrl(keyConnectorUrl, userId);
}
}
@ -108,11 +108,10 @@ export class UserApiLoginStrategy extends LoginStrategy {
await this.cryptoService.setMasterKeyEncryptedUserKey(response.key);
if (response.apiUseKeyConnector) {
const userId = (await firstValueFrom(this.accountService.activeAccount$))?.id;
const masterKey = await firstValueFrom(this.masterPasswordService.masterKey$(userId));
if (masterKey) {
const userKey = await this.cryptoService.decryptUserKeyWithMasterKey(masterKey);
await this.cryptoService.setUserKey(userKey);
await this.cryptoService.setUserKey(userKey, userId);
}
}
}
@ -123,6 +122,7 @@ export class UserApiLoginStrategy extends LoginStrategy {
);
}
// Overridden to save client ID and secret to token service
protected async saveAccountInformation(tokenResponse: IdentityTokenResponse): Promise<UserId> {
const userId = await super.saveAccountInformation(tokenResponse);

View File

@ -208,7 +208,10 @@ describe("WebAuthnLoginStrategy", () => {
// Assert
// Master key encrypted user key should be set
expect(cryptoService.setMasterKeyEncryptedUserKey).toHaveBeenCalledTimes(1);
expect(cryptoService.setMasterKeyEncryptedUserKey).toHaveBeenCalledWith(idTokenResponse.key);
expect(cryptoService.setMasterKeyEncryptedUserKey).toHaveBeenCalledWith(
idTokenResponse.key,
userId,
);
expect(cryptoService.decryptToBytes).toHaveBeenCalledTimes(1);
expect(cryptoService.decryptToBytes).toHaveBeenCalledWith(
@ -220,7 +223,7 @@ describe("WebAuthnLoginStrategy", () => {
idTokenResponse.userDecryptionOptions.webAuthnPrfOption.encryptedUserKey.encryptedString,
mockPrfPrivateKey,
);
expect(cryptoService.setUserKey).toHaveBeenCalledWith(mockUserKey);
expect(cryptoService.setUserKey).toHaveBeenCalledWith(mockUserKey, userId);
expect(cryptoService.setPrivateKey).toHaveBeenCalledWith(idTokenResponse.privateKey);
// Master key and private key should not be set

View File

@ -98,7 +98,7 @@ export class WebAuthnLoginStrategy extends LoginStrategy {
throw new Error("2FA not supported yet for WebAuthn Login.");
}
protected override async setMasterKey() {
protected override async setMasterKey(response: IdentityTokenResponse, userId: UserId) {
return Promise.resolve();
}
@ -107,7 +107,7 @@ export class WebAuthnLoginStrategy extends LoginStrategy {
if (masterKeyEncryptedUserKey) {
// set the master key encrypted user key if it exists
await this.cryptoService.setMasterKeyEncryptedUserKey(masterKeyEncryptedUserKey);
await this.cryptoService.setMasterKeyEncryptedUserKey(masterKeyEncryptedUserKey, userId);
}
const userDecryptionOptions = idTokenResponse?.userDecryptionOptions;
@ -134,7 +134,7 @@ export class WebAuthnLoginStrategy extends LoginStrategy {
);
if (userKey) {
await this.cryptoService.setUserKey(new SymmetricCryptoKey(userKey) as UserKey);
await this.cryptoService.setUserKey(new SymmetricCryptoKey(userKey) as UserKey, userId);
}
}
}

View File

@ -1,8 +1,9 @@
import { Organization } from "../../admin-console/models/domain/organization";
import { UserId } from "../../types/guid";
import { IdentityTokenResponse } from "../models/response/identity-token.response";
export abstract class KeyConnectorService {
setMasterKeyFromUrl: (url?: string) => Promise<void>;
setMasterKeyFromUrl: (url: string, userId: UserId) => Promise<void>;
getManagingOrganization: () => Promise<Organization>;
getUsesKeyConnector: () => Promise<boolean>;
migrateUser: () => Promise<void>;
@ -10,6 +11,7 @@ export abstract class KeyConnectorService {
convertNewSsoUserToKeyConnector: (
tokenResponse: IdentityTokenResponse,
orgId: string,
userId: UserId,
) => Promise<void>;
setUsesKeyConnector: (enabled: boolean) => Promise<void>;
setConvertAccountRequired: (status: boolean) => Promise<void>;

View File

@ -1,9 +1,11 @@
import { Utils } from "../../../platform/misc/utils";
import { UserId } from "../../../types/guid";
import { TwoFactorProviderType } from "../../enums/two-factor-provider-type";
import { ForceSetPasswordReason } from "./force-set-password-reason";
export class AuthResult {
userId: UserId;
captchaSiteKey = "";
// TODO: PM-3287 - Remove this after 3 releases of backwards compatibility. - Target release 2023.12 for removal
/**

View File

@ -215,7 +215,7 @@ describe("KeyConnectorService", () => {
const masterKey = new SymmetricCryptoKey(keyArr) as MasterKey;
// Act
await keyConnectorService.setMasterKeyFromUrl(url);
await keyConnectorService.setMasterKeyFromUrl(url, mockUserId);
// Assert
expect(apiService.getMasterKeyFromKeyConnector).toHaveBeenCalledWith(url);
@ -235,7 +235,7 @@ describe("KeyConnectorService", () => {
try {
// Act
await keyConnectorService.setMasterKeyFromUrl(url);
await keyConnectorService.setMasterKeyFromUrl(url, mockUserId);
} catch {
// Assert
expect(logService.error).toHaveBeenCalledWith(error);

View File

@ -16,6 +16,7 @@ import {
StateProvider,
UserKeyDefinition,
} from "../../platform/state";
import { UserId } from "../../types/guid";
import { MasterKey } from "../../types/key";
import { AccountService } from "../abstractions/account.service";
import { KeyConnectorService as KeyConnectorServiceAbstraction } from "../abstractions/key-connector.service";
@ -100,12 +101,11 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction {
}
// TODO: UserKey should be renamed to MasterKey and typed accordingly
async setMasterKeyFromUrl(url: string) {
async setMasterKeyFromUrl(url: string, userId: UserId) {
try {
const masterKeyResponse = await this.apiService.getMasterKeyFromKeyConnector(url);
const keyArr = Utils.fromB64ToArray(masterKeyResponse.key);
const masterKey = new SymmetricCryptoKey(keyArr) as MasterKey;
const userId = (await firstValueFrom(this.accountService.activeAccount$))?.id;
await this.masterPasswordService.setMasterKey(masterKey, userId);
} catch (e) {
this.handleKeyConnectorError(e);
@ -123,7 +123,11 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction {
);
}
async convertNewSsoUserToKeyConnector(tokenResponse: IdentityTokenResponse, orgId: string) {
async convertNewSsoUserToKeyConnector(
tokenResponse: IdentityTokenResponse,
orgId: string,
userId: UserId,
) {
// TODO: Remove after tokenResponse.keyConnectorUrl is deprecated in 2023.10 release (https://bitwarden.atlassian.net/browse/PM-3537)
const {
kdf,
@ -145,12 +149,11 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction {
kdfConfig,
);
const keyConnectorRequest = new KeyConnectorUserKeyRequest(masterKey.encKeyB64);
const userId = (await firstValueFrom(this.accountService.activeAccount$))?.id;
await this.masterPasswordService.setMasterKey(masterKey, userId);
const userKey = await this.cryptoService.makeUserKey(masterKey);
await this.cryptoService.setUserKey(userKey[0]);
await this.cryptoService.setMasterKeyEncryptedUserKey(userKey[1].encryptedString);
await this.cryptoService.setUserKey(userKey[0], userId);
await this.cryptoService.setMasterKeyEncryptedUserKey(userKey[1].encryptedString, userId);
const [pubKey, privKey] = await this.cryptoService.makeKeyPair();

View File

@ -771,6 +771,19 @@ export class CryptoService implements CryptoServiceAbstraction {
publicKey: string;
privateKey: EncString;
}> {
// Verify keys don't exist
const existingUserKey = await this.getUserKey();
const existingPrivateKey = await this.getPrivateKey();
if (existingUserKey != null || existingPrivateKey != null) {
if (existingUserKey != null) {
this.logService.error("Tried to initialize account with existing user key.");
}
if (existingPrivateKey != null) {
this.logService.error("Tried to initialize account with existing private key.");
}
throw new Error("Cannot initialize account, keys already exist.");
}
const userKey = (await this.keyGenerationService.createKey(512)) as UserKey;
const [publicKey, privateKey] = await this.makeKeyPair(userKey);
await this.setUserKey(userKey);