Upstream/bitwarden-browser
1
0
Fork 0
mirror of https://github.com/bitwarden/browser.git synced 2024-12-21 16:18:28 +01:00
Code Issues Projects Releases Wiki Activity

[PM-5302] Refactor Passkey feature enable/disable logic (#7242)

Browse Source 
* feat: add missing tests for `isFido2FeatureEnabled`

* feat: add user logged in check

* chore: rewrite with cartesian product

* chore: remove test

The test was more complex than the actual function, removing.

* feat: add domain exclusion

* feat: add origin equal vault case

* chore: clean up the old code from `content-secript`

* feat: return early to avoid making api calls

* fix: prettier linting

* fix: incorrect logic inversion

---------

Co-authored-by: bnagawiecki <107435978+bnagawiecki@users.noreply.github.com>
Co-authored-by: SmithThe4th <gsmith@bitwarden.com>
This commit is contained in:
Andreas Coroiu 2024-01-02 19:42:16 +01:00 committed by GitHub
parent a682f2a0ef
commit 551d2c2441
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 66 additions and 89 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
apps/browser/src/background/runtime.background.ts
View File

@ -264,7 +264,7 @@ export default class RuntimeBackground {
this.abortManager.abort(msg.abortedRequestId);
break;
case "checkFido2FeatureEnabled":
return await this.main.fido2ClientService.isFido2FeatureEnabled();
return await this.main.fido2ClientService.isFido2FeatureEnabled(msg.hostname, msg.origin);
case "fido2RegisterCredentialRequest":
return await this.abortManager.runWithAbortController(
msg.requestId,

57
apps/browser/src/vault/fido2/content/content-script.ts
View File

@ -9,46 +9,16 @@ import { Messenger } from "./messaging/messenger";
function isFido2FeatureEnabled(): Promise<boolean> {
return new Promise((resolve) => {
chrome.runtime.sendMessage(
{ command: "checkFido2FeatureEnabled" },
{
command: "checkFido2FeatureEnabled",
hostname: window.location.hostname,
origin: window.location.origin,
},
(response: { result?: boolean }) => resolve(response.result),
);
});
}
async function getFromLocalStorage(keys: string | string[]): Promise<Record<string, any>> {
return new Promise((resolve) => {
chrome.storage.local.get(keys, (storage: Record<string, any>) => resolve(storage));
});
}
async function getActiveUserSettings() {
// TODO: This is code copied from `notification-bar.tsx`. We should refactor this into a shared function.
// Look up the active user id from storage
const activeUserIdKey = "activeUserId";
let activeUserId: string;
const activeUserStorageValue = await getFromLocalStorage(activeUserIdKey);
if (activeUserStorageValue[activeUserIdKey]) {
activeUserId = activeUserStorageValue[activeUserIdKey];
}
const settingsStorage = await getFromLocalStorage(activeUserId);
// Look up the user's settings from storage
return settingsStorage?.[activeUserId]?.settings;
}
async function isDomainExcluded(activeUserSettings: Record<string, any>) {
const excludedDomains = activeUserSettings?.neverDomains;
return excludedDomains && window.location.hostname in excludedDomains;
}
async function hasActiveUser() {
const activeUserIdKey = "activeUserId";
const activeUserStorageValue = await getFromLocalStorage(activeUserIdKey);
return activeUserStorageValue[activeUserIdKey] !== undefined;
}
function isSameOriginWithAncestors() {
try {
return window.self === window.top;
@ -56,11 +26,6 @@ function isSameOriginWithAncestors() {
return false;
}
}
async function isLocationBitwardenVault(activeUserSettings: Record<string, any>) {
return window.location.origin === activeUserSettings.serverConfig.environment.vault;
}
const messenger = Messenger.forDOMCommunication(window);
function injectPageScript() {
@ -156,17 +121,7 @@ function initializeFido2ContentScript() {
}
async function run() {
if (!(await hasActiveUser())) {
return;
}
const activeUserSettings = await getActiveUserSettings();
if (
activeUserSettings == null ||
!(await isFido2FeatureEnabled()) ||
(await isDomainExcluded(activeUserSettings)) ||
(await isLocationBitwardenVault(activeUserSettings))
) {
if (!(await isFido2FeatureEnabled())) {
return;
}

4
libs/common/src/vault/abstractions/fido2/fido2-client.service.abstraction.ts
View File

@ -14,6 +14,8 @@ export type UserVerification = "discouraged" | "preferred" | "required";
* and for returning the results of the latter operations to the Web Authentication API's callers.
*/
export abstract class Fido2ClientService {
isFido2FeatureEnabled: (hostname: string, origin: string) => Promise<boolean>;
/**
* 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
@ -42,8 +44,6 @@ export abstract class Fido2ClientService {
tab: chrome.tabs.Tab,
abortController?: AbortController,
) => Promise<AssertCredentialResult>;
isFido2FeatureEnabled: () => Promise<boolean>;
}
/**

25
libs/common/src/vault/services/fido2/fido2-client.service.spec.ts
View File

@ -1,4 +1,5 @@
import { mock, MockProxy } from "jest-mock-extended";
import { of } from "rxjs";
import { AuthService } from "../../../auth/abstractions/auth.service";
import { AuthenticationStatus } from "../../../auth/enums/authentication-status";
@ -23,6 +24,8 @@ import { Fido2Utils } from "./fido2-utils";
import { guidToRawFormat } from "./guid-utils";
const RpId = "bitwarden.com";
const Origin = "https://bitwarden.com";
const VaultUrl = "https://vault.bitwarden.com";
describe("FidoAuthenticatorService", () => {
let authenticator!: MockProxy<Fido2AuthenticatorService>;
@ -40,7 +43,9 @@ describe("FidoAuthenticatorService", () => {
client = new Fido2ClientService(authenticator, configService, authService, stateService);
configService.getFeatureFlag.mockResolvedValue(true);
configService.serverConfig$ = of({ environment: { vault: VaultUrl } } as any);
stateService.getEnablePasskeys.mockResolvedValue(true);
authService.getAuthStatus.mockResolvedValue(AuthenticationStatus.Unlocked);
tab = { id: 123, windowId: 456 } as chrome.tabs.Tab;
});
@ -249,6 +254,15 @@ describe("FidoAuthenticatorService", () => {
const rejects = expect(result).rejects;
await rejects.toThrow(FallbackRequestedError);
});
it("should throw FallbackRequestedError if origin equals the bitwarden vault", async () => {
const params = createParams({ origin: VaultUrl });
const result = async () => await client.createCredential(params, tab);
const rejects = expect(result).rejects;
await rejects.toThrow(FallbackRequestedError);
});
});
function createParams(params: Partial<CreateCredentialParams> = {}): CreateCredentialParams {
@ -420,6 +434,15 @@ describe("FidoAuthenticatorService", () => {
const rejects = expect(result).rejects;
await rejects.toThrow(FallbackRequestedError);
});
it("should throw FallbackRequestedError if origin equals the bitwarden vault", async () => {
const params = createParams({ origin: VaultUrl });
const result = async () => await client.assertCredential(params, tab);
const rejects = expect(result).rejects;
await rejects.toThrow(FallbackRequestedError);
});
});
describe("assert non-discoverable credential", () => {
@ -485,7 +508,7 @@ describe("FidoAuthenticatorService", () => {
return {
allowedCredentialIds: params.allowedCredentialIds ?? [],
challenge: params.challenge ?? Fido2Utils.bufferToString(randomBytes(16)),
origin: params.origin ?? "https://bitwarden.com",
origin: params.origin ?? Origin,
rpId: params.rpId ?? RpId,
timeout: params.timeout,
userVerification: params.userVerification,

67
libs/common/src/vault/services/fido2/fido2-client.service.ts
View File

@ -1,3 +1,4 @@
import { firstValueFrom } from "rxjs";
import { parse } from "tldts";
import { AuthService } from "../../../auth/abstractions/auth.service";
@ -45,12 +46,31 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction {
private logService?: LogService,
) {}
async isFido2FeatureEnabled(): Promise<boolean> {
async isFido2FeatureEnabled(hostname: string, origin: string): Promise<boolean> {
const userEnabledPasskeys = await this.stateService.getEnablePasskeys();
const isUserLoggedIn =
(await this.authService.getAuthStatus()) !== AuthenticationStatus.LoggedOut;
const neverDomains = await this.stateService.getNeverDomains();
const isExcludedDomain = neverDomains != null && hostname in neverDomains;
const serverConfig = await firstValueFrom(this.configService.serverConfig$);
const isOriginEqualBitwardenVault = origin === serverConfig.environment?.vault;
if (
!userEnabledPasskeys ||
!isUserLoggedIn ||
isExcludedDomain ||
isOriginEqualBitwardenVault
) {
return false;
}
const featureFlagEnabled = await this.configService.getFeatureFlag<boolean>(
FeatureFlag.Fido2VaultCredentials,
);
const userEnabledPasskeys = await this.stateService.getEnablePasskeys();
return featureFlagEnabled && userEnabledPasskeys;
return featureFlagEnabled;
}
async createCredential(
@ -58,20 +78,17 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction {
tab: chrome.tabs.Tab,
abortController = new AbortController(),
): Promise<CreateCredentialResult> {
const enableFido2VaultCredentials = await this.isFido2FeatureEnabled();
const parsedOrigin = parse(params.origin, { allowPrivateDomains: true });
const enableFido2VaultCredentials = await this.isFido2FeatureEnabled(
parsedOrigin.hostname,
params.origin,
);
if (!enableFido2VaultCredentials) {
this.logService?.warning(`[Fido2Client] Fido2VaultCredential is not enabled`);
throw new FallbackRequestedError();
}
const authStatus = await this.authService.getAuthStatus();
if (authStatus === AuthenticationStatus.LoggedOut) {
this.logService?.warning(`[Fido2Client] Fido2VaultCredential is not enabled`);
throw new FallbackRequestedError();
}
if (!params.sameOriginWithAncestors) {
this.logService?.warning(
`[Fido2Client] Invalid 'sameOriginWithAncestors' value: ${params.sameOriginWithAncestors}`,
@ -87,15 +104,7 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction {
throw new TypeError("Invalid 'user.id' length");
}
const parsedOrigin = parse(params.origin, { allowPrivateDomains: true });
params.rp.id = params.rp.id ?? parsedOrigin.hostname;
const neverDomains = await this.stateService.getNeverDomains();
if (neverDomains != null && parsedOrigin.hostname in neverDomains) {
this.logService?.warning(`[Fido2Client] Excluded domain`);
throw new FallbackRequestedError();
}
if (parsedOrigin.hostname == undefined || !params.origin.startsWith("https://")) {
this.logService?.warning(`[Fido2Client] Invalid https origin: ${params.origin}`);
throw new DOMException("'origin' is not a valid https origin", "SecurityError");
@ -210,29 +219,19 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction {
tab: chrome.tabs.Tab,
abortController = new AbortController(),
): Promise<AssertCredentialResult> {
const enableFido2VaultCredentials = await this.isFido2FeatureEnabled();
const parsedOrigin = parse(params.origin, { allowPrivateDomains: true });
const enableFido2VaultCredentials = await this.isFido2FeatureEnabled(
parsedOrigin.hostname,
params.origin,
);
if (!enableFido2VaultCredentials) {
this.logService?.warning(`[Fido2Client] Fido2VaultCredential is not enabled`);
throw new FallbackRequestedError();
}
const authStatus = await this.authService.getAuthStatus();
if (authStatus === AuthenticationStatus.LoggedOut) {
this.logService?.warning(`[Fido2Client] Fido2VaultCredential is not enabled`);
throw new FallbackRequestedError();
}
const parsedOrigin = parse(params.origin, { allowPrivateDomains: true });
params.rpId = params.rpId ?? parsedOrigin.hostname;
const neverDomains = await this.stateService.getNeverDomains();
if (neverDomains != null && parsedOrigin.hostname in neverDomains) {
this.logService?.warning(`[Fido2Client] Excluded domain`);
throw new FallbackRequestedError();
}
if (parsedOrigin.hostname == undefined || !params.origin.startsWith("https://")) {
this.logService?.warning(`[Fido2Client] Invalid https origin: ${params.origin}`);
throw new DOMException("'origin' is not a valid https origin", "SecurityError");