From 666d473b6c4b8196cb562f4452242a80a31ea8f8 Mon Sep 17 00:00:00 2001
From: Nikita Bulai <bulaj.nikita@gmail.com>
Date: Mon, 1 Apr 2019 12:33:47 +0300
Subject: [PATCH] Improve blank redirect URI validation and config

---
 app/validators/redirect_uri_validator.rb      |  4 +--
 lib/doorkeeper/config.rb                      | 20 +++++++++++---
 .../doorkeeper/templates/initializer.rb       | 16 +++++++++++
 spec/models/doorkeeper/application_spec.rb    | 15 +++++++++++
 .../validators/redirect_uri_validator_spec.rb | 27 +++++++++++++++++++
 5 files changed, 76 insertions(+), 6 deletions(-)

diff --git a/app/validators/redirect_uri_validator.rb b/app/validators/redirect_uri_validator.rb
index 4e9fcb7d9..462f9d266 100644
--- a/app/validators/redirect_uri_validator.rb
+++ b/app/validators/redirect_uri_validator.rb
@@ -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|
diff --git a/lib/doorkeeper/config.rb b/lib/doorkeeper/config.rb
index 7b2361137..9e4468026 100644
--- a/lib/doorkeeper/config.rb
+++ b/lib/doorkeeper/config.rb
@@ -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,
@@ -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
diff --git a/lib/generators/doorkeeper/templates/initializer.rb b/lib/generators/doorkeeper/templates/initializer.rb
index 51076e500..a7b864a9c 100644
--- a/lib/generators/doorkeeper/templates/initializer.rb
+++ b/lib/generators/doorkeeper/templates/initializer.rb
@@ -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.
diff --git a/spec/models/doorkeeper/application_spec.rb b/spec/models/doorkeeper/application_spec.rb
index 667c87fd1..7e9d28884 100644
--- a/spec/models/doorkeeper/application_spec.rb
+++ b/spec/models/doorkeeper/application_spec.rb
@@ -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
diff --git a/spec/validators/redirect_uri_validator_spec.rb b/spec/validators/redirect_uri_validator_spec.rb
index 44093239b..40ec09ed8 100644
--- a/spec/validators/redirect_uri_validator_spec.rb
+++ b/spec/validators/redirect_uri_validator_spec.rb
@@ -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