Shouldn't controllers inherit `Doorkeeper::ApplicationMetalController`?

[sato11]

Struggling to avoid #166 in our application I've found that patchable hook only exists in metal_controller, whereas controllers defined in this repo inherits from Doorkeeper::ApplicationController which is independent of Doorkeeper::ApplicationMetalController.

After skimming through doorkeeper's controller implementation I have a feeling that ApplicationController is for a dashboard and MetalController is for json endpoints. In fact Doorkeeper::TokensController and Doorkeeper::TokenInfoController choose to inherit MetalController: https://github.com/doorkeeper-gem/doorkeeper/pull/443.

Since both DiscoveryController and UserInfoController responds with application/json data, I feel ApplicationMetalController is of more fit for these as well. I would propose that we change like this:

diff --git a/app/controllers/doorkeeper/openid_connect/discovery_controller.rb b/app/controllers/doorkeeper/openid_connect/discovery_controller.rb
index e6a9a14..21f242c 100644
--- a/app/controllers/doorkeeper/openid_connect/discovery_controller.rb
+++ b/app/controllers/doorkeeper/openid_connect/discovery_controller.rb
@@ -2,7 +2,7 @@

 module Doorkeeper
   module OpenidConnect
-    class DiscoveryController < ::Doorkeeper::ApplicationController
+    class DiscoveryController < ::Doorkeeper::ApplicationMetalController
       include Doorkeeper::Helpers::Controller

       WEBFINGER_RELATION = 'http://openid.net/specs/connect/1.0/issuer'
diff --git a/app/controllers/doorkeeper/openid_connect/userinfo_controller.rb b/app/controllers/doorkeeper/openid_connect/userinfo_controller.rb
index 9f99024..c6ace1a 100644
--- a/app/controllers/doorkeeper/openid_connect/userinfo_controller.rb
+++ b/app/controllers/doorkeeper/openid_connect/userinfo_controller.rb
@@ -2,10 +2,7 @@

 module Doorkeeper
   module OpenidConnect
-    class UserinfoController < ::Doorkeeper::ApplicationController
-      unless Doorkeeper.configuration.api_only
-        skip_before_action :verify_authenticity_token
-      end
+    class UserinfoController < ::Doorkeeper::ApplicationMetalController
       before_action -> { doorkeeper_authorize! :openid }

       def show

Regarding verify_authenticity_token, it is only available for subclasses of ActionController::Base, so would no longer be useable when we change superclass. However I believe removing skip_before_action can be justified since its only reasoning is to avoid unintentional error due to the dependency to ApplicationController. Using MetalController can rather eradicate the double detours of unless and skip_before_action.

With this change we can finally give grounds for patching. Concerning #166 I have a thing like this in mind:

module Doorkeeper
  module OpenidConnect
    module MockOauthIntrospectUrl
      private

      def oauth_introspect_url(...)
        nil
      end
    end
  end
end

# config/initializers/doorkeeper_openid_connect.rb
ActiveSupport.on_load :doorkeeper_metal_controller do
  include Doorkeeper::OpenidConnect::MockOauthIntrospectUrl
end

[sato11]

I have worked it around by hooking not controllers but initializer like this:

# config/initializers/doorkeeper_openid_connect.rb
Rails.application.config.to_prepare do
  Doorkeeper::OpenidConnect::DiscoveryController.prepend Module.new {
    private

    def oauth_introspect_url(...)
      nil
    end
  }
end

While this solves my incidental motivation, the focal discussion remains relevant, so I'm leaving this open. Hope this could be of help if someone came across the same stuff 🙂