Problem with params on master

[georgepalmer]

Steps to reproduce

I think I've found a problem with the latest, unreleased, version of the gem which results in a strong params error. We are using the latest version of the gem because we need the base_metal_controller option.

To eliminate our client code we recreated the problem using these steps:
https://github.com/doorkeeper-gem/doorkeeper/wiki/Authorization-Code-Flow It works when on v5.1 of the gem but not when on master.

When requesting authorisation in the first step of that document we get a strong params error:

ActionController::UnpermittedParameters (found unpermitted parameter: :client_id)

We believe that's because a client_id is passed through but this commit doesn't permit that param: 03b1437 I know that parameter isn't used there but it is elsewhere and that strong param check will still flag it if present. To test our theory we monkeypatched the gem to allow a client_id through but that then resulted in another error:

ActionView::Template::Error (undefined method `client' for nil:NilClass):
<main role="main">
   <p class="h4">
     <%= raw t('.prompt', client_name: content_tag(:strong, class: 'text-info') { @pre_auth.client.name }) %>
   </p>

  <% if @pre_auth.scopes.count > 0 %>

At this point we got a bit lost as we don't know the codebase very well and couldn't track down the issue.

Any insight/pointers/sanity checks that could help us track it down?

[nbulaj]

Hi @georgepalmer . Thanks for reporting the issue!
I don't have a quick answer, I'm also need to check what's going on in the controller and with params. I let you know if I find something

[nbulaj]

Hi @georgepalmer . Could you please check it with rc1 and rc2 version of 5.2 Doorkeeper ? I don't sure it's actually a strong params issue, maybe some other change introduced the problem.

[georgepalmer]

@nbulaj I've just tested and rc1 is fine but rc2 has the issue

[georgepalmer]

@nbulaj can you reproduce from the steps above? Or do you nee more info? It's just entering the authorization url as outlined in step 2 of the wiki page into a browser and it'll hit the issue

[nbulaj]

It would be great to have a feature test (cucumber) if it's possible. I don't have a time to setup a project and test it right now :(

[nbulaj]

I take a look a little bit deeper. PreAuth doesn't need client_id in params as if has client instance passed on initialization from the Doorkeeper server: https://github.com/doorkeeper-gem/doorkeeper/blob/master/app/controllers/doorkeeper/authorizations_controller.rb#L70

Now it looks like client_id missed for your request or maybe something else? It's hard to say

[georgepalmer]

Yeah so it grabs client_id from there but that doesn't fix the strong params issue. If you look at 03b1437 you'll see the permit call doesn't allow client_id. So if you submit a url with a client_id then it blows up as that strong param call will have a client_id in the params

[nbulaj]

@georgepalmer can we have a sample app or Capybara feature spec to research the issue?

[georgepalmer]

I setup a new sample app to eliminate anything we were doing and also to create as minimal test case as possible. This highlighted I'm seeing the error because we run with the rails setting which I guess is why you've missed it:

config.action_controller.action_on_unpermitted_parameters = :raise

This throws an exception when extra params are present rather than silently swallowing it.

Steps to reproduce
Step 1: Clone app from https://github.com/georgepalmer/doorkeeper-example-app
Step 2: Do a bundle install
Step 3: Create an oauth app - http://localhost:3000/oauth/applications/new Untick confidential and leave scope empty. Any name and redirect uri will do
Step 4: Request an oauth password authorisatoin subbing in the client_id of the application you've just created: http://localhost:3000/oauth/authorize?client_id=...&redirect_uri=urn:ietf:wg:oauth:2.0:oob&response_type=code

You'll get the params error

[nbulaj]

Hi @georgepalmer ! Could you please check (and possible trace) your application with the latest master version of Doorkeeper? We put some fix in #1296

[georgepalmer]

It didn't but as the complex part was done I was able to code a fix - see #1298 With this everything works as expected

[nbulaj]

Does this issue still reproduce @georgepalmer ?

[georgepalmer]

Sorry should have closed this off with the PR. All good now!