Browse Source

Fixes issue #2983 - HTTP 401 responses causing issues with Basic Authentication.

Thorsten Eckel 4 years ago
parent
commit
876c0b18fd

+ 2 - 2
app/assets/javascripts/app/controllers/_profile/out_of_office.coffee

@@ -148,8 +148,8 @@ class ProfileOutOfOffice extends App.ControllerSubContent
     data = JSON.parse(xhr.responseText)
 
     # show error message
-    if xhr.status is 401 || error is 'Unauthorized'
-      message     = '» ' + App.i18n.translateInline('Unauthorized') + ' «'
+    if xhr.status is 403 || error is 'Not authorized'
+      message     = '» ' + App.i18n.translateInline('Not authorized') + ' «'
     else if xhr.status is 404 || error is 'Not Found'
       message     = '» ' + App.i18n.translateInline('Not Found') + ' «'
     else if data.error

+ 2 - 2
app/assets/javascripts/app/controllers/ticket_zoom.coffee

@@ -115,8 +115,8 @@ class App.TicketZoom extends App.Controller
           return
 
         # show error message
-        if status is 401 || statusText is 'Unauthorized'
-          @taskHead      = '» ' + App.i18n.translateInline('Unauthorized') + ' «'
+        if status is 403 || statusText is 'Not authorized'
+          @taskHead      = '» ' + App.i18n.translateInline('Not authorized') + ' «'
           @taskIconClass = 'diagonal-cross'
           @renderScreenUnauthorized(objectName: 'Ticket')
         else if status is 404 || statusText is 'Not Found'

+ 2 - 1
app/assets/javascripts/app/lib/app_post/ajax.coffee

@@ -93,8 +93,9 @@ class _ajaxSingleton
       # 200, all is fine
       return if status is 200
 
-      # do not show any error message with code 401/404 (handled by controllers)
+      # do not show any error message for various 4** codes (handled by controllers)
       return if status is 401
+      return if status is 403
       return if status is 404
       return if status is 422
 

+ 20 - 9
app/controllers/application_controller/authenticates.rb

@@ -14,12 +14,12 @@ module ApplicationController::Authenticates
       )
       return false if user
 
-      raise Exceptions::NotAuthorized, 'Not authorized (token)!'
+      raise Exceptions::Forbidden, 'Not authorized (token)!'
     end
 
     return false if current_user&.permissions?(key)
 
-    raise Exceptions::NotAuthorized, 'Not authorized (user)!'
+    raise Exceptions::Forbidden, 'Not authorized (user)!'
   end
 
   def authentication_check(auth_param = {})
@@ -33,7 +33,7 @@ module ApplicationController::Authenticates
 
     # return auth not ok
     if !user
-      raise Exceptions::NotAuthorized, 'authentication failed'
+      raise Exceptions::Forbidden, 'Authentication required'
     end
 
     # return auth ok
@@ -45,12 +45,15 @@ module ApplicationController::Authenticates
     #logger.debug params.inspect
     #logger.debug session.inspect
     #logger.debug cookies.inspect
+    authentication_errors = []
 
     # already logged in, early exit
     if session.id && session[:user_id]
       logger.debug { 'session based auth check' }
       user = User.lookup(id: session[:user_id])
       return authentication_check_prerequesits(user, 'session', auth_param) if user
+
+      authentication_errors.push("Can't find User with ID #{session[:user_id]} from Session")
     end
 
     # check http basic based authentication
@@ -58,11 +61,13 @@ module ApplicationController::Authenticates
       request.session_options[:skip] = true # do not send a session cookie
       logger.debug { "http basic auth check '#{username}'" }
       if Setting.get('api_password_access') == false
-        raise Exceptions::NotAuthorized, 'API password access disabled!'
+        raise Exceptions::Forbidden, 'API password access disabled!'
       end
 
       user = User.authenticate(username, password)
       return authentication_check_prerequesits(user, 'basic_auth', auth_param) if user
+
+      authentication_errors.push('Invalid BasicAuth credentials')
     end
 
     # check http token based authentication
@@ -70,7 +75,7 @@ module ApplicationController::Authenticates
       logger.debug { "http token auth check '#{token_string}'" }
       request.session_options[:skip] = true # do not send a session cookie
       if Setting.get('api_token_access') == false
-        raise Exceptions::NotAuthorized, 'API token access disabled!'
+        raise Exceptions::Forbidden, 'API token access disabled!'
       end
 
       user = Token.check(
@@ -106,13 +111,15 @@ module ApplicationController::Authenticates
 
       @_token_auth = token_string # remember for permission_check
       return authentication_check_prerequesits(user, 'token_auth', auth_param) if user
+
+      authentication_errors.push("Can't find User for Token")
     end
 
     # check oauth2 token based authentication
     token = Doorkeeper::OAuth::Token.from_bearer_authorization(request)
     if token
       request.session_options[:skip] = true # do not send a session cookie
-      logger.debug { "oauth2 token auth check '#{token}'" }
+      logger.debug { "OAuth2 token auth check '#{token}'" }
       access_token = Doorkeeper::AccessToken.by_token(token)
 
       raise Exceptions::NotAuthorized, 'Invalid token!' if !access_token
@@ -128,9 +135,13 @@ module ApplicationController::Authenticates
 
       user = User.find(access_token.resource_owner_id)
       return authentication_check_prerequesits(user, 'token_auth', auth_param) if user
+
+      authentication_errors.push("Can't find User with ID #{access_token.resource_owner_id} for OAuth2 token")
     end
 
-    false
+    return false if authentication_errors.blank?
+
+    raise Exceptions::NotAuthorized, authentication_errors.join(', ')
   end
 
   def authenticate_with_password
@@ -142,7 +153,7 @@ module ApplicationController::Authenticates
   end
 
   def authentication_check_prerequesits(user, auth_type, auth_param)
-    raise Exceptions::NotAuthorized, 'Maintenance mode enabled!' if in_maintenance_mode?(user)
+    raise Exceptions::Forbidden, 'Maintenance mode enabled!' if in_maintenance_mode?(user)
 
     raise_unified_login_error if !user.active
 
@@ -150,7 +161,7 @@ module ApplicationController::Authenticates
       ActiveSupport::Deprecation.warn("Parameter ':permission' is deprecated. Use Pundit policy and `authorize!` instead.")
 
       if !user.permissions?(auth_param[:permission])
-        raise Exceptions::NotAuthorized, 'Not authorized (user)!'
+        raise Exceptions::Forbidden, 'Not authorized (user)!'
       end
     end
 

+ 1 - 1
app/controllers/application_controller/authorizes.rb

@@ -11,7 +11,7 @@ module ApplicationController::Authorizes
   def authorized?(record = policy_record, query = nil)
     authorize!(record, query)
     true
-  rescue Exceptions::NotAuthorized, Pundit::NotAuthorizedError
+  rescue Exceptions::Forbidden, Pundit::NotAuthorizedError
     false
   end
 

+ 16 - 4
app/controllers/application_controller/handles_errors.rb

@@ -11,6 +11,7 @@ module ApplicationController::HandlesErrors
     rescue_from ArgumentError, with: :unprocessable_entity
     rescue_from Exceptions::UnprocessableEntity, with: :unprocessable_entity
     rescue_from Exceptions::NotAuthorized, with: :unauthorized
+    rescue_from Exceptions::Forbidden, with: :forbidden
     rescue_from Pundit::NotAuthorizedError, with: :pundit_not_authorized_error
   end
 
@@ -40,13 +41,21 @@ module ApplicationController::HandlesErrors
     http_log
   end
 
+  def forbidden(e)
+    logger.info { e }
+    error = humanize_error(e)
+    response.headers['X-Failure'] = error.fetch(:error_human, error[:error])
+    respond_to_exception(e, :forbidden)
+    http_log
+  end
+
   def pundit_not_authorized_error(e)
     logger.info { e }
     # check if a special authorization_error should be shown in the result payload
     # which was raised in one of the policies. Fall back to a simple "Not authorized"
     # error to hide actual cause for security reasons.
-    exeption = e.policy.custom_exception || Exceptions::NotAuthorized.new
-    unauthorized(exeption)
+    exeption = e.policy&.custom_exception || Exceptions::Forbidden.new('Not authorized')
+    forbidden(exeption)
   end
 
   private
@@ -79,10 +88,13 @@ module ApplicationController::HandlesErrors
       data[:error_human] = 'Object already exists!'
     elsif e.message =~ /null value in column "(.+?)" violates not-null constraint/i || e.message =~ /Field '(.+?)' doesn't have a default value/i
       data[:error_human] = "Attribute '#{$1}' required!"
-    elsif e.message == 'Exceptions::NotAuthorized'
+    elsif e.message == 'Exceptions::Forbidden'
       data[:error]       = 'Not authorized'
       data[:error_human] = data[:error]
-    elsif [ActionController::RoutingError, ActiveRecord::RecordNotFound, Exceptions::UnprocessableEntity, Exceptions::NotAuthorized].include?(e.class)
+    elsif e.message == 'Exceptions::NotAuthorized'
+      data[:error]       = 'Authorization failed'
+      data[:error_human] = data[:error]
+    elsif [ActionController::RoutingError, ActiveRecord::RecordNotFound, Exceptions::UnprocessableEntity, Exceptions::NotAuthorized, Exceptions::Forbidden].include?(e.class)
       data[:error_human] = data[:error]
     end
 

+ 2 - 2
app/controllers/application_controller/has_user.rb

@@ -43,7 +43,7 @@ module ApplicationController::HasUser
     return if !user_real
 
     # check if the user has admin rights
-    raise Exceptions::NotAuthorized, "Current user has no permission to use 'X-On-Behalf-Of'!" if !user_real.permissions?('admin.user')
+    raise Exceptions::Forbidden, "Current user has no permission to use 'X-On-Behalf-Of'!" if !user_real.permissions?('admin.user')
 
     # find user for execution based on the header
     %i[id login email].each do |field|
@@ -55,7 +55,7 @@ module ApplicationController::HasUser
     end
 
     # no behalf of user found
-    raise Exceptions::NotAuthorized, "No such user '#{request.headers['X-On-Behalf-Of']}'"
+    raise Exceptions::Forbidden, "No such user '#{request.headers['X-On-Behalf-Of']}'"
   end
 
   def search_attributes(field)

+ 1 - 1
app/controllers/attachments_controller.rb

@@ -75,7 +75,7 @@ class AttachmentsController < ApplicationController
     valid_disposition = %w[inline attachment]
     return disposition if valid_disposition.include?(disposition)
 
-    raise Exceptions::NotAuthorized, "Invalid disposition #{disposition} requested. Only #{valid_disposition.join(', ')} are valid."
+    raise Exceptions::Forbidden, "Invalid disposition #{disposition} requested. Only #{valid_disposition.join(', ')} are valid."
   end
 
   def verify_object_permissions

+ 4 - 2
app/controllers/calendar_subscriptions_controller.rb

@@ -7,7 +7,8 @@ class CalendarSubscriptionsController < ApplicationController
   # @summary          Returns an iCal file with all objects matching the calendar subscriptions preferences of the current user as events.
   #
   # @response_message 200 [String] iCal file ready to import in calendar applications.
-  # @response_message 401          Permission denied.
+  # @response_message 403          Forbidden / Invalid session.
+  # @response_message 422          Unprocessable Entity.
   def all
     calendar_subscriptions = CalendarSubscriptions.new(current_user)
     ical                   = calendar_subscriptions.all
@@ -29,7 +30,8 @@ class CalendarSubscriptionsController < ApplicationController
   # @summary          Returns an iCal file of the given object (and method) matching the calendar subscriptions preferences of the current user as events.
   #
   # @response_message 200 [String] iCal file ready to import in calendar applications.
-  # @response_message 401          Permission denied.
+  # @response_message 403          Forbidden / Invalid session.
+  # @response_message 422          Unprocessable Entity.
   def object
     calendar_subscriptions = CalendarSubscriptions.new(current_user)
     ical                   = calendar_subscriptions.generic(params[:object], params[:method])

+ 2 - 2
app/controllers/channels_email_controller.rb

@@ -267,7 +267,7 @@ class ChannelsEmailController < ApplicationController
   def check_online_service
     return true if !Setting.get('system_online_service')
 
-    raise Exceptions::NotAuthorized
+    raise Exceptions::Forbidden
   end
 
   def check_access(id = nil)
@@ -279,6 +279,6 @@ class ChannelsEmailController < ApplicationController
     channel = Channel.find(id)
     return true if channel.preferences && !channel.preferences[:online_service_disable]
 
-    raise Exceptions::NotAuthorized
+    raise Exceptions::Forbidden
   end
 end

Some files were not shown because too many files changed in this diff