From c6b1668364e7f12f9cdc6b62e410c1124d4585c7 Mon Sep 17 00:00:00 2001
From: Kris Coulson <KrisCoulson@gmail.com>
Date: Tue, 19 Sep 2023 10:54:06 -0700
Subject: [PATCH] Fix infinite loops on nested private routes with roles
 (#9204)

Fix for #9131

This handles use cases where there are multiple nested routes with
different levels of authentication and redirect contexts

---------

Co-authored-by: Daniel Choudhury <dannychoudhury@gmail.com>
---
 .../src/__tests__/analyzeRoutes.test.tsx      | 109 +++++++--
 packages/router/src/__tests__/router.test.tsx | 220 +++++++++++++++++-
 packages/router/src/router.tsx                |  48 ++--
 packages/router/src/util.ts                   |  24 +-
 4 files changed, 348 insertions(+), 53 deletions(-)

diff --git a/packages/router/src/__tests__/analyzeRoutes.test.tsx b/packages/router/src/__tests__/analyzeRoutes.test.tsx
index 8da79e36ce1d..95a8b3426d54 100644
--- a/packages/router/src/__tests__/analyzeRoutes.test.tsx
+++ b/packages/router/src/__tests__/analyzeRoutes.test.tsx
@@ -144,10 +144,12 @@ describe('AnalyzeRoutes: with homePage and Children', () => {
         whileLoadingPage: undefined,
         wrappers: [WrapperX],
         // Props passed through from set
-        setProps: {
-          id: 'set-one',
-          passThruProp: 'bazinga',
-        },
+        setProps: [
+          {
+            id: 'set-one',
+            passThruProp: 'bazinga',
+          },
+        ],
       })
     )
 
@@ -158,11 +160,16 @@ describe('AnalyzeRoutes: with homePage and Children', () => {
         path: '/b',
         whileLoadingPage: undefined,
         wrappers: [WrapperX, WrapperY], // both wrappers
-        setProps: {
-          id: 'set-two', // the id gets overwritten by the second Set
-          theme: 'blue',
-          passThruProp: 'bazinga', // from the first set
-        },
+        setProps: [
+          {
+            id: 'set-one',
+            passThruProp: 'bazinga',
+          },
+          {
+            id: 'set-two',
+            theme: 'blue',
+          },
+        ],
       })
     )
 
@@ -173,11 +180,16 @@ describe('AnalyzeRoutes: with homePage and Children', () => {
         path: '/c',
         whileLoadingPage: undefined,
         wrappers: [WrapperX, WrapperY], // both wrappers
-        setProps: {
-          id: 'set-two',
-          theme: 'blue',
-          passThruProp: 'bazinga', // from the first set
-        },
+        setProps: [
+          {
+            id: 'set-one',
+            passThruProp: 'bazinga',
+          },
+          {
+            id: 'set-two',
+            theme: 'blue',
+          },
+        ],
       })
     )
   })
@@ -276,10 +288,12 @@ describe('AnalyzeRoutes: with homePage and Children', () => {
       page: FakePage,
       wrappers: [],
       setId: 1,
-      setProps: {
-        private: true,
-        unauthenticated: 'home',
-      },
+      setProps: [
+        {
+          private: true,
+          unauthenticated: 'home',
+        },
+      ],
     })
   })
 
@@ -310,4 +324,63 @@ describe('AnalyzeRoutes: with homePage and Children', () => {
     expect(namedRoutesMap.simple()).toBe('/simple')
     expect(namedRoutesMap.rdSimple()).toBe('/rdSimple')
   })
