Add InstalledAppFlow #128

theacodes · 2017-03-02T00:06:03Z

Resolves #122

theacodes · 2017-03-02T00:10:11Z

This is DO NOT MERGE because I want a sanity check before I go any further.

What do you think about this being in the library in general?
What do you think about the way the implementation is structured?

Specifically for (2), I had some thoughts that it might make more sense to separate the strategies into separate classes:

flow = google.oauth2.flow.InstalledAppFlow.from_client_secrets_file(...)

strategy = flow.ServerStrategy(port=9000)
# or
strategy = flow.ConsoleStrategy()

flow.run(strategy)
# or
flow.strategy = strategy
flow.run()

But I'm unsure if that's over-engineering, even if it feels cleaner.

theacodes · 2017-03-02T00:10:43Z

/cc @proppy

lukesneeringer · 2017-03-02T22:14:07Z

Can you provide some sample code for what it looks like with a single, combined flow? It would make it easier to reason about without doing a deep dive.

theacodes · 2017-03-02T22:17:33Z

@lukesneeringer not sure what you're asking for.

lukesneeringer · 2017-03-02T22:32:35Z

@lukesneeringer not sure what you're asking for.

I was referring to (2) specifically.

Right now it seems to be:

flow = InstalledAppFlow(...)
flow.run(strategy=SERVER, port=9000)

I think that is fine. I do not think strategy objects are necessary. And it does seem reasonable that some arguments (like port) are ignored if they are not applicable to the strategy in use.

theacodes · 2017-03-02T22:38:47Z

@lukesneeringer that's nearly correct, it would be:

flow = InstalledAppFlow(...)
flow.web_port = 9000
flow.run(strategy=SERVER)

I can keep that same interface but separate the internals into two separate strategy classes. Curious as to what @dhermes thinks as well.

lukesneeringer · 2017-03-02T22:43:07Z

That makes a little less sense to me, but it certainly seems good enough. And it is probably better than making the user create a strategy object.

theacodes · 2017-03-02T22:44:50Z

That makes a little less sense to me

It makes more sense to pass into run then we can do that. There's plenty of malleability here.

Another option is to have two runs: run_console and run_server.

lukesneeringer · 2017-03-03T16:41:35Z

Oooh, I think run_console and run_server taking the appropriate arguments is a good approach.

theacodes · 2017-03-06T18:59:32Z

Oooh, I think run_console and run_server taking the appropriate arguments is a good approach.

Cool, that's what I'll go with. I'll remove run and just have run_server and run_console.

theacodes · 2017-03-16T22:50:36Z

@dhermes @lukesneeringer this is ready for review. :)

theacodes · 2017-03-16T22:52:51Z

@proppy it would be super cool if you took a look as well.

google/oauth2/flow.py

@@ -15,7 +15,7 @@
 """OAuth 2.0 Authorization Flow

 .. warning::
- This module is experimental and is subject to change signficantly
+ This module is experimental and is subject to change significantly


google/oauth2/flow.py

@@ -54,12 +54,22 @@
 """

 import json
+import logging
+import webbrowser


google/oauth2/flow.py


 import google.auth.transport.requests
 import google.oauth2.credentials
 import google.oauth2.oauthlib


+_LOGGER = logging.getLogger(__name__)


google/oauth2/flow.py

