Fix silent auth scopes and refresh behavior #327
Conversation
fixed wrong authParams passed to Refresh function
chlowell
requested review from
rayluo,
jhendrixMSFT,
bgavrilMS,
siddhijain and
keegan-caruso
| } | ||
|
|
||
| var cc *accesstokens.Credential | ||
| if silent.RequestType == accesstokens.ATConfidential { | ||
| cc = silent.Credential | ||
| } | ||
|
|
||
| token, err := b.Token.Refresh(ctx, silent.RequestType, b.AuthParams, cc, storageTokenResponse.RefreshToken) |
There was a problem hiding this comment.
This is problem 1. b.AuthParams holds common parameters for all auth requests. Its type has a scopes field but on this instance that field always has a zero value (default scopes are handled elsewhere). authParams is an instance of the same type which carries overriding values for this auth request, and the scopes requested by the user application.
| @@ -94,10 +94,7 @@ func (m *Manager) Read(ctx context.Context, authParameters authority.AuthParams, | |||
| return TokenResponse{}, err | |||
| } | |||
|
|
|||
| accessToken, err := m.readAccessToken(homeAccountID, metadata.Aliases, realm, clientID, scopes) | |||
There was a problem hiding this comment.
This is problem 2. This helper returns an error when it misses. A miss isn't fatal and using an error to signal one requires the caller to inspect the error to determine whether it's safe to proceed. I could've used a sentinel error for misses but that seemed excessive because the helper really doesn't need to return an error--it only searches a map.
To clarify some aspects for issue 1.
|
|
Thanks for the additional context. I added a test to verify granted scopes are handled correctly in caching. Clients appear to add "openid profile offline_access" scopes as you describe--at least, I see this in live traffic and the code looks right--but that doesn't have unit test coverage today (#329). |
|
Kudos, SonarCloud Quality Gate passed!
|
| // Validate validates that this AccessToken can be used. | ||
| func (a AccessToken) Validate() error { | ||
| if FakeValidate != nil { |
There was a problem hiding this comment.
It looks like a bad idea. It should be possible to just pass a valid token in the test.
There was a problem hiding this comment.
To give some context, the cache calls this method before writing a token and won't write that token if this method returns an error. This method will return an error if the token expires within the next 5 minutes. A test that requires the cache to contain an expired token therefore can't pass in a valid token unless it then waits for that token to expire. Perhaps the cache shouldn't check expiry before writing tokens (too big a change to contemplate here) but there are of course other ways to work around this. The test could cache a token that expires in 5 minutes and 1 millisecond then time.Sleep(2*time.Millisecond). I like that approach, but I still prefer FakeValidate because it may prove useful in other test cases and doesn't require the one I'm adding in this PR to take a dependency on timing or behavior not under test.
Edit: for anyone not familiar with Go, let me add that FakeValidate isn't part of the public API because it's in an internal package.
Fix silent auth scopes and refresh behavior (#327)
This fixes two problems with silent authentication:
Fixing the first problem is straightforward: just pass the right argument. The second problem is more complex. It arises in the cache's
Readmethod, which assumes the caller requires an access token matching the given parameters and returns an error when it doesn't find one. If the caller could in principle use a previously cached refresh token to authenticate, it never gets the chance becauseReaddoesn't return that refresh token. I fixed this by havingReadcontinue past an access token cache miss to fetch more data, such as a refresh token, before returning. This meansReadnow returns a zero-valued access token in some cases. This is okay becausebase.Clientis the only caller and already validates access tokens before returning them to the application.