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

feat: add PKCE support to AuthorizationCodeFlow #470

Merged
merged 19 commits into from Jun 29, 2020

Conversation

StFS
Copy link
Contributor

@StFS StFS commented May 29, 2020

feat: Add PKCE support to the AuthorizationCodeFlow

According to the OAuth 2.0 for Native Apps RFC, public native clients MUST implement PKCE (https://tools.ietf.org/html/rfc8252#section-6). It is therefor important to add this mechanism to the google-oauth-java-client library.

  • [✓] Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • [✓] Ensure the tests and linter pass
  • [✓] Code coverage does not decrease (if any source code was changed)
  • [✓] Appropriate docs were updated (if necessary)

Fixes #469 ☕️

@StFS StFS requested a review from a team as a code owner May 29, 2020 17:10
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 29, 2020
@chingor13 chingor13 changed the title Add PKCE support to AuthorizationCodeFlow feat: add PKCE support to AuthorizationCodeFlow Jun 8, 2020
Copy link
Contributor

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this. The overall approach seems ok to me.

@chingor13 chingor13 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 8, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 8, 2020
@StFS
Copy link
Contributor Author

StFS commented Jun 8, 2020

@chingor13 I think I've responded to all your comments and resolved them.

Thank you again so much for taking the time to review this and thanks for your excellent comments. Please let me know if anything else is needed. Should I update the versions.txt file to bump the version or is that just handled by you when you release a new version?

samples/keycloak-pkce-cmdline-sample/README.md Outdated Show resolved Hide resolved
**Prerequisites:** install [Java 6 or higher][install-java], [git][install-git], and
[Maven][install-maven]. You may need to set your `JAVA_HOME`.

1. Check out the sample code:
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what keycloak is, or if we can include this part in a google project

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh... that had not occurred to me. Keycloak is a JBoss (Red Hat) project, it's an "open source identity and access management solution" (https://www.keycloak.org/) and it's released under the Apache 2.0 license.
So, as such there shouldn't be a problem using it, especially since it's only being used for the sample demonstration and not as an actual component. But maybe there is some other more political/legal reason for it not being possible. I could remove the sample but I would like to keep it as it's pretty good and clear for the purpose of testing and demonstration.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a license issue, but we would like the docs for this product to be self-contained.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotcha. However, the daily motion example isn't exactly self-contained is is? It actually depends on the user registering an account with dailymotion.com.

But if using keycloak is not acceptable, can you suggest an alternative or should I remove the sample completely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elharo any thoughts on this?

samples/keycloak-pkce-cmdline-sample/pom.xml Outdated Show resolved Hide resolved
@StFS
Copy link
Contributor Author

StFS commented Jun 11, 2020

@chingor13 what are the next steps here? I believe I've addressed everything you and @elharo have pointed out. The only still outstanding question is whether we can include the PKCE sampe project because it requires the user to run Keycloak in a docker container. I can remove it, but as I said, compared to the other example which requires a user to register with dailymotion.com I would argue that the PKCE sample is more self contained that that.

But mainly I'm eager to get this merged and into a new release so I don't have to create my own custom patch version of the library for us to use ;-)

@chingor13 chingor13 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 19, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 19, 2020
Copy link
Contributor

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in reviewing again. Keycloak looks fine to me. The only change we need to make is to ensure the source files have license headers. You can duplicate the other source files for examples.

@StFS
Copy link
Contributor Author

StFS commented Jun 28, 2020

@chingor13 just another friendly nudge to get this merged :)

@chingor13 chingor13 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 29, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 29, 2020
@chingor13 chingor13 merged commit 13433cd into googleapis:master Jun 29, 2020
@StFS
Copy link
Contributor Author

StFS commented Jun 29, 2020

Thanks @chingor13

Just two questions:

  1. When would you expect a new version to be released?
  2. You commented on the PR that this should be a bump on the minor version since new API is being introduced. Do you keep track of that somewhere (that the next release should be a minor release, not a patch release)?

@StFS StFS deleted the pkce-support branch June 30, 2020 09:50
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.

PKCE support?
5 participants