feat: add impersonation credentials to ADC #613

liuchaoren · 2021-03-25T19:42:14Z

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes #614 ☕️

codecov · 2021-03-25T19:43:36Z

Codecov Report

Merging #613 (55a0c61) into master (292e81a) will increase coverage by 0.38%.
The diff coverage is 89.18%.

❗ Current head 55a0c61 differs from pull request most recent head b5340d0. Consider uploading reports for the commit b5340d0 to get more accurate results

@@             Coverage Diff              @@
##             master     #613      +/-   ##
============================================
+ Coverage     83.59%   83.98%   +0.38%     
- Complexity      606      622      +16     
============================================
  Files            42       42              
  Lines          2712     2778      +66     
  Branches        289      295       +6     
============================================
+ Hits           2267     2333      +66     
+ Misses          303      300       -3     
- Partials        142      145       +3

Impacted Files	Coverage Δ	Complexity Δ
...java/com/google/auth/oauth2/GoogleCredentials.java	`63.26% <50.00%> (+1.56%)`	`12.00 <0.00> (+1.00)`
...om/google/auth/oauth2/ImpersonatedCredentials.java	`90.67% <90.00%> (+4.27%)`	`33.00 <17.00> (+17.00)`
...google/auth/oauth2/ExternalAccountCredentials.java	`98.46% <100.00%> (+0.70%)`	`27.00 <0.00> (-2.00)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f90c81...b5340d0. Read the comment docs.

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

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

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

elharo · 2021-03-31T13:27:04Z

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

+import java.io.IOException;
+
+/** Indicates that the provided credential has a unsupported type. */
+public class CredentialTypeException extends IOException {


This doesn't feel like an IOException.

This is untested. This class is simple but it suggests there's a code path elsewhere that should throw this that isn't tested.

I removed this customized exception and adopt the same strategy as

google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/GoogleCredentials.java

Line 172 in a5150b5

throw new IOException(

to avoid introducing one more exception in the throw which will propagate to upper level code.

elharo · 2021-03-31T13:30:56Z

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

+    String serviceAccountImpersonationUrl = (String) json.get("service_account_impersonation_url");
+    String targetPrincipal = extractTargetPrincipal(serviceAccountImpersonationUrl);
+
+    List<String> delegates = (List<String>) json.get("delegates");


It looks like they're a lot of potential ClassCastExceptions here that are not being handled. Where does the json object come from?

The json object is passed from

google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/GoogleCredentials.java

Line 149 in a5150b5

public static GoogleCredentials fromStream(

and it reads credential json file from disk. The json file is generated by gcloud which should be well-formatted and user shouldn't manually edit the file. We have done something similar in

google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/ExternalAccountCredentials.java

Line 269 in a5150b5

String audience = (String) json.get("audience");

The code should check this input and be prepared to handle a malformed or invalid file. It does not know where the file came from or how it might have been modified since it was generated. Do not assume it has the format you expect. Even if it's right today, the output of gcloud could change in the future.

If other code already makes this mistake, it should be fixed too.

fair enough, thanks for pointing this out! I added the exception handling and added a test to cover it. Please take another look.

elharo · 2021-04-02T16:12:31Z

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

+    String serviceAccountImpersonationUrl = (String) json.get("service_account_impersonation_url");
+    String targetPrincipal = extractTargetPrincipal(serviceAccountImpersonationUrl);
+
+    List<String> delegates = (List<String>) json.get("delegates");


The code should check this input and be prepared to handle a malformed or invalid file. It does not know where the file came from or how it might have been modified since it was generated. Do not assume it has the format you expect. Even if it's right today, the output of gcloud could change in the future.

If other code already makes this mistake, it should be fixed too.

elharo · 2021-04-02T21:50:21Z

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

@@ -54,6 +54,7 @@
  static final String QUOTA_PROJECT_ID_HEADER_KEY = "x-goog-user-project";
  static final String USER_FILE_TYPE = "authorized_user";
  static final String SERVICE_ACCOUNT_FILE_TYPE = "service_account";
+  static final String IMPERSONATED_SERVICE_ACCOUNT = "impersonated_service_account";


This constant does not improve on the literal string. I suggest just using the string.

I am debating on these two options. I think even though the constant is not more readable than the string itself. I like the way we define all credential types in one place. So people can see all supported types there. I am not very familiar with Java convention, I trust your judgement on this. Please let me know if you still want me to make this change.

That's something that belongs in documentation. Don't rely on the code for that.

elharo · 2021-04-02T21:51:48Z

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

@@ -144,7 +148,52 @@ public static ImpersonatedCredentials create(
  }

  /**
-   * @param sourceCredentials the source credential used as to acquire the impersonated credentials
+   * @param sourceCredentials the source credential used as to acquire the impersonated credentials.


delete "as",
delete final period since it's not a complete sentence

elharo · 2021-04-02T21:51:54Z

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

  private final String transportFactoryClassName;

  private transient HttpTransportFactory transportFactory;

  /**
-   * @param sourceCredentials the source credential used as to acquire the impersonated credentials
+   * @param sourceCredentials the source credential used as to acquire the impersonated credentials.


delete "as",
delete final period since it's not a complete sentence

resolved

silvolu

Please make sure you get approval from @elharo before merging.

elharo · 2021-04-07T14:07:52Z

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

@@ -54,6 +54,7 @@
  static final String QUOTA_PROJECT_ID_HEADER_KEY = "x-goog-user-project";
  static final String USER_FILE_TYPE = "authorized_user";
  static final String SERVICE_ACCOUNT_FILE_TYPE = "service_account";
+  static final String IMPERSONATED_SERVICE_ACCOUNT = "impersonated_service_account";


That's something that belongs in documentation. Don't rely on the code for that.

elharo · 2021-04-07T14:09:08Z

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

+    assertEquals(QUOTA_PROJECT_ID, credentials.getQuotaProjectId());
+    assertEquals(DELEGATES, credentials.getDelegates());
+    assertEquals(new ArrayList<String>(), credentials.getScopes());
+    assertEquals(credentials.getLifetime(), 3600);


expected value comes first; swap the arguments

Done. I made the change in new commits to my branch https://github.com/liuchaoren/google-auth-library-java which should update the PR automatically. Somehow the new commits do not show up in the PR. There might be some lag. I will keep an eye on it.

New commits show up in the PR now.

elharo · 2021-04-07T14:09:16Z

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

+    assertNull(credentials.getQuotaProjectId());
+    assertEquals(new ArrayList<String>(), credentials.getDelegates());
+    assertEquals(new ArrayList<String>(), credentials.getScopes());
+    assertEquals(credentials.getLifetime(), 3600);


expected value comes first; swap the arguments

elharo · 2021-04-07T14:09:22Z

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

+    assertNull(credentials.getQuotaProjectId());
+    assertEquals(DELEGATES, credentials.getDelegates());
+    assertEquals(new ArrayList<String>(), credentials.getScopes());
+    assertEquals(credentials.getLifetime(), 3600);


expected value comes first; swap the arguments

elharo · 2021-04-07T14:09:27Z

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

+    assertEquals(QUOTA_PROJECT_ID, credentials.getQuotaProjectId());
+    assertEquals(DELEGATES, credentials.getDelegates());
+    assertEquals(new ArrayList<String>(), credentials.getScopes());
+    assertEquals(credentials.getLifetime(), 3600);


expected value comes first; swap the arguments

elharo · 2021-04-07T14:10:36Z

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

+      return this;
+    }
+
+    public String getQuotaProjectId() {


This method is not tested. Is it needed at all? It can probably be removed at no loss.

Thanks for pointing it out! It is not used and I removed it.

elharo · 2021-04-07T14:11:56Z

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

+    } catch (ClassCastException | NullPointerException e) {
+      throw new CredentialFormatException("An invalid input stream was provided.", e);
+    }
+    String targetPrincipal = extractTargetPrincipal(serviceAccountImpersonationUrl);


need to handle the IllegalArgumentException here.

Thanks, it is handled.

elharo · 2021-05-10T14:51:27Z

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

+            USER_ACCOUNT_CLIENT_ID,
+            USER_ACCOUNT_CLIENT_SECRET,
+            REFRESH_TOKEN);
+    MockIAMCredentialsServiceTransportFactory mtransportFactory =


mockTransportFactory because style does not abbreviate in variable names

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

elharo · 2021-05-10T14:52:02Z

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

+            USER_ACCOUNT_CLIENT_SECRET,
+            REFRESH_TOKEN);
+    json.remove("delegates");
+    MockIAMCredentialsServiceTransportFactory mtransportFactory =


mockTransportFactory

elharo · 2021-05-10T14:52:09Z

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

+  public void fromJson_ServiceAccountAsSource() throws IOException {
+    GenericJson json =
+        buildImpersonationCredentialsJson(IMPERSONATION_URL, DELEGATES, QUOTA_PROJECT_ID);
+    MockIAMCredentialsServiceTransportFactory mtransportFactory =


mockTransportFactory

elharo · 2021-05-10T14:52:14Z

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

+  @Test()
+  public void fromJson_InvalidFormat() throws IOException {
+    GenericJson json = buildInvalidCredentialsJson();
+    MockIAMCredentialsServiceTransportFactory mtransportFactory =


mockTransportFactory

elharo · 2021-05-10T14:52:41Z

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

+  @Test()
+  public void createScoped() throws IOException {
+    GoogleCredentials sourceCredentials = getSourceCredentials();
+    MockIAMCredentialsServiceTransportFactory mtransportFactory =


mockTransportFactory

elharo · 2021-05-10T14:52:59Z

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

+  public void getRequestMetadata_withQuotaProjectId() throws IOException, IllegalStateException {
+
+    GoogleCredentials sourceCredentials = getSourceCredentials();
+    MockIAMCredentialsServiceTransportFactory mtransportFactory =


mockTransportFactory

elharo · 2021-05-10T14:53:46Z

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

+  @Test()
+  public void getRequestMetadata_withQuotaProjectId() throws IOException, IllegalStateException {
+
+    GoogleCredentials sourceCredentials = getSourceCredentials();


You might be able to move these two lines into a fixture. They are repeated a lot.

elharo · 2021-05-10T14:53:52Z

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

+  public void getRequestMetadata_withoutQuotaProjectId() throws IOException, IllegalStateException {
+
+    GoogleCredentials sourceCredentials = getSourceCredentials();
+    MockIAMCredentialsServiceTransportFactory mtransportFactory =


mockTransportFactory

elharo · 2021-05-10T14:54:49Z

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

@@ -690,7 +908,7 @@ public void serialize() throws IOException, ClassNotFoundException {
    assertSame(deserializedCredentials.clock, Clock.SYSTEM);
  }

-  private String getDefaultExpireTime() {
+  public static String getDefaultExpireTime() {
    Date currentDate = new Date();


why set the date? Calendar.getInstance is already the current time.

elharo · 2021-05-11T10:05:54Z

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

+            GoogleCredentials.fromStream(impersonationCredentialsStream, transportFactoryForSource);
+    credentials.setTransportFactory(transportFactory);
+
+    assertNotNull(credentials);


Line 333 will have already thrown a NullPointerException if credentials is null so you can remove this

elharo · 2021-05-11T10:06:32Z

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

+            GoogleCredentials.fromStream(impersonationCredentialsStream, transportFactoryForSource);
+    credentials.setTransportFactory(transportFactory);
+
+    assertNotNull(credentials);


Line 298 will have already thrown a NullPointerException if credentials is null so you can remove this assertNotNull

elharo · 2021-05-11T10:08:24Z

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

+  }
+
+  @Override
+  public boolean createScopedRequired() {


This name doesn't quite fit. Create tends t be a factory method. This is more of an is method. Is this new in this PR?

This method is not invented in this PR. It is defined in the base class 'GoogleCredentials' which is overridden by its subclasses. I think it uses "create" in its name because of its relationship with the createScoped method.

google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/GoogleCredentials.java

Line 218 in 292e81a

* Indicates whether the credentials require scopes to be specified via a call to {@link

elharo · 2021-05-11T15:03:52Z

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

+    return this.quotaProjectId;
+  }
+
+  public List<String> getDelegates() {


This method doesn't seem used. Can it be removed?

This method is used in tests to verify the delegates.

In that case please annotate it as @VisibleForTesting. Also, make it non-public if possible. (It might not be but worth checking.)

Great, thanks for letting me know the annotation! I added it and removed the "public" to limit the visibility.

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

elharo

Github actions was having trouble yesterday systemwide. Might be fixed now. I pushed the button to rerun.

…s_in (#699) * feat: Adding functional tests for Service Account (#685) ServiceAccountCredentials tests for 4110 * feat: allow scopes for self signed jwt (#689) * feat: self signed jwt support * update * address comments * allow to use uri as audience * address comments * chore: release 0.27.0 (#678) :robot: I have created a release \*beep\* \*boop\* --- ## [0.27.0](https://www.github.com/googleapis/google-auth-library-java/compare/v0.26.0...v0.27.0) (2021-07-14) ### Features * add Id token support for UserCredentials ([#650](https://www.github.com/googleapis/google-auth-library-java/issues/650)) ([5a8f467](https://www.github.com/googleapis/google-auth-library-java/commit/5a8f4676630854c53aa708a9c8b960770067f858)) * add impersonation credentials to ADC ([#613](https://www.github.com/googleapis/google-auth-library-java/issues/613)) ([b9823f7](https://www.github.com/googleapis/google-auth-library-java/commit/b9823f70d7f3f7461b7de40bee06f5e7ba0e797c)) * Adding functional tests for Service Account ([#685](https://www.github.com/googleapis/google-auth-library-java/issues/685)) ([dfe118c](https://www.github.com/googleapis/google-auth-library-java/commit/dfe118c261aadf137a3cf47a7acb9892c7a6db4d)) * allow scopes for self signed jwt ([#689](https://www.github.com/googleapis/google-auth-library-java/issues/689)) ([f4980c7](https://www.github.com/googleapis/google-auth-library-java/commit/f4980c77566bbd5ef4c532acb199d7d484dbcd01)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). * test: adds integration tests for downscoping with credential access boundaries * fix: STS does not always return expires_in, fallback to source credential expiration for DownscopedCredentials * fix: review Co-authored-by: Timur Sadykov <[email protected]> Co-authored-by: arithmetic1728 <[email protected]> Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

liuchaoren added 4 commits March 22, 2021 18:09

ADC can load impersonation credentials

887c67d

Add tests for new features in ImpersonationCredentials

f98b61c

Merge remote-tracking branch 'upstream/master'

9f805fb

Add tests for GoogleCredentials

ebeba8b

liuchaoren requested a review from a team as a code owner March 25, 2021 19:42

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Mar 25, 2021

liuchaoren changed the title ~~Add impersonation credentials to ADC.~~ feat: add impersonation credentials to ADC Mar 25, 2021

liuchaoren added 2 commits March 25, 2021 15:53

Fix linter errors

33f6db0

Fix linter errors in ImpersonatedCredentialsTest

8b38844

silvolu reviewed Mar 30, 2021

View reviewed changes

oauth2_http/java/com/google/auth/oauth2/ImpersonatedCredentials.java Outdated Show resolved Hide resolved

silvolu reviewed Mar 31, 2021

View reviewed changes

elharo reviewed Mar 31, 2021

View reviewed changes

liuchaoren added 3 commits April 1, 2021 13:30

Fix issues after receiving comments

3a98164

Merge remote-tracking branch 'upstream/master'

ea3d14a

Fix lint errors

815e39b

elharo previously requested changes Apr 2, 2021

View reviewed changes

liuchaoren added 3 commits April 2, 2021 14:48

Handle ClassCastException in fromJson

0ee39aa

Fix lint errors

35b347a

minor refactoring

56ed2bb

liuchaoren requested a review from elharo April 2, 2021 19:12

elharo reviewed Apr 2, 2021

View reviewed changes

liuchaoren added 2 commits April 2, 2021 18:09

fix doc strings

a36da60

fix lint errors

1b3bcd6

silvolu approved these changes Apr 2, 2021

View reviewed changes

liuchaoren added 2 commits April 2, 2021 23:55

delegates can be missing from the json file

685ea43

Mark test using @test()

3a20de4

elharo previously requested changes Apr 7, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master'

3ac3809

Fix docstring and move one variable to inner scope.

2febd2a

liuchaoren requested a review from elharo May 5, 2021 14:06

Merge branch 'master' into master

3402664

elharo approved these changes May 5, 2021

View reviewed changes

Merge branch 'master' into master

eee39af

elharo reviewed May 10, 2021

View reviewed changes

liuchaoren added 4 commits May 10, 2021 23:17

Merge remote-tracking branch 'upstream/master'

b2751ae

Refactor ImpersonatedCredentialsTest

8420528

Merge branch 'master' of github.com:liuchaoren/google-auth-library-java

51525ab

Reformat the ImpersonatedCredentialsTest

d34dae2

liuchaoren requested a review from elharo May 11, 2021 04:45

elharo reviewed May 11, 2021

View reviewed changes

Remove redundant checks in tests

4ea84e9

liuchaoren requested a review from elharo May 12, 2021 19:45

Use VisibleForTesting annotation to limit visibility

55a0c61

elharo approved these changes May 12, 2021

View reviewed changes

Merge branch 'master' into master

2884531

elharo reviewed May 19, 2021

View reviewed changes

elharo added 3 commits May 20, 2021 15:04

Merge branch 'master' into master

35206dc

Merge branch 'master' into master

6ea5150

Merge branch 'master' into master

b5340d0

liuchaoren merged commit b9823f7 into googleapis:master May 21, 2021

codyoss mentioned this pull request Sep 3, 2021

impersonated_service_account do not take into account golang/oauth2#515

Closed

quartzmo mentioned this pull request Nov 15, 2021

Add full support for service account impersonation googleapis/google-auth-library-ruby#353

Open

release-please bot mentioned this pull request Jan 4, 2022

chore(main): release google-auth-library-java 1.0.0 #819

Closed

release-please bot mentioned this pull request Aug 11, 2022

chore(main): release 1.11.0 #974

Merged

feat: add impersonation credentials to ADC #613

feat: add impersonation credentials to ADC #613

Conversation

liuchaoren commented Mar 25, 2021 • edited Loading

codecov bot commented Mar 25, 2021 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo Apr 2, 2021 • edited Loading

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

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

silvolu 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

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

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

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

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

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo left a comment

Choose a reason for hiding this comment

liuchaoren commented Mar 25, 2021 •

edited

Loading

codecov bot commented Mar 25, 2021 •

edited

Loading

elharo Apr 2, 2021 •

edited

Loading