breaking changes for 4.0 #444

aarongranick-okta · 2020-08-01T03:10:37Z

breaking changes

remove onSessionExpired
implement active autoRenew
remove renew logic from tokenManager.get()

new features

adds tokenManager.hasExpired(token) to test if a token is expired or not.

swiftone

I'd suggest adding a "Migration" section to the README like we have in okta-react, so people understand how to transition their code from 3.x to 4.x

swiftone · 2020-08-05T18:11:49Z

lib/TokenManager.ts

@@ -116,24 +119,9 @@ function get(storage, key) {
 }

 function getAsync(sdk, tokenMgmtRef, storage, key) {


Is there a reason not to declare this function async?

no real reason. it could be added, I don't think it would produce any side effect.

I think it makes sense to declare async when await is included, but not for a function just return a promise.

Why only then? Async tells the IDE and someone skimming the code that the function returns a promise. It's straight up information.

By returning a promise, the function still can be called with await to resolve the value. But with async with the function, I'll expect the function to resolve a value instead of returning a promise.
There is also some eslint to match async and await together. https://eslint.org/docs/rules/require-await#rule-details-260

I don't follow. That eslint rule just says "you may not have intended this", it doesn't actually explain that the practice is bad in any way (and like any eslint rule, likely an opposite rule exists).

async functions return a promise. await converts a resolved value. I can await promises, or I can use the promise directly, and I'll decide which based on what I'm doing.

swiftone · 2020-08-05T19:09:44Z

README.md

@@ -265,15 +265,15 @@ tokenManager: {
 }
 ```

-By default, the `tokenManager` will attempt to renew expired tokens. When an expired token is requested by the `tokenManager.get()` method, a renewal request is executed to update the token. If you wish to manually control token renewal, set `autoRenew` to false to disable this feature. You can listen to  [`expired`](#tokenmanageronevent-callback-context) events to know when the token has expired.
+By default, the `tokenManager` will attempt to renew tokens before they expire. If you wish to manually control token renewal, set `autoRenew` to false to disable this feature. You can listen to [`expired`](#tokenmanageronevent-callback-context) events to know when the token has expired.


This contradicts the CHANGELOG (unless you read real close)

Can you clarify where the confusion/contradiction is?

This says tokenManager will attempt to renew tokens. The CHANGELOG says that tokenManager.get() no longer does, but doesn't mention that the renewal ability still exists.

OK, I will update CHANGELOG to re-terate that renew is being done by tokenManager (outside the get()) method.

swiftone · 2020-08-05T20:00:56Z

CHANGELOG.md

+- [#444](https://github.com/okta/okta-auth-js/pull/444)
+  - Implements "active" autoRenew. If autoRenew is true, tokens will be renewed before expiration (without calling `tokenManager.get`). If autoRenew is false, tokens will be removed from storage on expiration.
+  - `onSessionExpired` option has been removed
+  - `tokenManager.get` no longer implements autoRenew functionality. It is possible that the token returned from the TokenManager may be expired. New method `tokenManager.hasExpired` can be used to test the token.

 ## PENDING


Just noting that the PENDING part will have to be fixed before we release. I assume you're on that, but I wanted a comment so it doesn't get overlooked.

shuowu · 2020-08-05T20:50:26Z

lib/TokenManager.ts

-      ));
-    }
-
+  return new Promise(function(resolve) {


Do we still need to keep this method async? Looks like all async logic has been removed.

We have a long-standing feature request to provide async option for custom storage provider: https://oktainc.atlassian.net/browse/OKTA-269814 The use case here is if you store tokens in a database. If we intend to suppor this, it is a good idea to keep get/set as async.

shuowu-okta · 2020-08-05T23:24:34Z

lib/TokenManager.ts

+        this.remove(key);
+      }
+    };
+    this.on('expired', onTokenExpiredHandler);

    setExpireEventTimeoutAll(sdk, tokenMgmtRef, storage);


App might be closed when tokens are still valid storage, then have expired tokens sit in storage on the next startup. Feels like we need a logic to try to renew tokens during app startup.

^ great catch

This logic does run on startup. It reads existing tokens and sets timeouts for expiration. If a token is already expired, that timeout would be 0 (still async). The renew operation itself is obviously async. So it's not possible to have the tokens validated and renewed synchronously at app startup. The app can/should wait for this case though. This is why tokenManager.hasExpired is exposed. The isAuthenticated method should do something like:

let accessToken = await tokenManager.get('accessToken'); if (tokenManager.hasExpired(accessToken) && config.autoRenew) { accessToken = await tokenManager.renew('accessToken'); }

The renew operation itself will handle multiple concurrent requests and ensure only a single renew operation is executed.

This is why okra-react has isPending - that covers the time between startup and decision about state. Since we're looking at shifting the state logic to auth-js in 5.0, it might make sense NOT to worry about this status here in 4.0, but I definitely advocate for keeping that idea when we do get there.

Internally isPending is set while updateAuthState is waiting on the result of isAuthenticated. This logic of renewing tokens when asking isAuthenticated was happening before implicitly on tokenManager.get(), but it must be done explicitly now. The
"active" autoRenew feature will work in the background to keep tokens refreshed but we cannot rely on it being completed before the first call to isAuthenticated. In 5.0 we will handle all this in okta-auth-js, borrowing the implementation from React, but for the 4.0 release, each downstream SDK must handle this.

implements active autoRenew

* bump version to 4.0.0 * ES6 Refactor (#388) * move module from packages to repo root * rename browser/server index * update eslint, use ecmaVersion 2020 * feat: add typescript declarations (#413) * feat: support typescript, use named exports * removes "onSessionExpired" option (#444) implements active autoRenew * Builds to "build" folder, CDN content in "build/dist" * Fixes compatibility with CDN / static app

aarongranick-okta marked this pull request as ready for review August 5, 2020 17:13

aarongranick-okta requested review from shuowu and swiftone August 5, 2020 17:23

swiftone reviewed Aug 5, 2020

View reviewed changes

shuowu reviewed Aug 5, 2020

View reviewed changes

shuowu-okta reviewed Aug 5, 2020

View reviewed changes

shuowu approved these changes Aug 10, 2020

View reviewed changes

aarongranick-okta force-pushed the dev-4.0 branch 2 times, most recently from a020c64 to 68ae3a3 Compare August 11, 2020 23:48

removes "onSessionExpired" option

35a50e6

implements active autoRenew

aarongranick-okta force-pushed the ag-breaking-4.0-OKTA-319173 branch from 4a2d8c6 to 35a50e6 Compare August 11, 2020 23:50

aarongranick-okta merged commit 1a9abf5 into dev-4.0 Aug 12, 2020

aarongranick-okta deleted the ag-breaking-4.0-OKTA-319173 branch August 12, 2020 01:05

aarongranick-okta added a commit that referenced this pull request Aug 12, 2020

removes "onSessionExpired" option (#444)

4a81e55

implements active autoRenew

aarongranick-okta added a commit that referenced this pull request Aug 12, 2020

removes "onSessionExpired" option (#444)

5709990

implements active autoRenew

aarongranick-okta mentioned this pull request Apr 5, 2021

PKCE Code Verification Failure on Authentication okta/okta-oidc-js#804

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

breaking changes for 4.0 #444

breaking changes for 4.0 #444

aarongranick-okta commented Aug 1, 2020 •

edited

Loading

swiftone left a comment

swiftone Aug 5, 2020

aarongranick-okta Aug 5, 2020

shuowu Aug 5, 2020

swiftone Aug 5, 2020

shuowu Aug 5, 2020

swiftone Aug 5, 2020

swiftone Aug 5, 2020 •

edited

Loading

aarongranick-okta Aug 5, 2020

swiftone Aug 5, 2020

aarongranick-okta Aug 10, 2020

swiftone Aug 5, 2020

shuowu Aug 5, 2020

aarongranick-okta Aug 10, 2020

shuowu-okta Aug 5, 2020

swiftone Aug 6, 2020

aarongranick-okta Aug 10, 2020 •

edited

Loading

swiftone Aug 10, 2020

aarongranick-okta Aug 10, 2020 •

edited

Loading

		@@ -116,24 +119,9 @@ function get(storage, key) {
		}

		function getAsync(sdk, tokenMgmtRef, storage, key) {

breaking changes for 4.0 #444

breaking changes for 4.0 #444

Conversation

aarongranick-okta commented Aug 1, 2020 • edited Loading

breaking changes

new features

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

swiftone Aug 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarongranick-okta Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarongranick-okta Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

aarongranick-okta commented Aug 1, 2020 •

edited

Loading

swiftone Aug 5, 2020 •

edited

Loading

aarongranick-okta Aug 10, 2020 •

edited

Loading

aarongranick-okta Aug 10, 2020 •

edited

Loading