fix(storage): anonymous credentials for private bucket

[HemangChothani]

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]

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]

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).

[HemangChothani]

@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]

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

[frankyn]

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

[HemangChothani]

_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.

[frankyn]

1 more question. Thanks for fixing this @HemangChothani!

[frankyn]

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

[frankyn]

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

[HemangChothani]

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

[frankyn]

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.

[frankyn]

@HemangChothani pending tests passing, but LGTM

[frankyn]

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