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

Fixes #14852 - do not pass in "data" param unless needed #436

Merged
merged 1 commit into from
Apr 28, 2016

Conversation

beav
Copy link
Contributor

@beav beav commented Apr 27, 2016

Previously, the "data" param was passed to all oauth requests. This caused
issues with GET requests, since data and headers gets mashed into
*arguments, and then the oauth gem will shift off the first argument if
there's a POST or PUT method. The end result was that headers weren't being
sent on GET requests.

Instead, only supply the "data" param if we are doing a POST or PUT.

@beav
Copy link
Contributor Author

beav commented Apr 27, 2016

note: this PR should fix the katello dev setup 401 issue during proxy registration

@stbenjam
Copy link
Member

@beav
Copy link
Contributor Author

beav commented Apr 27, 2016

this PR has a bug related to POSTing. I am looking into it now and will update.

if [:post, :put, :patch].include?(method)
response = oauth_consumer.request(method, uri.to_s, generate_token, {}, data, headers)
else
response = oauth_consumer.request(method, uri.to_s, generate_token, {}, headers)
Copy link
Member

Choose a reason for hiding this comment

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

Is GET the only one this applies to? I might flip the conditions around in that case. Could :delete for example be a possible method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be flipped, I did it like that so it's the inverse of what the oauth gem does.

in 0.4.7 it's:

      if [:post, :put].include?(http_method)
        data = arguments.shift
      end

Later versions added :patch so I added it here as well.

Copy link
Member

Choose a reason for hiding this comment

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

Ok makes sense then

@stbenjam
Copy link
Member

Tested, clean installer run, and the proxy gets registered.

@beav
Copy link
Contributor Author

beav commented Apr 27, 2016

travis fails on 1.8.7:

Gem::InstallError: webmock requires Ruby version >= 1.9.3.

An error occurred while installing webmock (2.0.0), and Bundler cannot continue.

This appears to not be related to this PR.

@domcleal
Copy link
Contributor

#437 should fix the webmock error.

Tested this and it works fine, I'm guessing you've enabled oauth_map_users, which is why the contents of the headers are important. Seems to fix it for me.

@mmoll
Copy link
Contributor

mmoll commented Apr 28, 2016

@beav please rebase.

Previously, the "data" param was passed to all oauth requests. This caused
issues with `GET` requests, since `data` and `headers` gets mashed into
`*arguments`, and then the oauth gem will `shift` off the first argument if
there's a `POST` or `PUT` method. The end result was that headers weren't being
sent on `GET` requests.

Instead, only supply the "data" param if we are doing a `POST` or `PUT`.
@beav
Copy link
Contributor Author

beav commented Apr 28, 2016

@mmoll rebased

@mmoll
Copy link
Contributor

mmoll commented Apr 28, 2016

merged, thanks @beav!

cegeka-jenkins pushed a commit to cegeka/puppet-foreman that referenced this pull request Oct 23, 2017
)

Previously, the "data" param was passed to all oauth requests. This caused
issues with `GET` requests, since `data` and `headers` gets mashed into
`*arguments`, and then the oauth gem will `shift` off the first argument if
there's a `POST` or `PUT` method. The end result was that headers weren't being
sent on `GET` requests.

Instead, only supply the "data" param if we are doing a `POST` or `PUT`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants