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

fix: oauth1 signing for url encoded content #538

Merged

Conversation

@cmunden
Copy link
Contributor

@cmunden cmunden commented Oct 7, 2020

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 Oct 7, 2020
…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.
Copy link
Collaborator

@elharo elharo left a comment

an issue explaining the bug would help.

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
Collaborator

@elharo elharo Oct 7, 2020

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

@cmunden cmunden Oct 7, 2020

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 Oct 7, 2020
@elharo elharo changed the title Oauth1 signing for url encoded content fix: oauth1 signing for url encoded content Oct 7, 2020
@elharo
Copy link
Collaborator

@elharo elharo commented Oct 7, 2020

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

parameters.signer = new MockSigner();

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

@elharo elharo Oct 7, 2020

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

@cmunden cmunden Oct 7, 2020

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 vladokrsymphony commented Feb 2, 2021

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 approved these changes Feb 2, 2021
@elharo elharo merged commit d9507e4 into googleapis:master Feb 2, 2021
7 checks passed
@cmunden
Copy link
Contributor Author

@cmunden 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 issue Apr 9, 2021
🤖 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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants