feat: Support universe domain by amanda-tarafa · Pull Request #2635 · googleapis/google-api-dotnet-client

amanda-tarafa · 2023-12-15T22:18:44Z

@jskeet for when you get to this, as usual, one commit at a time.

jskeet

Almost entirely typo nits; one breaking change that does need fixing.

For ComputeCredential, it looks like we don't detect when we're using a ComputeCredential with an explicitly set universe domain, but that's not the one it's actually running in. I'll need to check the specs again to see if that's okay.

jskeet · 2023-12-19T10:27:59Z

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

+        /// After that, this method will always return a completed task.
+        /// The task's result will never be null.
+        /// </remarks>
+        Task<string> GetUniverseDomainAsync(CancellationToken cancellationToken);


And presumably the cancellation token should only ever apply to this call - if this call times out, a later call could still succeed?

Yes! And even viceversa assuming the first cancellation token waits for a shorter time than a second call cancellation token. I'm adding this in docs.

jskeet · 2023-12-19T10:31:18Z

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


        /// <summary>IAM access token endpoint format string. To use it insert the service account email.</summary>
-        internal static readonly string IamAccessTokenEndpointFormatString = $"{IamServiceAccountEndpointCommonPrefix}{{0}}:{IamAccessTokenVerb}";
+        internal static readonly string IamAccessTokenEndpointFormatString = $"{IamServiceAccountEndpointCommonPrefixFormat}{{1}}:{IamAccessTokenVerb}";


We can make all of these const, I believe. (Before .NET 6, we couldn't - but IamServiceAccountEndpointCommonPrefixFormat proves that we can use const interpolated strings.)

Ah yes, true! Done.

jskeet · 2023-12-19T10:32:57Z

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

+using System.Threading.Tasks;
+using Xunit;
+
+namespace Google.Apis.Auth.Tests.OAuth2


File-scoped namespace?

jskeet · 2023-12-19T10:34:12Z

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

+
+            public IDataStore DataStore => throw new NotImplementedException();
+
+            public AuthorizationCodeRequestUrl CreateAuthorizationCodeRequest(string redirectUri)


Use expression-bodied members for all of these? (I think it's fine for that to mean long lines, probably not even with line-breaks between the methods, for just a block of "stuff we haven't implemented.)

jskeet · 2023-12-19T10:34:49Z

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

+using System.Threading.Tasks;
+using Xunit;
+
+namespace Google.Apis.Auth.Tests.OAuth2


And again... I won't add any more comments like this.

Done, and yes, I'll o through all, thanks!

jskeet · 2023-12-19T10:50:42Z

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

+        [Fact]
+        public async Task WithUniverseDomain()
+        {
+            string principal = "principal";


Some comments occasionally would help to make the structure of this test clearer, I think.

jskeet · 2023-12-19T10:52:41Z

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

+                // i.e. we could calculate the endpoint synchronusly on credential creation.
+                return TokenServerUrl;
+            }
+            string univerDomain = await (this as IGoogleCredential).GetUniverseDomainAsync(cancellationToken: default).ConfigureAwait(false);


univerDomain => universeDomain

jskeet · 2023-12-19T10:52:47Z

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

-        private void ThrowIfCustomTokenUrl()
+        /// <summary>
+        /// Returns the token URL to be used by this credential, which may be a custom token URL
+        /// or the IAM API access tokne endpoint URL which is built using the universe domain and the


tokne => token

jskeet · 2023-12-19T10:53:29Z

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

+        }
+
+        /// <summary>
+        /// Get's the id token URL if this credential supports id token emission.


Get's => Gets

jskeet · 2023-12-19T10:55:57Z

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

            }
+            if (credentialParameters.UniverseDomain is not null && credentialParameters.UniverseDomain != GoogleAuthConsts.DefaultUniverseDomain)
+            {
+                throw new InvalidOperationException($"{nameof(UserCredential)} is not supported in other than {GoogleAuthConsts.DefaultUniverseDomain}");


"not supported in other than" => "not supported in universe domains other than"

TimurSadykov · 2023-12-21T15:23:30Z

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

+
+                        return await response.Content.ReadAsStringAsync().ConfigureAwait(false);
+                    }
+                    catch (Exception) when (response?.StatusCode == HttpStatusCode.NotFound)


