diff --git a/CHANGELOG.md b/CHANGELOG.md index 2dea73827..64c8fb5fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ User-visible changes worth mentioning. ## master +- [#1435] Make error response not redirectable when client is unauthorized - [#1426] Ensure ActiveRecord callbacks are executed on token revocation. - [#1407] Remove redundant and complex to support helpers froms tests (`should_have_json`, etc). - [#1416] Don't add introspection route if token introspection completely disabled. diff --git a/lib/doorkeeper/oauth/error_response.rb b/lib/doorkeeper/oauth/error_response.rb index edb25c569..71613052a 100644 --- a/lib/doorkeeper/oauth/error_response.rb +++ b/lib/doorkeeper/oauth/error_response.rb @@ -5,6 +5,8 @@ module OAuth class ErrorResponse < BaseResponse include OAuth::Helpers + NON_REDIRECTABLE_STATES = %i[invalid_redirect_uri invalid_client unauthorized_client].freeze + def self.from_request(request, attributes = {}) new( attributes.merge( @@ -32,7 +34,7 @@ def body end def status - if name == :invalid_client + if name == :invalid_client || name == :unauthorized_client :unauthorized else :bad_request @@ -40,8 +42,7 @@ def status end def redirectable? - name != :invalid_redirect_uri && name != :invalid_client && - !URIChecker.oob_uri?(@redirect_uri) + !NON_REDIRECTABLE_STATES.include?(name) && !URIChecker.oob_uri?(@redirect_uri) end def redirect_uri diff --git a/lib/doorkeeper/oauth/pre_authorization.rb b/lib/doorkeeper/oauth/pre_authorization.rb index b81ccc3d6..33b53d969 100644 --- a/lib/doorkeeper/oauth/pre_authorization.rb +++ b/lib/doorkeeper/oauth/pre_authorization.rb @@ -84,6 +84,11 @@ def validate_client_supports_grant_flow Doorkeeper.config.allow_grant_flow_for_client?(grant_type, client.application) end + def validate_resource_owner_authorize_for_client + # The `authorize_resource_owner_for_client` config option is used for this validation + client.application.authorized_for_resource_owner?(@resource_owner) + end + def validate_redirect_uri return false if redirect_uri.blank? @@ -125,11 +130,6 @@ def validate_code_challenge_method (code_challenge_method.present? && code_challenge_method =~ /^plain$|^S256$/) end - def validate_resource_owner_authorize_for_client - # The `authorize_resource_owner_for_client` config option is used for this validation - client.application.authorized_for_resource_owner?(@resource_owner) - end - def response_on_fragment? response_type == "token" end diff --git a/spec/controllers/authorizations_controller_spec.rb b/spec/controllers/authorizations_controller_spec.rb index 91afeb5bf..bef5b672c 100644 --- a/spec/controllers/authorizations_controller_spec.rb +++ b/spec/controllers/authorizations_controller_spec.rb @@ -144,6 +144,37 @@ def query_params end end + context "when client can not use grant flow" do + before do + config_is_set(:allow_grant_flow_for_client, ->(*_) { false }) + post :create, params: { + client_id: client.uid, + response_type: "token", + redirect_uri: client.redirect_uri, + } + end + + let(:response_json_body) { JSON.parse(response.body) } + + it "renders 401 error" do + expect(response.status).to eq 401 + end + + it "includes error name" do + expect(response_json_body["error"]).to eq("unauthorized_client") + end + + it "includes error description" do + expect(response_json_body["error_description"]).to eq( + translated_error_message(:unauthorized_client), + ) + end + + it "does not issue any access token" do + expect(Doorkeeper::AccessToken.all).to be_empty + end + end + context "when user cannot access application" do before do allow(Doorkeeper.configuration).to receive(:authorize_resource_owner_for_client).and_return(->(*_) { false }) @@ -156,7 +187,7 @@ def query_params let(:response_json_body) { JSON.parse(response.body) } - it "renders 400 error" do + it "renders 401 error" do expect(response.status).to eq 401 end @@ -214,10 +245,10 @@ def query_params end describe "POST #create in API mode with errors" do + before { config_is_set(:api_only, true) } + context "when missing client_id" do before do - allow(Doorkeeper.config).to receive(:api_only).and_return(true) - post :create, params: { client_id: "", response_type: "token", @@ -246,9 +277,39 @@ def query_params end end + context "when client can not use grant flow" do + before do + config_is_set(:allow_grant_flow_for_client, ->(*_) { false }) + post :create, params: { + client_id: client.uid, + response_type: "token", + redirect_uri: client.redirect_uri, + } + end + + let(:response_json_body) { JSON.parse(response.body) } + + it "renders 401 error" do + expect(response.status).to eq 401 + end + + it "includes error name" do + expect(response_json_body["error"]).to eq("unauthorized_client") + end + + it "includes error description" do + expect(response_json_body["error_description"]).to eq( + translated_error_message(:unauthorized_client), + ) + end + + it "does not issue any access token" do + expect(Doorkeeper::AccessToken.all).to be_empty + end + end + context "when user cannot access application" do before do - allow(Doorkeeper.configuration).to receive(:api_only).and_return(true) allow(Doorkeeper.configuration).to receive(:authorize_resource_owner_for_client).and_return(->(*_) { false }) post :create, params: { @@ -260,7 +321,7 @@ def query_params let(:response_json_body) { JSON.parse(response.body) } - it "renders 400 error" do + it "renders 401 error" do expect(response.status).to eq 401 end @@ -281,7 +342,6 @@ def query_params context "when other error happens" do before do - allow(Doorkeeper.config).to receive(:api_only).and_return(true) default_scopes_exist :public post :create, params: { diff --git a/spec/lib/oauth/error_response_spec.rb b/spec/lib/oauth/error_response_spec.rb index 9ef28e759..028241673 100644 --- a/spec/lib/oauth/error_response_spec.rb +++ b/spec/lib/oauth/error_response_spec.rb @@ -13,6 +13,12 @@ expect(subject.status).to eq(:unauthorized) end + + it "has a status of unauthorized for an unauthorized_client error" do + subject = described_class.new(name: :unauthorized_client) + + expect(subject.status).to eq(:unauthorized) + end end describe ".from_request" do @@ -62,4 +68,36 @@ it { expect(headers).to include("error_description=\"#{error_response.description}\"") } end end + + describe ".redirectable?" do + it "not redirectable when error name is invalid_redirect_uri" do + subject = described_class.new(name: :invalid_redirect_uri, redirect_uri: "https://example.com") + + expect(subject.redirectable?).to be false + end + + it "not redirectable when error name is invalid_client" do + subject = described_class.new(name: :invalid_client, redirect_uri: "https://example.com") + + expect(subject.redirectable?).to be false + end + + it "not redirectable when error name is unauthorized_client" do + subject = described_class.new(name: :unauthorized_client, redirect_uri: "https://example.com") + + expect(subject.redirectable?).to be false + end + + it "not redirectable when redirect_uri is oob uri" do + subject = described_class.new(name: :other_error, redirect_uri: Doorkeeper::OAuth::NonStandard::IETF_WG_OAUTH2_OOB) + + expect(subject.redirectable?).to be false + end + + it "is redirectable when error is not related to client or redirect_uri, and redirect_uri is not oob uri" do + subject = described_class.new(name: :other_error, redirect_uri: "https://example.com") + + expect(subject.redirectable?).to be true + end + end end diff --git a/spec/lib/oauth/invalid_request_response_spec.rb b/spec/lib/oauth/invalid_request_response_spec.rb index c908d2732..0b8847a20 100644 --- a/spec/lib/oauth/invalid_request_response_spec.rb +++ b/spec/lib/oauth/invalid_request_response_spec.rb @@ -58,4 +58,18 @@ end end end + + describe ".redirectable?" do + it "not redirectable when missing_param is client_id" do + subject = described_class.new(missing_param: :client_id) + + expect(subject.redirectable?).to be false + end + + it "is redirectable when missing_param is other than client_id" do + subject = described_class.new(missing_param: :code_verifier) + + expect(subject.redirectable?).to be true + end + end end diff --git a/spec/lib/oauth/pre_authorization_spec.rb b/spec/lib/oauth/pre_authorization_spec.rb index f71689d56..77b635712 100644 --- a/spec/lib/oauth/pre_authorization_spec.rb +++ b/spec/lib/oauth/pre_authorization_spec.rb @@ -27,18 +27,23 @@ } end - it "is authorizable when request is valid" do - expect(pre_auth).to be_authorizable + it "must call the validations on client and redirect_uri before other validations because they are not redirectable" do + validation_attributes = described_class.validations.map { |validation| validation[:attribute] } + + expect(validation_attributes).to eq(%i[ + client_id + client + client_supports_grant_flow + resource_owner_authorize_for_client + redirect_uri + params + response_type + scopes + code_challenge_method + ]) end - it "accepts code as response type" do - attributes[:response_type] = "code" - expect(pre_auth).to be_authorizable - end - - it "accepts token as response type" do - allow(server).to receive(:grant_flows).and_return(["implicit"]) - attributes[:response_type] = "token" + it "is authorizable when request is valid" do expect(pre_auth).to be_authorizable end @@ -77,18 +82,6 @@ end end - context "when grant flow is client credentials & redirect_uri is nil" do - before do - allow(server).to receive(:grant_flows).and_return(["client_credentials"]) - allow(Doorkeeper.configuration).to receive(:allow_grant_flow_for_client?).and_return(false) - application.update_column :redirect_uri, nil - end - - it "is not authorizable" do - expect(pre_auth).not_to be_authorizable - end - end - context "when client application does not restrict valid scopes" do it "accepts valid scopes" do attributes[:scope] = "public"