Skip to content

Commit

Permalink
fix(passkeys): [PM-13932] Fix passkey flow incorrect routing (#12363)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
rr-bw authored Dec 12, 2024
1 parent 2da3043 commit 8ec7561
Show file tree
Hide file tree
Showing 6 changed files with 85 additions and 9 deletions.
9 changes: 8 additions & 1 deletion apps/browser/src/popup/app-routing.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: "",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand All @@ -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,
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
62 changes: 62 additions & 0 deletions libs/angular/src/utils/extension-refresh-redirect.spec.ts
Original file line number Diff line number Diff line change
@@ -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,
});
});
});
2 changes: 1 addition & 1 deletion libs/angular/src/utils/extension-refresh-redirect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 6 additions & 0 deletions libs/auth/src/angular/lock/lock.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down

0 comments on commit 8ec7561

Please sign in to comment.