fix[react]: onSessionExpired default behaviour #848

shuowu · 2020-07-29T22:37:35Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

What is the current behavior?

Issue Number: OKTA-318290

What is the new behavior?

Updated auth-js dependency to 3.2.2
Removed default onSessionExpired callback
Added isPendng authState when login happens to avoid concurrent login flow
Moved login logic to useEffect in SecureRoute component

Does this PR introduce a breaking change?

Yes
No

Other information

Tested SecureRoute Comp for both okta-hosted-signIn and custom-signIn

Reviewers

packages/okta-react/CHANGELOG.md

swiftone · 2020-07-29T22:48:26Z

packages/okta-react/README.md

@@ -314,7 +314,7 @@ 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.
+- **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 [clearAuthState](##authserviceclearauthstate) to clear authState in context. Passing a function here will disable the default handler.


I thought we had decided that the default handler would do nothing, and we'd separately listen to the expired event to call clearAuthState.

I think this change should make sense: 7311f47

I believe the effect here is that if the accessToken cannot be renewed, the idToken will also be removed. This might be the right logical choice, but there could be a timing error if the idToken is checked first. If we want to invalidate all tokens when accessToken renew fails, we would need to make sure to always try to retrieve accessToken before idToken. This is kind of subtle and error prone. Maybe a better choice is to make default isAuthenticated logic only true if BOTH idToken and accessToken are present.

That change would definitely be a major version change - discussing that change is what led to the idea of removing isAuthenticated entirely

I also don't think that renewal failure of the accessToken in any way invalidates the idToken. That requires expiration or revocation.

https://github.com/okta/okta-auth-js/blob/master/packages/okta-auth-js/lib/browser/browser.js#L184
Looks like based on current auth-js implementation, onSessionExired callback will only be triggered on accessToken renew failure, then it would make sense to just clear accessToken in context.

But how should we handle idToken renew failure and isAuthenticated state. I think the purpose for this fix is to update the whole authState to keep user out of SecureRoute.

No - the purpose is to have authState be accurate. If only accessToken is invalid, then authState is updated. (updateAuthState instead of clearAuthState).

it might will cause infinite loop. error -> updateAuthState -> error -> ...
might need some thoughts on when onSessionExpired be triggered first, imo.

packages/okta-react/package.json

packages/okta-react/src/AuthService.js

packages/okta-react/src/SecureRoute.js

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

shuowu · 2020-07-31T16:08:23Z

Should also include fixes from okta/okta-auth-js#440 to avoid loop from signOut.

swiftone · 2020-08-04T17:28:36Z

packages/okta-react/src/SecureRoute.js

-      const fromUri = history.createHref(history.location);
-      authService.login(fromUri);
-    }
+  useEffect(() => {


I like this use of useEffect

swiftone

Other than my question about potential infinite loops from dev code, lgtm

aarongranick-okta · 2020-08-04T17:36:00Z

packages/okta-react/src/SecureRoute.js

-    }
+  useEffect(() => {
+    if(!authState.isAuthenticated && !authState.isPending) { 
+      authService.login();


Is this where it is possible to begin concurrent login attempts? If the authState is updated (due to token changes, etc). Do we need to add aa concurrent check on this or place this call into the PromiseQ?

Yes. Probably the isPending state is not enough here as it's triggered by emitting an event, which may still run into concurrent flow.

Resolved by providing pending state in the login method to make sure it only be called once.

aarongranick-okta · 2020-08-04T17:38:25Z

packages/okta-react/README.md

@@ -314,7 +314,8 @@ 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.
+- **onSessionExpired** *(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.


We have this marked as deprecated in authJS 3.2.2. I think we can remove it from the documentation here.

Still kept for this patch fix, marked as deprecated.

aarongranick-okta · 2020-08-04T17:38:57Z

packages/okta-react/CHANGELOG.md

+
+### Bug Fixes
+
+- [#848](https://github.com/okta/okta-oidc-js/pull/848) Change default `onSessionExpired` behaviour to `authService.clearAuthState`


removes onSessionExpired behavior

aarongranick-okta

See comments.

major version bump
remove onSessionExpired
change logic on isAuthenticated to return false if either token is missing
double check for concurrency errors with useEffect -> login()

swiftone · 2020-08-05T18:04:09Z

packages/okta-react/CHANGELOG.md

@@ -1,3 +1,9 @@
+# 3.0.4


I (again) thought we had decided on a major version bump (see comments from Aaron) - what am I missing?

I think we decide to make remove default onSessionExpired as a patch fix to avoid noise. Then put the isAuthenticated behavior change into 4.0.

Right. We agreed that removing onSessionExpired is a fix not a new feature or breaking change. It was never working as intended, and has mostly negative side effects. The positive side effect was triggering a login if only the accessToken is missing, but this can be done better by the user providing an isAuthenticated function which asserts accessToken && idToken. We will change this to be default logic in the next version but it would likely be considered a breaking change.

See comments.

major version bump

remove onSessionExpired

change logic on isAuthenticated to return false if either token is missing

double check for concurrency errors with useEffect -> login()

As we discussed we will push breaking change with isAuthenticated to subsequent release that includes [email protected].

I think we can update the README to provide better guidance for users of this version. (see comments).

aarongranick-okta · 2020-08-05T18:25:16Z

packages/okta-react/README.md

- **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.
+  > :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 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.


Is this text correct? It says "if both" return a value. But I think the current logic is "if either" return a value. Let's double check this and also indicate that we plan on changing this default logic in the next version. And we recommend the user to provide a function here and assert both tokens are present, unless their application can function with only one token present.

We've marked this as deprecated, but don't say anything about what they should be doing instead.

aarongranick-okta · 2020-08-05T18:25:32Z

packages/okta-react/src/AuthService.js

@@ -196,12 +190,24 @@ class AuthService {
  }

  async login(fromUri, additionalParams) {
+    if(this._pending.handleLogin) { 


nice. thanks!

the closer the easier to manage, hah

If login should only be run once, we probably can move the check to auth-js v5.0.

The underlying token.getWithRedirect will be placed in the PromiseQueue, but there is additional logic here. One possible error would be calling login(myUri) and then calling login again with no parameters. This would overwrite the saved URI and affect the final redirect value.

Another option instead of silently ignoring a concurrent login attempt is to actually error out. Maybe we could do this in the next release? What I'd like is a way to compare authState with previous authState so we only trigger login if we are transitioning isAuthenticated from pending -> false or true -> false. A transition of false -> false should not start a login flow.

if we are transitioning isAuthenticated from pending -> false or true -> false

Interesting. I think that level of check is better done in auth-js, but either way it does look like a nice approach to guide developers away from pitfalls that can otherwise mostly work.

login might not only be triggered by isAuthenticated state change, users may also call it from callbacks which is out of our control. I think it still makes sense to protect it generally from concurrency cases.

aarongranick-okta

Please double check description of isAuthenticated in the README and let us also provide some guidance to verify both tokens. This will cover any side effects of removing onSessionExpired

swiftone · 2020-08-05T18:40:47Z

packages/okta-react/src/AuthService.js

+      if (this._config.onAuthRequired) {
+        return await this._config.onAuthRequired(this);
+      }
+      return await this.redirect(additionalParams);


Errors are just silently lost - is there anything there we'd want to inform the user of?

only finally been added here, but without catch, I think users still can catch errors if any happen.

scrunches face up in confusion
writes test code
blinks furiously
writes more test code
googles
Huh. This is not how I expected finally and errors to interact. I sit corrected.

Test added.

swiftone · 2020-08-05T18:41:36Z

packages/okta-react/README.md

@@ -314,7 +314,9 @@ 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.
+  > :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.


These warnings are just the advice I wanted, thanks!

shuowu · 2020-08-05T19:25:46Z

Please double check description of isAuthenticated in the README and let us also provide some guidance to verify both tokens. This will cover any side effects of removing onSessionExpired

Updated doc, also provided code example.

…Expired

* changelog fix * prevent concurrent login calls by handling pending state in authService * fix error state when load states in loginRedirect state

shuowu requested review from swiftone and aarongranick-okta July 29, 2020 22:37

swiftone reviewed Jul 29, 2020

View reviewed changes

packages/okta-react/CHANGELOG.md Show resolved Hide resolved

swiftone reviewed Jul 29, 2020

View reviewed changes

packages/okta-react/package.json Show resolved Hide resolved

amcdnl mentioned this pull request Jul 30, 2020

PKCE Code Verification Failure on Authentication #804

Closed

9 tasks

swiftone approved these changes Jul 30, 2020

View reviewed changes

swiftone self-requested a review July 30, 2020 16:42

swiftone reviewed Jul 30, 2020

View reviewed changes

packages/okta-react/src/AuthService.js Show resolved Hide resolved

swiftone reviewed Jul 30, 2020

View reviewed changes

packages/okta-react/src/SecureRoute.js Outdated Show resolved Hide resolved

swiftone reviewed Jul 30, 2020

View reviewed changes

packages/okta-react/test/jest/authService.test.js Outdated Show resolved Hide resolved

shuowu-okta changed the title ~~fix[react]: onSessionExpired default behaviour~~ [WIP] fix[react]: onSessionExpired default behaviour Jul 31, 2020

shuowu mentioned this pull request Jul 31, 2020

ID Token disappears from localstorage upon expiration okta-react #842

Open

9 tasks

shuowu changed the title ~~[WIP] fix[react]: onSessionExpired default behaviour~~ fix[react]: onSessionExpired default behaviour Aug 4, 2020

shuowu requested a review from swiftone August 4, 2020 17:05

okta deleted a comment from shuowu Aug 4, 2020

swiftone reviewed Aug 4, 2020

View reviewed changes

swiftone approved these changes Aug 4, 2020

View reviewed changes

aarongranick-okta reviewed Aug 4, 2020

View reviewed changes

aarongranick-okta suggested changes Aug 4, 2020

View reviewed changes

swiftone self-requested a review August 4, 2020 17:55

swiftone mentioned this pull request Aug 4, 2020

Refresh Token #750

Open

7 tasks

shuowu requested a review from aarongranick-okta August 5, 2020 02:51

swiftone reviewed Aug 5, 2020

View reviewed changes

aarongranick-okta reviewed Aug 5, 2020

View reviewed changes

aarongranick-okta approved these changes Aug 5, 2020

View reviewed changes

aarongranick-okta suggested changes Aug 5, 2020

View reviewed changes

swiftone reviewed Aug 5, 2020

View reviewed changes

shuowu requested a review from aarongranick-okta August 5, 2020 19:25

aarongranick-okta approved these changes Aug 5, 2020

View reviewed changes

shuowu added 8 commits August 5, 2020 17:20

fix[react]: onSessionExpired default behaviour

60f4c26

chore: version bump and changelog

9ce55a3

fix: call both default behaviour and customized behavior in onSession…

c3e7f09

…Expired

fix: authState update based on token renew error

6f6f4e9

update tests and doc

4c473de

update authjs dependency & add pending check for login flows

526d357

Resolves comments

42ad32f

* changelog fix * prevent concurrent login calls by handling pending state in authService * fix error state when load states in loginRedirect state

resolve comments

72e82aa

shuowu-okta force-pushed the sw-fix-onSessionExpired-default-behaviour-OKTA-318290 branch from 1bf651c to 72e82aa Compare August 5, 2020 21:21

shuowu-okta merged commit f26e777 into master Aug 5, 2020

CristianoYL mentioned this pull request Oct 7, 2020

okta-react LoginCallback not displaying error if user not assigned application #926

Closed

8 tasks

dependabot bot mentioned this pull request Mar 12, 2021

Bump @okta/okta-react from 1.4.1 to 5.0.0 Fotogjengen/hilfling-frontend#97

Closed


		### Bug Fixes

		- [#848](https://github.com/okta/okta-oidc-js/pull/848) Change default `onSessionExpired` behaviour to `authService.clearAuthState`

fix[react]: onSessionExpired default behaviour #848

fix[react]: onSessionExpired default behaviour #848

Conversation

shuowu commented Jul 29, 2020 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Reviewers

swiftone Jul 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuowu commented Jul 31, 2020

Choose a reason for hiding this comment

swiftone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarongranick-okta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarongranick-okta left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuowu commented Aug 5, 2020

shuowu commented Jul 29, 2020 •

edited

Loading

swiftone Jul 29, 2020 •

edited

Loading

aarongranick-okta left a comment •

edited

Loading