Prevent requested scope be empty on authorization request, handle and add description for invalid request error

CHANGELOG.md

@@ -15,6 +15,7 @@ User-visible changes worth mentioning.
 - [#1286] Add ability to customize grant flows per application (OAuth client) (#1245 , #1207)
 - [#1283] Allow to customize base class for `Doorkeeper::ApplicationMetalController` (new configuration
   option called `base_metal_controller` (fix #1273).
+- [#1277] Prevent requested scope be empty on authorization request, handle and add description for invalid request.
 
 ## 5.2.0.rc2
 

app/controllers/doorkeeper/authorizations_controller.rb

@@ -12,7 +12,6 @@ def new
       end
     end
 
-    # TODO: Handle raise invalid authorization
     def create
       redirect_or_render authorize_response
     end
@@ -84,10 +83,11 @@ def strategy
 
     def authorize_response
       @authorize_response ||= begin
-        authorizable = pre_auth.authorizable?
-        before_successful_authorization if authorizable
+        return pre_auth.error_response unless pre_auth.authorizable?
+
+        before_successful_authorization
         auth = strategy.authorize
-        after_successful_authorization if authorizable
+        after_successful_authorization
         auth
       end
     end

config/locales/en.yml

@@ -88,7 +88,11 @@ en:
     errors:
       messages:
         # Common error messages
-        invalid_request: 'The request is missing a required parameter, includes an unsupported parameter value, or is otherwise malformed.'
+        invalid_request:
+          unknown: 'The request is missing a required parameter, includes an unsupported parameter value, or is otherwise malformed.'
+          missing_param: 'Missing required parameter: #{value}.'
+          not_support_pkce: 'Invalid code_verifier parameter. Server does not support pkce.'
+          request_not_authorized: 'Request need to be authorized. Required parameter for authorizing request is missing or invalid.'
         invalid_redirect_uri: "The requested redirect uri is malformed or doesn't match client redirect URI."
         unauthorized_client: 'The client is not authorized to perform this request using this method.'
         access_denied: 'The resource owner or authorization server denied the request.'

lib/doorkeeper.rb

@@ -52,6 +52,7 @@
 require "doorkeeper/oauth/token_introspection"
 require "doorkeeper/oauth/invalid_token_response"
 require "doorkeeper/oauth/forbidden_token_response"
+require "doorkeeper/oauth/invalid_request_response"
 require "doorkeeper/oauth/nonstandard"
 
 require "doorkeeper/secret_storing/base"

lib/doorkeeper/errors.rb

@@ -8,18 +8,6 @@ def type
       end
     end
 
-    class InvalidAuthorizationStrategy < DoorkeeperError
-      def type
-        :unsupported_response_type
-      end
-    end
-
-    class InvalidTokenReuse < DoorkeeperError
-      def type
-        :invalid_request
-      end
-    end
-
     class InvalidGrantReuse < DoorkeeperError
       def type
         :invalid_grant
@@ -32,7 +20,14 @@ def type
       end
     end
 
-    class MissingRequestStrategy < DoorkeeperError
+    class MissingRequiredParameter < DoorkeeperError
+      attr_reader :missing_param
+
+      def initialize(missing_param)
+        super
+        @missing_param = missing_param
+      end
+
       def type
         :invalid_request
       end

lib/doorkeeper/helpers/controller.rb

@@ -44,6 +44,10 @@ def config_methods
       def get_error_response_from_exception(exception)
         if exception.respond_to?(:response)
           exception.response
+        elsif exception.type == :invalid_request
+          OAuth::InvalidRequestResponse.new(name: exception.type,
+                                            state: params[:state],
+                                            missing_param: exception.missing_param)
         else
           OAuth::ErrorResponse.new(name: exception.type, state: params[:state])
         end

lib/doorkeeper/oauth/authorization_code_request.rb

@@ -3,7 +3,8 @@
 module Doorkeeper
   module OAuth
     class AuthorizationCodeRequest < BaseRequest
-      validate :attributes,   error: :invalid_request
+      validate :pkce_support, error: :invalid_request
+      validate :params,       error: :invalid_request
       validate :client,       error: :invalid_client
       validate :grant,        error: :invalid_grant
       # @see https://tools.ietf.org/html/rfc6749#section-5.2
@@ -12,6 +13,7 @@ class AuthorizationCodeRequest < BaseRequest
 
       attr_accessor :server, :grant, :client, :redirect_uri, :access_token,
                     :code_verifier
+      attr_reader :invalid_request_reason, :missing_param
 
       def initialize(server, grant, client, parameters = {})
         @server = server
@@ -24,10 +26,6 @@ def initialize(server, grant, client, parameters = {})
 
       private
 
-      def client_by_uid(parameters)
-        Doorkeeper::Application.by_uid(parameters[:client_id])
-      end
-
       def before_successful_response
         grant.transaction do
           grant.lock!
@@ -42,11 +40,22 @@ def before_successful_response
         super
       end
 
-      def validate_attributes
-        return false if grant&.uses_pkce? && code_verifier.blank?
-        return false if grant && !grant.pkce_supported? && !code_verifier.blank?
+      def validate_pkce_support
+        @invalid_request_reason = :not_support_pkce if grant &&
+                                                       !grant.pkce_supported? &&
+                                                       code_verifier.present?
+
+        @invalid_request_reason.nil?
+      end
+
+      def validate_params
+        @missing_param = if grant&.uses_pkce? && code_verifier.blank?
+                           :code_verifier
+                         elsif redirect_uri.blank?
+                           :redirect_uri
+                         end
 
-        redirect_uri.present?
+        @missing_param.nil?
       end
 
       def validate_client

lib/doorkeeper/oauth/base_request.rb

@@ -15,6 +15,8 @@ def authorize
           @response = TokenResponse.new(access_token)
           after_successful_response
           @response
+        elsif error == :invalid_request
+          @response = InvalidRequestResponse.from_request(self)
         else
           @response = ErrorResponse.from_request(self)
         end

lib/doorkeeper/oauth/code_request.rb

@@ -3,31 +3,22 @@
 module Doorkeeper
   module OAuth
     class CodeRequest
-      attr_accessor :pre_auth, :resource_owner, :client
+      attr_accessor :pre_auth, :resource_owner
 
       def initialize(pre_auth, resource_owner)
         @pre_auth       = pre_auth
-        @client         = pre_auth.client
         @resource_owner = resource_owner
       end
 
       def authorize
-        if pre_auth.authorizable?
-          auth = Authorization::Code.new(pre_auth, resource_owner)
-          auth.issue_token
-          @response = CodeResponse.new(pre_auth, auth)
-        else
-          @response = ErrorResponse.from_request(pre_auth)
-        end
+        auth = Authorization::Code.new(pre_auth, resource_owner)
+        auth.issue_token
+        CodeResponse.new(pre_auth, auth)
       end
 
       def deny
         pre_auth.error = :access_denied
-
-        ErrorResponse.from_request(
-          pre_auth,
-          redirect_uri: pre_auth.redirect_uri
-        )
+        pre_auth.error_response
       end
     end
   end

lib/doorkeeper/oauth/invalid_request_response.rb

@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+module Doorkeeper
+  module OAuth
+    class InvalidRequestResponse < ErrorResponse
+      attr_reader :reason
+
+      def self.from_request(request, attributes = {})
+        new(
+          attributes.merge(
+            state: request.try(:state),
+            redirect_uri: request.try(:redirect_uri),
+            missing_param: request.try(:missing_param),
+            reason: request.try(:invalid_request_reason)
+          )
+        )
+      end
+
+      def initialize(attributes = {})
+        super(attributes.merge(name: :invalid_request))
+        @missing_param = attributes[:missing_param]
+        @reason = @missing_param.nil? ? attributes[:reason] : :missing_param
+      end
+
+      def status
+        :bad_request
+      end
+
+      def description
+        I18n.translate(
+          reason,
+          scope: %i[doorkeeper errors messages invalid_request],
+          default: :unknown,
+          value: @missing_param
+        )
+      end
+
+      def redirectable?
+        super && @missing_param != :client_id
+      end
+    end
+  end
+end

lib/doorkeeper/oauth/pre_authorization.rb

@@ -5,15 +5,17 @@ module OAuth
     class PreAuthorization
       include Validations
 
-      validate :response_type, error: :unsupported_response_type
-      validate :client, error: :invalid_client
-      validate :scopes, error: :invalid_scope
-      validate :redirect_uri, error: :invalid_redirect_uri
+      validate :client_id,             error: :invalid_request
+      validate :client,                error: :invalid_client
+      validate :redirect_uri,          error: :invalid_redirect_uri
+      validate :params,                error: :invalid_request
+      validate :response_type,         error: :unsupported_response_type
+      validate :scopes,                error: :invalid_scope
       validate :code_challenge_method, error: :invalid_code_challenge_method
       validate :client_supports_grant_flow, error: :unauthorized_client
 
-      attr_reader :server, :client, :client_id, :response_type, :redirect_uri,
-                  :state, :code_challenge, :code_challenge_method
+      attr_reader :server, :client_id, :client, :redirect_uri, :response_type, :state,
+                  :code_challenge, :code_challenge_method, :missing_param
 
       def initialize(server, attrs = {})
         @server                = server
@@ -39,11 +41,17 @@ def scopes
       end
 
       def scope
-        @scope.presence || build_scopes
+        @scope.presence || (server.default_scopes.presence && build_scopes)
       end
 
       def error_response
-        OAuth::ErrorResponse.from_request(self)
+        is_implicit_flow = response_type == "token"
+
+        if error == :invalid_request
+          OAuth::InvalidRequestResponse.from_request(self, response_on_fragment: is_implicit_flow)
+        else
+          OAuth::ErrorResponse.from_request(self, response_on_fragment: is_implicit_flow)
+        end
       end
 
       def as_json(attributes = {})
@@ -63,18 +71,41 @@ def build_scopes
         end
       end
 
-      def validate_response_type
-        server.authorization_response_types.include?(response_type)
+      def validate_client_id
+        @missing_param = :client_id if client_id.blank?
+
+        @missing_param.nil?
       end
 
       def validate_client
         @client = OAuth::Client.find(client_id)
         @client.present?
       end
 
-      def validate_scopes
-        return true if scope.blank?
+      def validate_redirect_uri
+        return false if redirect_uri.blank?
+
+        Helpers::URIChecker.valid_for_authorization?(
+          redirect_uri,
+          client.redirect_uri
+        )
+      end
+
+      def validate_params
+        @missing_param = if response_type.blank?
+                           :response_type
+                         elsif @scope.blank? && server.default_scopes.blank?
+                           :scope
+                         end
 
+        @missing_param.nil?
+      end
+
+      def validate_response_type
+        server.authorization_response_types.include?(response_type)
+      end
+
+      def validate_scopes
         Helpers::ScopeChecker.valid?(
           scope_str: scope,
           server_scopes: server.scopes,
@@ -87,15 +118,6 @@ def grant_type
         response_type == "code" ? AUTHORIZATION_CODE : IMPLICIT
       end
 
-      def validate_redirect_uri
-        return false if redirect_uri.blank?
-
-        Helpers::URIChecker.valid_for_authorization?(
-          redirect_uri,
-          client.redirect_uri
-        )
-      end
-
       def validate_code_challenge_method
         code_challenge.blank? ||
           (code_challenge_method.present? && code_challenge_method =~ /^plain$|^S256$/)

lib/doorkeeper/oauth/refresh_token_request.rb

@@ -13,6 +13,7 @@ class RefreshTokenRequest < BaseRequest
 
       attr_accessor :access_token, :client, :credentials, :refresh_token,
                     :server
+      attr_reader   :missing_param
 
       def initialize(server, refresh_token, credentials, parameters = {})
         @server = server
@@ -32,7 +33,7 @@ def load_client(credentials)
       def before_successful_response
         refresh_token.transaction do
           refresh_token.lock!
-          raise Errors::InvalidTokenReuse if refresh_token.revoked?
+          raise Errors::InvalidGrantReuse if refresh_token.revoked?
 
           refresh_token.revoke unless refresh_token_revoked_on_use?
           create_access_token
@@ -76,7 +77,9 @@ def access_token_expires_in
       end
 
       def validate_token_presence
-        refresh_token.present? || @refresh_token_parameter.present?
+        @missing_param = :refresh_token if refresh_token.blank? && @refresh_token_parameter.blank?
+
+        @missing_param.nil?
       end
 
       def validate_token