fix(storage): anonymous credentials for private bucket #107

HemangChothani · 2020-04-15T14:47:52Z

Fixes #102

We can override the private variables of blob class related to api_endpoint, upload_uri, download_uri.
Still confusion with these two parameters:
1. blob._API_ACCESS_ENDPOINT
2. bucket._API_ACCESS_ENDPOINT

frankyn

Thanks @HemangChothani left some comments.

frankyn · 2020-04-16T00:25:01Z

google/cloud/storage/client.py

@@ -122,6 +124,7 @@ def __init__(
            if client_options.api_endpoint:
                api_endpoint = client_options.api_endpoint
                kw_args["api_endpoint"] = api_endpoint
+                _override_private_url_variables(api_endpoint)


Thanks for putting this together. I think the code needs to be refactored for maintainability but the this is a good start.

The URLs in URL Templates could be refactored to be pulled in from the client which is passed around the library.

For example:

python-storage/google/cloud/storage/blob.py

Line 1020 in a50cdd1

base_url = _MULTIPART_URL_TEMPLATE.format(bucket_path=self.bucket.path)

Could be updated to:

_BASE_UPLOAD_TEMPLATE = u"{hostname}/upload/storage/v1{bucket_path}/o?uploadType=" _MULTIPART_URL_TEMPLATE = _BASE_UPLOAD_TEMPLATE + u"multipart" # truncated.... base_url = _MULTIPART_URL_TEMPLATE.format(hostname=storage_client._connection.API_BASE_URL,bucket_path=self.bucket.path)

WDYT?

frankyn · 2020-04-16T00:27:33Z

google/cloud/storage/client.py

+    blob._API_ACCESS_ENDPOINT = api_endpoint
+    bucket._API_ACCESS_ENDPOINT = api_endpoint


These URLs are used for Signature based requests like Signed URLs. These can be left alone because the method has a way to update the hostname because they don't use the same API (Download and Upload use JSON while Signature requests use XML API).

…into storage_issue_102

HemangChothani · 2020-04-16T14:26:27Z

@frankyn still we can refactor, need your suggestion.

We can remove _get_transport_method() from blob class as we are calling _require_client() method just above _get_transport_method() call, so no need of that method and we can also directly pass client._http.

OR

We can modify return type of _get_transport_method method, as of now it returns transport object but also need to return client object as well, so no need to call _require_client() in the respective method where we are using *_URL_TEMPLATE global variables.

frankyn

Let's keep it the way you have it with one minor nit. This looks much better than before.

Thanks @HemangChothani

frankyn · 2020-04-16T20:49:30Z

google/cloud/storage/blob.py

@@ -650,19 +648,24 @@ def _get_transport(self, client):
        client = self._require_client(client)
        return client._http

-    def _get_download_url(self):
+    def _get_download_url(self, client):


Instead of this could you use client() that's already available here: https://github.com/googleapis/python-storage/pull/107/files#diff-5d3277a5f0f072a447a1eb89f9fa1ae0R273-R275

this was a bad call, you'll need to do this along with _require_client() otherwise you can end up in a bad state.

_require_client() has been call by _get_transport method so moving _get_download_url method call after that.

One system test has failed so fixed that.

…into storage_issue_102

frankyn

1 more question. Thanks for fixing this @HemangChothani!

frankyn · 2020-04-22T18:32:10Z

google/cloud/storage/client.py

@@ -530,7 +530,7 @@ def download_blob_to_file(self, blob_or_uri, file_obj, start=None, end=None):
        try:
            blob_or_uri.download_to_file(file_obj, client=self, start=start, end=end)
        except AttributeError:
-            blob = Blob.from_string(blob_or_uri)
+            blob = Blob.from_string(blob_or_uri, client=self)


was this an accidental miss in implementation? Seems like it but want to confirm.

Looking at it again, client is passed below to download_to_file(). The change doesn't look necessary.

Yes, but the problem arise here , if i didn't pass the client so bucket class has received None

python-storage/google/cloud/storage/blob.py

Line 351 in 18b1e07

bucket = Bucket(client, name=netloc)

so when call _require_client() method it find the client object which passed in download_to_file() so hostname=self.client._connection.API_BASE_URL line hit the error as self.client found None

Gotcha, when I raised that you should revert my last comment, I was referring to updating the method _get_download_url() to accept client parameter because even though _get_transport calls _require_client() it doesn't update self.client with a value if it's None.

I think you may want to pass client returned from _require_client() to _get_download_url().

Thank you for your patience.

…into storage_issue_102

…-storage into storage_issue_102

frankyn

@HemangChothani pending tests passing, but LGTM

frankyn · 2020-04-24T15:42:02Z

@HemangChothani asked @crwilcox to take another pass to confirm.

* fix(storage): anonymous credentials for private bucket * fix(storage): use API_BASE_URL for hostname * fix(storage): nit * fix(storage): fix system test * fix(storage): revert changes Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>

fix(storage): anonymous credentials for private bucket

3204edc

HemangChothani requested a review from frankyn April 15, 2020 14:47

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 15, 2020

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

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

frankyn requested changes Apr 16, 2020

View reviewed changes

HemangChothani added 2 commits April 16, 2020 19:09

fix(storage): use API_BASE_URL for hostname

134066e

Merge branch 'master' of https://github.com/googleapis/python-storage …

85729b7

…into storage_issue_102

frankyn requested changes Apr 17, 2020

View reviewed changes

HemangChothani added 2 commits April 17, 2020 17:22

fix(storage): nit

dbd74fa

Merge branch 'master' of https://github.com/googleapis/python-storage …

1d1ae94

…into storage_issue_102

HemangChothani marked this pull request as ready for review April 17, 2020 11:56

HemangChothani and others added 2 commits April 20, 2020 15:43

fix(storage): fix system test

ec45809

Merge branch 'master' into storage_issue_102

95f12c4

HemangChothani added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 21, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 21, 2020

Merge branch 'master' into storage_issue_102

6cc573b

frankyn requested changes Apr 22, 2020

View reviewed changes

HemangChothani added 3 commits April 23, 2020 13:18

Merge branch 'master' of https://github.com/googleapis/python-storage …

c293e2f

…into storage_issue_102

Merge branch 'storage_issue_102' of https://github.com/q-logic/python…

0588395

…-storage into storage_issue_102

fix(storage): revert changes

6e8213b

frankyn approved these changes Apr 24, 2020

View reviewed changes

frankyn requested a review from crwilcox April 24, 2020 15:41

Merge branch 'master' into storage_issue_102

dc32e92

crwilcox approved these changes Apr 25, 2020

View reviewed changes

frankyn merged commit 6152ab4 into googleapis:master Apr 25, 2020

release-please bot mentioned this pull request Jan 12, 2022

chore(main): release 3.0.0 #688

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(storage): anonymous credentials for private bucket #107

fix(storage): anonymous credentials for private bucket #107

HemangChothani commented Apr 15, 2020

frankyn left a comment

frankyn Apr 16, 2020

frankyn Apr 16, 2020

HemangChothani commented Apr 16, 2020 •

edited by frankyn

Loading

frankyn left a comment

frankyn Apr 16, 2020

frankyn Apr 17, 2020

HemangChothani Apr 20, 2020

frankyn left a comment

frankyn Apr 22, 2020

frankyn Apr 22, 2020

HemangChothani Apr 23, 2020

frankyn Apr 24, 2020 •

edited

Loading

frankyn left a comment

frankyn commented Apr 24, 2020

		blob._API_ACCESS_ENDPOINT = api_endpoint
		bucket._API_ACCESS_ENDPOINT = api_endpoint

fix(storage): anonymous credentials for private bucket #107

fix(storage): anonymous credentials for private bucket #107

Conversation

HemangChothani commented Apr 15, 2020

frankyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HemangChothani commented Apr 16, 2020 • edited by frankyn Loading

frankyn 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

frankyn 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

frankyn Apr 24, 2020 • edited Loading

Choose a reason for hiding this comment

frankyn left a comment

Choose a reason for hiding this comment

frankyn commented Apr 24, 2020

HemangChothani commented Apr 16, 2020 •

edited by frankyn

Loading

frankyn Apr 24, 2020 •

edited

Loading