More Reliable oAuth

[mafrosis]

Opening this draft PR for discussion.

• Set a timeout on the oauth callback listening server, such that it doesn't simply hang forever holding a port open (depends on feat: Raise meaningful exception when oauth callback times out googleapis/google-auth-library-python-oauthlib#363)
• Handle an edge case where the system has no configured browser, which fixes Allow open_browser=False #187

Also commit 2703518 will likely need to be updated to the actual version number once google-auth-oauthlib is released to pypi.

[kuzmoyev]

Hey @mafrosis, thanks for the PR!

I left two comments. LMK, what you think.

[kuzmoyev]

I decided not to incorporate timeout handling for flow.run_local_server a while back (#54) and just described the way to do it for somebody who needs it in the docs. It allows for a more fine-tuned approach (e.x. setting a timeout) and doesn't require the newest version of the google-auth-oauthlib.

[mafrosis]

Interesting trick to use a separate process to provide a timeout. I might argue that it's a bit of a hack 😄

I'm already running multiprocess async, and there's no technical blocker why I couldn't use this pattern. My 2c is that timeouts ought to be supported first-class in the library.

Would it be a problem to require the most recent google-auth-oauthlib, or are you concerned about cross-compatibility with other libraries?

[kuzmoyev]

That's a nice idea!

There are several things I'd like to change though:

1. I think open_browser should be an argument in the GoogleCalendar initialization with 3 options:
   • None (default): this implementation, i.e. try with the browser. If failed, log the message (using logger, e.x. here) and try without the browser
   • True: try with the browser, fail fast if failed (i.e. re-raise webbrowser.Error)
   • False: try without a browser
2. Instead of calling _get_credentials recursively, just use

   credentials = flow.run_local_server(
                       host=host,
                       port=port,
                       bind_addr=bind_addr,
                       open_browser=False
                   )

   Otherwise, the logic before that line will be the same but run twice redundantly.

[mafrosis]

1. Yes 💯
2. The recursive call is there because of the timeout handling. If you didn't recurse, then you would either need to repeat the Timeout exception handling, or drop the timeout support commit

[kuzmoyev]

Regarding 2., I see. I know nested try-except might look weird, but in this case it seems like

try:
    try:
        credentials = flow.run_local_server(
            host=host, port=port, bind_addr=bind_addr, timeout_seconds=120,
            open_browser=open_browser
        )
    except webbrowser.Error:
        # System has no default browser configured, retry without opening browser
        credentials = flow.run_local_server(
            host=host, port=port, bind_addr=bind_addr, timeout_seconds=120,
            open_browser=False
        )
 except WSGITimeout:
     print('Authentication flow timed out. Please try again.')

would make sense.

In any case, both options would do, but that's a discussion for a future release :)

[mafrosis]

It's tumbleweeds over on the googleapis/google-auth-library-python-oauthlib repo, so I'll just leave this PR here for now. Let me know if you're interested in accepting part of it, and I can rebase as necessary.

As it happens, I don't need this patch for my use-case anymore, as I switched to using a GCP service account instead of oauth2 user account for auth. Still, there's some good ideas in here I think and happy to see something merged if you're interested

[kuzmoyev]

Hey, @mafrosis. Sorry for taking so long to respond.

I reconsidered the in-library support for timeout handling. But it does require the google-auth-library-python-oauthlib update and would be in the major release of gcsa (due to backward compatibility).

As for the open_browser update, I'll happily merge it. If you could remove the timeout handling changes and push updates regarding my comment above, please do. Otherwise, I'll push it into dev myself when I have time.

Thanks for the PR and suggestions!

[kuzmoyev]

@mafrosis, thanks again for PR and ideas! I've incorporated the open_browser logic in #192. I will close this PR. When (If) google-auth-library-python-oauthlib gets updated with your changes, we'll revisit the timeout handling.

[mafrosis]

A big if on the google library 😂