From 0f5807d6feb4fe67e4746df609ce9a312b388a67 Mon Sep 17 00:00:00 2001
From: Martin Gruner <mg@zammad.com>
Date: Wed, 8 Sep 2021 14:31:42 +0200
Subject: [PATCH] Maintenance: Improved updating of user records in the front
 end.

---
 app/assets/javascripts/app/models/user.coffee |  5 +-
 spec/system/manage/users_spec.rb              | 69 +++++++++++++++++++
 2 files changed, 73 insertions(+), 1 deletion(-)

diff --git a/app/assets/javascripts/app/models/user.coffee b/app/assets/javascripts/app/models/user.coffee
index f1ed97298..423035030 100644
--- a/app/assets/javascripts/app/models/user.coffee
+++ b/app/assets/javascripts/app/models/user.coffee
@@ -344,9 +344,12 @@ class App.User extends App.Model
     @sameOrganization?(requester)
 
   isChangeableBy: (requester) ->
+    # full access for admins
     return true if requester.permission('admin.user')
-    # allow agents to change customers
+    # forbid non-agents to change users
     return false if !requester.permission('ticket.agent')
+    # allow agents to change customers only
+    return false if @permission(['admin.user', 'ticket.agent'])
     @permission('ticket.customer')
 
   isDeleteableBy: (requester) ->
diff --git a/spec/system/manage/users_spec.rb b/spec/system/manage/users_spec.rb
index 8072ba604..fe10293a9 100644
--- a/spec/system/manage/users_spec.rb
+++ b/spec/system/manage/users_spec.rb
@@ -110,4 +110,73 @@ RSpec.describe 'Manage > Users', type: :system do
       end
     end
   end
+
+  describe 'check user edit permissions', authenticated_as: -> { user } do
+
+    shared_examples 'user permission' do |allow|
+      it(allow ? 'allows editing' : 'forbids editing') do
+        visit "#user/profile/#{record.id}"
+        find('.js-action .icon-arrow-down').click
+        selector = '.js-action [data-type="edit"]'
+        expect(page).to(allow ? have_css(selector) : have_no_css(selector))
+      end
+    end
+
+    context 'when admin tries to change admin' do
+      let(:user) { create(:admin) }
+      let(:record) { create(:admin) }
+
+      include_examples 'user permission', true
+    end
+
+    context 'when admin tries to change agent' do
+      let(:user) { create(:admin) }
+      let(:record) { create(:agent) }
+
+      include_examples 'user permission', true
+    end
+
+    context 'when admin tries to change customer' do
+      let(:user) { create(:admin) }
+      let(:record) { create(:customer) }
+
+      include_examples 'user permission', true
+    end
+
+    context 'when agent tries to change admin' do
+      let(:user) { create(:agent) }
+      let(:record) { create(:admin) }
+
+      include_examples 'user permission', false
+    end
+
+    context 'when agent tries to change agent' do
+      let(:user) { create(:agent) }
+      let(:record) { create(:agent) }
+
+      include_examples 'user permission', false
+    end
+
+    context 'when agent tries to change customer' do
+      let(:user) { create(:agent) }
+      let(:record) { create(:customer) }
+
+      include_examples 'user permission', true
+    end
+
+    context 'when agent tries to change customer who is also admin' do
+      let(:user) { create(:agent) }
+      let(:record) { create(:customer, role_ids: Role.signup_role_ids.push(Role.find_by(name: 'Admin').id)) }
+
+      include_examples 'user permission', false
+    end
+
+    context 'when agent tries to change customer who is also agent' do
+      let(:user) { create(:agent) }
+      let(:record) { create(:customer, role_ids: Role.signup_role_ids.push(Role.find_by(name: 'Agent').id)) }
+
+      include_examples 'user permission', false
+    end
+
+  end
 end