-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Shouldn't controllers inherit Doorkeeper::ApplicationMetalController
?
#169
Comments
sato11
added a commit
to sato11/doorkeeper-openid_connect
that referenced
this issue
Jul 6, 2022
This closes doorkeeper-gem#169.
sato11
added a commit
to sato11/doorkeeper-openid_connect
that referenced
this issue
Jul 6, 2022
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 🙂 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 ofDoorkeeper::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
andDoorkeeper::TokenInfoController
choose to inherit MetalController: doorkeeper-gem/doorkeeper#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:
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 removingskip_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 ofunless
andskip_before_action
.With this change we can finally give grounds for patching. Concerning #166 I have a thing like this in mind:
The text was updated successfully, but these errors were encountered: