-
Notifications
You must be signed in to change notification settings - Fork 545
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Storage v4 POST Policy #4866
Storage v4 POST Policy #4866
Conversation
@frankyn Currently failing on the signature (test run currently limited to just the first conformance test.) Do you see anything missing in the signature calculation?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @quartzmo! Mostly have nits.
google-cloud-storage/lib/google/cloud/storage/file/signer_v4.rb
Outdated
Show resolved
Hide resolved
a5fe591
to
ed98c7e
Compare
ff416e9
to
88ba5c8
Compare
88ba5c8
to
c891e4e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks much better from a usability stand-point.
One nit on documentation, and the PR is still missing integration tests which will be unblocked ~EOW.
Thanks for your patience @quartzmo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @quartzmo, LGTM, now it's pending integration tests and surface checkoff.
LGTM by comment until that's complete.
91b9cc8
to
610390c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two nits, I'm waiting on confirmation for consistent naming across languages.
google-cloud-storage/acceptance/storage/bucket_post_object_v4_test.rb
Outdated
Show resolved
Hide resolved
google-cloud-storage/acceptance/storage/bucket_post_object_v4_test.rb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got confirmation on method name for v4.
# post.fields["x-goog-date"] #=> "20200128T000000Z" | ||
# post.fields["x-goog-signature"] #=> "4893a0e...cd82" | ||
# | ||
def post_object_v4 path, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using consistent naming across languages, please use: generate_signed_post_policy_v4
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do this after I get a passing conformance test for character escaping.
@frankyn Here is the current test failure and debug output. The actual
|
Thanks @quartzmo, one issue I see is that expectedDecodedPolicy is not consistent with the PR: googleapis/conformance-tests#27. The expectedDecodedPolicy and encodedPolicy are the following:
|
Output from
|
google-cloud-storage/test/google/cloud/storage/file/signer_v4/escape_characters_test.rb
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @quartzmo, thank you!
Confirming escape table, LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
a024990
to
41f5772
Compare
Acceptance tests are passing locally with my development project.