[Identity] [core-http] Concept PR for the token refresher update #10085

sadasant · 2020-07-15T22:12:58Z

Jonathan will take over this today, but in any case, here's my concept PR to:

Tests pass, at least 🌞

Also fixes #9777 (since the feature crew decided to not make it configurable).

sophiajt · 2020-07-16T03:25:59Z

Ended up getting pulled onto house maintenance instead of coding today.

What you have looks good. cc @schaabs to double check we're going in the right direction. If it looks good, I can work with you to take it to the finish line.

sophiajt · 2020-08-02T23:48:24Z

sdk/core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts

@@ -63,6 +66,12 @@ export class BearerTokenAuthenticationPolicy extends BaseRequestPolicy {
    private tokenCache: AccessTokenCache
  ) {
    super(nextPolicy, options);
+    const requiredMillisecondsBeforeNewRefresh = 30000;


Commenting so we don't forget it - probably don't want to hardcode the delay like this. At the very least, we should pick the buffer, make it a const, and document it.

we should pick the buffer, make it a const, and document it

What do you mean by picking the buffer?

You're hardcoding 30000 here. What does it mean? Why 30000?

What do you think of this as a property of the BearerTokenAuthenticationPolicy class? Like:

export class BearerTokenAuthenticationPolicy extends BaseRequestPolicy { // The automated token refresh will only start to happen at the // expiration date minus the value of millisecondsBeforeNewRefresh, // which is by default 30 seconds. public millisecondsBeforeNewRefresh: number = 30000;

sophiajt · 2020-08-02T23:48:45Z

sdk/core/core-http/test/policies/bearerTokenAuthenticationPolicyTests.ts

-      [refreshCred1, 2],
-      [refreshCred2, 2],
+      [refreshCred1, 1],
+      [refreshCred2, 1],


What's the difference between 1 and 2 here?

number of refreshments have changed because of this PR, since we know use the existing tokens first, and in parallel attempt to request (?) Please let me know if I'm missing something

Ah okay, got it.

sdk/core/core-http/src/credentials/accessTokenRefresher.ts

schaabs

Refresh logic LGTM please get approval from a JS team member.

sdk/core/core-http/review/core-http.api.md

xirzec

Overall looks very good, but there are a few things I'd like to have fix before this gets merged.

sdk/core/core-http/src/credentials/accessTokenRefresher.ts

xirzec · 2020-08-04T23:16:13Z

sdk/core/core-http/src/credentials/accessTokenRefresher.ts

+   * Returns null if the required time between each call hasn't been reached.
+   * @param options getToken options
+   */
+  public refresh(options: GetTokenOptions): Promise<AccessToken | undefined> | null {


I don't think we should return Promise | null - that's a confusing and hard to use contract with async/await. In the null case we should return Promise.resolve() or a cached version of that.

Here's a much better approach with some small tweaks! How come I didn't think of this earlier? 😓 well, it's here now! 😁 16c88f5

xirzec · 2020-08-04T23:16:24Z

sdk/core/core-http/src/credentials/accessTokenRefresher.ts

+   * Forces the requests of a new token if we're not currently waiting for a new token.
+   * @param options getToken options
+   */
+  public forcedRefresh(options: GetTokenOptions): Promise<AccessToken | undefined> | null {


I think the return type is wrong here, since how can it be null? If the promise doesn't exist it gets created.

Here's a much better approach with some small tweaks! How come I didn't think of this earlier? 😓 well, it's here now! 😁 16c88f5

xirzec · 2020-08-04T23:17:30Z

sdk/core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts

+   * expiration date minus the value of timeBetweenRefreshAttemptsInMs,
+   * which is by default 30 seconds.
+   */
+  private timeBetweenRefreshAttemptsInMs: number = 30000;


nit: no need to annotate the type as number

also why make this a class member? It seems to be constant, in which case it could be pulled out into a module const

I moved it to a module const here: 16c88f5

xirzec · 2020-08-04T23:18:48Z

sdk/core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts

+      // only if the cache is unable to retrieve the access token,
+      // which means that it has expired, or it has never been set.
+      const refreshPromise = this.tokenRefresher.forcedRefresh(options);
+      if (refreshPromise !== null) {


yeah having to do this check is ugly

Here's a much better approach with some small tweaks! How come I didn't think of this earlier? 😓 well, it's here now! 😁 16c88f5

sdk/core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts

xirzec · 2020-08-04T23:19:18Z

sdk/core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts

+      // If the tokenRefresher returned null, some other refresh is happening already.
+      // if this is a new refresh, we set it up to update the cachedToken once it finishes.
+      if (refreshPromise !== null) {
+        refreshPromise.then((accessToken: AccessToken | undefined) => {


why does this fork the promise chain instead of using await?

the idea is to not wait for this to happen, that's the main point of this feature: this should happen in the background.

xirzec · 2020-08-04T23:20:07Z

sdk/core/core-http/test/policies/bearerTokenAuthenticationPolicyTests.ts

@@ -73,6 +74,19 @@ describe("BearerTokenAuthenticationPolicy", function() {
    }
  });

+  it("tests that AccessTokenRefresher is working", async function() {
+    this.timeout(35000);


no, please don't do this. There is no reason to make a test take 30+ seconds just because of a default timer. We should mock the clock with sinon if you want to test that, but not sit there and waste CPU cycles

alright, I'll follow your advice here

I believe I did it here: 7f3192e thank you again for the feedback!

daviwil

I think this doesn't really fit in with what we're already doing for token refreshing. If the idea is that you want to make sure that there's only a single refresh happening at any given point based on a pre-expiration delta, you could do it using a single Promise field inside of the existing BearerTokenAuthenticationPolicy class. We already have logic for calculating the pre-expiration delta so I don't think we need to add another way to calculate that.

sdk/core/core-http/src/credentials/accessTokenRefresher.ts

daviwil · 2020-08-04T23:13:36Z

sdk/core/core-http/test/policies/bearerTokenAuthenticationPolicyTests.ts

+    const request = createRequest();
+    const policy = createBearerTokenPolicy("testscope", credentialToTest);
+    await policy.sendRequest(request);
+    await delay(30000);


You should stub this out like you did in the Identity tests so that we don't have to wait 30 seconds on a test run

thank you! I'll do what you say

I believe I did it here: 7f3192e thank you again for the feedback!

daviwil · 2020-08-04T23:15:24Z

sdk/core/core-http/src/credentials/accessTokenRefresher.ts

+   * Forces the requests of a new token if we're not currently waiting for a new token.
+   * @param options getToken options
+   */
+  public forcedRefresh(options: GetTokenOptions): Promise<AccessToken | undefined> | null {


Could this just be an optional parameter on refresh? Ideally it would be, otherwise I'd call this method forceRefresh

I thought on intentionally not doing the forced parameter since it becomes sort of meaningless once passed in, like refresh(true). I wonder if extending GetTokenOptions to an interface like RefreshOptions with an extra property would be better. What do you think?

Here's a much better approach with some small tweaks! How come I didn't think of this earlier? 😓 well, it's here now! 😁 16c88f5

daviwil · 2020-08-04T23:23:08Z

sdk/core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts

+    } else {
+      // If we still have a cached access token,
+      // then attempt to refresh without waiting.
+      const refreshPromise = this.tokenRefresher.refresh(options);


Why refresh if the token cache's refresh time hasn't been reached? We use a delta to determine the time before the real expiration time that we should start a refresh. The previous logic case (accessToken === undefined) should handle that, in theory.

This has changed, but the gist is that we will not attempt to refresh based on a conditional. We can make the conditional better too! if that makes sense.

daviwil · 2020-08-04T23:28:23Z

sdk/core/core-http/src/credentials/accessTokenRefresher.ts

+/**
+ * Helps the core-http token authentication policies with requesting a new token if we're not currently waiting for a new token.
+ */
+export class AccessTokenRefresher {


After looking over the code I wonder if we even need a separate class for this behavior. You could just do the promise management inside of BearerTokenAuthenticationPolicy itself.

I think that @schaabs could help me here. The idea is to be able to have something plug-able, to use somewhere else, as far as I understand.

I spoke with @schaabs on Teams, he clarified things for me.

jeremymeng · 2020-08-04T23:47:47Z

sdk/core/core-http/src/credentials/accessTokenRefresher.ts

+ */
+export class AccessTokenRefresher {
+  private promise: Promise<AccessToken | undefined> | undefined;
+  private lastCalled: number | undefined;


instead of undefined, a minimum number like 0 or beginning of the time can be used here instead?

I'll assign 0

ramya-rao-a · 2020-08-05T05:58:52Z

@sadasant, @jonathandturner,

We are a little late in the game to take in changes for the core packages in the current release cycle.
Code complete for core packages for current release cycle was last Friday and this week is when we release the core packages.

I would strongly recommend pushing this for the next release cycle.

sophiajt · 2020-08-05T19:06:49Z

@schaabs - is it possible to move this fix out of GA, or do we need to slip the GA for Identity/core-http to ensure it's in?

daviwil

Thanks for making the improvements! This looks good to me.

sdk/core/core-http/src/credentials/accessTokenRefresher.ts

sophiajt · 2020-08-05T21:45:02Z

Just got word back from @schaabs - this won't make the GA release. We'll put it in the first 1.2 prerelease next month instead.

sadasant · 2020-08-13T22:08:52Z

@xirzec a new review is appreciated!

xirzec

I like this a lot more! Left a few cleanup/style suggestions and thoughts.

sdk/core/core-http/src/credentials/accessTokenRefresher.ts

xirzec · 2020-08-13T22:46:23Z

sdk/core/core-http/src/credentials/accessTokenRefresher.ts

+  private lastCalled = 0;
+
+  constructor(
+    private credential: TokenCredential,


personal style nit: I find this way of initializing private variables to be confusing for some folks and always explicitly initialize them in the constructor body instead.

@bterlson you have any pref here?

sdk/core/core-http/src/credentials/accessTokenRefresher.ts

sdk/core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts

Co-authored-by: Jeff Fisher <xirzec@xirzec.com>

…set-token-cache

sadasant · 2020-08-18T23:27:40Z

sdk/core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts

-      this.tokenCache.setCachedToken(accessToken);
+      // Waiting for the next refresh only if the cache is unable to retrieve the access token,
+      // which means that it has expired, or it has never been set.
+      accessToken = await this.tokenRefresher.refresh(options);


Oops! I forgot to this.tokenCache.setCachedToken(accessToken); here!

Thank you, @HarshaNalluru for helping me see this! 🌞

Here's a PR: #10692

sadasant · 2020-08-19T00:29:13Z

sdk/core/core-http/test/policies/bearerTokenAuthenticationPolicyTests.ts

@@ -61,7 +62,7 @@ describe("BearerTokenAuthenticationPolicy", function() {
    const credentialsToTest: [MockRefreshAzureCredential, number][] = [
      [refreshCred1, 2],
      [refreshCred2, 2],
-      [notRefreshCred1, 1]
+      [notRefreshCred1, 2]


Well, this was actually important: https://github.com/Azure/azure-sdk-for-js/pull/10692/files#r472566282

sadasant requested a review from sophiajt July 15, 2020 22:12

sadasant self-assigned this Jul 15, 2020

ghost added the Azure.Core label Jul 15, 2020

sadasant mentioned this pull request Jul 28, 2020

[Identity] Make MSAL Token Refresh Configurable #9777

Closed

sophiajt reviewed Aug 2, 2020

View reviewed changes

sadasant force-pushed the identity/refresh-offset-token-cache branch from 97d5469 to f973ea5 Compare August 3, 2020 23:02

schaabs reviewed Aug 4, 2020

View reviewed changes

sdk/core/core-http/src/credentials/accessTokenRefresher.ts Show resolved Hide resolved

sadasant marked this pull request as ready for review August 4, 2020 22:40

sadasant requested review from bterlson, chradek, daviwil, jeremymeng, willmtemple and xirzec as code owners August 4, 2020 22:40

schaabs approved these changes Aug 4, 2020

View reviewed changes

schaabs reviewed Aug 4, 2020

View reviewed changes

sdk/core/core-http/review/core-http.api.md Outdated Show resolved Hide resolved

xirzec requested changes Aug 4, 2020

View reviewed changes

daviwil suggested changes Aug 4, 2020

View reviewed changes

jeremymeng reviewed Aug 4, 2020

View reviewed changes

daviwil approved these changes Aug 5, 2020

View reviewed changes

sdk/core/core-http/src/credentials/accessTokenRefresher.ts Show resolved Hide resolved

sdk/core/core-http/src/credentials/accessTokenRefresher.ts Outdated Show resolved Hide resolved

sadasant changed the title ~~[Identity] Concept PR for the token refresher update~~ [DO NOT MERGE] [Identity] Concept PR for the token refresher update Aug 5, 2020

sadasant changed the title ~~[DO NOT MERGE] [Identity] Concept PR for the token refresher update~~ [DO NOT MERGE] [Identity] [core-http] Concept PR for the token refresher update Aug 6, 2020

sadasant requested a review from xirzec August 6, 2020 17:16

sadasant changed the title ~~[DO NOT MERGE] [Identity] [core-http] Concept PR for the token refresher update~~ [Identity] [core-http] Concept PR for the token refresher update Aug 13, 2020

AccessTokenRefresher concept

83176ee

sadasant added 11 commits August 13, 2020 21:44

formatting

2f9d1b8

simple test

b6006d4

changed to timeBetweenRefreshAttemptsInMs

9a1a49c

forcedRefresh if no cached token

788e67a

making timeBetweenRefreshAttemptsInMs private

cd16794

undoing some of the previous test changes

6db62c0

much better approach after some tweaks

dd186db

license header and lastCalled default value to 0

dbc678e

mocking instead of delaying, for the tests

a7f7118

moved the undefined to another place, it looks better, and a fmt fix

ed73cb7

updates after fixing conflicts

b58608d

sadasant force-pushed the identity/refresh-offset-token-cache branch from de8f86b to b58608d Compare August 13, 2020 22:07

sadasant requested a review from daviwil August 13, 2020 22:08

xirzec approved these changes Aug 13, 2020

View reviewed changes

sadasant and others added 6 commits August 14, 2020 07:12

Apply suggestions from code review

8ea19e6

Co-authored-by: Jeff Fisher <xirzec@xirzec.com>

Merge remote-tracking branch 'Azure/master' into identity/refresh-off…

d5572fd

…set-token-cache

fixes after recent suggestions

851bee9

formatting

5672735

Merge remote-tracking branch 'Azure/master' into identity/refresh-off…

265eefa

…set-token-cache

requiredMillisecondsBeforeNewRefresh defaults to 30 seconds

3b27671

sadasant requested a review from jeremymeng August 14, 2020 20:22

daviwil approved these changes Aug 14, 2020

View reviewed changes

sadasant merged commit 03db02d into Azure:master Aug 14, 2020

sadasant deleted the identity/refresh-offset-token-cache branch August 14, 2020 21:40

sadasant mentioned this pull request Aug 18, 2020

[core-http] The previous core-http change had a bug #10692

Merged

sadasant commented Aug 18, 2020

View reviewed changes

sadasant mentioned this pull request Aug 19, 2020

[Core HTTP] Token refresher is not shared among multiple requests #10694

Closed

sadasant commented Aug 19, 2020

View reviewed changes

HarshaNalluru mentioned this pull request Aug 20, 2020

[Core HTTP] Bug Fix - TokenRefresher shouldn't be recreated for each request #10728

Merged

[Identity] [core-http] Concept PR for the token refresher update #10085

[Identity] [core-http] Concept PR for the token refresher update #10085

Conversation

sadasant commented Jul 15, 2020 • edited

sophiajt commented Jul 16, 2020

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

schaabs left a comment

Choose a reason for hiding this comment

xirzec 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviwil left a comment • edited

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

Choose a reason for hiding this comment

ramya-rao-a commented Aug 5, 2020

sophiajt commented Aug 5, 2020

daviwil left a comment

Choose a reason for hiding this comment

sophiajt commented Aug 5, 2020

sadasant commented Aug 13, 2020

xirzec 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

sadasant commented Jul 15, 2020 •

edited

daviwil left a comment •

edited