feat: UrlSigner supports custom endpoints #12042

amanda-tarafa · 2024-03-05T00:33:23Z

And can be built from StorageClient

@jskeet as always, one commit at a time. And a couple of notes:

The second commit is a Firestore commit implementing /test: add tests for update with exists=false precondition conformance-tests#84 because Firestore tests started failing after I updated the conformance tests module.
The Storage conformance tests should be fine now, so this PR is ready for full review.

jskeet

Almost entirely comment typos - but I think we should discuss the property vs method choice (unless you're happy to just make the change).

jskeet · 2024-03-05T12:18:20Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/UrlSigner.Options.cs

@@ -174,15 +195,31 @@ public sealed class Options
            /// <param name="urlStyle">The new url style.</param>
            /// <returns>A new set ofoptions with the given url style.</returns>
            public Options WithUrlStyle(UrlStyle urlStyle) =>
-                new Options(Duration, Expiration, SigningVersion, urlStyle, Scheme, null);
+                new Options(Duration, Expiration, SigningVersion, urlStyle, Scheme, Host, Port, null);


Possibly note that this clears BucketBoundHostname? I'm pretty sure that's deliberate, but it confused me for a while.

Ah, yes, done.

jskeet · 2024-03-05T12:20:16Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1.Tests/UrlSignerTest.OptionsTest.cs

@@ -240,6 +241,45 @@ public void WithPort_Null()
                Assert.NotSame(options, newOptions);
                Assert.Null(newOptions.Port);
            }
+
+            [Fact]
+            public void WithDefultOverrides_NullOverrideValues()


Defult => Default here and in the other tests

(And while amending this commit, the message is missing an "r" from "URLSigner".)

Oh, done, both, and default here and eerywhere.

jskeet · 2024-03-05T12:20:56Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/UrlSigner.DefaultOptionsOverrides.cs

+{
+    /// <summary>
+    /// Overrides for default values for some of the options in <see cref="Options"/>.
+    /// Useful for creating signers where signing options default to specific valued, for instance


valued => values

jskeet · 2024-03-05T12:21:59Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/UrlSigner.Options.cs


            /// <summary>
            /// The host to use for generating the signed URL.
            /// This will never be null. If null is specified in <see cref="WithHost(string)"/>
            /// then <see cref="DefaultStorageHost"/> will be used.
            /// Will be ignored if <see cref="UrlStyle"/> is set to <see cref="UrlStyle.BucketBoundHostname"/>.
            /// </summary>
-            public string Host { get; }
+            public string Host => ExplicitHost ?? DefaultOptionsOverrides?.Host ?? DefaultStorageHost;


I think the comment on line 82 may need to be amended.

jskeet · 2024-03-05T12:22:24Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/UrlSigner.Options.cs

-                TimeSpan? duration, DateTimeOffset? expiration, SigningVersion version, UrlStyle? urlStyle, string scheme, string host, int? port, string bucketBoundHostname)
+            /// <summary>
+            /// Values to be used as default for some options in case they are not set,
+            /// instead of the usual default values.May be null.


Nit: space after the period before "May be null"

jskeet · 2024-03-05T12:23:35Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/UrlSigner.cs

+        internal UrlSigner WithClock(IClock clock) => new UrlSigner(_blobSignerProvider, clock, _defaultOptionsOverrides);
+
+        /// <summary>
+        /// Returns a URL signer identical to this one, excepto for the default options overrides used for signing.


excepto => except

jskeet · 2024-03-05T12:26:37Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/StorageClient.cs

+        /// on compatible credentials.
+        /// </summary>
+        /// <remarks>
+        /// Because credentials used by this client may changed, this property will return a new <see cref="UrlSigner"/> instance


I think it would be better to make this a method (CreateUrlSigner()) in that case. It just makes it really clear - and it's unlikely to be significantly harder to use that way.

I'd argue that when it's a method, we should allow any exceptions to propagate instead of returning null, too. (Or maybe have a TryCreateUrlSigner method that returns null.)

Yes, I debated over this, settled on a property, but I wasn't convinced. I'm adding the method then, CreateUrlSigner and letting exceptions bobble up.

amanda-tarafa

All comments addressed on ordered commits prefixed review:.

Also, take a look at the Firestore change, or let me know if you'd prefer me to bring that out to another PR. According to googleapis/conformance-tests#84 you were fine with this, and it's simple enough though.

amanda-tarafa · 2024-03-05T20:54:37Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/UrlSigner.Options.cs

@@ -174,15 +195,31 @@ public sealed class Options
            /// <param name="urlStyle">The new url style.</param>
            /// <returns>A new set ofoptions with the given url style.</returns>
            public Options WithUrlStyle(UrlStyle urlStyle) =>
-                new Options(Duration, Expiration, SigningVersion, urlStyle, Scheme, null);
+                new Options(Duration, Expiration, SigningVersion, urlStyle, Scheme, Host, Port, null);


Ah, yes, done.

