Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LoginCallback useEffect is impure and multiple runs will trigger AuthSdkError #263

Open
trescenzi opened this issue Aug 4, 2023 · 1 comment
Labels
bug Something isn't working

Comments

@trescenzi
Copy link

Describe the bug

Calling oktaAuth.handleLoginRedirect() multiple times before in a row will cause parseFromUrl within okta-auth-js to throw.

This is expected as the auth flow expects to only be running once at a time. However with React18 in development mode every effect is run twice as documented here. As a result the issue must be resolved within this package.

Reproduction Steps?

I've created a reproducer here on codesandbox however it's a bit annoying to use as you have to enter your okta instance information to use it effectively.

A stripped down reproducer is as follows(note you must use React18 and have dev mode on):

import {
  createBrowserRouter,
  createRoutesFromElements,
  Route,
  RouterProvider
} from "react-router-dom";
import { Security, LoginCallback } from "@okta/okta-react";

const oktaConfig = {}//your okta config;
const router = createBrowserRouter(
  createRoutesFromElements(
    <Route
      path="/"
      element={() => {
        const auth = useOktaAuth();
        const login = async () => auth.oktaAuth?.signInWithRedirect();
        return (
        <button
          disabled={auth.authState?.isAuthenticated}
          onClick={login}
          type="button"
        >
          {auth.authState?.isAuthenticated ? "Logged in" : "Login"}
        </button>
        );
     }}
    >
      <Route
        path="/login/callback"
        element={
          <LoginCallback
            loadingElement={<h1>loading<h1>}
            errorComponent={(e) => {
              console.log("AN ERROR HAS OCCURRED DURING LOGIN CALLBACK");
              return (
                <p>
                  {e?.name}:{e?.message}
                </p>
              );
            }}
          />
        }
      />
    </Route>
  )
);

export default function App() {
  return (
      <Security
        oktaAuth={oktaConfig}
        restoreOriginalUri={() => (window.location = "/")}
      >
        <RouterProvider router={router} />
      </Security>
  );
}

SDK Versions

  System:
    OS: macOS 13.4.1
    CPU: (8) arm64 Apple M1
    Memory: 104.86 MB / 16.00 GB
    Shell: 3.6.1 - /opt/homebrew/bin/fish
  Binaries:
    Node: 16.20.0 - /opt/homebrew/opt/node@16/bin/node
    Yarn: 1.22.19 - /opt/homebrew/bin/yarn
    npm: 8.19.4 - /opt/homebrew/opt/node@16/bin/npm
  Browsers:
    Chrome: 115.0.5790.170
    Chrome Canary: 49.0.2566.0
    Firefox: 109.0
    Safari: 16.5.1
  npmPackages:
    @okta/okta-auth-js: ^7.2.0 => 7.3.0
    @okta/okta-react: ^6.7.0 => 6.7.0
    @okta/okta-signin-widget: ^6.1.0 => 6.9.0
    react: 18.2.0 => 18.2.0
    react-azure-maps: ^0.4.4 => 0.4.6
    react-azure-maps-next: ^0.4.4-a => 0.4.4a
    react-dom: 18.2.0 => 18.2.0
    react-error-boundary: ^3.1.4 => 3.1.4
    react-intl: ^6.2.10 => 6.4.2
    react-markdown: ^8.0.7 => 8.0.7
    react-pdf: ^5.7.2 => 5.7.2
    react-reveal: ^1.2.2 => 1.2.2
    react-router-dom: ^6.10.0 => 6.10.0
    react-test-renderer: ^18.2.0 => 18.2.0
    react-use: ^17.4.0 => 17.4.0
    react-use-file-upload: ^0.9.5 => 0.9.5
    react-waypoint: ^10.3.0 => 10.3.0

Additional Information

A potential solution to this issue is as follows and is based off of React's example of how to handle app initialization

let handlingRedirect = false;

const LoginCallback: React.FC<LoginCallbackProps> = ({ errorComponent, loadingElement = null, onAuthResume }) => { 
  const { oktaAuth, authState } = useOktaAuth();
  const [callbackError, setCallbackError] = React.useState(null);
  
  const ErrorReporter = errorComponent || OktaError;
  React.useEffect(() => {
    // eslint-disable-next-line @typescript-eslint/ban-ts-comment
    // @ts-ignore OKTA-464505: backward compatibility support for auth-js@5
    const isInteractionRequired = oktaAuth.idx.isInteractionRequired || oktaAuth.isInteractionRequired.bind(oktaAuth);
    if (onAuthResume && isInteractionRequired()) {
      onAuthResume();
      return;
    }
    if (handlingRedirect) {
      handlingRedirect = true;
      oktaAuth.handleLoginRedirect().catch(e => {
        setCallbackError(e);
      }).finally(() => handlingRedirect = false);
    }
  }, [oktaAuth]);

  const authError = authState?.error;
  const displayError = callbackError || authError;
  if (displayError) { 
    return <ErrorReporter error={displayError}/>;
  }

  return loadingElement;
};

export default LoginCallback;

I'm happy to open a PR with that change but unsure if that's the approach you'd like. Thank you for taking the time to read through this issue 😸

@trescenzi trescenzi added the bug Something isn't working label Aug 4, 2023
@jaredperreault-okta
Copy link
Contributor

@trescenzi Thank you sincerely for the thorough bug report. I will look into this

Internal Ref: OKTA-635977

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants