diff --git a/authentication/views.py b/authentication/views.py index d052d20593..396fbbcb4e 100644 --- a/authentication/views.py +++ b/authentication/views.py @@ -13,7 +13,7 @@ log = logging.getLogger(__name__) -def get_redirect_url(request): +def get_redirect_url(request, param_name): """ Get the redirect URL from the request. @@ -23,7 +23,7 @@ def get_redirect_url(request): Returns: str: Redirect URL """ - next_url = request.GET.get("next") or request.COOKIES.get("next") + next_url = request.GET.get(param_name) or request.COOKIES.get(param_name) return ( next_url if next_url @@ -49,7 +49,7 @@ def get( GET endpoint reached after logging a user out from Keycloak """ user = getattr(request, "user", None) - user_redirect_url = get_redirect_url(request) + user_redirect_url = get_redirect_url(request, "next") if user and user.is_authenticated: logout(request) if request.META.get(ApisixUserMiddleware.header): @@ -73,15 +73,18 @@ def get( """ GET endpoint for logging a user in. """ - redirect_url = get_redirect_url(request) - if not request.user.is_anonymous: + login_redirect_url = get_redirect_url(request, "next") + signup_redirect_url = get_redirect_url(request, "signup_next") + if ( + not request.user.is_anonymous + and not request.user.profile.completed_onboarding + ): profile = request.user.profile - if ( - not profile.completed_onboarding - and request.GET.get("skip_onboarding", "0") == "0" - ): - params = urlencode({"next": redirect_url}) - redirect_url = f"{settings.MITOL_NEW_USER_LOGIN_URL}?{params}" + if request.GET.get("skip_onboarding", "0") == "0": + params = urlencode({"next": signup_redirect_url}) + login_redirect_url = f"{settings.MITOL_NEW_USER_LOGIN_URL}?{params}" profile.completed_onboarding = True profile.save() - return redirect(redirect_url) + else: + return redirect(signup_redirect_url) + return redirect(login_redirect_url) diff --git a/authentication/views_test.py b/authentication/views_test.py index 71acfce998..63b1b15b2a 100644 --- a/authentication/views_test.py +++ b/authentication/views_test.py @@ -8,24 +8,32 @@ from django.conf import settings from django.test import RequestFactory from django.urls import reverse +from django.utils.http import urlencode from authentication.views import CustomLoginView, get_redirect_url @pytest.mark.parametrize( - ("next_url", "allowed"), + ("next_url", "allowed", "param_name"), [ - ("/app", True), - ("http://open.odl.local:8062/search", True), - ("http://open.odl.local:8069/search", False), - ("https://ocw.mit.edu", True), - ("https://fake.fake.edu", False), + ("/app", True, "next"), + ("http://open.odl.local:8062/search", True, "next"), + ("http://open.odl.local:8069/search", False, "next"), + ("https://ocw.mit.edu", True, "next"), + ("https://fake.fake.edu", False, "next"), + ("/app", True, "signup_next"), + ("http://open.odl.local:8062/search", True, "signup_next"), + ("http://open.odl.local:8069/search", False, "signup_next"), + ("https://ocw.mit.edu", True, "signup_next"), + ("https://fake.fake.edu", False, "signup_next"), ], ) -def test_custom_login(mocker, next_url, allowed): +def test_get_redirect_url(mocker, next_url, allowed, param_name): """Next url should be respected if host is allowed""" - mock_request = mocker.MagicMock(GET={"next": next_url}) - assert get_redirect_url(mock_request) == (next_url if allowed else "/app") + mock_request = mocker.MagicMock(GET={param_name: next_url}) + assert get_redirect_url(mock_request, param_name=param_name) == ( + next_url if allowed else "/app" + ) @pytest.mark.parametrize("has_apisix_header", [True, False]) @@ -116,13 +124,12 @@ def test_custom_logout_view(mocker, client, user, is_authenticated, has_next): def test_custom_login_view_authenticated_user_with_onboarding(mocker): """Test CustomLoginView for an authenticated user with incomplete onboarding""" factory = RequestFactory() - request = factory.get(reverse("login"), {"next": "/dashboard"}) + request = factory.get( + reverse("login"), + {"next": "/irrelevant", "signup_next": "/search?resource=184"}, + ) request.user = MagicMock(is_anonymous=False) request.user.profile = MagicMock(completed_onboarding=False) - mocker.patch("authentication.views.get_redirect_url", return_value="/dashboard") - mocker.patch( - "authentication.views.urlencode", return_value="next=/search?resource=184" - ) mocker.patch( "authentication.views.settings.MITOL_NEW_USER_LOGIN_URL", "/onboarding" ) @@ -130,18 +137,22 @@ def test_custom_login_view_authenticated_user_with_onboarding(mocker): response = CustomLoginView().get(request) assert response.status_code == 302 - assert response.url == "/onboarding?next=/search?resource=184" + assert response.url == f"/onboarding?{urlencode({'next': '/search?resource=184'})}" def test_custom_login_view_authenticated_user_skip_onboarding(mocker): """Test skip_onboarding flag skips redirect to onboarding and sets completed_onboarding""" factory = RequestFactory() request = factory.get( - reverse("login"), {"next": "/dashboard", "skip_onboarding": "1"} + reverse("login"), + { + "next": "/irrelevant", + "signup_next": "/search?resource=184", + "skip_onboarding": "1", + }, ) request.user = MagicMock(is_anonymous=False) request.user.profile = MagicMock(completed_onboarding=False) - mocker.patch("authentication.views.get_redirect_url", return_value="/dashboard") response = CustomLoginView().get(request) request.user.profile.refresh_from_db() @@ -149,16 +160,17 @@ def test_custom_login_view_authenticated_user_skip_onboarding(mocker): assert request.user.profile.completed_onboarding is False assert response.status_code == 302 - assert response.url == "/dashboard" + assert response.url == "/search?resource=184" def test_custom_login_view_authenticated_user_with_completed_onboarding(mocker): """Test test that user who has completed onboarding is redirected to next url""" factory = RequestFactory() - request = factory.get(reverse("login"), {"next": "/dashboard"}) + request = factory.get( + reverse("login"), {"next": "/dashboard", "signup_next": "/irrelevant"} + ) request.user = MagicMock(is_anonymous=False) request.user.profile = MagicMock(completed_onboarding=True) - mocker.patch("authentication.views.get_redirect_url", return_value="/dashboard") response = CustomLoginView().get(request) diff --git a/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.test.tsx b/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.test.tsx index 4e6f817b5f..252a9d1ca9 100644 --- a/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.test.tsx +++ b/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.test.tsx @@ -1,6 +1,6 @@ import React from "react" import { renderWithProviders, screen, waitFor } from "../../test-utils" -import { HOME, login } from "@/common/urls" +import * as routes from "@/common/urls" import ForbiddenPage from "./ForbiddenPage" import { setMockResponse, urls, factories } from "api/test-utils" import { useUserMe } from "api/hooks/user" @@ -22,7 +22,7 @@ test("The ForbiddenPage loads with a link that directs to HomePage", async () => setMockResponse.get(urls.userMe.get(), makeUser({ is_authenticated: true })) renderWithProviders() const homeLink = await screen.findByRole("link", { name: "Home" }) - expect(homeLink).toHaveAttribute("href", HOME) + expect(homeLink).toHaveAttribute("href", routes.HOME) }) test("Fetches auth data afresh and redirects unauthenticated users to auth", async () => { @@ -53,9 +53,11 @@ test("Fetches auth data afresh and redirects unauthenticated users to auth", asy await waitFor(() => { expect(mockedRedirect).toHaveBeenCalledWith( - login({ - pathname: "/foo", - searchParams: new URLSearchParams({ cat: "meow" }), + routes.auth({ + loginNext: { + pathname: "/foo", + searchParams: new URLSearchParams({ cat: "meow" }), + }, }), ) }) diff --git a/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.tsx b/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.tsx index ab2f834b35..3a0fcb9a80 100644 --- a/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.tsx +++ b/frontends/main/src/app-pages/ErrorPage/ForbiddenPage.tsx @@ -2,7 +2,7 @@ import React, { useEffect } from "react" import ErrorPageTemplate from "./ErrorPageTemplate" import { userQueries } from "api/hooks/user" import { useQuery } from "@tanstack/react-query" -import { redirectLoginToCurrent } from "@/common/utils" +import { redirectAuthToCurrent } from "@/common/utils" const ForbiddenPage: React.FC = () => { const user = useQuery({ @@ -15,7 +15,7 @@ const ForbiddenPage: React.FC = () => { useEffect(() => { if (shouldRedirect) { - redirectLoginToCurrent() + redirectAuthToCurrent() } }, [shouldRedirect]) diff --git a/frontends/main/src/app-pages/HomePage/HomePage.test.tsx b/frontends/main/src/app-pages/HomePage/HomePage.test.tsx index 6bfcbe16d5..f02f181fed 100644 --- a/frontends/main/src/app-pages/HomePage/HomePage.test.tsx +++ b/frontends/main/src/app-pages/HomePage/HomePage.test.tsx @@ -283,9 +283,11 @@ describe("Home Page personalize section", () => { const link = within(personalize).getByRole("link") expect(link).toHaveAttribute( "href", - routes.login({ - pathname: routes.DASHBOARD_HOME, - searchParams: null, + routes.auth({ + loginNext: { + pathname: routes.DASHBOARD_HOME, + searchParams: null, + }, }), ) }) diff --git a/frontends/main/src/app-pages/HomePage/PersonalizeSection.tsx b/frontends/main/src/app-pages/HomePage/PersonalizeSection.tsx index ed1238afb9..e1d2167ae6 100644 --- a/frontends/main/src/app-pages/HomePage/PersonalizeSection.tsx +++ b/frontends/main/src/app-pages/HomePage/PersonalizeSection.tsx @@ -76,7 +76,9 @@ const AUTH_TEXT_DATA = { text: "As a member, get personalized recommendations, curate learning lists, and follow your areas of interest.", linkProps: { children: "Sign Up for Free", - href: urls.login({ pathname: urls.DASHBOARD_HOME, searchParams: null }), + href: urls.auth({ + loginNext: { pathname: urls.DASHBOARD_HOME, searchParams: null }, + }), }, }, } diff --git a/frontends/main/src/common/urls.test.ts b/frontends/main/src/common/urls.test.ts index 3a762250ee..2f35dbdb49 100644 --- a/frontends/main/src/common/urls.test.ts +++ b/frontends/main/src/common/urls.test.ts @@ -1,27 +1,35 @@ -import { login } from "./urls" +import { auth } from "./urls" const MITOL_API_BASE_URL = process.env.NEXT_PUBLIC_MITOL_API_BASE_URL test("login encodes the next parameter appropriately", () => { - expect(login({ pathname: null, searchParams: null })).toBe( - `${MITOL_API_BASE_URL}/login?next=http://test.learn.odl.local:8062/`, + expect( + auth({ + loginNext: { pathname: null, searchParams: null }, + }), + ).toBe( + `${MITOL_API_BASE_URL}/login?next=http://test.learn.odl.local:8062/&signup_next=http://test.learn.odl.local:8062/dashboard`, ) expect( - login({ - pathname: "/foo/bar", - searchParams: null, + auth({ + loginNext: { + pathname: "/foo/bar", + searchParams: null, + }, }), ).toBe( - `${MITOL_API_BASE_URL}/login?next=http://test.learn.odl.local:8062/foo/bar`, + `${MITOL_API_BASE_URL}/login?next=http://test.learn.odl.local:8062/foo/bar&signup_next=http://test.learn.odl.local:8062/dashboard`, ) expect( - login({ - pathname: "/foo/bar", - searchParams: new URLSearchParams("?cat=meow"), + auth({ + loginNext: { + pathname: "/foo/bar", + searchParams: new URLSearchParams("?cat=meow"), + }, }), ).toBe( - `${MITOL_API_BASE_URL}/login?next=http://test.learn.odl.local:8062/foo/bar%3Fcat%3Dmeow`, + `${MITOL_API_BASE_URL}/login?next=http://test.learn.odl.local:8062/foo/bar%3Fcat%3Dmeow&signup_next=http://test.learn.odl.local:8062/dashboard`, ) }) diff --git a/frontends/main/src/common/urls.ts b/frontends/main/src/common/urls.ts index 1937457001..6a259c3ef2 100644 --- a/frontends/main/src/common/urls.ts +++ b/frontends/main/src/common/urls.ts @@ -55,40 +55,6 @@ if (process.env.NODE_ENV !== "production") { const MITOL_API_BASE_URL = process.env.NEXT_PUBLIC_MITOL_API_BASE_URL -export const LOGIN = `${MITOL_API_BASE_URL}/login` -export const LOGOUT = `${MITOL_API_BASE_URL}/logout/` - -/** - * Returns the URL to the login page, with a `next` parameter to redirect back - * to the given pathname + search parameters. - * - * NOTES: - * 1. useLoginToCurrent() is a convenience function that uses the current - * pathname and search parameters to generate the next URL. - * 2. `next` is required to encourage its use. You can explicitly pass `null` - * for values to skip them if desired. - */ -export const login = (next: { - pathname: string | null - searchParams: URLSearchParams | null - hash?: string | null -}) => { - const pathname = next.pathname ?? "/" - const searchParams = next.searchParams ?? new URLSearchParams() - const hash = next.hash ?? "" - /** - * To include search parameters in the next URL, we need to encode them. - * If we pass `?next=/foo/bar?cat=meow` directly, Django receives two separate - * parameters: `next` and `cat`. - * - * There's no need to encode the path parameter (it might contain slashes, - * but those are allowed in search parameters) so let's keep it readable. - */ - const search = searchParams?.toString() ? `?${searchParams.toString()}` : "" - const nextHref = `${ORIGIN}${pathname}${encodeURIComponent(search)}${encodeURIComponent(hash as string)}` - return `${LOGIN}?next=${nextHref}` -} - export const DASHBOARD_VIEW = "/dashboard/[tab]" const dashboardView = (tab: string) => generatePath(DASHBOARD_VIEW, { tab }) @@ -166,4 +132,59 @@ export const SEARCH_LEARNING_MATERIAL = querifiedSearchUrl({ resource_category: "learning_material", }) +export const LOGIN = `${MITOL_API_BASE_URL}/login` +export const LOGOUT = `${MITOL_API_BASE_URL}/logout/` + +type UrlDescriptor = { + pathname: string | null + searchParams: URLSearchParams | null + hash?: string | null +} +export type LoginUrlOpts = { + /** + * URL to redirect to after login. + */ + loginNext: UrlDescriptor + /** + * URL to redirect to after signup. + */ + signupNext?: UrlDescriptor +} + +const DEFAULT_SIGNUP_NEXT: UrlDescriptor = { + pathname: DASHBOARD_HOME, + searchParams: null, +} + +/** + * Returns the URL to the authentication page (login and signup). + * + * NOTES: + * 1. useAuthToCurrent() is a convenience function that uses the current + * pathname and search parameters to generate the next URL. + * 2. `next` is required to encourage its use. You can explicitly pass `null` + * for values to skip them if desired. + */ +export const auth = (opts: LoginUrlOpts) => { + const { loginNext, signupNext = DEFAULT_SIGNUP_NEXT } = opts + const encode = (value: UrlDescriptor) => { + const pathname = value.pathname ?? "/" + const searchParams = value.searchParams ?? new URLSearchParams() + const hash = value.hash ?? "" + /** + * To include search parameters in the next URL, we need to encode them. + * If we pass `?next=/foo/bar?cat=meow` directly, Django receives two separate + * parameters: `next` and `cat`. + * + * There's no need to encode the path parameter (it might contain slashes, + * but those are allowed in search parameters) so let's keep it readable. + */ + const search = searchParams?.toString() ? `?${searchParams.toString()}` : "" + return `${ORIGIN}${pathname}${encodeURIComponent(search)}${encodeURIComponent(hash)}` + } + const loginNextHref = encode(loginNext) + const signupNextHref = encode(signupNext) + return `${LOGIN}?next=${loginNextHref}&signup_next=${signupNextHref}` +} + export const ECOMMERCE_CART = "/cart/" as const diff --git a/frontends/main/src/common/utils.ts b/frontends/main/src/common/utils.ts index 886ccf5367..4003ef0feb 100644 --- a/frontends/main/src/common/utils.ts +++ b/frontends/main/src/common/utils.ts @@ -1,5 +1,5 @@ import { ChannelCounts } from "api/v0" -import { login } from "./urls" +import { auth } from "./urls" import { redirect, usePathname, useSearchParams } from "next/navigation" const getSearchParamMap = (urlParams: URLSearchParams) => { @@ -51,7 +51,13 @@ const getCsrfToken = () => { ) } -const useLoginToCurrent = () => { +/** + * Returns a URL to authentication that redirects to current page after auth. + * + * By default, new users will be redirected to default signup destination. + * If `signup` is true, they will be redirected to the current page. + */ +const useAuthToCurrent = ({ signup }: { signup?: boolean } = {}) => { /** * NOTES: * 1. This is reactive; if current URL changes, the result of this hook @@ -63,21 +69,39 @@ const useLoginToCurrent = () => { */ const pathname = usePathname() const searchParams = useSearchParams() - return login({ pathname, searchParams }) + const current = { pathname, searchParams } + return auth({ + loginNext: current, + signupNext: signup ? current : undefined, + }) } /** - * Redirect user to login route with ?next=. + * Redirect user to auth. After auth: + * - existing users redirected to their current URL + * - new users redirected to the default signup destination, unless `signup` is true. */ -const redirectLoginToCurrent = (): never => { +const redirectAuthToCurrent = ({ + signup, +}: { + /** + * Whether to redirect signup to current; by default, false. New users + * will be redirected to the default signup destination. + */ + signup?: boolean +} = {}): never => { + const current = { + pathname: window.location.pathname, + searchParams: new URLSearchParams(window.location.search), + } redirect( /** * Calculating the ?next= via window.location is appropriate * here since it happens time of redirect call. */ - login({ - pathname: window.location.pathname, - searchParams: new URLSearchParams(window.location.search), + auth({ + loginNext: current, + signupNext: signup ? current : undefined, }), ) } @@ -87,6 +111,6 @@ export { aggregateProgramCounts, aggregateCourseCounts, getCsrfToken, - useLoginToCurrent, - redirectLoginToCurrent, + useAuthToCurrent, + redirectAuthToCurrent, } diff --git a/frontends/main/src/components/RestrictedRoute/RestrictedRoute.tsx b/frontends/main/src/components/RestrictedRoute/RestrictedRoute.tsx index 4d2694c961..109507cb96 100644 --- a/frontends/main/src/components/RestrictedRoute/RestrictedRoute.tsx +++ b/frontends/main/src/components/RestrictedRoute/RestrictedRoute.tsx @@ -3,7 +3,7 @@ import React, { useEffect } from "react" import { ForbiddenError } from "@/common/errors" import { Permission, userQueries } from "api/hooks/user" -import { redirectLoginToCurrent } from "@/common/utils" +import { redirectAuthToCurrent } from "@/common/utils" import { useQuery } from "@tanstack/react-query" type RestrictedRouteProps = { @@ -54,7 +54,7 @@ const RestrictedRoute: React.FC = ({ * and any "secret" data is gated via API auth checks anyway. */ if (shouldRedirect) { - redirectLoginToCurrent() + redirectAuthToCurrent() } }, [shouldRedirect]) if (isLoading) return null diff --git a/frontends/main/src/page-components/Header/Header.test.tsx b/frontends/main/src/page-components/Header/Header.test.tsx index 158cc55491..379f7e49c5 100644 --- a/frontends/main/src/page-components/Header/Header.test.tsx +++ b/frontends/main/src/page-components/Header/Header.test.tsx @@ -60,9 +60,11 @@ describe("UserMenu", () => { test("Unauthenticated users see the Sign Up / Login link", async () => { const isAuthenticated = false const initialUrl = "/foo/bar?cat=meow" - const expectedUrl = urlConstants.login({ - pathname: "/foo/bar", - searchParams: new URLSearchParams("?cat=meow"), + const expectedUrl = urlConstants.auth({ + loginNext: { + pathname: "/foo/bar", + searchParams: new URLSearchParams("?cat=meow"), + }, }) setMockResponse.get(urls.userMe.get(), { is_authenticated: isAuthenticated, diff --git a/frontends/main/src/page-components/Header/UserMenu.tsx b/frontends/main/src/page-components/Header/UserMenu.tsx index 4bbb21bf1c..41c7916830 100644 --- a/frontends/main/src/page-components/Header/UserMenu.tsx +++ b/frontends/main/src/page-components/Header/UserMenu.tsx @@ -12,7 +12,7 @@ import { } from "@remixicon/react" import { useUserMe, User } from "api/hooks/user" import MITLogoLink from "@/components/MITLogoLink/MITLogoLink" -import { useLoginToCurrent } from "@/common/utils" +import { useAuthToCurrent } from "@/common/utils" const FlexContainer = styled.div({ display: "flex", @@ -125,7 +125,7 @@ type UserMenuProps = { const UserMenu: React.FC = ({ variant }) => { const [visible, setVisible] = useState(false) - const loginUrl = useLoginToCurrent() + const loginUrl = useAuthToCurrent() const { isLoading, data: user } = useUserMe() if (isLoading) { diff --git a/frontends/main/src/page-components/SignupPopover/SignupPopover.test.tsx b/frontends/main/src/page-components/SignupPopover/SignupPopover.test.tsx index 98f8917064..0e9b9cac6f 100644 --- a/frontends/main/src/page-components/SignupPopover/SignupPopover.test.tsx +++ b/frontends/main/src/page-components/SignupPopover/SignupPopover.test.tsx @@ -15,9 +15,15 @@ test("SignupPopover shows link to sign up", async () => { const link = within(dialog).getByRole("link") invariant(link instanceof HTMLAnchorElement) expect(link.href).toMatch( - urls.login({ - pathname: "/some-path", - searchParams: new URLSearchParams("dog=woof"), + urls.auth({ + loginNext: { + pathname: "/some-path", + searchParams: new URLSearchParams("dog=woof"), + }, + signupNext: { + pathname: "/some-path", + searchParams: new URLSearchParams("dog=woof"), + }, }), ) }) diff --git a/frontends/main/src/page-components/SignupPopover/SignupPopover.tsx b/frontends/main/src/page-components/SignupPopover/SignupPopover.tsx index 0ba15d563f..b38f12bff6 100644 --- a/frontends/main/src/page-components/SignupPopover/SignupPopover.tsx +++ b/frontends/main/src/page-components/SignupPopover/SignupPopover.tsx @@ -2,7 +2,7 @@ import React from "react" import { Popover, Typography, styled } from "ol-components" import { ButtonLink } from "@mitodl/smoot-design" import type { PopoverProps } from "ol-components" -import { useLoginToCurrent } from "@/common/utils" +import { useAuthToCurrent } from "@/common/utils" const StyledPopover = styled(Popover)({ width: "300px", @@ -31,7 +31,7 @@ type SignupPopoverProps = Pick< "anchorEl" | "onClose" | "placement" > const SignupPopover: React.FC = (props) => { - const loginUrl = useLoginToCurrent() + const loginUrl = useAuthToCurrent({ signup: true }) return ( diff --git a/pytest.ini b/pytest.ini index 44c42e250e..5aeb14fcd3 100644 --- a/pytest.ini +++ b/pytest.ini @@ -20,3 +20,4 @@ env = POSTHOG_PERSONAL_API_KEY=fake_key # pragma: allowlist secret POSTHOG_PROJECT_API_KEY=fake_key # pragma: allowlist secret POSTHOG_PROJECT_ID=1234 + ALLOWED_REDIRECT_HOSTS=["ocw.mit.edu","open.odl.local:8062"]