-
Notifications
You must be signed in to change notification settings - Fork 6.5k
Add web sample for using google-auth for end-user auth #1127
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
Conversation
Change-Id: I90856d5da694b9eab1b363f7982638482e10429a
@AndyDiamondstein can you review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sample works for me. Thanks!
Change-Id: Icb43629fa835682eeb0a248dbb56e12b15d0dd15
auth/end-user/web/main.py
Outdated
|
||
app = flask.Flask(__name__) | ||
# Note: A secret key is included in the sample so that it works, but if you | ||
# use this code in your application please replace this with a truly secret |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:s/ please/, please/
auth/end-user/web/main.py
Outdated
# Note: A secret key is included in the sample so that it works, but if you | ||
# use this code in your application please replace this with a truly secret | ||
# key. See http://flask.pocoo.org/docs/0.12/quickstart/#sessions. | ||
app.secret_key = 'example key, please replace.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the value be capitalized to make it more eye-catching, eg"REPLACE ME: sample key"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Done.
auth/end-user/web/main.py
Outdated
if __name__ == '__main__': | ||
# When running locally, disable OAuthlib's HTTPs verification. When | ||
# running in production *do not* leave this option enabled. | ||
os.environ['OAUTHLIB_INSECURE_TRANSPORT'] = '1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also add a capitalized in-line comment, a la # TODO: disable this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary in this case, as typically you'll use gunicorn. But I'll expand the note.
auth/end-user/web/main.py
Outdated
# limitations under the License. | ||
|
||
"""An example web application that obtains authorization and credentials from | ||
an end user.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest adding to these top-of-file comments some comments on how to run this---in particular the prerequisites that you allude to below about getting the client secrets file and setting the environment variable. For the former, a link would be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
auth/end-user/web/main.py
Outdated
# API Client Library and Google Cloud Client Library. | ||
session = google.auth.transport.requests.AuthorizedSession(credentials) | ||
|
||
response = session.get('https://www.googleapis.com/oauth2/v2/userinfo') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're calling the OAuth2 API in this example. Since this is an example for how to use auth, it might be less completely if the API was something unrelated (like GMail or Maps)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actual quite lame that this API is called the "oauth2" API. It's a great candidate for this sample because it doesn't require being turned on in the API console, has a stupid high quota, and is extremely deterministic in what it returns.
I could maybe alternatively use the gmail api and fetch the user profile via that, but I'd like to see how this fairs before doing that.
Change-Id: I3f7492601a6ec7f64dab611928dc1d6c5a23d9af
Change-Id: I98ea11e842c679caf944b134e745013962c5c34a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just some questions and a few nits.
(If we disagree on comma philosophy, that's fine. Just want to make sure you see it.)
|
||
app = flask.Flask(__name__) | ||
# TODO: A secret key is included in the sample so that it works but if you | ||
# use this code in your application please replace this with a truly secret |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: ", please"
CLIENT_SECRETS_FILENAME, scopes=SCOPES) | ||
flow.redirect_uri = flask.url_for('oauth2callback', _external=True) | ||
authorization_url, state = flow.authorization_url( | ||
# This parameter enables offline access which gives your application |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: ", which"
auth/end-user/web/main.py
Outdated
flow.redirect_uri = flask.url_for('oauth2callback', _external=True) | ||
authorization_url, state = flow.authorization_url( | ||
# This parameter enables offline access which gives your application | ||
# both an access and refresh token. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "both access and refresh tokens"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rephrased so it's clear it's one of each.
auth/end-user/web/main.py
Outdated
include_granted_scopes='true') | ||
|
||
# Store the state in the session so that the callback can verify that | ||
# the authorization server response. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean "responded"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this sentence is wonky, fixed.
|
||
# Load the credentials from the session. | ||
credentials = google.oauth2.credentials.Credentials( | ||
**flask.session['credentials']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to check credential validity, either here or after issuing the request if it failed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
credentials.valid
but the apiclient will refresh if needed and raise a useful error if it's unrefreshable/otherwise invalid.
state = flask.session['state'] | ||
flow = google_auth_oauthlib.flow.Flow.from_client_secrets_file( | ||
CLIENT_SECRETS_FILENAME, scopes=SCOPES, state=state) | ||
flow.redirect_uri = flask.url_for('oauth2callback', _external=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does flow.redirect_uri
do here? I'm surprised this handler is setting it to itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has to be set for oauthlib to verify the response - oauthlib verifies state, http(s), and makes sure the url base is correct.
Change-Id: I44d4c690c8fb3702988cb303d4afc9872331bb32
Merging based on review from @AndyDiamondstein, we can always make some changes to this once we get it out there and get feedback, but it's a huge improvement on what's there. |
No description provided.