Merge pull request #1676 from camero2734/authorizations-controller-re…

…sponse

Improve AuthorizationsController error response handling

CHANGELOG.md

@@ -12,6 +12,7 @@ User-visible changes worth mentioning.
 - [#1652] Add custom attributes support to token generator.
 - [#1667] Pass `client` instead of `grant.application` to `find_or_create_access_token`.
 - [#1673] Honor `custom_access_token_attributes` in client credentials grant flow.
+- [#1676] Improve AuthorizationsController error response handling
 
 ## 5.6.6
 

app/controllers/doorkeeper/authorizations_controller.rb

@@ -41,11 +41,14 @@ def render_success
     end
 
     def render_error
-      if Doorkeeper.configuration.api_only
-        render json: pre_auth.error_response.body,
-               status: :bad_request
+      pre_auth.error_response.raise_exception! if Doorkeeper.config.raise_on_errors?
+
+      if Doorkeeper.configuration.redirect_on_errors? && pre_auth.error_response.redirectable?
+        redirect_or_render(pre_auth.error_response)
+      elsif Doorkeeper.configuration.api_only
+        render json: pre_auth.error_response.body, status: pre_auth.error_response.status
       else
-        render :error, locals: { error_response: pre_auth.error_response }
+        render :error, locals: { error_response: pre_auth.error_response }, status: pre_auth.error_response.status
       end
     end
 

lib/doorkeeper/config.rb

@@ -501,6 +501,10 @@ def raise_on_errors?
       handle_auth_errors == :raise
     end
 
+    def redirect_on_errors?
+      handle_auth_errors == :redirect
+    end
+
     def application_secret_hashed?
       instance_variable_defined?(:"@application_secret_strategy")
     end

lib/doorkeeper/errors.rb

@@ -45,6 +45,7 @@ def initialize(response)
     TokenGeneratorNotFound = Class.new(DoorkeeperError)
     NoOrmCleaner = Class.new(DoorkeeperError)
 
+    InvalidRequest = Class.new(BaseResponseError)
     InvalidToken = Class.new(BaseResponseError)
     TokenExpired = Class.new(InvalidToken)
     TokenRevoked = Class.new(InvalidToken)

lib/doorkeeper/oauth/invalid_request_response.rb

@@ -35,6 +35,10 @@ def description
         )
       end
 
+      def exception_class
+        Doorkeeper::Errors::InvalidRequest
+      end
+
       def redirectable?
         super && @missing_param != :client_id
       end

lib/generators/doorkeeper/templates/initializer.rb

@@ -312,6 +312,12 @@
   #   Doorkeeper::Errors::TokenRevoked, Doorkeeper::Errors::TokenUnknown
   #
   # handle_auth_errors :raise
+  #
+  # If you want to redirect back to the client application in accordance with
+  # https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1, you can set
+  # +handle_auth_errors+ to :redirect
+  #
+  # handle_auth_errors :redirect
 
   # Customize token introspection response.
   # Allows to add your own fields to default one that are required by the OAuth spec

spec/controllers/authorizations_controller_spec.rb

@@ -984,8 +984,8 @@ def query_params
         }
       end
 
-      it "renders bad request" do
-        expect(response).to have_http_status(:bad_request)
+      it "renders unauthorized" do
+        expect(response).to have_http_status(:unauthorized)
       end
 
       it "includes error in body" do
@@ -1044,6 +1044,129 @@ def query_params
     end
   end
 
+  describe "GET #new with errors with handle_auth_errors :redirect" do
+    before { config_is_set(:handle_auth_errors, :redirect) }
+
+    context "without valid params" do
+      before do
+        default_scopes_exist :public
+        get :new, params: { an_invalid: "request" }
+      end
+
+      it "does not redirect" do
+        expect(response).not_to be_redirect
+      end
+
+      it "does not issue any token" do
+        expect(Doorkeeper::AccessGrant.count).to eq 0
+        expect(Doorkeeper::AccessToken.count).to eq 0
+      end
+    end
+
+    context "invalid scope" do
+      before do
+        default_scopes_exist :public
+        get :new, params: {
+          client_id: client.uid,
+          response_type: "token",
+          scope: "invalid",
+          redirect_uri: client.redirect_uri,
+          state: "return-this",
+        }
+      end
+
+      it "redirects to client redirect uri" do
+        expect(response).to be_redirect
+        expect(response.location).to match(/^#{client.redirect_uri}/)
+      end
+
+      it "includes error in fragment" do
+        expect(response.query_params["error"]).to eq("invalid_scope")
+      end
+
+      it "includes error description in fragment" do
+        expect(response.query_params["error_description"]).to eq(translated_error_message(:invalid_scope))
+      end
+
+      it "includes state in fragment" do
+        expect(response.query_params["state"]).to eq("return-this")
+      end
+
+      it "does not issue any token" do
+        expect(Doorkeeper::AccessGrant.count).to eq 0
+        expect(Doorkeeper::AccessToken.count).to eq 0
+      end
+    end
+
+    context "invalid redirect_uri" do
+      before do
+        default_scopes_exist :public
+        get :new, params: {
+          client_id: client.uid,
+          response_type: "token",
+          redirect_uri: "invalid",
+        }
+      end
+
+      it "does not redirect" do
+        expect(response).not_to be_redirect
+      end
+
+      it "does not issue any token" do
+        expect(Doorkeeper::AccessGrant.count).to eq 0
+        expect(Doorkeeper::AccessToken.count).to eq 0
+      end
+    end
+
+    context "with client_id and redirect_uri" do
+      before do
+        default_scopes_exist :public
+        get :new, params: {
+          client_id: client.uid,
+          redirect_uri: client.redirect_uri,
+          response_mode: "fragment"
+        }
+      end
+
+      it "redirects to client redirect uri" do
+        expect(response).to be_redirect
+        expect(response.location).to match(/^#{client.redirect_uri}/)
+      end
+
+      it "includes error in fragment" do
+        expect(response.query_params["error"]).to eq("invalid_request")
+      end
+
+      it "includes error description in fragment" do
+        expect(response.query_params["error_description"]).to eq(translated_invalid_request_error_message(:missing_param, :response_type))
+      end
+
+      it "does not issue any token" do
+        expect(Doorkeeper::AccessGrant.count).to eq 0
+        expect(Doorkeeper::AccessToken.count).to eq 0
+      end
+    end
+  end
+
+  describe "GET #new with errors with handle_auth_errors :raise" do
+    before { config_is_set(:handle_auth_errors, :raise) }
+
+    context "without valid params" do
+      before do
+        default_scopes_exist :public
+      end
+
+      it "does not redirect" do
+        expect { get :new, params: { an_invalid: "request" } }.to raise_error(Doorkeeper::Errors::InvalidRequest)
+      end
+
+      it "does not issue any token" do
+        expect(Doorkeeper::AccessGrant.count).to eq 0
+        expect(Doorkeeper::AccessToken.count).to eq 0
+      end
+    end
+  end
+
   describe "GET #new with callbacks" do
     after do
       client.update_attribute :redirect_uri, "urn:ietf:wg:oauth:2.0:oob"

spec/requests/flows/authorization_code_spec.rb

@@ -441,7 +441,7 @@ def authorize(redirect_url)
     scenario "scope is invalid because default scope is different from application scope" do
       default_scopes_exist :admin
       visit authorization_endpoint_url(client: @client)
-      response_status_should_be 200
+      response_status_should_be 400
       i_should_not_see "Authorize"
       i_should_see_translated_error_message :invalid_scope
     end

spec/requests/flows/implicit_grant_spec.rb

@@ -29,7 +29,7 @@
     scenario "scope is invalid because default scope is different from application scope" do
       default_scopes_exist :admin
       visit authorization_endpoint_url(client: @client, response_type: "token")
-      response_status_should_be 200
+      response_status_should_be 400
       i_should_not_see "Authorize"
       i_should_see_translated_error_message :invalid_scope
     end