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.

Sign up for GitHub

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

Jump to bottom

Defer to Doorkeeper when scopes are missing #79

Closed
wants to merge 7 commits into from
Closed

Defer to Doorkeeper when scopes are missing #79

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
578fc0e
Update README.md
Jul 16, 2019
125a86f
Update README.md
Jul 16, 2019
90c7549
Add handling for invalid parameters in controller helper
Jul 16, 2019
76f8011
Merge branch 'fix_invalid_parameter_500'
Jul 16, 2019
f842e22
Defer to doorkeeper's default behavior with invalid params
Jul 23, 2019
d8a6e3d
Refactor to accomodate old behavior and desired behavior
Jul 23, 2019
38b129e
Merge pull request #1 from mavenlink/defer-to-doorkeeper-on-invalid-p…
urnf Jul 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions lib/doorkeeper/openid_connect/helpers/controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,34 @@ module Controller

def authenticate_resource_owner!
super.tap do |owner|
next unless controller_path == Doorkeeper::Rails::Routes.mapping[:authorizations][:controllers] &&
action_name == 'new'
next unless pre_auth.scopes.include?('openid')
next unless valid_controller_path?
next unless scopes.include?('openid')

handle_prompt_param!(owner)
handle_max_age_param!(owner)
end
rescue Errors::OpenidConnectError => exception
handle_error(exception)
end

def valid_controller_path?
controller_path == Doorkeeper::Rails::Routes.mapping[:authorizations][:controllers] && action_name == 'new'
end

def scopes
pre_auth.scopes
rescue NoMethodError
Copy link

Choose a reason for hiding this comment

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

To be clear - this PR is obviously meant to raise questions and NOT to be merged - we're curious if we need to guarantee the validity of pre_auth after resource_owner_authenticator is run. Are we supposed to verify that pre_auth is valid with a valid client before we get to this point?

If not, calling scopes can attempt to build_scopes in the PreAuthorization with a nil client.

Copy link
Member

Choose a reason for hiding this comment

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

we're curious if we need to guarantee the validity of pre_auth after resource_owner_authenticator is run

Hmm not sure what you mean, can you rephrase?

Copy link

Choose a reason for hiding this comment

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

Thanks for looking at this @toupeira !

I might be missing something about this, so correct me if I'm wrong with my assumptions:

  • Doorkeeper::OpenIDConnect::Helpers::Controller overrides base Doorkeeper's authenticate_resource_owner!, calls it (with super), opens the resource owner returned, and runs OIDC specific behavior (prompt analysis + max_age check)
  • In order to do so, it currently requires that pre_auth.client is valid. Every other part of the PreAuthorization can be missing - redirect_uri, scopes, and response_type can be nil and it'd still work fine because only scopes is used in this context and scopes in PreAuthorization does a client lookup if it's missing.
  • Doorkeeper's authenticate_resource_owner! is calling doorkeeper config resource_owner_authenticator

Because of that:

  • Would our resource_owner_authenticator need to validate pre_auth.client? so that by the time we drop into Doorkeeper::OpenIDConnect::Helpers::Controller:authenticate_resource_owner! we guarantee that pre_auth.scopes doesn't raise an exception trying to grab client if scopes doesn't exist.

I noticed that #80 puts it back in, so I think that answers the question.

That being said, I'm wondering if it's worth checking pre_auth.valid? rather than specifically pre_auth.client.

Would it make sense to ensure that response_type, client, redirect_uri are valid before we take action (they seem required in the OIDC RFC?) or is it something you want to leave to the base Doorkeeper gem itself when we finish and it drops down to the authorized_applications_controller or authorizations_controller?

Let me know if it's a concern I should put on that PR. Thanks!

[]
end

def handle_error(exception = nil)
# clear the previous response body to avoid a DoubleRenderError
self.response_body = nil

# FIXME: workaround for Rails 5, see https://github.com/rails/rails/issues/25106
@_response_body = nil

error_response = if pre_auth.valid?
error_response = if pre_auth.valid? && exception.present?
::Doorkeeper::OAuth::ErrorResponse.new(
name: exception.error_name,
state: params[:state],
Expand Down
7 changes: 7 additions & 0 deletions spec/controllers/doorkeeper/authorizations_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,4 +262,11 @@ def reauthenticate!
end
end
end

describe 'when params are missing' do
it 'skips over the openid behavior' do
authorize!({ scope: nil, current_user: nil, client_id: nil })
expect(response.status).to be(302)
end
end
end