Merge pull request #1237 from doorkeeper-gem/optional_redirect_uri

Fix #947: Allow blank redirect URI for URI-less grant flows

NEWS.md

@@ -7,6 +7,7 @@ User-visible changes worth mentioning.
 
 ## master
 
+- [#1237] Allow to set blank redirect URI if Doorkeeper configured to use redirect URI-less grant flows.
 - [#1234] Fix `StaleRecordsCleaner` to properly work with big amount of records.
 - [#1228] Allow to explicitly set non-expiring tokens in `custom_access_token_expires_in` configuration
   option using `Float::INIFINITY` return value.

app/validators/redirect_uri_validator.rb

@@ -8,6 +8,8 @@ def self.native_redirect_uri
   end
 
   def validate_each(record, attribute, value)
+    return if Doorkeeper.configuration.allow_blank_redirect_uri?
+
     if value.blank?
       record.errors.add(attribute, :blank)
     else

app/views/doorkeeper/applications/_form.html.erb

@@ -25,6 +25,12 @@
             <%= raw t('doorkeeper.applications.help.native_redirect_uri', native_redirect_uri: content_tag(:code, class: 'bg-light') { Doorkeeper.configuration.native_redirect_uri }) %>
           </span>
       <% end %>
+
+      <% if Doorkeeper.configuration.allow_blank_redirect_uri? %>
+        <span class="form-text text-secondary">
+          <%= t('doorkeeper.applications.help.blank_redirect_uri') %>
+        </span>
+      <% end %>
     </div>
   </div>
 

config/locales/en.yml

@@ -32,6 +32,7 @@ en:
       help:
         confidential: 'Application will be used where the client secret can be kept confidential. Native mobile apps and Single Page Apps are considered non-confidential.'
         redirect_uri: 'Use one line per URI'
+        blank_redirect_uri: "Leave it blank if you configured your provider to use Client Credentials, Resource Owner Password Credentials or any other grant type that doesn't require redirect URI."
         native_redirect_uri: 'Use %{native_redirect_uri} if you want to add localhost URIs for development purposes'
         scopes: 'Separate scopes with spaces. Leave blank to use the default scopes.'
       edit:

lib/doorkeeper/config.rb

@@ -405,6 +405,11 @@ 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")
+    end
+
     def option_defined?(name)
       !instance_variable_get("@#{name}").nil?
     end

lib/generators/doorkeeper/application_owner_generator.rb

@@ -4,6 +4,9 @@
 require "rails/generators/active_record"
 
 module Doorkeeper
+  # Generates migration to add reference to owner of the
+  # Doorkeeper application.
+  #
   class ApplicationOwnerGenerator < ::Rails::Generators::Base
     include ::Rails::Generators::Migration
     source_root File.expand_path("templates", __dir__)

lib/generators/doorkeeper/confidential_applications_generator.rb

@@ -4,6 +4,9 @@
 require "rails/generators/active_record"
 
 module Doorkeeper
+  # Generates migration to add confidential column to Doorkeeper
+  # applications table.
+  #
   class ConfidentialApplicationsGenerator < ::Rails::Generators::Base
     include ::Rails::Generators::Migration
     source_root File.expand_path("templates", __dir__)

lib/generators/doorkeeper/install_generator.rb

@@ -4,6 +4,8 @@
 require "rails/generators/active_record"
 
 module Doorkeeper
+  # Setup doorkeeper into Rails application: locales, routes, etc.
+  #
   class InstallGenerator < ::Rails::Generators::Base
     include ::Rails::Generators::Migration
     source_root File.expand_path("templates", __dir__)

lib/generators/doorkeeper/migration_generator.rb

@@ -4,6 +4,8 @@
 require "rails/generators/active_record"
 
 module Doorkeeper
+  # Copies main Doorkeeper migration into parent Rails application.
+  #
   class MigrationGenerator < ::Rails::Generators::Base
     include ::Rails::Generators::Migration
     source_root File.expand_path("templates", __dir__)

lib/generators/doorkeeper/pkce_generator.rb

@@ -4,6 +4,9 @@
 require "rails/generators/active_record"
 
 module Doorkeeper
+  # Generates migration with PKCE required database columns for
+  # Doorkeeper tables.
+  #
   class PkceGenerator < ::Rails::Generators::Base
     include ::Rails::Generators::Migration
     source_root File.expand_path("templates", __dir__)

lib/generators/doorkeeper/previous_refresh_token_generator.rb

@@ -4,6 +4,9 @@
 require "rails/generators/active_record"
 
 module Doorkeeper
+  # Generates migration to add previous refresh token column to the
+  # database for Doorkeeper tables.
+  #
   class PreviousRefreshTokenGenerator < ::Rails::Generators::Base
     include ::Rails::Generators::Migration
     source_root File.expand_path("templates", __dir__)

lib/generators/doorkeeper/templates/migration.rb.erb

@@ -4,6 +4,10 @@ class CreateDoorkeeperTables < ActiveRecord::Migration<%= migration_version %>
       t.string  :name,         null: false
       t.string  :uid,          null: false
       t.string  :secret,       null: false
+
+      # Remove `null: false` if you are planning to use grant flows
+      # that doesn't require redirect URI to be used during authorization
+      # like Client Credentials flow or Resource Owner Password.
       t.text    :redirect_uri, null: false
       t.string  :scopes,       null: false, default: ''
       t.boolean :confidential, null: false, default: true

lib/generators/doorkeeper/views_generator.rb

@@ -2,6 +2,8 @@
 
 module Doorkeeper
   module Generators
+    # Generates doorkeeper views for Rails application
+    #
     class ViewsGenerator < ::Rails::Generators::Base
       source_root File.expand_path("../../../app/views", __dir__)
 

spec/dummy/db/schema.rb

@@ -47,7 +47,7 @@
     t.string "name", null: false
     t.string "uid", null: false
     t.string "secret", null: false
-    t.text "redirect_uri", null: false
+    t.text "redirect_uri"
     t.string "scopes", default: "", null: false
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false

spec/lib/oauth/password_access_token_request_spec.rb

@@ -157,8 +157,6 @@ module Doorkeeper::OAuth
               222
             elsif context.scopes.exists?("magic")
               Float::INFINITY
-            else
-              nil
             end
           }
         )

spec/models/doorkeeper/application_spec.rb

@@ -82,10 +82,34 @@ module Doorkeeper
       expect(new_application).not_to be_valid
     end
 
-    it "is invalid without redirect_uri" do
-      new_application.save
-      new_application.redirect_uri = nil
-      expect(new_application).not_to be_valid
+    context "redirect URI" do
+      context "when grant flows allow blank redirect URI" do
+        before do
+          Doorkeeper.configure do
+            grant_flows %w[password client_credentials]
+          end
+        end
+
+        it "is valid without redirect_uri" do
+          new_application.save
+          new_application.redirect_uri = nil
+          expect(new_application).to be_valid
+        end
+      end
+
+      context "when grant flows require redirect URI" do
+        before do
+          Doorkeeper.configure do
+            grant_flows %w[password client_credentials authorization_code]
+          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

spec/requests/applications/applications_request_spec.rb

@@ -94,6 +94,36 @@
         true
       )
     end
