Support for refresh_in for ConfidentialClient

[4gust]

PR Description:

This update introduces support for a refreshIn field, which is calculated as half of the expiresIn value, but only if expiresIn is greater than 2 hours and if it was not received in the response.

Key changes:

• When storing data: If expiresIn is more than 2 hours and not provided in the response, refreshIn will be set to half of expiresIn.
• When retrieving from cache: We now check the refreshIn value. 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.

[bgavrilMS]

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

[AndyOHart]

LGTM

[AndyOHart]

Not sure if we need this comment

[chlowell]

can you set this field unconditionally i.e., what happens when pr.RefreshInSeconds == 0? Do you not just get a zero Time?

[chlowell]

I'd inline this. All else being equal, it's best to have a critical section in a single function because it's easier to understand and maintain that way

[chlowell]

is refresh_in always a number, or could it be a string?

[4gust]

Always a number, same as expires_in

[chlowell]

expires_in can be a string though. I'm referring to JSON value types here i.e., is refresh_in always a number like 42 or can it be a string like "42"

[4gust]

Updated, tested it and it works fine both ways :)

[chlowell]

Why do this in a separate goroutine, and with a done channel? You want the goroutine executing the test to wait for the others--it can just call wg.Wait() as before. Maybe this will help explain the mechanics: https://play.golang.com/p/hWAE3Cenyd8

[4gust]

I created the go routine, so that we can safely close the channels and create a wait()
I understand what i missed in there
I updated it

[chlowell]

👀

[chlowell]

this has no effect--the next line will never execute--because mockClient has only one response and would panic upon receiving a second request

[4gust]

This checks two things

1. Have it token updated.
2. The updated token we got was not from cache.

In these scenarios should only happen once. If we don't check this then how do, we know that the request to the mockClient has been made or not. As, this is concurrent we can get the updated token from cache before the token from Identity. This check is useful.
The request can only be made once, as there is only one request in queue.

[chlowell]

🤔 this test is unclear and maybe incorrect. How to clarify or fix it depends on what behaviors you want to verify. I suppose it's a subset of these?

• proactive refreshes in general don't race
• client refreshes different tenants concurrently
• concurrent refreshes in particular don't race

[sonarqubecloud]

Quality Gate passed

Issues
7 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[chlowell]

You want to observe the first and second goroutines refreshing concurrently. That means observing them sending token requests via AcquireTokenSilent at the same time. As written, this test can't do that because it synchronizes the goroutines after they call AcquireTokenSilent, making this possible:

1. first completes its refresh
2. first waits for second
3. second completes its refresh
4. second signals first to continue
5. test passes

You need to synchronize during the refresh requests, while the goroutines are executing AcquireTokenSilent. You can do that with the response callback (adjust the mock client so it doesn't lock around this callback) and a channel the goroutines use to announce when they're refreshing, like this:

// goroutines use present to announce they've reached the (fake) server and are waiting to proceed
present := make(chan string)
// the test goroutine uses this channel to signal the goroutines to complete their refreshes
proceed := make(chan struct{})
wg := sync.WaitGroup{}
// need a timeout because a bug could cause deadlock
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
defer cancel()
for _, tenant := range []string{"A", "B"} {
    wg.Add(1)
    mockClient.AppendResponse(
        mock.WithBody(mock.GetAccessTokenBody(tenant, "", "", "", expiresIn, refreshIn)),
        mock.WithCallback(func(r *http.Request) {
            if s := strings.SplitN(r.URL.Path, "/", 3); len(s) == 3 {
                // send the tenant from the URL, which uniquely identifies the goroutine
                present <- s[1]
                for range proceed {
                    // wait for the test goroutine to close this channel
                }
            } else {
                t.Error("unexpected token request URL: " + r.URL.String())
            }
        }),
    )
    go func(id string) {
        defer wg.Done()
        if _, err := client.AcquireTokenSilent(ctx, tokenScope, WithTenantID(id)); err != nil {
            t.Error("Unexpected error", err)
        }
    }(tenant)
}
for a, b := false, false; !(a && b); {
    select {
    case <-ctx.Done():
        t.Fatal("timed out waiting for both goroutines to refresh")
    case g := <-present:
        switch g {
        case "A":
            a = true
        case "B":
            b = true
        default:
            t.Fatal("unexpected goroutine: " + g)
        }
    }
}
// both goroutines are refreshing concurrently. Signal
// them to proceed and wait for them to finish
close(proceed)
wg.Wait()