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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: allow signed url version v4 without signed credentials #356

Merged
merged 9 commits into from Feb 19, 2021

Conversation

@guillaumeblaquiere
Copy link
Contributor

@guillaumeblaquiere guillaumeblaquiere commented Jan 4, 2021

Fixes #355 馃

I used the service_account_email passed in method parameter as "signer" instead of taking it in the credentials.signer_email method. That allows to not use google.auth.credentials.Signing type as credential parameter and to use the Service Account credential API instead.

I preserved the checks in case of access_token or service_account_email set to nil, to preserve the efficiency and the existing behavior.


I also updated tests. Successful with python 3.8.

Open to comment and to improve this PR if needed

鈥 checks for efficiency in case of neither access_token nor service_account_email are provided.

Fix: tests v4 with token to take into account not Signing credential class.
@guillaumeblaquiere guillaumeblaquiere requested a review from Jan 4, 2021
@google-cla google-cla bot added the cla: yes label Jan 4, 2021
@guillaumeblaquiere guillaumeblaquiere changed the title Fix #355: allow signed url version v4 without service account email fix: allow signed url version v4 without service account email Jan 4, 2021
google/cloud/storage/_signing.py Show resolved Hide resolved
tests/unit/test__signing.py Outdated Show resolved Hide resolved
@tseaver tseaver changed the title fix: allow signed url version v4 without service account email fix: allow signed url version v4 without signed credentials Jan 20, 2021
@cajoek
Copy link

@cajoek cajoek commented Jan 21, 2021

I really want this!

@guillaumeblaquiere
Copy link
Contributor Author

@guillaumeblaquiere guillaumeblaquiere commented Jan 28, 2021

Sorry for the delay, the week was awful!

@guillaumeblaquiere
Copy link
Contributor Author

@guillaumeblaquiere guillaumeblaquiere commented Feb 9, 2021

@tseaver any review possible on this?

@frankyn frankyn requested a review from as a code owner Feb 18, 2021
Copy link
Member

@frankyn frankyn left a comment

Thanks @guillaumeblaquiere LGTM

@frankyn frankyn dismissed tseaver鈥檚 stale review Feb 19, 2021

Feedback has been addressed

@frankyn frankyn merged commit 3e69bf9 into googleapis:master Feb 19, 2021
4 checks passed
cojenco added a commit to cojenco/python-storage that referenced this issue Oct 13, 2021
鈥s#356)

* Fix: signed_url_v4 to accept credentials without private key. Preserve checks for efficiency in case of neither access_token nor service_account_email are provided.
Fix: tests v4 with token to take into account not Signing credential class.

* fix typo: 2 new lines before new test class

* fix doc: Improve docstring to explain the use of the access_token AND the service_account_email, or the signer email.

* fix: test coverage with the new IF branch

* lint

Co-authored-by: Tres Seaver <tseaver@palladion.com>
Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
Co-authored-by: Frank Natividad <franknatividad@google.com>
cojenco added a commit to cojenco/python-storage that referenced this issue Oct 13, 2021
鈥s#356)

* Fix: signed_url_v4 to accept credentials without private key. Preserve checks for efficiency in case of neither access_token nor service_account_email are provided.
Fix: tests v4 with token to take into account not Signing credential class.

* fix typo: 2 new lines before new test class

* fix doc: Improve docstring to explain the use of the access_token AND the service_account_email, or the signer email.

* fix: test coverage with the new IF branch

* lint

Co-authored-by: Tres Seaver <tseaver@palladion.com>
Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
Co-authored-by: Frank Natividad <franknatividad@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants