Handle stale auth better

frontends/api/src/hooks/user/index.ts

@@ -1,23 +1,14 @@
 import { useQuery } from "@tanstack/react-query"
-import { usersApi } from "../../clients"
 import type { User } from "../../generated/v0/api"
+import { userQueries } from "./queries"
 
 enum Permission {
   ArticleEditor = "is_article_editor",
   Authenticated = "is_authenticated",
   LearningPathEditor = "is_learning_path_editor",
 }
 
-const useUserMe = () =>
-  useQuery({
-    queryKey: ["userMe"],
-    queryFn: async (): Promise<User> => {
-      const response = await usersApi.usersMeRetrieve()
-      return {
-        ...response.data,
-      }
-    },
-  })
+const useUserMe = () => useQuery(userQueries.me())
 
 const useUserIsAuthenticated = () => {
   const { data: user } = useUserMe()
@@ -29,5 +20,11 @@ const useUserHasPermission = (permission: Permission) => {
   return !!user?.[permission]
 }
 
-export { useUserMe, useUserIsAuthenticated, useUserHasPermission, Permission }
+export {
+  userQueries,
+  useUserMe,
+  useUserIsAuthenticated,
+  useUserHasPermission,
+  Permission,
+}
 export type { User }

frontends/api/src/hooks/user/queries.ts

@@ -0,0 +1,22 @@
+import { queryOptions } from "@tanstack/react-query"
+import { usersApi } from "../../clients"
+
+const userKeys = {
+  root: ["users"],
+  me: () => [...userKeys.root, "me"],
+}
+
+const userQueries = {
+  me: () =>
+    queryOptions({
+      queryKey: userKeys.me(),
+      queryFn: () => usersApi.usersMeRetrieve().then((res) => res.data),
+      /**
+       * Always refetch on window focus in case the user has logged in or out
+       * in another tab or window while this tab was inactive.
+       */
+      refetchOnWindowFocus: "always",
+    }),
+}
+
+export { userQueries, userKeys }

frontends/api/src/ssr/usePrefetchWarnings.ts

@@ -1,5 +1,6 @@
 import { useEffect } from "react"
 import type { Query, QueryClient, QueryKey } from "@tanstack/react-query"
+import { userQueries } from "../hooks/user/queries"
 
 const logQueries = (...args: [...string[], Query[]]) => {
   const queries = args.pop() as Query[]
@@ -16,7 +17,7 @@ const logQueries = (...args: [...string[], Query[]]) => {
   )
 }
 
-const PREFETCH_EXEMPT_QUERIES = [["userMe"]]
+const PREFETCH_EXEMPT_QUERIES = [userQueries.me().queryKey]
 
 /**
  * Call this as high as possible in render tree to detect query usage on

frontends/api/src/test-utils/factories/user.ts

@@ -25,6 +25,17 @@ const profile: PartialFactory<Profile> = (overrides = {}): Profile => ({
 const enforcerId = new UniqueEnforcer()
 
 const user: PartialFactory<User> = (overrides = {}): User => {
+  if (overrides?.is_authenticated === false) {
+    return {
+      is_authenticated: false,
+      is_article_editor: false,
+      is_learning_path_editor: false,
+      // @ts-expect-error API Response can include anonymous user
+      id: null,
+      username: "",
+    }
+  }
+
   const result: User = {
     id: enforcerId.enforce(faker.number.int),
     first_name: faker.person.firstName(),

frontends/main/src/app-pages/ErrorPage/ErrorPageTemplate.tsx

@@ -1,5 +1,5 @@
 import React from "react"
-import { Typography, styled } from "ol-components"
+import { Container, Skeleton, Typography, styled } from "ol-components"
 import { ButtonLink } from "@mitodl/smoot-design"
 import { HOME } from "@/common/urls"
 import Image from "next/image"
@@ -10,9 +10,10 @@ import timSpeechBubble from "@/public/images/tim_speech_bubble.svg"
 type ErrorPageTemplateProps = {
   title: string
   timSays?: string
+  loading?: boolean
 }
 
-const Page = styled.div(({ theme }) => ({
+const Page = styled(Container)(({ theme }) => ({
   backgroundImage: `url(${backgroundImage.src})`,
   backgroundAttachment: "fixed",
   backgroundRepeat: "no-repeat",
@@ -83,7 +84,29 @@ const Button = styled(ButtonLink)({
 const ErrorPageTemplate: React.FC<ErrorPageTemplateProps> = ({
   title,
   timSays,
+  loading = false,
 }) => {
+  if (loading) {
+    return (
+      <Page>
+        <ErrorContainer>
+          <ImageContainer>
+            <Skeleton width="100%" height="100%" />
+          </ImageContainer>
+          <Typography
+            component="h1"
+            variant="h3"
+            sx={{ textAlign: "center", width: "50%" }}
+          >
+            <Skeleton />
+          </Typography>
+          <Footer>
+            <Skeleton width="200px" height="40px" />
+          </Footer>
+        </ErrorContainer>
+      </Page>
+    )
+  }
   return (
     <Page>
       <ErrorContainer>

frontends/main/src/app-pages/ErrorPage/ForbiddenPage.test.tsx

@@ -1,44 +1,62 @@
 import React from "react"
-import { renderWithProviders, screen } from "../../test-utils"
-import { HOME } from "@/common/urls"
+import { renderWithProviders, screen, waitFor } from "../../test-utils"
+import { HOME, login } from "@/common/urls"
 import ForbiddenPage from "./ForbiddenPage"
-import { setMockResponse, urls } from "api/test-utils"
-import { Permission } from "api/hooks/user"
+import { setMockResponse, urls, factories } from "api/test-utils"
+import { useUserMe } from "api/hooks/user"
+import { redirect } from "next/navigation"
 
-const oldWindowLocation = window.location
+const mockedRedirect = jest.mocked(redirect)
 
-beforeAll(() => {
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  delete (window as any).location
+const makeUser = factories.user.user
 
-  window.location = Object.defineProperties({} as Location, {
-    ...Object.getOwnPropertyDescriptors(oldWindowLocation),
-    assign: {
-      configurable: true,
-      value: jest.fn(),
-    },
+test("The ForbiddenPage loads with Correct Title", async () => {
+  setMockResponse.get(urls.userMe.get(), makeUser({ is_authenticated: true }))
+  renderWithProviders(<ForbiddenPage />)
+  await screen.findByRole("heading", {
+    name: "You do not have permission to access this resource.",
   })
 })
 
-afterAll(() => {
-  window.location = oldWindowLocation
+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)
 })
 
-test("The ForbiddenPage loads with Correct Title", () => {
-  setMockResponse.get(urls.userMe.get(), {
-    [Permission.Authenticated]: true,
-  })
-  renderWithProviders(<ForbiddenPage />)
-  screen.getByRole("heading", {
-    name: "You do not have permission to access this resource.",
+test("Fetches auth data afresh and redirects unauthenticated users to auth", async () => {
+  const user = makeUser()
+  setMockResponse.get(urls.userMe.get(), user)
+
+  const FakeHeader = ({ children }: { children?: React.ReactNode }) => {
+    const user = useUserMe()
+    return (
+      <div>
+        {user.data?.first_name}
+        {children}
+      </div>
+    )
+  }
+  const { view } = renderWithProviders(<FakeHeader />, {
+    url: "/foo?cat=meow",
   })
-})
+  await screen.findByText(user.first_name)
+
+  setMockResponse.get(urls.userMe.get(), makeUser({ is_authenticated: false }))
 
-test("The ForbiddenPage loads with a link that directs to HomePage", () => {
-  setMockResponse.get(urls.userMe.get(), {
-    [Permission.Authenticated]: true,
+  view.rerender(
+    <FakeHeader>
+      <ForbiddenPage />
+    </FakeHeader>,
+  )
+
+  await waitFor(() => {
+    expect(mockedRedirect).toHaveBeenCalledWith(
+      login({
+        pathname: "/foo",
+        searchParams: new URLSearchParams({ cat: "meow" }),
+      }),
+    )
   })
-  renderWithProviders(<ForbiddenPage />)
-  const homeLink = screen.getByRole("link", { name: "Home" })
-  expect(homeLink).toHaveAttribute("href", HOME)
 })

frontends/main/src/app-pages/ErrorPage/ForbiddenPage.tsx

@@ -1,20 +1,30 @@
 import React, { useEffect } from "react"
 import ErrorPageTemplate from "./ErrorPageTemplate"
-import { useUserMe } from "api/hooks/user"
-import { redirect } from "next/navigation"
-import * as urls from "@/common/urls"
+import { userQueries } from "api/hooks/user"
+import { useQuery } from "@tanstack/react-query"
+import { redirectLoginToCurrent } from "@/common/utils"
 
 const ForbiddenPage: React.FC = () => {
-  const { data: user } = useUserMe()
+  const user = useQuery({
+    ...userQueries.me(),
+    staleTime: 0, // Force refetch on mount
+  })
+
+  const shouldRedirect =
+    user.isFetchedAfterMount && !user.data?.is_authenticated
 
   useEffect(() => {
-    if (!user?.is_authenticated) {
-      const loginUrl = urls.login()
-      redirect(loginUrl)
+    if (shouldRedirect) {
+      redirectLoginToCurrent()
     }
-  }, [user])
+  }, [shouldRedirect])
+
   return (
-    <ErrorPageTemplate title="You do not have permission to access this resource." />
+    <ErrorPageTemplate
+      // Continue to show loading state while redirecting
+      loading={!user.isFetchedAfterMount || shouldRedirect}
+      title="You do not have permission to access this resource."
+    />
   )
 }
 

frontends/main/src/app-pages/HomePage/HomePage.test.tsx

@@ -285,6 +285,7 @@ describe("Home Page personalize section", () => {
       "href",
       routes.login({
         pathname: routes.DASHBOARD_HOME,
+        searchParams: null,
       }),
     )
   })

frontends/main/src/app-pages/HomePage/PersonalizeSection.tsx

@@ -76,7 +76,7 @@ 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 }),
+      href: urls.login({ pathname: urls.DASHBOARD_HOME, searchParams: null }),
     },
   },
 }

frontends/main/src/app/error.test.tsx

@@ -3,6 +3,9 @@ import { renderWithProviders, screen } from "@/test-utils"
 import { HOME } from "@/common/urls"
 import ErrorPage from "./error"
 import { ForbiddenError } from "@/common/errors"
+import { setMockResponse, urls, factories } from "api/test-utils"
+
+const makeUser = factories.user.user
 
 test("The error page shows error message", () => {
   const error = new Error("Ruh roh")
@@ -13,10 +16,12 @@ test("The error page shows error message", () => {
   expect(homeLink).toHaveAttribute("href", HOME)
 })
 
-test("The NotFoundPage loads with a link that directs to HomePage", () => {
+test("The Forbidden loads with a link that directs to HomePage", async () => {
   const error = new ForbiddenError()
-  renderWithProviders(<ErrorPage error={error} />, { user: {} })
-  screen.getByRole("heading", {
+  setMockResponse.get(urls.userMe.get(), makeUser())
+
+  renderWithProviders(<ErrorPage error={error} />)
+  await screen.findByRole("heading", {
     name: "You do not have permission to access this resource.",
   })
   screen.getByText("Oops!")

frontends/main/src/app/getQueryClient.ts

@@ -19,7 +19,6 @@ const makeQueryClient = (): QueryClient => {
   return new QueryClient({
     defaultOptions: {
       queries: {
-        refetchOnWindowFocus: false,
         staleTime: Infinity,
 
         /**

frontends/main/src/common/urls.test.ts

@@ -3,16 +3,14 @@ import { login } from "./urls"
 const MITOL_API_BASE_URL = process.env.NEXT_PUBLIC_MITOL_API_BASE_URL
 
 test("login encodes the next parameter appropriately", () => {
-  expect(login()).toBe(
-    `${MITOL_API_BASE_URL}/login?next=http://test.learn.odl.local:8062/`,
-  )
-  expect(login({})).toBe(
+  expect(login({ pathname: null, searchParams: null })).toBe(
     `${MITOL_API_BASE_URL}/login?next=http://test.learn.odl.local:8062/`,
   )
 
   expect(
     login({
       pathname: "/foo/bar",
+      searchParams: null,
     }),
   ).toBe(
     `${MITOL_API_BASE_URL}/login?next=http://test.learn.odl.local:8062/foo/bar`,

frontends/main/src/common/urls.ts

@@ -61,16 +61,21 @@ 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 = ({
-  pathname = "/",
-  searchParams = new URLSearchParams(),
-  hash = "",
-}: {
-  pathname?: string | null
-  searchParams?: URLSearchParams
+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
@@ -79,9 +84,9 @@ export const login = ({
    * 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 next = `${ORIGIN}${pathname}${encodeURIComponent(search)}${encodeURIComponent(hash as string)}`
-  return `${LOGIN}?next=${next}`
+  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]"