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_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
- 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

@@ -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

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

@@ -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.

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,130 @@ 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,
+ 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
+ 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
+ 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