feat: Improve impersonation support. #2394

amanda-tarafa · 2023-04-22T00:37:46Z

No description provided.

amanda-tarafa · 2023-04-22T00:38:38Z

@jskeet as always one commit at a time. The first commit is unrelated, but it's something that I caught while running the tests.

jskeet

I suspect this is all fine, but let's chat before merging.

jskeet · 2023-04-24T07:14:21Z

Src/Support/Google.Apis.Auth.Tests/OAuth2/DefaultCredentialProviderTests.cs

+  ""type"": ""authorized_user""
+},
+""type"": ""impersonated_service_account""}";
+        private const string RecursiveImpersonatedServiceAccountCredential = @"{


Possibly add a blank line between each of these service credentials? (Or better yet, maybe now's a good time to move them to resource files that we load? That way we don't need the quote doubling etc.)

Moving to files on the latest commit.

jskeet · 2023-04-24T07:16:14Z

Src/Support/Google.Apis.Auth/OAuth2/DefaultCredentialProvider.cs

+            }
+
+            // If source credential is of a credential type that does not support impersonation, attemting to create the
+            // impersonated credential will faill a few lines after.


faill => fail (and I'd personally use "later" instead of "after" but the meaning is clear)

Yep, both done.

jskeet · 2023-04-24T07:17:26Z

Src/Support/Google.Apis.Auth/OAuth2/JsonCredentialParameters.cs

+        /// <summary>
+        /// ImpersonatedCredential is created by the GCloud SDK tool when the user runs
+        /// <a href="https://cloud.google.com/sdk/gcloud/reference/auth/application-default/login">GCloud Auth ADC Login</a>
+        /// using the <a href="https://cloud.google.com/sdk/gcloud/reference#--impersonate-service-account">--impersonate-service-accont</a>


accont => account (assuming there's no typo in gcloud itself)

Luckily the typo is just mine, changing.

TimurSadykov · 2023-04-24T17:53:13Z

passing by comment ) Thanks!

… in googleapis#2394.

amanda-tarafa

@jskeet the las commit contains the changes to DefaultCredentialProvider tests.

The other couple requests are squashed in their respective commits, they were typos.

amanda-tarafa · 2023-04-24T13:03:36Z

Src/Support/Google.Apis.Auth.Tests/OAuth2/DefaultCredentialProviderTests.cs

+  ""type"": ""authorized_user""
+},
+""type"": ""impersonated_service_account""}";
+        private const string RecursiveImpersonatedServiceAccountCredential = @"{


Moving to files on the latest commit.

amanda-tarafa · 2023-04-24T13:04:54Z

Src/Support/Google.Apis.Auth/OAuth2/DefaultCredentialProvider.cs

+            }
+
+            // If source credential is of a credential type that does not support impersonation, attemting to create the
+            // impersonated credential will faill a few lines after.


Yep, both done.

amanda-tarafa · 2023-04-24T13:07:14Z

Src/Support/Google.Apis.Auth/OAuth2/JsonCredentialParameters.cs

+        /// <summary>
+        /// ImpersonatedCredential is created by the GCloud SDK tool when the user runs
+        /// <a href="https://cloud.google.com/sdk/gcloud/reference/auth/application-default/login">GCloud Auth ADC Login</a>
+        /// using the <a href="https://cloud.google.com/sdk/gcloud/reference#--impersonate-service-account">--impersonate-service-accont</a>


Luckily the typo is just mine, changing.

jskeet

Much nicer :)

jskeet · 2023-04-25T05:52:47Z

Src/Support/Google.Apis.Auth.Tests/Google.Apis.Auth.Tests.csproj

@@ -34,6 +35,25 @@
    <PackageReference Include="Microsoft.Extensions.Configuration.Binder" Version="6.0.0" />
  </ItemGroup>

+  <ItemGroup>
+    <EmbeddedResource Include="OAuth2\DummyCredentialFiles\aws_workforce_external_account_credential.json" />


Any reason for not just using OAuth2\DummyCredentialFiles\*.json here?

Just because I marked the files as embeded with right click on VS, but yes, this is better, changing. :)

jskeet · 2023-04-25T05:55:49Z

Src/Support/Google.Apis.Auth.Tests/OAuth2/DefaultCredentialProviderTests.cs


        protected override Stream GetStream(string filePath)
        {
-            if (!fileContents.ContainsKey(filePath))
+            if (!pathToFileName.TryGetValue(filePath, out var resourcaName))


Nit: resourcaName => resourceName

And maybe just:

var resourceName = pathToFileName.TryGetValue(filePath, out var mappedPath) ? mappedPath : filePath;

Yep, done on both.

Documentation does not specify the exact reason given on each case and this seems to have changed recently. StatusCode continues to be the same. See: https://developers.google.com/identity/protocols/oauth2/web-server#httprest_8

Closes googleapis#2246

Closes googleapis#2363

… in googleapis#2394.

- Move all dummy credentials to files that are loaded as resources. - Use Assert.Throws instead of try ... catch.

amanda-tarafa

Both comments addressed.

I'll merge on green, thanks!

amanda-tarafa · 2023-04-25T07:13:39Z

Src/Support/Google.Apis.Auth.Tests/Google.Apis.Auth.Tests.csproj

@@ -34,6 +35,25 @@
    <PackageReference Include="Microsoft.Extensions.Configuration.Binder" Version="6.0.0" />
  </ItemGroup>

+  <ItemGroup>
+    <EmbeddedResource Include="OAuth2\DummyCredentialFiles\aws_workforce_external_account_credential.json" />


Just because I marked the files as embeded with right click on VS, but yes, this is better, changing. :)

