okta / okta-react

Okta OIDC SDK for React
https://github.com/okta/okta-react
Other
113 stars 78 forks source link

Support react-router v6 #178

Closed baumandm closed 1 year ago

baumandm commented 4 years ago

I'm submitting this issue for the package(s):

I'm submitting a:

Current behavior

The upcoming version of React-router, v6, makes a number of breaking changes, specifically breaking <SecureRoute /> due to the deprecation of useHistory() and render props. That is the only breaking feature I've encountered, but there may be others.

Expected behavior

This library should support react-router v6.

Minimal reproduction of the problem with instructions

Use the following react-router-dom dependency:

  "react-router-dom": "^6.0.0-beta.0",

Extra information about the use case/user story you are trying to implement

I've implemented a custom <SecureRoute /> component that is sufficient for my use cases. It's slightly simplified so it might not be a drop-in-replacement for all cases, but I wanted to share in case anyone else needs an interim solution.

import { useOktaAuth } from '@okta/okta-react';
import React from 'react';
import { Route } from 'react-router-dom';

const RequireAuth = ({ children }) => {
  const { authService, authState } = useOktaAuth();

  if (!authState.isAuthenticated) {
    if (!authState.isPending) {
      const fromUri = window.location.href;
      authService.login(fromUri);
    }
    return null;
  }

  return <React.Fragment>{children}</React.Fragment>;
};

const SecureRoute = ({ element, ...props }) => {
  const WrappedComponent = (wrappedProps) => <RequireAuth>{element}</RequireAuth>;
  return <Route {...props} element={<WrappedComponent />} />;
};

export default SecureRoute;

Environment

swiftone commented 4 years ago

@baumandm - Thanks for the ticket and the interim implementation. We'll put this on the backlog and work to make sure we have compatibility with react-router 6 ASAP following the official release. Given the scope of some of the changes, this will likely require a major version change on our part.

swiftone commented 4 years ago

Internal ref: OKTA-318565

bartimaeus commented 4 years ago

@swiftone I have been experiencing an issue with too. I'm using "react-router-dom": "^5.2.0".

Thanks @baumandm! I spent half a day debugging my app trying to figure out why after upgrading @okta/okta-react and @okta/okta-signin-widget my nested routes weren't working correctly. Finding your comment saved me!

I thought the issue was only a problem with nested routes, but then discovered that my component referenced in <SecureRoute path="/" component={<AuthenticatedLayout/>} /> would re-render each time I navigated or changed routes.

The useEffect added here was the issue for me.

I used the latest version of <SecureRoute/>, and reverted the useEffect.

- useEffect(() => {
-   // Make sure login process is not triggered when the app just start
-   if(!authState.isAuthenticated && !authState.isPending) { 
-     const fromUri = history.createHref(history.location); 
-     authService.login(fromUri);
-   }  
- }, [authState, authService]);
- 
- if (!authState.isAuthenticated) {
-   return null;
- }
+ if(!authState.isAuthenticated) { 
+   if(!authState.isPending) {
+     // Using @baumandm's suggestion here for react-router-dom v6
+     const fromUri = window.location.href;
+     authService.login(fromUri);
+   }
+   return null;
+ }
shuowu commented 4 years ago

@bartimaeus Thanks for reporting the issue!

~Internal Ref: OKTA-322622~ (Duplicate)

swiftone commented 4 years ago

@bartimaeus - Your issue sounds separate from react-router v6 (beta), and sounds more like https://github.com/okta/okta-oidc-js/issues/777 (which is on my current sprint)

bartimaeus commented 4 years ago

@swiftone you're right. okta/okta-oidc-js#777 is my exact issue.

relm923 commented 2 years ago

@swiftone any update on this now that v6 has officially been released?

Steel9561 commented 2 years ago

Can someone create a TypeScript version of this code? 👍

import { useOktaAuth } from '@okta/okta-react'; import React from 'react'; import { Route } from 'react-router-dom';

const RequireAuth = ({ children }) => { const { authService, authState } = useOktaAuth();

if (!authState.isAuthenticated) { if (!authState.isPending) { const fromUri = window.location.href; authService.login(fromUri); } return null; }

return {children}</React.Fragment>; };

const SecureRoute = ({ element, ...props }) => { const WrappedComponent = (wrappedProps) => {element}; return <Route {...props} element={} />; };

export default SecureRoute;

Steel9561 commented 2 years ago

I'm submitting this issue for the package(s):

  • [ ] jwt-verifier
  • [ ] okta-angular
  • [ ] oidc-middleware
  • [x] okta-react
  • [ ] okta-react-native
  • [ ] okta-vue

I'm submitting a:

  • [ ] Bug report
  • [x] Feature request
  • [ ] Other (Describe below)

Current behavior

The upcoming version of React-router, v6, makes a number of breaking changes, specifically breaking <SecureRoute /> due to the deprecation of useHistory() and render props. That is the only breaking feature I've encountered, but there may be others.

Expected behavior

This library should support react-router v6.

Minimal reproduction of the problem with instructions

Use the following react-router-dom dependency:

  "react-router-dom": "^6.0.0-beta.0",

Extra information about the use case/user story you are trying to implement

I've implemented a custom <SecureRoute /> component that is sufficient for my use cases. It's slightly simplified so it might not be a drop-in-replacement for all cases, but I wanted to share in case anyone else needs an interim solution.

import { useOktaAuth } from '@okta/okta-react';
import React from 'react';
import { Route } from 'react-router-dom';

const RequireAuth = ({ children }) => {
  const { authService, authState } = useOktaAuth();

  if (!authState.isAuthenticated) {
    if (!authState.isPending) {
      const fromUri = window.location.href;
      authService.login(fromUri);
    }
    return null;
  }

  return <React.Fragment>{children}</React.Fragment>;
};

const SecureRoute = ({ element, ...props }) => {
  const WrappedComponent = (wrappedProps) => <RequireAuth>{element}</RequireAuth>;
  return <Route {...props} element={<WrappedComponent />} />;
};

export default SecureRoute;

Environment

  • Package Version: 3.0.2
  • Browser: Firefox
  • OS: MacOS
  • Node version (node -v): v14.3.0
  • Other:

I tried creating a similar but different implementation, but it would seem that for some reason it's still looking for the SecureRoute that comes with OktaReact and not the custom one I created.

I did verify that the SecureRoute links are using the custom SecureRoute (not the okta one that is picked from the bundle file). Here is error in chrome browser:

"Uncaught Error: [SecureRoute] is not a component. All component children of must be a or "

Any ideas please to help with this?

baumandm commented 2 years ago

The latest version of React Router v6 breaks SecureRoute because all the children of <Routes /> must be <Route />. This means creating a custom Route wrapper is off the table.

The solution I've found was to rewrite SecureRoute to do an auth check and conditionally render its children. Then it's can be used inside a standard <Route />, which does the route matching:


<Routes>
  <Route path="/admin" element={<SecureRoute><Private /></SecureRoute>} />
</Routes>
``

Here's a blog post with more details: https://dev.to/iamandrewluca/private-route-in-react-router-v6-lg5
Steel9561 commented 2 years ago

<Route path="/admin" element={} />

Thanks so much! will give this a try. Do you happen to have a typescript version of your custom secureroute component by any chance?

Steel9561 commented 2 years ago

The latest version of React Router v6 breaks SecureRoute because all the children of <Routes /> must be <Route />. This means creating a custom Route wrapper is off the table.

The solution I've found was to rewrite SecureRoute to do an auth check and conditionally render its children. Then it's can be used inside a standard <Route />, which does the route matching:

<Routes>
  <Route path="/admin" element={<SecureRoute><Private /></SecureRoute>} />
</Routes>
``

Here's a blog post with more details: https://dev.to/iamandrewluca/private-route-in-react-router-v6-lg5

This example helped, but now I am getting this error:

Cannot destructure property 'oktaAuth' of '(0 , _okta_context__WEBPACK_IMPORTED_MODULE_1__.useOktaAuth)(...)' as it is null.

Steel9561 commented 2 years ago

Now I am getting a new error saying that all Route needs to be wrapped up around a Routes container. Its weird because I have all my Route components inside the Routes component. Any ideas?

swiftone commented 2 years ago

@relm923, @andreawyss, @dsabanin - I'm no longer with Okta myself, so I'm afraid I can't help on this issue, but I'm sure the Okta crew will be responding soon.

If you have an immediate need, my unofficial recommendation is to implement your own SecureRoute implementation as @baumandm suggested. If I recall correctly, it's a fairly thin layer that uses useOktaAuth to decide if the route should render or not.

Edit: https://github.com/okta/okta-react/blob/master/src/SecureRoute.tsx

kellengreen commented 2 years ago

What's the status of this? v6 is now live.

jaredperreault-okta commented 2 years ago

We are aware of this issue and are working towards a solution. Stay tuned for updates.

In the meantime, if upgrading to the v6 router is required, I suggest writing your own version of SecureRoute as suggested in this thread

Internal Ref: OKTA-447325

Suresh-Lakum commented 2 years ago

Any update on this? I am facing similar issue and using latest react-router-dom v6. I can't include SecureRoute directly as part of Routes and

`<Route path="/" element={

                    }
                />`

isn't working either. Getting below error : image

jonrimmer commented 2 years ago

@jaredperreault-okta I have tried writing my own version of SecureRoute as you suggest, but I get the following error when trying to build my app:

node_modules/@okta/okta-react/bundles/okta-react.esm.js:21:9: error: No matching export in "node_modules/react-router-dom/index.js" for import "useRouteMatch"

What is your recommended workaround for okta-react importing useRouteMatch when it no longer exists in react-router-dom?

jaredperreault-okta commented 2 years ago

@jonrimmer I suggest trying useMatch

Docs: https://reactrouter.com/docs/en/v6/upgrading/v5#replace-useroutematch-with-usematch

jonrimmer commented 2 years ago

@jaredperreault-okta This usage in your codebase, not mine. ESBuild is tracing the imports in your esm bundle and finding invalid imports.

dubzzz commented 2 years ago

Why isn't react-router-dom a dependency of the package (with ^ and not >=)? Having it into peerDependencies makes bundlers fail whenever you have to pull react-dom@6.x.y in a project having okta-react.

I know that the use of react-router-dom is limited to SecureRoute (that I do not use on my project) but the fact to have a peer instead of a clear dep makes bundlers totally fail even if you don't use anything linked to it.

jaredperreault-okta commented 2 years ago

@dubzzz react-router-dom is a peer dependency because the version used inside our sdk and the application need to be the same. However, this is something we are currently re-evaluating

@jonrimmer a release is going out soon that should address the useRouteMatch issue

jaredperreault-okta commented 2 years ago

To provide an update here

We are still discussing React routing holistically, including react-router-dom v6. In the meantime we have put together a POC test app to illustrate the usage of various React routing libraries

https://github.com/okta/okta-react/tree/routing-poc/test/apps/react-routing

Would love some community feedback

baumandm commented 2 years ago

I tested out v6.4.2 but I'm still getting Typescript errors despite the change in import:

.../node_modules/@okta/okta-react/bundles/okta-react.esm.js
Attempted import error: 'useRouteMatch' is not exported from 'react-router-dom' (imported as 'ReactRouterDom').

Any ideas?

jaredperreault-okta commented 2 years ago

@baumandm Are you using <SecureRoute /> in your application?

baumandm commented 2 years ago

I am still using a custom implementation from the beginning of the thread, not importing the built-in one.

It's very possible the error is due to my tsconfig or project setup specifically, but I looked through your sample project to compare and can't see the issue.

