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.

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

Do not use cancelled RP IDs for recovery device #2813

Merged
merged 1 commit into from
Jan 23, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 8 additions & 1 deletion src/frontend/src/flows/recovery/recoverWith/device.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,5 +115,12 @@ const attemptRecovery = async ({
.map(convertToValidCredentialData)
.filter(nonNullish);

return await connection.fromWebauthnCredentials(userNumber, credentialData);
// Cancelled RP IDs are based on the other credentials, we don't want them for the recovery device.
// If we had multiple recovery devices, we would need to handle this differently.
const skipCancelledRpIdsStorage = true;
return await connection.fromWebauthnCredentials(
userNumber,
credentialData,
skipCancelledRpIdsStorage
);
};
111 changes: 110 additions & 1 deletion src/frontend/src/utils/iiConnection.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@ import {
IdentityMetadata,
RECOVERY_PAGE_SHOW_TIMESTAMP_MILLIS,
} from "$src/repositories/identityMetadata";
import { setLastShownAddCurrentDevicePage } from "$src/storage";
import {
addAnchorCancelledRpId,
getCancelledRpIds,
setLastShownAddCurrentDevicePage,
} from "$src/storage";
import { ActorSubclass, DerEncodedPublicKey, Signature } from "@dfinity/agent";
import { DelegationIdentity, WebAuthnIdentity } from "@dfinity/identity";
import { IDBFactory } from "fake-indexeddb";
Expand Down Expand Up @@ -394,6 +398,111 @@ describe("Connection.login", () => {
);
}
});

describe("Connection.fromWebauthnCredentials", () => {
const userNumber = BigInt(12345);
const deviceFromCurrentDomain: DeviceData =
createMockDevice(currentOrigin);
const credentialDataFromCurrentDomain = convertToValidCredentialData(
deviceFromCurrentDomain
) as CredentialData;
const deviceAnotherDomain: DeviceData = createMockDevice(
"htts://identity.ic0.app"
);
const credentialDataAnotherDomain = convertToValidCredentialData(
deviceAnotherDomain
) as CredentialData;
const skipCancelledRpIdsStorage = true;

it("doesn't use the cancelled RP ID if skipCancelledRpIdsStorage is true", async () => {
const cancelledRpId = new URL(currentOrigin).hostname;
await addAnchorCancelledRpId({
userNumber,
origin: currentOrigin,
cancelledRpId,
});
const connection = new Connection("aaaaa-aa", mockActor);

const loginResult = await connection.fromWebauthnCredentials(
userNumber,
[credentialDataFromCurrentDomain],
skipCancelledRpIdsStorage
);

expect(loginResult.kind).toBe("loginSuccess");
if (loginResult.kind === "loginSuccess") {
expect(MultiWebAuthnIdentity.fromCredentials).toHaveBeenCalledTimes(
1
);
expect(MultiWebAuthnIdentity.fromCredentials).toHaveBeenCalledWith(
[credentialDataFromCurrentDomain],
// `undefined` means the current origin which is the one that was cancelled
undefined
);
}
});

it("uses the cancelled RP ID if skipCancelledRpIdsStorage is false", async () => {
const cancelledRpId = new URL(currentOrigin).hostname;
await addAnchorCancelledRpId({
userNumber,
origin: currentOrigin,
cancelledRpId,
});
const connection = new Connection("aaaaa-aa", mockActor);

failSign = true;
const call = () =>
connection.fromWebauthnCredentials(
userNumber,
[credentialDataFromCurrentDomain],
!skipCancelledRpIdsStorage
);

// This is because the device is filtered out and then the `findWebAuthnRpId` doesn't receive any device.
await expect(call).rejects.toThrowError(
new Error(
"Not possible. Every registered user has at least one device."
)
);
});

it("doesn't persist the cancelled RP ID if skipCancelledRpIdsStorage is true", async () => {
const connection = new Connection("aaaaa-aa", mockActor);

failSign = true;
const loginResult = await connection.fromWebauthnCredentials(
userNumber,
[credentialDataFromCurrentDomain, credentialDataAnotherDomain],
skipCancelledRpIdsStorage
);

expect(loginResult.kind).toBe("webAuthnFailed");
const { cancelledRpIds } = await getCancelledRpIds({
userNumber,
origin: currentOrigin,
});
expect(cancelledRpIds).toEqual(new Set());
});

it("doesn't persist the cancelled RP ID if skipCancelledRpIdsStorage is false", async () => {
const connection = new Connection("aaaaa-aa", mockActor);

failSign = true;
const loginResult = await connection.fromWebauthnCredentials(
userNumber,
[credentialDataFromCurrentDomain, credentialDataAnotherDomain],
!skipCancelledRpIdsStorage
);

expect(loginResult.kind).toBe("possiblyWrongRPID");
const { cancelledRpIds } = await getCancelledRpIds({
userNumber,
origin: currentOrigin,
});
expect(cancelledRpIds).toEqual(new Set([undefined]));
});
});
});

describe("domains compatibility flag enabled and browser doesn't support", () => {
Expand Down
17 changes: 12 additions & 5 deletions src/frontend/src/utils/iiConnection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -402,14 +402,20 @@ export class Connection {

fromWebauthnCredentials = async (
userNumber: bigint,
credentials: CredentialData[]
credentials: CredentialData[],
skipCancelledRpIdsStorage = false
): Promise<LoginSuccess | WebAuthnFailed | PossiblyWrongRPID | AuthFail> => {
// Get cancelled rpids for the user from local storage.
const { cancelledRpIds, lastShownAddCurrentDevicePage } =
await getCancelledRpIds({
userNumber,
origin: window.location.origin,
});
skipCancelledRpIdsStorage
? {
cancelledRpIds: new Set<string>(),
lastShownAddCurrentDevicePage: undefined,
}
: await getCancelledRpIds({
userNumber,
origin: window.location.origin,
});
const currentOrigin = window.location.origin;
const dynamicRPIdEnabled =
DOMAIN_COMPATIBILITY.isEnabled() &&
Expand Down Expand Up @@ -442,6 +448,7 @@ export class Connection {
// We only want to cache cancelled rpids if there can be multiple rpids.
if (
dynamicRPIdEnabled &&
!skipCancelledRpIdsStorage &&
hasCredentialsFromMultipleOrigins(credentials)
) {
try {
Expand Down
Loading