+
+  test('Redirect routes analysis', () => {
+    const HomePage = () => <h1>Home Page</h1>
+    const PrivateAdminPage = () => <h1>Private Admin Page</h1>
+    const PrivateEmployeePage = () => <h1>Private Employee Page</h1>
+    const PrivateNoRolesAssigned = () => <h1>Private Employee Page</h1>
+
+    const RedirectedRoutes = (
+      <Router>
+        <Route path="/" page={HomePage} name="home" />
+        <Private unauthenticated="home">
+          <Route
+            path="/no-roles-assigned"
+            page={PrivateNoRolesAssigned}
+            name="noRolesAssigned"
+          />
+          <Set
+            private
+            unauthenticated="noRolesAssigned"
+            roles={['ADMIN', 'EMPLOYEE']}
+            someProp="propFromNoRolesSet"
+          >
+            <Private unauthenticated="admin" roles={'EMPLOYEE'}>
+              <Route
+                path="/employee"
+                page={PrivateEmployeePage}
+                name="privateEmployee"
+              />
+            </Private>
+
+            <Private unauthenticated="employee" roles={'ADMIN'}>
+              <Route
+                path="/admin"
+                page={PrivateAdminPage}
+                name="privateAdmin"
+              />
+            </Private>
+          </Set>
+        </Private>
+      </Router>
+    )
+
+    const { pathRouteMap, namedRoutesMap } = analyzeRoutes(
+      RedirectedRoutes.props.children,
+      {
+        currentPathName: '/simple',
+      }
+    )
+
+    console.log(JSON.stringify({ pathRouteMap, namedRoutesMap }, null, 3))
+
+    // expect(pathRouteMap['/simple'].redirect).toBe('/rdSimple')
+    // expect(pathRouteMap['/rdSimple'].redirect).toBeFalsy()
+
+    // // @TODO true for now, but we may not allow names on a redirect route
+    // expect(Object.keys(namedRoutesMap).length).toBe(2)
+    // expect(namedRoutesMap.simple()).toBe('/simple')
+    // expect(namedRoutesMap.rdSimple()).toBe('/rdSimple')
+  })
 })
diff --git a/packages/router/src/__tests__/router.test.tsx b/packages/router/src/__tests__/router.test.tsx
index ec00943cbac9..a44c738f99cb 100644
--- a/packages/router/src/__tests__/router.test.tsx
+++ b/packages/router/src/__tests__/router.test.tsx
@@ -20,32 +20,32 @@ jest.mock('../util', () => {
 
 import React, { useEffect, useState } from 'react'
 
+import '@testing-library/jest-dom/extend-expect'
 import {
-  render,
-  waitFor,
   act,
-  fireEvent,
   configure,
+  fireEvent,
+  render,
+  waitFor,
 } from '@testing-library/react'
-import '@testing-library/jest-dom/extend-expect'
 
 import type { AuthContextInterface } from '@redwoodjs/auth'
 
 import {
-  Router,
-  Route,
-  Private,
-  Redirect,
+  back,
   routes as generatedRoutes,
   Link,
   navigate,
-  back,
+  Private,
+  Redirect,
+  Route,
+  Router,
   usePageLoadingContext,
 } from '../'
 import { useLocation } from '../location'
 import { useParams } from '../params'
 import { Set } from '../Set'
-import type { Spec, GeneratedRoutesMap } from '../util'
+import type { GeneratedRoutesMap, Spec } from '../util'
 
 /** running into intermittent test timeout behavior in https://github.com/redwoodjs/redwood/pull/4992
  attempting to work around by bumping the default timeout of 5000 */
@@ -98,7 +98,7 @@ function createDummyAuthContextValues(
 interface MockAuth {
   isAuthenticated?: boolean
   loading?: boolean
-  hasRole?: boolean
+  hasRole?: boolean | ((role: string[]) => boolean)
   loadingTimeMs?: number
 }
 
@@ -138,7 +138,7 @@ const mockUseAuth =
     return createDummyAuthContextValues({
       loading: authLoading,
       isAuthenticated: authIsAuthenticated,
-      hasRole: () => hasRole,
+      hasRole: typeof hasRole === 'boolean' ? () => hasRole : hasRole,
     })
   }
 
@@ -1593,3 +1593,199 @@ describe('Unauthorized redirect error messages', () => {
     )
   })
 })
