separate login and signup redirects

Author: ChristopherChudzicki

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)

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,49 +124,53 @@ 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"
     )
 
     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()
     # user should not be marked as completed onboarding
     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)
 

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(<ForbiddenPage />)
   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" }),
+        },
       }),
     )
   })

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])
 

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,
+        },
       }),
     )
   })

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 },
+      }),
     },
   },
 }

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`,
   )
 })

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