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

Samples for migrating from Python 2.7 runtime to Python 3.7 #3656

Merged
merged 51 commits into from
Jun 9, 2020
Merged

Conversation

engelke
Copy link
Contributor

@engelke engelke commented Apr 30, 2020

Replacing use of Python 2.7 runtime-only libraries for storage and web requests.

@engelke engelke added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 30, 2020
@engelke engelke requested a review from a team as a code owner April 30, 2020 22:47
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 30, 2020
gguuss
gguuss previously requested changes May 1, 2020
Copy link
Contributor

@gguuss gguuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM, a few nits and it looks like lint's failing.

appengine/standard/migration/incoming/README.md Outdated Show resolved Hide resolved
appengine/standard/migration/storage/main.py Show resolved Hide resolved
appengine/standard/migration/incoming/main.py Show resolved Hide resolved
url = request.form['url']
token = id_token.fetch_id_token(reqs.Request(), url)

resp = requests.get(url,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe better as:

    resp = requests.get(
            url,
            headers={'Authorization': 'Bearer {}'.format(token)}
    )

Comment on lines +49 to +55
info = id_token.verify_oauth2_token(token, requests.Request())
service_account_email = info['email']
incoming_app_id, domain = service_account_email.split('@')
if domain != 'appspot.gserviceaccount.com': # Not App Engine svc acct
return None
else:
return incoming_app_id
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May also want to verify the audience & issuer here.

The (Python) verify_oauth2_token function verifies the JWT signature, the aud claim, and the exp claim. You must also verify the iss claim and the hd claim (if applicable) by examining the object that verify_oauth2_token returns. If multiple clients access the backend server, also manually verify the aud claim. (ref)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought long and hard about verifying the audience, but eventually decided that I was showing how to migrate from the old X-Appengine-Inbound-Appid approach, which did not perform that check.

The Google verify_oauth2_token verifies tokens issued by Google's OAuth2 server, so I don't think there needs to a separate check of the issuer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM.

The text & samples on their website for other languages vs. Python are a little misleading. I will file a bug.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible that those other language libraries don't do the same issuer
verification automatically.

@engelke Hold on, I thought the text implied that Python doesn't verify issuer automatically.

Copy link
Member

@anguillanneuf anguillanneuf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See inline comments.

@tmatsuo
Copy link
Contributor

tmatsuo commented Jun 1, 2020

It's been a while from the last activity. Please rebase to the master when you push your next commit.

Also I'll close this PR on Fri June 5th. Feel free to reopen with rebasing.

@engelke engelke dismissed stale reviews from anguillanneuf and gguuss June 1, 2020 17:27

Issues were addressed in comments.

@engelke engelke removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 2, 2020
@engelke engelke merged commit 3d77a5c into master Jun 9, 2020
@engelke engelke deleted the msprint branch June 9, 2020 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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