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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding scope to credentials in Connection constructors. #840

Merged
merged 1 commit into from
May 14, 2015

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Apr 19, 2015

This also obsoletes get_scoped_connection() so it is removed.

FYI @jgeewax thanks for nudging in this direction. It moves the scope adding behavior into the Connection constructors.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 19, 2015
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 0cb6fc9 on dhermes:create-scoped-help into 286207b on GoogleCloudPlatform:master.

@tseaver
Copy link
Contributor

tseaver commented Apr 23, 2015

I might be misremembering, but it feels like we've been around a complete circle here.

@dhermes
Copy link
Contributor Author

dhermes commented Apr 23, 2015

What's the circle? I don't recall the Connection constructors adding scope, but maybe it happened? AFAIK I am the only "resident expert" when it comes to oauth2client.

@tseaver
Copy link
Contributor

tseaver commented Apr 23, 2015

6ebc65e moved the scope handling from gcloud.datastore._implicit_environ.get_connection and gcloud.storage._implicit_environ.get_connection up to gcloud._helpers.get_scoped_connection
5588987 moved the function from gcloud._helpers to gcloud.connection.
Now we are pushing the scope handling back down into gcloud.datastore.connection and gcloud.storage.connection.

@dhermes
Copy link
Contributor Author

dhermes commented Apr 23, 2015

The idea behind this commit is not just churn / moving the code.

We want people to be able to get credentials from elsewhere and pass them in to a connection without worrying about adding the scopes.

If we never expect people to construct a Connection themselves, then this PR is totally moot, but we'd still need to add credentials to the get_connection() methods to allow people a BYOC (bring your own credentials).


This was all brought about by @jgeewax pointing out some alternate auth / credentials flows.

@dhermes
Copy link
Contributor Author

dhermes commented Apr 27, 2015

@tseaver Can we keep moving on this?

@@ -94,6 +93,25 @@ def http(self):
self._http = self._credentials.authorize(self._http)
return self._http

@staticmethod
def _scoped_credentials(credentials, scope):

This comment was marked as spam.

This comment was marked as spam.

@jgeewax
Copy link
Contributor

jgeewax commented Apr 29, 2015

If I recall correctly, the difference this makes becomes apparent when you look at the case where you want to use some credentials from a specific file to talk to a service.

Old way:

from gcloud.credentials import get_for_service_account_json
from gcloud import pubsub

credentials = get_for_service_account_json('/path/to/key.json')
credentials = credentials.create_scoped(pubsub.SCOPE)  # <-- The thing I don't want to have to type...
connection = pubsub.Connection(credentials=credentials)
pubsub.set_default_connection(connection)
pubsub.Topic('topic_name').create()

(Also note that in gcloud.datastore SCOPE was inside _implicit_environ which made the code much less readable....)

New way

from gcloud.credentials import get_for_service_account_json
from gcloud import pubsub

credentials = get_for_service_account_json('/path/to/key.json')
connection = pubsub.Connection(credentials=credentials)  # Scoping happens here, where we know we need it...
pubsub.set_default_connection(connection)
pubsub.Topic('topic_name').create()

Is there a reason why you'd want to keep the "scope credentials" action separate from the "create connection" action? I can't think of a reason but maybe you guys know more about this... If there isn't then I fully support this change -- seems much more convenient to do this when creating the connection (and expecting the connection to know which scopes are necessary on the credentials you supplied).

@dhermes
Copy link
Contributor Author

dhermes commented Apr 29, 2015

I also fully support this change FWIW

@dhermes
Copy link
Contributor Author

dhermes commented Apr 30, 2015

@tseaver PTAL

:class:`NoneType`
:returns: A new credentials object that has a scope added (if needed).
"""
if credentials and credentials.create_scoped_required():

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@dhermes dhermes force-pushed the create-scoped-help branch from 0cb6fc9 to 7cfc884 Compare May 7, 2015 23:29
@dhermes
Copy link
Contributor Author

dhermes commented May 7, 2015

Just rebased on top of HEAD in master.

@tseaver Can we resolve the discussion and move forward here?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 7cfc884 on dhermes:create-scoped-help into a9bc7fe on GoogleCloudPlatform:master.

@dhermes
Copy link
Contributor Author

dhermes commented May 12, 2015

@tseaver Can we wrap this up?

@@ -17,6 +17,10 @@
from gcloud import connection as base_connection


SCOPE = ('https://www.googleapis.com/auth/pubsub',
'https://www.googleapis.com/auth/cloud-platform')

This comment was marked as spam.

@tseaver
Copy link
Contributor

tseaver commented May 14, 2015

LGTM, module my comments this morning on the SCOPE docstrings.

This also obsoletes get_scoped_connection() so it is removed.
@dhermes dhermes force-pushed the create-scoped-help branch from 7cfc884 to b44b25b Compare May 14, 2015 18:36
@dhermes
Copy link
Contributor Author

dhermes commented May 14, 2015

Rebased on top of master (#879 #881 in particular) and added the docstrings into the original PR. Will merge when Travis finishes.

dhermes added a commit that referenced this pull request May 14, 2015
Adding scope to credentials in Connection constructors.
@dhermes dhermes merged commit a888f64 into googleapis:master May 14, 2015
@dhermes dhermes deleted the create-scoped-help branch May 14, 2015 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: core auth cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants