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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 12 commits into from
Apr 25, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3204edc
fix(storage): anonymous credentials for private bucket
HemangChothani Apr 15, 2020
134066e
fix(storage): use API_BASE_URL for hostname
HemangChothani Apr 16, 2020
85729b7
Merge branch 'master' of https://github.com/googleapis/python-storage…
HemangChothani Apr 16, 2020
dbd74fa
fix(storage): nit
HemangChothani Apr 17, 2020
1d1ae94
Merge branch 'master' of https://github.com/googleapis/python-storage…
HemangChothani Apr 17, 2020
ec45809
fix(storage): fix system test
HemangChothani Apr 20, 2020
95f12c4
Merge branch 'master' into storage_issue_102
frankyn Apr 20, 2020
6cc573b
Merge branch 'master' into storage_issue_102
frankyn Apr 22, 2020
c293e2f
Merge branch 'master' of https://github.com/googleapis/python-storage…
HemangChothani Apr 23, 2020
0588395
Merge branch 'storage_issue_102' of https://github.com/q-logic/python…
HemangChothani Apr 23, 2020
6e8213b
fix(storage): revert changes
HemangChothani Apr 24, 2020
dc32e92
Merge branch 'master' into storage_issue_102
frankyn Apr 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 17 additions & 9 deletions google/cloud/storage/blob.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@
from google.cloud._helpers import _rfc3339_to_datetime
from google.cloud._helpers import _to_bytes
from google.cloud.exceptions import NotFound
from google.cloud.storage._helpers import _get_storage_host
from google.cloud.storage._helpers import _PropertyMixin
from google.cloud.storage._helpers import _scalar_property
from google.cloud.storage._helpers import _convert_to_timestamp
Expand All @@ -70,12 +69,11 @@
from google.cloud.storage.constants import REGIONAL_LEGACY_STORAGE_CLASS
from google.cloud.storage.constants import STANDARD_STORAGE_CLASS

_STORAGE_HOST = _get_storage_host()

_API_ACCESS_ENDPOINT = "https://storage.googleapis.com"
_DEFAULT_CONTENT_TYPE = u"application/octet-stream"
_DOWNLOAD_URL_TEMPLATE = _STORAGE_HOST + u"/download/storage/v1{path}?alt=media"
_BASE_UPLOAD_TEMPLATE = _STORAGE_HOST + u"/upload/storage/v1{bucket_path}/o?uploadType="
_DOWNLOAD_URL_TEMPLATE = u"{hostname}/download/storage/v1{path}?alt=media"
_BASE_UPLOAD_TEMPLATE = u"{hostname}/upload/storage/v1{bucket_path}/o?uploadType="
_MULTIPART_URL_TEMPLATE = _BASE_UPLOAD_TEMPLATE + u"multipart"
_RESUMABLE_URL_TEMPLATE = _BASE_UPLOAD_TEMPLATE + u"resumable"
# NOTE: "acl" is also writeable but we defer ACL management to
Expand Down Expand Up @@ -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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""Get the download URL for the current blob.

If the ``media_link`` has been loaded, it will be used, otherwise
the URL will be constructed from the current blob's path (and possibly
generation) to avoid a round trip.

:type client: :class:`~google.cloud.storage.client.Client`
:param client: The client to use.

:rtype: str
:returns: The download URL for the current blob.
"""
name_value_pairs = []
if self.media_link is None:
base_url = _DOWNLOAD_URL_TEMPLATE.format(path=self.path)
base_url = _DOWNLOAD_URL_TEMPLATE.format(
hostname=client._connection.API_BASE_URL, path=self.path
)
if self.generation is not None:
name_value_pairs.append(("generation", "{:d}".format(self.generation)))
else:
Expand Down Expand Up @@ -790,7 +793,8 @@ def download_to_file(

:raises: :class:`google.cloud.exceptions.NotFound`
"""
download_url = self._get_download_url()
client = self._require_client(client)
download_url = self._get_download_url(client)
headers = _get_encryption_headers(self._encryption_key)
headers["accept-encoding"] = "gzip"

