From 477cf73def3850c5ce7b38d63725b516497e10f4 Mon Sep 17 00:00:00 2001 From: drewdelano <22156330+drewdelano@users.noreply.github.com> Date: Thu, 13 Oct 2022 11:54:19 -0700 Subject: [PATCH] fix: login redirect should preserve query parameters (#2012) https://www.notion.so/Ensure-redirect-from-Pricing-page-takes-you-to-post-login-modal-of-selected-tier-e9fb5131d7b2486596de0d8c17f77161 This PR is a follow up to a conversation BenGo (not tagging him here to avoid notification noise) and I had a couple of weeks ago. The high level summary is: Something like the pricing page could use a `requiresAuth: true` behavior. This would function similar to the `redirectTo: '...'` behavior, except it would capture the url and handle the redirecting automatically. This is important for tiered pricing because otherwise we can't include which tier the customer selected on the public pricing page before we redirect to the authenticated accounts payment page because the `redirectTo` must be static. --- .../contexts/authorizationContext.js | 2 ++ .../components/general/restrictedRoute.js | 18 ++++++++++++------ packages/website/components/types.d.ts | 1 + packages/website/pages/account/payment.js | 3 +-- .../website/tests/accountPayment.e2e.spec.ts | 17 +++++++++++++---- 5 files changed, 29 insertions(+), 12 deletions(-) diff --git a/packages/website/components/contexts/authorizationContext.js b/packages/website/components/contexts/authorizationContext.js index 12d6533c58..d011fca735 100644 --- a/packages/website/components/contexts/authorizationContext.js +++ b/packages/website/components/contexts/authorizationContext.js @@ -12,6 +12,8 @@ import { isLoggedIn, getMagic } from 'lib/magic'; * @property {string} [redirectTo] If redirectTo is set, redirect if the user was not found * @property {boolean} [redirectIfFound] If redirectIfFound is also set, redirect if the user was found. * @property {boolean} [authOnLoad] Whether or not to check for magic authorization, defaults to true + * @property {boolean} [requiresAuth] Whether or not to perform automatic login and redirect back + * to this page if unauthenticated */ /** diff --git a/packages/website/components/general/restrictedRoute.js b/packages/website/components/general/restrictedRoute.js index 9616f8cd01..68bebb61c0 100644 --- a/packages/website/components/general/restrictedRoute.js +++ b/packages/website/components/general/restrictedRoute.js @@ -1,3 +1,5 @@ +import { stringify } from 'querystring'; + import { useRouter } from 'next/router'; import { useEffect } from 'react'; @@ -11,6 +13,8 @@ import { useAuthorization } from 'components/contexts/authorizationContext'; * @property {boolean} [isRestricted] whether or not this route is restricted to auhtenticated users * @property {string} [redirectTo] If redirectTo is set, redirect if the user was not found * @property {boolean} [redirectIfFound] If redirectIfFound is also set, redirect if the user was found. + * @property {boolean} [requiresAuth] If requiresAuth is set and redirectTo isn't set then, + * redirect to the login page setting the return uri to this page */ /** @@ -20,12 +24,12 @@ import { useAuthorization } from 'components/contexts/authorizationContext'; * * @returns */ -const RestrictedRoute = ({ isRestricted = false, children, redirectTo, redirectIfFound = false }) => { - const { push } = useRouter(); +const RestrictedRoute = ({ isRestricted = false, children, redirectTo, redirectIfFound = false, requiresAuth }) => { + const { push, asPath } = useRouter(); const { isLoading, isFetching, isLoggedIn } = useAuthorization(); useEffect(() => { - if (!redirectTo || isLoading || isFetching) { + if (!(redirectTo || requiresAuth) || isLoading || isFetching) { return; } if ( @@ -34,11 +38,13 @@ const RestrictedRoute = ({ isRestricted = false, children, redirectTo, redirectI // If redirectIfFound is also set, redirect if the user was found (redirectIfFound && isLoggedIn) ) { - push(redirectTo); + push(redirectTo ?? ''); + } else if (requiresAuth && !isLoggedIn) { + push(`/login/?${stringify({ redirect_uri: asPath })}`); } - }, [redirectTo, redirectIfFound, isFetching, isLoading, isLoggedIn, push]); + }, [redirectTo, redirectIfFound, isFetching, isLoading, isLoggedIn, push, asPath, requiresAuth]); - const shouldWaitForLoggedIn = isRestricted && !isLoggedIn; + const shouldWaitForLoggedIn = (isRestricted || requiresAuth) && !isLoggedIn; return !shouldWaitForLoggedIn ? ( children diff --git a/packages/website/components/types.d.ts b/packages/website/components/types.d.ts index 785d72fcf7..fce0aeaaab 100644 --- a/packages/website/components/types.d.ts +++ b/packages/website/components/types.d.ts @@ -4,6 +4,7 @@ export interface RestrictedRouteProps { isRestricted?: boolean; redirectTo?: string; redirectIfFound?: boolean; + requiresAuth?: boolean; pageBgColor?: string; // TODO: Remove if unused navBgColor?: string; // TODO: Remove if unused footerBgColor?: string; // TODO: Remove if unused diff --git a/packages/website/pages/account/payment.js b/packages/website/pages/account/payment.js index 7327c4d386..fc9de028b0 100644 --- a/packages/website/pages/account/payment.js +++ b/packages/website/pages/account/payment.js @@ -218,8 +218,7 @@ export function getStaticProps() { return { props: { title: 'Payment', - isRestricted: true, - redirectTo: '/login/?redirect_uri=/account/payment', + requiresAuth: true, stripePublishableKey, breadcrumbs: [crumbs.index, crumbs.payment], }, diff --git a/packages/website/tests/accountPayment.e2e.spec.ts b/packages/website/tests/accountPayment.e2e.spec.ts index 06a1a9e74e..27072e926b 100644 --- a/packages/website/tests/accountPayment.e2e.spec.ts +++ b/packages/website/tests/accountPayment.e2e.spec.ts @@ -11,20 +11,29 @@ test.beforeEach(async ({ page }) => { test.describe('/account/payment', () => { test('redirects through login and back when not initially authenticated', async ({ page }, testInfo) => { - const accountPaymentPathname = '/account/payment'; + const accountPaymentPathname = '/account/payment/'; + const accountQuery = '?plan=lite' + const accountUrl = `${accountPaymentPathname}${accountQuery}`; + // try to go to page that requires authn - await page.goto(accountPaymentPathname, { waitUntil: 'networkidle' }); + await page.goto(accountUrl, { waitUntil: 'networkidle' }); // wait for redirect to a Log in page await page.locator('text=Log in').waitFor(); const pageUrl = new URL(page.url()); + expect(pageUrl.pathname).toEqual('/login/'); - expect(pageUrl.searchParams.get('redirect_uri')).toEqual(accountPaymentPathname); + expect(pageUrl.searchParams.get('redirect_uri')).toEqual(accountUrl); // fill login await LoginTester().login(page, { email: MAGIC_SUCCESS_EMAIL, }); + + console.log(`pathname: ${new URL(page.url()).pathname}`); + console.log(`search: ${new URL(page.url()).search}`); + // should be back at our initial target destination - expect(new URL(page.url()).pathname).toEqual(accountPaymentPathname + '/'); + expect(new URL(page.url()).pathname).toEqual(accountPaymentPathname); + expect(new URL(page.url()).search).toEqual(accountQuery); await page.screenshot({ fullPage: true, path: await E2EScreenshotPath(testInfo, `accountPayment-noauth`),