From 11d340bc97e7432f23aea7c181a977500d43cb3c Mon Sep 17 00:00:00 2001
From: Andreas Coroiu <andreas.coroiu@gmail.com>
Date: Wed, 5 Apr 2023 11:38:32 +0200
Subject: [PATCH] [EC-598] feat: add initial implementation of UI sessions
---
.../browser-fido2-user-interface.service.ts | 73 +++++++++++++++++++
...ido2-user-interface.service.abstraction.ts | 23 ++++++
.../fido2-authenticator.service.spec.ts | 66 ++++++++++-------
.../services/fido2-authenticator.service.ts | 15 ++--
.../webauthn/services/fido2-client.service.ts | 2 +
5 files changed, 146 insertions(+), 33 deletions(-)
diff --git a/apps/browser/src/services/fido2/browser-fido2-user-interface.service.ts b/apps/browser/src/services/fido2/browser-fido2-user-interface.service.ts
index c378cf1ada..b570c4980a 100644
--- a/apps/browser/src/services/fido2/browser-fido2-user-interface.service.ts
+++ b/apps/browser/src/services/fido2/browser-fido2-user-interface.service.ts
@@ -3,6 +3,7 @@ import { filter, first, lastValueFrom, Observable, Subject, takeUntil } from "rx
import { Utils } from "@bitwarden/common/misc/utils";
import {
Fido2UserInterfaceService as Fido2UserInterfaceServiceAbstraction,
+ Fido2UserInterfaceSession,
NewCredentialParams,
} from "@bitwarden/common/webauthn/abstractions/fido2-user-interface.service.abstraction";
@@ -87,6 +88,10 @@ export class BrowserFido2UserInterfaceService implements Fido2UserInterfaceServi
constructor(private popupUtilsService: PopupUtilsService) {}
+ async newSession(abortController?: AbortController): Promise<Fido2UserInterfaceSession> {
+ return await BrowserFido2UserInterfaceSession.create(this, abortController);
+ }
+
async confirmCredential(
cipherId: string,
abortController = new AbortController()
@@ -265,3 +270,71 @@ export class BrowserFido2UserInterfaceService implements Fido2UserInterfaceServi
return setTimeout(() => abortController.abort());
}
}
+
+export class BrowserFido2UserInterfaceSession implements Fido2UserInterfaceSession {
+ static async create(
+ parentService: BrowserFido2UserInterfaceService,
+ abortController?: AbortController
+ ): Promise<BrowserFido2UserInterfaceSession> {
+ return new BrowserFido2UserInterfaceSession(parentService, abortController);
+ }
+
+ readonly abortListener: () => void;
+
+ private constructor(
+ private readonly parentService: BrowserFido2UserInterfaceService,
+ readonly abortController = new AbortController(),
+ readonly sessionId = Utils.newGuid()
+ ) {
+ this.abortListener = () => this.abort();
+ abortController.signal.addEventListener("abort", this.abortListener);
+ }
+
+ fallbackRequested = false;
+
+ get aborted() {
+ return this.abortController.signal.aborted;
+ }
+
+ confirmCredential(cipherId: string, abortController?: AbortController): Promise<boolean> {
+ return this.parentService.confirmCredential(cipherId, this.abortController);
+ }
+
+ pickCredential(cipherIds: string[], abortController?: AbortController): Promise<string> {
+ return this.parentService.pickCredential(cipherIds, this.abortController);
+ }
+
+ confirmNewCredential(
+ params: NewCredentialParams,
+ abortController?: AbortController
+ ): Promise<boolean> {
+ return this.parentService.confirmNewCredential(params, this.abortController);
+ }
+
+ confirmNewNonDiscoverableCredential(
+ params: NewCredentialParams,
+ abortController?: AbortController
+ ): Promise<string> {
+ return this.parentService.confirmNewNonDiscoverableCredential(params, this.abortController);
+ }
+
+ informExcludedCredential(
+ existingCipherIds: string[],
+ newCredential: NewCredentialParams,
+ abortController?: AbortController
+ ): Promise<void> {
+ return this.parentService.informExcludedCredential(
+ existingCipherIds,
+ newCredential,
+ this.abortController
+ );
+ }
+
+ private abort() {
+ this.close();
+ }
+
+ private close() {
+ this.abortController.signal.removeEventListener("abort", this.abortListener);
+ }
+}
diff --git a/libs/common/src/webauthn/abstractions/fido2-user-interface.service.abstraction.ts b/libs/common/src/webauthn/abstractions/fido2-user-interface.service.abstraction.ts
index 2e1507349a..8313ec5102 100644
--- a/libs/common/src/webauthn/abstractions/fido2-user-interface.service.abstraction.ts
+++ b/libs/common/src/webauthn/abstractions/fido2-user-interface.service.abstraction.ts
@@ -4,6 +4,29 @@ export interface NewCredentialParams {
}
export abstract class Fido2UserInterfaceService {
+ newSession: (abortController?: AbortController) => Promise<Fido2UserInterfaceSession>;
+
+ // confirmCredential: (cipherId: string, abortController?: AbortController) => Promise<boolean>;
+ // pickCredential: (cipherIds: string[], abortController?: AbortController) => Promise<string>;
+ // confirmNewCredential: (
+ // params: NewCredentialParams,
+ // abortController?: AbortController
+ // ) => Promise<boolean>;
+ // confirmNewNonDiscoverableCredential: (
+ // params: NewCredentialParams,
+ // abortController?: AbortController
+ // ) => Promise<string | undefined>;
+ // informExcludedCredential: (
+ // existingCipherIds: string[],
+ // newCredential: NewCredentialParams,
+ // abortController?: AbortController
+ // ) => Promise<void>;
+}
+
+export abstract class Fido2UserInterfaceSession {
+ fallbackRequested = false;
+ aborted = false;
+
confirmCredential: (cipherId: string, abortController?: AbortController) => Promise<boolean>;
pickCredential: (cipherIds: string[], abortController?: AbortController) => Promise<string>;
confirmNewCredential: (
diff --git a/libs/common/src/webauthn/services/fido2-authenticator.service.spec.ts b/libs/common/src/webauthn/services/fido2-authenticator.service.spec.ts
index 4264cccf34..70635817f9 100644
--- a/libs/common/src/webauthn/services/fido2-authenticator.service.spec.ts
+++ b/libs/common/src/webauthn/services/fido2-authenticator.service.spec.ts
@@ -16,6 +16,7 @@ import {
} from "../abstractions/fido2-authenticator.service.abstraction";
import {
Fido2UserInterfaceService,
+ Fido2UserInterfaceSession,
NewCredentialParams,
} from "../abstractions/fido2-user-interface.service.abstraction";
import { Fido2Utils } from "../abstractions/fido2-utils";
@@ -28,11 +29,14 @@ const RpId = "bitwarden.com";
describe("FidoAuthenticatorService", () => {
let cipherService!: MockProxy<CipherService>;
let userInterface!: MockProxy<Fido2UserInterfaceService>;
+ let userInterfaceSession!: MockProxy<Fido2UserInterfaceSession>;
let authenticator!: Fido2AuthenticatorService;
beforeEach(async () => {
cipherService = mock<CipherService>();
userInterface = mock<Fido2UserInterfaceService>();
+ userInterfaceSession = mock<Fido2UserInterfaceSession>();
+ userInterface.newSession.mockResolvedValue(userInterfaceSession);
authenticator = new Fido2AuthenticatorService(cipherService, userInterface);
});
@@ -77,7 +81,7 @@ describe("FidoAuthenticatorService", () => {
});
it("should not request confirmation from user", async () => {
- userInterface.confirmNewCredential.mockResolvedValue(true);
+ userInterfaceSession.confirmNewCredential.mockResolvedValue(true);
const invalidParams = await createInvalidParams();
for (const p of Object.values(invalidParams)) {
@@ -86,7 +90,7 @@ describe("FidoAuthenticatorService", () => {
// eslint-disable-next-line no-empty
} catch {}
}
- expect(userInterface.confirmNewCredential).not.toHaveBeenCalled();
+ expect(userInterfaceSession.confirmNewCredential).not.toHaveBeenCalled();
});
});
@@ -120,19 +124,19 @@ describe("FidoAuthenticatorService", () => {
* Deviation: Consent is not asked and the user is simply informed of the situation.
**/
it("should inform user", async () => {
- userInterface.informExcludedCredential.mockResolvedValue();
+ userInterfaceSession.informExcludedCredential.mockResolvedValue();
try {
await authenticator.makeCredential(params);
// eslint-disable-next-line no-empty
} catch {}
- expect(userInterface.informExcludedCredential).toHaveBeenCalled();
+ expect(userInterfaceSession.informExcludedCredential).toHaveBeenCalled();
});
/** Spec: return an error code equivalent to "NotAllowedError" and terminate the operation. */
it("should throw error", async () => {
- userInterface.informExcludedCredential.mockResolvedValue();
+ userInterfaceSession.informExcludedCredential.mockResolvedValue();
const result = async () => await authenticator.makeCredential(params);
@@ -140,7 +144,7 @@ describe("FidoAuthenticatorService", () => {
});
it("should not inform user of duplication when input data does not pass checks", async () => {
- userInterface.informExcludedCredential.mockResolvedValue();
+ userInterfaceSession.informExcludedCredential.mockResolvedValue();
const invalidParams = await createInvalidParams();
for (const p of Object.values(invalidParams)) {
@@ -149,7 +153,7 @@ describe("FidoAuthenticatorService", () => {
// eslint-disable-next-line no-empty
} catch {}
}
- expect(userInterface.informExcludedCredential).not.toHaveBeenCalled();
+ expect(userInterfaceSession.informExcludedCredential).not.toHaveBeenCalled();
});
it.todo(
@@ -181,19 +185,19 @@ describe("FidoAuthenticatorService", () => {
* Deviation: Consent is not asked and the user is simply informed of the situation.
**/
it("should inform user", async () => {
- userInterface.informExcludedCredential.mockResolvedValue();
+ userInterfaceSession.informExcludedCredential.mockResolvedValue();
try {
await authenticator.makeCredential(params);
// eslint-disable-next-line no-empty
} catch {}
- expect(userInterface.informExcludedCredential).toHaveBeenCalled();
+ expect(userInterfaceSession.informExcludedCredential).toHaveBeenCalled();
});
/** Spec: return an error code equivalent to "NotAllowedError" and terminate the operation. */
it("should throw error", async () => {
- userInterface.informExcludedCredential.mockResolvedValue();
+ userInterfaceSession.informExcludedCredential.mockResolvedValue();
const result = async () => await authenticator.makeCredential(params);
@@ -201,7 +205,7 @@ describe("FidoAuthenticatorService", () => {
});
it("should not inform user of duplication when input data does not pass checks", async () => {
- userInterface.informExcludedCredential.mockResolvedValue();
+ userInterfaceSession.informExcludedCredential.mockResolvedValue();
const invalidParams = await createInvalidParams();
for (const p of Object.values(invalidParams)) {
@@ -210,7 +214,7 @@ describe("FidoAuthenticatorService", () => {
// eslint-disable-next-line no-empty
} catch {}
}
- expect(userInterface.informExcludedCredential).not.toHaveBeenCalled();
+ expect(userInterfaceSession.informExcludedCredential).not.toHaveBeenCalled();
});
it.todo(
@@ -231,7 +235,7 @@ describe("FidoAuthenticatorService", () => {
* Deviation: Only `rpEntity.name` and `userEntity.name` is shown.
* */
it("should request confirmation from user", async () => {
- userInterface.confirmNewCredential.mockResolvedValue(true);
+ userInterfaceSession.confirmNewCredential.mockResolvedValue(true);
cipherService.encrypt.mockResolvedValue({} as unknown as Cipher);
cipherService.createWithServer.mockImplementation(async (cipher) => {
cipher.id = Utils.newGuid();
@@ -240,7 +244,7 @@ describe("FidoAuthenticatorService", () => {
await authenticator.makeCredential(params, new AbortController());
- expect(userInterface.confirmNewCredential).toHaveBeenCalledWith(
+ expect(userInterfaceSession.confirmNewCredential).toHaveBeenCalledWith(
{
credentialName: params.rpEntity.name,
userName: params.userEntity.displayName,
@@ -251,7 +255,7 @@ describe("FidoAuthenticatorService", () => {
it("should save credential to vault if request confirmed by user", async () => {
const encryptedCipher = {};
- userInterface.confirmNewCredential.mockResolvedValue(true);
+ userInterfaceSession.confirmNewCredential.mockResolvedValue(true);
cipherService.encrypt.mockResolvedValue(encryptedCipher as unknown as Cipher);
cipherService.createWithServer.mockImplementation(async (cipher) => {
cipher.id = Utils.newGuid();
@@ -284,7 +288,7 @@ describe("FidoAuthenticatorService", () => {
/** Spec: If the user does not consent or if user verification fails, return an error code equivalent to "NotAllowedError" and terminate the operation. */
it("should throw error if user denies creation request", async () => {
- userInterface.confirmNewCredential.mockResolvedValue(false);
+ userInterfaceSession.confirmNewCredential.mockResolvedValue(false);
const result = async () => await authenticator.makeCredential(params);
@@ -294,7 +298,7 @@ describe("FidoAuthenticatorService", () => {
/** Spec: If any error occurred while creating the new credential object, return an error code equivalent to "UnknownError" and terminate the operation. */
it("should throw unkown error if creation fails", async () => {
const encryptedCipher = {};
- userInterface.confirmNewCredential.mockResolvedValue(true);
+ userInterfaceSession.confirmNewCredential.mockResolvedValue(true);
cipherService.encrypt.mockResolvedValue(encryptedCipher as unknown as Cipher);
cipherService.createWithServer.mockRejectedValue(new Error("Internal error"));
@@ -322,11 +326,13 @@ describe("FidoAuthenticatorService", () => {
* Deviation: Only `rpEntity.name` and `userEntity.name` is shown.
* */
it("should request confirmation from user", async () => {
- userInterface.confirmNewNonDiscoverableCredential.mockResolvedValue(existingCipher.id);
+ userInterfaceSession.confirmNewNonDiscoverableCredential.mockResolvedValue(
+ existingCipher.id
+ );
await authenticator.makeCredential(params, new AbortController());
- expect(userInterface.confirmNewNonDiscoverableCredential).toHaveBeenCalledWith(
+ expect(userInterfaceSession.confirmNewNonDiscoverableCredential).toHaveBeenCalledWith(
{
credentialName: params.rpEntity.name,
userName: params.userEntity.displayName,
@@ -337,7 +343,9 @@ describe("FidoAuthenticatorService", () => {
it("should save credential to vault if request confirmed by user", async () => {
const encryptedCipher = Symbol();
- userInterface.confirmNewNonDiscoverableCredential.mockResolvedValue(existingCipher.id);
+ userInterfaceSession.confirmNewNonDiscoverableCredential.mockResolvedValue(
+ existingCipher.id
+ );
cipherService.encrypt.mockResolvedValue(encryptedCipher as unknown as Cipher);
await authenticator.makeCredential(params);
@@ -368,7 +376,7 @@ describe("FidoAuthenticatorService", () => {
/** Spec: If the user does not consent or if user verification fails, return an error code equivalent to "NotAllowedError" and terminate the operation. */
it("should throw error if user denies creation request", async () => {
- userInterface.confirmNewNonDiscoverableCredential.mockResolvedValue(undefined);
+ userInterfaceSession.confirmNewNonDiscoverableCredential.mockResolvedValue(undefined);
const params = await createParams();
const result = async () => await authenticator.makeCredential(params);
@@ -379,7 +387,9 @@ describe("FidoAuthenticatorService", () => {
/** Spec: If any error occurred while creating the new credential object, return an error code equivalent to "UnknownError" and terminate the operation. */
it("should throw unkown error if creation fails", async () => {
const encryptedCipher = Symbol();
- userInterface.confirmNewNonDiscoverableCredential.mockResolvedValue(existingCipher.id);
+ userInterfaceSession.confirmNewNonDiscoverableCredential.mockResolvedValue(
+ existingCipher.id
+ );
cipherService.encrypt.mockResolvedValue(encryptedCipher as unknown as Cipher);
cipherService.updateWithServer.mockRejectedValue(new Error("Internal error"));
@@ -408,8 +418,8 @@ describe("FidoAuthenticatorService", () => {
beforeEach(async () => {
const cipher = createCipherView({ id: cipherId, type: CipherType.Login });
params = await createParams({ requireResidentKey });
- userInterface.confirmNewNonDiscoverableCredential.mockResolvedValue(cipherId);
- userInterface.confirmNewCredential.mockResolvedValue(true);
+ userInterfaceSession.confirmNewNonDiscoverableCredential.mockResolvedValue(cipherId);
+ userInterfaceSession.confirmNewCredential.mockResolvedValue(true);
cipherService.get.mockImplementation(async (cipherId) =>
cipherId === cipher.id ? ({ decrypt: () => cipher } as any) : undefined
);
@@ -625,16 +635,16 @@ describe("FidoAuthenticatorService", () => {
/** Spec: Prompt the user to select a public key credential source selectedCredential from credentialOptions. */
it("should request confirmation from the user", async () => {
- userInterface.pickCredential.mockResolvedValue(ciphers[0].id);
+ userInterfaceSession.pickCredential.mockResolvedValue(ciphers[0].id);
await authenticator.getAssertion(params);
- expect(userInterface.pickCredential).toHaveBeenCalledWith(ciphers.map((c) => c.id));
+ expect(userInterfaceSession.pickCredential).toHaveBeenCalledWith(ciphers.map((c) => c.id));
});
/** Spec: If the user does not consent, return an error code equivalent to "NotAllowedError" and terminate the operation. */
it("should throw error", async () => {
- userInterface.pickCredential.mockResolvedValue(undefined);
+ userInterfaceSession.pickCredential.mockResolvedValue(undefined);
const result = async () => await authenticator.getAssertion(params);
@@ -690,7 +700,7 @@ describe("FidoAuthenticatorService", () => {
});
}
cipherService.getAllDecrypted.mockResolvedValue(ciphers);
- userInterface.pickCredential.mockResolvedValue(ciphers[0].id);
+ userInterfaceSession.pickCredential.mockResolvedValue(ciphers[0].id);
});
/** Spec: Increment the credential associated signature counter */
diff --git a/libs/common/src/webauthn/services/fido2-authenticator.service.ts b/libs/common/src/webauthn/services/fido2-authenticator.service.ts
index 745b9570cb..daea9a440e 100644
--- a/libs/common/src/webauthn/services/fido2-authenticator.service.ts
+++ b/libs/common/src/webauthn/services/fido2-authenticator.service.ts
@@ -41,6 +41,8 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
params: Fido2AuthenticatorMakeCredentialsParams,
abortController?: AbortController
): Promise<Fido2AuthenticatorMakeCredentialResult> {
+ const userInterfaceSession = await this.userInterface.newSession(abortController);
+
if (params.credTypesAndPubKeyAlgs.every((p) => p.alg !== Fido2AlgorithmIdentifier.ES256)) {
throw new Fido2AutenticatorError(Fido2AutenticatorErrorCode.NotSupported);
}
@@ -62,7 +64,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
const isExcluded = await this.vaultContainsCredentials(params.excludeCredentialDescriptorList);
if (isExcluded) {
- await this.userInterface.informExcludedCredential(
+ await userInterfaceSession.informExcludedCredential(
[Utils.guidToStandardFormat(params.excludeCredentialDescriptorList[0].id)],
{
credentialName: params.rpEntity.name,
@@ -78,7 +80,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
let fido2Key: Fido2KeyView;
let keyPair: CryptoKeyPair;
if (params.requireResidentKey) {
- const userVerification = await this.userInterface.confirmNewCredential(
+ const userVerification = await userInterfaceSession.confirmNewCredential(
{
credentialName: params.rpEntity.name,
userName: params.userEntity.displayName,
@@ -104,7 +106,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
throw new Fido2AutenticatorError(Fido2AutenticatorErrorCode.Unknown);
}
} else {
- const cipherId = await this.userInterface.confirmNewNonDiscoverableCredential(
+ const cipherId = await userInterfaceSession.confirmNewNonDiscoverableCredential(
{
credentialName: params.rpEntity.name,
userName: params.userEntity.displayName,
@@ -157,8 +159,11 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
}
async getAssertion(
- params: Fido2AuthenticatorGetAssertionParams
+ params: Fido2AuthenticatorGetAssertionParams,
+ abortController?: AbortController
): Promise<Fido2AuthenticatorGetAssertionResult> {
+ const userInterfaceSession = await this.userInterface.newSession(abortController);
+
if (
params.requireUserVerification != undefined &&
typeof params.requireUserVerification !== "boolean"
@@ -186,7 +191,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
throw new Fido2AutenticatorError(Fido2AutenticatorErrorCode.NotAllowed);
}
- const selectedCipherId = await this.userInterface.pickCredential(
+ const selectedCipherId = await userInterfaceSession.pickCredential(
cipherOptions.map((cipher) => cipher.id)
);
const selectedCipher = cipherOptions.find((c) => c.id === selectedCipherId);
diff --git a/libs/common/src/webauthn/services/fido2-client.service.ts b/libs/common/src/webauthn/services/fido2-client.service.ts
index 2ed13a05f7..49718c82ba 100644
--- a/libs/common/src/webauthn/services/fido2-client.service.ts
+++ b/libs/common/src/webauthn/services/fido2-client.service.ts
@@ -124,9 +124,11 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction {
}
throw new DOMException(undefined, "NotAllowedError");
}
+
if (abortController.signal.aborted) {
throw new DOMException(undefined, "AbortError");
}
+
clearTimeout(timeout);
return {
credentialId: Fido2Utils.bufferToString(makeCredentialResult.credentialId),