1
0
mirror of https://github.com/bitwarden/browser synced 2025-01-26 19:25:10 +01:00

fix(passkeys): [PM-13932] Fix passkey flow incorrect routing (#12363)

This PR fixes a bug in the LockComponent refresh that affected the setup/save and use passkey flows. The user was wrongly directly to the /vault after unlock instead of to /fido2 (the passkey screen).

Feature Flag: ExtensionRefresh ON
This commit is contained in:
rr-bw 2024-12-12 13:10:54 -08:00 committed by GitHub
parent 2da3043697
commit 8ec75613dc
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 85 additions and 9 deletions

View File

@ -659,7 +659,14 @@ const routes: Routes = [
},
showReadonlyHostname: true,
showAcctSwitcher: true,
} satisfies ExtensionAnonLayoutWrapperData,
elevation: 1,
/**
* This ensures that in a passkey flow the `/fido2?<queryParams>` URL does not get
* overwritten in the `BrowserRouterService` by the `/lockV2` route. This way, after
* unlocking, the user can be redirected back to the `/fido2?<queryParams>` URL.
*/
doNotSaveUrl: true,
} satisfies ExtensionAnonLayoutWrapperData & RouteDataProperties,
children: [
{
path: "",

View File

@ -40,15 +40,14 @@ describe("unauthUiRefreshRedirect", () => {
it("returns UrlTree when UnauthenticatedExtensionUIRefresh flag is enabled and preserves query params", async () => {
configService.getFeatureFlag.mockResolvedValue(true);
const queryParams = { test: "test" };
const urlTree = new UrlTree();
urlTree.queryParams = { test: "test" };
const navigation: Navigation = {
extras: {
queryParams: queryParams,
},
extras: {},
id: 0,
initialUrl: new UrlTree(),
extractedUrl: new UrlTree(),
extractedUrl: urlTree,
trigger: "imperative",
previousNavigation: undefined,
};
@ -60,6 +59,8 @@ describe("unauthUiRefreshRedirect", () => {
expect(configService.getFeatureFlag).toHaveBeenCalledWith(
FeatureFlag.UnauthenticatedExtensionUIRefresh,
);
expect(router.createUrlTree).toHaveBeenCalledWith(["/redirect"], { queryParams });
expect(router.createUrlTree).toHaveBeenCalledWith(["/redirect"], {
queryParams: urlTree.queryParams,
});
});
});

View File

@ -17,7 +17,7 @@ export function unauthUiRefreshRedirect(redirectUrl: string): () => Promise<bool
);
if (shouldRedirect) {
const currentNavigation = router.getCurrentNavigation();
const queryParams = currentNavigation?.extras?.queryParams || {};
const queryParams = currentNavigation?.extractedUrl?.queryParams || {};
// Preserve query params when redirecting as it is likely that the refreshed component
// will be consuming the same query params.

View File

@ -0,0 +1,62 @@
import { TestBed } from "@angular/core/testing";
import { Navigation, Router, UrlTree } from "@angular/router";
import { mock, MockProxy } from "jest-mock-extended";
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
import { extensionRefreshRedirect } from "./extension-refresh-redirect";
describe("extensionRefreshRedirect", () => {
let configService: MockProxy<ConfigService>;
let router: MockProxy<Router>;
beforeEach(() => {
configService = mock<ConfigService>();
router = mock<Router>();
TestBed.configureTestingModule({
providers: [
{ provide: ConfigService, useValue: configService },
{ provide: Router, useValue: router },
],
});
});
it("returns true when ExtensionRefresh flag is disabled", async () => {
configService.getFeatureFlag.mockResolvedValue(false);
const result = await TestBed.runInInjectionContext(() =>
extensionRefreshRedirect("/redirect")(),
);
expect(result).toBe(true);
expect(configService.getFeatureFlag).toHaveBeenCalledWith(FeatureFlag.ExtensionRefresh);
expect(router.parseUrl).not.toHaveBeenCalled();
});
it("returns UrlTree when ExtensionRefresh flag is enabled and preserves query params", async () => {
configService.getFeatureFlag.mockResolvedValue(true);
const urlTree = new UrlTree();
urlTree.queryParams = { test: "test" };
const navigation: Navigation = {
extras: {},
id: 0,
initialUrl: new UrlTree(),
extractedUrl: urlTree,
trigger: "imperative",
previousNavigation: undefined,
};
router.getCurrentNavigation.mockReturnValue(navigation);
await TestBed.runInInjectionContext(() => extensionRefreshRedirect("/redirect")());
expect(configService.getFeatureFlag).toHaveBeenCalledWith(FeatureFlag.ExtensionRefresh);
expect(router.createUrlTree).toHaveBeenCalledWith(["/redirect"], {
queryParams: urlTree.queryParams,
});
});
});

View File

@ -16,7 +16,7 @@ export function extensionRefreshRedirect(redirectUrl: string): () => Promise<boo
const shouldRedirect = await configService.getFeatureFlag(FeatureFlag.ExtensionRefresh);
if (shouldRedirect) {
const currentNavigation = router.getCurrentNavigation();
const queryParams = currentNavigation?.extras?.queryParams || {};
const queryParams = currentNavigation?.extractedUrl?.queryParams || {};
// Preserve query params when redirecting as it is likely that the refreshed component
// will be consuming the same query params.

View File

@ -534,8 +534,14 @@ export class LockV2Component implements OnInit, OnDestroy {
if (this.clientType === "browser") {
const previousUrl = this.lockComponentService.getPreviousUrl();
/**
* In a passkey flow, the `previousUrl` will still be `/fido2?<queryParams>` at this point
* because the `/lockV2` route doesn't save the URL in the `BrowserRouterService`. This is
* handled by the `doNotSaveUrl` property on the `lockV2` route in `app-routing.module.ts`.
*/
if (previousUrl) {
await this.router.navigateByUrl(previousUrl);
return;
}
}