From 4ac115d905cf2409faae16473c6b3e0daa1d314c Mon Sep 17 00:00:00 2001 From: Nikita Bulai Date: Thu, 17 Jan 2019 11:30:46 +0300 Subject: [PATCH] Fix #947: allow blank redirect URI for URI-less greant flows --- NEWS.md | 1 + app/validators/redirect_uri_validator.rb | 2 ++ .../doorkeeper/applications/_form.html.erb | 6 ++++ config/locales/en.yml | 1 + lib/doorkeeper/config.rb | 5 +++ .../doorkeeper/application_owner_generator.rb | 3 ++ .../confidential_applications_generator.rb | 3 ++ .../doorkeeper/install_generator.rb | 2 ++ .../doorkeeper/migration_generator.rb | 2 ++ lib/generators/doorkeeper/pkce_generator.rb | 3 ++ .../previous_refresh_token_generator.rb | 3 ++ .../doorkeeper/templates/migration.rb.erb | 4 +++ lib/generators/doorkeeper/views_generator.rb | 2 ++ spec/dummy/db/schema.rb | 2 +- .../password_access_token_request_spec.rb | 2 -- spec/models/doorkeeper/application_spec.rb | 32 ++++++++++++++++--- .../applications/applications_request_spec.rb | 30 +++++++++++++++++ spec/requests/flows/password_spec.rb | 2 +- 18 files changed, 97 insertions(+), 8 deletions(-) diff --git a/NEWS.md b/NEWS.md index e0877b7c0..b818619d4 100644 --- a/NEWS.md +++ b/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. diff --git a/app/validators/redirect_uri_validator.rb b/app/validators/redirect_uri_validator.rb index 3c8a84148..4e9fcb7d9 100644 --- a/app/validators/redirect_uri_validator.rb +++ b/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 diff --git a/app/views/doorkeeper/applications/_form.html.erb b/app/views/doorkeeper/applications/_form.html.erb index f60a162c3..12bb12b7b 100644 --- a/app/views/doorkeeper/applications/_form.html.erb +++ b/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 }) %> <% end %> + + <% if Doorkeeper.configuration.allow_blank_redirect_uri? %> + + <%= t('doorkeeper.applications.help.blank_redirect_uri') %> + + <% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 0e5bbf2f4..b53dcdc5b 100644 --- a/config/locales/en.yml +++ b/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: diff --git a/lib/doorkeeper/config.rb b/lib/doorkeeper/config.rb index ff7b88874..7b2361137 100644 --- a/lib/doorkeeper/config.rb +++ b/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 diff --git a/lib/generators/doorkeeper/application_owner_generator.rb b/lib/generators/doorkeeper/application_owner_generator.rb index 0993a9b6f..4ce2a4799 100644 --- a/lib/generators/doorkeeper/application_owner_generator.rb +++ b/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__) diff --git a/lib/generators/doorkeeper/confidential_applications_generator.rb b/lib/generators/doorkeeper/confidential_applications_generator.rb index 487280754..3d2935d5e 100644 --- a/lib/generators/doorkeeper/confidential_applications_generator.rb +++ b/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__) diff --git a/lib/generators/doorkeeper/install_generator.rb b/lib/generators/doorkeeper/install_generator.rb index dac31e899..f169179e8 100644 --- a/lib/generators/doorkeeper/install_generator.rb +++ b/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__) diff --git a/lib/generators/doorkeeper/migration_generator.rb b/lib/generators/doorkeeper/migration_generator.rb index b607eaa0a..ac3223fcd 100644 --- a/lib/generators/doorkeeper/migration_generator.rb +++ b/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__) diff --git a/lib/generators/doorkeeper/pkce_generator.rb b/lib/generators/doorkeeper/pkce_generator.rb index 86577767e..13c0614b2 100644 --- a/lib/generators/doorkeeper/pkce_generator.rb +++ b/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__) diff --git a/lib/generators/doorkeeper/previous_refresh_token_generator.rb b/lib/generators/doorkeeper/previous_refresh_token_generator.rb index f9eeff87b..bfe54433f 100644 --- a/lib/generators/doorkeeper/previous_refresh_token_generator.rb +++ b/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__) diff --git a/lib/generators/doorkeeper/templates/migration.rb.erb b/lib/generators/doorkeeper/templates/migration.rb.erb index e5e5619d0..b757b591e 100644 --- a/lib/generators/doorkeeper/templates/migration.rb.erb +++ b/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 diff --git a/lib/generators/doorkeeper/views_generator.rb b/lib/generators/doorkeeper/views_generator.rb index 16ccd0bea..ce379c1c1 100644 --- a/lib/generators/doorkeeper/views_generator.rb +++ b/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__) diff --git a/spec/dummy/db/schema.rb b/spec/dummy/db/schema.rb index 94bee7da8..8310180f6 100644 --- a/spec/dummy/db/schema.rb +++ b/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 diff --git a/spec/lib/oauth/password_access_token_request_spec.rb b/spec/lib/oauth/password_access_token_request_spec.rb index 6c3b62dfe..67e488642 100644 --- a/spec/lib/oauth/password_access_token_request_spec.rb +++ b/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 } ) diff --git a/spec/models/doorkeeper/application_spec.rb b/spec/models/doorkeeper/application_spec.rb index 900145548..667c87fd1 100644 --- a/spec/models/doorkeeper/application_spec.rb +++ b/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 diff --git a/spec/requests/applications/applications_request_spec.rb b/spec/requests/applications/applications_request_spec.rb index db35e1d87..c68e1e324 100644 --- a/spec/requests/applications/applications_request_spec.rb +++ b/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 diff --git a/spec/requests/flows/password_spec.rb b/spec/requests/flows/password_spec.rb index 42935a120..2967d9c7d 100644 --- a/spec/requests/flows/password_spec.rb +++ b/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"])