fix: Tune auth token refresh and valid windows. #2630

amanda-tarafa · 2023-12-07T05:18:42Z

And some code cleanup and refactor.

@jskeet as always, one commit at a time.

jskeet

Most of this is absolutely fine - just a few questions that affect the public API. Probably worth chatting about in a meeting rather than back-and-forth here.

jskeet · 2023-12-07T08:02:24Z

Src/Support/Google.Apis.Auth.Tests/OAuth2/Responses/TokenResponseTests.cs

-                // it's expired now.
-                long isHardExpiredSeconds = 60 * 15;
+                // it's not valid and may not be used.
+                long mayNotBeUsesSeconds = 60 * 15;


Uses => Used

jskeet · 2023-12-07T08:04:15Z

Src/Support/Google.Apis.Auth/OAuth2/Responses/TokenResponse.cs

+
+        /// <summary>
+        /// Returns true if the token represented by this token response is valid, that is, it may be used
+        /// for authentication and authorizations purposes.


authorizations => authorization

jskeet · 2023-12-07T08:06:34Z

Src/Support/Google.Apis.Auth/OAuth2/Responses/TokenResponse.cs

+        /// <list type="bullet">
+        /// <item>At least one of <see cref="AccessToken"/> and <see cref="IdToken"/> is not null.</item>
+        /// <item><see cref="ExpiresInSeconds"/> is not null.</item>
+        /// <item>The token has not expired and will not expire in the very near future. That is if


Hmm. This is where I have a problem with "valid" as the term. If a token hasn't expired, I would say it's valid. If I send it to a service and there isn't clock skew involved, it's still going to succeed.

Do we need to make this public immediately? Can we make it internal for now (or even remove it if we're using a clock elsewhere) and then make it public when we've got a better name?

I'm removing this property entirely, we don't use it and we didn't have anything like this before.

jskeet · 2023-12-07T08:10:20Z

Src/Support/Google.Apis.Auth/OAuth2/Responses/TokenResponse.cs

+        internal bool MayBeUsed(IClock clock) =>
+            (AccessToken is not null || IdToken is not null) &&
+            ExpiresInSeconds.HasValue &&
+            IssuedUtc.AddSeconds(ExpiresInSeconds.Value - TokenInvalidWindowSeconds) > clock.UtcNow;


Just as a potential future refactor, I wonder whether we should have an EffectiveExpiryTimeUtc which is IssuedUtc.AddSeconds(ExpiresInSeconds.Value - TokenInvalidWindowSeconds) (and make it nullable) so we could make this:

(AccessToken is not null || IdToken is not null) && EffectiveExpiryUtc is DateTime effectiveExpiry && clock.UtcNow < effectiveExpiry

Happy to bikeshed the "effective expiry" name - maybe "DoNotUseAfter" or something :)

Yep, and I'm doing the same for the ShouldRefresh one. I'm adding: RefreshWindowStartUtc and ExpiryWindowStartUtc. Does that sound good?

jskeet · 2023-12-07T08:11:12Z

Src/Support/Google.Apis.Auth/OAuth2/Web/AuthorizationCodeWebApp.cs

@@ -135,7 +135,7 @@ public bool ShouldRequestAuthorizationCode(TokenResponse token)
            // If the flow includes a parameter that requires a new token, if the stored token is null or it doesn't
            // have a refresh token and the access token is expired we need to retrieve a new authorization code.
            return Flow.ShouldForceTokenRetrieval() || token == null || (token.RefreshToken == null
-                && token.IsExpired(flow.Clock));
+                && token.ShouldBeRefreshed(flow.Clock));


This is a semantic change, right? (Admittedly one that would only be hit very rarely.)

We've talked about this offline, but just to close the loop: this is just change in the method name that was not clear before. The semantics are exactly the same as before, it's just the new method name that matches those semantics better.

amanda-tarafa

@jskeet I've addressed your comments and also increased the "invalid" window to 1 minute before expiry (from 30 seconds). Changes are in ordered commits named "Address comments..."

amanda-tarafa · 2023-12-12T07:34:55Z

Src/Support/Google.Apis.Auth.Tests/OAuth2/Responses/TokenResponseTests.cs

-                // it's expired now.
-                long isHardExpiredSeconds = 60 * 15;
+                // it's not valid and may not be used.
+                long mayNotBeUsesSeconds = 60 * 15;


amanda-tarafa · 2023-12-12T07:35:55Z

