Add default content type for Policy load API endpoints

[micahlee]

What does this PR do?

If no content-type is specified in a POST request, Rack assumes the content
type is either application/x-www-form-urlencoded or
multipart/form-data.

The Conjur policy documents are YAML content, and some special
characters allowed in policy cause the Rack pody parsing to fail, leading to
a Rack error: Invalid or incomplete POST parameters.

This PR adds a Rack middleware that allow default content-types to be
configured on a per request path basis to better control the behavior when
no content-type is provided in the request headers.

What ticket does this PR close?

Connected to conjurinc/dap-support#74

Checklists

Change log

• The CHANGELOG has been updated, or
• This PR does not include user-facing changes and doesn't require a CHANGELOG update

Test coverage

• This PR includes new unit and integration tests to go with the code changes, or
• The changes in this PR do not require tests

Follow-on issues

• Follow-up issue(s) have been logged (and links included below) to update documentation or related projects, or
• No follow-up issues are required

[jonahx]

You should always avoid @@. And indeed it should be superfluous here. The receiving object, because of the self. qualifier, is the class itself. So @default_content_types would refer to a variable on the class object, not one of its instances.

[micahlee]

Interesting... I hadn't realized there was such a thing as a "class instance variable" distinct from a "class variable." Cool!

[jonahx]

This reek comments look valid and related to my comment above about @@. I'll try post a solution that avoid all this in a few minutes.

[jonahx]

Untested:

=begin

Currently we're doing:

  Rails.application.configure do
    config.middleware.use ::Rack::DefaultContentType
  end

Instead we'll do:

  Rails.application.configure do
    config.middleware.use(
      ::Rack::DefaultContentType,
      { %r{^\/policies} => 'application/x-yaml'}
    )
  end

And you can delete:

  def self.path_default_content_type(path_match, content_type)
    ::Rack::DefaultContentType.default_content_types[path_match] = content_type
  end

And replace it with a comment that the configuration is done in
config.application.rb, just so readers know there is some magic
afoot.

=end

module Rack


  # DefaultContentType is a Rack middleware that supports
  # providing a default Content-Type header for requests
  # to specific paths that do not include a Content-Type
  # header in the request.
  #
  # Default content types are configured by adding entries
  # to the Rack::DefaultContentType.default_content_types
  # class member hash.
  #
  # Each hash key must be a regular expression to test
  # request paths for a match. The hash value is the content
  # type string to add to the request.
  class DefaultContentType

    # A Hash-like whose keys are Regex objects. It looks up values by matching
    # keys given to its #[] method against those regexes, and returning the
    # value of the first match.
    class RegexHash
      # Note: this will work with a Hash or keyword args
      def initialize(**kwargs)
        @vals_by_regex = kwargs
      end

      def [](key)
        matching_re = @vals_by_regex.each_key.find { |re, v| re =~ key }
        @vals_by_regex[matching_re] # Note: returns nil if no match
      end
    end

    def initialize(app, content_type_by_path)
      @app = app
      @content_type_by_path = RegexHash.new(content_type_by_path)
    end

    def call(env)
      # Note: Avoid Rails dependency -- don't use `.present?`
      content_type_missing = env['CONTENT_TYPE']&.empty?
      default_content_type = @content_type_by_path[env['PATH_INFO']]

      # If a content type is already present on the request, we
      # don't need to do anything.
      if content_type_missing && default_content_type
        env['CONTENT_TYPE'] = default_content_type
        Rack::Request.new(env).body.rewind
      end

      @app.call(env)
    end
  end

end

[micahlee]

Avoid Rails dependency -- don't use .present?

I fine with the change, but we're in a Rails app. Why are we avoiding using it?

[micahlee]

And replace it with a comment that the configuration is done in
config.application.rb, just so readers know there is some magic
afoot.

I think I get where you're coming from with this suggestion. I still think, however, it's going to be easier to discover, understand, and maintain if the middleware only provides the application-wide capability to set default content types, with the path-specific settings configured closer to the path-specific logic (either the routes or the controller).

A global config with a comment is a weak connection with it comes to understanding what is happening when debugging later.

[jonahx]

I fine with the change, but we're in a Rails app. Why are we avoiding using it?

Because this is middleware, not part of the Rails app.

[jonahx]

A global config with a comment is a weak connection with it comes to understanding what is happening when debugging later.

I partially agree, but you're really choosing between lesser evils.

The downside of the solution you merged is that you're using mutation, and allowing the middleware to be mutated at runtime, from anywhere, at any time. This can give rise much trickier bugs than an immutable middleware that is statically configured one time. So yes, the way I originally suggested has the downside that the comment is deleted, and then it's not clear by looking locally in the controller what's happening. But I think this is unlikely to happen, especially if the comment says "Do not delete" :)

[codeclimate]

Code Climate has analyzed commit d19fc7e and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 85.4% (0.0% change).

View more on Code Climate.

[micahlee]

Thanks @jtuttle and @jonahx!

This is ready for another look when one of you has time again. Thanks!