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

Jump to bottom

fix: oauth1 signing for url encoded content #538

Merged
merged 3 commits into from Feb 2, 2021
Merged

fix: oauth1 signing for url encoded content #538

merged 3 commits into from Feb 2, 2021

Conversation

cmunden
Copy link
Contributor

@cmunden cmunden commented Oct 7, 2020 •
edited

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • 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 #1 ☕️

cmunden and others added 2 commits October 7, 2020 12:33
@cmunden
Fixed OAuth 1.0 signing for requests with URL Encoded Content (typica…
d0ec683 
…l of POST and PUT HTTP methods)

A simple change the the OAuthParameters intercept method that checks if the request has UrlEncodedContent.  If it does, then its content is added to the GenericUrl so that the signature can be computed correctly.  After the call to computeSignature, the content parameters are removed from the GenericUrl to leave it in its original form.

As per the OAuth 1.0 spec here (https://tools.ietf.org/html/rfc5849#page-28), any form encoded request body with a Content-Type of "application/x-www-form-urlencoded" must be included when the request signature is created.
@cmunden
Changes to support java 1.7. Also added new test code.
87662e1
@cmunden cmunden requested a review from a team as a code owner October 7, 2020 16:05
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 7, 2020
@elharo elharo added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 7, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 7, 2020
elharo
elharo reviewed Oct 7, 2020
View reviewed changes
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

an issue explaining the bug would help.

google-oauth-client/src/main/java/com/google/api/client/auth/oauth/OAuthParameters.java Outdated
import com.google.api.client.http.HttpExecuteInterceptor;
import com.google.api.client.http.HttpRequest;
import com.google.api.client.http.HttpRequestInitializer;
import com.google.api.client.http.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

Google doesn't use wildcard imports

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 have removed the wildcard import in my most recent commit. Regarding an issue to explain the bug, a pre-existing issue was referenced in the pull request description.

Fixes #1

@elharo elharo requested a review from silvolu October 7, 2020 20:57
@elharo elharo changed the title Oauth1 signing for url encoded content fix: oauth1 signing for url encoded content Oct 7, 2020
@elharo
Copy link
Contributor

elharo commented Oct 7, 2020

@silvolu The referenced issue is pretty old. Is this still something we want to do?

@elharo elharo added the kokoro:run Add this label to force Kokoro to re-run the tests. label Oct 7, 2020
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Oct 7, 2020
elharo
elharo reviewed Oct 7, 2020
View reviewed changes
google-oauth-client/src/test/java/com/google/api/client/auth/oauth/OAuthParametersTest.java
parameters.signer = new MockSigner();

GenericUrl url = new GenericUrl("https://example.local?foo=bar");
Map<String, Object> contentParameters = Collections.singletonMap("this", (Object) "that");
Copy link
Contributor

Choose a reason for hiding this comment

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

cast to Object probably isn't needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The cast to Object is necessary in order to create the contentParameters templated to a map of type Map<String, Object>. Without the explicit cast to Object, the singletonMap method will template to a map of type Map<String, String>.

@vladokrsymphony
Copy link

vladokrsymphony commented Feb 2, 2021 •
edited

Hello, any plans soon to merge this PR? Additional fix should be done for the query parameters, not only to the request body.
Using the library it generates wrong oauth signature when the URL contains non ascii characters.

@elharo elharo merged commit d9507e4 into googleapis:master Feb 2, 2021
@cmunden
Copy link
Contributor Author

cmunden commented Feb 2, 2021

@vladokrsymphony I recommend that your report your additional issue regarding non ascii characters in the Issues section. Just go here and choose "Get Started" in the Bug Report area.

Since this PR is now merged and your bug is of a slightly different nature, then it can be tracked and not missed. Also remember to provide some good step-by-step instructions on how to recreate the issue.

Happy coding 👍

gcf-merge-on-green bot pushed a commit that referenced this pull request Apr 9, 2021
@release-please
chore: release 1.31.5 (#619)
79d7edc 
🤖 I have created a release \*beep\* \*boop\*
---
### [1.31.5](https://www.github.com/googleapis/google-oauth-java-client/compare/v1.31.4...v1.31.5) (2021-04-09)


### Bug Fixes

* don't swallow exceptions in LocalServerReceiver ([#595](https://www.github.com/googleapis/google-oauth-java-client/issues/595)) ([f39faec](https://www.github.com/googleapis/google-oauth-java-client/commit/f39faec9980fa65602a216fbf34555b744139443))
* oauth1 signing for url encoded content ([#538](https://www.github.com/googleapis/google-oauth-java-client/issues/538)) ([d9507e4](https://www.github.com/googleapis/google-oauth-java-client/commit/d9507e4c367cc870b811e28e3b206ef4661c67d8))
* remove Jackson from assembly ([#605](https://www.github.com/googleapis/google-oauth-java-client/issues/605)) ([a482000](https://www.github.com/googleapis/google-oauth-java-client/commit/a482000eddf3c056f57492487c4a2f1e2f81feeb))
* switch to GSON per security team advice ([#586](https://www.github.com/googleapis/google-oauth-java-client/issues/586)) ([58a1828](https://www.github.com/googleapis/google-oauth-java-client/commit/58a1828e8e291c59494893b2632c294dffe98b23))


### Dependencies

* update appengine packages to v1.9.84 ([#577](https://www.github.com/googleapis/google-oauth-java-client/issues/577)) ([3fbd4d5](https://www.github.com/googleapis/google-oauth-java-client/commit/3fbd4d5205215447969adb7fa93a46f309eed4a5))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
@release-please release-please bot mentioned this pull request Jan 4, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elharo elharo elharo approved these changes

@silvolu silvolu Awaiting requested review from silvolu

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OAuth 1.0: support URL form encoded parameters
4 participants
@cmunden @elharo @vladokrsymphony @kokoro-team