Skip to content

Enable anonymous access to blobs in public buckets #4315

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
Merged
tseaver merged 2 commits into from
Nov 6, 2017
Merged

Enable anonymous access to blobs in public buckets #4315

tseaver merged 2 commits into from
Nov 6, 2017

Conversation

@tseaver
Copy link
Contributor

@tseaver tseaver commented Nov 1, 2017 •
edited
Loading

/cc @jskeet

Closes #4279

@tseaver tseaver added api: storage Issues related to the Cloud Storage API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Nov 1, 2017
@tseaver tseaver requested a review from dhermes November 1, 2017 19:04
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 1, 2017
dhermes
dhermes reviewed Nov 1, 2017
View reviewed changes
Copy link
Contributor

@dhermes dhermes left a comment

Choose a reason for hiding this comment

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

@lukesneeringer @jskeet Do we expect list_blobs() to be dependable on the gcp-public-data-landsat? (i.e. could we add a system test for that).

@tseaver Don't you need more tests for methods that do and should fail?

storage/setup.py
'google-cloud-core >= 0.28.0, < 0.29dev',
'google-api-core >= 0.1.1, < 0.2.0dev',
'google-auth >= 1.0.0',
'google-auth >= 1.2.0',

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

storage/google/cloud/storage/client.py
:rtype: :class:`google.cloud.storage.client.Client`
:returns: Instance w/ anonymous credentials and no project.
"""
client = cls(project='<none>', credentials=AnonymousCredentials())

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

storage/tests/system.py Outdated
anonymous = storage.Client.create_anonymous_client()
bucket = anonymous.bucket(self.PUBLIC_BUCKET)
for blob in bucket.list_blobs():
print("Downloading blob: {}".format(blob.name))

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

storage/tests/system.py Outdated
print("Downloading blob: {}".format(blob.name))
with tempfile.TemporaryFile() as stream:
blob.download_to_file(stream)
break # just use the first one

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

storage/tests/system.py Outdated
for blob in bucket.list_blobs():
print("Downloading blob: {}".format(blob.name))
with tempfile.TemporaryFile() as stream:
blob.download_to_file(stream)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@tseaver
Copy link
Contributor Author

tseaver commented Nov 1, 2017

@dhermes

Don't you need more tests for methods that do and should fail?

The back-end enforces the policy by raising 401s: why bother testing them?

@dhermes
Copy link
Contributor

dhermes commented Nov 1, 2017

The back-end enforces the policy by raising 401s: why bother testing them?

I sympathize with the sentiment, but am just weary of the cases we haven't thought about.

dhermes
dhermes reviewed Nov 1, 2017
View reviewed changes
storage/tests/system.py Outdated
with tempfile.TemporaryFile() as stream:
blob.download_to_file(stream)
break # just use the first one
blob, = list(bucket.list_blobs(max_results=1))

This comment was marked as spam.

Sign in to view

theacodes
theacodes approved these changes Nov 1, 2017
View reviewed changes
Copy link
Contributor

@theacodes theacodes left a comment

Choose a reason for hiding this comment

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

Mostly LGTM

storage/google/cloud/storage/client.py
:rtype: :class:`google.cloud.storage.client.Client`
:returns: Instance w/ anonymous credentials and no project.
"""
client = cls(project='<none>', credentials=AnonymousCredentials())

This comment was marked as spam.

Sign in to view

storage/google/cloud/storage/client.py Outdated

@classmethod
def create_anonymous_client(cls):
"""Factory: return client w/ anonymous credentials.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

storage/tests/unit/test_client.py Outdated
self.assertIsInstance(client._connection, Connection)
self.assertIsInstance(
client._connection.credentials, AnonymousCredentials)
self.assertIsNone(client.current_batch)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@lukesneeringer
Copy link
Contributor

lukesneeringer commented Nov 6, 2017

@dhermes Apparently...

ValueError: invalid version number '0.28.1.dev1'
Command exited with code 1

...but only on Windows.

@tseaver
Copy link
Contributor Author

tseaver commented Nov 6, 2017

@dhermes, @lukesneeringer That spelling is correct according to PEP 440. We could try dev0 (we won't need more than one) or dev (an allowed abbreviation).

@tseaver tseaver force-pushed the 4279-storage-create_anonymous_client branch from ed35ccc to 9c685f1 Compare November 6, 2017 16:09
@tseaver
Copy link
Contributor Author

tseaver commented Nov 6, 2017

Rebased / semi-squashed to fix Appveyor, tidy up.

@dhermes
Copy link
Contributor

dhermes commented Nov 6, 2017

The AppVeyor issue was fixed in #4313 (I just removed bdist_msi from our build since we never used the artifacts). @jonparrott and I discussed, there seems to be a bug in bdist_msi, it uses StrictVersion.

@tseaver
Copy link
Contributor Author

tseaver commented Nov 6, 2017

@dhermes Yup, that was why I rebased on current master.

@tseaver
Copy link
Contributor Author

tseaver commented Nov 6, 2017

AFAIK, this PR is ready to merge once CI is green.

@tseaver tseaver merged commit 1ed56fd into master Nov 6, 2017
@tseaver tseaver deleted the 4279-storage-create_anonymous_client branch November 6, 2017 18:05
chemelnucfin pushed a commit to chemelnucfin/google-cloud-python that referenced this pull request Nov 6, 2017
@tseaver @chemelnucfin
Enable anonymous access to blobs in public buckets (googleapis#4315)
3377419
chemelnucfin pushed a commit to chemelnucfin/google-cloud-python that referenced this pull request Nov 6, 2017
@tseaver @chemelnucfin
Enable anonymous access to blobs in public buckets (googleapis#4315)
8b5a48b
@tseaver tseaver mentioned this pull request Jan 10, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lukesneeringer lukesneeringer Awaiting requested review from lukesneeringer

2 more reviewers

@theacodes theacodes theacodes approved these changes

@dhermes dhermes dhermes left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: storage Issues related to the Cloud Storage API. 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@tseaver @dhermes @lukesneeringer @theacodes @googlebot