Passing by comment )

what if status code is not 404, but still an error? technically possible.. would it fall into the InvalidOperaition?

In that case, and if it's not a timeout (timeout is handled by catching the OperationCanceldException) then the original exception is bubled up. This is also what happens on token fetching, and I'm mimicking that here as per spec.
Does that sound good?

Oh got it.. I misunderstood the Exception catch the first time. Looks good.

amanda-tarafa

@jskeet , @TimurSadykov I've addressed all your comments.

Changes are in ordered commits that start with Address Review:, and the last commit is also new.

PTAL, thanks!

amanda-tarafa · 2024-01-04T08:05:00Z

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


        /// <summary>IAM access token endpoint format string. To use it insert the service account email.</summary>
-        internal static readonly string IamAccessTokenEndpointFormatString = $"{IamServiceAccountEndpointCommonPrefix}{{0}}:{IamAccessTokenVerb}";
+        internal static readonly string IamAccessTokenEndpointFormatString = $"{IamServiceAccountEndpointCommonPrefixFormat}{{1}}:{IamAccessTokenVerb}";


Ah yes, true! Done.

amanda-tarafa · 2024-01-05T00:44:11Z

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

+        /// After that, this method will always return a completed task.
+        /// The task's result will never be null.
+        /// </remarks>
+        Task<string> GetUniverseDomainAsync(CancellationToken cancellationToken);


Yes! And even viceversa assuming the first cancellation token waits for a shorter time than a second call cancellation token. I'm adding this in docs.

amanda-tarafa · 2024-01-05T00:52:16Z

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

+using System.Threading.Tasks;
+using Xunit;
+
+namespace Google.Apis.Auth.Tests.OAuth2


amanda-tarafa · 2024-01-05T00:59:01Z

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

+
+            public IDataStore DataStore => throw new NotImplementedException();
+
+            public AuthorizationCodeRequestUrl CreateAuthorizationCodeRequest(string redirectUri)


amanda-tarafa · 2024-01-05T02:41:34Z

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

+using System.Threading.Tasks;
+using Xunit;
+
+namespace Google.Apis.Auth.Tests.OAuth2


Done, and yes, I'll o through all, thanks!

amanda-tarafa · 2024-01-05T06:13:13Z

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

-        private void ThrowIfCustomTokenUrl()
+        /// <summary>
+        /// Returns the token URL to be used by this credential, which may be a custom token URL
+        /// or the IAM API access tokne endpoint URL which is built using the universe domain and the


amanda-tarafa · 2024-01-05T06:14:06Z

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

+                // i.e. we could calculate the endpoint synchronusly on credential creation.
+                return TokenServerUrl;
+            }
+            string univerDomain = await (this as IGoogleCredential).GetUniverseDomainAsync(cancellationToken: default).ConfigureAwait(false);


amanda-tarafa · 2024-01-05T06:14:25Z

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

+        }
+
+        /// <summary>
+        /// Get's the id token URL if this credential supports id token emission.


amanda-tarafa · 2024-01-05T06:14:50Z

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

+        [Fact]
+        public async Task WithUniverseDomain()
+        {
+            string principal = "principal";


amanda-tarafa · 2024-01-05T06:48:21Z

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

            }