Happy to wait for the long-term fix before upgrading.

johnflux commented 2 years ago

@baumandm Any progress? I'm unable to get past that typescript error.

baumandm commented 2 years ago

@johnflux No unfortunately, I wasn't able to get it to work so I'm sticking with 3.x for now.

jaredperreault-okta commented 2 years ago

@johnflux can you provide you tsconfig.json?

johnflux commented 2 years ago

Hi Jared, I really appreciate you helping me! This is a standard create-react-app:

{ "compilerOptions": { "target": "es5", "lib": [ "dom", "dom.iterable", "esnext" ], "allowJs": true, "skipLibCheck": true, "esModuleInterop": true, "allowSyntheticDefaultImports": true, "strict": true, "forceConsistentCasingInFileNames": true, "noFallthroughCasesInSwitch": true, "module": "esnext", "moduleResolution": "node", "resolveJsonModule": true, "isolatedModules": true, "noEmit": true, "jsx": "react-jsx" }, "include": [ "src" ] }

jaredperreault-okta commented 2 years ago

@johnflux nothing here stands out to me. I suggest taking a look at the routing test apps here

gitforajmal commented 2 years ago

I have modified the 'SecureRoute ' as per the test app implementation. I am still getting the error like below. I am using router v6

./node_modules/@okta/okta-react/bundles/okta-react.esm.js 284:14-42 export 'useRouteMatch' (imported as 'ReactRouterDom') was not found in 'react-router-dom' (possible exports: BrowserRouter, HashRouter, Link, MemoryRouter, NavLink, Navigate, Outlet, Route, Router, Routes, UNSAFE_LocationContext, UNSAFE_NavigationContext, UNSAFE_RouteContext, createRoutesFromChildren, createSearchParams, generatePath, matchPath, matchRoutes, renderMatches, resolvePath, unstable_HistoryRouter, useHref, useInRouterContext, useLinkClickHandler, useLocation, useMatch, useNavigate, useNavigationType, useOutlet, useOutletContext, useParams, useResolvedPath, useRoutes, useSearchParams)

jaredperreault-okta commented 2 years ago

@gitforajmal can you provide the relevant dependency versions?

gitforajmal commented 2 years ago

@jaredperreault-okta "@okta/okta-auth-js": "^6.1.0", "@okta/okta-react": "^6.4.2", -- I did replace as like your test app "*" "react": "^17.0.2", "react-router-dom": "^6.2.1",

flying-sheep commented 2 years ago

Unfortunately it’s not clear which tool here emits the “Attempted import error”, but as long as the emitted ESM contains

import * as ReactRouterDom from 'react-router-dom';
[…]
var match = ReactRouterDom.useRouteMatch(routeProps);

Any good tool analyzing imports will see a static import of useRouteMatch and therefore complain that it doesn’t exist.

Does anyone know which tool it is that complains here and which code has to be emitted so it ignores the problem?

flying-sheep commented 2 years ago

I tried some things:

const hookName = 'useRouteMatch'
const match = ReactRouterDom[hookName](routeProps)

does work.

const match = ReactRouterDom['useRouteMatch'](routeProps) does not work.

To be sure and to have a static reference, I’d suggest something like

import * as ReactRouterDom from 'react-router-dom'

const useMatch = ReactRouterDom['useMatch' in ReactRouterDom ? 'useMatch' : 'useRouteMatch']
...
function SecureRoute(_ref) {
  ...
  const match = useMatch(routeProps)
}
flying-sheep commented 2 years ago

Fixed in https://github.com/okta/okta-react/pull/210

uday-at-tomo commented 2 years ago

This is what we had to do with the latest react; react router after forking this repo:

import * as React from 'react';
import { useOktaAuth, OnAuthRequiredFunction } from './OktaContext';
import * as ReactRouterDom from 'react-router-dom';
import { toRelativeUrl } from '@okta/okta-auth-js';
import OktaError from './OktaError';

