1
0
mirror of https://github.com/bitwarden/browser synced 2026-02-21 11:54:02 +00:00

[PM-3807] Store all passkeys as login cipher type (#6255)

* [PM-3807] feat: add `discoverable` property to fido2keys

* [PM-3807] feat: assign discoverable property during creation

* [PM-3807] feat: save discoverable field to server

* [PM-3807] feat: filter credentials by rpId AND discoverable

* [PM-3807] chore: remove discoverable tests which are no longer needed

* [PM-3807] chore: remove all logic for handling standalone Fido2Key

View and components will be cleaned up as part of UI tickets

* [PM-3807] fix: add missing discoverable property handling to tests
This commit is contained in:
Andreas Coroiu
2023-09-12 15:32:02 +02:00
committed by GitHub
parent 539d379c43
commit d4822b1083
15 changed files with 84 additions and 183 deletions

View File

@@ -81,7 +81,7 @@ export interface Fido2AuthenticatorMakeCredentialsParams {
};
/** A Boolean value that indicates that individually-identifying attestation MAY be returned by the authenticator. */
enterpriseAttestationPossible?: boolean; // Ignored by bitwarden at the moment
/** The effective resident key requirement for credential creation, a Boolean value determined by the client. */
/** The effective resident key requirement for credential creation, a Boolean value determined by the client. Resident is synonymous with discoverable. */
requireResidentKey: boolean;
requireUserVerification: boolean;
/** Forwarded to user interface */

View File

@@ -11,6 +11,7 @@ export class Fido2KeyApi extends BaseResponse {
counter: string;
rpName: string;
userDisplayName: string;
discoverable: string;
constructor(data: any = null) {
super(data);
@@ -28,5 +29,6 @@ export class Fido2KeyApi extends BaseResponse {
this.counter = this.getResponseProperty("Counter");
this.rpName = this.getResponseProperty("RpName");
this.userDisplayName = this.getResponseProperty("UserDisplayName");
this.discoverable = this.getResponseProperty("Discoverable");
}
}

View File

@@ -4,7 +4,6 @@ import { CipherResponse } from "../response/cipher.response";
import { AttachmentData } from "./attachment.data";
import { CardData } from "./card.data";
import { Fido2KeyData } from "./fido2-key.data";
import { FieldData } from "./field.data";
import { IdentityData } from "./identity.data";
import { LoginData } from "./login.data";
@@ -27,7 +26,6 @@ export class CipherData {
secureNote?: SecureNoteData;
card?: CardData;
identity?: IdentityData;
fido2Key?: Fido2KeyData;
fields?: FieldData[];
attachments?: AttachmentData[];
passwordHistory?: PasswordHistoryData[];
@@ -60,8 +58,6 @@ export class CipherData {
switch (this.type) {
case CipherType.Login:
this.login = new LoginData(response.login);
this.fido2Key =
response.fido2Key != undefined ? new Fido2KeyData(response.fido2Key) : undefined;
break;
case CipherType.SecureNote:
this.secureNote = new SecureNoteData(response.secureNote);
@@ -72,9 +68,6 @@ export class CipherData {
case CipherType.Identity:
this.identity = new IdentityData(response.identity);
break;
case CipherType.Fido2Key:
this.fido2Key = new Fido2KeyData(response.fido2Key);
break;
default:
break;
}

View File

@@ -11,6 +11,7 @@ export class Fido2KeyData {
counter: string;
rpName: string;
userDisplayName: string;
discoverable: string;
constructor(data?: Fido2KeyApi) {
if (data == null) {
@@ -27,5 +28,6 @@ export class Fido2KeyData {
this.counter = data.counter;
this.rpName = data.rpName;
this.userDisplayName = data.userDisplayName;
this.discoverable = data.discoverable;
}
}

View File

@@ -13,7 +13,6 @@ import { CipherView } from "../view/cipher.view";
import { Attachment } from "./attachment";
import { Card } from "./card";
import { Fido2Key } from "./fido2-key";
import { Field } from "./field";
import { Identity } from "./identity";
import { Login } from "./login";
@@ -39,7 +38,6 @@ export class Cipher extends Domain implements Decryptable<CipherView> {
identity: Identity;
card: Card;
secureNote: SecureNote;
fido2Key: Fido2Key;
attachments: Attachment[];
fields: Field[];
passwordHistory: Password[];
@@ -96,9 +94,6 @@ export class Cipher extends Domain implements Decryptable<CipherView> {
case CipherType.Identity:
this.identity = new Identity(obj.identity);
break;
case CipherType.Fido2Key:
this.fido2Key = new Fido2Key(obj.fido2Key);
break;
default:
break;
}
@@ -148,9 +143,6 @@ export class Cipher extends Domain implements Decryptable<CipherView> {
case CipherType.Identity:
model.identity = await this.identity.decrypt(this.organizationId, encKey);
break;
case CipherType.Fido2Key:
model.fido2Key = await this.fido2Key.decrypt(this.organizationId, encKey);
break;
default:
break;
}
@@ -236,9 +228,6 @@ export class Cipher extends Domain implements Decryptable<CipherView> {
case CipherType.Identity:
c.identity = this.identity.toIdentityData();
break;
case CipherType.Fido2Key:
c.fido2Key = this.fido2Key.toFido2KeyData();
break;
default:
break;
}
@@ -292,9 +281,6 @@ export class Cipher extends Domain implements Decryptable<CipherView> {
case CipherType.SecureNote:
domain.secureNote = SecureNote.fromJSON(obj.secureNote);
break;
case CipherType.Fido2Key:
domain.fido2Key = Fido2Key.fromJSON(obj.fido2Key);
break;
default:
break;
}

View File

@@ -22,6 +22,7 @@ describe("Fido2Key", () => {
rpName: null,
userDisplayName: null,
counter: null,
discoverable: null,
});
});
@@ -37,6 +38,7 @@ describe("Fido2Key", () => {
counter: "counter",
rpName: "rpName",
userDisplayName: "userDisplayName",
discoverable: "discoverable",
};
const fido2Key = new Fido2Key(data);
@@ -51,6 +53,7 @@ describe("Fido2Key", () => {
counter: { encryptedString: "counter", encryptionType: 0 },
rpName: { encryptedString: "rpName", encryptionType: 0 },
userDisplayName: { encryptedString: "userDisplayName", encryptionType: 0 },
discoverable: { encryptedString: "discoverable", encryptionType: 0 },
});
});
@@ -76,6 +79,7 @@ describe("Fido2Key", () => {
fido2Key.counter = mockEnc("2");
fido2Key.rpName = mockEnc("rpName");
fido2Key.userDisplayName = mockEnc("userDisplayName");
fido2Key.discoverable = mockEnc("true");
const fido2KeyView = await fido2Key.decrypt(null);
@@ -90,6 +94,7 @@ describe("Fido2Key", () => {
rpName: "rpName",
userDisplayName: "userDisplayName",
counter: 2,
discoverable: true,
});
});
});
@@ -104,9 +109,10 @@ describe("Fido2Key", () => {
keyValue: "keyValue",
rpId: "rpId",
userHandle: "userHandle",
counter: "counter",
counter: "2",
rpName: "rpName",
userDisplayName: "userDisplayName",
discoverable: "true",
};
const fido2Key = new Fido2Key(data);
@@ -129,6 +135,7 @@ describe("Fido2Key", () => {
fido2Key.counter = createEncryptedEncString("2");
fido2Key.rpName = createEncryptedEncString("rpName");
fido2Key.userDisplayName = createEncryptedEncString("userDisplayName");
fido2Key.discoverable = createEncryptedEncString("discoverable");
const json = JSON.stringify(fido2Key);
const result = Fido2Key.fromJSON(JSON.parse(json));

View File

@@ -17,6 +17,7 @@ export class Fido2Key extends Domain {
counter: EncString;
rpName: EncString;
userDisplayName: EncString;
discoverable: EncString;
constructor(obj?: Fido2KeyData) {
super();
@@ -38,6 +39,7 @@ export class Fido2Key extends Domain {
counter: null,
rpName: null,
userDisplayName: null,
discoverable: null,
},
[]
);
@@ -56,6 +58,7 @@ export class Fido2Key extends Domain {
userHandle: null,
rpName: null,
userDisplayName: null,
discoverable: null,
},
orgId,
encKey
@@ -72,6 +75,16 @@ export class Fido2Key extends Domain {
// Counter will end up as NaN if this fails
view.counter = parseInt(counter);
const { discoverable } = await this.decryptObj(
{ discoverable: "" },
{
discoverable: null,
},
orgId,
encKey
);
view.discoverable = discoverable === "true";
return view;
}
@@ -88,6 +101,7 @@ export class Fido2Key extends Domain {
counter: null,
rpName: null,
userDisplayName: null,
discoverable: null,
});
return i;
}
@@ -107,6 +121,7 @@ export class Fido2Key extends Domain {
const counter = EncString.fromJSON(obj.counter);
const rpName = EncString.fromJSON(obj.rpName);
const userDisplayName = EncString.fromJSON(obj.userDisplayName);
const discoverable = EncString.fromJSON(obj.discoverable);
return Object.assign(new Fido2Key(), obj, {
credentialId,
@@ -119,6 +134,7 @@ export class Fido2Key extends Domain {
counter,
rpName,
userDisplayName,
discoverable,
});
}
}

View File

@@ -123,6 +123,7 @@ describe("Login DTO", () => {
counter: "counter" as EncryptedString,
rpName: "rpName" as EncryptedString,
userDisplayName: "userDisplayName" as EncryptedString,
discoverable: "discoverable" as EncryptedString,
},
});
@@ -143,6 +144,7 @@ describe("Login DTO", () => {
counter: "counter_fromJSON",
rpName: "rpName_fromJSON",
userDisplayName: "userDisplayName_fromJSON",
discoverable: "discoverable_fromJSON",
},
});
expect(actual).toBeInstanceOf(Login);

View File

@@ -23,7 +23,6 @@ export class CipherRequest {
secureNote: SecureNoteApi;
card: CardApi;
identity: IdentityApi;
fido2Key: Fido2KeyApi;
fields: FieldApi[];
passwordHistory: PasswordHistoryRequest[];
// Deprecated, remove at some point and rename attachments2 to attachments
@@ -104,6 +103,10 @@ export class CipherRequest {
cipher.login.fido2Key.userDisplayName != null
? cipher.login.fido2Key.userDisplayName.encryptedString
: null;
this.login.fido2Key.discoverable =
cipher.login.fido2Key.discoverable != null
? cipher.login.fido2Key.discoverable.encryptedString
: null;
}
break;
case CipherType.SecureNote:
@@ -165,39 +168,6 @@ export class CipherRequest {
? cipher.identity.licenseNumber.encryptedString
: null;
break;
case CipherType.Fido2Key:
this.fido2Key = new Fido2KeyApi();
this.fido2Key.credentialId =
cipher.fido2Key.credentialId != null
? cipher.fido2Key.credentialId.encryptedString
: null;
this.fido2Key.keyType =
cipher.fido2Key.keyType != null
? (cipher.fido2Key.keyType.encryptedString as "public-key")
: null;
this.fido2Key.keyAlgorithm =
cipher.fido2Key.keyAlgorithm != null
? (cipher.fido2Key.keyAlgorithm.encryptedString as "ECDSA")
: null;
this.fido2Key.keyCurve =
cipher.fido2Key.keyCurve != null
? (cipher.fido2Key.keyCurve.encryptedString as "P-256")
: null;
this.fido2Key.keyValue =
cipher.fido2Key.keyValue != null ? cipher.fido2Key.keyValue.encryptedString : null;
this.fido2Key.rpId =
cipher.fido2Key.rpId != null ? cipher.fido2Key.rpId.encryptedString : null;
this.fido2Key.rpName =
cipher.fido2Key.rpName != null ? cipher.fido2Key.rpName.encryptedString : null;
this.fido2Key.counter =
cipher.fido2Key.counter != null ? cipher.fido2Key.counter.encryptedString : null;
this.fido2Key.userHandle =
cipher.fido2Key.userHandle != null ? cipher.fido2Key.userHandle.encryptedString : null;
this.fido2Key.userDisplayName =
cipher.fido2Key.userDisplayName != null
? cipher.fido2Key.userDisplayName.encryptedString
: null;
break;
default:
break;
}

View File

@@ -4,7 +4,6 @@ import { IdentityApi } from "../../../models/api/identity.api";
import { LoginApi } from "../../../models/api/login.api";
import { SecureNoteApi } from "../../../models/api/secure-note.api";
import { BaseResponse } from "../../../models/response/base.response";
import { Fido2KeyApi } from "../../api/fido2-key.api";
import { CipherRepromptType } from "../../enums/cipher-reprompt-type";
import { AttachmentResponse } from "./attachment.response";
@@ -22,7 +21,6 @@ export class CipherResponse extends BaseResponse {
card: CardApi;
identity: IdentityApi;
secureNote: SecureNoteApi;
fido2Key: Fido2KeyApi;
favorite: boolean;
edit: boolean;
viewPassword: boolean;
@@ -76,11 +74,6 @@ export class CipherResponse extends BaseResponse {
this.secureNote = new SecureNoteApi(secureNote);
}
const fido2Key = this.getResponseProperty("Fido2Key");
if (fido2Key != null) {
this.fido2Key = new Fido2KeyApi(fido2Key);
}
const fields = this.getResponseProperty("Fields");
if (fields != null) {
this.fields = fields.map((f: any) => new FieldApi(f));

View File

@@ -78,8 +78,6 @@ export class CipherView implements View, InitializerMetadata {
return this.card;
case CipherType.Identity:
return this.identity;
case CipherType.Fido2Key:
return this.fido2Key;
default:
break;
}
@@ -174,9 +172,6 @@ export class CipherView implements View, InitializerMetadata {
case CipherType.SecureNote:
view.secureNote = SecureNoteView.fromJSON(obj.secureNote);
break;
case CipherType.Fido2Key:
view.fido2Key = Fido2KeyView.fromJSON(obj.fido2Key);
break;
default:
break;
}

View File

@@ -13,6 +13,7 @@ export class Fido2KeyView extends ItemView {
counter: number;
rpName: string;
userDisplayName: string;
discoverable: boolean;
get subTitle(): string {
return this.userDisplayName;

View File

@@ -1112,6 +1112,10 @@ export class CipherService implements CipherServiceAbstraction {
String(model.login.fido2Key.counter),
key
);
cipher.login.fido2Key.discoverable = await this.cryptoService.encrypt(
String(model.login.fido2Key.discoverable),
key
);
}
return;
case CipherType.SecureNote:
@@ -1162,30 +1166,6 @@ export class CipherService implements CipherServiceAbstraction {
key
);
return;
case CipherType.Fido2Key:
cipher.fido2Key = new Fido2Key();
await this.encryptObjProperty(
model.fido2Key,
cipher.fido2Key,
{
credentialId: null,
keyType: null,
keyAlgorithm: null,
keyCurve: null,
keyValue: null,
rpId: null,
rpName: null,
userHandle: null,
userDisplayName: null,
origin: null,
},
key
);
cipher.fido2Key.counter = await this.cryptoService.encrypt(
String(model.fido2Key.counter),
key
);
break;
default:
throw new Error("Unknown cipher type.");
}

View File

@@ -101,7 +101,7 @@ describe("FidoAuthenticatorService", () => {
describe.skip("when extensions parameter is present", () => undefined);
describe("vault contains excluded non-discoverable credential", () => {
describe("vault contains excluded credential", () => {
let excludedCipher: CipherView;
let params: Fido2AuthenticatorMakeCredentialsParams;
@@ -179,90 +179,13 @@ describe("FidoAuthenticatorService", () => {
);
});
describe("vault contains excluded discoverable credential", () => {
let excludedCipherView: CipherView;
let params: Fido2AuthenticatorMakeCredentialsParams;
beforeEach(async () => {
excludedCipherView = createCipherView();
params = await createParams({
excludeCredentialDescriptorList: [
{
id: guidToRawFormat(excludedCipherView.fido2Key.credentialId),
type: "public-key",
},
],
});
cipherService.get.mockImplementation(async (id) =>
id === excludedCipherView.id
? ({ decrypt: async () => excludedCipherView } as any)
: undefined
);
cipherService.getAllDecrypted.mockResolvedValue([excludedCipherView]);
});
/**
* Spec: collect an authorization gesture confirming user consent for creating a new credential.
* Deviation: Consent is not asked and the user is simply informed of the situation.
**/
it("should inform user", async () => {
userInterfaceSession.informExcludedCredential.mockResolvedValue();
try {
await authenticator.makeCredential(params);
// eslint-disable-next-line no-empty
} catch {}
expect(userInterfaceSession.informExcludedCredential).toHaveBeenCalled();
});
/** Spec: return an error code equivalent to "NotAllowedError" and terminate the operation. */
it("should throw error", async () => {
userInterfaceSession.informExcludedCredential.mockResolvedValue();
const result = async () => await authenticator.makeCredential(params);
await expect(result).rejects.toThrowError(Fido2AutenticatorErrorCode.NotAllowed);
});
/** Devation: Organization ciphers are not checked against excluded credentials, even if the user has access to them. */
it("should not inform user of duplication when the excluded credential belongs to an organization", async () => {
userInterfaceSession.informExcludedCredential.mockResolvedValue();
excludedCipherView.organizationId = "someOrganizationId";
try {
await authenticator.makeCredential(params);
// eslint-disable-next-line no-empty
} catch {}
expect(userInterfaceSession.informExcludedCredential).not.toHaveBeenCalled();
});
it("should not inform user of duplication when input data does not pass checks", async () => {
userInterfaceSession.informExcludedCredential.mockResolvedValue();
const invalidParams = await createInvalidParams();
for (const p of Object.values(invalidParams)) {
try {
await authenticator.makeCredential(p);
// eslint-disable-next-line no-empty
} catch {}
}
expect(userInterfaceSession.informExcludedCredential).not.toHaveBeenCalled();
});
it.todo(
"should not throw error if the excluded credential has been marked as deleted in the vault"
);
});
describe("credential creation", () => {
let existingCipher: CipherView;
let params: Fido2AuthenticatorMakeCredentialsParams;
beforeEach(async () => {
existingCipher = createCipherView({ type: CipherType.Login });
params = await createParams();
params = await createParams({ requireResidentKey: false });
cipherService.get.mockImplementation(async (id) =>
id === existingCipher.id ? ({ decrypt: () => existingCipher } as any) : undefined
);
@@ -321,6 +244,7 @@ describe("FidoAuthenticatorService", () => {
userHandle: Fido2Utils.bufferToString(params.userEntity.id),
counter: 0,
userDisplayName: params.userEntity.displayName,
discoverable: false,
}),
}),
})
@@ -498,7 +422,7 @@ describe("FidoAuthenticatorService", () => {
* Spec: If requireUserVerification is true and the authenticator cannot perform user verification, return an error code equivalent to "ConstraintError" and terminate the operation.
* Deviation: User verification is checked before checking for excluded credentials
**/
/** TODO: This test should only be activated if we disable support for user verification */
/** NOTE: This test should only be activated if we disable support for user verification */
it.skip("should throw error if requireUserVerification is set to true", async () => {
const params = await createParams({ requireUserVerification: true });
@@ -583,11 +507,11 @@ describe("FidoAuthenticatorService", () => {
ciphers = [
await createCipherView(
{ type: CipherType.Login },
{ credentialId: credentialIds[0], rpId: RpId }
{ credentialId: credentialIds[0], rpId: RpId, discoverable: false }
),
await createCipherView(
{ type: CipherType.Login },
{ credentialId: credentialIds[1], rpId: RpId }
{ credentialId: credentialIds[1], rpId: RpId, discoverable: true }
),
];
params = await createParams({
@@ -600,6 +524,36 @@ describe("FidoAuthenticatorService", () => {
cipherService.getAllDecrypted.mockResolvedValue(ciphers);
});
it("should ask for all credentials in list when `params` contains allowedCredentials list", async () => {
userInterfaceSession.pickCredential.mockResolvedValue({
cipherId: ciphers[0].id,
userVerified: false,
});
await authenticator.getAssertion(params);
expect(userInterfaceSession.pickCredential).toHaveBeenCalledWith({
cipherIds: ciphers.map((c) => c.id),
userVerification: false,
});
});
it("should only ask for discoverable credentials matched by rpId when params does not contains allowedCredentials list", async () => {
params.allowCredentialDescriptorList = undefined;
const discoverableCiphers = ciphers.filter((c) => c.login.fido2Key.discoverable);
userInterfaceSession.pickCredential.mockResolvedValue({
cipherId: discoverableCiphers[0].id,
userVerified: false,
});
await authenticator.getAssertion(params);
expect(userInterfaceSession.pickCredential).toHaveBeenCalledWith({
cipherIds: [discoverableCiphers[0].id],
userVerification: false,
});
});
for (const userVerification of [true, false]) {
/** Spec: Prompt the user to select a public key credential source selectedCredential from credentialOptions. */
it(`should request confirmation from user when user verification is ${userVerification}`, async () => {
@@ -787,7 +741,7 @@ function createCipherView(
): CipherView {
const cipher = new CipherView();
cipher.id = data.id ?? Utils.newGuid();
cipher.type = data.type ?? CipherType.Fido2Key;
cipher.type = CipherType.Login;
cipher.localData = {};
const fido2KeyView = new Fido2KeyView();
@@ -797,6 +751,7 @@ function createCipherView(
fido2KeyView.userHandle = fido2Key.userHandle ?? Fido2Utils.bufferToString(randomBytes(16));
fido2KeyView.keyAlgorithm = fido2Key.keyAlgorithm ?? "ECDSA";
fido2KeyView.keyCurve = fido2Key.keyCurve ?? "P-256";
fido2KeyView.discoverable = fido2Key.discoverable ?? true;
fido2KeyView.keyValue =
fido2KeyView.keyValue ??
"MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgTC-7XDZipXbaVBlnkjlBgO16ZmqBZWejK2iYo6lV0dehRANCAASOcM2WduNq1DriRYN7ZekvZz-bRhA-qNT4v0fbp5suUFJyWmgOQ0bybZcLXHaerK5Ep1JiSrQcewtQNgLtry7f";

View File

@@ -188,8 +188,6 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
}
let cipherOptions: CipherView[];
// eslint-disable-next-line no-empty
if (params.allowCredentialDescriptorList?.length > 0) {
cipherOptions = await this.findCredentialsById(
params.allowCredentialDescriptorList,
@@ -301,10 +299,9 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
(cipher) =>
!cipher.isDeleted &&
cipher.organizationId == undefined &&
((cipher.type === CipherType.Fido2Key && ids.includes(cipher.fido2Key.credentialId)) ||
(cipher.type === CipherType.Login &&
cipher.login.fido2Key != undefined &&
ids.includes(cipher.login.fido2Key.credentialId)))
cipher.type === CipherType.Login &&
cipher.login.fido2Key != undefined &&
ids.includes(cipher.login.fido2Key.credentialId)
)
.map((cipher) => cipher.id);
}
@@ -344,7 +341,8 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
!cipher.isDeleted &&
cipher.type === CipherType.Login &&
cipher.login.fido2Key != undefined &&
cipher.login.fido2Key.rpId === rpId
cipher.login.fido2Key.rpId === rpId &&
cipher.login.fido2Key.discoverable
);
}
}
@@ -380,6 +378,7 @@ async function createKeyView(
fido2Key.counter = 0;
fido2Key.rpName = params.rpEntity.name;
fido2Key.userDisplayName = params.userEntity.displayName;
fido2Key.discoverable = params.requireResidentKey;
return fido2Key;
}