+
+    context "redirect URI" do
+      scenario "adding app with blank redirect URI when configured flows requires redirect uri" do
+        config_is_set("grant_flows", %w[authorization_code implicit client_credentials])
+
+        fill_in "doorkeeper_application[name]", with: "My Application"
+        fill_in "doorkeeper_application[redirect_uri]",
+                with: ""
+
+        click_button "Submit"
+        i_should_see "Whoops! Check your form for possible errors"
+      end
+
+      scenario "adding app with blank redirect URI when configured flows without redirect uri" do
+        config_is_set("grant_flows", %w[client_credentials password])
+
+        # Visit it once again to consider grant flows
+        visit "/oauth/applications/new"
+
+        i_should_see I18n.t("doorkeeper.applications.help.blank_redirect_uri")
+
+        fill_in "doorkeeper_application[name]", with: "My Application"
+        fill_in "doorkeeper_application[redirect_uri]",
+                with: ""
+
+        click_button "Submit"
+        i_should_see "Application created"
+        i_should_see "My Application"
+      end
+    end
   end
 end
 

spec/requests/flows/password_spec.rb

@@ -18,7 +18,7 @@
 end
 
 describe "Resource Owner Password Credentials Flow" do
-  let(:client_attributes) { {} }
+  let(:client_attributes) { { redirect_uri: nil } }
 
   before do
     config_is_set(:grant_flows, ["password"])