janko / rodauth-rails

Rails integration for Rodauth authentication framework
https://github.com/jeremyevans/rodauth
MIT License
593 stars 40 forks source link

Unexpected response content type #272

Closed baptistejub closed 8 months ago

baptistejub commented 8 months ago

Hello,

We've recently moved our (basic) auth step from the rails controller into the rodauth middleware and we've noticed a change in the response content type.

When given a HTTP Accept header and a path extension (.xml, .json...), with rodauth rails authenticating in its middleware (with basic auth), we've noticed it is the Accept header that is used to determine the response content type. Without auth or with auth in rails controller, it would use the extension in priority.

For exemple, with Accept: */* and GET /my_data.xml, the detected request format is */* so it returns the first format block of "respond_to" from the rails controller instead of XML (without rodauth auth, rails would return some XML as expected).

After a little digging, we've found out that, during the auth process in rodauth rails middleware, the session is updated, thus calling the clear_session method which instantiates a rails controller instance that parses and sets the request formats from the Accept header here https://github.com/janko/rodauth-rails/blob/main/lib/rodauth/rails/feature/render.rb#L51, because the path extension is not yet extracted at this point (request.parameters[:format] is nil).

We were expecting to have the same behaviour as vanilla rails (or auth in controller) when authenticating in the middleware.

What's do you think about this behaviour? Is there a misconfiguration on our side?

Right now we patched it by overriding the _rails_controller_instance method in our rodauth class (to use the method definition from lib/rodauth/rails/feature/base.rb).

Some reproduction steps (base on the current test suite):

# test/integration/request_format_test.rb
require "test_helper"

class RequestFormatTest < IntegrationTest
  test "unexpected" do
    page.driver.browser.get "/test_format.xml", { 'clear_session' => '1' }, { 'HTTP_ACCEPT' => '*/*' }
    # Uses the first format of the controller
    assert_match /JSON response/, page.body
  end

  test "expected" do
    page.driver.browser.get "/test_format.xml", {}, { 'HTTP_ACCEPT' => '*/*' }
    assert_match /XML response/, page.body
  end
end

# test/rails_app/app/misc/rodauth_app.rb
class RodauthApp < Rodauth::Rails::App
  # [...]
  route do |r|
    # [...]
    if r.path.start_with?(rails_routes.test_format_path) && request.params['clear_session'] == '1'
      # Mock the action of auth that updates the session (like basic auth) and calls rails_controller_instance
      rodauth.clear_session
    end
  end
end

# test/rails_app/app/controllers/test_controller.rb
class TestController < ApplicationController
  def test_format
    respond_to do |format|
      format.json { render json: 'JSON response' }
      format.xml { render xml: '<test>XML response</xml>' }
    end
  end
end

# test/rails_app/config/routes.rb
Rails.application.routes.draw do
  controller :test do
    # [...]
    get :test_format
  end
end
janko commented 8 months ago

Thanks for the report. I believe this is a bug in rodauth-rails, I didn't test whether paths with extensions still work.

One solution would be for rodauth-rails to set the formats only before rendering inside Rodauth, because that's when they're needed. We shouldn't be modifying anything if Rails is the one serving requests.

Could you post your actual RodauthApp, just so that I know what is getting called in the route block other than r.rodauth? I'm not sure in which scenario it makes sense to clear the session while serving a Rails route (as opposed to a Rodauth route).

janko commented 8 months ago

OK, after re-reading the report, I understand now that you're using the http_basic_auth feature, and probably calling rodauth.http_basic_auth in the route block. I'm working on a fix.

baptistejub commented 8 months ago

Thanks for the quick response. It's indeed when using http_basic_auth.

Here a very simple RodauthApp to reproduce (if you still need it):

require 'sequel/core'

class RodauthApp < Rodauth::Rails::App
  plugin :pass

  configure do
    enable :login, :logout, :session_expiration, :account_expiration, :bearer_auth, :http_basic_auth, :internal_request

    db Sequel.connect(
      "#{ActiveRecord::Base.connection_db_config.adapter}://",
      extensions: :activerecord_connection
    )

    login_column :username
  end

  route do |r|
    rodauth.require_http_basic_auth
  end
end

and a controller

class TestsController < ApplicationController
  def index
    respond_to do |format|
      format.json { render json: { message: 'json' } }
      format.xml { render xml: '<test>xml</test>' }
      format.any { render plain: 'plain' }
    end
  end
end

A some curl examples

# returns json
curl --location 'localhost:3000/tests.xml' --header 'Accept: */*' --header 'Authorization: Basic xxxxxx'

# returns xml
curl --location 'localhost:3000/tests.xml' --header 'Authorization: Basic xxxxxx'
janko commented 8 months ago

I believe the issue should be addressed with the latest commit. Thanks to you already identifying the root cause, I could just focus on the fix 🙂

I plan to cut a new release as soon as a new Rodauth version is released, which should hopefully be any day now 🤞🏻

baptistejub commented 8 months ago

Thanks a lot! I can confirm it's working after testing with the main branch 🎉