+
+describe('Multiple nested private sets', () => {
+  const HomePage = () => <h1>Home Page</h1>
+  const PrivateNoRolesAssigned = () => <h1>Private No Roles Page</h1>
+  const PrivateEmployeePage = () => <h1>Private Employee Page</h1>
+  const PrivateAdminPage = () => <h1>Private Admin Page</h1>
+
+  const LevelLayout = ({ children, level }) => (
+    <div>
+      Level: {level}
+      {children}
+    </div>
+  )
+
+  const TestRouter = ({ useAuthMock }) => (
+    <Router useAuth={useAuthMock}>
+      <Route path="/" page={HomePage} name="home" />
+      <Set unauthenticated="home" level="1" wrap={LevelLayout} private>
+        <Route
+          path="/no-roles-assigned"
+          page={PrivateNoRolesAssigned}
+          name="noRolesAssigned"
+        />
+        <Set
+          private
+          unauthenticated="noRolesAssigned"
+          roles={['ADMIN', 'EMPLOYEE']}
+          level="2"
+        >
+          <Set
+            unauthenticated="privateAdmin"
+            roles={['EMPLOYEE']}
+            level="3"
+            private
+          >
+            <Route
+              path="/employee"
+              page={PrivateEmployeePage}
+              name="privateEmployee"
+            />
+          </Set>
+
+          <Set
+            unauthenticated="privateEmployee"
+            roles={['ADMIN']}
+            level="3"
+            private
+          >
+            <Route path="/admin" page={PrivateAdminPage} name="privateAdmin" />
+          </Set>
+        </Set>
+      </Set>
+    </Router>
+  )
+
+  test('is authenticated but does not have matching roles', async () => {
+    const screen = render(
+      <TestRouter
+        useAuthMock={mockUseAuth({
+          isAuthenticated: true,
+          hasRole: false,
+        })}
+      />
+    )
+
+    act(() => navigate('/employee'))
+
+    await waitFor(() => {
+      expect(screen.queryByText(`Private No Roles Page`)).toBeInTheDocument()
+      expect(screen.queryByText(`Level: 1`)).toBeInTheDocument()
+    })
+  })
+
+  test('is not authenticated', async () => {
+    const screen = render(
+      <TestRouter
+        useAuthMock={mockUseAuth({
+          isAuthenticated: false,
+          hasRole: false,
+        })}
+      />
+    )
+
+    act(() => navigate('/employee'))
+
+    await waitFor(() => {
+      expect(screen.queryByText(`Home Page`)).toBeInTheDocument()
+      expect(screen.queryByText(`Level`)).not.toBeInTheDocument()
+    })
+  })
+
+  test('is authenticated and has a matching role', async () => {
+    const screen = render(
+      <TestRouter
+        useAuthMock={mockUseAuth({
+          isAuthenticated: true,
+          hasRole: (role) => {
+            return role.includes('ADMIN')
+          },
+        })}
+      />
+    )
+
+    act(() => navigate('/admin'))
+    await waitFor(() => {
+      expect(screen.queryByText(`Private Admin Page`)).toBeInTheDocument()
+      expect(screen.queryByText(`Level: 3`)).toBeInTheDocument()
+    })
+  })
+
+  test('returns the correct page if has a matching role', async () => {
+    const screen = render(
+      <TestRouter
+        useAuthMock={mockUseAuth({
+          isAuthenticated: true,
+          hasRole: (role) => {
+            return role.includes('ADMIN')
+          },
+        })}
+      />
+    )
+
+    act(() => navigate('/admin'))
+
+    await waitFor(() => {
+      expect(screen.queryByText(`Private Admin Page`)).toBeInTheDocument()
+      expect(screen.queryByText(`Level: 3`)).toBeInTheDocument()
+    })
+  })
+})
+
+describe('Multiple nested private sets', () => {
+  const HomePage = () => <h1>Home Page</h1>
+  const Page = () => <h1>Page</h1>
+
+  const DebugLayout = (props) => {
+    return (
+      <div>
+        <p>Theme: {props.theme}</p>
+        <p>Other Prop: {props.otherProp}</p>
+        <p>Page Level: {props.level}</p>
+        {props.children}
+      </div>
+    )
+  }
+
+  const TestRouter = () => (
+    <Router>
+      <Route path="/" page={HomePage} name="home" />
+      <Set level="1" theme="blue" wrap={DebugLayout}>
+        <Route path="/level1" page={Page} name="level1" />
+        <Set level="2" theme="red" otherProp="bazinga">
+          <Route path="/level2" page={Page} name="level2" />
+          <Set level="3" theme="green">
+            <Route path="/level3" page={Page} name="level3" />
+          </Set>
+        </Set>
+      </Set>
+    </Router>
+  )
+
+  test('level 1, matches expected props', async () => {
+    const screen = render(<TestRouter />)
+
+    act(() => navigate('/level1'))
+
+    await waitFor(() => {
+      expect(screen.queryByText(`Theme: blue`)).toBeInTheDocument()
+      expect(screen.queryByText(`Page Level: 1`)).toBeInTheDocument()
+    })
+  })
+
+  test('level 2, should override level 1', async () => {
+    const screen = render(<TestRouter />)
+
+    act(() => navigate('/level2'))
+
+    await waitFor(() => {
+      expect(screen.queryByText(`Theme: red`)).toBeInTheDocument()
+      expect(screen.queryByText(`Other Prop: bazinga`)).toBeInTheDocument()
+      expect(screen.queryByText(`Page Level: 2`)).toBeInTheDocument()
+    })
+  })
+
+  test('level 3, should override level 1 & 2 and pass through other props', async () => {
+    const screen = render(<TestRouter />)
+
+    act(() => navigate('/level3'))
+
+    await waitFor(() => {
+      expect(screen.queryByText(`Theme: green`)).toBeInTheDocument()
+      expect(screen.queryByText(`Other Prop: bazinga`)).toBeInTheDocument()
+      expect(screen.queryByText(`Page Level: 3`)).toBeInTheDocument()
+    })
+  })
+})
diff --git a/packages/router/src/router.tsx b/packages/router/src/router.tsx
index 8f8d5785987b..4e6c71c7183b 100644
--- a/packages/router/src/router.tsx
+++ b/packages/router/src/router.tsx
@@ -188,7 +188,6 @@ const LocationAwareRouter: React.FC<RouterProps> = ({
                   spec={normalizePage(page as any)}
                   params={allParams}
                   whileLoadingPage={whileLoadingPage as any}
-                  {...setProps}
                 />
               }
               setProps={setProps}
@@ -203,7 +202,7 @@ const LocationAwareRouter: React.FC<RouterProps> = ({
 interface WrappedPageProps {
   wrappers: Wrappers
   routeLoaderElement: ReactNode
-  setProps: Record<any, any>
+  setProps: Record<any, any>[]
 }
 
 /**
@@ -220,27 +219,50 @@ const WrappedPage = memo(
   ({ wrappers, routeLoaderElement, setProps }: WrappedPageProps) => {
     // @NOTE: don't mutate the wrappers array, it causes full page re-renders
     // Instead just create a new array with the AuthenticatedRoute wrapper
+
+    // we need to pass the setProps from each set to each wrapper
     let wrappersWithAuthMaybe = wrappers
-    if (setProps.private) {
-      if (!setProps.unauthenticated) {
-        throw new Error(
-          'You must specify an `unauthenticated` route when marking a Route as private'
-        )
-      }
+    const reveresedSetProps = [...setProps].reverse()
 
-      wrappersWithAuthMaybe = [AuthenticatedRoute, ...wrappers]
-    }
+    reveresedSetProps
+      // @MARK note the reverse() here, because we spread wrappersWithAuthMaybe
+      .forEach((propsFromSet) => {
+        if (propsFromSet.private) {
+          if (!propsFromSet.unauthenticated) {
+            throw new Error(
+              'You must specify an `unauthenticated` route when marking a Route as private'
+            )
+          }
+
+          // @MARK: this component intentionally removes all props except children
+          // because the .reduce below will apply props inside out
+          const AuthComponent: React.FC<{ children: ReactNode }> = ({
+            children,
+          }) => {
+            return (
+              <AuthenticatedRoute {...propsFromSet}>
+                {children}
+              </AuthenticatedRoute>
+            )
+          }
+
+          wrappersWithAuthMaybe = [AuthComponent, ...wrappersWithAuthMaybe]
+        }
+      })
 
     if (wrappersWithAuthMaybe.length > 0) {
       // If wrappers exist e.g. [a,b,c] -> <a><b><c><routeLoader></c></b></a> and returns a single ReactNode
       // Wrap AuthenticatedRoute this way, because if we mutate the wrappers array itself
       // it causes full rerenders of the page
       return wrappersWithAuthMaybe.reduceRight((acc, wrapper) => {
+        // Merge props from set, the lowest set props will override the higher ones
+        const mergedSetProps = setProps.reduce((acc, props) => {
+          return { ...acc, ...props }
+        }, {})
+
         return React.createElement(
           wrapper as any,
-          {
-            ...setProps,
-          },
+          mergedSetProps,
           acc ? acc : routeLoaderElement
         )
       }, undefined as ReactNode)
diff --git a/packages/router/src/util.ts b/packages/router/src/util.ts
index 70d28012cb8a..e0bec64c60b6 100644
--- a/packages/router/src/util.ts
+++ b/packages/router/src/util.ts
@@ -464,7 +464,7 @@ interface AnalyzedRoute {
   page: PageType | null
   redirect: string | null
   wrappers: Wrappers
-  setProps: Record<any, any>
+  setProps: Record<any, any>[]
   setId: number
 }
 
@@ -483,7 +483,7 @@ export function analyzeRoutes(
     whileLoadingPageFromSet?: WhileLoadingPage
     wrappersFromSet?: Wrappers
     // we don't know, or care about, what props users are passing down
-    propsFromSet?: Record<string, unknown>
+    propsFromSet?: Record<string, unknown>[]
     setId?: number
   }
 
@@ -507,7 +507,7 @@ export function analyzeRoutes(
     nodes,
     whileLoadingPageFromSet,
     wrappersFromSet = [],
-    propsFromSet: previousSetProps = {},
+    propsFromSet: previousSetProps = [],
   }: RecurseParams) => {
     nodes.forEach((node) => {
       if (isValidRoute(node)) {
@@ -612,8 +612,10 @@ export function analyzeRoutes(
         // @MARK note unintuitive, but intentional
         // You cannot make a nested set public if the parent is private
         // i.e. the private prop cannot be overriden by a child Set
+
+        // @TODO: Double check this logic is correct for privatge logic in previous set props
         const privateProps =
-          isPrivateNode(node) || previousSetProps.private
+          isPrivateNode(node) || previousSetProps.some((props) => props.private)
             ? { private: true }
             : {}
 
@@ -627,13 +629,15 @@ export function analyzeRoutes(
               whileLoadingPageFromCurrentSet || whileLoadingPageFromSet,
             setId,
             wrappersFromSet: [...wrappersFromSet, ...wrapperComponentsArray],
-            propsFromSet: {
+            propsFromSet: [
               ...previousSetProps,
-              // Current one takes precedence
-              ...otherPropsFromCurrentSet,
-              // See comment at definiion, intenionally at the end
-              ...privateProps,
-            },
+              {
+                // Current one takes precedence
+                ...otherPropsFromCurrentSet,
+                // See comment at definiion, intenionally at the end
+                ...privateProps,
+              },
+            ],
           })
         }
       }