[Storage] Signed URLs v4 #1742

jdpedrie · 2019-03-21T15:20:54Z

This is still a work in progress, but it's progressed far enough to start getting some additional eyes on it.

The signing logic is going to move to the Auth library. A change for that will be forthcoming.

cc @frankyn

Closes #1672.

frankyn

Apologies for the delay, here's a first pass review.

Please update signed-url-v4-testdata.json as it was recently updated.

Storage/src/StorageObject.php

Storage/tests/System/SignedUrlTest.php

frankyn

Implementation LGTM, pending tests passing with conformance tests. I added small nits in this request for changes.

Thanks @jdpedrie.

Storage/src/StorageObject.php

frankyn · 2019-03-29T17:26:05Z

Storage/src/SigningHelper.php

+        //@codeCoverageIgnoreStart
+        if ($version === null && self::DEFAULT_URL_SIGNING_VERSION === 'v2') {
+            // raise a little notice to poke users towards v4.
+            @trigger_error(


Will this error out an application or only log a warning?

it will only log a notice.

Core/src/AnonymousCredentials.php

frankyn

Pending failed tests and auth PR the implementation LGTM.

Storage/src/Bucket.php

frankyn · 2019-04-03T21:58:37Z

Storage/src/Bucket.php

+     *     @type array $queryParams Additional query parameters to be included
+     *           as part of the signed URL query string. For allowed values,
+     *           see [Reference Headers](https://cloud.google.com/storage/docs/xml-api/reference-headers#query).
+     *     @type string $version One of "v2" or "v4". In the future, "v4" will


Remove mention of "v2 -> v4 as default" here as well.

frankyn

You can move forward with language reviews if necessary. LGTM, thank you for your patience @jdpedrie

codecov · 2019-04-15T19:43:15Z

Codecov Report

❗ No coverage uploaded for pull request base (master@afc9ee3). Click here to learn what that means.
The diff coverage is 96.04%.

@@            Coverage Diff            @@
##             master    #1742   +/-   ##
=========================================
  Coverage          ?   91.58%           
  Complexity        ?     4403           
=========================================
  Files             ?      305           
  Lines             ?    13089           
  Branches          ?        0           
=========================================
  Hits              ?    11987           
  Misses            ?     1102           
  Partials          ?        0

Impacted Files	Coverage Δ	Complexity Δ
Storage/src/EncryptionTrait.php	`100% <ø> (ø)`	`16 <0> (?)`
Storage/src/Connection/Rest.php	`100% <ø> (ø)`	`41 <0> (?)`
Core/src/RequestWrapperTrait.php	`93.93% <0%> (ø)`	`11 <1> (?)`
Storage/src/Bucket.php	`97.18% <100%> (ø)`	`70 <2> (?)`
Core/src/JsonTrait.php	`100% <100%> (ø)`	`4 <4> (?)`
Storage/src/StorageObject.php	`100% <100%> (ø)`	`38 <0> (?)`
Storage/src/SigningHelper.php	`96.42% <96.42%> (ø)`	`54 <54> (?)`

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 afc9ee3...c426754. Read the comment docs.

dwsupplee

Still finishing up, just had some thoughts that seemed worthwhile to get out in case it is anything we could continue on in parallel.

Storage/src/Bucket.php

Storage/src/SigningHelper.php

Storage/src/EncryptionTrait.php

Storage/src/Bucket.php

Storage/src/EncryptionTrait.php

dwsupplee · 2019-04-15T19:47:10Z

Storage/src/SigningHelper.php

+            }
+
+            $options['keyFile'] = json_decode(file_get_contents($keyFilePath), true);
+            if (json_last_error() !== JSON_ERROR_NONE) {


We could use the JsonTrait here

Sure thing. Please note that this required making the methods in JsonTrait static.

Storage/src/SigningHelper.php

dwsupplee · 2019-04-15T19:58:41Z

Storage/src/SigningHelper.php

+     *
+     * @return SigningHelper
+     */
+    public static function getHelper()


Do we need an instance at all? It looks like we should be able to make v2sign and v4sign static. Just being able to call SigngingHelper::v2Sign seems friendlier than having to work with this getHelper method.

Storage/src/SigningHelper.php

dwsupplee

Really nice work abstracting this out 👍

Storage/src/SigningHelper.php

Storage/src/StorageObject.php

Storage/src/SigningHelper.php

Core/src/AnonymousCredentials.php

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 21, 2019

jdpedrie added the api: storage Issues related to the Cloud Storage API. label Mar 21, 2019

jdpedrie changed the title ~~Storage signed url v4~~ [Storage] Signed URLs v4 Mar 21, 2019

frankyn suggested changes Mar 27, 2019

View reviewed changes

Storage/src/StorageObject.php Show resolved Hide resolved

Storage/src/StorageObject.php Outdated Show resolved Hide resolved

Storage/src/StorageObject.php Show resolved Hide resolved

Storage/tests/System/SignedUrlTest.php Show resolved Hide resolved

yoshi-automation added the 🚨 This issue needs some love. label Mar 28, 2019

jdpedrie added needs work This is a pull request that needs a little love. and removed 🚨 This issue needs some love. labels Mar 29, 2019

frankyn suggested changes Mar 29, 2019

View reviewed changes

Storage/src/StorageObject.php Show resolved Hide resolved

Storage/src/StorageObject.php Outdated Show resolved Hide resolved

Storage/src/StorageObject.php Outdated Show resolved Hide resolved

frankyn reviewed Mar 29, 2019

View reviewed changes

Core/src/AnonymousCredentials.php Outdated Show resolved Hide resolved

frankyn approved these changes Mar 29, 2019

View reviewed changes

frankyn reviewed Mar 29, 2019

View reviewed changes

Storage/src/Bucket.php Show resolved Hide resolved

frankyn reviewed Apr 3, 2019

View reviewed changes

frankyn approved these changes Apr 4, 2019

View reviewed changes

jdpedrie mentioned this pull request Apr 12, 2019

GCS signedUrl fails with GAE default service account #1672

Closed

jdpedrie marked this pull request as ready for review April 15, 2019 19:27

jdpedrie requested a review from dwsupplee as a code owner April 15, 2019 19:27

jdpedrie added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 15, 2019

jdpedrie added 12 commits April 15, 2019 15:27

Use pull request title for autosynth PRs

a0a34bc

wip

5a7d854

WIP

ba5ed75

WIP

01bf9dc

WIP

5ba1bf2

WIP

8b2695d

Fix v2 signing

44f8878

WIP

1ab1a02

Fix generation query param

97a2116

Update v2 and header constraints

7727386

Add unit test coverage for signed urls

84ec087

Fix path normalization with custom cname

8b529fa

jdpedrie added 11 commits April 15, 2019 15:28

Conform to test specification

85a8c45

Remove unused property

37faf15

Update tests for auth changes

a863f7b

Add Bucket Signed URL and address review comments

9060627

fixes

0c8ef66

Add tests and save helper instance

b089f16

Remove v2 warning

00f3b7d

Remove v2 warning from docs, enable bucket test

cdf4f5a

Fix code style

6385aa5

Update auth dependency, fix snippet tests

58d27a1

Require auth ^1.5 everywhere

0614e79

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 15, 2019

dwsupplee suggested changes Apr 15, 2019

View reviewed changes

dwsupplee reviewed Apr 15, 2019

View reviewed changes

jdpedrie added 3 commits April 16, 2019 09:41

Address review pt 1

7378337

Fix tests

96dd0f2

Address review pt 2

85ba5b3

dwsupplee reviewed Apr 16, 2019

View reviewed changes

Core/src/AnonymousCredentials.php Outdated Show resolved Hide resolved

Bump to patched auth version

c426754

jdpedrie removed the needs work This is a pull request that needs a little love. label Apr 16, 2019

dwsupplee approved these changes Apr 16, 2019

View reviewed changes

yoshi-automation added the 🚨 This issue needs some love. label Apr 16, 2019

jdpedrie added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 16, 2019

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 16, 2019

jdpedrie merged commit 9061f97 into googleapis:master Apr 16, 2019

jdpedrie deleted the storage-signed-url-v4 branch April 16, 2019 19:36

jdpedrie mentioned this pull request Apr 16, 2019

Prepare v0.99.0 #1823

Merged

[Storage] Signed URLs v4 #1742

[Storage] Signed URLs v4 #1742

Uh oh!

Conversation

jdpedrie commented Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frankyn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

frankyn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

frankyn Mar 29, 2019

Choose a reason for hiding this comment

Uh oh!

jdpedrie Mar 29, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

frankyn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

frankyn Apr 3, 2019

Choose a reason for hiding this comment

Uh oh!

frankyn left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dwsupplee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dwsupplee Apr 15, 2019

Choose a reason for hiding this comment

Uh oh!

jdpedrie Apr 16, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dwsupplee Apr 15, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dwsupplee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

jdpedrie commented Mar 21, 2019 •

edited

Loading

codecov bot commented Apr 15, 2019 •

edited

Loading