bitwarden/browser
1
0
Fork 0
mirror of https://github.com/bitwarden/browser synced 2025-12-10 13:23:34 +00:00
Code Issues Releases Wiki Activity

[PM-11419] Fix issues encountered with inline menu passkeys (#10892)

Browse Source 
* [PM-11419] Login items do not display after adding passkey

* [PM-11419] Login items do not display after adding passkey

* [PM-11419] Incorporating fixes for deleting a cipher from the inline menu as well as authenticating using passkeys

* [PM-11419] Fixing an issue where master password reprompt is ignored for a set passkey cipher

* [PM-11419] Fixing an issue where saving a passkey does not trigger a clearing of cached cipher values

* [PM-11419] Refactoring implementation

* [PM-11419] Ensuring that passkeys must be enabled in order for ciphers to appear

* [PM-11419] Adding an abort event from the active request manager

* [PM-11419] Adding an abort event from the active request manager

* [PM-11419] Working through jest tests within implementation

* [PM-11419] Fixing jest tests within Fido2ClientService and Fido2AuthenticatorService

* [PM-11419] Adding jest tests for added logic within OverlayBackground

* [PM-11419] Adding jest tests for added logic within OverlayBackground

* [PM-11419] Reworking how we handle assuming user presence when master password reprompt is required

* [PM-11419] Reworking how we handle assuming user presence when master password reprompt is required

* [PM-11419] Reworking how we handle assuming user presence when master password reprompt is required

* [PM-11419] Refactoring implementation

* [PM-11419] Incorporating suggestion for reporting failed passkey authentication from the inline menu

* [PM-11419] Reworking positioning of the abort controller that informs the background script of an error

* [PM-11419] Scoping down the behavior surrounding master password reprompt a bit more tightly

* [PM-11419] Reworking how we handle reacting to active fido2 requests to avoid ambiguity

* [PM-11419] Reworking how we handle reacting to active fido2 requests to avoid ambiguity

* [PM-11419] Adjusting implementation to ensure we clear any active requests when the passkeys setting is modified
This commit is contained in:
Cesar Gonzalez
2024-09-09 07:44:08 -05:00
committed by GitHub
parent 3af0590807
commit 2827d338ee
17 changed files with 288 additions and 84 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
libs/common/src/platform/abstractions/fido2/fido2-active-request-manager.abstraction.ts
View File

@@ -2,9 +2,22 @@ import { Observable, Subject } from "rxjs";
import { Fido2CredentialView } from "../../../vault/models/view/fido2-credential.view";
export const Fido2ActiveRequestEvents = {
Refresh: "refresh-fido2-active-request",
Abort: "abort-fido2-active-request",
Continue: "continue-fido2-active-request",
} as const;
type Fido2ActiveRequestEvent = typeof Fido2ActiveRequestEvents;
export type RequestResult =
| { type: Fido2ActiveRequestEvent["Refresh"] }
| { type: Fido2ActiveRequestEvent["Abort"] }
| { type: Fido2ActiveRequestEvent["Continue"]; credentialId: string };
export interface ActiveRequest {
credentials: Fido2CredentialView[];
subject: Subject<string>;
subject: Subject<RequestResult>;
}
export type RequestCollection = Readonly<{ [tabId: number]: ActiveRequest }>;
@@ -16,6 +29,7 @@ export abstract class Fido2ActiveRequestManager {
tabId: number,
credentials: Fido2CredentialView[],
abortController: AbortController,
) => Promise<string>;
) => Promise<RequestResult>;
removeActiveRequest: (tabId: number) => void;
removeAllActiveRequests: () => void;
}

8
libs/common/src/platform/abstractions/fido2/fido2-client.service.abstraction.ts
View File

