1
0
mirror of https://github.com/bitwarden/browser synced 2025-12-16 08:13:42 +00:00

[PM-5499] auth request service migrations (#8597)

* move auth request storage to service

* create migrations for auth requests

* fix tests

* fix browser

* fix login strategy

* update migration

* use correct test descriptions in migration
This commit is contained in:
Jake Fink
2024-04-15 12:34:30 -04:00
committed by GitHub
parent d0bcc75721
commit 576431d29e
26 changed files with 503 additions and 120 deletions

View File

@@ -1,12 +1,52 @@
import { Observable } from "rxjs";
import { AdminAuthRequestStorable } from "@bitwarden/common/auth/models/domain/admin-auth-req-storable";
import { AuthRequestResponse } from "@bitwarden/common/auth/models/response/auth-request.response";
import { AuthRequestPushNotification } from "@bitwarden/common/models/response/notification.response";
import { UserId } from "@bitwarden/common/types/guid";
import { UserKey, MasterKey } from "@bitwarden/common/types/key";
export abstract class AuthRequestServiceAbstraction {
/** Emits an auth request id when an auth request has been approved. */
authRequestPushNotification$: Observable<string>;
/**
* Returns true if the user has chosen to allow auth requests to show on this client.
* Intended to prevent spamming the user with auth requests.
* @param userId The user id.
* @throws If `userId` is not provided.
*/
abstract getAcceptAuthRequests: (userId: UserId) => Promise<boolean>;
/**
* Sets whether to allow auth requests to show on this client for this user.
* @param accept Whether to allow auth requests to show on this client.
* @param userId The user id.
* @throws If `userId` is not provided.
*/
abstract setAcceptAuthRequests: (accept: boolean, userId: UserId) => Promise<void>;
/**
* Returns an admin auth request for the given user if it exists.
* @param userId The user id.
* @throws If `userId` is not provided.
*/
abstract getAdminAuthRequest: (userId: UserId) => Promise<AdminAuthRequestStorable | null>;
/**
* Sets an admin auth request for the given user.
* Note: use {@link clearAdminAuthRequest} to clear the request.
* @param authRequest The admin auth request.
* @param userId The user id.
* @throws If `authRequest` or `userId` is not provided.
*/
abstract setAdminAuthRequest: (
authRequest: AdminAuthRequestStorable,
userId: UserId,
) => Promise<void>;
/**
* Clears an admin auth request for the given user.
* @param userId The user id.
* @throws If `userId` is not provided.
*/
abstract clearAdminAuthRequest: (userId: UserId) => Promise<void>;
/**
* Approve or deny an auth request.
* @param approve True to approve, false to deny.

View File

@@ -132,7 +132,10 @@ export class AuthRequestLoginStrategy extends LoginStrategy {
}
}
protected override async setUserKey(response: IdentityTokenResponse): Promise<void> {
protected override async setUserKey(
response: IdentityTokenResponse,
userId: UserId,
): Promise<void> {
const authRequestCredentials = this.cache.value.authRequestCredentials;
// User now may or may not have a master password
// but set the master key encrypted user key if it exists regardless
@@ -143,7 +146,6 @@ export class AuthRequestLoginStrategy extends LoginStrategy {
} else {
await this.trySetUserKeyWithMasterKey();
const userId = (await this.stateService.getUserId()) as UserId;
// Establish trust if required after setting user key
await this.deviceTrustCryptoService.trustDeviceIfRequired(userId);
}

View File

@@ -32,6 +32,7 @@ import {
AccountProfile,
AccountTokens,
} from "@bitwarden/common/platform/models/domain/account";
import { UserId } from "@bitwarden/common/types/guid";
import { InternalUserDecryptionOptionsServiceAbstraction } from "../abstractions/user-decryption-options.service.abstraction";
import {
@@ -160,14 +161,11 @@ export abstract class LoginStrategy {
* @param {IdentityTokenResponse} tokenResponse - The response from the server containing the identity token.
* @returns {Promise<void>} - A promise that resolves when the account information has been successfully saved.
*/
protected async saveAccountInformation(tokenResponse: IdentityTokenResponse): Promise<void> {
protected async saveAccountInformation(tokenResponse: IdentityTokenResponse): Promise<UserId> {
const accountInformation = await this.tokenService.decodeAccessToken(tokenResponse.accessToken);
const userId = accountInformation.sub;
// If you don't persist existing admin auth requests on login, they will get deleted.
const adminAuthRequest = await this.stateService.getAdminAuthRequest({ userId });
const vaultTimeoutAction = await this.stateService.getVaultTimeoutAction();
const vaultTimeout = await this.stateService.getVaultTimeout();
@@ -197,7 +195,6 @@ export abstract class LoginStrategy {
tokens: {
...new AccountTokens(),
},
adminAuthRequest: adminAuthRequest?.toJSON(),
}),
);
@@ -206,6 +203,7 @@ export abstract class LoginStrategy {
);
await this.billingAccountProfileStateService.setHasPremium(accountInformation.premium, false);
return userId as UserId;
}
protected async processTokenResponse(response: IdentityTokenResponse): Promise<AuthResult> {
@@ -228,7 +226,7 @@ export abstract class LoginStrategy {
}
// Must come before setting keys, user key needs email to update additional keys
await this.saveAccountInformation(response);
const userId = await this.saveAccountInformation(response);
if (response.twoFactorToken != null) {
// note: we can read email from access token b/c it was saved in saveAccountInformation
@@ -238,7 +236,7 @@ export abstract class LoginStrategy {
}
await this.setMasterKey(response);
await this.setUserKey(response);
await this.setUserKey(response, userId);
await this.setPrivateKey(response);
this.messagingService.send("loggedIn");
@@ -248,7 +246,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 setUserKey(response: IdentityTokenResponse): Promise<void>;
protected abstract setUserKey(response: IdentityTokenResponse, userId: UserId): Promise<void>;
protected abstract setPrivateKey(response: IdentityTokenResponse): Promise<void>;
// Old accounts used master key for encryption. We are forcing migrations but only need to

View File

@@ -25,6 +25,7 @@ import { StateService } from "@bitwarden/common/platform/abstractions/state.serv
import { HashPurpose } from "@bitwarden/common/platform/enums";
import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key";
import { PasswordStrengthServiceAbstraction } from "@bitwarden/common/tools/password-strength";
import { UserId } from "@bitwarden/common/types/guid";
import { MasterKey } from "@bitwarden/common/types/key";
import { LoginStrategyServiceAbstraction } from "../abstractions";
@@ -207,14 +208,16 @@ export class PasswordLoginStrategy extends LoginStrategy {
await this.masterPasswordService.setMasterKeyHash(localMasterKeyHash, userId);
}
protected override async setUserKey(response: IdentityTokenResponse): Promise<void> {
protected override async setUserKey(
response: IdentityTokenResponse,
userId: UserId,
): Promise<void> {
// If migration is required, we won't have a user key to set yet.
if (this.encryptionKeyMigrationRequired(response)) {
return;
}
await this.cryptoService.setMasterKeyEncryptedUserKey(response.key);
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);

View File

@@ -301,7 +301,7 @@ describe("SsoLoginStrategy", () => {
id: "1",
privateKey: "PRIVATE" as any,
} as AdminAuthRequestStorable;
stateService.getAdminAuthRequest.mockResolvedValue(
authRequestService.getAdminAuthRequest.mockResolvedValue(
new AdminAuthRequestStorable(adminAuthRequest),
);
});
@@ -364,7 +364,7 @@ describe("SsoLoginStrategy", () => {
await ssoLoginStrategy.logIn(credentials);
expect(stateService.setAdminAuthRequest).toHaveBeenCalledWith(null);
expect(authRequestService.clearAdminAuthRequest).toHaveBeenCalled();
expect(
authRequestService.setKeysAfterDecryptingSharedMasterKeyAndHash,
).not.toHaveBeenCalled();

View File

@@ -216,7 +216,10 @@ export class SsoLoginStrategy extends LoginStrategy {
// TODO: future passkey login strategy will need to support setting user key (decrypting via TDE or admin approval request)
// so might be worth moving this logic to a common place (base login strategy or a separate service?)
protected override async setUserKey(tokenResponse: IdentityTokenResponse): Promise<void> {
protected override async setUserKey(
tokenResponse: IdentityTokenResponse,
userId: UserId,
): Promise<void> {
const masterKeyEncryptedUserKey = tokenResponse.key;
// Note: masterKeyEncryptedUserKey is undefined for SSO JIT provisioned users
@@ -232,7 +235,7 @@ export class SsoLoginStrategy extends LoginStrategy {
// Note: TDE and key connector are mutually exclusive
if (userDecryptionOptions?.trustedDeviceOption) {
await this.trySetUserKeyWithApprovedAdminRequestIfExists();
await this.trySetUserKeyWithApprovedAdminRequestIfExists(userId);
const hasUserKey = await this.cryptoService.hasUserKey();
@@ -252,9 +255,9 @@ export class SsoLoginStrategy extends LoginStrategy {
// is responsible for deriving master key from MP entry and then decrypting the user key
}
private async trySetUserKeyWithApprovedAdminRequestIfExists(): Promise<void> {
private async trySetUserKeyWithApprovedAdminRequestIfExists(userId: UserId): Promise<void> {
// At this point a user could have an admin auth request that has been approved
const adminAuthReqStorable = await this.stateService.getAdminAuthRequest();
const adminAuthReqStorable = await this.authRequestService.getAdminAuthRequest(userId);
if (!adminAuthReqStorable) {
return;
@@ -268,7 +271,7 @@ export class SsoLoginStrategy extends LoginStrategy {
} catch (error) {
if (error instanceof ErrorResponse && error.statusCode === HttpStatusCode.NotFound) {
// if we get a 404, it means the auth request has been deleted so clear it from storage
await this.stateService.setAdminAuthRequest(null);
await this.authRequestService.clearAdminAuthRequest(userId);
}
// Always return on an error here as we don't want to block the user from logging in
@@ -295,12 +298,11 @@ export class SsoLoginStrategy extends LoginStrategy {
if (await this.cryptoService.hasUserKey()) {
// Now that we have a decrypted user key in memory, we can check if we
// need to establish trust on the current device
const userId = (await this.stateService.getUserId()) as UserId;
await this.deviceTrustCryptoService.trustDeviceIfRequired(userId);
// if we successfully decrypted the user key, we can delete the admin auth request out of state
// TODO: eventually we post and clean up DB as well once consumed on client
await this.stateService.setAdminAuthRequest(null);
await this.authRequestService.clearAdminAuthRequest(userId);
this.platformUtilsService.showToast("success", null, this.i18nService.t("loginApproved"));
}

View File

@@ -18,6 +18,7 @@ import { LogService } from "@bitwarden/common/platform/abstractions/log.service"
import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service";
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
import { StateService } from "@bitwarden/common/platform/abstractions/state.service";
import { UserId } from "@bitwarden/common/types/guid";
import { InternalUserDecryptionOptionsServiceAbstraction } from "../abstractions/user-decryption-options.service.abstraction";
import { UserApiLoginCredentials } from "../models/domain/login-credentials";
@@ -97,7 +98,10 @@ export class UserApiLoginStrategy extends LoginStrategy {
}
}
protected override async setUserKey(response: IdentityTokenResponse): Promise<void> {
protected override async setUserKey(
response: IdentityTokenResponse,
userId: UserId,
): Promise<void> {
await this.cryptoService.setMasterKeyEncryptedUserKey(response.key);
if (response.apiUseKeyConnector) {
@@ -116,8 +120,8 @@ export class UserApiLoginStrategy extends LoginStrategy {
);
}
protected async saveAccountInformation(tokenResponse: IdentityTokenResponse) {
await super.saveAccountInformation(tokenResponse);
protected async saveAccountInformation(tokenResponse: IdentityTokenResponse): Promise<UserId> {
const userId = await super.saveAccountInformation(tokenResponse);
const vaultTimeout = await this.stateService.getVaultTimeout();
const vaultTimeoutAction = await this.stateService.getVaultTimeoutAction();
@@ -134,6 +138,7 @@ export class UserApiLoginStrategy extends LoginStrategy {
vaultTimeoutAction as VaultTimeoutAction,
vaultTimeout,
);
return userId;
}
exportCache(): CacheData {

View File

@@ -17,6 +17,7 @@ import { MessagingService } from "@bitwarden/common/platform/abstractions/messag
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
import { StateService } from "@bitwarden/common/platform/abstractions/state.service";
import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key";
import { UserId } from "@bitwarden/common/types/guid";
import { UserKey } from "@bitwarden/common/types/key";
import { InternalUserDecryptionOptionsServiceAbstraction } from "../abstractions";
@@ -98,7 +99,7 @@ export class WebAuthnLoginStrategy extends LoginStrategy {
return Promise.resolve();
}
protected override async setUserKey(idTokenResponse: IdentityTokenResponse) {
protected override async setUserKey(idTokenResponse: IdentityTokenResponse, userId: UserId) {
const masterKeyEncryptedUserKey = idTokenResponse.key;
if (masterKeyEncryptedUserKey) {

View File

@@ -9,6 +9,7 @@ import { CryptoService } from "@bitwarden/common/platform/abstractions/crypto.se
import { Utils } from "@bitwarden/common/platform/misc/utils";
import { EncString } from "@bitwarden/common/platform/models/domain/enc-string";
import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key";
import { StateProvider } from "@bitwarden/common/platform/state";
import { FakeAccountService, mockAccountServiceWith } from "@bitwarden/common/spec";
import { UserId } from "@bitwarden/common/types/guid";
import { MasterKey, UserKey } from "@bitwarden/common/types/key";
@@ -18,6 +19,7 @@ import { AuthRequestService } from "./auth-request.service";
describe("AuthRequestService", () => {
let sut: AuthRequestService;
const stateProvider = mock<StateProvider>();
let accountService: FakeAccountService;
let masterPasswordService: FakeMasterPasswordService;
const appIdService = mock<AppIdService>();
@@ -38,6 +40,7 @@ describe("AuthRequestService", () => {
masterPasswordService,
cryptoService,
apiService,
stateProvider,
);
mockPrivateKey = new Uint8Array(64);
@@ -59,6 +62,31 @@ describe("AuthRequestService", () => {
});
});
describe("AcceptAuthRequests", () => {
it("returns an error when userId isn't provided", async () => {
await expect(sut.getAcceptAuthRequests(undefined)).rejects.toThrow("User ID is required");
await expect(sut.setAcceptAuthRequests(true, undefined)).rejects.toThrow(
"User ID is required",
);
});
});
describe("AdminAuthRequest", () => {
it("returns an error when userId isn't provided", async () => {
await expect(sut.getAdminAuthRequest(undefined)).rejects.toThrow("User ID is required");
await expect(sut.setAdminAuthRequest(undefined, undefined)).rejects.toThrow(
"User ID is required",
);
await expect(sut.clearAdminAuthRequest(undefined)).rejects.toThrow("User ID is required");
});
it("does not allow clearing from setAdminAuthRequest", async () => {
await expect(sut.setAdminAuthRequest(null, "USER_ID" as UserId)).rejects.toThrow(
"Auth request is required",
);
});
});
describe("approveOrDenyAuthRequest", () => {
beforeEach(() => {
cryptoService.rsaEncrypt.mockResolvedValue({

View File

@@ -1,8 +1,10 @@
import { firstValueFrom, Observable, Subject } from "rxjs";
import { Observable, Subject, firstValueFrom } from "rxjs";
import { Jsonify } from "type-fest";
import { ApiService } from "@bitwarden/common/abstractions/api.service";
import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { InternalMasterPasswordServiceAbstraction } from "@bitwarden/common/auth/abstractions/master-password.service.abstraction";
import { AdminAuthRequestStorable } from "@bitwarden/common/auth/models/domain/admin-auth-req-storable";
import { PasswordlessAuthRequest } from "@bitwarden/common/auth/models/request/passwordless-auth.request";
import { AuthRequestResponse } from "@bitwarden/common/auth/models/response/auth-request.response";
import { AuthRequestPushNotification } from "@bitwarden/common/models/response/notification.response";
@@ -10,10 +12,43 @@ import { AppIdService } from "@bitwarden/common/platform/abstractions/app-id.ser
import { CryptoService } from "@bitwarden/common/platform/abstractions/crypto.service";
import { Utils } from "@bitwarden/common/platform/misc/utils";
import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key";
import {
AUTH_REQUEST_DISK_LOCAL,
StateProvider,
UserKeyDefinition,
} from "@bitwarden/common/platform/state";
import { UserId } from "@bitwarden/common/types/guid";
import { MasterKey, UserKey } from "@bitwarden/common/types/key";
import { AuthRequestServiceAbstraction } from "../../abstractions/auth-request.service.abstraction";
/**
* Disk-local to maintain consistency between tabs (even though
* approvals are currently only available on desktop). We don't
* want to clear this on logout as it's a user preference.
*/
export const ACCEPT_AUTH_REQUESTS_KEY = new UserKeyDefinition<boolean>(
AUTH_REQUEST_DISK_LOCAL,
"acceptAuthRequests",
{
deserializer: (value) => value ?? false,
clearOn: [],
},
);
/**
* Disk-local to maintain consistency between tabs. We don't want to
* clear this on logout since admin auth requests are long-lived.
*/
export const ADMIN_AUTH_REQUEST_KEY = new UserKeyDefinition<Jsonify<AdminAuthRequestStorable>>(
AUTH_REQUEST_DISK_LOCAL,
"adminAuthRequest",
{
deserializer: (value) => value,
clearOn: [],
},
);
export class AuthRequestService implements AuthRequestServiceAbstraction {
private authRequestPushNotificationSubject = new Subject<string>();
authRequestPushNotification$: Observable<string>;
@@ -24,10 +59,61 @@ export class AuthRequestService implements AuthRequestServiceAbstraction {
private masterPasswordService: InternalMasterPasswordServiceAbstraction,
private cryptoService: CryptoService,
private apiService: ApiService,
private stateProvider: StateProvider,
) {
this.authRequestPushNotification$ = this.authRequestPushNotificationSubject.asObservable();
}
async getAcceptAuthRequests(userId: UserId): Promise<boolean> {
if (userId == null) {
throw new Error("User ID is required");
}
const value = await firstValueFrom(
this.stateProvider.getUser(userId, ACCEPT_AUTH_REQUESTS_KEY).state$,
);
return value;
}
async setAcceptAuthRequests(accept: boolean, userId: UserId): Promise<void> {
if (userId == null) {
throw new Error("User ID is required");
}
await this.stateProvider.setUserState(ACCEPT_AUTH_REQUESTS_KEY, accept, userId);
}
async getAdminAuthRequest(userId: UserId): Promise<AdminAuthRequestStorable | null> {
if (userId == null) {
throw new Error("User ID is required");
}
const authRequestSerialized = await firstValueFrom(
this.stateProvider.getUser(userId, ADMIN_AUTH_REQUEST_KEY).state$,
);
const adminAuthRequestStorable = AdminAuthRequestStorable.fromJSON(authRequestSerialized);
return adminAuthRequestStorable;
}
async setAdminAuthRequest(authRequest: AdminAuthRequestStorable, userId: UserId): Promise<void> {
if (userId == null) {
throw new Error("User ID is required");
}
if (authRequest == null) {
throw new Error("Auth request is required");
}
await this.stateProvider.setUserState(ADMIN_AUTH_REQUEST_KEY, authRequest.toJSON(), userId);
}
async clearAdminAuthRequest(userId: UserId): Promise<void> {
if (userId == null) {
throw new Error("User ID is required");
}
await this.stateProvider.setUserState(ADMIN_AUTH_REQUEST_KEY, null, userId);
}
async approveOrDenyAuthRequest(
approve: boolean,
authRequest: AuthRequestResponse,