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-4882] Passkeys: funnel rp name or id to the cipher name on save #7969

Merged
merged 3 commits into from
Mar 6, 2024
Merged

[PM-4882] Passkeys: funnel rp name or id to the cipher name on save #7969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6459f3f
funnel rp name or id to the cipher name on save
Feb 15, 2024
c3c8dff
remove comment
Feb 16, 2024
ff9a2a7
add rp name and id to addCipher function
Feb 28, 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 @@ -67,6 +67,7 @@ export type BrowserFido2Message = { sessionId: string } & (
userName: string;
userVerification: boolean;
fallbackSupported: boolean;
rpId: string;
}
| {
type: "ConfirmNewCredentialResponse";
Expand Down Expand Up @@ -242,6 +243,7 @@ export class BrowserFido2UserInterfaceSession implements Fido2UserInterfaceSessi
credentialName,
userName,
userVerification,
rpId,
}: NewCredentialParams): Promise<{ cipherId: string; userVerified: boolean }> {
const data: BrowserFido2Message = {
type: "ConfirmNewCredentialRequest",
Expand All @@ -250,6 +252,7 @@ export class BrowserFido2UserInterfaceSession implements Fido2UserInterfaceSessi
userName,
userVerification,
fallbackSupported: this.fallbackSupported,
rpId,
};

await this.send(data);
Expand Down
14 changes: 7 additions & 7 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
Expand Up @@ -16,7 +16,6 @@
import { SettingsService } from "@bitwarden/common/abstractions/settings.service";
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
import { Utils } from "@bitwarden/common/platform/misc/utils";
import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service";
import { SecureNoteType, CipherType } from "@bitwarden/common/vault/enums";
import { CipherRepromptType } from "@bitwarden/common/vault/enums/cipher-reprompt-type";
Expand Down Expand Up @@ -245,7 +244,8 @@
protected async saveNewLogin() {
const data = this.message$.value;
if (data?.type === "ConfirmNewCredentialRequest") {
await this.createNewCipher();
const name = data.credentialName || data.rpId;
await this.createNewCipher(name);

Check warning on line 248 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#L248 

Added line #L248 was not covered by tests

// We are bypassing user verification pending implementation of PIN and biometric support.
this.send({
Expand Down Expand Up @@ -296,7 +296,7 @@
// eslint-disable-next-line @typescript-eslint/no-floating-promises
this.router.navigate(["/add-cipher"], {
queryParams: {
name: Utils.getHostname(this.url),
name: data.credentialName || data.rpId,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

update: had to funnel in the credentialName and rpId here as well for when creating a new cipher (see last screenshot)

Copy link
Contributor

Choose a reason for hiding this comment

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

I was gonna suggest using ?? instead, but realized that || is actually better because it also covers an empty string "" 👍

uri: this.url,
uilocation: "popout",
senderTabId: this.senderTabId,
Expand Down Expand Up @@ -344,9 +344,9 @@
this.destroy$.complete();
}

private buildCipher() {
private buildCipher(name: string) {
this.cipher = new CipherView();
this.cipher.name = Utils.getHostname(this.url);
this.cipher.name = name;

Check warning on line 349 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#L349 

Added line #L349 was not covered by tests
this.cipher.type = CipherType.Login;
this.cipher.login = new LoginView();
this.cipher.login.uris = [new LoginUriView()];
Expand All @@ -358,8 +358,8 @@
this.cipher.reprompt = CipherRepromptType.None;
}

private async createNewCipher() {
this.buildCipher();
private async createNewCipher(name: string) {
this.buildCipher(name);

Check warning on line 362 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#L362 

Added line #L362 was not covered by tests
const cipher = await this.cipherService.encrypt(this.cipher);
try {
await this.cipherService.createWithServer(cipher);
Expand Down
4 changes: 4 additions & 0 deletions libs/common/src/vault/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 @@ -16,6 +16,10 @@ export interface NewCredentialParams {
* Whether or not the user must be verified before completing the operation.
*/
userVerification: boolean;
/**
* The relying party ID is usually the URL
*/
rpId: string;
}

/**
Expand Down
1 change: 1 addition & 0 deletions libs/common/src/vault/services/fido2/fido2-authenticator.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ describe("FidoAuthenticatorService", () => {
credentialName: params.rpEntity.name,
userName: params.userEntity.displayName,
userVerification,
rpId: params.rpEntity.id,
} as NewCredentialParams);
});
}
Expand Down
1 change: 1 addition & 0 deletions libs/common/src/vault/services/fido2/fido2-authenticator.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@
credentialName: params.rpEntity.name,
userName: params.userEntity.displayName,
userVerification: params.requireUserVerification,
rpId: params.rpEntity.id,

Check warning on line 116 in libs/common/src/vault/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 124 to 125. 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.
});
const cipherId = response.cipherId;
userVerified = response.userVerified;
Expand Down
Loading