Src/Support/Google.Apis.Auth/OAuth2/Responses/TokenResponse.cs

+
+        /// <summary>
+        /// Returns true if the token represented by this token response is valid, that is, it may be used
+        /// for authentication and authorizations purposes.


amanda-tarafa · 2023-12-12T07:36:41Z

Src/Support/Google.Apis.Auth/OAuth2/Responses/TokenResponse.cs

+        /// <list type="bullet">
+        /// <item>At least one of <see cref="AccessToken"/> and <see cref="IdToken"/> is not null.</item>
+        /// <item><see cref="ExpiresInSeconds"/> is not null.</item>
+        /// <item>The token has not expired and will not expire in the very near future. That is if


I'm removing this property entirely, we don't use it and we didn't have anything like this before.

amanda-tarafa · 2023-12-12T07:41:09Z

Src/Support/Google.Apis.Auth/OAuth2/Web/AuthorizationCodeWebApp.cs

@@ -135,7 +135,7 @@ public bool ShouldRequestAuthorizationCode(TokenResponse token)
            // If the flow includes a parameter that requires a new token, if the stored token is null or it doesn't
            // have a refresh token and the access token is expired we need to retrieve a new authorization code.
            return Flow.ShouldForceTokenRetrieval() || token == null || (token.RefreshToken == null
-                && token.IsExpired(flow.Clock));
+                && token.ShouldBeRefreshed(flow.Clock));


We've talked about this offline, but just to close the loop: this is just change in the method name that was not clear before. The semantics are exactly the same as before, it's just the new method name that matches those semantics better.

amanda-tarafa · 2023-12-12T07:45:26Z

Src/Support/Google.Apis.Auth/OAuth2/Responses/TokenResponse.cs

+        internal bool MayBeUsed(IClock clock) =>
+            (AccessToken is not null || IdToken is not null) &&
+            ExpiresInSeconds.HasValue &&
+            IssuedUtc.AddSeconds(ExpiresInSeconds.Value - TokenInvalidWindowSeconds) > clock.UtcNow;


Yep, and I'm doing the same for the ShouldRefresh one. I'm adding: RefreshWindowStartUtc and ExpiryWindowStartUtc. Does that sound good?

jskeet

Great, thanks!

…ay be refreshed.

This is to account for Metadata Servers caching the tokens.

We are using a shared extension method instead.

amanda-tarafa · 2023-12-12T17:09:25Z

Thanks! I've squashed the review commits and will merge on green.

Fixes: - #2613 Default JSON serializer is not affected by gloabl JSON settings. - #2630 Tune authorization token staleness and validity windows so that it doesn't overlap with Metadata Server authorization token caching. Features: - #2616 Makes batch response handling more efficient, particularly benefiting large responses.

amanda-tarafa requested a review from jskeet December 7, 2023 05:18

amanda-tarafa requested a review from a team as a code owner December 7, 2023 05:18

jskeet reviewed Dec 7, 2023

View reviewed changes

chore: Simplify JSON attributes usage.

5aa2705

amanda-tarafa force-pushed the refresh_window branch from 9d0af17 to b7df1b7 Compare December 12, 2023 08:22

amanda-tarafa commented Dec 12, 2023

View reviewed changes

jskeet approved these changes Dec 12, 2023

View reviewed changes

amanda-tarafa added 3 commits December 12, 2023 09:07

refactor: Makes it easier to know when a token is valid and when it m…

3a5884a

…ay be refreshed.

fix: Tune auth token refresh and valid windows.

f7f8450

This is to account for Metadata Servers caching the tokens.

chore: Remove unused method.

09e2199

We are using a shared extension method instead.

amanda-tarafa force-pushed the refresh_window branch from b7df1b7 to 09e2199 Compare December 12, 2023 17:08

amanda-tarafa merged commit 01f70c7 into main Dec 12, 2023

amanda-tarafa deleted the refresh_window branch December 12, 2023 17:13

amanda-tarafa mentioned this pull request Jan 9, 2024

Update version: 1.64.0 -> 1.65.0 #2641

Merged

fix: Tune auth token refresh and valid windows. #2630

fix: Tune auth token refresh and valid windows. #2630

Uh oh!

Conversation

amanda-tarafa commented Dec 7, 2023

Uh oh!

jskeet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanda-tarafa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jskeet left a comment

Choose a reason for hiding this comment

Uh oh!

amanda-tarafa commented Dec 12, 2023

Uh oh!

Uh oh!