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.

Sign up for GitHub

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

Jump to bottom

feat: /account/login?redirect_to=/final/url should redirect to /final/url after authn #1950

Merged
merged 7 commits into from
Sep 29, 2022
Merged

feat: /account/login?redirect_to=/final/url should redirect to /final/url after authn #1950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
dee728d
wip
gobengo Sep 22, 2022
e734441
polish
gobengo Sep 26, 2022
962056d
ci: website e2e uses --reporter=list (in addition to html)
gobengo Sep 26, 2022
76ae348
fix e2e test now that login redirects through to redirect_uri
gobengo Sep 26, 2022
8f66205
Revert "ci: website e2e uses --reporter=list (in addition to html)"
gobengo Sep 27, 2022
dcc8cc7
fix e2e test
gobengo Sep 27, 2022
b256d55
fix tests when using mocks
gobengo Sep 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 8 additions & 2 deletions packages/website/lib/magic.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,17 @@ export async function isLoggedIn() {
* Login with email
*
* @param {string} email
* @param {string} [finalRedirectHref] - href of final url that end-user should
* be redirected to after successful authentication
*/
export async function loginEmail(email) {
export async function loginEmail(email, finalRedirectHref) {
const redirectUri = new URL('/callback/', window.location.origin);
if (finalRedirectHref) {
redirectUri.searchParams.set('redirect_uri', finalRedirectHref);
}
const didToken = await getMagic().auth.loginWithMagicLink({
email: email,
redirectURI: new URL('/callback/', window.location.origin).href,
redirectURI: redirectUri.href,
});

if (didToken) {
Expand Down
2 changes: 1 addition & 1 deletion packages/website/pages/account/payment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ export function getStaticProps() {
props: {
title: 'Payment',
isRestricted: true,
redirectTo: '/login/',
redirectTo: '/login/?redirect_uri=/account/payment',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should just be /account/payment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redirectTo is where the http response will redirect to when the end-user isn't authenticated. This page is served at /account/payment. If redirect_to=/account/payment, it would be an infinite redirect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, I misunderstood what was happening there.

stripePublishableKey,
},
};
Expand Down
15 changes: 9 additions & 6 deletions packages/website/pages/callback/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ import { useEffect } from 'react';
import { useRouter } from 'next/router';
import { useQueryClient } from 'react-query';

import Loading from 'components/loading/loading';
import { redirectMagic, redirectSocial } from 'lib/magic.js';
import { redirectMagic, redirectSocial } from '../../lib/magic.js';
import Loading from '../../components/loading/loading.js';

export function getStaticProps() {
return {
Expand All @@ -16,13 +16,15 @@ export function getStaticProps() {
const Callback = () => {
const router = useRouter();
const queryClient = useQueryClient();

const redirectUriQuery = router.query.redirect_uri;
const redirectUri =
(redirectUriQuery && Array.isArray(redirectUriQuery) ? redirectUriQuery[0] : redirectUriQuery) ?? '/account';
useEffect(() => {
const finishSocialLogin = async () => {
try {
await redirectSocial();
await queryClient.invalidateQueries('magic-user');
router.push('/account');
router.push(redirectUri);
} catch (err) {
console.error(err);
await queryClient.invalidateQueries('magic-user');
Expand All @@ -33,7 +35,8 @@ const Callback = () => {
try {
await redirectMagic();
await queryClient.invalidateQueries('magic-user');
router.push('/account');

router.push(redirectUri);
} catch (err) {
console.error(err);
await queryClient.invalidateQueries('magic-user');
Expand All @@ -46,7 +49,7 @@ const Callback = () => {
if (router.query.provider) {
finishSocialLogin();
}
}, [router, router.query, queryClient]);
}, [router, router.query, queryClient, redirectUri]);

// TODO handle errors
return (
Expand Down
21 changes: 17 additions & 4 deletions packages/website/pages/login/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,24 @@ const LoginType = {
EMAIL: 'email',
};

/**
* Get the first thing
* @template T
* @param {T | T[] | undefined} thingOrThings
*/
function first(thingOrThings) {
if (Array.isArray(thingOrThings)) {
return thingOrThings[0];
}
return thingOrThings;
}

const Login = () => {
// App query client
const queryClient = useQueryClient();

// App wide methods
const { push } = useRouter();
const { push, query } = useRouter();

// Error states
const [errors, setErrors] = useState(/** @type {{email?: string}} */ ({}));
Expand All @@ -36,10 +48,11 @@ const Login = () => {
const onLoginWithEmail = useCallback(async () => {
setErrors({ email: undefined });
setIsLoggingIn(LoginType.EMAIL);
const finalRedirectUri = first(query.redirect_uri) ?? '/account';
try {
await loginEmail(email || '');
await loginEmail(email || '', finalRedirectUri);
await queryClient.invalidateQueries('magic-user');
push('/account');
push(finalRedirectUri);
} catch (error) {
setIsLoggingIn('');

Expand All @@ -48,7 +61,7 @@ const Login = () => {
// @ts-ignore Catch clause variable type annotation must be 'any' or 'unknown' if specified.ts(1196)
setErrors({ email: error.message });
}
}, [email, push, queryClient]);
}, [email, push, queryClient, query.redirect_uri]);

// Callback for github login logic
const onGithubLogin = useCallback(async () => {
Expand Down
56 changes: 28 additions & 28 deletions packages/website/tests/accountPayment.e2e.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,21 @@ test.beforeEach(async ({ page }) => {
});

test.describe('/account/payment', () => {
test('redirects to /login/ when not authenticated', async ({ page }, testInfo) => {
await Promise.all([
page.waitForNavigation({
waitUntil: 'networkidle',
}),
page.goto('/account/payment'),
]);
await expect(page).toHaveURL('/login/');
test('redirects through login and back when not initially authenticated', async ({ page }, testInfo) => {
const accountPaymentPathname = '/account/payment';
// try to go to page that requires authn
await page.goto(accountPaymentPathname, { 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);
// fill login
await LoginTester().login(page, {
email: MAGIC_SUCCESS_EMAIL,
});
// should be back at our initial target destination
expect(new URL(page.url()).pathname).toEqual(accountPaymentPathname + '/');
await page.screenshot({
fullPage: true,
path: await E2EScreenshotPath(testInfo, `accountPayment-noauth`),
Expand All @@ -27,35 +34,33 @@ test.describe('/account/payment', () => {
page.on('pageerror', err => {
console.error('pageerror', err);
});
const goToPayment = (page: Page) =>
Promise.all([
page.waitForNavigation({
waitUntil: 'networkidle',
}),
page.goto('/account/payment'),
]);
await goToPayment(page);
await LoginTester().login(page, {
const tester = LoginTester();
await page.goto(tester.loginHref);
await tester.login(page, {
email: MAGIC_SUCCESS_EMAIL,
});
// @todo - this should redirect you back to where you wanted to go: /account/payment
await expect(page).toHaveURL('/account/');
await goToPayment(page);
await page.goto('/account/payment/');
await expect(page).toHaveURL('/account/payment/');
await page.screenshot({
fullPage: true,
path: await E2EScreenshotPath(testInfo, `accountPayment`),
});
});
test('can enter credit card details', async ({ page }) => {
await LoginTester().login(page, { email: MAGIC_SUCCESS_EMAIL });
const tester = LoginTester();
await page.goto(tester.loginHref);
await tester.login(page, { email: MAGIC_SUCCESS_EMAIL });
await page.goto(AccountPaymentTester().url);
await AccountPaymentTester().fillCreditCardDetails(page);
await AccountPaymentTester().clickAddCardButton(page);
});
});

function LoginTester() {
function LoginTester({
loginHref = '/login/',
}: {
loginHref?: string;
} = {}) {
async function fillLoginForm(page: Page, email: string) {
await page.locator('.login-email').fill(email);
}
Expand All @@ -67,20 +72,15 @@ function LoginTester() {
async function login(
page: Page,
{
url = '/login/',
email,
}: {
url?: string;
email: string;
}
) {
if (new URL(page.url()).pathname !== url) {
await page.goto(url);
}
await fillLoginForm(page, email);
await submitLoginForm(page);
}
return { login, submitLoginForm };
return { login, loginHref, submitLoginForm };
}

function AccountPaymentTester() {
Expand Down