Add config to enforce content type #1076

baxang · 2018-04-08T14:25:18Z

Summary

TODOs:

Fix on Rails 4.2
Improve test code so that HTTP action methods work with headers
Add the config to initializer template
Update NEWS

houndci-bot · 2018-04-08T14:25:28Z

spec/controllers/application_metal_controller_spec.rb

@@ -0,0 +1,48 @@
+require "spec_helper_integration"


Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.

houndci-bot · 2018-04-08T14:25:28Z

app/controllers/doorkeeper/application_metal_controller.rb

      Helpers::Controller
    ].freeze

    MODULES.each do |mod|
      include mod
    end

+    before_action :enforce_content_type, if: -> { Doorkeeper.configuration.enforce_content_type }


Metrics/LineLength: Line is too long. [97/80]

baxang · 2018-04-09T22:56:10Z

Hi @nbulaj and @hallucinations,

Although this PR needs some more work, I think it is ready for someone else' review. Can you please take a look?

A few comments on my decisions are:

Config API

I made the new config API enforce_content_type similar to api_only. The previous behaviour (allowing non-urlencoded-form media type) is preserved if not used.

Since many people, including myself, are already running Doorkeeper in production, I thought about more options than simply turning it on/off. For example, an option to log/collect metrics on content type so systems admins can understand their traffic. However I ended up thinking - ok let's just make it simple.

Testing

I had some tricky problems in writing tests to support both Rails 4 and 5. There's some HTTP method shims for Rails 5 and the shims are written as if no headers are passed to them.

I ended up having some not pretty test code like below.

it '200 for the correct media type' do
  @request.headers['Content-Type'] = 'application/x-www-form-urlencoded'
  get :index, {}, as: :url_encoded_form
  expect(response).to have_http_status 200
end

@request.headers... is for Rails 4 testing and as: :url_encoded_form is for Rails 5. Maybe I should spend some more time to make it more elegant. What do you think?

`charset=utf-8`

As I said in #1067 I think it is reasonable to skip charset checking.

Regards,

nbulaj · 2018-04-10T07:14:05Z

I made the new config API enforce_content_type

This one good 👍

For example, an option to log/collect metrics on content type so systems admins can understand their traffic. However I ended up thinking - ok let's just make it simple.

I think every production system has some additional services/tools for log analysis (graphana, ELK, NewRelic, etc). So they already have an ability to find useful info about content type in the requests. If not - well, they need to use it :)

There's some HTTP method shims for Rails 5 and the shims are written as if no headers are passed to them.

So let's fix them to work with headers :) In this case tests will look good.

We can do something like this (for Rails <= 5):

module ControllerHTTPMethodShim
  def get(path, params = {}, headers = nil)
    # Set request headers for :controller specs
    if headers.present?
      headers.each { |key, value| request.headers[key.to_s] = value  }
    end
    
    super(path, params: params)
  end

  # ...
end

As I said in #1067 I think it is reasonable to skip charset checking.

Absolutely agree.

Interesting thing: Hydra OAuth2 provider (Golang) supports both application/json and application/x-www-form-urlencoded for the requests https://github.com/ory/hydra/blob/366ed57d9c39d7601a40b5545f91361e6a2b9f5a/doc.go#L33 .

baxang · 2018-04-10T08:11:19Z

@nbulaj Thanks for your review.

I will try to make test code nicer as well the rest of TODO items.

And regarding Hyda, it's interesting that I found an issue that rejected a feature request to support JSON: ory/hydra#786. It might be the case JSON is used for other endpoints that are not part of OAuth 2.0 but I'm just guessing here.

houndci-bot · 2018-04-16T12:17:59Z

spec/controllers/applications_controller_spec.rb

