Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ComputeEngineCredentials.createScoped should invalidate existing AccessToken #1428

Merged
merged 18 commits into from
Aug 29, 2024
Merged

fix: ComputeEngineCredentials.createScoped should invalidate existing AccessToken #1428

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
1ce3436
fix: invalidate accesstoken when create scoped for compute engine cre…
zhumin8 Jun 20, 2024
399135c
fix: invalidate access token for service account credentials when cre…
zhumin8 Jun 20, 2024
b7448c6
fix: createScoped with default scopes should also respect universeDom…
zhumin8 Jul 3, 2024
0b1f546
fix: format.
zhumin8 Jul 3, 2024
8ade1cd
add test to get request metadata when scope changes, expect empty con…
zhumin8 Jul 10, 2024
549e473
update test and mock transport for expected behavior.
zhumin8 Jul 11, 2024
3891ea8
revert ServiceAccountCredentials changes.
zhumin8 Jul 11, 2024
84b8608
mock transport to only return different access token when url is scoped.
zhumin8 Jul 11, 2024
48737d1
test: simplify createScoped test.
zhumin8 Jul 11, 2024
af9ea2a
test: remove redundant var.
zhumin8 Jul 11, 2024
960e9d5
Merge branch 'main' into fix-scoped-accesstoken
zhumin8 Jul 11, 2024
c789204
fix: combine two tests together.
zhumin8 Jul 17, 2024
b995726
change builder call, added test for project id.
zhumin8 Jul 17, 2024
b4e4e24
review comment: to not hardcode access token for scoped case in MockM…
zhumin8 Jul 17, 2024
f6364be
address review comments.
zhumin8 Jul 23, 2024
cbf5940
review comment.
zhumin8 Jul 24, 2024
6d00eef
fix lint.
zhumin8 Jul 24, 2024
ea25409
Merge branch 'main' into fix-scoped-accesstoken
zhumin8 Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,20 +155,20 @@ private ComputeEngineCredentials(ComputeEngineCredentials.Builder builder) {
/** Clones the compute engine account with the specified scopes. */
@Override
public GoogleCredentials createScoped(Collection<String> newScopes) {
ComputeEngineCredentials.Builder builder =
this.toBuilder().setHttpTransportFactory(transportFactory).setScopes(newScopes);
return new ComputeEngineCredentials(builder);
return createScoped(newScopes, null);
zhumin8 marked this conversation as resolved.
Show resolved Hide resolved
}

/** Clones the compute engine account with the specified scopes and default scopes. */
@Override
public GoogleCredentials createScoped(
Collection<String> newScopes, Collection<String> newDefaultScopes) {
ComputeEngineCredentials.Builder builder =
ComputeEngineCredentials.newBuilder()
.setHttpTransportFactory(transportFactory)
.setScopes(newScopes)
.setDefaultScopes(newDefaultScopes);
(Builder)
this.toBuilder()
.setHttpTransportFactory(transportFactory)
.setScopes(newScopes)
.setDefaultScopes(newDefaultScopes)
.setAccessToken(null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@lqiu96 lqiu96 Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return new ComputeEngineCredentials(builder);
}

Expand Down
4 changes: 2 additions & 2 deletions oauth2_http/java/com/google/auth/oauth2/GoogleCredentials.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,8 +427,8 @@ public boolean createScopedRequired() {
}

/**
* If the credentials support scopes, creates a copy of the identity with the specified scopes;
* otherwise, returns the same instance.
* If the credentials support scopes, creates a copy of the identity with the specified scopes,
* invalidate access token (scoped); otherwise, returns the same instance.
zhumin8 marked this conversation as resolved.
Show resolved Hide resolved
*
* @param scopes Collection of scopes to request.
* @return GoogleCredentials with requested scopes.
Expand Down
49 changes: 48 additions & 1 deletion oauth2_http/javatests/com/google/auth/oauth2/ComputeEngineCredentialsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,20 @@ public void buildScoped_scopesPresent() throws IOException {
assertEquals("foo", scopes.toArray()[0]);
}

@Test
public void buildScoped_shouldInvalidateAccessToken() throws IOException {
ComputeEngineCredentials credentials =
(ComputeEngineCredentials)
ComputeEngineCredentials.newBuilder()
.setScopes(null)
.setAccessToken(AccessToken.newBuilder().build())
.build();
List<String> newScopes = Arrays.asList(null, "foo", "", "bar");
assertNotNull(credentials.getAccessToken());
GoogleCredentials credentialsScoped = credentials.createScoped(newScopes);
assertNull(credentialsScoped.getAccessToken());
}

@Test
public void buildScoped_correctMargins() throws IOException {
ComputeEngineCredentials credentials =
Expand All @@ -215,7 +229,7 @@ public void buildScoped_explicitUniverse() throws IOException {
(ComputeEngineCredentials) credentials.createScoped(Arrays.asList("foo"));

assertEquals("some-universe", scopedCredentials.getUniverseDomain());
assertEquals(true, scopedCredentials.isExplicitUniverseDomain());
assertTrue(scopedCredentials.isExplicitUniverseDomain());
}

@Test
Expand All @@ -228,6 +242,20 @@ public void createScoped_defaultScopes() {
assertEquals("foo", scopes.toArray()[0]);
}

@Test
public void buildDefaultScoped_explicitUniverse() throws IOException {
ComputeEngineCredentials credentials =
ComputeEngineCredentials.newBuilder()
.setScopes(null)
.setUniverseDomain("some-universe")
.build();
ComputeEngineCredentials scopedCredentials =
(ComputeEngineCredentials) credentials.createScoped(null, Arrays.asList("foo"));

assertEquals("some-universe", scopedCredentials.getUniverseDomain());
assertTrue(scopedCredentials.isExplicitUniverseDomain());
}
lqiu96 marked this conversation as resolved.
Show resolved Hide resolved

@Test
public void create_scoped_correctMargins() {
GoogleCredentials credentials =
Expand All @@ -250,6 +278,25 @@ public void getRequestMetadata_hasAccessToken() throws IOException {
TestUtils.assertContainsBearerToken(metadata, accessToken);
}

@Test
public void getRequestMetadata_newAccessTokenWhenScoped() throws IOException {
String accessToken = "1/MkSJoj1xsli0AccessToken_NKPY2";
String accessTokenWithScopes = "fake access token with scope";
MockMetadataServerTransportFactory transportFactory = new MockMetadataServerTransportFactory();
transportFactory.transport.setAccessToken(accessToken);
ComputeEngineCredentials credentials =
ComputeEngineCredentials.newBuilder().setHttpTransportFactory(transportFactory).build();
Map<String, List<String>> metadata = credentials.getRequestMetadata(CALL_URI);

TestUtils.assertContainsBearerToken(metadata, accessToken);

ComputeEngineCredentials scopedCredentialCopy =
(ComputeEngineCredentials) credentials.createScoped(Arrays.asList("foo", "bar"));
Map<String, List<String>> metadataForCopiedCredentials =
scopedCredentialCopy.getRequestMetadata(CALL_URI);
TestUtils.assertContainsBearerToken(metadataForCopiedCredentials, accessTokenWithScopes);
}
zhumin8 marked this conversation as resolved.
Show resolved Hide resolved

@Test
public void getRequestMetadata_missingServiceAccount_throws() {
String accessToken = "1/MkSJoj1xsli0AccessToken_NKPY2";
Expand Down
8 changes: 6 additions & 2 deletions oauth2_http/javatests/com/google/auth/oauth2/MockMetadataServerTransport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ public void setIdToken(String idToken) {

@Override
public LowLevelHttpRequest buildRequest(String method, String url) throws IOException {
if (url.equals(ComputeEngineCredentials.getTokenServerEncodedUrl())) {
if (url.startsWith(ComputeEngineCredentials.getTokenServerEncodedUrl())) {
lqiu96 marked this conversation as resolved.
Show resolved Hide resolved
return getMockRequestForTokenEndpoint(url);
} else if (isGetServiceAccountsUrl(url)) {
return getMockRequestForServiceAccount(url);
Expand Down Expand Up @@ -164,7 +164,11 @@ public LowLevelHttpResponse execute() throws IOException {
// Create the JSON response
GenericJson refreshContents = new GenericJson();
refreshContents.setFactory(OAuth2Utils.JSON_FACTORY);
refreshContents.put("access_token", accessToken);
if (url.contains("?scopes=")) {
refreshContents.put("access_token", "fake access token with scope");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible, could the fake access token not be hard coded in the MockMetaDataServerTransport code?

Perhaps we could make the logic something like:

  1. MockMetaDataServerTransport takes a map of scopes -> access token pairings/ MockMetaDataServerTransport.addAccessTokenScope(scope, accessToken) will add the the pairing
  2. If url.contains("?scopes="), check the map for scopes and returns the configured access token
  3. Test passes the pairing of (scope, accesstoken) to MockMetaDataServerTransport

} else {
refreshContents.put("access_token", accessToken);
}
refreshContents.put("expires_in", 3600000);
refreshContents.put("token_type", "Bearer");
String refreshText = refreshContents.toPrettyString();
Expand Down