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
|
|
…verse domain (#1528) for context: b/340602527 Changes in this pr: - Override `getUniverseDomain()` to grab source credentials’s universe domain (UD) by default. Always use source credentials UD, not explicit provided UD. (In current design, impersonated credentials may not have universe domain in the outer layer. relay on UD from source credential. This may change in future) - Fix `isDefaultUniverseDomain()` in `GoogleCredentials` to account for `getUniverseDomain()` overrides in child classes. - In refreshAccessToken(), use endpoint url pattern to account for TPC case. - note that I choose to bypass this refreshIfExpired step because it wrongly steps into code path meant only for OAuth2 token request (GDU flow). Filed #1534 to address this separately. But for GDU flow here, this refresh step is redundant because the SSJ will get re-generated at [initialize request](https://github.com/googleapis/google-auth-library-java/blob/a987ecd06fd25a0048cdb3da6d1df4d029d85d79/oauth2_http/java/com/google/auth/oauth2/ImpersonatedCredentials.java#L558). Also skip this step for SA GDU with SSJ flow. - Throw IllegalStateException if UD is explicitly set (with parent class setter) and not matching source credential's UD - Fix toBuilder() to invoke super, and fix related issue with createScoped. (see #1489, #1428); Also fix equals() to compare super first. Not in this pr: - idtoken and signBlob endpoint changes are out-of-scope for this pr, will raise separate pr for it. sa-to-sa impersonation is successfully E2E tested for TPC usage according to [go/prptst-testing-service-account-impersonation](http://goto.google.com/prptst-testing-service-account-impersonation). --------- Co-authored-by: Blake Li <blakeli@google.com>
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())