Remove query parameters from callback_url

[wjordan]

Because query parameter removal is needed for request_phase in addition to build_access_token, this PR overrides the callback_url parameter instead of build_access_token.

Otherwise, the oauth check will fail with the following error if any query parameters are included in the redirected request:

Authentication failure! invalid_credentials: OAuth2::Error, invalid_grant: The provided value for the 'redirect_uri' is not valid. The value must exactly match the redirect URI used to obtain the authorization code.
{"error":"invalid_grant","error_description":"The provided value for the 'redirect_uri' is not valid. The value must exactly match the redirect URI used to obtain the authorization code."}

[coveralls]

Coverage remained the same at 100.0% when pulling afa2753 on wjordan:query_parameter_fix into 2617222 on joel:master.

[coveralls]

Coverage remained the same at 100.0% when pulling afa2753 on wjordan:query_parameter_fix into 2617222 on joel:master.

[coveralls]

Coverage remained the same at 100.0% when pulling afa2753 on wjordan:query_parameter_fix into 2617222 on joel:master.

[joel]

Thanks @wjordan can you add or change test?

[coveralls]

Coverage remained the same at 100.0% when pulling b35a57b on wjordan:query_parameter_fix into 2617222 on joel:master.

[wjordan]

@joel added test, please take a look.

[mrj]

Removing all query parameters from the callback URL removes any way for application parameters to be propagated through the auth. Windows Live OAuth supports using callback URLs with query parameters added to the registered callback URLs, but doesn't seem to support adding application state parameters to authorization code requests. And the alternative of using cookies is more fragile.

I've used the following modification on my Groove Music fork to preserve only the application parameters, allowing the callback parameters to remain the same through the auth:

redirect_uri = URI.parse(callback_url).tap { |uri| uri.query = Rack::Utils.parse_query(uri.query).reject { |k,v| %w(code state).include?(k) }.to_query }.to_s

necessitating requiring 'rack/utils' and 'active_support' in this strategy file.

Perhaps removing the code and state parameters with a regular expression would be more efficient, and definitely wouldn't mess with parameter order, which is important to keep the callback URL matched.

I'd do a PR, but can't yet work out how to do a second fork under the original name.

[joel]

@wjordan and @mrj okay guys, I guess the best options is to put some setting in this gem like that :

you can choose between several options :

• To clean all query params
• Leave all query params
• Provide a list of keys you want to keep
• Provide a list of keys you want to wipe out.

Like that we can cover all cases.

What do you think?

[mrj]

Joel, always removing just the code and state keys from the callback (plus any dangling question mark) should ensure that the callbacks always match, whether or not the callback has app parameters.

[joel]

Well @mrj then give me a PR please

[mrj]

Joel, as I said, a PR is difficult because I already have a very different fork. I'd have to make a fork branch with a restored strategy file.

Really, removing code and state parameters from the callback url should be done by the omniauth-oauth2 gem itself. There are a number of issues there that cover the callback parameters problem, and a change to remove all the query parameters, as was done here, was reversed. One patch attempts to remove just the code and state parameters.

So I'll comment on the most recent issue discussing this.

[joel]

https://rubygems.org/gems/omniauth-windowslive/versions/0.0.12

[mrj]

My comment on this issue at omniauth-oauth2 is here.