Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

PM-4877: Only allow replacing passkeys for the same userhandle #9804

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+28 −2
Open

PM-4877: Only allow replacing passkeys for the same userhandle #9804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2c09ec9
Initial draft
abergs Jun 24, 2024
bf8e6dd
small cleanup
abergs Jun 24, 2024
f7491f9
show vaul items without passkeys
abergs Jun 25, 2024
41aa0b8
Refactored a bit
abergs Jun 25, 2024
c9b53ca
tests run for me?
abergs Jun 25, 2024
bc37b44
Fixed platform test
abergs Jun 25, 2024
f4e5585
null and undefined
abergs Jun 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions apps/browser/src/vault/fido2/browser-fido2-user-interface.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ export type BrowserFido2Message = { sessionId: string } & (
type: "ConfirmNewCredentialRequest";
credentialName: string;
userName: string;
userHandle: string;
userVerification: boolean;
fallbackSupported: boolean;
rpId: string;
Expand Down Expand Up @@ -242,6 +243,7 @@ export class BrowserFido2UserInterfaceSession implements Fido2UserInterfaceSessi
async confirmNewCredential({
credentialName,
userName,
userHandle,
userVerification,
rpId,
}: NewCredentialParams): Promise<{ cipherId: string; userVerified: boolean }> {
Expand All @@ -250,6 +252,7 @@ export class BrowserFido2UserInterfaceSession implements Fido2UserInterfaceSessi
sessionId: this.sessionId,
credentialName,
userName,
userHandle,
userVerification,
fallbackSupported: this.fallbackSupported,
rpId,
Expand Down
20 changes: 18 additions & 2 deletions apps/browser/src/vault/popup/components/fido2/fido2.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Component, OnDestroy, OnInit } from "@angular/core";

Check warning on line 1 in apps/browser/src/vault/popup/components/fido2/fido2.component.ts

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (main)

❌ New issue: String Heavy Function Arguments 

In this module, 41.7% of all arguments to its 17 functions are strings. The threshold for string arguments is 39.0%. The functions in this file have a high ratio of strings as arguments. Avoid adding more.
import { ActivatedRoute, Router } from "@angular/router";
import {
BehaviorSubject,
Expand Down Expand Up @@ -143,8 +143,10 @@
this.ciphers = (await this.cipherService.getAllDecrypted()).filter(
(cipher) => cipher.type === CipherType.Login && !cipher.isDeleted,
);
this.displayedCiphers = this.ciphers.filter((cipher) =>
cipher.login.matchesUri(this.url, equivalentDomains),
this.displayedCiphers = this.ciphers.filter(

Check warning on line 146 in apps/browser/src/vault/popup/components/fido2/fido2.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/components/fido2/fido2.component.ts#L146 

Added line #L146 was not covered by tests
(cipher) =>
cipher.login.matchesUri(this.url, equivalentDomains) &&
this.hasNoOtherPasskeys(cipher, message.userHandle),

Check warning on line 149 in apps/browser/src/vault/popup/components/fido2/fido2.component.ts

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (main)

❌ Getting worse: Complex Method 

Fido2Component.ngOnInit increases in cyclomatic complexity from 14 to 15, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
);

if (this.displayedCiphers.length > 0) {
Expand Down Expand Up @@ -402,4 +404,18 @@
...msg,
});
}

/**
* This methods returns true if a cipher either has no passkeys, or has a passkey matching with userHandle
* @param userHandle
*/
private hasNoOtherPasskeys(cipher: CipherView, userHandle: string): boolean {
if (cipher.login.fido2Credentials == null || cipher.login.fido2Credentials.length === 0) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need re-review because I was using === null which caused an error. Thanks to @justindbaur who told me to fix that in another PR i knew right away what to do!

return true;

Check warning on line 414 in apps/browser/src/vault/popup/components/fido2/fido2.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/components/fido2/fido2.component.ts#L414 

Added line #L414 was not covered by tests
}

return cipher.login.fido2Credentials.some((passkey) => {
passkey.userHandle === userHandle;

Check warning on line 418 in apps/browser/src/vault/popup/components/fido2/fido2.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/components/fido2/fido2.component.ts#L417-L418 

Added lines #L417 - L418 were not covered by tests
});
}
}
5 changes: 5 additions & 0 deletions libs/common/src/platform/abstractions/fido2/fido2-user-interface.service.abstraction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ export interface NewCredentialParams {
*/
userName: string;

/**
* The userhandle (userid) of the user.
*/
userHandle: string;

/**
* Whether or not the user must be verified before completing the operation.
*/
Expand Down
1 change: 1 addition & 0 deletions libs/common/src/platform/services/fido2/fido2-authenticator.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ describe("FidoAuthenticatorService", () => {
expect(userInterfaceSession.confirmNewCredential).toHaveBeenCalledWith({
credentialName: params.rpEntity.name,
userName: params.userEntity.displayName,
userHandle: Fido2Utils.bufferToString(params.userEntity.id),
userVerification,
rpId: params.rpEntity.id,
} as NewCredentialParams);
Expand Down
1 change: 1 addition & 0 deletions libs/common/src/platform/services/fido2/fido2-authenticator.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@
const response = await userInterfaceSession.confirmNewCredential({
credentialName: params.rpEntity.name,
userName: params.userEntity.displayName,
userHandle: Fido2Utils.bufferToString(params.userEntity.id),

Check warning on line 115 in libs/common/src/platform/services/fido2/fido2-authenticator.service.ts

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (main)

❌ Getting worse: Complex Method 

Fido2AuthenticatorService.makeCredential already has high cyclomatic complexity, and now it increases in Lines of Code from 125 to 126. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
userVerification: params.requireUserVerification,
rpId: params.rpEntity.id,
});
Expand Down
Loading