Skip to content
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

feat(storage): improve v4 signature query parameters encoding #48

Merged
merged 18 commits into from
Mar 7, 2020

Conversation

IlyaFaer
Copy link

@IlyaFaer IlyaFaer commented Feb 7, 2020

Closes #9

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 7, 2020
google/cloud/storage/_signing.py Show resolved Hide resolved
google/cloud/storage/_signing.py Show resolved Hide resolved
tests/unit/test__signing.py Show resolved Hide resolved
google/cloud/storage/_signing.py Show resolved Hide resolved
@IlyaFaer IlyaFaer marked this pull request as ready for review February 10, 2020 09:50
@IlyaFaer IlyaFaer added the type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. label Feb 10, 2020
@frankyn
Copy link
Member

frankyn commented Feb 11, 2020

@IlyaFaer this LGTM, but please update relevant conformance tests before merging this code to confirm that it's working as expected.

I updated conformance tests in https://github.com/googleapis/conformance-tests/tree/master/storage/v1

If you have questions PLMK. Specifically for this change you should rely on the following two conformance tests:

  • Query Parameter Encoding
  • Query Parameter Ordering

google/cloud/storage/_signing.py Show resolved Hide resolved
tests/unit/test__signing.py Show resolved Hide resolved
google/cloud/storage/_signing.py Outdated Show resolved Hide resolved
@tseaver
Copy link
Contributor

tseaver commented Feb 12, 2020

This PR also addresses part of the change for #12.

@tseaver
Copy link
Contributor

tseaver commented Feb 12, 2020

@frankyn We don't currently do any of the cross-language conformance tests inside python-storage. I just made #57 to track that.

@tseaver tseaver added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 12, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 12, 2020
@frankyn
Copy link
Member

frankyn commented Feb 13, 2020

@IlyaFaer please respond to @tseaver's comments before merging

google/cloud/storage/_signing.py Outdated Show resolved Hide resolved
google/cloud/storage/_signing.py Show resolved Hide resolved
@frankyn
Copy link
Member

frankyn commented Feb 20, 2020

Friendly ping and pending Kokoro passing.

@frankyn frankyn requested a review from tseaver February 26, 2020 15:23
@frankyn
Copy link
Member

frankyn commented Feb 26, 2020

Merging when green.

@frankyn
Copy link
Member

frankyn commented Feb 26, 2020

Pending approval from @tseaver as there is on-going discussion.

@frankyn
Copy link
Member

frankyn commented Mar 6, 2020

@tseaver friendly ping

@crwilcox crwilcox dismissed tseaver’s stale review March 6, 2020 23:35

Reviewed, attempted to remove coercion, but due to truthiness of bytes and strs in python 2 I am going to approve this.

@crwilcox crwilcox added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 6, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 6, 2020
@crwilcox crwilcox added the automerge Merge the pull request once unit tests and other checks pass. label Mar 6, 2020
@crwilcox crwilcox removed the request for review from tseaver March 7, 2020 00:21
@crwilcox crwilcox merged commit 8df0b55 into googleapis:master Mar 7, 2020
@IlyaFaer IlyaFaer deleted the v4_sign_query_params branch April 1, 2020 20:02
cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
…apis#48)

* feat(storage): improve v4 signature query parameters encoding

* add URL encoding test

* add query_parameters arg into conformance tests

* add tests for _quote_param() function

* declare test file encoding

* fix the param type

* add test with bytes

* Update _signing.py

Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
Co-authored-by: Christopher Wilcox <crwilcox@google.com>
cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
…apis#48)

* feat(storage): improve v4 signature query parameters encoding

* add URL encoding test

* add query_parameters arg into conformance tests

* add tests for _quote_param() function

* declare test file encoding

* fix the param type

* add test with bytes

* Update _signing.py

Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
Co-authored-by: Christopher Wilcox <crwilcox@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the pull request once unit tests and other checks pass. cla: yes This human has signed the Contributor License Agreement. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Storage] V4 Signature Supplying Additional Query Parameters With GCS Signed URLs
6 participants