const SecureRoute: React.FC<{
  onAuthRequired?: OnAuthRequiredFunction;
  errorComponent?: React.ComponentType<{ error: Error }>;
} & ReactRouterDom.RouteProps & React.HTMLAttributes<HTMLDivElement>> = ({ 
  onAuthRequired,
  errorComponent,
  ...routeProps
}) => { 
  const { oktaAuth, authState, _onAuthRequired } = useOktaAuth();
  const { path = '/*', caseSensitive } = routeProps;
  const match = ReactRouterDom.useMatch({ path, caseSensitive })
  const pendingLogin = React.useRef(false);
  const [handleLoginError, setHandleLoginError] = React.useState<Error | null>(null);
  const ErrorReporter = errorComponent || OktaError;

  React.useEffect(() => {
    const handleLogin = async () => {
      if (pendingLogin.current) {
        return;
      }

      pendingLogin.current = true;

      const originalUri = toRelativeUrl(window.location.href, window.location.origin);
      oktaAuth.setOriginalUri(originalUri);
      const onAuthRequiredFn = onAuthRequired || _onAuthRequired;
      if (onAuthRequiredFn) {
        await onAuthRequiredFn(oktaAuth);
      } else {
        await oktaAuth.signInWithRedirect();
      }
    };

    // Only process logic if the route matches
    if (!match) {
      return;
    }

    if (!authState) {
      return;
    }

    if (authState.isAuthenticated) {
      pendingLogin.current = false;
      return;
    }

    // Start login if app has decided it is not logged in and there is no pending signin
    if(!authState.isAuthenticated) { 
      handleLogin().catch(err => {
        setHandleLoginError(err as Error);
      });
    }  

  }, [
    authState,
    oktaAuth, 
    match, 
    onAuthRequired, 
    _onAuthRequired
  ]);

  if (handleLoginError) {
    return <ErrorReporter error={handleLoginError} />;
  }

  if (!authState || !authState.isAuthenticated) {
    return null;
  }

  return (<ReactRouterDom.Outlet />);
};

export default SecureRoute;

Usage:

        <Route path="/" element={<SecureRoute />}>
          <Route path="" element={<Protected />} />
          <Route path="/other" element={<OtherProtected />} />
        </Route>

We see this as a stop gap until the Okta team gets some room to solve for this.

flying-sheep commented 2 years ago

cf00f9da9fb8077d2c92960b9b72ce05050133c7 (released as v6.4.3) allows to use okta-react with react-router v6 without any static analysis tools complaining.

If you use SecureRoute you also still have to reimplement it.

rapaccinim commented 2 years ago

@flying-sheep I have tried to implement this code example coming directly from Okta. Do you have any suggestions on an implementation considering that I need to protect all the routes (not only specific ones)?

jaredperreault-okta commented 2 years ago

@rapaccinim

<Routes>
    <Route path='login/callback' element={<LoginCallback loadingElement={<Loading />} />} />
    <Route path='app' element={<RequiredAuth />}>
      <Route path='dashboard' element={<Dashboard />} />
      <Route path='profile' element={<Profile />} />
      {/* rest of your routes which require auth */}
    </Route>
</Routes>

You can nest multiple routes under <Route path='app' element={<RequiredAuth />}>. However the LoginCallback needs to be mounted as an unprotected route

rapaccinim commented 2 years ago

@jaredperreault-okta thank you for your comment. This is my current implementation:

        <Routes>
            <Route path='/' element={<Home />}/>
            <Route path='login/callback' element={<LoginCallback loadingElement={<div>Loading...</div>} />} />
            <Route path='/app' element={<RequiredAuth />}>
                <Route path='onboarding' element={<OnboardingCheck/>} />
                <Route path='hello' element={<Hello/>} />
            </Route>
        </Routes>

The problem is that when I navigate to a protected page (e.g. onboarding or hello), the element component follows a 3 step rendering (instead of 1):