amanda-tarafa · 2024-03-05T21:01:27Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1.Tests/UrlSignerTest.OptionsTest.cs

@@ -240,6 +241,45 @@ public void WithPort_Null()
                Assert.NotSame(options, newOptions);
                Assert.Null(newOptions.Port);
            }
+
+            [Fact]
+            public void WithDefultOverrides_NullOverrideValues()


Oh, done, both, and default here and eerywhere.

amanda-tarafa · 2024-03-05T21:08:27Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/UrlSigner.DefaultOptionsOverrides.cs

+{
+    /// <summary>
+    /// Overrides for default values for some of the options in <see cref="Options"/>.
+    /// Useful for creating signers where signing options default to specific valued, for instance


amanda-tarafa · 2024-03-05T21:10:36Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/UrlSigner.Options.cs


            /// <summary>
            /// The host to use for generating the signed URL.
            /// This will never be null. If null is specified in <see cref="WithHost(string)"/>
            /// then <see cref="DefaultStorageHost"/> will be used.
            /// Will be ignored if <see cref="UrlStyle"/> is set to <see cref="UrlStyle.BucketBoundHostname"/>.
            /// </summary>
-            public string Host { get; }
+            public string Host => ExplicitHost ?? DefaultOptionsOverrides?.Host ?? DefaultStorageHost;


amanda-tarafa · 2024-03-05T21:11:03Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/UrlSigner.Options.cs

-                TimeSpan? duration, DateTimeOffset? expiration, SigningVersion version, UrlStyle? urlStyle, string scheme, string host, int? port, string bucketBoundHostname)
+            /// <summary>
+            /// Values to be used as default for some options in case they are not set,
+            /// instead of the usual default values.May be null.


amanda-tarafa · 2024-03-05T21:12:15Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/UrlSigner.cs

+        internal UrlSigner WithClock(IClock clock) => new UrlSigner(_blobSignerProvider, clock, _defaultOptionsOverrides);
+
+        /// <summary>
+        /// Returns a URL signer identical to this one, excepto for the default options overrides used for signing.


amanda-tarafa · 2024-03-05T21:17:09Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/StorageClient.cs

+        /// on compatible credentials.
+        /// </summary>
+        /// <remarks>
+        /// Because credentials used by this client may changed, this property will return a new <see cref="UrlSigner"/> instance


Yes, I debated over this, settled on a property, but I wasn't convinced. I'm adding the method then, CreateUrlSigner and letting exceptions bobble up.

jskeet

Only nits left :)

jskeet · 2024-03-06T10:40:07Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/UrlSigner.Options.cs

@@ -65,8 +65,7 @@ public sealed class Options
            /// <summary>
            /// The Scheme to use for the request. Only http or https supported.
            /// This will never be null. If null is specified in <see cref="WithScheme(string)"/>
-            /// then https will be used.
-            /// Defaults to https.
+            /// an appropiate default value will be used.


Nit (sorry) - appropiate => appropriate

jskeet · 2024-03-06T10:40:14Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/UrlSigner.Options.cs

@@ -79,7 +78,7 @@ public sealed class Options
            /// <summary>
            /// The host to use for generating the signed URL.
            /// This will never be null. If null is specified in <see cref="WithHost(string)"/>
-            /// then <see cref="DefaultStorageHost"/> will be used.
+            /// then the appropiate default value will be used.


jskeet · 2024-03-06T10:41:17Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/StorageClient.cs

@@ -167,6 +132,29 @@ public UrlSigner UrlSigner
                UnauthenticatedAccess = true
            }.Build();

+        /// <summary>
+        /// Creates a URL signer built base on this client, that is using the same credential as this client, as well


base => based (and maybe remove the "built"?)

is using => uses

jskeet · 2024-03-06T10:41:44Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/StorageClient.cs

@@ -167,6 +132,29 @@ public UrlSigner UrlSigner
                UnauthenticatedAccess = true
            }.Build();

+        /// <summary>
+        /// Creates a URL signer built base on this client, that is using the same credential as this client, as well
+        /// as defaulting to this client's URI scheme, host and port. If the credential used by this client is not compatible


Possibly just use <exception> instead of summary for the exception bit? (I don't mind either way.)

jskeet · 2024-03-06T10:42:13Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/StorageClient.cs

+        /// See <see cref="UrlSigner"/>'s documentation for more information on compatible credentials.
+        /// </summary>
+        /// <remarks>
+        /// Because credentials used by this client may changed, this method will always return a new <see cref="UrlSigner"/> instance


I think we can probably remove these remarks now - I'd expect a method called "Create" to return a new instance anyway.

jskeet · 2024-03-06T10:43:37Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/StorageClient.cs

+                // If the original URI didn't specify a port, we want signed URLs to not have a port either;
+                // but Uri.Port returns the default port for the URI scheme when the port was not specified on the
+                // original URI.
+                baseUri.IsDefaultPort && !Service.BaseUri.Contains(baseUri.Port.ToString()) ? null : baseUri.Port));


