From 08f8e8a43b7c6565dcca3eab07b0515f0a0b3af1 Mon Sep 17 00:00:00 2001 From: Daniel Bird Date: Sat, 18 Sep 2021 15:10:31 -0400 Subject: [PATCH] Make redirect_uri optional in authorization request and token request --- CHANGELOG.md | 1 + lib/doorkeeper/config.rb | 18 ++++ .../oauth/authorization_code_request.rb | 13 ++- lib/doorkeeper/oauth/code_response.rb | 8 +- lib/doorkeeper/oauth/pre_authorization.rb | 9 +- .../orm/active_record/mixins/access_grant.rb | 23 ++++- .../doorkeeper/templates/initializer.rb | 17 ++++ .../doorkeeper/templates/migration.rb.erb | 2 +- spec/dummy/config/initializers/doorkeeper.rb | 17 ++++ ...20151223192035_create_doorkeeper_tables.rb | 2 +- spec/dummy/db/schema.rb | 2 +- spec/lib/config_spec.rb | 15 +++ .../oauth/authorization_code_request_spec.rb | 92 ++++++++++++++----- spec/lib/oauth/pre_authorization_spec.rb | 37 +++++++- spec/models/doorkeeper/access_grant_spec.rb | 64 +++++++++++++ .../requests/flows/authorization_code_spec.rb | 29 ++++++ .../flows/implicit_grant_errors_spec.rb | 2 + spec/requests/flows/implicit_grant_spec.rb | 13 +++ 18 files changed, 328 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ef6861637..dfb162f3e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ User-visible changes worth mentioning. ## main +- [#1529] Redirect_uri is optional in token request if it was not present in authorization request. - [#PR ID] Add your PR description here. ## 5.5.3 diff --git a/lib/doorkeeper/config.rb b/lib/doorkeeper/config.rb index b1cdb3df7..dfd51c80d 100644 --- a/lib/doorkeeper/config.rb +++ b/lib/doorkeeper/config.rb @@ -437,6 +437,24 @@ def configure_secrets_for(type, using:, fallback:) end end) + # Allows the client to not pass a redirect_uri parameter to the + # authorization endpoint during the authorization code flow and implicit + # flow. If the client does not pass a redirect_uri parameter to the + # authorization endpoint then then Doorkeeper will redirect the resource + # owner to the redirection endpoint specified when the application was + # created, as per Section 3.1.2 of the OAuth 2.0 specification: + # https://datatracker.ietf.org/doc/html/rfc6749#section-3.1.2 + # + # By default this option is false, meaning that the client must pass a + # redirect_uri parameter to the authorization endpoint. + # + # This option should only be set to true if your application does not have + # a NOT_NULL constraint applied to the redirect_uri column of its + # oauth_access_grants database table. + # + option :redirect_uri_optional_during_authorization, + default: false + attr_reader :reuse_access_token, :token_secret_fallback_strategy, :application_secret_fallback_strategy diff --git a/lib/doorkeeper/oauth/authorization_code_request.rb b/lib/doorkeeper/oauth/authorization_code_request.rb index 990ac3e6a..a14f1136a 100644 --- a/lib/doorkeeper/oauth/authorization_code_request.rb +++ b/lib/doorkeeper/oauth/authorization_code_request.rb @@ -57,7 +57,7 @@ def pkce_supported? def validate_params @missing_param = if grant&.uses_pkce? && code_verifier.blank? :code_verifier - elsif redirect_uri.blank? + elsif grant&.redirect_uri.present? && redirect_uri.blank? :redirect_uri end @@ -75,9 +75,18 @@ def validate_grant end def validate_redirect_uri + # 4.1.3. Access Token Request + # redirect_uri + # REQUIRED, if the "redirect_uri" parameter was included in the + # authorization request as described in Section 4.1.1, and their + # values MUST be identical. + # + # @see https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.3 + return true if redirect_uri.blank? && grant.redirect_uri.blank? + Helpers::URIChecker.valid_for_authorization?( redirect_uri, - grant.redirect_uri, + grant.redirect_uri.presence || client.redirect_uri, ) end diff --git a/lib/doorkeeper/oauth/code_response.rb b/lib/doorkeeper/oauth/code_response.rb index 2cf7ce4a7..77c24e686 100644 --- a/lib/doorkeeper/oauth/code_response.rb +++ b/lib/doorkeeper/oauth/code_response.rb @@ -38,12 +38,14 @@ def body end def redirect_uri - if URIChecker.oob_uri?(pre_auth.redirect_uri) + uri = pre_auth.redirect_uri.presence || pre_auth.client.redirect_uri + + if URIChecker.oob_uri?(uri) auth.oob_redirect elsif response_on_fragment - Authorization::URIBuilder.uri_with_fragment(pre_auth.redirect_uri, body) + Authorization::URIBuilder.uri_with_fragment(uri, body) else - Authorization::URIBuilder.uri_with_query(pre_auth.redirect_uri, body) + Authorization::URIBuilder.uri_with_query(uri, body) end end end diff --git a/lib/doorkeeper/oauth/pre_authorization.rb b/lib/doorkeeper/oauth/pre_authorization.rb index 3febade4e..c43a98e0a 100644 --- a/lib/doorkeeper/oauth/pre_authorization.rb +++ b/lib/doorkeeper/oauth/pre_authorization.rb @@ -97,7 +97,14 @@ def validate_resource_owner_authorize_for_client end def validate_redirect_uri - return false if redirect_uri.blank? + # 4.1.1. Authorization Request + # redirect_uri + # OPTIONAL. As described in Section 3.1.2. + # + # @see https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.1 + return true if redirect_uri.blank? && + client.redirect_uri.present? && + Doorkeeper.config.redirect_uri_optional_during_authorization Helpers::URIChecker.valid_for_authorization?( redirect_uri, diff --git a/lib/doorkeeper/orm/active_record/mixins/access_grant.rb b/lib/doorkeeper/orm/active_record/mixins/access_grant.rb index b9ba92b6b..ccc882a80 100644 --- a/lib/doorkeeper/orm/active_record/mixins/access_grant.rb +++ b/lib/doorkeeper/orm/active_record/mixins/access_grant.rb @@ -23,9 +23,18 @@ module AccessGrant validates :application_id, :token, :expires_in, - :redirect_uri, presence: true + validates :redirect_uri, + presence: true, + on: :create, + unless: :redirect_uri_optional_during_authorization? + + validates :redirect_uri, + presence: true, + on: :update, + if: :validate_redirect_uri_on_update? + validates :token, uniqueness: { case_sensitive: true } before_validation :generate_token, on: :create @@ -54,6 +63,18 @@ def generate_token @raw_token = Doorkeeper::OAuth::Helpers::UniqueToken.generate secret_strategy.store_secret(self, :token, @raw_token) end + + def redirect_uri_optional_during_authorization? + Doorkeeper.config.redirect_uri_optional_during_authorization + end + + def redirect_uri_required_during_authorization? + !redirect_uri_optional_during_authorization? + end + + def validate_redirect_uri_on_update? + redirect_uri_required_during_authorization? && redirect_uri_changed? + end end module ClassMethods diff --git a/lib/generators/doorkeeper/templates/initializer.rb b/lib/generators/doorkeeper/templates/initializer.rb index 20dbcd1ef..209c686c1 100644 --- a/lib/generators/doorkeeper/templates/initializer.rb +++ b/lib/generators/doorkeeper/templates/initializer.rb @@ -485,6 +485,23 @@ # If you need to block the request at all, then configure your routes.rb or web-server # like nginx to forbid the request. + # Allows the client to not pass a redirect_uri parameter to the + # authorization endpoint during the authorization code flow and implicit + # flow. If the client does not pass a redirect_uri parameter to the + # authorization endpoint then then Doorkeeper will redirect the resource + # owner to the redirection endpoint specified when the application was + # created, as per Section 3.1.2 of the OAuth 2.0 specification: + # https://datatracker.ietf.org/doc/html/rfc6749#section-3.1.2 + # + # By default this option is false, meaning that the client must pass a + # redirect_uri parameter to the authorization endpoint. + # + # This option should only be set to true if your application does not have + # a NOT_NULL constraint applied to the redirect_uri column of its + # oauth_access_grants database table. + # + # redirect_uri_optional_during_authorization false + # WWW-Authenticate Realm (default: "Doorkeeper"). # # realm "Doorkeeper" diff --git a/lib/generators/doorkeeper/templates/migration.rb.erb b/lib/generators/doorkeeper/templates/migration.rb.erb index a0169dd4d..adc8c5930 100644 --- a/lib/generators/doorkeeper/templates/migration.rb.erb +++ b/lib/generators/doorkeeper/templates/migration.rb.erb @@ -23,7 +23,7 @@ class CreateDoorkeeperTables < ActiveRecord::Migration<%= migration_version %> t.references :application, null: false t.string :token, null: false t.integer :expires_in, null: false - t.text :redirect_uri, null: false + t.text :redirect_uri t.datetime :created_at, null: false t.datetime :revoked_at t.string :scopes, null: false, default: '' diff --git a/spec/dummy/config/initializers/doorkeeper.rb b/spec/dummy/config/initializers/doorkeeper.rb index 73364ae31..3805e07fc 100644 --- a/spec/dummy/config/initializers/doorkeeper.rb +++ b/spec/dummy/config/initializers/doorkeeper.rb @@ -161,6 +161,23 @@ # If you need to block the request at all, then configure your routes.rb or web-server # like nginx to forbid the request. + # Allows the client to not pass a redirect_uri parameter to the + # authorization endpoint during the authorization code flow and implicit + # flow. If the client does not pass a redirect_uri parameter to the + # authorization endpoint then then Doorkeeper will redirect the resource + # owner to the redirection endpoint specified when the application was + # created, as per Section 3.1.2 of the OAuth 2.0 specification: + # https://datatracker.ietf.org/doc/html/rfc6749#section-3.1.2 + # + # By default this option is false, meaning that the client must pass a + # redirect_uri parameter to the authorization endpoint. + # + # This option should only be set to true if your application does not have + # a NOT_NULL constraint applied to the redirect_uri column of its + # oauth_access_grants database table. + # + # redirect_uri_optional_during_authorization false + # WWW-Authenticate Realm (default "Doorkeeper"). realm "Doorkeeper" end diff --git a/spec/dummy/db/migrate/20151223192035_create_doorkeeper_tables.rb b/spec/dummy/db/migrate/20151223192035_create_doorkeeper_tables.rb index 3220b983e..22e009432 100644 --- a/spec/dummy/db/migrate/20151223192035_create_doorkeeper_tables.rb +++ b/spec/dummy/db/migrate/20151223192035_create_doorkeeper_tables.rb @@ -22,7 +22,7 @@ def change t.references :application, null: false t.string :token, null: false t.integer :expires_in, null: false - t.text :redirect_uri, null: false + t.text :redirect_uri t.datetime :created_at, null: false t.datetime :revoked_at t.string :scopes, null: false, default: "" diff --git a/spec/dummy/db/schema.rb b/spec/dummy/db/schema.rb index da31d4a2c..05011ea9d 100644 --- a/spec/dummy/db/schema.rb +++ b/spec/dummy/db/schema.rb @@ -18,7 +18,7 @@ t.integer "application_id", null: false t.string "token", null: false t.integer "expires_in", null: false - t.text "redirect_uri", null: false + t.text "redirect_uri" t.datetime "created_at", null: false t.datetime "revoked_at" t.string "scopes" diff --git a/spec/lib/config_spec.rb b/spec/lib/config_spec.rb index 80a8215de..6d68b8ca7 100644 --- a/spec/lib/config_spec.rb +++ b/spec/lib/config_spec.rb @@ -810,6 +810,21 @@ class FakeCustomModel; end end end + describe "redirect_uri_optional_during_authorization" do + it "is false by default" do + expect(config.redirect_uri_optional_during_authorization).to be(false) + end + + it "can be set to true" do + Doorkeeper.configure do + orm DOORKEEPER_ORM + redirect_uri_optional_during_authorization true + end + + expect(config.redirect_uri_optional_during_authorization).to be(true) + end + end + describe "options deprecation" do it "prints a warning message when an option is deprecated" do expect(Kernel).to receive(:warn).with( diff --git a/spec/lib/oauth/authorization_code_request_spec.rb b/spec/lib/oauth/authorization_code_request_spec.rb index de78f957f..b66b67938 100644 --- a/spec/lib/oauth/authorization_code_request_spec.rb +++ b/spec/lib/oauth/authorization_code_request_spec.rb @@ -4,7 +4,7 @@ RSpec.describe Doorkeeper::OAuth::AuthorizationCodeRequest do subject(:request) do - described_class.new(server, grant, client, params) + described_class.new(server, grant, client, define_params) end let(:server) do @@ -23,8 +23,6 @@ resource_owner_type: resource_owner.class.name end let(:client) { grant.application } - let(:redirect_uri) { client.redirect_uri } - let(:params) { { redirect_uri: redirect_uri } } before do allow(server).to receive(:option_defined?).with(:custom_access_token_expires_in).and_return(true) @@ -54,33 +52,20 @@ end it "requires the grant" do - request = described_class.new(server, nil, client, params) + request = described_class.new(server, nil, client, define_params) request.validate expect(request.error).to eq(:invalid_grant) end it "requires the client" do - request = described_class.new(server, grant, nil, params) + request = described_class.new(server, grant, nil, define_params) request.validate expect(request.error).to eq(:invalid_client) end - it "requires the redirect_uri" do - request = described_class.new(server, grant, nil, params.except(:redirect_uri)) - request.validate - expect(request.error).to eq(:invalid_request) - expect(request.missing_param).to eq(:redirect_uri) - end - - it "matches the redirect_uri with grant's one" do - request = described_class.new(server, grant, client, params.merge(redirect_uri: "http://other.com")) - request.validate - expect(request.error).to eq(:invalid_grant) - end - it "matches the client with grant's one" do other_client = FactoryBot.create :application - request = described_class.new(server, grant, other_client, params) + request = described_class.new(server, grant, other_client, define_params) request.validate expect(request.error).to eq(:invalid_grant) end @@ -140,6 +125,7 @@ let(:redirect_uri) { "#{client.redirect_uri}?query=q" } it "responds with invalid_grant" do + request = described_class.new(server, grant, client, define_params(redirect_uri: redirect_uri)) request.validate expect(request.error).to eq(:invalid_grant) end @@ -149,6 +135,7 @@ let(:redirect_uri) { "123d#!s" } it "responds with invalid_grant" do + request = described_class.new(server, grant, client, define_params(redirect_uri: redirect_uri)) request.validate expect(request.error).to eq(:invalid_grant) end @@ -158,17 +145,67 @@ let(:redirect_uri) { "urn:ietf:wg:oauth:2.0:oob" } it "invalidates when redirect_uri of the grant is not native" do + request = described_class.new(server, grant, client, define_params(redirect_uri: redirect_uri)) request.validate expect(request.error).to eq(:invalid_grant) end it "validates when redirect_uri of the grant is also native" do allow(grant).to receive(:redirect_uri) { redirect_uri } + request = described_class.new(server, grant, client, define_params(redirect_uri: redirect_uri)) request.validate expect(request.error).to eq(nil) end end + context "when the grant's redirect_uri is present" do + it "requires the redirect_uri" do + request = described_class.new(server, grant, client, define_params(redirect_uri: nil)) + request.validate + expect(request.error).to eq(:invalid_request) + expect(request.missing_param).to eq(:redirect_uri) + end + + it "matches the redirect_uri with grant's one" do + request = described_class.new(server, grant, client, define_params(redirect_uri: "http://other.com")) + request.validate + expect(request.error).to eq(:invalid_grant) + end + end + + context "when the grant's redirect_uri is blank" do + before do + Doorkeeper.configure do + redirect_uri_optional_during_authorization true + end + end + + let(:grant) do + FactoryBot.create :access_grant, + redirect_uri: nil, + resource_owner_id: resource_owner.id, + resource_owner_type: resource_owner.class.name + end + + it "is valid with a blank redirect_uri" do + request = described_class.new(server, grant, grant.application, define_params(redirect_uri: nil)) + request.valid? + expect(request.valid?).to eq(true) + end + + it "is valid if the redirect_uri matches the client's redirect_uri" do + request = described_class.new(server, grant, grant.application, define_params) + request.valid? + expect(request.valid?).to eq(true) + end + + it "is invalid if the redirect_uri does not match the client's redirect_uri" do + request = described_class.new(server, grant, grant.application, define_params(redirect_uri: "http://other.com")) + request.validate + expect(request.error).to eq(:invalid_grant) + end + end + context "when using PKCE params" do context "when PKCE is supported" do before do @@ -179,14 +216,17 @@ end it "validates when code_verifier is present" do - params[:code_verifier] = grant.code_challenge + params = define_params(code_verifier: grant.code_challenge) + request = described_class.new(server, grant, client, params) request.validate expect(request.error).to eq(nil) end it "validates when both code_verifier and code_challenge are blank" do - params[:code_verifier] = grant.code_challenge = "" + grant.code_challenge = "" + params = define_params(code_verifier: grant.code_challenge) + request = described_class.new(server, grant, client, params) request.validate expect(request.error).to eq(nil) @@ -200,7 +240,8 @@ end it "invalidates when code_verifier is the wrong value" do - params[:code_verifier] = "foobar" + params = define_params(code_verifier: "foobar") + request = described_class.new(server, grant, client, params) request.validate expect(request.error).to eq(:invalid_grant) @@ -213,11 +254,16 @@ end it "validates when code_verifier is present" do - params[:code_verifier] = "foobar" + params = define_params(code_verifier: "foobar") + request = described_class.new(server, grant, client, params) request.validate expect(request.error).to be_nil end end end + + def define_params(redirect_uri: client.redirect_uri, **args) + { redirect_uri: redirect_uri }.merge(args) + end end diff --git a/spec/lib/oauth/pre_authorization_spec.rb b/spec/lib/oauth/pre_authorization_spec.rb index 81820b4d5..1f87efb5e 100644 --- a/spec/lib/oauth/pre_authorization_spec.rb +++ b/spec/lib/oauth/pre_authorization_spec.rb @@ -243,9 +243,40 @@ expect(pre_auth).not_to be_authorizable end - it "requires a redirect uri" do - attributes[:redirect_uri] = nil - expect(pre_auth).not_to be_authorizable + context "when configured so that redirect uri is required during authorization" do + it "requires a redirect uri" do + attributes[:redirect_uri] = nil + expect(pre_auth).not_to be_authorizable + end + end + + context "when configured so that redirect uri is optional during authorization" do + before do + Doorkeeper.configure do + redirect_uri_optional_during_authorization true + end + end + + it "accepts a nil redirect uri" do + attributes[:redirect_uri] = nil + expect(pre_auth).to be_authorizable + end + + context "when client does not have a redirect_uri" do + before do + Doorkeeper.configure do + redirect_uri_optional_during_authorization true + allow_blank_redirect_uri true + end + end + + let(:application) { FactoryBot.create(:application, redirect_uri: nil) } + + it "requires a redirect_uri" do + attributes[:redirect_uri] = nil + expect(pre_auth).not_to be_authorizable + end + end end context "when resource_owner cannot access client application" do diff --git a/spec/models/doorkeeper/access_grant_spec.rb b/spec/models/doorkeeper/access_grant_spec.rb index cdb1f7d06..f2365f3b1 100644 --- a/spec/models/doorkeeper/access_grant_spec.rb +++ b/spec/models/doorkeeper/access_grant_spec.rb @@ -128,6 +128,70 @@ access_grant.expires_in = nil expect(access_grant).not_to be_valid end + + context "when redirect_uri is required during authorization" do + context "when not yet persisted" do + it "is invalid without a redirect_uri" do + access_grant.redirect_uri = nil + expect(access_grant).not_to be_valid + end + + it "is valid with a redirect_uri" do + expect(access_grant).to be_valid + end + end + + context "when persisted" do + before do + access_grant.save! + end + + it "is invalid if its redirect_uri is changed to blank" do + access_grant.redirect_uri = nil + expect(access_grant).not_to be_valid + end + + it "is valid if its blank redirect_uri is unchanged" do + access_grant.update_column(:redirect_uri, nil) + expect(access_grant).to be_valid + end + end + end + + context "when redirect_uri is optional during authorization" do + before do + Doorkeeper.configure do + redirect_uri_optional_during_authorization true + end + end + + context "when not yet persisted" do + it "is valid without a redirect_uri" do + access_grant.redirect_uri = nil + expect(access_grant).to be_valid + end + + it "is valid with a redirect_uri" do + expect(access_grant).to be_valid + end + end + + context "when persisted" do + before do + access_grant.save! + end + + it "is valid if its redirect_uri is changed to blank" do + access_grant.redirect_uri = nil + expect(access_grant).to be_valid + end + + it "is valid its blank redirect_uri is unchanged" do + access_grant.update_column(:redirect_uri, nil) + expect(access_grant).to be_valid + end + end + end end describe ".revoke_all_for" do diff --git a/spec/requests/flows/authorization_code_spec.rb b/spec/requests/flows/authorization_code_spec.rb index a9944231e..342b14283 100644 --- a/spec/requests/flows/authorization_code_spec.rb +++ b/spec/requests/flows/authorization_code_spec.rb @@ -24,6 +24,35 @@ url_should_not_have_param("error") end + context "when configured so that redirect uri is required during authorization" do + scenario "displays invalid_redirect_uri error when redirect_uri is missing" do + visit "/oauth/authorize?client_id=#{@client.uid}&response_type=code" + + i_should_not_see("Authorize") + i_should_see_translated_error_message(:invalid_redirect_uri) + end + end + + context "when configured so that redirect uri is optional during authorization" do + before do + config_is_set(:redirect_uri_optional_during_authorization, true) + end + + scenario "resource owner authorizes the client without redirect URI provided" do + visit "/oauth/authorize?client_id=#{@client.uid}&response_type=code" + + click_on "Authorize" + + access_grant_should_exist_for(@client, @resource_owner) + + i_should_be_on_client_callback(@client) + + url_should_have_param("code", Doorkeeper::AccessGrant.first.token) + url_should_not_have_param("state") + url_should_not_have_param("error") + end + end + context "when configured to check application supported grant flow" do before do config_is_set(:allow_grant_flow_for_client, ->(_grant_flow, client) { client.name == "admin" }) diff --git a/spec/requests/flows/implicit_grant_errors_spec.rb b/spec/requests/flows/implicit_grant_errors_spec.rb index 6f27acab0..e0c87b0cf 100644 --- a/spec/requests/flows/implicit_grant_errors_spec.rb +++ b/spec/requests/flows/implicit_grant_errors_spec.rb @@ -36,7 +36,9 @@ i_should_not_see "Authorize" i_should_see_translated_error_message :invalid_redirect_uri end + end + context "when configured so that redirect uri is required during authorization" do scenario "displays invalid_redirect_uri error when redirect_uri is missing" do visit authorization_endpoint_url(client: @client, redirect_uri: "", response_type: "token") i_should_not_see "Authorize" diff --git a/spec/requests/flows/implicit_grant_spec.rb b/spec/requests/flows/implicit_grant_spec.rb index 180c5e7ff..f48668d61 100644 --- a/spec/requests/flows/implicit_grant_spec.rb +++ b/spec/requests/flows/implicit_grant_spec.rb @@ -42,6 +42,19 @@ access_token_should_have_scopes :public, :write end end + + context "when configured so that redirect uri is optional during authorization" do + before do + config_is_set(:redirect_uri_optional_during_authorization, true) + end + + scenario "resource owner authorizes the client without the redirect uri provided" do + visit authorization_endpoint_url(client: @client, redirect_uri: "", response_type: "token") + click_on "Authorize" + access_token_should_exist_for @client, @resource_owner + i_should_be_on_client_callback @client + end + end end RSpec.describe "Implicit Grant Flow (request spec)" do