Merge pull request #1435 from linhdangduy/not_redirectable_when_client_is_unauthorized

Make error response not redirectable when client is unauthorized

Author: nbulaj

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.

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,16 +34,15 @@ def body
       end
 
       def status
-        if name == :invalid_client
+        if name == :invalid_client || name == :unauthorized_client
           :unauthorized
         else
           :bad_request
         end
       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

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

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: {

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

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

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"