I'd personally move this logic (including the comment) into a separate local variable declaration of port, so we can then just use new UrlSigner.DefaultOptionsOverrides(baseUri.Scheme, baseUri.Host, port).

jskeet · 2024-03-06T11:08:34Z

Also, take a look at the Firestore change, or let me know if you'd prefer me to bring that out to another PR. According to googleapis/conformance-tests#84 you were fine with this, and it's simple enough though.

Yes, I'm fine doing that in this PR.

And regenerated conformance tests protos.

…for update.

This will allow creating signers from StorageClients.

The created UrlSigner will use the same credential and base URI as the StorageClient it is created from.

amanda-tarafa

All comments addressed. I've squahsed in meaningful comments and will merge on green. Thanks!

amanda-tarafa · 2024-03-06T19:21:08Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/UrlSigner.Options.cs

@@ -65,8 +65,7 @@ public sealed class Options
            /// <summary>
            /// The Scheme to use for the request. Only http or https supported.
            /// This will never be null. If null is specified in <see cref="WithScheme(string)"/>
-            /// then https will be used.
-            /// Defaults to https.
+            /// an appropiate default value will be used.


amanda-tarafa · 2024-03-06T19:21:15Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/UrlSigner.Options.cs

@@ -79,7 +78,7 @@ public sealed class Options
            /// <summary>
            /// The host to use for generating the signed URL.
            /// This will never be null. If null is specified in <see cref="WithHost(string)"/>
-            /// then <see cref="DefaultStorageHost"/> will be used.
+            /// then the appropiate default value will be used.


amanda-tarafa · 2024-03-06T19:24:27Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/StorageClient.cs

@@ -167,6 +132,29 @@ public UrlSigner UrlSigner
                UnauthenticatedAccess = true
            }.Build();

+        /// <summary>
+        /// Creates a URL signer built base on this client, that is using the same credential as this client, as well


amanda-tarafa · 2024-03-06T19:25:24Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/StorageClient.cs

@@ -167,6 +132,29 @@ public UrlSigner UrlSigner
                UnauthenticatedAccess = true
            }.Build();

+        /// <summary>
+        /// Creates a URL signer built base on this client, that is using the same credential as this client, as well
+        /// as defaulting to this client's URI scheme, host and port. If the credential used by this client is not compatible


amanda-tarafa · 2024-03-06T19:25:35Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/StorageClient.cs

+        /// See <see cref="UrlSigner"/>'s documentation for more information on compatible credentials.
+        /// </summary>
+        /// <remarks>
+        /// Because credentials used by this client may changed, this method will always return a new <see cref="UrlSigner"/> instance


amanda-tarafa · 2024-03-06T19:26:04Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/StorageClient.cs

+                // If the original URI didn't specify a port, we want signed URLs to not have a port either;
+                // but Uri.Port returns the default port for the URI scheme when the port was not specified on the
+                // original URI.
+                baseUri.IsDefaultPort && !Service.BaseUri.Contains(baseUri.Port.ToString()) ? null : baseUri.Port));


amanda-tarafa requested a review from jskeet March 5, 2024 00:33

amanda-tarafa requested review from a team as code owners March 5, 2024 00:33

amanda-tarafa force-pushed the storage-url-signer branch 2 times, most recently from 343a743 to 252d85d Compare March 5, 2024 06:09

jskeet reviewed Mar 5, 2024

View reviewed changes

amanda-tarafa force-pushed the storage-url-signer branch from 252d85d to 24db1df Compare March 5, 2024 21:37

amanda-tarafa commented Mar 5, 2024

View reviewed changes

jskeet approved these changes Mar 6, 2024

View reviewed changes

amanda-tarafa added 6 commits March 6, 2024 11:14

chore: Update conformance tests submodule.

5af82e7

And regenerated conformance tests protos.

feat: Throw if an must-exist precondition is explicitly set to false …

ae87137

…for update.

chore: Remove leftover target-specific code.

3976481

feat: Support custom hostname and port in signers.

2f10f72

feat: Support overriding some URLSigner.Options default values.

3c2ecfb

This will allow creating signers from StorageClients.

feat: Allows creating UrlSigner directly from a StorageClient.

fc00fa2

The created UrlSigner will use the same credential and base URI as the StorageClient it is created from.

amanda-tarafa force-pushed the storage-url-signer branch from 24db1df to fc00fa2 Compare March 6, 2024 20:09

amanda-tarafa commented Mar 6, 2024

View reviewed changes

amanda-tarafa merged commit 4f0d708 into googleapis:main Mar 6, 2024
10 checks passed

amanda-tarafa deleted the storage-url-signer branch March 6, 2024 20:28

feat: UrlSigner supports custom endpoints #12042

feat: UrlSigner supports custom endpoints #12042

Conversation

amanda-tarafa commented Mar 5, 2024 • edited Loading

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

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

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

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

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

jskeet commented Mar 6, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-tarafa commented Mar 5, 2024 •

edited

Loading