Add InstalledAppFlow by theacodes · Pull Request #128 · googleapis/google-auth-library-python

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


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


 import json
+import logging
+import webbrowser


google/oauth2/flow.py

 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

    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


 import json
+import logging
+import webbrowser


google/oauth2/flow.py

 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 dhermes and lukesneeringer 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

google/oauth2/flow.py

import json

import logging

import webbrowser

This comment was marked as spam.

Sign in to view

google/oauth2/flow.py

import google.oauth2.oauthlib

_LOGGER = logging.getLogger(__name__)

This comment was marked as spam.

Sign in to view

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


		_LOGGER = logging.getLogger(__name__)


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

Conversation

theacodes commented Mar 2, 2017

Uh oh!

theacodes commented Mar 2, 2017

Uh oh!

theacodes commented Mar 2, 2017

Uh oh!

lukesneeringer commented Mar 2, 2017

Uh oh!

theacodes commented Mar 2, 2017

Uh oh!

lukesneeringer commented Mar 2, 2017

Uh oh!

theacodes commented Mar 2, 2017

Uh oh!

lukesneeringer commented Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theacodes commented Mar 2, 2017

Uh oh!

lukesneeringer commented Mar 3, 2017

Uh oh!

theacodes commented Mar 6, 2017

Uh oh!

theacodes commented Mar 16, 2017

Uh oh!

theacodes commented Mar 16, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

proppy commented Mar 17, 2017

Uh oh!

theacodes commented Mar 18, 2017

Uh oh!

theacodes commented Mar 18, 2017

lukesneeringer commented Mar 2, 2017 •

edited

Loading