Skip to content

Commit c4da25f

Browse files
refactor login url more; fix tests
1 parent 82fd1a9 commit c4da25f

File tree

5 files changed

+116
-43
lines changed

5 files changed

+116
-43
lines changed

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

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,8 @@
11
import React, { useEffect } from "react"
22
import ErrorPageTemplate from "./ErrorPageTemplate"
33
import { userQueries } from "api/hooks/user"
4-
import { redirect } from "next/navigation"
54
import { useQuery } from "@tanstack/react-query"
6-
import { useLoginToCurrent } from "@/common/utils"
5+
import { redirectLoginToCurrent } from "@/common/utils"
76

87
const ForbiddenPage: React.FC = () => {
98
const user = useQuery({
@@ -14,12 +13,11 @@ const ForbiddenPage: React.FC = () => {
1413
const shouldRedirect =
1514
user.isFetchedAfterMount && !user.data?.is_authenticated
1615

17-
const loginUrl = useLoginToCurrent()
1816
useEffect(() => {
1917
if (shouldRedirect) {
20-
redirect(loginUrl)
18+
redirectLoginToCurrent()
2119
}
22-
}, [shouldRedirect, loginUrl])
20+
}, [shouldRedirect])
2321

2422
return (
2523
<ErrorPageTemplate

frontends/main/src/common/utils.ts

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { ChannelCounts } from "api/v0"
22
import { login } from "./urls"
3-
import { usePathname, useSearchParams } from "next/navigation"
3+
import { redirect, usePathname, useSearchParams } from "next/navigation"
44

55
const getSearchParamMap = (urlParams: URLSearchParams) => {
66
const params: Record<string, string[] | string> = {}
@@ -53,24 +53,40 @@ const getCsrfToken = () => {
5353

5454
const useLoginToCurrent = () => {
5555
/**
56-
* NOTE: In contrast to, say
57-
* login({
58-
* pathname: window.location.pathname,
59-
* searchParams: new URLSearchParams(window.location.search)
60-
* }),
61-
* the version here is reactive: when pathname/searchParams change, the values
62-
* here update automatically and will (appropriately) trigger a re-render.
56+
* NOTES:
57+
* 1. This is reactive; if current URL changes, the result of this hook
58+
* will update and trigger re-renders. This makes it suitable for use as
59+
* an href.
60+
* 2. However, the use of search params / pathname hooks may require Suspense
61+
* or NextJS's dynamic rendering.
6362
*
6463
*/
6564
const pathname = usePathname()
6665
const searchParams = useSearchParams()
6766
return login({ pathname, searchParams })
6867
}
6968

69+
/**
70+
* Redirect user to login route with ?next=<current-url>.
71+
*/
72+
const redirectLoginToCurrent = (): never => {
73+
redirect(
74+
/**
75+
* Calculating the ?next=<current-url> via window.location is appropriate
76+
* here since it happens time of redirect call.
77+
*/
78+
login({
79+
pathname: window.location.pathname,
80+
searchParams: new URLSearchParams(window.location.search),
81+
}),
82+
)
83+
}
84+
7085
export {
7186
getSearchParamMap,
7287
aggregateProgramCounts,
7388
aggregateCourseCounts,
7489
getCsrfToken,
7590
useLoginToCurrent,
91+
redirectLoginToCurrent,
7692
}
Lines changed: 29 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,42 +1,51 @@
11
import React from "react"
2-
import { renderWithProviders, screen } from "../../test-utils"
2+
import {
3+
renderWithProviders,
4+
screen,
5+
TestingErrorBoundary,
6+
waitFor,
7+
} from "../../test-utils"
38
import RestrictedRoute from "./RestrictedRoute"
49
import { Permission } from "api/hooks/user"
510
import { allowConsoleErrors } from "ol-test-utilities"
11+
import { setMockResponse, urls, factories } from "api/test-utils"
12+
import { ForbiddenError } from "@/common/errors"
613

7-
test("Renders children if permission check satisfied", () => {
14+
const makeUser = factories.user.user
15+
16+
test("Renders children if permission check satisfied", async () => {
817
const errors: unknown[] = []
18+
setMockResponse.get(urls.userMe.get(), makeUser({ is_authenticated: true }))
919

1020
renderWithProviders(
1121
<RestrictedRoute requires={Permission.Authenticated}>
1222
Hello, world!
1323
</RestrictedRoute>,
14-
15-
{
16-
user: { [Permission.Authenticated]: true },
17-
},
1824
)
1925

20-
screen.getByText("Hello, world!")
26+
await screen.findByText("Hello, world!")
2127
expect(!errors.length).toBe(true)
2228
})
2329

24-
test.each(Object.values(Permission))(
30+
test.each(
31+
Object.values(Permission).filter((p) => p !== Permission.Authenticated),
32+
)(
2533
"Throws error if and only if lacking required permission",
2634
async (permission) => {
27-
// if a user is not authenticated they are redirected to login before an error is thrown
28-
if (permission === Permission.Authenticated) {
29-
return
30-
}
3135
allowConsoleErrors()
36+
setMockResponse.get(urls.userMe.get(), makeUser({ [permission]: false }))
37+
38+
const onError = jest.fn()
39+
renderWithProviders(
40+
<TestingErrorBoundary onError={onError}>
41+
<RestrictedRoute requires={permission}>Hello, world!</RestrictedRoute>
42+
</TestingErrorBoundary>,
43+
)
3244

33-
expect(() =>
34-
renderWithProviders(
35-
<RestrictedRoute requires={permission}>Hello, world!</RestrictedRoute>,
36-
{
37-
user: { [permission]: false },
38-
},
39-
),
40-
).toThrow("Not allowed.")
45+
await waitFor(() => {
46+
expect(onError).toHaveBeenCalled()
47+
})
48+
const error = onError.mock.calls[0][0]
49+
expect(error).toEqual(new ForbiddenError("Not allowed."))
4150
},
4251
)

frontends/main/src/components/RestrictedRoute/RestrictedRoute.tsx

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
"use client"
22

3-
import React from "react"
3+
import React, { useEffect } from "react"
44
import { ForbiddenError } from "@/common/errors"
5-
import { Permission, useUserMe } from "api/hooks/user"
6-
import { redirect } from "next/navigation"
7-
import { useLoginToCurrent } from "@/common/utils"
5+
import { Permission, userQueries } from "api/hooks/user"
6+
import { redirectLoginToCurrent } from "@/common/utils"
7+
import { useQuery } from "@tanstack/react-query"
88

99
type RestrictedRouteProps = {
1010
children?: React.ReactNode
@@ -41,13 +41,24 @@ const RestrictedRoute: React.FC<RestrictedRouteProps> = ({
4141
children,
4242
requires,
4343
}) => {
44-
const { isLoading, data: user } = useUserMe()
45-
const loginUrl = useLoginToCurrent()
44+
const { isLoading, data: user } = useQuery({
45+
...userQueries.me(),
46+
staleTime: 0, // Force refetch on mount
47+
})
48+
const shouldRedirect = !isLoading && !user?.is_authenticated
49+
useEffect(() => {
50+
/**
51+
* Note: If user data exists in query cache, user might see content
52+
* while refetching fresh user data to verify auth.
53+
* This is optimistic: since the cached data will almost always be valid
54+
* and any "secret" data is gated via API auth checks anyway.
55+
*/
56+
if (shouldRedirect) {
57+
redirectLoginToCurrent()
58+
}
59+
}, [shouldRedirect])
4660
if (isLoading) return null
47-
if (!user?.is_authenticated) {
48-
redirect(loginUrl)
49-
return null
50-
}
61+
if (shouldRedirect) return null
5162
if (!isLoading && !user?.[requires]) {
5263
// This error should be caught by an [`errorElement`](https://reactrouter.com/en/main/route/error-element).
5364
throw new ForbiddenError("Not allowed.")

frontends/main/src/test-utils/index.tsx

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,44 @@ const assertPartialMetas = (expected: Partial<TestableMetas>) => {
226226
)
227227
}
228228

229+
type ErrorBoundaryProps = {
230+
onError?: (error: unknown) => void
231+
children?: React.ReactNode
232+
}
233+
type ErrorBoundaryState = { hasError: boolean }
234+
/**
235+
* Useful in rare circumstances to test an error throw during subsequent
236+
* renders:
237+
*
238+
* const Fallback = jest.fn()
239+
* renderWithProviders(
240+
* <TestingErrorBoundary fallback={<Fallback />}>
241+
* <ComponentThatThrows />
242+
* </TestingErrorBoundary>
243+
* )
244+
*/
245+
class TestingErrorBoundary extends React.Component<
246+
ErrorBoundaryProps,
247+
ErrorBoundaryState
248+
> {
249+
constructor(props: ErrorBoundaryProps) {
250+
super(props)
251+
this.state = { hasError: false }
252+
}
253+
254+
static getDerivedStateFromError() {
255+
return { hasError: true }
256+
}
257+
258+
componentDidCatch(error: Error): void {
259+
this.props.onError?.(error)
260+
}
261+
262+
render() {
263+
return this.state.hasError ? null : this.props.children
264+
}
265+
}
266+
229267
export {
230268
renderWithProviders,
231269
renderWithTheme,
@@ -235,6 +273,7 @@ export {
235273
ignoreError,
236274
getMetas,
237275
assertPartialMetas,
276+
TestingErrorBoundary,
238277
}
239278
// Conveniences
240279
export { setMockResponse }

0 commit comments

Comments
 (0)