amanda-tarafa · 2023-04-25T07:59:56Z

Src/Support/Google.Apis.Auth.Tests/OAuth2/DefaultCredentialProviderTests.cs


        protected override Stream GetStream(string filePath)
        {
-            if (!fileContents.ContainsKey(filePath))
+            if (!pathToFileName.TryGetValue(filePath, out var resourcaName))


Yep, done on both.

Features - googleapis#2429 Utilities for date/time parsing. - googleapis#2394 Which improves impersonation support. - googleapis#2349 and googleapis#2379 Which improve error reported when ADC is not configured.

Features - #2429 Utilities for date/time parsing. - #2394 Which improves impersonation support. - #2349 and #2379 Which improve error reported when ADC is not configured.

Updates support version: 1.61.0 - beta02 -> 1.61.0 Features - Improvements for date/time parsing. - googleapis#2441 - googleapis#2432 - googleapis#2429 - googleapis#2435 PKCE support. - googleapis#2394 Which improves impersonation support. - googleapis#2349 and googleapis#2379 Which improve error reported when ADC is not configured.

Updates support version: 1.61.0 - beta02 -> 1.61.0 Features - Improvements for date/time parsing. - #2441 - #2432 - #2429 - #2435 PKCE support. - #2394 Which improves impersonation support. - #2349 and #2379 Which improve error reported when ADC is not configured.

amanda-tarafa requested a review from jskeet April 22, 2023 00:37

amanda-tarafa requested a review from a team as a code owner April 22, 2023 00:37

amanda-tarafa mentioned this pull request Apr 22, 2023

Support impersonated_service_account for application default credentials #2159

Closed

jskeet approved these changes Apr 24, 2023

View reviewed changes

amanda-tarafa added a commit to amanda-tarafa/google-api-dotnet-client that referenced this pull request Apr 24, 2023

chore: Remove googleapis#2159 from backlog as it has been implemented…

2b0182c

… in googleapis#2394.

amanda-tarafa force-pushed the impersonation-improvements branch from f674a11 to a0bd208 Compare April 24, 2023 22:55

amanda-tarafa commented Apr 24, 2023

View reviewed changes

jskeet approved these changes Apr 25, 2023

View reviewed changes

amanda-tarafa added 5 commits April 25, 2023 09:01

feat: ComputeCredential supports impersonation.

6c558e1

Closes googleapis#2246

feat: Support impersonated credentials in ADC.

d5d6f39

Closes googleapis#2363

chore: Remove googleapis#2159 from backlog as it has been implemented…

de5fe0f

… in googleapis#2394.

tests: Improvements to DefaultCredentialProvider tests.

e226361

- Move all dummy credentials to files that are loaded as resources. - Use Assert.Throws instead of try ... catch.

amanda-tarafa force-pushed the impersonation-improvements branch from a0bd208 to e226361 Compare April 25, 2023 08:05

amanda-tarafa commented Apr 25, 2023

View reviewed changes

amanda-tarafa merged commit 0b8714c into googleapis:main Apr 25, 2023
4 checks passed

amanda-tarafa added a commit that referenced this pull request Apr 25, 2023

chore: Remove #2159 from backlog as it has been implemented in #2394.

074abeb

amanda-tarafa deleted the impersonation-improvements branch April 25, 2023 08:10

amanda-tarafa mentioned this pull request Jun 14, 2023

Update support version: 1.60.0 -> 1.61.0-beta01 #2430

Merged

amanda-tarafa mentioned this pull request Jun 26, 2023

Update version: 1.60.0 -> 1.61.0 #2442

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Improve impersonation support. #2394

feat: Improve impersonation support. #2394

amanda-tarafa commented Apr 22, 2023

amanda-tarafa commented Apr 22, 2023

jskeet left a comment

jskeet Apr 24, 2023

amanda-tarafa Apr 24, 2023

jskeet Apr 24, 2023

amanda-tarafa Apr 24, 2023

jskeet Apr 24, 2023

amanda-tarafa Apr 24, 2023

TimurSadykov commented Apr 24, 2023

amanda-tarafa left a comment

amanda-tarafa Apr 24, 2023

amanda-tarafa Apr 24, 2023

amanda-tarafa Apr 24, 2023

jskeet left a comment

jskeet Apr 25, 2023

amanda-tarafa Apr 25, 2023

jskeet Apr 25, 2023

amanda-tarafa Apr 25, 2023

amanda-tarafa left a comment

amanda-tarafa Apr 25, 2023

amanda-tarafa Apr 25, 2023

feat: Improve impersonation support. #2394

feat: Improve impersonation support. #2394

Conversation

amanda-tarafa commented Apr 22, 2023

amanda-tarafa commented Apr 22, 2023

jskeet 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

TimurSadykov commented Apr 24, 2023

amanda-tarafa 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

jskeet 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

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment