From ac822fc9872a6df84c89d8654f7a6a7d04ff58ea Mon Sep 17 00:00:00 2001
From: camero2734 <42698419+camero2734@users.noreply.github.com>
Date: Tue, 7 Nov 2023 15:40:41 +0100
Subject: [PATCH 1/8] Throw error if raise_on_errors is enabled

---
 app/controllers/doorkeeper/authorizations_controller.rb | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/app/controllers/doorkeeper/authorizations_controller.rb b/app/controllers/doorkeeper/authorizations_controller.rb
index 439cf167b..bae5d21a0 100644
--- a/app/controllers/doorkeeper/authorizations_controller.rb
+++ b/app/controllers/doorkeeper/authorizations_controller.rb
@@ -41,6 +41,8 @@ def render_success
     end
 
     def render_error
+      pre_auth.error.raise_exception! if Doorkeeper.config.raise_on_errors?
+
       if Doorkeeper.configuration.api_only
         render json: pre_auth.error_response.body,
                status: :bad_request

From f5dcdad7b172da6a0aa7f67fafe87fe0e28dea3b Mon Sep 17 00:00:00 2001
From: camero2734 <42698419+camero2734@users.noreply.github.com>
Date: Tue, 7 Nov 2023 15:43:31 +0100
Subject: [PATCH 2/8] Redirect if error is redirectable

---
 app/controllers/doorkeeper/authorizations_controller.rb | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/app/controllers/doorkeeper/authorizations_controller.rb b/app/controllers/doorkeeper/authorizations_controller.rb
index bae5d21a0..1addd9df2 100644
--- a/app/controllers/doorkeeper/authorizations_controller.rb
+++ b/app/controllers/doorkeeper/authorizations_controller.rb
@@ -47,7 +47,11 @@ def render_error
         render json: pre_auth.error_response.body,
                status: :bad_request
       else
-        render :error, locals: { error_response: pre_auth.error_response }
+        if pre_auth.error_response.redirectable?
+          redirect_or_render(pre_auth.error_response)
+        else
+          render :error, locals: { error_response: pre_auth.error_response }
+        end
       end
     end
 

From 6cd5cca263ed6c1462cbc1d7b1d27f43b8d4e55f Mon Sep 17 00:00:00 2001
From: camero2734 <42698419+camero2734@users.noreply.github.com>
Date: Tue, 7 Nov 2023 15:44:10 +0100
Subject: [PATCH 3/8] Set status of response to error status

---
 app/controllers/doorkeeper/authorizations_controller.rb | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/app/controllers/doorkeeper/authorizations_controller.rb b/app/controllers/doorkeeper/authorizations_controller.rb
index 1addd9df2..33afdc5bc 100644
--- a/app/controllers/doorkeeper/authorizations_controller.rb
+++ b/app/controllers/doorkeeper/authorizations_controller.rb
@@ -44,13 +44,12 @@ def render_error
       pre_auth.error.raise_exception! if Doorkeeper.config.raise_on_errors?
 
       if Doorkeeper.configuration.api_only
-        render json: pre_auth.error_response.body,
-               status: :bad_request
+        render json: pre_auth.error_response.body, status: pre_auth.error_response.status
       else
         if pre_auth.error_response.redirectable?
           redirect_or_render(pre_auth.error_response)
         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
     end

From 09142d0b22a346bb373b49c4a78770e879f64b9d Mon Sep 17 00:00:00 2001
From: camero2734 <42698419+camero2734@users.noreply.github.com>
Date: Tue, 7 Nov 2023 20:44:31 +0100
Subject: [PATCH 4/8] Add option to avoid introducing a breaking change

---
 .../doorkeeper/authorizations_controller.rb            | 10 ++++------
 lib/doorkeeper/config.rb                               |  9 +++++++++
 spec/controllers/authorizations_controller_spec.rb     |  2 +-
 spec/requests/flows/authorization_code_spec.rb         |  2 +-
 spec/requests/flows/implicit_grant_spec.rb             |  2 +-
 5 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/app/controllers/doorkeeper/authorizations_controller.rb b/app/controllers/doorkeeper/authorizations_controller.rb
index 33afdc5bc..fe7132fd7 100644
--- a/app/controllers/doorkeeper/authorizations_controller.rb
+++ b/app/controllers/doorkeeper/authorizations_controller.rb
@@ -43,14 +43,12 @@ def render_success
     def render_error
       pre_auth.error.raise_exception! if Doorkeeper.config.raise_on_errors?
 
-      if Doorkeeper.configuration.api_only
+      if Doorkeeper.configuration.redirect_on_error && 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
-        if pre_auth.error_response.redirectable?
-          redirect_or_render(pre_auth.error_response)
-        else
-          render :error, locals: { error_response: pre_auth.error_response }, status: pre_auth.error_response.status
-        end
+        render :error, locals: { error_response: pre_auth.error_response }, status: pre_auth.error_response.status
       end
     end
 
diff --git a/lib/doorkeeper/config.rb b/lib/doorkeeper/config.rb
index eae3aa70d..48bf6f980 100644
--- a/lib/doorkeeper/config.rb
+++ b/lib/doorkeeper/config.rb
@@ -112,6 +112,11 @@ def api_only
         @config.instance_variable_set(:@api_only, true)
       end
 
+      # Redirect on error instead of rendering error response
+      def redirect_on_error
+        @config.instance_variable_set(:@redirect_on_error, true)
+      end
+
       # Enables polymorphic Resource Owner association for Access Grant and
       # Access Token models. Requires additional database columns to be setup.
       def use_polymorphic_resource_owner
@@ -454,6 +459,10 @@ def api_only
       @api_only ||= false
     end
 
+    def redirect_on_error
+      @redirect_on_error ||= false
+    end
+
     def enforce_content_type
       @enforce_content_type ||= false
     end
diff --git a/spec/controllers/authorizations_controller_spec.rb b/spec/controllers/authorizations_controller_spec.rb
index a1c82ab64..5b0f6bc73 100644
--- a/spec/controllers/authorizations_controller_spec.rb
+++ b/spec/controllers/authorizations_controller_spec.rb
@@ -985,7 +985,7 @@ def query_params
       end
 
       it "renders bad request" do
-        expect(response).to have_http_status(:bad_request)
+        expect(response).to have_http_status(:unauthorized)
       end
 
       it "includes error in body" do
diff --git a/spec/requests/flows/authorization_code_spec.rb b/spec/requests/flows/authorization_code_spec.rb
index f62d82bbc..f32b36cb6 100644
--- a/spec/requests/flows/authorization_code_spec.rb
+++ b/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
diff --git a/spec/requests/flows/implicit_grant_spec.rb b/spec/requests/flows/implicit_grant_spec.rb
index 180c5e7ff..3b7f32a2d 100644
--- a/spec/requests/flows/implicit_grant_spec.rb
+++ b/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

From cfeb44479b56dbbbf16653d6b5978d1038d03ec5 Mon Sep 17 00:00:00 2001
From: camero2734 <42698419+camero2734@users.noreply.github.com>
Date: Tue, 7 Nov 2023 22:16:05 +0100
Subject: [PATCH 5/8] Fix and add specs for raise_exception! handling

---
 .../doorkeeper/authorizations_controller.rb   |   2 +-
 lib/doorkeeper/errors.rb                      |   1 +
 .../oauth/invalid_request_response.rb         |   4 +
 .../authorizations_controller_spec.rb         | 118 ++++++++++++++++++
 4 files changed, 124 insertions(+), 1 deletion(-)

diff --git a/app/controllers/doorkeeper/authorizations_controller.rb b/app/controllers/doorkeeper/authorizations_controller.rb
index fe7132fd7..e463f8ee5 100644
--- a/app/controllers/doorkeeper/authorizations_controller.rb
+++ b/app/controllers/doorkeeper/authorizations_controller.rb
@@ -41,7 +41,7 @@ def render_success
     end
 
     def render_error
-      pre_auth.error.raise_exception! if Doorkeeper.config.raise_on_errors?
+      pre_auth.error_response.raise_exception! if Doorkeeper.config.raise_on_errors?
 
       if Doorkeeper.configuration.redirect_on_error && pre_auth.error_response.redirectable?
         redirect_or_render(pre_auth.error_response)
diff --git a/lib/doorkeeper/errors.rb b/lib/doorkeeper/errors.rb
index 9b91652b2..9c09a263c 100644
--- a/lib/doorkeeper/errors.rb
+++ b/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)
diff --git a/lib/doorkeeper/oauth/invalid_request_response.rb b/lib/doorkeeper/oauth/invalid_request_response.rb
index 12408168a..9b66e6596 100644
--- a/lib/doorkeeper/oauth/invalid_request_response.rb
+++ b/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
diff --git a/spec/controllers/authorizations_controller_spec.rb b/spec/controllers/authorizations_controller_spec.rb
index 5b0f6bc73..cb2e34be4 100644
--- a/spec/controllers/authorizations_controller_spec.rb
+++ b/spec/controllers/authorizations_controller_spec.rb
@@ -1044,6 +1044,124 @@ def query_params
     end
   end
 
+  describe "GET #new with errors with redirect_on_error" do
+    before { config_is_set(:redirect_on_error, true) }
+
+    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,
+        }
+      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 "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"

From 21362a974c368f62f8240cd647034cd1966a12fa Mon Sep 17 00:00:00 2001
From: camero2734 <42698419+camero2734@users.noreply.github.com>
Date: Wed, 8 Nov 2023 11:00:57 +0100
Subject: [PATCH 6/8] Add CHANGELOG entry; Fix spec wording; Add section to
 initializer template

---
 CHANGELOG.md                                       |  1 +
 lib/generators/doorkeeper/templates/initializer.rb | 10 ++++++++++
 spec/controllers/authorizations_controller_spec.rb |  2 +-
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index c1fa776e6..6b73d071f 100644
--- a/CHANGELOG.md
+++ b/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
 
diff --git a/lib/generators/doorkeeper/templates/initializer.rb b/lib/generators/doorkeeper/templates/initializer.rb
index f1f48309b..3cf720f3e 100644
--- a/lib/generators/doorkeeper/templates/initializer.rb
+++ b/lib/generators/doorkeeper/templates/initializer.rb
@@ -313,6 +313,16 @@
   #
   # handle_auth_errors :raise
 
+  # If an exception occurs during the authorization request, Doorkeeper will, by
+  # default, render an HTML error response with the exception message and HTTP
+  # status code. 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
+  # enable this option.
+  #
+  # This will have no effect if handle_auth_errors is set to :raise.
+  #
+  # redirect_on_error
+
   # Customize token introspection response.
   # Allows to add your own fields to default one that are required by the OAuth spec
   # for the introspection response. It could be `sub`, `aud` and so on.
diff --git a/spec/controllers/authorizations_controller_spec.rb b/spec/controllers/authorizations_controller_spec.rb
index cb2e34be4..2798ad3d5 100644
--- a/spec/controllers/authorizations_controller_spec.rb
+++ b/spec/controllers/authorizations_controller_spec.rb
@@ -984,7 +984,7 @@ def query_params
         }
       end
 
-      it "renders bad request" do
+      it "renders unauthorized" do
         expect(response).to have_http_status(:unauthorized)
       end
 

From 423dbe49fb96be857ad4b705c70cbaa066d5ce73 Mon Sep 17 00:00:00 2001
From: camero2734 <42698419+camero2734@users.noreply.github.com>
Date: Wed, 8 Nov 2023 11:09:16 +0100
Subject: [PATCH 7/8] Check that state is returned in redirect, according to
 spec

---
 spec/controllers/authorizations_controller_spec.rb | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/spec/controllers/authorizations_controller_spec.rb b/spec/controllers/authorizations_controller_spec.rb
index 2798ad3d5..26cfa412b 100644
--- a/spec/controllers/authorizations_controller_spec.rb
+++ b/spec/controllers/authorizations_controller_spec.rb
@@ -1071,6 +1071,7 @@ def query_params
           response_type: "token",
           scope: "invalid",
           redirect_uri: client.redirect_uri,
+          state: "return-this",
         }
       end
 
@@ -1087,6 +1088,11 @@ def query_params
         expect(response.query_params["error_description"]).to eq(translated_error_message(:invalid_scope))
       end
 
+      it "includes state in fragment" do
+        pry
+        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

From 41ae563299d031c487b855681928e1966829e2b2 Mon Sep 17 00:00:00 2001
From: camero2734 <42698419+camero2734@users.noreply.github.com>
Date: Wed, 8 Nov 2023 19:48:47 +0100
Subject: [PATCH 8/8] Use existing handle_auth_errors option instead of
 creating a new one

---
 .../doorkeeper/authorizations_controller.rb         |  2 +-
 lib/doorkeeper/config.rb                            | 13 ++++---------
 lib/generators/doorkeeper/templates/initializer.rb  | 12 ++++--------
 spec/controllers/authorizations_controller_spec.rb  |  5 ++---
 4 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/app/controllers/doorkeeper/authorizations_controller.rb b/app/controllers/doorkeeper/authorizations_controller.rb
index e463f8ee5..7601a5cdd 100644
--- a/app/controllers/doorkeeper/authorizations_controller.rb
+++ b/app/controllers/doorkeeper/authorizations_controller.rb
@@ -43,7 +43,7 @@ def render_success
     def render_error
       pre_auth.error_response.raise_exception! if Doorkeeper.config.raise_on_errors?
 
-      if Doorkeeper.configuration.redirect_on_error && pre_auth.error_response.redirectable?
+      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
diff --git a/lib/doorkeeper/config.rb b/lib/doorkeeper/config.rb
index 48bf6f980..2391cabd6 100644
--- a/lib/doorkeeper/config.rb
+++ b/lib/doorkeeper/config.rb
@@ -112,11 +112,6 @@ def api_only
         @config.instance_variable_set(:@api_only, true)
       end
 
-      # Redirect on error instead of rendering error response
-      def redirect_on_error
-        @config.instance_variable_set(:@redirect_on_error, true)
-      end
-
       # Enables polymorphic Resource Owner association for Access Grant and
       # Access Token models. Requires additional database columns to be setup.
       def use_polymorphic_resource_owner
@@ -459,10 +454,6 @@ def api_only
       @api_only ||= false
     end
 
-    def redirect_on_error
-      @redirect_on_error ||= false
-    end
-
     def enforce_content_type
       @enforce_content_type ||= false
     end
@@ -510,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
diff --git a/lib/generators/doorkeeper/templates/initializer.rb b/lib/generators/doorkeeper/templates/initializer.rb
index 3cf720f3e..d1d2aea3d 100644
--- a/lib/generators/doorkeeper/templates/initializer.rb
+++ b/lib/generators/doorkeeper/templates/initializer.rb
@@ -312,16 +312,12 @@
   #   Doorkeeper::Errors::TokenRevoked, Doorkeeper::Errors::TokenUnknown
   #
   # handle_auth_errors :raise
-
-  # If an exception occurs during the authorization request, Doorkeeper will, by
-  # default, render an HTML error response with the exception message and HTTP
-  # status code. 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
-  # enable this option.
   #
-  # This will have no effect if handle_auth_errors is set to :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
   #
-  # redirect_on_error
+  # handle_auth_errors :redirect
 
   # Customize token introspection response.
   # Allows to add your own fields to default one that are required by the OAuth spec
diff --git a/spec/controllers/authorizations_controller_spec.rb b/spec/controllers/authorizations_controller_spec.rb
index 26cfa412b..523438ed1 100644
--- a/spec/controllers/authorizations_controller_spec.rb
+++ b/spec/controllers/authorizations_controller_spec.rb
@@ -1044,8 +1044,8 @@ def query_params
     end
   end
 
-  describe "GET #new with errors with redirect_on_error" do
-    before { config_is_set(:redirect_on_error, true) }
+  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
@@ -1089,7 +1089,6 @@ def query_params
       end
 
       it "includes state in fragment" do
-        pry
         expect(response.query_params["state"]).to eq("return-this")
       end