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

Adds support for RFC7636 PKCE #588

Merged
merged 1 commit into from Aug 11, 2016

Conversation

Projects
None yet
5 participants
@bjmc
Contributor

bjmc commented Aug 4, 2016

This adds support for Proof Key for Code Exchange (PKCE) as specified by RFC7636. This is particularly useful for installable applications (either desktop or mobile) that cannot protect a client secret.

@googlebot googlebot added the cla: yes label Aug 4, 2016

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 4, 2016

Member

Thank you for this contribution!

Lint seems to be failing in travis, see here. Please fix lint errors.

Member

theacodes commented Aug 4, 2016

Thank you for this contribution!

Lint seems to be failing in travis, see here. Please fix lint errors.

Show outdated Hide outdated oauth2client/client.py Outdated
Show outdated Hide outdated oauth2client/pkce.py Outdated
Show outdated Hide outdated oauth2client/pkce.py Outdated
Show outdated Hide outdated oauth2client/pkce.py Outdated
Show outdated Hide outdated oauth2client/pkce.py Outdated
Show outdated Hide outdated tests/test_client.py Outdated

@theacodes theacodes self-assigned this Aug 4, 2016

Show outdated Hide outdated oauth2client/pkce.py Outdated
Show outdated Hide outdated tests/test_client.py Outdated
@@ -1816,6 +1817,7 @@ def __init__(self, client_id,
device_uri=oauth2client.GOOGLE_DEVICE_URI,
token_info_uri=oauth2client.GOOGLE_TOKEN_INFO_URI,
authorization_header=None,
pkce=False,

This comment has been minimized.

@nathanielmanistaatgoogle

nathanielmanistaatgoogle Aug 4, 2016

Contributor

How can you add a parameter to this constructor without also adding an entry to the constructor's doc string's "Args:" section?

@nathanielmanistaatgoogle

nathanielmanistaatgoogle Aug 4, 2016

Contributor

How can you add a parameter to this constructor without also adding an entry to the constructor's doc string's "Args:" section?

This comment has been minimized.

@bjmc

bjmc Aug 4, 2016

Contributor

D'oh! Good catch.

@bjmc

bjmc Aug 4, 2016

Contributor

D'oh! Good catch.

Show outdated Hide outdated tests/test_pkce.py Outdated
@bjmc

This comment has been minimized.

Show comment
Hide comment
@bjmc

bjmc Aug 4, 2016

Contributor

I think I've finally mollified Travis-CI. How are people feeling?

Contributor

bjmc commented Aug 4, 2016

I think I've finally mollified Travis-CI. How are people feeling?

@nathanielmanistaatgoogle

This comment has been minimized.

Show comment
Hide comment
@nathanielmanistaatgoogle

nathanielmanistaatgoogle Aug 4, 2016

Contributor

I developed an upset stomach and went home for the afternoon. Thanks for asking! Too often these code reviews are coldly technical and lacking the human element...

Contributor

nathanielmanistaatgoogle commented Aug 4, 2016

I developed an upset stomach and went home for the afternoon. Thanks for asking! Too often these code reviews are coldly technical and lacking the human element...

@bjmc

This comment has been minimized.

Show comment
Hide comment
@bjmc

bjmc Aug 4, 2016

Contributor

Feel better! 🏥 💉

Contributor

bjmc commented Aug 4, 2016

Feel better! 🏥 💉

Show outdated Hide outdated tests/test_pkce.py Outdated
@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 9, 2016

Member

@bjmc gentle nudge here. I think @nathanielmanistaatgoogle still has some outstanding comments.

Member

theacodes commented Aug 9, 2016

@bjmc gentle nudge here. I think @nathanielmanistaatgoogle still has some outstanding comments.

@bjmc

This comment has been minimized.

Show comment
Hide comment
@bjmc

bjmc Aug 9, 2016

Contributor

I thought I'd addressed them all? I've added the documentation requested, and I've underscore-prefixed everything in sight. 😉

@nathanielmanistaatgoogle Did I miss something?

Contributor

bjmc commented Aug 9, 2016

I thought I'd addressed them all? I've added the documentation requested, and I've underscore-prefixed everything in sight. 😉

@nathanielmanistaatgoogle Did I miss something?

Show outdated Hide outdated tests/test_pkce.py Outdated
Show outdated Hide outdated tests/test_pkce.py Outdated
@nathanielmanistaatgoogle

This comment has been minimized.

Show comment
Hide comment
@nathanielmanistaatgoogle

nathanielmanistaatgoogle Aug 9, 2016

Contributor

Three very-small action items.

Please squash commits and ensure that your commit message conforms to the guidelines.

... and thank you very much for the contribution and for enduring our onerous code review!

Contributor

nathanielmanistaatgoogle commented Aug 9, 2016

Three very-small action items.

Please squash commits and ensure that your commit message conforms to the guidelines.

... and thank you very much for the contribution and for enduring our onerous code review!

@bjmc

This comment has been minimized.

Show comment
Hide comment
@bjmc

bjmc Aug 9, 2016

Contributor

Okay, made those changes. One thing I'm actually wondering though: Should we have a way to pass in the code_verifier? Either as an argument to step2_exchange() or to the constructor? I know users could pickle the entire flow object in order to hold onto the generated code_verifier, but some people might rather not have to store the entire object, and just pass in the code_verifier before performing the exchange step.

Also, we probably can't change this any time soon, but OAuth2WebServerFlow should probably be renamed something more like OAuth2AuthorizationCodeFlow since it would be used for installed applications in this case.

Contributor

bjmc commented Aug 9, 2016

Okay, made those changes. One thing I'm actually wondering though: Should we have a way to pass in the code_verifier? Either as an argument to step2_exchange() or to the constructor? I know users could pickle the entire flow object in order to hold onto the generated code_verifier, but some people might rather not have to store the entire object, and just pass in the code_verifier before performing the exchange step.

Also, we probably can't change this any time soon, but OAuth2WebServerFlow should probably be renamed something more like OAuth2AuthorizationCodeFlow since it would be used for installed applications in this case.

@nathanielmanistaatgoogle

This comment has been minimized.

Show comment
Hide comment
@nathanielmanistaatgoogle

nathanielmanistaatgoogle Aug 9, 2016

Contributor

I'm not actually enough of a subject matter expert to judge. @jonparrott?

Contributor

nathanielmanistaatgoogle commented Aug 9, 2016

I'm not actually enough of a subject matter expert to judge. @jonparrott?

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 10, 2016

Member

Should we have a way to pass in the code_verifier?

Yes. Put it in the place that makes the most sense, probably the constructor.

Also, we probably can't change this any time soon, but OAuth2WebServerFlow should probably be renamed something more like OAuth2AuthorizationCodeFlow

Agreed, we will probably address this during the great refactor (#597)

Member

theacodes commented Aug 10, 2016

Should we have a way to pass in the code_verifier?

Yes. Put it in the place that makes the most sense, probably the constructor.

Also, we probably can't change this any time soon, but OAuth2WebServerFlow should probably be renamed something more like OAuth2AuthorizationCodeFlow

Agreed, we will probably address this during the great refactor (#597)

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 10, 2016

Member

To qualify - I would do the following:

  1. Make _code_verifier a public attribute (rename to code_verifier.
  2. Add an optional parameter code_verifier to the constructor.
  3. Generate a new code_verifier if None is specified to the constructor.
Member

theacodes commented Aug 10, 2016

To qualify - I would do the following:

  1. Make _code_verifier a public attribute (rename to code_verifier.
  2. Add an optional parameter code_verifier to the constructor.
  3. Generate a new code_verifier if None is specified to the constructor.
@bjmc

This comment has been minimized.

Show comment
Hide comment
@bjmc

bjmc Aug 10, 2016

Contributor

@jonparrott What do you think of that?

Also, there's some weird build failure related to django that I don't think has anything to do with this changeset.

Contributor

bjmc commented Aug 10, 2016

@jonparrott What do you think of that?

Also, there's some weird build failure related to django that I don't think has anything to do with this changeset.

@bjmc

This comment has been minimized.

Show comment
Hide comment
@bjmc

bjmc Aug 10, 2016

Contributor

Also: Do I need to add the same arguments to these two helper functions? Or is it fine that they don't accept all the same arguments as the OAuth2WebServerFlow() constructor?

Contributor

bjmc commented Aug 10, 2016

Also: Do I need to add the same arguments to these two helper functions? Or is it fine that they don't accept all the same arguments as the OAuth2WebServerFlow() constructor?

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 10, 2016

Member

@bjmc I want to say yes.

Member

theacodes commented Aug 10, 2016

@bjmc I want to say yes.

@bjmc

This comment has been minimized.

Show comment
Hide comment
@bjmc

bjmc Aug 10, 2016

Contributor

Do you want me to change those to pass though all the constructor arguments, or just the two I'm adding right here?

Contributor

bjmc commented Aug 10, 2016

Do you want me to change those to pass though all the constructor arguments, or just the two I'm adding right here?

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 10, 2016

Member

Let's keep it minimal and just the two you're adding.

Member

theacodes commented Aug 10, 2016

Let's keep it minimal and just the two you're adding.

Add support for RFC7636 PKCE
RFC7636 extends OAuth2 to include a challenge-response protocol
called "Proof Key for Code Exchange" (PKCE) in order to mitigate
attacks in situations where clients that cannot protect a client
secret (e.g.installed desktop applications).
@bjmc

This comment has been minimized.

Show comment
Hide comment
@bjmc

bjmc Aug 11, 2016

Contributor

Are people happy with this? I rebased to drop Python 3.3 tests.

Contributor

bjmc commented Aug 11, 2016

Are people happy with this? I rebased to drop Python 3.3 tests.

@theacodes theacodes merged commit 3614fd1 into googleapis:master Aug 11, 2016

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 11, 2016

Member

@bjmc thanks for your contribution!

Member

theacodes commented Aug 11, 2016

@bjmc thanks for your contribution!

@pferate

This comment has been minimized.

Show comment
Hide comment
@pferate

pferate Aug 11, 2016

Contributor

@jonparrott, just rebased onto this commit and found some problems. unittest2 was recently removed so these tests are failing now. I'm about to submit another PR concerning imports, should I update it there, or should it be handled a different way.

Contributor

pferate commented Aug 11, 2016

@jonparrott, just rebased onto this commit and found some problems. unittest2 was recently removed so these tests are failing now. I'm about to submit another PR concerning imports, should I update it there, or should it be handled a different way.

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 11, 2016

Member

@pferate separate commits/PRs are always better

Member

theacodes commented Aug 11, 2016

@pferate separate commits/PRs are always better

@pferate

This comment has been minimized.

Show comment
Hide comment
@pferate

pferate Aug 11, 2016

Contributor

OK. 2 PRs are coming down the pipes.

Contributor

pferate commented Aug 11, 2016

OK. 2 PRs are coming down the pipes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment