Support for refresh_in for ConfidentialClient #542
Conversation
| @@ -94,6 +94,23 @@ func GetAccessTokenBody(accessToken, idToken, refreshToken, clientInfo string, e | |||
| return []byte(body) | |||
| } | |||
|
|
|||
| func GetAccessTokenBodyWithRefreshIn(accessToken, idToken, refreshToken, clientInfo string, expiresIn int, refreshIn int) []byte { | |||
There was a problem hiding this comment.
That's quite a lot of dup code. Why don't you add a parameter to the existing method instead? or have one call the other.
| desc: "Success", | ||
| payload: ` | ||
| { | ||
| "access_token": "secret", |
There was a problem hiding this comment.
Did you want to add refresh_in here?
| return err | ||
| } | ||
|
|
||
| // If refresh_in is not set, compute it as (now - expires_on) / 2 if expires_on > 2 hours from now |
There was a problem hiding this comment.
This is wrong. Imagine:
- I get a token with expires_on = 4h from AAD. It's 1 PM.
- I get later need that token, I get it from cache. It's 2 PM (so 3h expiration remaining). I now get refresh_in = 1.5h
- I need the token later too. It's 4 PM (so 1h expiration remaining). I now get refresh_in = nil.
What should actually happen is:
- at step 1, save the computed refresh_in value in the cache.
- at step 2 and step 3, evaluate if a new token from AAD is needed.
There was a problem hiding this comment.
This only takes care (partially) of adding RefreshOn in the cache. However, the feature is also meant to provide proactive token refresh capabilities, meaning:
- when app dev requests a token
- if RefreshOn exists AND it has elasped AND expiresON has not elapsed
- make a request to AAD for a new token
- and if that request fails AAD is unavailable (HTTP 5xx error or HTTP 429 error), return the old token
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
| defer cancel() | ||
| if tr, er := b.Token.Credential(ctx, authParams, silent.Credential); er == nil { | ||
| return b.AuthResultFromToken(ctx, authParams, tr, true) |
There was a problem hiding this comment.
nit: Is there any code path where AuthResultFromToken is called with the "cacheWrite" flag set to false? If not, consider getting rid of that param.
| @@ -31,4 +31,6 @@ type TokenProviderResult struct { | |||
| AccessToken string | |||
| // ExpiresInSeconds is the lifetime of the token in seconds | |||
| ExpiresInSeconds int | |||
| // RefreshInSeconds indicates the suggested time to refresh the token, if any | |||
| RefreshInSeconds int | |||
There was a problem hiding this comment.
nit: this extensiblity point is really for Azure SDK only, so that they can plug in their MSI logic and benefit from MSAL caching. We don't have to add this here, but it's ok either way.
There was a problem hiding this comment.
But if you do add it, we need tests for it. And you need to plug it in.
Shifted RefreshOn in Metadata
| @@ -458,6 +481,15 @@ func (b Client) AuthResultFromToken(ctx context.Context, authParams authority.Au | |||
| return ar, err | |||
| } | |||
|
|
|||
| // This function wraps time.Now() and is used for refreshing the application | |||
| // was created to test the function against refreshin | |||
| var GetCurrentTime = time.Now | |||
There was a problem hiding this comment.
- Making sure this isn't part of the public API?
- Does it make sense to keep it higher up?
|
PR Description:
This update introduces support for a
refreshInfield, which is calculated as half of theexpiresInvalue, but only ifexpiresInis greater than 2 hours and if it was not received in the response.Key changes:
expiresInis more than 2 hours and not provided in the response,refreshInwill be set to half ofexpiresIn.refreshInvalue. If it has expired, a server call will be triggered on the main thread to refresh the data.This ensures that cache data is refreshed proactively, reducing the need for repeated server calls while improving overall performance.