+               params: {
+                 doorkeeper_application: {
+                   name: 'Example',
+                   redirect_uri: 'https://example.com' }


Layout/MultilineHashBraceLayout: Closing hash brace must be on the line after the last hash element when opening brace is on a separate line from the first hash element.

houndci-bot · 2018-04-16T12:17:59Z

spec/controllers/applications_controller_spec.rb

+               params: {
+                 doorkeeper_application: {
+                   name: 'Example',
+                   redirect_uri: 'https://example.com' }


Layout/MultilineHashBraceLayout: Closing hash brace must be on the line after the last hash element when opening brace is on a separate line from the first hash element.

baxang · 2018-04-16T12:35:56Z

Hi @nbulaj I've added a couple commits to improve test code a bit.

What I've done is basically I inverted Rails 4 -> 5 syntax conversion: b6c2227#diff-3b61ca35a7f47da3a2dbf21b5b9e3dd0

I think doing this is better for a few reasons:

Rails 5 is the current stable so using its syntax makes sense
Rails 5 syntax is more clearer with keyword arguments
Importantly :-) it is easier to convert 5 -> 4

I'm still to fix CI failures, but before I spend more time I'd love to hear your thoughts.

nbulaj · 2018-04-18T07:44:57Z

Hi @baxang . Totally agree: in future versions we will drop support of Rails 4.x, so it is reasonable to use actual syntax. Feel free to continue 👍

baxang · 2018-04-20T12:16:03Z

Test fails on all non-rails-5 versions and can't reproduce on local machine :-( Any advice would be appreciated. I will keep looking.

nbulaj · 2018-04-20T14:01:43Z

spec/support/http_method_shim.rb

+    if as = args.delete(:as)
+      @request.headers['Content-Type'] = Mime[as].to_s
+    end
+    super(action, http_method, args[:params], args[:session], args[:flash])


What about headers?

This is calling Rails 4's and in Rails 4 adding headers is not supported.

nbulaj · 2018-04-20T14:42:26Z

I cloned your repo and executed rspec from the rails_4_2 bundle and everything is OK O_o

baxang · 2018-04-22T13:00:07Z

Thanks for your effort and you're right 😕 that's what I'm experiencing too. What even further confuses me is CI errors.

Based on CI error log (from Ruby 2.4/Rails 4.2 https://travis-ci.org/doorkeeper-gem/doorkeeper/jobs/367131874 ) the first error reported is
https://travis-ci.org/doorkeeper-gem/doorkeeper/jobs/367131874#L576
https://travis-ci.org/doorkeeper-gem/doorkeeper/jobs/367131874#L637

which is

rspec ./spec/controllers/authorizations_controller_spec.rb:207 # Doorkeeper::AuthorizationsController implicit grant flow POST #create with callbacks when successful should call :before_successful_authorization callback

And the line is https://github.com/baxang/doorkeeper/blob/1067-enforce-strict-content-type/spec/controllers/authorizations_controller_spec.rb#L207 which does not look related to the failed spec I have in the log.

I'll keep digging

nbulaj · 2018-04-27T21:12:08Z

Any news here?

houndci-bot · 2018-04-29T11:38:22Z

lib/generators/doorkeeper/templates/initializer.rb

@@ -25,6 +25,11 @@
  #
  # api_only

+  # Enforce token request content type strictly to application/x-www-form-urlencoded.


Metrics/LineLength: Line is too long. [85/80]

baxang · 2018-04-29T13:02:11Z

I could finally reproduce the errors with upstream master merged into my branch - which still does not explain failures on CI though. Maybe there's something that I don't know about how Travis CI runs tests.

Anyways the errors were caused by a couple specs recently added to master branch which obviously used Rails 4 post syntax. Easy fix and all looks good now.

baxang · 2018-05-01T10:48:27Z

Let me know if you want me to squash commits

nbulaj · 2018-05-01T11:28:08Z

Yes, could you please squash them? I think everything is ready to be merged

…ed. (Fixes #1067)

baxang · 2018-05-01T13:09:24Z

No problem - I've squashed and pushed. Just waiting for Travis to turn green 💚 Thanks for your guidance. Can't wait to see 5.0 to out!

nbulaj · 2018-05-01T17:06:03Z

Thank you @baxang for your work and patience! 👍

nbulaj · 2018-05-30T12:07:30Z

Hi @baxang . I've added format: :json parameter for controller tests and it's failing for JSON API's. Could you please take a look? https://travis-ci.org/doorkeeper-gem/doorkeeper/jobs/385600167 Currently I'm looking how to pass this to original method

Nevermind, fixed :)

houndci-bot reviewed Apr 8, 2018

View reviewed changes

nbulaj added this to the 5.0 milestone Apr 9, 2018

nbulaj added the enhancement label Apr 10, 2018

nbulaj self-assigned this Apr 10, 2018

houndci-bot reviewed Apr 16, 2018

View reviewed changes

nbulaj reviewed Apr 20, 2018

View reviewed changes

houndci-bot reviewed Apr 29, 2018

View reviewed changes

Add config to enforce content type to application/x-www-form-urlencod…

d6069f1

…ed. (Fixes #1067)

nbulaj merged commit 84bc70a into doorkeeper-gem:master May 1, 2018

baxang deleted the 1067-enforce-strict-content-type branch May 30, 2018 15:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config to enforce content type #1076

Add config to enforce content type #1076

baxang commented Apr 8, 2018 •

edited

Loading

houndci-bot Apr 8, 2018

houndci-bot Apr 8, 2018

baxang commented Apr 9, 2018

nbulaj commented Apr 10, 2018 •

edited

Loading

baxang commented Apr 10, 2018

houndci-bot Apr 16, 2018

houndci-bot Apr 16, 2018

baxang commented Apr 16, 2018

nbulaj commented Apr 18, 2018

baxang commented Apr 20, 2018

nbulaj Apr 20, 2018

baxang Apr 22, 2018

nbulaj commented Apr 20, 2018

baxang commented Apr 22, 2018

nbulaj commented Apr 27, 2018

houndci-bot Apr 29, 2018

baxang commented Apr 29, 2018

baxang commented May 1, 2018

nbulaj commented May 1, 2018

baxang commented May 1, 2018

nbulaj commented May 1, 2018

nbulaj commented May 30, 2018 •

edited

Loading

Add config to enforce content type #1076

Add config to enforce content type #1076

Conversation

baxang commented Apr 8, 2018 • edited Loading

Summary

houndci-bot Apr 8, 2018

Choose a reason for hiding this comment

houndci-bot Apr 8, 2018

Choose a reason for hiding this comment

baxang commented Apr 9, 2018

Config API

Testing

charset=utf-8

nbulaj commented Apr 10, 2018 • edited Loading

baxang commented Apr 10, 2018

houndci-bot Apr 16, 2018

Choose a reason for hiding this comment

houndci-bot Apr 16, 2018

Choose a reason for hiding this comment

baxang commented Apr 16, 2018

nbulaj commented Apr 18, 2018

baxang commented Apr 20, 2018

nbulaj Apr 20, 2018

Choose a reason for hiding this comment

baxang Apr 22, 2018

Choose a reason for hiding this comment

nbulaj commented Apr 20, 2018

baxang commented Apr 22, 2018

nbulaj commented Apr 27, 2018

houndci-bot Apr 29, 2018

Choose a reason for hiding this comment

baxang commented Apr 29, 2018

baxang commented May 1, 2018

nbulaj commented May 1, 2018

baxang commented May 1, 2018

nbulaj commented May 1, 2018

nbulaj commented May 30, 2018 • edited Loading

baxang commented Apr 8, 2018 •

edited

Loading

`charset=utf-8`

nbulaj commented Apr 10, 2018 •

edited

Loading

nbulaj commented May 30, 2018 •

edited

Loading