Skip to content
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

Move hidden fields from view to pre_authorization, for supporting other parameters. #420

Closed

Conversation

takada-s
Copy link

Currently, authorizations/new view has hardwired hidden_field_tags, used to pass parameters, such as scope and state, between authN and authZ.

I also trying to support OpenID Connect on this Doorkeeper, so I want to pass a new parameter nonce.
Using my changes, we can easily pass the new parameter and can avoid overriding/repeating code.

@@ -36,6 +36,16 @@ def error_response
Doorkeeper::OAuth::ErrorResponse.from_request(self)
end

def hidden_fields
{
client_id: client.uid,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.

@tute
Copy link
Contributor

tute commented Jul 6, 2014

With this change you'd need to override/monkey patch Doorkeeper::OAuth::PreAuthorization#hidden_fields. It would be easier to override the view, using the standard Rails practice of recreating the view tree in the app folder. Doorkeeper has a generator for this:

$ rails g doorkeeper:views
      create  app/views/doorkeeper/applications
      create  app/views/doorkeeper/applications/_delete_form.html.erb
      create  app/views/doorkeeper/applications/_form.html.erb
      create  app/views/doorkeeper/applications/edit.html.erb
      create  app/views/doorkeeper/applications/index.html.erb
      create  app/views/doorkeeper/applications/new.html.erb
      ...

Closing for now, willing to continue discussion for sure. Thank you for your work and willing to share!

@toupeira
Copy link
Member

toupeira commented Nov 2, 2016

@tute follow-up to my comment in #897: I've now implemented nonces in doorkeeper-openid_connect by extending certain Doorkeeper classes (switching to prepend so I can use super).

So since I'm already monkey-patching, the current solution in this PR seems acceptable to me, and I think overriding the view would only make things more complicated for end-users of my gem. Would you be willing to merge this as is?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants