From f26e777eb868407b9631995102382c990fe96155 Mon Sep 17 00:00:00 2001 From: Shuo Wu Date: Wed, 5 Aug 2020 17:31:56 -0400 Subject: [PATCH] fix[react]: onSessionExpired default behaviour (#848) --- packages/okta-react/CHANGELOG.md | 6 ++ packages/okta-react/README.md | 21 ++++++- packages/okta-react/package.json | 4 +- packages/okta-react/src/AuthService.js | 24 +++++--- packages/okta-react/src/LoginCallback.js | 10 ++-- packages/okta-react/src/SecureRoute.js | 12 ++-- packages/okta-react/src/Security.js | 5 +- .../okta-react/test/jest/authService.test.js | 55 +++++++++++-------- .../test/jest/loginCallback.test.js | 16 ++---- .../okta-react/test/jest/secureRoute.test.js | 7 ++- .../okta-react/test/jest/security.test.js | 20 ++++++- packages/okta-react/yarn.lock | 30 +++++++--- 12 files changed, 141 insertions(+), 69 deletions(-) diff --git a/packages/okta-react/CHANGELOG.md b/packages/okta-react/CHANGELOG.md index a73392e6f..9ab909f84 100644 --- a/packages/okta-react/CHANGELOG.md +++ b/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 diff --git a/packages/okta-react/README.md b/packages/okta-react/README.md index 30475359b..86794da32 100644 --- a/packages/okta-react/README.md +++ b/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); + } + }); + + ``` + - **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. diff --git a/packages/okta-react/package.json b/packages/okta-react/package.json index eedc21382..3c5b4f475 100644 --- a/packages/okta-react/package.json +++ b/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" }, diff --git a/packages/okta-react/src/AuthService.js b/packages/okta-react/src/AuthService.js index edb4e363a..c2f9178db 100644 --- a/packages/okta-react/src/AuthService.js +++ b/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) { diff --git a/packages/okta-react/src/LoginCallback.js b/packages/okta-react/src/LoginCallback.js index 4ab8b94ed..e1404debf 100644 --- a/packages/okta-react/src/LoginCallback.js +++ b/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 ; diff --git a/packages/okta-react/src/SecureRoute.js b/packages/okta-react/src/SecureRoute.js index 0d8c7c96b..419722315 100644 --- a/packages/okta-react/src/SecureRoute.js +++ b/packages/okta-react/src/SecureRoute.js @@ -10,7 +10,7 @@ * 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'; @@ -18,11 +18,15 @@ 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; } diff --git a/packages/okta-react/src/Security.js b/packages/okta-react/src/Security.js index cbbe25d37..506fc9aba 100644 --- a/packages/okta-react/src/Security.js +++ b/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]); diff --git a/packages/okta-react/test/jest/authService.test.js b/packages/okta-react/test/jest/authService.test.js index 036da490e..e0c1ddcf8 100644 --- a/packages/okta-react/test/jest/authService.test.js +++ b/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', () => { diff --git a/packages/okta-react/test/jest/loginCallback.test.js b/packages/okta-react/test/jest/loginCallback.test.js index a075033f0..3feb02d83 100644 --- a/packages/okta-react/test/jest/loginCallback.test.js +++ b/packages/okta-react/test/jest/loginCallback.test.js @@ -15,7 +15,12 @@ describe('', () => { 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('', () => { expect(authService.handleAuthentication).toHaveBeenCalledTimes(1); }); - it('does not call handleAuthentication when authState.isPending', () => { - mount( - - - - ); - expect(authService.handleAuthentication).toHaveBeenCalledTimes(0); - }); - describe('shows errors', () => { it('does not render errors without an error', () => { const wrapper = mount( diff --git a/packages/okta-react/test/jest/secureRoute.test.js b/packages/okta-react/test/jest/secureRoute.test.js index 587662256..b3cd18dd0 100644 --- a/packages/okta-react/test/jest/secureRoute.test.js +++ b/packages/okta-react/test/jest/secureRoute.test.js @@ -17,7 +17,12 @@ describe('', () => { 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 }; }); diff --git a/packages/okta-react/test/jest/security.test.js b/packages/okta-react/test/jest/security.test.js index a45c2b320..132241909 100644 --- a/packages/okta-react/test/jest/security.test.js +++ b/packages/okta-react/test/jest/security.test.js @@ -20,7 +20,12 @@ describe('', () => { 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('', () => { 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( + + + + ); + expect(authService.updateAuthState).not.toHaveBeenCalled(); + }); + it('subscribes to "authStateChange" and updates the context', () => { const mockAuthStates = [ initialAuthState, diff --git a/packages/okta-react/yarn.lock b/packages/okta-react/yarn.lock index 5cf7bf41f..69cc382bb 100644 --- a/packages/okta-react/yarn.lock +++ b/packages/okta-react/yarn.lock @@ -115,10 +115,10 @@ version "0.4.1" resolved "https://registry.yarnpkg.com/@okta/configuration-validation/-/configuration-validation-0.4.1.tgz#6fa4520bc96c27b3d7aedcb0523de1fbceee9105" -"@okta/okta-auth-js@^3.1.2": - version "3.1.2" - resolved "https://registry.yarnpkg.com/@okta/okta-auth-js/-/okta-auth-js-3.1.2.tgz#4046bd006a5d311b222d17f05f3d2a567c11db41" - integrity sha512-Z3Ay1axae2EMTcK7oPJZ0DnhmDZByaYmfnRbjr2UVPrnCWJFZuEPpIal59mC75QoPQNn95RaS/gclnXhM0exJg== +"@okta/okta-auth-js@^3.2.2": + version "3.2.2" + resolved "https://registry.yarnpkg.com/@okta/okta-auth-js/-/okta-auth-js-3.2.2.tgz#a6af150b76741ebe16d3541db182387172daf5f1" + integrity sha512-1pq1l8FVQsrtC4P2eSsWVByIODg4/93g+KJ0XWi6s4Pl6C52EZayvFkac8+FfOfy5CSnAaYQij7bD/V39AuROQ== dependencies: Base64 "0.3.0" cross-fetch "^3.0.0" @@ -142,6 +142,7 @@ Base64@0.3.0: version "0.3.0" resolved "https://registry.yarnpkg.com/Base64/-/Base64-0.3.0.tgz#6da261a4e80d4fa0f5c684254e5bccd16bbdce9f" + integrity sha1-baJhpOgNT6D1xoQlTlvM0Wu9zp8= abab@^2.0.0: version "2.0.3" @@ -1403,6 +1404,7 @@ cliui@^4.0.0: clone@2.x: version "2.1.2" resolved "https://registry.yarnpkg.com/clone/-/clone-2.1.2.tgz#1b7f4b9f591f1e8f83670401600345a02887435f" + integrity sha1-G39Ln1kfHo+DZwQBYANFoCiHQ18= co@^4.6.0: version "4.6.0" @@ -1495,11 +1497,11 @@ core-util-is@1.0.2, core-util-is@~1.0.0: resolved "https://registry.yarnpkg.com/core-util-is/-/core-util-is-1.0.2.tgz#b5fd54220aa2bc5ab57aab7140c940754503c1a7" cross-fetch@^3.0.0: - version "3.0.4" - resolved "https://registry.yarnpkg.com/cross-fetch/-/cross-fetch-3.0.4.tgz#7bef7020207e684a7638ef5f2f698e24d9eb283c" + version "3.0.5" + resolved "https://registry.yarnpkg.com/cross-fetch/-/cross-fetch-3.0.5.tgz#2739d2981892e7ab488a7ad03b92df2816e03f4c" + integrity sha512-FFLcLtraisj5eteosnX1gf01qYDCOc4fDy0+euOt8Kn9YBY2NtXL/pCoYPavw24NIQkQqm5ZOLsGD5Zzj0gyew== dependencies: node-fetch "2.6.0" - whatwg-fetch "3.0.0" cross-spawn@^5.1.0: version "5.1.0" @@ -3310,6 +3312,7 @@ jest@^23.6.0: js-cookie@2.2.0: version "2.2.0" resolved "https://registry.yarnpkg.com/js-cookie/-/js-cookie-2.2.0.tgz#1b2c279a6eece380a12168b92485265b35b1effb" + integrity sha1-Gywnmm7s44ChIWi5JIUmWzWx7/s= js-tokens@^3.0.0, js-tokens@^3.0.2: version "3.0.2" @@ -3519,10 +3522,15 @@ lodash.sortby@^4.7.0: version "4.7.0" resolved "https://registry.yarnpkg.com/lodash.sortby/-/lodash.sortby-4.7.0.tgz#edd14c824e2cc9c1e0b0a1b42bb5210516a42438" -lodash@^4.15.0, lodash@^4.17.14, lodash@^4.17.15, lodash@^4.17.4, lodash@^4.2.0, lodash@^4.3.0: +lodash@^4.15.0, lodash@^4.17.14, lodash@^4.17.4, lodash@^4.2.0, lodash@^4.3.0: version "4.17.15" resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.15.tgz#b447f6670a0455bbfeedd11392eff330ea097548" +lodash@^4.17.15: + version "4.17.19" + resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.19.tgz#e48ddedbe30b3321783c5b4301fbd353bc1e4a4b" + integrity sha512-JNvd8XER9GQX0v2qJgsaN/mzFCNA5BRe/j8JN9d+tWyGLSodKQHKFicdwNYzWwI3wjRnaKPsGj1XkBjx/F96DQ== + loose-envify@^1.0.0, loose-envify@^1.1.0, loose-envify@^1.2.0, loose-envify@^1.3.1, loose-envify@^1.4.0: version "1.4.0" resolved "https://registry.yarnpkg.com/loose-envify/-/loose-envify-1.4.0.tgz#71ee51fa7be4caec1a63839f7e682d8132d30caf" @@ -3755,6 +3763,7 @@ nice-try@^1.0.4: node-cache@^4.2.0: version "4.2.1" resolved "https://registry.yarnpkg.com/node-cache/-/node-cache-4.2.1.tgz#efd8474dee4edec4138cdded580f5516500f7334" + integrity sha512-BOb67bWg2dTyax5kdef5WfU3X8xu4wPg+zHzkvls0Q/QpYycIFRLEEIdAx9Wma43DxG6Qzn4illdZoYseKWa4A== dependencies: clone "2.x" lodash "^4.17.15" @@ -3762,6 +3771,7 @@ node-cache@^4.2.0: node-fetch@2.6.0: version "2.6.0" resolved "https://registry.yarnpkg.com/node-fetch/-/node-fetch-2.6.0.tgz#e633456386d4aa55863f676a7ab0daa8fdecb0fd" + integrity sha512-8dG4H5ujfvFiqDmVu9fQ5bOHUC15JMjMY/Zumv26oOvvVJjM67KF8koCWIabKQ1GJIa9r2mMZscBq/TbdOcmNA== node-fetch@^1.0.1: version "1.7.3" @@ -5075,6 +5085,7 @@ through@^2.3.6: tiny-emitter@1.1.0: version "1.1.0" resolved "https://registry.yarnpkg.com/tiny-emitter/-/tiny-emitter-1.1.0.tgz#ab405a21ffed814a76c19739648093d70654fecb" + integrity sha1-q0BaIf/tgUp2wZc5ZICT1wZU/ss= tiny-emitter@^2.1.0: version "2.1.0" @@ -5324,7 +5335,7 @@ whatwg-encoding@^1.0.1, whatwg-encoding@^1.0.3: dependencies: iconv-lite "0.4.24" -whatwg-fetch@3.0.0, whatwg-fetch@>=0.10.0: +whatwg-fetch@>=0.10.0: version "3.0.0" resolved "https://registry.yarnpkg.com/whatwg-fetch/-/whatwg-fetch-3.0.0.tgz#fc804e458cc460009b1a2b966bc8817d2578aefb" @@ -5406,6 +5417,7 @@ ws@^5.2.0: xhr2@0.1.3: version "0.1.3" resolved "https://registry.yarnpkg.com/xhr2/-/xhr2-0.1.3.tgz#cbfc4759a69b4a888e78cf4f20b051038757bd11" + integrity sha1-y/xHWaabSoiOeM9PILBRA4dXvRE= xml-name-validator@^3.0.0: version "3.0.0"