feat: ComputeEngineCredential supports specifying scopes #457

ahmedtd · 2020-08-06T00:43:19Z

The existing behavior of ComputeEngineCredentials is to not provide the ?scopes parameter in its access token request to the metadata server. This is workable on GCE, which has the concept of a scope lock / default scopes applied to the VM. However, neither new AppEngine nor GKE Workload Identity have this concept; they just return a hardcoded set of default scopes if no
specific scopes are requested. This makes usage of some Google APIs impossible using the Java client libraries.

This change adds scope support to ComputeEngineCredentials. It's optional, so the current behavior will stay the default.
However, if the user specifies scopes on the builder or using createScoped, then those scopes will be passed to the metadata server.

ahmedtd · 2020-08-06T00:45:17Z

@chingor13 This is the patch we discussed.

elharo

It really helps if changes like this are associated with a detailed issue.

pom.xml

chingor13 · 2020-08-06T16:55:45Z

oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java

@@ -117,13 +126,27 @@ private ComputeEngineCredentials(HttpTransportFactory transportFactory) {
   * @return new ComputeEngineCredentials
   */
  public static ComputeEngineCredentials create() {
-    return new ComputeEngineCredentials(null);
+    return new ComputeEngineCredentials(ImmutableSet.<String>of(), null);


Should this default to null instead?

An empty collection of scopes might make the url something like https://some-url/?scopes= which may be correct if you ask for no scopes, but if you pass null, it should exclude the scopes param.

Yes, you're right. I originally wrote this so that an empty collection would result in not sending the ?scopes param, but then changed it to null.

chingor13 · 2020-08-06T17:13:59Z

cc @salrashid123

The existing behavior of ComputeEngineCredentials is to not provide the `?scopes` parameter in its access token request to the metadata server. This is workable on GCE, which has the concept of a scope lock / default scopes applied to the VM. However, neither new AppEngine nor GKE Workload Identity have this concept; they just return a hardcoded set of default scopes if no specific scopes are requested. This makes usage of some Google APIs impossible using the Java client libraries. This change adds scope support to ComputeEngineCredentials. It's optional, so the current behavior will stay the default. However, if the user specifies scopes on the builder or using `createScoped`, then those scopes will be passed to the metadata server.

chingor13

Also please run mvn com.coveo:fmt-maven-plugin:format to appease the linter.

chingor13 · 2020-08-07T18:32:47Z

oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java

-    HttpResponse response = getMetadataResponse(getTokenServerEncodedUrl());
+    GenericUrl url = new GenericUrl(getTokenServerEncodedUrl());
+    if (scopes != null) {
+      url.set("scopes", String.join(",", scopes));


Looks like String.join was added in Java 8 and this library still currently support Java 7.

You can use Guava's Joiner in this class.

ahmedtd · 2020-08-08T01:15:52Z

@elharo I opened #458 . I have some follow up work to confirm that this works the same way in the AppEngine flex environment.

elharo · 2020-08-10T14:48:24Z

oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java

-    HttpResponse response = getMetadataResponse(getTokenServerEncodedUrl());
+    GenericUrl url = new GenericUrl(getTokenServerEncodedUrl());
+    if (scopes != null) {
+      url.set("scopes", String.join(",", scopes));


You can use Guava's Joiner in this class.

elharo · 2020-08-10T14:49:10Z

oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java

@@ -411,12 +437,21 @@ public Builder setHttpTransportFactory(HttpTransportFactory transportFactory) {
      return this;
    }

+    public Builder setScopes(Collection<String> scopes) {
+      this.scopes = scopes;


You probably want a defensive copy to avoid exposing mutable internal state.

elharo · 2020-08-10T14:49:25Z

oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java

    public HttpTransportFactory getHttpTransportFactory() {
      return transportFactory;
    }

+    public Collection<String> getScopes() {


You probably want a defensive copy to avoid exposing mutable internal state.

elharo · 2020-08-10T14:49:42Z

oauth2_http/javatests/com/google/auth/oauth2/MockMetadataServerTransport.java

+    GenericUrl parsedURL = new GenericUrl(url);
+
+    if ("/computeMetadata/v1/instance/service-accounts/default/token".equals(parsedURL.getRawPath())) {
+      String scopes = (String)parsedURL.getFirst("scopes");


space after cast

chingor13 · 2020-12-16T22:56:09Z

Addressed in #514

ahmedtd requested a review from a team as a code owner August 6, 2020 00:43

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 6, 2020

elharo reviewed Aug 6, 2020

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

chingor13 requested changes Aug 6, 2020

View reviewed changes

chingor13 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 6, 2020

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 6, 2020

ahmedtd force-pushed the gce-scopes branch from 72a3b06 to 828fadb Compare August 7, 2020 01:47

ahmedtd force-pushed the gce-scopes branch from 828fadb to fe7a190 Compare August 7, 2020 01:52

chingor13 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 7, 2020

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 7, 2020

chingor13 requested changes Aug 7, 2020

View reviewed changes

elharo reviewed Aug 10, 2020

View reviewed changes

chingor13 closed this Dec 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ComputeEngineCredential supports specifying scopes #457

feat: ComputeEngineCredential supports specifying scopes #457

ahmedtd commented Aug 6, 2020 •

edited

ahmedtd commented Aug 6, 2020

elharo left a comment

chingor13 Aug 6, 2020

ahmedtd Aug 7, 2020

ahmedtd Aug 7, 2020

chingor13 commented Aug 6, 2020

chingor13 left a comment

chingor13 Aug 7, 2020

elharo Aug 10, 2020

ahmedtd commented Aug 8, 2020

elharo Aug 10, 2020

elharo Aug 10, 2020

elharo Aug 10, 2020

elharo Aug 10, 2020

chingor13 commented Dec 16, 2020

feat: ComputeEngineCredential supports specifying scopes #457

feat: ComputeEngineCredential supports specifying scopes #457

Conversation

ahmedtd commented Aug 6, 2020 • edited

ahmedtd commented Aug 6, 2020

elharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chingor13 commented Aug 6, 2020

chingor13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmedtd commented Aug 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chingor13 commented Dec 16, 2020

ahmedtd commented Aug 6, 2020 •

edited