Adds handling of render and children to SecureRoute

packages/okta-react/CHANGELOG.md

@@ -1,3 +1,10 @@
+# PENDING
+
+### Bug Fixes
+
+- [#872](https://github.com/okta/okta-oidc-js/pull/872) Adjusts `<SecureRoute>` so that it enforces authentication requirement for components passed via "render" or "children" in addition to "component"
+  - NOTE: `<SecureRoute>`, like react-router `<Route>`, only wants ONE of the three ways of passing wrapped components per route
+
 # 3.0.4
 
 ### Bug Fixes

packages/okta-react/README.md

@@ -466,6 +466,11 @@ class App extends Component {
 
 `SecureRoute` integrates with `react-router`.  Other routers will need their own methods to ensure authentication using the hooks/HOC props provided by this SDK.
 
+As with `Route` from `react-router-dom`, `<SecureRoute>` can take one of:
+- a `component` prop that is passed a component
+- a `render` prop that is passed a function that returns a component.  This function will be passed any additional props that react-router injects (such as `history` or `match`)
+- children components
+
 ### `LoginCallback`
 
 `LoginCallback` handles the callback after the redirect to and back from the Okta-hosted login page. By default, it parses the tokens from the uri, stores them, then redirects to `/`. If a `SecureRoute` caused the redirect, then the callback redirects to the secured route. For more advanced cases, this component can be copied to your own source tree and modified as needed.

packages/okta-react/src/SecureRoute.js

@@ -14,38 +14,45 @@ import React, { useEffect } from 'react';
 import { useOktaAuth } from './OktaContext';
 import { useHistory, Route } from 'react-router-dom';
 
-const RequireAuth = ({ children }) => { 
+const RequireAuth = ({ render, routeProps }) => { 
   const { authService, authState } = useOktaAuth();
   const history = useHistory();
 
   useEffect(() => {
-    // Make sure login process is not triggered when the app just start
+    // Start login if and only if app has decided it is not logged inn
     if(!authState.isAuthenticated && !authState.isPending) { 
       const fromUri = history.createHref(history.location);
       authService.login(fromUri);
     }  
-  }, [authState, authService]);
+  }, [authState, authService, history]);
 
   if (!authState.isAuthenticated) {
     return null;
   }
 
   return (
     <React.Fragment>
-      {children}
+      { render(routeProps) }
     </React.Fragment>
   );
-
 };
 
-const SecureRoute = ( {component, ...props} ) => { 
+const SecureRoute = ( {component, render, children, ...props} ) => { 
+  // react-router Route uses exactly one of: render, component, children
+  // We wrap whichever they use to require authentication and use the render method on Route
+
+  let authRender = render;
+
+  if( component || !render ) { // React-router has component take precedence over render
+    const PassedComponent = component || function() { return <React.Fragment>{children}</React.Fragment>; };
+    // eslint-disable-next-line react/display-name
+    authRender = wrappedProps => <PassedComponent { ...wrappedProps} />;
+  }
 
-  const PassedComponent = component || function() { return null; };
-  const WrappedComponent = (wrappedProps) => (<RequireAuth><PassedComponent {...wrappedProps}/></RequireAuth>);
   return (
     <Route
       { ...props }
-      render={ (routeProps) => props.render ? props.render({...routeProps, component: WrappedComponent}) : <WrappedComponent {...routeProps}/> } 
+      render={ routeProps => <RequireAuth render={authRender} routeProps={routeProps}/> }
     />
   );
 };

packages/okta-react/test/jest/secureRoute.test.js

@@ -33,7 +33,7 @@ describe('<SecureRoute />', () => {
       authState.isAuthenticated = true;
     });
 
-    it('will render wrapped component', () => {
+    it('will render wrapped component using "component"', () => {
       const MyComponent = function() { return <div>hello world</div>; };
       const wrapper = mount(
         <MemoryRouter>
@@ -46,6 +46,34 @@ describe('<SecureRoute />', () => {
       );
       expect(wrapper.find(MyComponent).html()).toBe('<div>hello world</div>');
     });
+
+    it('will render wrapped component using "render"', () => {
+      const MyComponent = function() { return <div>hello world</div>; };
+      const wrapper = mount(
+        <MemoryRouter>
+          <Security {...mockProps}>
+            <SecureRoute
+              render={ () => <MyComponent/> }
+            />
+          </Security>
+        </MemoryRouter>
+      );
+      expect(wrapper.find(MyComponent).html()).toBe('<div>hello world</div>');
+    });
+
+    it('will render wrapped component as a child', () => {
+      const MyComponent = function() { return <div>hello world</div>; };
+      const wrapper = mount(
+        <MemoryRouter>
+          <Security {...mockProps}>
+            <SecureRoute>
+              <MyComponent/>
+            </SecureRoute>
+          </Security>
+        </MemoryRouter>
+      );
+      expect(wrapper.find(MyComponent).html()).toBe('<div>hello world</div>');
+    });
   });
 
   describe('isAuthenticated: false', () => {
@@ -54,7 +82,7 @@ describe('<SecureRoute />', () => {
       authState.isAuthenticated = false;
     });
 
-    it('will not render wrapped component', () => {
+    it('will not render wrapped component using "component"', () => {
       const MyComponent = function() { return <div>hello world</div>; };
       const wrapper = mount(
         <MemoryRouter>
@@ -68,6 +96,34 @@ describe('<SecureRoute />', () => {
       expect(wrapper.find(MyComponent).length).toBe(0);
     });
 
+    it('will not render wrapped component using "render"', () => {
+      const MyComponent = function() { return <div>hello world</div>; };
+      const wrapper = mount(
+        <MemoryRouter>
+          <Security {...mockProps}>
+            <SecureRoute
+              render={ () => <MyComponent/> }
+            />
+          </Security>
+        </MemoryRouter>
+      );
+      expect(wrapper.find(MyComponent).length).toBe(0);
+    });
+
+   it('will not render wrapped component with children', () => {
+      const MyComponent = function() { return <div>hello world</div>; };
+      const wrapper = mount(
+        <MemoryRouter>
+          <Security {...mockProps}>
+            <SecureRoute>
+              <MyComponent/>
+            </SecureRoute>
+          </Security>
+        </MemoryRouter>
+      );
+      expect(wrapper.find(MyComponent).length).toBe(0);
+    });
+
     describe('isPending: false', () => {
 
       beforeEach(() => {
@@ -218,7 +274,7 @@ describe('<SecureRoute />', () => {
     expect(secureRoute.find(Route).props().sensitive).toBe(true);
   });
 
-  it('should pass react-router props to an internal Route component', () => {
+  it('should pass react-router props to an component', () => {
     authState.isAuthenticated = true;
     const MyComponent = function(props) { return <div>{ props.history ? 'has history' : 'lacks history'}</div>; };
     const wrapper = mount(
@@ -234,6 +290,22 @@ describe('<SecureRoute />', () => {
     expect(wrapper.find(MyComponent).html()).toBe('<div>has history</div>');
   });
 
+  it('should pass react-router props to a render call', () => {
+    authState.isAuthenticated = true;
+    const MyComponent = function(props) { return <div>{ props.history ? 'has history' : 'lacks history'}</div>; };
+    const wrapper = mount(
+      <MemoryRouter>
+        <Security {...mockProps}>
+          <SecureRoute
+            path='/'
+            render = {props => <MyComponent {...props} />}
+          />
+        </Security>
+      </MemoryRouter>
+    );
+    expect(wrapper.find(MyComponent).html()).toBe('<div>has history</div>');
+  });
+
   it('should pass props using the "render" prop', () => {
     authState.isAuthenticated = true;
     const MyComponent = function(props) { return <div>{ props.someProp ? 'has someProp' : 'lacks someProp'}</div>; };