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

[PM-13932] Fix passkey flow incorrect routing #12363

Merged
merged 4 commits into from
Dec 12, 2024
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 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,
JaredSnider-Bitwarden marked this conversation as resolved.
Show resolved Hide resolved
} 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 || {};
JaredSnider-Bitwarden marked this conversation as resolved.
Show resolved Hide resolved

// 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 @@

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;

Check warning on line 544 in libs/auth/src/angular/lock/lock.component.ts

View check run for this annotation

Codecov / codecov/patch

libs/auth/src/angular/lock/lock.component.ts#L544

Added line #L544 was not covered by tests
JaredSnider-Bitwarden marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
Loading