Is there a way to avoid this behaviour? Thank you

jaredperreault-okta commented 2 years ago

@rapaccinim Under what conditions does this occur? If an unauthenticated user requests a protected page:

  1. the <Loading /> component is rendered via <RequiredAuth />
  2. then a redirect to Okta is made, where the user will enter their credentials (log in to Okta)
  3. a final redirect from Okta to {your/app}/login/callback is made, where a session is created (log in to your app)

What can happen, especially during development/testing, is you are logged into Okta (from accessing the admin console or otherwise), but their is no active session within your app. Therefore steps 1, 2, 3 happen in quick succession without requiring credential input (because Okta already knows who you are).

anjalichawlaa commented 2 years ago

@jaredperreault-okta I'm also facing a similar issue. My Current implementation : <Routes> <Route path="/callback" element={<LoginCallback loadingElement={<Loading />}/>} /> <Route path="/" element={<RequiredAuth />}> <Route path='' element={<VinListComparatorPage />} /> </Route> <Route path="/logged-out" element={<Login />} /> <Route path="*" element={<NotFoundErrorPage />} exact /> </Routes>

RequiredAuth


const RequiredAuth = () => {
  const { authState, oktaAuth} = useOktaAuth();
  if (!authState || !authState.isAuthenticated) {
    const originalUri = toRelativeUrl(window.location.href, window.location.origin);
    oktaAuth.setOriginalUri(originalUri);
    oktaAuth.signInWithRedirect();
    return (<Loading />);
  }
  return (<Outlet />);
}```
        Once the user is authenticated, after reload or refresh, authstate is always null which leads to the if statement and complete flow happens again but this time user doesnt need to enter credentials, but i can check in local storage that access token has changed.
rapaccinim commented 2 years ago

@jaredperreault-okta probably that was occuring because I was logged in OKTA's dev admin dashboard.

Anyway, I made a change to the code in secureRoute and now I don't have anymore unexpected behaviours.

Here is the code, a double check with the OKTA team will help me (and I hope other devs) to be sure about the correct implementation:

const login = async (oktaAuth:OktaAuth) => {
    await oktaAuth.signInWithRedirect({ originalUri: '/' })
}

const RequiredAuth = () => {
    const { oktaAuth, authState } = useOktaAuth();
    if(!authState) return <Loading/>
    if (authState.isAuthenticated){
        return (<Outlet />);
    }else{
        login(oktaAuth).catch((err:Error) => ErrorDisplay(err, "login with OKTA", "secureRoute"))
        return null
    }
}

Thank you in advance!

erichardson30 commented 2 years ago

@rapaccinim I was running into similar issues as well. After I would log in to my app via okta and then refresh the page, the content would load, flash the loading screen for half a second and then render again. This seems to have fixed it.

I'm still not fully understanding why this issue has been open for almost 2 years now and all we can get is a work around for the latest fully supported version of react-router since this package is so highly dependent on it. What can we do to speed this up? Or does this package just not have first-class support?

rapaccinim commented 2 years ago

@erichardson30 good to know that it worked for you, but it would be great if someone from OKTA's team could confirm that my solution is right/safe.

ZinkNotTheMetal commented 2 years ago

I found the follwing example (using Vite and TypeScript) in the documentation: React Router Dom v6 Example

Following the steps in the README.md

  1. I run yarn
  2. I put in the testenv file at the top level
  3. I fill it out with my environment details

The website will not load and says

Cannot GET /

Has anyone found this example and got it to work? I need something to follow to get Okta configured.

shuowu commented 2 years ago

@ZinkNotTheMetal The sample works fine for me. Maybe try following commands at the root dir of okta-react:

  1. pull latest master branch of okta-react
  2. add testenv
  3. run commnads
    yarn
    yarn workspace @okta/samples.react.react-router-dom-v6 start
ZinkNotTheMetal commented 2 years ago

I did all of those... still same error :(

image