+            if (credentialParameters.UniverseDomain is not null && credentialParameters.UniverseDomain != GoogleAuthConsts.DefaultUniverseDomain)
+            {
+                throw new InvalidOperationException($"{nameof(UserCredential)} is not supported in other than {GoogleAuthConsts.DefaultUniverseDomain}");


amanda-tarafa · 2024-01-05T06:58:49Z

For ComputeCredential, it looks like we don't detect when we're using a ComputeCredential with an explicitly set universe domain, but that's not the one it's actually running in. I'll need to check the specs again to see if that's okay.

Yes, thats OK, apparently there will be cases in which one universe accepts credentials from another (related) universe.

What we'll need to check is that the service client "intentions" are matched by the credential, i.e. the universe specified for the service client is the same as the one specified for the credential.

TimurSadykov

LGTM

jskeet · 2024-01-15T08:16:15Z

@amanda-tarafa: Is there anything new you'd like me to review here?

amanda-tarafa · 2024-01-15T18:35:05Z

Nope, my last push wash just about squashing the "address review" commits.

I'll merge and release later today.

…nts.

- feat: [#2635](#2635) Authentication support for multiple universe domains.

Features - Universe domain support: - feat: [#2675](#2675) Universe domain support for Discovery based libraries. - feat: [#2635](#2635) Universe domain support for authentication.

amanda-tarafa requested review from a team and jskeet December 15, 2023 22:18

jskeet reviewed Dec 19, 2023

View reviewed changes

TimurSadykov reviewed Dec 21, 2023

View reviewed changes

amanda-tarafa force-pushed the universe_domain branch from a45fb13 to 84c212d Compare January 5, 2024 06:50

amanda-tarafa commented Jan 5, 2024

View reviewed changes

TimurSadykov approved these changes Jan 5, 2024

View reviewed changes

jskeet approved these changes Jan 8, 2024

View reviewed changes

amanda-tarafa added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 12, 2024

amanda-tarafa force-pushed the universe_domain branch from 84c212d to be38474 Compare January 12, 2024 18:47

amanda-tarafa added 13 commits January 17, 2024 08:26

chore: Rename some test data.

47a81a5

feat: Read the token endpoint url from the SA credential JSON key file.

280c8f3

feat: Allow credentials to specify the universe when using IAM endpoi…

9d1ed8a

…nts.

feat: IGoogleCredential supports universe domain.

898fae8

feat: UserCredential supports universe domain.

04dfba1

feat: AccessTokenCredential supports universe domain.

7bb2571

feat: BYOID credentials support universe domain.

8636eb0

feat: ServiceAccountCredential supports universe domain.

45fc2a1

feat: ComputeCredential supports universe domain.

99c5b5e

feat: ImpersonatedCredential supports universe domain.

25378b8

feat: GoogleCredential supports universe domain.

dcff265

feat: ADC supports universe domain

c855eb3

tests: Explicitly name variables for fake values as such.

7c48ae4

amanda-tarafa force-pushed the universe_domain branch from be38474 to 7c48ae4 Compare January 17, 2024 16:27

amanda-tarafa removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 17, 2024

amanda-tarafa merged commit 9f1c8cb into main Jan 17, 2024

amanda-tarafa deleted the universe_domain branch January 17, 2024 16:29

amanda-tarafa added a commit that referenced this pull request Jan 17, 2024

Update support version v1.66.0 -> v1.67.0-beta01

a8f2437

- feat: [#2635](#2635) Authentication support for multiple universe domains.

amanda-tarafa mentioned this pull request Jan 17, 2024

Update support version v1.66.0 -> v1.67.0-beta01 #2651

Merged

amanda-tarafa added a commit that referenced this pull request Jan 17, 2024

Update support version v1.66.0 -> v1.67.0-beta01

dc2beea

- feat: [#2635](#2635) Authentication support for multiple universe domains.

amanda-tarafa mentioned this pull request Feb 22, 2024

Update support version 1.67.0-beta01 -> 1.67.0 #2683

Merged


		public IDataStore DataStore => throw new NotImplementedException();

		public AuthorizationCodeRequestUrl CreateAuthorizationCodeRequest(string redirectUri)

Conversation

amanda-tarafa commented Dec 15, 2023

Uh oh!

jskeet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimurSadykov Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanda-tarafa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanda-tarafa commented Jan 5, 2024

Uh oh!

TimurSadykov left a comment

TimurSadykov Dec 21, 2023 •

edited

Loading