Skip to content

Commit

Permalink
Improve blank redirect URI validation and config
Browse files Browse the repository at this point in the history
  • Loading branch information
nbulaj committed Apr 1, 2019
1 parent 119386a commit 666d473
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 6 deletions.
4 changes: 2 additions & 2 deletions app/validators/redirect_uri_validator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ def self.native_redirect_uri
end

def validate_each(record, attribute, value)
return if Doorkeeper.configuration.allow_blank_redirect_uri?

if value.blank?
return if Doorkeeper.configuration.allow_blank_redirect_uri?(record)

record.errors.add(attribute, :blank)
else
value.split.each do |val|
Expand Down
20 changes: 16 additions & 4 deletions lib/doorkeeper/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,15 @@ def configure_secrets_for(type, using:, fallback:)
option :base_controller,
default: "ActionController::Base"

# Allows to set blank redirect URIs for Applications in case
# server configured to use URI-less grant flows.
#
option :allow_blank_redirect_uri,
default: (lambda do |grant_flows, _application|
grant_flows.exclude?("authorization_code") &&
grant_flows.exclude?("implicit")
end)

attr_reader :api_only,
:enforce_content_type,
:reuse_access_token,
Expand Down Expand Up @@ -405,13 +414,16 @@ def token_grant_types
@token_grant_types ||= calculate_token_grant_types.freeze
end

def allow_blank_redirect_uri?
grant_flows.exclude?("authorization_code") &&
grant_flows.exclude?("implicit")
def allow_blank_redirect_uri?(application = nil)
if allow_blank_redirect_uri.respond_to?(:call)
allow_blank_redirect_uri.call(grant_flows, application)
else
allow_blank_redirect_uri
end
end

def option_defined?(name)
!instance_variable_get("@#{name}").nil?
instance_variable_defined?("@#{name}")
end

private
Expand Down
16 changes: 16 additions & 0 deletions lib/generators/doorkeeper/templates/initializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,22 @@
#
# forbid_redirect_uri { |uri| uri.scheme.to_s.downcase == 'javascript' }

# Allows to set blank redirect URIs for Applications in case Doorkeeper configured
# to use URI-less OAuth grant flows like Client Credentials or Resource Owner
# Password Credentials. The option is on by default and checks configured grant
# types, but you **need** to manually drop `NOT NULL` constraint from `redirect_uri`
# column for `oauth_applications` database table.
#
# You can completely disable this feature with:
#
# allow_blank_redirect_uri false
#
# Or you can define your custom check:
#
# allow_blank_redirect_uri do |grant_flows, client|
# client.superapp?
# end

# Specify how authorization errors should be handled.
# By default, doorkeeper renders json errors when access token
# is invalid, expired, revoked or has invalid scopes.
Expand Down
15 changes: 15 additions & 0 deletions spec/models/doorkeeper/application_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,21 @@ module Doorkeeper
expect(new_application).not_to be_valid
end
end

context "when blank URI option disabled" do
before do
Doorkeeper.configure do
grant_flows %w[password client_credentials]
allow_blank_redirect_uri false
end
end

it "is invalid without redirect_uri" do
new_application.save
new_application.redirect_uri = nil
expect(new_application).not_to be_valid
end
end
end

it "checks uniqueness of uid" do
Expand Down
27 changes: 27 additions & 0 deletions spec/validators/redirect_uri_validator_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -128,4 +128,31 @@
expect(subject).to be_invalid
end
end

context "blank redirect URI" do
it "forbids blank redirect uri by default" do
subject.redirect_uri = ""

expect(subject).to be_invalid
expect(subject.errors[:redirect_uri]).not_to be_blank
end

it "forbids blank redirect uri by custom condition" do
Doorkeeper.configure do
orm DOORKEEPER_ORM
allow_blank_redirect_uri do |_grant_flows, application|
application.name == "admin app"
end
end

subject.name = "test app"
subject.redirect_uri = ""

expect(subject).to be_invalid
expect(subject.errors[:redirect_uri]).not_to be_blank

subject.name = "admin app"
expect(subject).to be_valid
end
end
end

0 comments on commit 666d473

Please sign in to comment.