@@ -1,7 +1,3 @@
import { Observable } from "rxjs";
import { Fido2CredentialView } from "../../../vault/models/view/fido2-credential.view";
export const UserRequestedFallbackAbortReason = "UserRequestedFallback";
export type UserVerification = "discouraged" | "preferred" | "required";
@@ -20,10 +16,6 @@ export type UserVerification = "discouraged" | "preferred" | "required";
export abstract class Fido2ClientService {
isFido2FeatureEnabled: (hostname: string, origin: string) => Promise<boolean>;
availableAutofillCredentials$: (tabId: number) => Observable<Fido2CredentialView[]>;
autofillCredential: (tabId: number, credentialId: string) => Promise<void>;
/**
* Allows WebAuthn Relying Party scripts to request the creation of a new public key credential source.
* For more information please see: https://www.w3.org/TR/webauthn-3/#sctn-createCredential

5
libs/common/src/platform/abstractions/fido2/fido2-user-interface.service.abstraction.ts
View File

@@ -45,6 +45,11 @@ export interface PickCredentialParams {
* Bypass the UI and assume that the user has already interacted with the authenticator.
*/
assumeUserPresence?: boolean;
/**
* Identifies whether a cipher requires a master password reprompt when getting a credential.
*/
masterPasswordRepromptRequired?: boolean;
}
/**

19
libs/common/src/platform/services/fido2/fido2-active-request-manager.ts
View File

@@ -14,6 +14,8 @@ import {
ActiveRequest,
RequestCollection,
Fido2ActiveRequestManager as Fido2ActiveRequestManagerAbstraction,
Fido2ActiveRequestEvents,
RequestResult,
} from "../../abstractions/fido2/fido2-active-request-manager.abstraction";
export class Fido2ActiveRequestManager implements Fido2ActiveRequestManagerAbstraction {
@@ -53,7 +55,7 @@ export class Fido2ActiveRequestManager implements Fido2ActiveRequestManagerAbstr
tabId: number,
credentials: Fido2CredentialView[],
abortController: AbortController,
): Promise<string> {
): Promise<RequestResult> {
const newRequest: ActiveRequest = {
credentials,
subject: new Subject(),
@@ -65,10 +67,10 @@ export class Fido2ActiveRequestManager implements Fido2ActiveRequestManagerAbstr
const abortListener = () => this.abortActiveRequest(tabId);
abortController.signal.addEventListener("abort", abortListener);
const credentialId = firstValueFrom(newRequest.subject);
const requestResult = firstValueFrom(newRequest.subject);
abortController.signal.removeEventListener("abort", abortListener);
return credentialId;
return requestResult;
}
/**
@@ -85,12 +87,23 @@ export class Fido2ActiveRequestManager implements Fido2ActiveRequestManagerAbstr
});
}
/**
* Removes and aborts all active requests.
*/
removeAllActiveRequests() {
Object.keys(this.activeRequests$.value).forEach((tabId) => {
this.abortActiveRequest(Number(tabId));
});
this.updateRequests(() => ({}));
}
/**
* Aborts the active request associated with a given tab id.
*
* @param tabId - The tab id to abort the active request for.
*/
private abortActiveRequest(tabId: number): void {
this.activeRequests$.value[tabId]?.subject.next({ type: Fido2ActiveRequestEvents.Abort });
this.activeRequests$.value[tabId]?.subject.error(
new DOMException("The operation either timed out or was not allowed.", "AbortError"),
);

3
libs/common/src/platform/services/fido2/fido2-authenticator.service.spec.ts
View File

@@ -576,6 +576,7 @@ describe("FidoAuthenticatorService", () => {
expect(userInterfaceSession.pickCredential).toHaveBeenCalledWith({
cipherIds: ciphers.map((c) => c.id),
userVerification: false,
masterPasswordRepromptRequired: false,
});
});
@@ -592,6 +593,7 @@ describe("FidoAuthenticatorService", () => {
expect(userInterfaceSession.pickCredential).toHaveBeenCalledWith({
cipherIds: [discoverableCiphers[0].id],
userVerification: false,
masterPasswordRepromptRequired: false,
});
});
@@ -609,6 +611,7 @@ describe("FidoAuthenticatorService", () => {
expect(userInterfaceSession.pickCredential).toHaveBeenCalledWith({
cipherIds: ciphers.map((c) => c.id),
userVerification,
masterPasswordRepromptRequired: false,
});
});
}

13
libs/common/src/platform/services/fido2/fido2-authenticator.service.ts
View File

@@ -160,6 +160,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
}
const reencrypted = await this.cipherService.encrypt(cipher, activeUserId);
await this.cipherService.updateWithServer(reencrypted);
await this.cipherService.clearCache(activeUserId);
credentialId = fido2Credential.credentialId;
} catch (error) {
this.logService?.error(
@@ -243,12 +244,18 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
}
let response = { cipherId: cipherOptions[0].id, userVerified: false };
const masterPasswordRepromptRequired = cipherOptions.some(
(cipher) => cipher.reprompt !== CipherRepromptType.None,
);
if (this.requiresUserVerificationPrompt(params, cipherOptions)) {
if (
this.requiresUserVerificationPrompt(params, cipherOptions, masterPasswordRepromptRequired)
) {
response = await userInterfaceSession.pickCredential({
cipherIds: cipherOptions.map((cipher) => cipher.id),
userVerification: params.requireUserVerification,
assumeUserPresence: params.assumeUserPresence,
masterPasswordRepromptRequired,
});
}
@@ -292,6 +299,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
);
const encrypted = await this.cipherService.encrypt(selectedCipher, activeUserId);
await this.cipherService.updateWithServer(encrypted);
await this.cipherService.clearCache(activeUserId);
}
const authenticatorData = await generateAuthData({
@@ -330,13 +338,14 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
private requiresUserVerificationPrompt(
params: Fido2AuthenticatorGetAssertionParams,
cipherOptions: CipherView[],
masterPasswordRepromptRequired: boolean,
): boolean {
return (
params.requireUserVerification ||
!params.assumeUserPresence ||
cipherOptions.length > 1 ||
cipherOptions.length === 0 ||
cipherOptions.some((cipher) => cipher.reprompt !== CipherRepromptType.None)
masterPasswordRepromptRequired
);
}

35
libs/common/src/platform/services/fido2/fido2-client.service.spec.ts
View File

@@ -6,11 +6,12 @@ import { AuthenticationStatus } from "../../../auth/enums/authentication-status"
import { DomainSettingsService } from "../../../autofill/services/domain-settings.service";
import { Utils } from "../../../platform/misc/utils";
import { VaultSettingsService } from "../../../vault/abstractions/vault-settings/vault-settings.service";
import { Fido2CredentialView } from "../../../vault/models/view/fido2-credential.view";
import { ConfigService } from "../../abstractions/config/config.service";
import {
ActiveRequest,
Fido2ActiveRequestEvents,
Fido2ActiveRequestManager,
RequestResult,
} from "../../abstractions/fido2/fido2-active-request-manager.abstraction";
import {
Fido2AuthenticatorError,
@@ -56,7 +57,10 @@ describe("FidoAuthenticatorService", () => {
domainSettingsService = mock<DomainSettingsService>();
taskSchedulerService = mock<TaskSchedulerService>();
activeRequest = mock<ActiveRequest>({
subject: new BehaviorSubject<string>(""),
subject: new BehaviorSubject<RequestResult>({
type: Fido2ActiveRequestEvents.Continue,
credentialId: "",
}),
});
requestManager = mock<Fido2ActiveRequestManager>({
getActiveRequest$: (tabId: number) => new BehaviorSubject(activeRequest),
@@ -615,7 +619,10 @@ describe("FidoAuthenticatorService", () => {
});
beforeEach(() => {
requestManager.newActiveRequest.mockResolvedValue(crypto.randomUUID());
requestManager.newActiveRequest.mockResolvedValue({
type: Fido2ActiveRequestEvents.Continue,
credentialId: crypto.randomUUID(),
});
authenticator.getAssertion.mockResolvedValue(createAuthenticatorAssertResult());
});
@@ -676,28 +683,6 @@ describe("FidoAuthenticatorService", () => {
};
}
});
describe("autofill of credentials through the active request manager", () => {
it("returns an observable that updates with an array of the credentials for active Fido2 requests", async () => {
const activeRequestCredentials = mock<Fido2CredentialView>();
activeRequest.credentials = [activeRequestCredentials];
const observable = client.availableAutofillCredentials$(tab.id);
observable.subscribe((credentials) => {
expect(credentials).toEqual([activeRequestCredentials]);
});
});
it("triggers the logic of the next behavior subject of an active request", async () => {
const activeRequestCredentials = mock<Fido2CredentialView>();
activeRequest.credentials = [activeRequestCredentials];
jest.spyOn(activeRequest.subject, "next");
await client.autofillCredential(tab.id, activeRequestCredentials.credentialId);
expect(activeRequest.subject.next).toHaveBeenCalled();
});
});
});
/** This is a fake function that always returns the same byte sequence */

34
libs/common/src/platform/services/fido2/fido2-client.service.ts
View File

@@ -1,13 +1,15 @@
import { firstValueFrom, map, Observable, Subscription } from "rxjs";
import { firstValueFrom, Subscription } from "rxjs";
import { parse } from "tldts";
import { AuthService } from "../../../auth/abstractions/auth.service";
import { AuthenticationStatus } from "../../../auth/enums/authentication-status";
import { DomainSettingsService } from "../../../autofill/services/domain-settings.service";
import { VaultSettingsService } from "../../../vault/abstractions/vault-settings/vault-settings.service";
import { Fido2CredentialView } from "../../../vault/models/view/fido2-credential.view";
import { ConfigService } from "../../abstractions/config/config.service";
import { Fido2ActiveRequestManager } from "../../abstractions/fido2/fido2-active-request-manager.abstraction";
import {
Fido2ActiveRequestEvents,
Fido2ActiveRequestManager,
} from "../../abstractions/fido2/fido2-active-request-manager.abstraction";
import {
Fido2AuthenticatorError,
Fido2AuthenticatorErrorCode,
@@ -73,17 +75,6 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction {
);
}
availableAutofillCredentials$(tabId: number): Observable<Fido2CredentialView[]> {
return this.requestManager
.getActiveRequest$(tabId)
.pipe(map((request) => request?.credentials ?? []));
}
async autofillCredential(tabId: number, credentialId: string) {
const request = this.requestManager.getActiveRequest(tabId);
request.subject.next(credentialId);
}
async isFido2FeatureEnabled(hostname: string, origin: string): Promise<boolean> {
const isUserLoggedIn =
(await this.authService.getAuthStatus()) !== AuthenticationStatus.LoggedOut;
@@ -385,12 +376,23 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction {
this.logService?.info(
`[Fido2Client] started mediated request, available credentials: ${availableCredentials.length}`,
);
const credentialId = await this.requestManager.newActiveRequest(
const requestResult = await this.requestManager.newActiveRequest(
tab.id,
availableCredentials,
abortController,
);
params.allowedCredentialIds = [Fido2Utils.bufferToString(guidToRawFormat(credentialId))];
if (requestResult.type === Fido2ActiveRequestEvents.Refresh) {
continue;
}
if (requestResult.type === Fido2ActiveRequestEvents.Abort) {
break;
}
params.allowedCredentialIds = [
Fido2Utils.bufferToString(guidToRawFormat(requestResult.credentialId)),
];
assumeUserPresence = true;
const clientDataHash = await crypto.subtle.digest({ name: "SHA-256" }, clientDataJSONBytes);