fix[react]: onSessionExpired default behaviour (#848)

packages/okta-react/CHANGELOG.md

@@ -1,3 +1,9 @@
+# 3.0.4
+
+### Bug Fixes
+
+- [#848](https://github.com/okta/okta-oidc-js/pull/848) Removes `onSessionExpired` behavior.
+
 # 3.0.3
 
 ### Bug Fixes

packages/okta-react/README.md

@@ -314,8 +314,25 @@ These options are used by `Security` to configure the [Auth service][]. The most
 - **onAuthRequired** *(optional)* - callback function. Called when authentication is required. If this is not supplied, `okta-react` redirects to Okta. This callback will receive `authService` as the first function parameter. This is triggered when:
     1. [login](#authserviceloginfromuri-additionalparams) is called
     2. A `SecureRoute` is accessed without authentication
-- **onSessionExpired** *(optional)* - callback function. Called when the Okta SSO session has expired or was ended outside of the application. This SDK adds a default handler which will call [login](#authserviceloginfromuri-additionalparams) to initiate a login flow. Passing a function here will disable the default handler.
-- **isAuthenticated** *(optional)* - callback function. By default, `authService` will consider a user authenticated if both `getIdToken()` and `getAccessToken()` return a value. Setting a `isAuthenticated` function on the config will skip the default logic and call the supplied function instead. The function should return a Promise and resolve to either true or false.  Note that this is only evaluated when the `auth` code has reason to think the authentication state has changed.  You can call the `authService.updateAuthState()` method to trigger a re-evaluation.
+  > :warning: DO NOT trigger `authService.login()` in this callback. This callback is used inside the `login` method, call it again will trigger the protection logic to end the function.
+- **onSessionExpired (deprecated)** *(optional)* - callback function. Called when the Okta SSO session has expired or was ended outside of the application. This SDK provides an empty function as the default behaviour. Passing a function here will disable the default handler.
+  > :warning: DO NOT trigger token renew process, like `tokenManager.get()` or `tokenManager.renew()`, in this callback as it may end up with infinite loop.
+- **isAuthenticated** *(optional)* - callback function. By default, `authService` will consider a user authenticated if either `getIdToken()` or `getAccessToken()` return a value. Setting a `isAuthenticated` function on the config will skip the default logic and call the supplied function instead. The function should return a Promise and resolve to either true or false. This callback is only evaluated when the `auth` code has reason to think the authentication state has changed, by default it's been triggered when token state changes. You can call the `authService.updateAuthState()` method to trigger a re-evaluation.
+
+  **NOTE** The default behavior of this callback will be changed to resolve to true only when both `getIdToken()` and `getAccessToken()` return a value in the next major release. Currently, you can achieve the coming default behavior by
+
+  ```jsx
+  const authService = new AuthService({
+    // ...other configs
+    isAuthenticated: async () => {
+      const idToken = await authService.getTokenManager().get('idToken');
+      const accessToken = await authService.getTokenManager().get('accessToken');
+      return !!(idToken && accessToken);
+    }
+  });
+  <Security authService={authService} />
+  ```
+
 - **tokenManager** *(optional)*: An object containing additional properties used to configure the internal token manager. See [AuthJS TokenManager](https://github.com/okta/okta-auth-js#the-tokenmanager) for more detailed information.
   - `autoRenew` *(optional)*:
   By default, the library will attempt to renew expired tokens. When an expired token is requested by the library, a renewal request is executed to update the token. If you wish to  to disable auto renewal of tokens, set autoRenew to false.

packages/okta-react/package.json

@@ -1,6 +1,6 @@
 {
   "name": "@okta/okta-react",
-  "version": "3.0.3",
+  "version": "3.0.4",
   "description": "React support for Okta",
   "main": "./dist/index.js",
   "scripts": {
@@ -34,7 +34,7 @@
   "homepage": "https://github.com/okta/okta-oidc-js#readme",
   "dependencies": {
     "@okta/configuration-validation": "^0.4.1",
-    "@okta/okta-auth-js": "^3.1.2",
+    "@okta/okta-auth-js": "^3.2.2",
     "babel-runtime": "^6.26.0",
     "prop-types": "^15.5.10"
   },

packages/okta-react/src/AuthService.js

@@ -32,12 +32,6 @@ class AuthService {
     assertClientId(authConfig.clientId);
     assertRedirectUri(authConfig.redirectUri);
 
-    // Automatically enter login flow if session has expired or was ended outside the application
-    // The default behavior can be overriden by passing your own function via config: `config.onSessionExpired`
-    if (!authConfig.onSessionExpired) {
-      authConfig.onSessionExpired = this.login.bind(this);
-    }
-
     this._oktaAuth = new OktaAuth(authConfig);
     this._oktaAuth.userAgent = `${packageInfo.name}/${packageInfo.version} ${this._oktaAuth.userAgent}`;
     this._config = authConfig; // use normalized config
@@ -196,12 +190,24 @@ class AuthService {
   }
 
   async login(fromUri, additionalParams) {
+    if(this._pending.handleLogin) { 
+      // Don't trigger second round
+      return;
+    }
+
+    this._pending.handleLogin = true;
+    // Update UI pending state
+    this.emitAuthState({ ...this.getAuthState(), isPending: true });
     // Save the current url before redirect
     this.setFromUri(fromUri); // will save current location if fromUri is undefined
-    if (this._config.onAuthRequired) {
-      return this._config.onAuthRequired(this);
+    try {
+      if (this._config.onAuthRequired) {
+        return await this._config.onAuthRequired(this);
+      }
+      return await this.redirect(additionalParams);
+    } finally {
+      this._pending.handleLogin = null;
     }
-    return this.redirect(additionalParams);
   }
 
   _convertLogoutPathToOptions(redirectUri) { 

packages/okta-react/src/LoginCallback.js

@@ -14,17 +14,15 @@ import React, { useEffect } from 'react';
 import { useOktaAuth } from './OktaContext';
 import OktaError from './OktaError';
 
-const LoginCallback = ({ errorComponent}) => { 
+const LoginCallback = ({ errorComponent }) => { 
   const { authService, authState } = useOktaAuth();
   const authStateReady = !authState.isPending;
 
   let ErrorReporter = errorComponent || OktaError;
 
-  useEffect( () => {
-    if( authStateReady ) { // wait until initial check is done so it doesn't wipe any error
-      authService.handleAuthentication();
-    }
-  }, [authService, authStateReady]);
+  useEffect(() => {
+    authService.handleAuthentication();
+  }, [authService]);
 
   if(authStateReady && authState.error) { 
     return <ErrorReporter error={authState.error}/>;

packages/okta-react/src/SecureRoute.js

@@ -10,19 +10,23 @@
  * See the License for the specific language governing permissions and limitations under the License.
  */
 
-import React from 'react';
+import React, { useEffect } from 'react';
 import { useOktaAuth } from './OktaContext';
 import { useHistory, Route } from 'react-router-dom';
 
 const RequireAuth = ({ children }) => { 
   const { authService, authState } = useOktaAuth();
   const history = useHistory();
 
-  if(!authState.isAuthenticated) { 
-    if(!authState.isPending) { 
+  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;
   }
 

packages/okta-react/src/Security.js

@@ -29,7 +29,10 @@ const Security = (props) => {
       setAuthState(authService.getAuthState());
     });
 
-    authService.updateAuthState(); // Trigger an initial change event to make sure authState is latest
+    if (!authService._oktaAuth.token.isLoginRedirect()) {
+      // Trigger an initial change event to make sure authState is latest when not in loginRedirect state
+      authService.updateAuthState(); 
+    }
     return unsub;
   }, [authService]);
 

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

@@ -214,28 +214,6 @@ describe('AuthService', () => {
     });
   });
 
-  describe('onSessionExpired', () => {
-    it('By default, sets a handler for "onSessionExpired" which calls login()', () => {
-      jest.spyOn(AuthService.prototype, 'login').mockReturnValue(undefined);
-      const authService = new AuthService(validConfig);
-      const config = authService._config;
-      expect(config.onSessionExpired).toBeDefined();
-      config.onSessionExpired();
-      expect(AuthService.prototype.login).toHaveBeenCalled();
-    });
-
-    it('Accepts custom function "onSessionExpired" via config which disables default handler', () => {
-      jest.spyOn(AuthService.prototype, 'login').mockReturnValue(undefined);
-      const onSessionExpired = jest.fn();
-      const authService = new AuthService(extendConfig({ onSessionExpired }));
-      const config = authService._config;
-      expect(config.onSessionExpired).toBe(onSessionExpired);
-      config.onSessionExpired();
-      expect(onSessionExpired).toHaveBeenCalled();
-      expect(AuthService.prototype.login).not.toHaveBeenCalled();
-    });
-  });
-
   describe('logout', () => {
 
     test('defaults to passing an empty options to signOut', async () => {
@@ -492,7 +470,7 @@ describe('AuthService', () => {
         redirectUri: 'https://foo/redirect',
       });
       const expectedVal = 'fakey';
-      jest.spyOn(authService, 'redirect').mockReturnValue(expectedVal);
+      jest.spyOn(authService, 'redirect').mockResolvedValue(expectedVal);
 
       const retVal = await authService.login('/');
       expect(retVal).toBe(expectedVal);
@@ -501,7 +479,7 @@ describe('AuthService', () => {
 
     it('will call a custom method "onAuthRequired" instead of redirect()', async () => {
       const expectedVal = 'fakey';
-      const onAuthRequired = jest.fn().mockReturnValue(expectedVal);
+      const onAuthRequired = jest.fn().mockResolvedValue(expectedVal);
       const authService = new AuthService({
         issuer: 'https://foo/oauth2/default',
         clientId: 'foo',
@@ -515,6 +493,35 @@ describe('AuthService', () => {
       expect(onAuthRequired).toHaveBeenCalledWith(authService);
       expect(authService.redirect).not.toHaveBeenCalled();
     });
+
+    it('should not trigger second call if login is in progress', async () => {
+      expect.assertions(1);
+      const authService = new AuthService({
+        issuer: 'https://foo/oauth2/default',
+        clientId: 'foo',
+        redirectUri: 'https://foo/redirect',
+      });
+      authService.redirect = jest.fn();
+      Promise.all([authService.login('/'), authService.login('/')]).then(() => {
+        expect(authService.redirect).toHaveBeenCalledTimes(1);
+      });
+    });
+
+    it('should throw when error happens', async () => {
+      expect.assertions(1);
+      const authService = new AuthService({
+        issuer: 'https://foo/oauth2/default',
+        clientId: 'foo',
+        redirectUri: 'https://foo/redirect',
+      });
+      const mockErrorMessage = 'mock error';
+      authService.redirect = jest.fn().mockRejectedValue(new Error(mockErrorMessage));
+      try {
+        await authService.login('/')
+      } catch (e) {
+        expect(e.message).toEqual(mockErrorMessage);
+      }
+    });
   });
 
   describe('AuthState tracking', () => { 

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

@@ -15,7 +15,12 @@ describe('<LoginCallback />', () => {
       on: jest.fn(),
       updateAuthState: jest.fn(),
       getAuthState: jest.fn().mockImplementation(() => authState),
-      handleAuthentication: jest.fn()
+      handleAuthentication: jest.fn(),
+      _oktaAuth: {
+        token: {
+          isLoginRedirect: jest.fn().mockImplementation(() => false)
+        }
+      }
     };
     mockProps = { authService };
   });
@@ -42,15 +47,6 @@ describe('<LoginCallback />', () => {
     expect(authService.handleAuthentication).toHaveBeenCalledTimes(1);
   });
 
-  it('does not call handleAuthentication when authState.isPending', () => {
-    mount(
-      <Security {...mockProps}>
-        <LoginCallback />
-      </Security>
-    );
-    expect(authService.handleAuthentication).toHaveBeenCalledTimes(0);
-  });
-
   describe('shows errors', () => {
     it('does not render errors without an error', () => { 
       const wrapper = mount(

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

@@ -17,7 +17,12 @@ describe('<SecureRoute />', () => {
       on: jest.fn(),
       updateAuthState: jest.fn(),
       getAuthState: jest.fn().mockImplementation(() => authState),
-      login: jest.fn()
+      login: jest.fn(),
+      _oktaAuth: {
+        token: {
+          isLoginRedirect: jest.fn().mockImplementation(() => false)
+        }
+      }
     };
     mockProps = { authService };
   });

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

@@ -20,7 +20,12 @@ describe('<Security />', () => {
     authService = {
       on: jest.fn(),
       updateAuthState: jest.fn(),
-      getAuthState: jest.fn().mockImplementation(() => initialAuthState)
+      getAuthState: jest.fn().mockImplementation(() => initialAuthState),
+      _oktaAuth: {
+        token: {
+          isLoginRedirect: jest.fn().mockImplementation(() => false)
+        }
+      }
     };
   });
 
@@ -86,6 +91,19 @@ describe('<Security />', () => {
     expect(MyComponent).toHaveBeenCalledTimes(2);
   });
 
+  it('should not call updateAuthState when in login redirect state', () => {
+    authService._oktaAuth.token.isLoginRedirect = jest.fn().mockImplementation(() => true);
+    const mockProps = {
+      authService
+    };
+    mount(
+      <MemoryRouter>
+        <Security {...mockProps} />
+      </MemoryRouter>
+    );
+    expect(authService.updateAuthState).not.toHaveBeenCalled();
+  });
+
   it('subscribes to "authStateChange" and updates the context', () => {
     const mockAuthStates = [
       initialAuthState,