From f7c53b1a20512b73a0280ab2dbac520ec708cf52 Mon Sep 17 00:00:00 2001
From: Mantas Masalskis <mm@zammad.com>
Date: Thu, 5 Aug 2021 15:57:00 +0200
Subject: [PATCH] =?UTF-8?q?Fixes=20#2851=20-=20Wrong=20user=20is=20used=20?=
 =?UTF-8?q?when=20"X-On-Behalf-Of=E2=80=9D=20header=20value=20is=20an=20em?=
 =?UTF-8?q?ail=20that=20starts=20with=20digits?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 .../application_controller/has_user.rb        | 27 ++++++++++++-----
 spec/requests/api_auth_on_behalf_of_spec.rb   | 29 +++++++++++++++++++
 2 files changed, 49 insertions(+), 7 deletions(-)

diff --git a/app/controllers/application_controller/has_user.rb b/app/controllers/application_controller/has_user.rb
index beb53457a..51fc9f2c6 100644
--- a/app/controllers/application_controller/has_user.rb
+++ b/app/controllers/application_controller/has_user.rb
@@ -34,15 +34,14 @@ module ApplicationController::HasUser
       raise Exceptions::Forbidden, "Current user has no permission to use 'X-On-Behalf-Of'!"
     end
 
-    # find user for execution based on the header
-    %i[id login email].each do |field|
-      @_user_on_behalf = User.find_by(field => request.headers['X-On-Behalf-Of'].to_s.downcase.strip)
-
-      return @_user_on_behalf if @_user_on_behalf
-    end
+    @_user_on_behalf = find_on_behalf_user request.headers['X-On-Behalf-Of'].to_s.downcase.strip
 
     # no behalf of user found
-    raise Exceptions::Forbidden, "No such user '#{request.headers['X-On-Behalf-Of']}'"
+    if !@_user_on_behalf
+      raise Exceptions::Forbidden, "No such user '#{request.headers['X-On-Behalf-Of']}'"
+    end
+
+    @_user_on_behalf
   end
 
   def current_user_set(user, auth_type = 'session')
@@ -77,4 +76,18 @@ module ApplicationController::HasUser
 
     session[:user_agent] = request.env['HTTP_USER_AGENT']
   end
+
+  # find on behalf user by ID, login or email
+  def find_on_behalf_user(identifier)
+    # ActiveRecord casts string beginning with a numeric characters
+    # to numeric characters by dropping textual bits altogether
+    # thus 123@example.com returns user with ID 123
+    if identifier.match?(%r{^\d+$})
+      user = User.find_by(id: identifier)
+      return user if user
+    end
+
+    # find user for execution based on the header
+    User.where('login = :param OR email = :param', param: identifier).first
+  end
 end
diff --git a/spec/requests/api_auth_on_behalf_of_spec.rb b/spec/requests/api_auth_on_behalf_of_spec.rb
index 74f6f99c6..17d22095c 100644
--- a/spec/requests/api_auth_on_behalf_of_spec.rb
+++ b/spec/requests/api_auth_on_behalf_of_spec.rb
@@ -241,4 +241,33 @@ RSpec.describe 'Api Auth On Behalf Of', type: :request do
       end
     end
   end
+
+  describe 'user lookup' do
+    it 'does X-On-Behalf-Of auth - user lookup by ID' do
+      authenticated_as(admin, on_behalf_of: customer.id)
+      get '/api/v1/users/me', as: :json
+      expect(json_response.fetch('id')).to be customer.id
+    end
+
+    it 'does X-On-Behalf-Of auth - user lookup by login' do
+      authenticated_as(admin, on_behalf_of: customer.login)
+      get '/api/v1/users/me', as: :json
+      expect(json_response.fetch('id')).to be customer.id
+    end
+
+    it 'does X-On-Behalf-Of auth - user lookup by email' do
+      authenticated_as(admin, on_behalf_of: customer.email)
+      get '/api/v1/users/me', as: :json
+      expect(json_response.fetch('id')).to be customer.id
+    end
+
+    # https://github.com/zammad/zammad/issues/2851
+    it 'does X-On-Behalf-Of auth - user lookup by email even if email starts with a digit' do
+      customer.update! email: "#{agent.id}#{customer.email}"
+
+      authenticated_as(admin, on_behalf_of: customer.email)
+      get '/api/v1/users/me', as: :json
+      expect(json_response.fetch('id')).to be customer.id
+    end
+  end
 end