Expand Down Expand Up @@ -1055,7 +1059,9 @@ def _do_multipart_upload(
info = self._get_upload_arguments(content_type)
headers, object_metadata, content_type = info

base_url = _MULTIPART_URL_TEMPLATE.format(bucket_path=self.bucket.path)
base_url = _MULTIPART_URL_TEMPLATE.format(
hostname=self.client._connection.API_BASE_URL, bucket_path=self.bucket.path
)
name_value_pairs = []

if self.user_project is not None:
Expand Down Expand Up @@ -1190,7 +1196,9 @@ def _initiate_resumable_upload(
if extra_headers is not None:
headers.update(extra_headers)

base_url = _RESUMABLE_URL_TEMPLATE.format(bucket_path=self.bucket.path)
base_url = _RESUMABLE_URL_TEMPLATE.format(
hostname=self.client._connection.API_BASE_URL, bucket_path=self.bucket.path
)
name_value_pairs = []

if self.user_project is not None:
Expand Down
41 changes: 30 additions & 11 deletions tests/unit/test_blob.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -751,7 +751,10 @@ def test__get_download_url_with_media_link(self):
# Set the media link on the blob
blob._properties["mediaLink"] = media_link

download_url = blob._get_download_url()
client = mock.Mock(_connection=_Connection)
client._connection.API_BASE_URL = "https://storage.googleapis.com"
download_url = blob._get_download_url(client)

self.assertEqual(download_url, media_link)

def test__get_download_url_with_media_link_w_user_project(self):
Expand All @@ -763,7 +766,10 @@ def test__get_download_url_with_media_link_w_user_project(self):
# Set the media link on the blob
blob._properties["mediaLink"] = media_link

download_url = blob._get_download_url()
client = mock.Mock(_connection=_Connection)
client._connection.API_BASE_URL = "https://storage.googleapis.com"
download_url = blob._get_download_url(client)

self.assertEqual(
download_url, "{}?userProject={}".format(media_link, user_project)
)
Expand All @@ -774,7 +780,9 @@ def test__get_download_url_on_the_fly(self):
blob = self._make_one(blob_name, bucket=bucket)

self.assertIsNone(blob.media_link)
download_url = blob._get_download_url()
client = mock.Mock(_connection=_Connection)
client._connection.API_BASE_URL = "https://storage.googleapis.com"
download_url = blob._get_download_url(client)
expected_url = (
"https://storage.googleapis.com/download/storage/v1/b/"
"buhkit/o/bzzz-fly.txt?alt=media"
Expand All @@ -790,7 +798,9 @@ def test__get_download_url_on_the_fly_with_generation(self):
blob._properties["generation"] = str(generation)

self.assertIsNone(blob.media_link)
download_url = blob._get_download_url()
client = mock.Mock(_connection=_Connection)
client._connection.API_BASE_URL = "https://storage.googleapis.com"
download_url = blob._get_download_url(client)
expected_url = (
"https://storage.googleapis.com/download/storage/v1/b/"
"fictional/o/pretend.txt?alt=media&generation=1493058489532987"
Expand All @@ -804,7 +814,9 @@ def test__get_download_url_on_the_fly_with_user_project(self):
blob = self._make_one(blob_name, bucket=bucket)

self.assertIsNone(blob.media_link)
download_url = blob._get_download_url()
client = mock.Mock(_connection=_Connection)
client._connection.API_BASE_URL = "https://storage.googleapis.com"
download_url = blob._get_download_url(client)
expected_url = (
"https://storage.googleapis.com/download/storage/v1/b/"
"fictional/o/pretend.txt?alt=media&userProject={}".format(user_project)
Expand All @@ -823,7 +835,9 @@ def test__get_download_url_on_the_fly_with_kms_key_name(self):
blob = self._make_one(blob_name, bucket=bucket, kms_key_name=kms_resource)

self.assertIsNone(blob.media_link)
download_url = blob._get_download_url()
client = mock.Mock(_connection=_Connection)
client._connection.API_BASE_URL = "https://storage.googleapis.com"
download_url = blob._get_download_url(client)
expected_url = (
"https://storage.googleapis.com/download/storage/v1/b/"
"buhkit/o/bzzz-fly.txt?alt=media"
Expand Down Expand Up @@ -1003,7 +1017,8 @@ def test_download_to_file_with_failure(self):

def test_download_to_file_wo_media_link(self):
blob_name = "blob-name"
client = mock.Mock(spec=[u"_http"])
client = mock.Mock(_connection=_Connection, spec=[u"_http"])
client._connection.API_BASE_URL = "https://storage.googleapis.com"
bucket = _Bucket(client)
blob = self._make_one(blob_name, bucket=bucket)
blob._do_download = mock.Mock()
Expand Down Expand Up @@ -1319,7 +1334,8 @@ def _do_multipart_success(
transport = self._mock_transport(http_client.OK, {})

# Create some mock arguments.
client = mock.Mock(_http=transport, spec=["_http"])
client = mock.Mock(_http=transport, _connection=_Connection, spec=["_http"])
client._connection.API_BASE_URL = "https://storage.googleapis.com"
data = b"data here hear hier"
stream = io.BytesIO(data)
content_type = u"application/xml"
Expand Down Expand Up @@ -1485,7 +1501,8 @@ def _initiate_resumable_helper(
transport = self._mock_transport(http_client.OK, response_headers)

# Create some mock arguments and call the method under test.
client = mock.Mock(_http=transport, spec=[u"_http"])
client = mock.Mock(_http=transport, _connection=_Connection, spec=[u"_http"])
client._connection.API_BASE_URL = "https://storage.googleapis.com"
data = b"hello hallo halo hi-low"
stream = io.BytesIO(data)
content_type = u"text/plain"
Expand Down Expand Up @@ -1772,7 +1789,8 @@ def _do_resumable_helper(
)

# Create some mock arguments and call the method under test.
client = mock.Mock(_http=transport, spec=["_http"])
client = mock.Mock(_http=transport, _connection=_Connection, spec=["_http"])
client._connection.API_BASE_URL = "https://storage.googleapis.com"
stream = io.BytesIO(data)
content_type = u"text/html"
response = blob._do_resumable_upload(
Expand Down Expand Up @@ -2114,7 +2132,8 @@ def _create_resumable_upload_session_helper(self, origin=None, side_effect=None)
# Create some mock arguments and call the method under test.
content_type = u"text/plain"
size = 10000
client = mock.Mock(_http=transport, spec=[u"_http"])
client = mock.Mock(_http=transport, _connection=_Connection, spec=[u"_http"])
client._connection.API_BASE_URL = "https://storage.googleapis.com"
new_url = blob.create_resumable_upload_session(
content_type=content_type, size=size, origin=origin, client=client
)
Expand Down
8 changes: 3 additions & 5 deletions tests/unit/test_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,18 +137,16 @@ def test_ctor_w_empty_client_options(self):
)

def test_ctor_w_client_options_dict(self):

PROJECT = "PROJECT"
credentials = _make_credentials()
client_options = {"api_endpoint": "https://www.foo-googleapis.com"}
api_endpoint = "https://www.foo-googleapis.com"
client_options = {"api_endpoint": api_endpoint}

client = self._make_one(
project=PROJECT, credentials=credentials, client_options=client_options
)

self.assertEqual(
client._connection.API_BASE_URL, "https://www.foo-googleapis.com"
)
self.assertEqual(client._connection.API_BASE_URL, api_endpoint)

def test_ctor_w_client_options_object(self):
from google.api_core.client_options import ClientOptions
Expand Down