okta / okta-signin-widget

HTML/CSS/JS widget that provides out-of-the-box authentication UX for your organization's apps
Other
375 stars 317 forks source link

"An instance of the widget has already been rendered. Call remove() first." after widget.remove() was called. #1499

Open patkovskyi opened 4 years ago

patkovskyi commented 4 years ago

I'm submitting a

Background info

In one of the projects, we were using okta-signin-widget 3.2.2 Due to a recently found CVE in one of okta-signin-widget's transitive dependencies (handlebars, https://nvd.nist.gov/vuln/detail/CVE-2019-20922), we are forced to upgrade it to a newer version (currently trying to upgrade either to 3.9.2 or 4.5.2).

One of our tests started failing after the upgrade.

Expected behavior

Calling widget.remove should prevent an error when widget.renderEl is called next time.

What went wrong?

We have a test suite that runs 2 OktaSigninWidget tests.

The problem: after running the first test, the widget is not properly removed even though widget.remove() is clearly called in the componentWillUnmount. We're seeing an error Okta Widget Init Error: Error: An instance of the widget has already been rendered. Call remove() first. when the second test calls widget.renderEl, and the second test fails.

When I switch the test order, both pass 🤔

Steps to reproduce

Widget code:

import '@okta/okta-signin-widget/dist/css/okta-sign-in.min.css';
import OktaSignIn from '@okta/okta-signin-widget';
import React from 'react';
import ReactDOM from 'react-dom';

import { OktaConfig } from '../../../../config/app-config.interface';
import { Okta } from '../../../../utils/declarations/okta';

export interface Props {
  okta: OktaConfig;
  onSuccess: (response: Okta.AuthenticationResponse) => void;
  onError: (error: Error) => void;
}

export default class OktaSignInWidget extends React.Component<Props> {
  widget: typeof OktaSignIn;

  componentDidMount() {
    try {
      const el = ReactDOM.findDOMNode(this); // eslint-disable-line react/no-find-dom-node
      const { baseUrl, issuer, clientId, redirectUri } = this.props.okta;
      this.widget = new OktaSignIn({
        baseUrl,
        clientId,
        redirectUri,
        authParams: {
          issuer,
          pkce: false,
          autoRenew: false,
          display: 'page',
          responseType: ['id_token', 'token'],
          scopes: ['openid', 'email', 'profile', 'groups']
        }
      });

      this.widget.renderEl({ el }, this.props.onSuccess, this.props.onError);
    } catch (e) {
      console.error('Okta Widget Init Error:', e);
    }
  }

  componentWillUnmount() {
    try {
      if (this.widget) {
        this.widget.remove();
      }
    } catch (e) {}
  }

  render() {
    return <div />;
  }
}

Our test code:

import React from 'react';
import ReactDOM from 'react-dom';

import { mount } from '../../../../utils/tests/enzyme';

import OktaSignInWidget, { Props } from './OktaSignInWidget';

describe('OktaSignInWidget', () => {
  describe('Component', () => {
    let props: Props;
    beforeEach(() => {
      props = {
        okta: {
          baseUrl: 'https://dev-...okta.com/',
          clientId: '...',
          redirectUri: 'http://localhost:3000/implicit/callback',
          issuer: 'https://dev-....okta.com/oauth2/default'
        },
        onSuccess: jest.fn(),
        onError: jest.fn()
      };
    });
    it('App renders without crashing', () => {
      const div = document.createElement('div');
      ReactDOM.render(<OktaSignInWidget {...props} />, div);
      ReactDOM.unmountComponentAtNode(div);
    });
    it('Should render correctly', () => {
      expect(mount(<OktaSignInWidget {...props} />).html()).toMatchSnapshot();
    });
  });
});

Your environment

aarongranick-okta commented 4 years ago

@patkovskyi Thank you for the submission. While I can't say exactly what is causing this error, your observation that test run order matters does seem to indicate there is a case of information leaking from one test to another. In the version you are using, the error is being thrown here: https://github.com/okta/okta-signin-widget/blob/4.5/src/widget/OktaSignIn.js#L17 This should only occur if the same instance of the widget is rendered twice. It should not occur for two separate widget instances rendering. From the code you shared, I do not see how that is occurring; it appears to be two separate widget instances.

It may be helpful to wait for the "success" callback to fire. There is some asynchronous logic in the widget and it may not be fully rendered synchronously. You would not want to have actions still occurring after a test has completed. It is possible those actions may affect another test which is running.

You may have a better experience with version 5.0.0 of the widget. It includes the following changes:

patkovskyi commented 4 years ago

@aarongranick-okta thank you for taking a look and a quick reply, I'll try 5.0.0.

patkovskyi commented 4 years ago

Unfortunately, with 5.0.0 the success callback receives response object that lacks fields we use in our existing code, for example user field (we used response.user.profile) and response.session.setCookieAndRedirect() function we used.

aarongranick-okta commented 4 years ago

@patkovskyi There were some other changes in 5.0, but it should be easy to fix.

We are discouraging the use of renderEl and setCookieAndRedirect for OIDC flows. Your app appears to be a SPA application that is using the "implicit" flow (not PKCE). You have a choice of whether you would like to redirect to Okta and receive tokens on a callback URL, or whether you would like to receive tokens directly from the signin widget, with no redirect.

To receive tokens directly, without any redirect, use showSignInToGetTokens: https://github.com/okta/okta-signin-widget#showsignintogettokens Note that you should pass the "scopes" to this method. You may also pass the "el" to this method, or set it in the widget constructor. The tokens will be returned on the promise.

To continue using a redirect, use showSignInAndRedirect: https://github.com/okta/okta-signin-widget#showsigninandredirect This should work the same as renderEl did previously with the options you have set.