+
+ import google.oauth2.flow
+
+ flow = google.oauth2.flow.InstalledAppFlow.from_client_secrets_file(


google/oauth2/flow.py

+ """
+ kwargs.setdefault('prompt', 'consent')
+
+ self.redirect_uri = self._OOB_REDIRECT_URI


google/oauth2/flow.py

+
+ auth_url, _ = self.authorization_url(**kwargs)
+
+ print(authorization_prompt_message.format(url=auth_url))


google/oauth2/flow.py

+ code is then exchanged for a token.
+
+ Args:
+ host (str): The web host for the local redirect server.


google/oauth2/flow.py

+
+ return self.credentials
+
+ class _LocalRedirectServer(BaseHTTPServer.HTTPServer):


setup.py

@@ -39,7 +39,7 @@
 description='Google Authentication Library',
 long_description=long_description,
 url='https://github.com/GoogleCloudPlatform/google-auth-library-python',
- packages=find_packages(exclude=('tests', 'system_tests')),
+ packages=find_packages(exclude=('tests*', 'system_tests*')),


google/oauth2/flow.py

+ local_server = self._LocalRedirectServer(
+ success_message, (host, port), self._LocalRedirectRequestHandler)
+
+ webbrowser.open(auth_url, new=1, autoraise=True)


google/oauth2/flow.py

+ self.success_message = success_message
+ self.last_request_path = None
+
+ class _LocalRedirectRequestHandler(BaseHTTPServer.BaseHTTPRequestHandler):


proppy · 2017-03-17T17:06:37Z

do we want a "smart" run that fallback to run_console if opening the browser failed?

theacodes · 2017-03-18T05:04:15Z

do we want a "smart" run that fallback to run_console if opening the browser failed?

Webbrowser doesn't give us any indication if the open call was successful or not.

theacodes · 2017-03-18T05:25:38Z

@dhermes @lukesneeringer should be good to review again.

dhermes

LGTM I think?

google/oauth2/flow.py

@@ -54,12 +54,22 @@
 """

 import json
+import logging
+import webbrowser


google/oauth2/flow.py


 import google.auth.transport.requests
 import google.oauth2.credentials
 import google.oauth2.oauthlib


+_LOGGER = logging.getLogger(__name__)


theacodes · 2017-03-22T20:21:21Z

I meant "having this code period"

The good news is that it's in a separate package now, woohoo. :)

additional_packages/google_auth_oauthlib/google_auth_oauthlib/flow.py

+
+ auth_url, _ = self.authorization_url(**kwargs)
+
+ print(authorization_prompt_message.format(url=auth_url))


theacodes requested review from lukesneeringer and dhermes March 2, 2017 00:06

theacodes changed the title ~~[DO NOT MERGE] Add InstalledAppFlow~~ Add InstalledAppFlow Mar 16, 2017

dhermes reviewed Mar 17, 2017

View reviewed changes

proppy reviewed Mar 17, 2017

View reviewed changes

google/oauth2/flow.py Outdated

local_server = self._LocalRedirectServer(

success_message, (host, port), self._LocalRedirectRequestHandler)

webbrowser.open(auth_url, new=1, autoraise=True)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

proppy reviewed Mar 17, 2017

View reviewed changes

google/oauth2/flow.py Outdated

self.success_message = success_message

self.last_request_path = None

class _LocalRedirectRequestHandler(BaseHTTPServer.BaseHTTPRequestHandler):

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

theacodes force-pushed the installed-flow branch from 68ef67c to af9677a Compare March 18, 2017 05:02

dhermes approved these changes Mar 22, 2017

View reviewed changes

Jon Wayne Parrott added 6 commits March 22, 2017 13:06

Add InstalledAppFlow

761ce08

Fix typo

3c2b70a

Finish up implementation and tests

8d436ed

Address review comments

edd5681

Fix race condition

2c2d5af

Fix tests

d464f86

theacodes force-pushed the installed-flow branch from bebf653 to d464f86 Compare March 22, 2017 20:20

Fix futures dependency

17507af

theacodes merged commit 41a2bba into master Mar 22, 2017

theacodes deleted the installed-flow branch March 22, 2017 20:43

proppy reviewed Mar 27, 2017

View reviewed changes

additional_packages/google_auth_oauthlib/google_auth_oauthlib/flow.py

auth_url, _ = self.authorization_url(**kwargs)

print(authorization_prompt_message.format(url=auth_url))

This comment was marked as spam.

Sign in to view


		import google.oauth2.flow

		flow = google.oauth2.flow.InstalledAppFlow.from_client_secrets_file(


		auth_url, _ = self.authorization_url(**kwargs)

		print(authorization_prompt_message.format(url=auth_url))


		return self.credentials

		class _LocalRedirectServer(BaseHTTPServer.HTTPServer):

Add InstalledAppFlow #128

Add InstalledAppFlow #128

Conversation

theacodes commented Mar 2, 2017

theacodes commented Mar 2, 2017

theacodes commented Mar 2, 2017

lukesneeringer commented Mar 2, 2017

theacodes commented Mar 2, 2017

lukesneeringer commented Mar 2, 2017

theacodes commented Mar 2, 2017

lukesneeringer commented Mar 2, 2017 • edited Loading

theacodes commented Mar 2, 2017

lukesneeringer commented Mar 3, 2017

theacodes commented Mar 6, 2017

theacodes commented Mar 16, 2017

theacodes commented Mar 16, 2017

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.

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.

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.

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.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

proppy commented Mar 17, 2017

theacodes commented Mar 18, 2017

theacodes commented Mar 18, 2017

dhermes left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

theacodes commented Mar 22, 2017

This comment was marked as spam.

lukesneeringer commented Mar 2, 2017 •

edited

Loading