fix: ComputeEngineCredentials.createScoped should invalidate existing AccessToken #1428
Conversation
…tent exception with current mock transport.
oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java
Outdated
Show resolved
Hide resolved
| ComputeEngineCredentials.Builder builder = | ||
| ComputeEngineCredentials.newBuilder() | ||
| .setHttpTransportFactory(transportFactory) | ||
| .setScopes(newScopes) | ||
| .setDefaultScopes(newDefaultScopes); | ||
| (Builder) | ||
| this.toBuilder() | ||
| .setHttpTransportFactory(transportFactory) | ||
| .setScopes(newScopes) | ||
| .setDefaultScopes(newDefaultScopes) | ||
| .setAccessToken(null); |
There was a problem hiding this comment.
If a user passes in the same set of scopes (i.e. does not actually change the set of scopes for the credentials), this would invalidate the access token, right? Do you think it would make sense to check to see if the scopes changed and only invalidate the access token then? If that makes sense, perhaps that could be done in GoogleCredentials so that it could apply to every single type of Credentials?
There was a problem hiding this comment.
I am trying to narrow the scope of this pr only fix this regression in behavior introduced by 7e26861. To matches the original behavior.
That said, in a follow up PR, I am considering moving the logic upstream to GoogleCredentials or OAuth2Credentials. One thought is aside from createScoped(), does the same issue exists in creating copies and setting other properties differently, e.g. set a different universe. So perhaps going to invalidate directly in toBuilder(). We can discuss further when I put up that PR.
If a user passes in the same set of scopes (i.e. does not actually change the set of scopes for the credentials), this would invalidate the access token, right?
In this case, the old access token would probably work too, but it would be safer or at least no harm to invalidate it anyway and let the refresh workflow request the token from server again.
Do you think it would make sense to check to see if the scopes changed and only invalidate the access token then? If that makes sense, perhaps that could be done in GoogleCredentials so that it could apply to every single type of Credentials?
Hmm, let's discuss more in the follow-up pr, this will be a new check, and I'd rather introduce it in GoogleCredentials than individual credential classes if possible.
There was a problem hiding this comment.
Sounds good. We'll keep this scope for just fixing the bug and address anything else in a follow up PR.
In this case, the old access token would probably work too, but it would be safer or at least no harm to invalidate it anyway and let the refresh workflow request the token from server again.
Yep makes sense. I'm just wondering what the expected behavior is from the user POV is and wondering what the behavior is from other language auth libraries. IMO, I think it makes sense to always invalidate the access token when createScoped() is called even if the access token is technically valid. Let's aim to keep it consistent between languages if possible.
oauth2_http/javatests/com/google/auth/oauth2/ComputeEngineCredentialsTest.java
Show resolved
Hide resolved
oauth2_http/javatests/com/google/auth/oauth2/MockMetadataServerTransport.java
Show resolved
Hide resolved
| if (url.contains("?scopes=")) { | ||
| refreshContents.put("access_token", "fake access token with scope"); |
There was a problem hiding this comment.
If possible, could the fake access token not be hard coded in the MockMetaDataServerTransport code?
Perhaps we could make the logic something like:
- MockMetaDataServerTransport takes a map of scopes -> access token pairings/ MockMetaDataServerTransport.addAccessTokenScope(scope, accessToken) will add the the pairing
- If url.contains("?scopes="), check the map for scopes and returns the configured access token
- Test passes the pairing of (scope, accesstoken) to MockMetaDataServerTransport
oauth2_http/javatests/com/google/auth/oauth2/ComputeEngineCredentialsTest.java
Outdated
Show resolved
Hide resolved
| if (scopesToAccessToken == null) { | ||
| scopesToAccessToken = new HashMap<>(); | ||
| } |
There was a problem hiding this comment.
nit: initialize in the constructor
lqiu96
left a comment
There was a problem hiding this comment.
LGTM, but one last review from Wes
|
|
Fixes #1387 ☕️
As described in the original issue, this looks like a regression introduced in 7e26861 when migrating from deprecated constructor to use builder.
Access token is scoped and should be invalidated when scope changes.
This PR include changes:
Other credential types: e.g. ServiceAccountCredentials.creatScoped() should also invalidate existing AccessTokenWill raise separate pr for this.Follow up items not included in this PR:
ServiceAccountCredentials.creatScoped())