From d52634701d7cf6c9be5cc7b68e78afed21051d31 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Wed, 28 Mar 2018 07:33:41 +0200 Subject: [PATCH] Fixed issue #1794 - Admin users can be disabled by Agents. --- Gemfile | 2 +- Gemfile.lock | 12 +- .../_application_controller_generic.coffee | 2 +- .../sidebar_customer.coffee | 14 +- .../app/controllers/ticket_zoom.coffee | 2 +- .../ticket_zoom/sidebar_customer.coffee | 14 +- .../app/controllers/user_profile.coffee | 20 +- .../app/models/_application_model.coffee | 2 +- app/assets/javascripts/app/models/user.coffee | 48 ++ ...r_attributes_by_current_user_permission.rb | 45 +- app/controllers/users_controller.rb | 24 +- app/models/role.rb | 9 +- app/models/user/checks_access.rb | 49 +- script/build/test_slice_tests.sh | 11 +- spec/controllers/users_controller_spec.rb | 676 ++++++++++++++++++ spec/factories/user.rb | 2 +- spec/lib/auth/internal_spec.rb | 16 +- spec/lib/auth_spec.rb | 5 +- spec/models/user_spec.rb | 345 ++++++++- spec/support/avatar_check.rb | 26 + spec/support/controller.rb | 89 +++ spec/support/custom_matchers.rb | 7 + spec/support/user_info.rb | 28 + test/browser/agent_ticket_attachment_test.rb | 6 +- test/browser/chat_test.rb | 12 +- test/browser/switch_to_user_test.rb | 3 +- test/browser/user_access_permissions_test.rb | 376 ++++++++++ test/browser_test_helper.rb | 83 ++- 28 files changed, 1782 insertions(+), 146 deletions(-) create mode 100644 spec/controllers/users_controller_spec.rb create mode 100644 spec/support/avatar_check.rb create mode 100644 spec/support/controller.rb create mode 100644 spec/support/custom_matchers.rb create mode 100644 spec/support/user_info.rb create mode 100644 test/browser/user_access_permissions_test.rb diff --git a/Gemfile b/Gemfile index c980c2377..da0becb45 100644 --- a/Gemfile +++ b/Gemfile @@ -128,7 +128,7 @@ group :development, :test do gem 'simplecov-rcov' # UI tests w/ Selenium - gem 'selenium-webdriver', '2.53.4' + gem 'selenium-webdriver' # livereload on template changes (html, js, css) gem 'guard', require: false diff --git a/Gemfile.lock b/Gemfile.lock index 3f736f9a5..43ae0074c 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -77,7 +77,7 @@ GEM browser (2.5.2) buftok (0.2.0) builder (3.2.3) - childprocess (0.8.0) + childprocess (0.9.0) ffi (~> 1.0, >= 1.0.11) clavius (1.0.3) clearbit (0.2.8) @@ -141,7 +141,7 @@ GEM multipart-post (>= 1.2, < 3) faraday-http-cache (2.0.0) faraday (~> 0.8) - ffi (1.9.18) + ffi (1.9.23) ffi-compiler (0.1.3) ffi (>= 1.0.0) rake @@ -383,10 +383,9 @@ GEM sawyer (0.8.1) addressable (>= 2.3.5, < 2.6) faraday (~> 0.8, < 1.0) - selenium-webdriver (2.53.4) + selenium-webdriver (3.11.0) childprocess (~> 0.5) - rubyzip (~> 1.0) - websocket (~> 1.0) + rubyzip (~> 1.2) shellany (0.0.1) simple_oauth (0.3.1) simplecov (0.15.1) @@ -452,7 +451,6 @@ GEM addressable (>= 2.3.6) crack (>= 0.3.2) hashdiff - websocket (1.2.5) websocket-driver (0.6.5) websocket-extensions (>= 0.1.0) websocket-extensions (0.1.3) @@ -528,7 +526,7 @@ DEPENDENCIES rubocop rubyntlm! sass-rails - selenium-webdriver (= 2.53.4) + selenium-webdriver simplecov simplecov-rcov slack-notifier diff --git a/app/assets/javascripts/app/controllers/_application_controller_generic.coffee b/app/assets/javascripts/app/controllers/_application_controller_generic.coffee index fba868be0..f78f78fd0 100644 --- a/app/assets/javascripts/app/controllers/_application_controller_generic.coffee +++ b/app/assets/javascripts/app/controllers/_application_controller_generic.coffee @@ -1258,7 +1258,7 @@ class App.ObserverActionRow extends App.ObserverController render: (object) => return if _.isEmpty(object) - actions = @actions() + actions = @actions(object) @html App.view('generic/actions')( items: actions type: @type diff --git a/app/assets/javascripts/app/controllers/agent_ticket_create/sidebar_customer.coffee b/app/assets/javascripts/app/controllers/agent_ticket_create/sidebar_customer.coffee index 8de9ea04a..811e0013e 100644 --- a/app/assets/javascripts/app/controllers/agent_ticket_create/sidebar_customer.coffee +++ b/app/assets/javascripts/app/controllers/agent_ticket_create/sidebar_customer.coffee @@ -7,14 +7,18 @@ class SidebarCustomer extends App.Controller badgeCallback: @badgeRender sidebarHead: 'Customer' sidebarCallback: @showCustomer - sidebarActions: [ - { + sidebarActions: [] + } + if App.User.exists(@params.customer_id) + customer = App.User.find(@params.customer_id) + currentUser = App.User.find(App.Session.get('id')) + if customer.isAccessibleBy(currentUser, 'change') + @item.sidebarActions.push { title: 'Edit Customer' name: 'customer-edit' callback: @editCustomer - }, - ] - } + } + @item metaBadge: (user) => counter = '' diff --git a/app/assets/javascripts/app/controllers/ticket_zoom.coffee b/app/assets/javascripts/app/controllers/ticket_zoom.coffee index 9ffcec817..ded662c88 100644 --- a/app/assets/javascripts/app/controllers/ticket_zoom.coffee +++ b/app/assets/javascripts/app/controllers/ticket_zoom.coffee @@ -182,7 +182,7 @@ class App.TicketZoom extends App.Controller @formMeta = data.form_meta # load assets - App.Collection.loadAssets(data.assets) + App.Collection.loadAssets(data.assets, targetModel: 'Ticket') # get data @ticket = App.Ticket.fullLocal(@ticket_id) diff --git a/app/assets/javascripts/app/controllers/ticket_zoom/sidebar_customer.coffee b/app/assets/javascripts/app/controllers/ticket_zoom/sidebar_customer.coffee index e60e80a55..239b36105 100644 --- a/app/assets/javascripts/app/controllers/ticket_zoom/sidebar_customer.coffee +++ b/app/assets/javascripts/app/controllers/ticket_zoom/sidebar_customer.coffee @@ -15,11 +15,15 @@ class SidebarCustomer extends App.Controller ] } return @item if @ticket && @ticket.customer_id == 1 - @item.sidebarActions.push { - title: 'Edit Customer' - name: 'customer-edit' - callback: @editCustomer - } + currentUser = App.User.find(App.Session.get('id')) + if @ticket.customer_id && App.User.exists(@ticket.customer_id) + customer = App.User.find(@ticket.customer_id) + if customer.isAccessibleBy(currentUser, 'change') + @item.sidebarActions.push { + title: 'Edit Customer' + name: 'customer-edit' + callback: @editCustomer + } @item metaBadge: (user) => diff --git a/app/assets/javascripts/app/controllers/user_profile.coffee b/app/assets/javascripts/app/controllers/user_profile.coffee index fa12a6fc2..3a2614c72 100644 --- a/app/assets/javascripts/app/controllers/user_profile.coffee +++ b/app/assets/javascripts/app/controllers/user_profile.coffee @@ -106,13 +106,10 @@ class ActionRow extends App.ObserverActionRow newTicket: (user) => @navigate("ticket/create/customer/#{user.id}") - actions: => - [ - { - name: 'edit' - title: 'Edit' - callback: @editUser - } + actions: (user) => + currentUser = App.User.find(App.Session.get('id')) + + actions = [ { name: 'history' title: 'History' @@ -125,6 +122,15 @@ class ActionRow extends App.ObserverActionRow } ] + if user.isAccessibleBy(currentUser, 'change') + actions.unshift { + name: 'edit' + title: 'Edit' + callback: @editUser + } + + actions + class Object extends App.ObserverController model: 'User' observeNot: diff --git a/app/assets/javascripts/app/models/_application_model.coffee b/app/assets/javascripts/app/models/_application_model.coffee index f115e6077..25e7cb4aa 100644 --- a/app/assets/javascripts/app/models/_application_model.coffee +++ b/app/assets/javascripts/app/models/_application_model.coffee @@ -308,7 +308,7 @@ set new attributes of model (remove already available attributes) # full / load assets if data.assets - App.Collection.loadAssets(data.assets) + App.Collection.loadAssets(data.assets, targetModel: @className) # find / load object else diff --git a/app/assets/javascripts/app/models/user.coffee b/app/assets/javascripts/app/models/user.coffee index eacdeebb3..9ee5ac0b5 100644 --- a/app/assets/javascripts/app/models/user.coffee +++ b/app/assets/javascripts/app/models/user.coffee @@ -285,3 +285,51 @@ class App.User extends App.Model outOfOfficeText: -> return @preferences.out_of_office_text if !_.isEmpty(@preferences.out_of_office_text) App.User.outOfOfficeTextPlaceholder() + + ### + + Checks if requester has given access level on requested. + Possible access levels are: read, update and delete + See backend method User#access? + + requester = App.User.find(1) + requested = App.User.find(3) + result = requested.isAccessibleBy(requester, 'read') + + returns + + true|false + + ### + + isAccessibleBy: (requester, access) -> + return true if requester.permission('admin') + + capitalized = access.charAt(0).toUpperCase() + access.slice(1) + accessMethod = 'is' + capitalized + 'ableBy' + @[accessMethod](requester) + + isReadableBy: (requester) -> + return true if @ownAccount(requester) + return true if requester.permission('admin.*') + return true if requester.permission('ticket.agent') + # check same organization for customers + return false if !requester.permission('ticket.customer') + @sameOrganization?(requester) + + isChangeableBy: (requester) -> + return true if requester.permission('admin.user') + # allow agents to change customers + return false if !requester.permission('ticket.agent') + @permission('ticket.customer') + + isDeleteableBy: (requester) -> + requester.permission('admin.user') + + ownAccount: (requester) -> + @id is requester.id + + sameOrganization: (requester) -> + return false if @organization_id is null + return false if requester.organization_id is null + @organization_id == requester.organization_id diff --git a/app/controllers/concerns/checks_user_attributes_by_current_user_permission.rb b/app/controllers/concerns/checks_user_attributes_by_current_user_permission.rb index 9bd88e0b4..1f01f5ce2 100644 --- a/app/controllers/concerns/checks_user_attributes_by_current_user_permission.rb +++ b/app/controllers/concerns/checks_user_attributes_by_current_user_permission.rb @@ -4,35 +4,32 @@ module ChecksUserAttributesByCurrentUserPermission private def check_attributes_by_current_user_permission(params) + # admins can do whatever they want return true if current_user.permissions?('admin.user') - %i[role_ids roles].each do |key| - next if !params[key] - if current_user.permissions?('ticket.agent') - params.delete(key) - else - logger.info "Role assignment is only allowed by admin! current_user_id: #{current_user.id} assigned to #{params[key].inspect}" - raise Exceptions::NotAuthorized, 'This role assignment is only allowed by admin!' - end - end - if current_user.permissions?('ticket.agent') && !params[:role_ids] && !params[:roles] && params[:id].blank? - params[:role_ids] = Role.signup_role_ids - end + # non-agents (customers) can't set anything + response_access_deny if !current_user.permissions?('ticket.agent') - %i[group_ids groups].each do |key| - next if !params[key] - if current_user.permissions?('ticket.agent') - params.delete(key) - else - logger.info "Group relation assignment is only allowed by admin! current_user_id: #{current_user.id} assigned to #{params[key].inspect}" - raise Exceptions::NotAuthorized, 'Group relation is only allowed by admin!' + # regular agents are not allowed to set Groups and Roles + %w[Role Group].each do |model| + + %w[_ids s].each do |suffix| + attribute = "#{model.downcase}#{suffix}" + values = params[attribute] + + next if values.nil? + + logger.warn "#{model} assignment is only allowed by admin! User with ID #{current_user.id} tried to assign #{values.inspect}" + params.delete(attribute) end end - return true if current_user.permissions?('ticket.agent') - - response_access_deny - false + # check for create requests and set + # signup roles if no other roles are given + return true if params[:id].present? + return true if params[:role_ids] + return true if params[:roles] + params[:role_ids] = Role.signup_role_ids + true end - end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index b94587ce4..fa8f9432e 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -116,7 +116,7 @@ class UsersController < ApplicationController # check if it's first user, the admin user # inital admin account - count = User.all.count() + count = User.all.count admin_account_exists = true if count <= 2 admin_account_exists = false @@ -156,7 +156,7 @@ class UsersController < ApplicationController Role.where(name: %w[Admin Agent]).each do |role| role_ids.push role.id end - Group.all().each do |group| + Group.all.each do |group| group_ids.push group.id end @@ -207,6 +207,7 @@ class UsersController < ApplicationController # send inviteation if needed / only if session exists if params[:invite].present? && current_user + sleep 5 if ENV['REMOTE_URL'].present? token = Token.create(action: 'PasswordReset', user_id: user.id) NotificationFactory::Mailer.notification( template: 'user_invite', @@ -261,8 +262,6 @@ class UsersController < ApplicationController # @response_message 200 [User] Updated User record. # @response_message 401 Invalid session. def update - check_attributes_by_current_user_permission(params) - user = User.find(params[:id]) access!(user, 'change') @@ -273,19 +272,11 @@ class UsersController < ApplicationController clean_params = User.param_cleanup(clean_params, true) user.update!(clean_params) - # only allow Admin's - if current_user.permissions?('admin.user') && (params[:role_ids] || params[:roles]) - user.associations_from_param(role_ids: params[:role_ids], roles: params[:roles]) - end + # presence and permissions were checked via `check_attributes_by_current_user_permission` + privileged_attributes = params.slice(:role_ids, :roles, :group_ids, :groups, :organization_ids, :organizations) - # only allow Admin's - if current_user.permissions?('admin.user') && (params[:group_ids] || params[:groups]) - user.associations_from_param(group_ids: params[:group_ids], groups: params[:groups]) - end - - # only allow Admin's and Agent's - if current_user.permissions?(['admin.user', 'ticket.agent']) && (params[:organization_ids] || params[:organizations]) - user.associations_from_param(organization_ids: params[:organization_ids], organizations: params[:organizations]) + if privileged_attributes.present? + user.associations_from_param(privileged_attributes) end end @@ -1118,5 +1109,4 @@ curl http://localhost/api/v1/users/avatar -v -u #{login}:#{password} -H "Content end true end - end diff --git a/app/models/role.rb b/app/models/role.rb index 49989c510..743a3fc06 100644 --- a/app/models/role.rb +++ b/app/models/role.rb @@ -136,18 +136,13 @@ returns private_class_method def self.permission_ids_by_name(keys) - if keys.class != Array - keys = [keys] - end - permission_ids = [] - keys.each do |key| + Array(keys).each_with_object([]) do |key, result| ::Permission.with_parents(key).each do |local_key| permission = ::Permission.lookup(name: local_key) next if !permission - permission_ids.push permission.id + result.push permission.id end end - permission_ids end private diff --git a/app/models/user/checks_access.rb b/app/models/user/checks_access.rb index 3dfdd48dc..b62f0bb9a 100644 --- a/app/models/user/checks_access.rb +++ b/app/models/user/checks_access.rb @@ -13,19 +13,10 @@ class User # #=> true # # @return [Boolean] - def access?(user, _access) - - # check agent - return true if user.permissions?('admin.user') - return true if user.permissions?('ticket.agent') - - # check customer - if user.permissions?('ticket.customer') - # access ok if its own user - return id == user.id - end - - false + def access?(requester, access) + # full admins can do whatever they want + return true if requester.permissions?('admin') + send("#{access}able_by?".to_sym, requester) end # Checks the given access of a given user for another user and fails with an exception. @@ -42,5 +33,37 @@ class User return if access?(user, access) raise Exceptions::NotAuthorized end + + private + + def readable_by?(requester) + return true if own_account?(requester) + return true if requester.permissions?('admin.*') + return true if requester.permissions?('ticket.agent') + # check same organization for customers + return false if !requester.permissions?('ticket.customer') + same_organization?(requester) + end + + def changeable_by?(requester) + return true if requester.permissions?('admin.user') + # allow agents to change customers + return false if !requester.permissions?('ticket.agent') + permissions?('ticket.customer') + end + + def deleteable_by?(requester) + requester.permissions?('admin.user') + end + + def own_account?(requester) + id == requester.id + end + + def same_organization?(requester) + return false if organization_id.blank? + return false if requester.organization_id.blank? + organization_id == requester.organization_id + end end end diff --git a/script/build/test_slice_tests.sh b/script/build/test_slice_tests.sh index 7fd644c28..af19f1fc3 100755 --- a/script/build/test_slice_tests.sh +++ b/script/build/test_slice_tests.sh @@ -19,6 +19,7 @@ if [ "$LEVEL" == '1' ]; then rm test/browser/admin_overview_test.rb rm test/browser/admin_role_test.rb # test/browser/agent_navigation_and_title_test.rb + # test/browser/agent_organization_profile_test.rb rm test/browser/agent_ticket_attachment_test.rb rm test/browser/agent_ticket_auto_assignment_test.rb rm test/browser/agent_ticket_create_reset_customer_selection_test.rb @@ -63,6 +64,7 @@ if [ "$LEVEL" == '1' ]; then # test/browser/taskbar_session_test.rb # test/browser/taskbar_task_test.rb # test/browser/translation_test.rb + rm test/browser/user_access_permissions_test.rb rm test/browser/user_switch_cache_test.rb elif [ "$LEVEL" == '2' ]; then @@ -124,7 +126,8 @@ elif [ "$LEVEL" == '2' ]; then rm test/browser/taskbar_session_test.rb rm test/browser/taskbar_task_test.rb rm test/browser/translation_test.rb - #rm test/browser/user_switch_cache_test.rb + # test/browser/user_access_permissions_test.rb + # test/browser/user_switch_cache_test.rb elif [ "$LEVEL" == '3' ]; then echo "slicing level 3" @@ -185,6 +188,7 @@ elif [ "$LEVEL" == '3' ]; then rm test/browser/taskbar_session_test.rb rm test/browser/taskbar_task_test.rb rm test/browser/translation_test.rb + rm test/browser/user_access_permissions_test.rb rm test/browser/user_switch_cache_test.rb elif [ "$LEVEL" == '4' ]; then @@ -246,6 +250,7 @@ elif [ "$LEVEL" == '4' ]; then rm test/browser/taskbar_session_test.rb rm test/browser/taskbar_task_test.rb rm test/browser/translation_test.rb + rm test/browser/user_access_permissions_test.rb rm test/browser/user_switch_cache_test.rb elif [ "$LEVEL" == '5' ]; then @@ -261,7 +266,7 @@ elif [ "$LEVEL" == '5' ]; then # test/browser/admin_overview_test.rb rm test/browser/admin_role_test.rb rm test/browser/agent_navigation_and_title_test.rb - # test/browser/agent_organization_profile_test.rb + rm test/browser/agent_organization_profile_test.rb rm test/browser/agent_ticket_attachment_test.rb rm test/browser/agent_ticket_auto_assignment_test.rb rm test/browser/agent_ticket_create_reset_customer_selection_test.rb @@ -306,6 +311,7 @@ elif [ "$LEVEL" == '5' ]; then rm test/browser/taskbar_session_test.rb rm test/browser/taskbar_task_test.rb rm test/browser/translation_test.rb + rm test/browser/user_access_permissions_test.rb rm test/browser/user_switch_cache_test.rb elif [ "$LEVEL" == '6' ]; then @@ -369,6 +375,7 @@ elif [ "$LEVEL" == '6' ]; then rm test/browser/taskbar_session_test.rb rm test/browser/taskbar_task_test.rb rm test/browser/translation_test.rb + rm test/browser/user_access_permissions_test.rb rm test/browser/user_switch_cache_test.rb else diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb new file mode 100644 index 000000000..ca943e478 --- /dev/null +++ b/spec/controllers/users_controller_spec.rb @@ -0,0 +1,676 @@ +require 'rails_helper' + +RSpec.describe UsersController, type: :controller do + + let(:role_with_admin_user_permissions) do + create(:role).tap do |role| + role.permission_grant('admin.user') + end + end + let(:admin_with_admin_user_permissions) { create(:user, roles: [role_with_admin_user_permissions]) } + + let(:role_without_admin_user_permissions) do + create(:role).tap do |role| + role.permission_grant('admin.tag') + end + end + let(:admin_without_admin_user_permissions) { create(:user, roles: [role_without_admin_user_permissions]) } + + describe 'POST #create' do + + let(:attributes) { attributes_params_for(:user) } + + it 'responds unauthorized for customer' do + requester = create(:customer_user) + authenticated_as(requester) + + expect do + post :create, params: attributes + end.to not_change { + User.count + } + + expect(response).to have_http_status(:unauthorized) + end + + context 'privileged attributes' do + + context 'group assignment' do + + # group access assignment is in general only valid for agents + # see HasGroups.groups_access_permission? + let(:agent_attributes) do + attributes.merge( + roles: Role.where(name: 'Agent').map(&:name), + ) + end + + shared_examples 'group assignment' do |map_method_id| + + it 'responds success for admin.user' do + authenticated_as(admin_with_admin_user_permissions) + + expect do + post :create, params: payload + end.to change { + User.count + }.by(1) + + expect(response).to have_http_status(:success) + expect(User.last.send(map_method_id)).to eq(send(map_method_id)) + end + + it 'responds unauthorized for sub admin without admin.user' do + authenticated_as(admin_without_admin_user_permissions) + + expect do + post :create, params: payload + end.to not_change { + User.count + } + + expect(response).to have_http_status(:unauthorized) + end + + it 'responds successful for agent but removes assignment' do + requester = create(:agent_user) + authenticated_as(requester) + + expect do + post :create, params: payload + end.to change { + User.count + }.by(1) + + expect(response).to have_http_status(:success) + expect(User.last.send(map_method_id)).to be_blank + end + end + + context 'parameter groups' do + + let(:group_names_access_map) do + Group.all.map { |g| [g.name, ['full']] }.to_h + end + + let(:payload) do + agent_attributes.merge( + groups: group_names_access_map, + ) + end + + it_behaves_like 'group assignment', :group_names_access_map + end + + context 'parameter group_ids' do + + let(:group_ids_access_map) do + Group.all.map { |g| [g.id, ['full']] }.to_h + end + + let(:payload) do + agent_attributes.merge( + group_ids: group_ids_access_map, + ) + end + + it_behaves_like 'group assignment', :group_ids_access_map + end + end + + context 'role assignment' do + + shared_examples 'role assignment' do + + let(:privileged) { Role.where(name: 'Admin') } + + it 'responds success for admin.user' do + authenticated_as(admin_with_admin_user_permissions) + + expect do + post :create, params: payload + end.to change { + User.count + }.by(1) + + expect(response).to have_http_status(:success) + expect(User.last.roles).to eq(privileged) + end + + it 'responds unauthorized for sub admin without admin.user' do + authenticated_as(admin_without_admin_user_permissions) + + expect do + post :create, params: payload + end.to not_change { + User.count + } + + expect(response).to have_http_status(:unauthorized) + end + + it 'responds successful for agent but removes assignment' do + requester = create(:agent_user) + authenticated_as(requester) + + expect do + post :create, params: payload + end.to change { + User.count + }.by(1) + + expect(response).to have_http_status(:success) + expect(User.last.roles).to eq(Role.signup_roles) + end + end + + context 'parameter roles' do + let(:payload) do + attributes.merge( + roles: privileged.map(&:name), + ) + end + + it_behaves_like 'role assignment' + end + + context 'parameter role_ids' do + let(:payload) do + attributes.merge( + role_ids: privileged.map(&:id), + ) + end + + it_behaves_like 'role assignment' + end + end + end + end + + describe 'PUT #update' do + + def authorized_update_request(requester:, requested:) + authenticated_as(requester) + + expect do + put :update, params: cleaned_params_for(requested).merge(firstname: 'Changed') + end.to change { + requested.reload.firstname + } + + expect(response).to have_http_status(:success) + end + + def unauthorized_update_request(requester:, requested:) + authenticated_as(requester) + + expect do + put :update, params: cleaned_params_for(requested).merge(firstname: 'Changed') + end.to not_change { + requested.reload.attributes + } + + expect(response).to have_http_status(:unauthorized) + end + + context 'request by admin.user' do + + let(:requester) { admin_with_admin_user_permissions } + + it 'is successful for same admin' do + authorized_update_request( + requester: requester, + requested: requester, + ) + end + + it 'is successful for other admin' do + authorized_update_request( + requester: requester, + requested: create(:admin_user), + ) + end + + it 'is successful for agent' do + authorized_update_request( + requester: requester, + requested: create(:agent_user), + ) + end + + it 'is successful for customer' do + authorized_update_request( + requester: requester, + requested: create(:customer_user), + ) + end + end + + context 'request by sub admin without admin.user' do + + let(:requester) { admin_without_admin_user_permissions } + + it 'is unauthorized for same admin' do + unauthorized_update_request( + requester: requester, + requested: requester, + ) + end + + it 'is unauthorized for other admin' do + unauthorized_update_request( + requester: requester, + requested: create(:admin_user), + ) + end + + it 'is unauthorized for agent' do + unauthorized_update_request( + requester: requester, + requested: create(:agent_user), + ) + end + + it 'is unauthorized for customer' do + unauthorized_update_request( + requester: requester, + requested: create(:customer_user), + ) + end + end + + context 'request by agent' do + + let(:requester) { create(:agent_user) } + + it 'is unauthorized for admin' do + unauthorized_update_request( + requester: requester, + requested: create(:admin_user), + ) + end + + it 'is unauthorized same agent' do + unauthorized_update_request( + requester: requester, + requested: requester, + ) + end + + it 'is unauthorized for other agent' do + unauthorized_update_request( + requester: requester, + requested: create(:agent_user), + ) + end + + it 'is successful for customer' do + authorized_update_request( + requester: requester, + requested: create(:customer_user), + ) + end + end + + context 'request by customer' do + + let(:requester) { create(:customer_user) } + + it 'is unauthorized for admin' do + unauthorized_update_request( + requester: requester, + requested: create(:admin_user), + ) + end + + it 'is unauthorized for agent' do + unauthorized_update_request( + requester: requester, + requested: create(:agent_user), + ) + end + + it 'is unauthorized for same customer' do + unauthorized_update_request( + requester: requester, + requested: requester, + ) + end + + it 'is unauthorized for other customer' do + unauthorized_update_request( + requester: requester, + requested: create(:customer_user), + ) + end + + it 'is unauthorized for same organization' do + same_organization = create(:organization) + + requester.update!(organization: same_organization) + + unauthorized_update_request( + requester: requester, + requested: create(:customer_user, organization: same_organization), + ) + end + end + + context 'privileged attributes' do + + let(:requested) { create(:user) } + let(:attribute) { privileged.keys.first } + let(:payload) { cleaned_params_for(requested).merge(privileged) } + + def value_of_attribute + # we need to call .to_a otherwise Rails will load the + # ActiveRecord::Associations::CollectionProxy + # on comparsion which is to late + requested.reload.public_send(attribute).to_a + end + + shared_examples 'admin types requests' do + + it 'responds success for admin.user' do + authenticated_as(admin_with_admin_user_permissions) + + expect do + put :update, params: payload + end.to change { + value_of_attribute + } + expect(response).to have_http_status(:success) + end + + it 'responds unauthorized for sub admin without admin.user' do + authenticated_as(admin_without_admin_user_permissions) + + expect do + put :update, params: payload + end.to not_change { + value_of_attribute + } + + expect(response).to have_http_status(:unauthorized) + end + end + + shared_examples 'permitted agent update' do + + it 'responds successful for agent but removes assignment' do + requester = create(:agent_user) + authenticated_as(requester) + + expect do + put :update, params: payload + end.to change { + value_of_attribute + } + + expect(response).to have_http_status(:success) + end + end + + shared_examples 'forbidden agent update' do + + it 'responds successful for agent but removes assignment' do + requester = create(:agent_user) + authenticated_as(requester) + + expect do + put :update, params: payload + end.to not_change { + value_of_attribute + } + + expect(response).to have_http_status(:success) + end + end + + context 'group assignment' do + + context 'parameter groups' do + + let(:privileged) do + { + groups: Group.all.map { |g| [g.name, ['full']] }.to_h + } + end + + it_behaves_like 'admin types requests' + it_behaves_like 'forbidden agent update' + end + + context 'parameter group_ids' do + + let(:privileged) do + { + group_ids: Group.all.map { |g| [g.id, ['full']] }.to_h + } + end + + it_behaves_like 'admin types requests' + it_behaves_like 'forbidden agent update' + end + end + + context 'role assignment' do + + let(:admin_role) { Role.where(name: 'Admin') } + + context 'parameter roles' do + let(:privileged) do + { + roles: admin_role.map(&:name), + } + end + + it_behaves_like 'admin types requests' + it_behaves_like 'forbidden agent update' + end + + context 'parameter role_ids' do + let(:privileged) do + { + role_ids: admin_role.map(&:id), + } + end + + it_behaves_like 'admin types requests' + it_behaves_like 'forbidden agent update' + end + end + + context 'organization assignment' do + + let(:new_organizations) { create_list(:organization, 2) } + + context 'parameter organizations' do + let(:privileged) do + { + organizations: new_organizations.map(&:name), + } + end + + it_behaves_like 'admin types requests' + it_behaves_like 'permitted agent update' + end + + context 'parameter organization_ids' do + let(:privileged) do + { + organization_ids: new_organizations.map(&:id), + } + end + + it_behaves_like 'admin types requests' + it_behaves_like 'permitted agent update' + end + end + end + end + + describe 'DELETE #destroy' do + + def authorized_destroy_request(requester:, requested:) + authenticated_as(requester) + + delete :destroy, params: { id: requested.id } + + expect(response).to have_http_status(:success) + expect(requested).not_to exist_in_database + end + + def unauthorized_destroy_request(requester:, requested:) + authenticated_as(requester) + + delete :destroy, params: { id: requested.id } + + expect(response).to have_http_status(:unauthorized) + expect(requested).to exist_in_database + end + + context 'request by admin.user' do + + let(:requester) { admin_with_admin_user_permissions } + + it 'is successful for same admin' do + authorized_destroy_request( + requester: requester, + requested: requester, + ) + end + + it 'is successful for other admin' do + authorized_destroy_request( + requester: requester, + requested: create(:admin_user), + ) + end + + it 'is successful for agent' do + authorized_destroy_request( + requester: requester, + requested: create(:agent_user), + ) + end + + it 'is successful for customer' do + authorized_destroy_request( + requester: requester, + requested: create(:customer_user), + ) + end + end + + context 'request by sub admin without admin.user' do + + let(:requester) { admin_without_admin_user_permissions } + + it 'is unauthorized for same admin' do + unauthorized_destroy_request( + requester: requester, + requested: requester, + ) + end + + it 'is unauthorized for other admin' do + unauthorized_destroy_request( + requester: requester, + requested: create(:admin_user), + ) + end + + it 'is unauthorized for agent' do + unauthorized_destroy_request( + requester: requester, + requested: create(:agent_user), + ) + end + + it 'is unauthorized for customer' do + unauthorized_destroy_request( + requester: requester, + requested: create(:customer_user), + ) + end + end + + context 'request by agent' do + + let(:requester) { create(:agent_user) } + + it 'is unauthorized for admin' do + unauthorized_destroy_request( + requester: requester, + requested: create(:admin_user), + ) + end + + it 'is unauthorized same agent' do + unauthorized_destroy_request( + requester: requester, + requested: requester, + ) + end + + it 'is unauthorized for other agent' do + unauthorized_destroy_request( + requester: requester, + requested: create(:agent_user), + ) + end + + it 'is unauthorized for customer' do + unauthorized_destroy_request( + requester: requester, + requested: create(:customer_user), + ) + end + end + + context 'request by customer' do + + let(:requester) { create(:customer_user) } + + it 'is unauthorized for admin' do + unauthorized_destroy_request( + requester: requester, + requested: create(:admin_user), + ) + end + + it 'is unauthorized for agent' do + unauthorized_destroy_request( + requester: requester, + requested: create(:agent_user), + ) + end + + it 'is unauthorized for same customer' do + unauthorized_destroy_request( + requester: requester, + requested: requester, + ) + end + + it 'is unauthorized for other customer' do + unauthorized_destroy_request( + requester: requester, + requested: create(:customer_user), + ) + end + + it 'is unauthorized for same organization' do + same_organization = create(:organization) + + requester.update!(organization: same_organization) + + unauthorized_destroy_request( + requester: requester, + requested: create(:customer_user, organization: same_organization), + ) + end + end + end +end diff --git a/spec/factories/user.rb b/spec/factories/user.rb index 38cfeb0c9..409dc7719 100644 --- a/spec/factories/user.rb +++ b/spec/factories/user.rb @@ -11,7 +11,7 @@ FactoryBot.define do firstname 'Nicole' lastname 'Braun' email { generate(:email) } - password 'zammad' + password nil active true login_failed 0 updated_by_id 1 diff --git a/spec/lib/auth/internal_spec.rb b/spec/lib/auth/internal_spec.rb index cfddfb2e8..12059ec87 100644 --- a/spec/lib/auth/internal_spec.rb +++ b/spec/lib/auth/internal_spec.rb @@ -3,14 +3,15 @@ require 'lib/auth/backend_examples' RSpec.describe Auth::Internal do - let(:user) { create(:user) } + let(:password) { 'zammad' } + let(:user) { create(:user, password: password) } let(:instance) { described_class.new({ adapter: described_class.name }) } context '#valid?' do it_behaves_like 'Auth backend' it 'authenticates via password' do - result = instance.valid?(user, 'zammad') + result = instance.valid?(user, password) expect(result).to be true end @@ -21,17 +22,16 @@ RSpec.describe Auth::Internal do it 'converts legacy sha2 passwords' do - pw_plain = 'zammad' - sha2_pw = PasswordHash.sha2(pw_plain) - user = create(:user, password: sha2_pw) + sha2 = PasswordHash.sha2(password) + user = create(:user, password: sha2) expect(PasswordHash.crypted?(user.password)).to be true - expect(PasswordHash.legacy?(user.password, pw_plain)).to be true + expect(PasswordHash.legacy?(user.password, password)).to be true - result = instance.valid?(user, pw_plain) + result = instance.valid?(user, password) expect(result).to be true - expect(PasswordHash.legacy?(user.password, pw_plain)).to be false + expect(PasswordHash.legacy?(user.password, password)).to be false expect(PasswordHash.crypted?(user.password)).to be true end end diff --git a/spec/lib/auth_spec.rb b/spec/lib/auth_spec.rb index 6bc122b12..edd795cc3 100644 --- a/spec/lib/auth_spec.rb +++ b/spec/lib/auth_spec.rb @@ -40,8 +40,9 @@ RSpec.describe Auth do end it 'authenticates users' do - user = create(:user) - result = described_class.valid?(user, 'zammad') + password = 'zammad' + user = create(:user, password: password) + result = described_class.valid?(user, password) expect(result).to be true end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 79aa14f33..73277580a 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -95,8 +95,9 @@ RSpec.describe User do context '.authenticate' do it 'authenticates by username and password' do - user = create(:user) - result = described_class.authenticate(user.login, 'zammad') + password = 'zammad' + user = create(:user, password: password) + result = described_class.authenticate(user.login, password) expect(result).to be_an(User) end @@ -158,7 +159,7 @@ RSpec.describe User do end end - context '.password_reset_via_token' do + context '#password_reset_via_token' do it 'changes the password of the token user and destroys the token' do token = create(:token_password_reset) @@ -191,4 +192,342 @@ RSpec.describe User do } end end + + context '.access?' do + + let(:role_with_admin_user_permissions) do + create(:role).tap do |role| + role.permission_grant('admin.user') + end + end + let(:admin_with_admin_user_permissions) { create(:user, roles: [role_with_admin_user_permissions]) } + + let(:role_without_admin_user_permissions) do + create(:role).tap do |role| + role.permission_grant('admin.tag') + end + end + let(:admin_without_admin_user_permissions) { create(:user, roles: [role_without_admin_user_permissions]) } + + context 'read' do + + context 'admin' do + + let(:requested) { create(:admin_user) } + + it 'is possible for admin.user' do + requester = admin_with_admin_user_permissions + access = requested.access?(requester, 'read') + expect(access).to be(true) + end + + it 'is possible for sub admin without admin.user' do + requester = admin_without_admin_user_permissions + access = requested.access?(requester, 'read') + expect(access).to be(true) + end + + it 'is possible for agent' do + requester = create(:agent_user) + access = requested.access?(requester, 'read') + expect(access).to be(true) + end + + it 'is not possible for customer' do + requester = create(:customer_user) + access = requested.access?(requester, 'read') + expect(access).to be(false) + end + end + + context 'agent' do + + let(:requested) { create(:agent_user) } + + it 'is possible for admin.user' do + requester = admin_with_admin_user_permissions + access = requested.access?(requester, 'read') + expect(access).to be(true) + end + + it 'is possible for sub admin without admin.user' do + requester = admin_without_admin_user_permissions + access = requested.access?(requester, 'read') + expect(access).to be(true) + end + + it 'is possible for agent' do + requester = create(:agent_user) + access = requested.access?(requester, 'read') + expect(access).to be(true) + end + + it 'is not possible for customer' do + requester = create(:customer_user) + access = requested.access?(requester, 'read') + expect(access).to be(false) + end + end + + context 'customer' do + + let(:requested) { create(:customer_user) } + + it 'is possible for admin.user' do + requester = admin_with_admin_user_permissions + access = requested.access?(requester, 'read') + expect(access).to be(true) + end + + it 'is possible for sub admin without admin.user' do + requester = admin_without_admin_user_permissions + access = requested.access?(requester, 'read') + expect(access).to be(true) + end + + it 'is possible for agent' do + requester = create(:agent_user) + access = requested.access?(requester, 'read') + expect(access).to be(true) + + end + + it 'is possible for same customer' do + access = requested.access?(requested, 'read') + expect(access).to be(true) + end + + it 'is possible for same organization' do + organization = create(:organization) + requester = create(:customer_user, organization: organization) + requested.update!(organization: organization) + access = requested.access?(requester, 'read') + expect(access).to be(true) + end + + it 'is not possible for different customer' do + requester = create(:customer_user) + access = requested.access?(requester, 'read') + expect(access).to be(false) + end + end + end + + context 'change' do + + context 'admin' do + + let(:requested) { create(:admin_user) } + + it 'is possible for admin.user' do + requester = admin_with_admin_user_permissions + access = requested.access?(requester, 'change') + expect(access).to be(true) + end + + it 'is not possible for sub admin without admin.user' do + requester = admin_without_admin_user_permissions + access = requested.access?(requester, 'change') + expect(access).to be(false) + end + + it 'is not possible for same for sub admin without admin.user' do + access = admin_without_admin_user_permissions.access?(admin_without_admin_user_permissions, 'change') + expect(access).to be(false) + end + + it 'is not possible for agent' do + requester = create(:agent_user) + access = requested.access?(requester, 'change') + expect(access).to be(false) + end + + it 'is not possible for customer' do + requester = create(:customer_user) + access = requested.access?(requester, 'change') + expect(access).to be(false) + end + end + + context 'agent' do + + let(:requested) { create(:agent_user) } + + it 'is possible for admin.user' do + requester = admin_with_admin_user_permissions + access = requested.access?(requester, 'change') + expect(access).to be(true) + end + + it 'is not possible for sub admin without admin.user' do + requester = admin_without_admin_user_permissions + access = requested.access?(requester, 'change') + expect(access).to be(false) + end + + it 'is not possible for same agent' do + access = requested.access?(requested, 'change') + expect(access).to be(false) + end + + it 'is not possible for other agent' do + requester = create(:agent_user) + access = requested.access?(requester, 'change') + expect(access).to be(false) + end + + it 'is not possible for customer' do + requester = create(:customer_user) + access = requested.access?(requester, 'change') + expect(access).to be(false) + end + end + + context 'customer' do + + let(:requested) { create(:customer_user) } + + it 'is possible for admin.user' do + requester = admin_with_admin_user_permissions + access = requested.access?(requester, 'change') + expect(access).to be(true) + end + + it 'is not possible for sub admin without admin.user' do + requester = admin_without_admin_user_permissions + access = requested.access?(requester, 'change') + expect(access).to be(false) + end + + it 'is possible for agent' do + requester = create(:agent_user) + access = requested.access?(requester, 'change') + expect(access).to be(true) + + end + + it 'is not possible for same customer' do + access = requested.access?(requested, 'change') + expect(access).to be(false) + end + + it 'is not possible for same organization' do + organization = create(:organization) + requester = create(:customer_user, organization: organization) + requested.update!(organization: organization) + access = requested.access?(requester, 'change') + expect(access).to be(false) + end + + it 'is not possible for different customer' do + requester = create(:customer_user) + access = requested.access?(requester, 'change') + expect(access).to be(false) + end + end + end + + context 'delete' do + + context 'admin' do + + let(:requested) { create(:admin_user) } + + it 'is possible for admin.user' do + requester = admin_with_admin_user_permissions + access = requested.access?(requester, 'delete') + expect(access).to be(true) + end + + it 'is not possible for sub admin without admin.user' do + requester = admin_without_admin_user_permissions + access = requested.access?(requester, 'delete') + expect(access).to be(false) + end + + it 'is not possible for agent' do + requester = create(:agent_user) + access = requested.access?(requester, 'delete') + expect(access).to be(false) + end + + it 'is not possible for customer' do + requester = create(:customer_user) + access = requested.access?(requester, 'delete') + expect(access).to be(false) + end + end + + context 'agent' do + + let(:requested) { create(:agent_user) } + + it 'is possible for admin.user' do + requester = admin_with_admin_user_permissions + access = requested.access?(requester, 'delete') + expect(access).to be(true) + end + + it 'is not possible for sub admin without admin.user' do + requester = admin_without_admin_user_permissions + access = requested.access?(requester, 'delete') + expect(access).to be(false) + end + + it 'is not possible for agent' do + requester = create(:agent_user) + access = requested.access?(requester, 'delete') + expect(access).to be(false) + end + + it 'is not possible for customer' do + requester = create(:customer_user) + access = requested.access?(requester, 'delete') + expect(access).to be(false) + end + end + + context 'customer' do + + let(:requested) { create(:customer_user) } + + it 'is possible for admin.user' do + requester = admin_with_admin_user_permissions + access = requested.access?(requester, 'delete') + expect(access).to be(true) + end + + it 'is not possible for sub admin without admin.user' do + requester = admin_without_admin_user_permissions + access = requested.access?(requester, 'delete') + expect(access).to be(false) + end + + it 'is not possible for agent' do + requester = create(:agent_user) + access = requested.access?(requester, 'delete') + expect(access).to be(false) + end + + it 'is not possible for same customer' do + access = requested.access?(requested, 'delete') + expect(access).to be(false) + end + + it 'is not possible for same organization' do + organization = create(:organization) + requester = create(:customer_user, organization: organization) + requested.update!(organization: organization) + access = requested.access?(requester, 'delete') + expect(access).to be(false) + end + + it 'is not possible for different customer' do + requester = create(:customer_user) + access = requested.access?(requester, 'delete') + expect(access).to be(false) + end + end + end + end end diff --git a/spec/support/avatar_check.rb b/spec/support/avatar_check.rb new file mode 100644 index 000000000..6cafabc93 --- /dev/null +++ b/spec/support/avatar_check.rb @@ -0,0 +1,26 @@ +module ZammadSpecSupportAvatarCheck + + def self.included(base) + + # Execute in RSpec class context + base.class_exec do + + # This method disables the avatar for email check for all examples. + # It's possible to re-enable the check by adding the + # meta tag `perform_avatar_for_email_check` to the needing example: + # + # @example + # it 'does stuff with avatar check', perform_avatar_for_email_check: true do + # + before(:each) do |example| + if !example.metadata[:perform_avatar_for_email_check] + allow(Avatar).to receive(:auto_detection).and_return(false) + end + end + end + end +end + +RSpec.configure do |config| + config.include ZammadSpecSupportAvatarCheck +end diff --git a/spec/support/controller.rb b/spec/support/controller.rb new file mode 100644 index 000000000..28dfebebc --- /dev/null +++ b/spec/support/controller.rb @@ -0,0 +1,89 @@ +module ZammadSpecSupportController + + # Authenticates all requests of the current example as the given user. + # + # @example + # authenticated_as(some_admin_user) + # + # @return nil + def authenticated_as(user) + session[:user_id] = user.id + end + + # Provides a Hash of attributes for the given FactoryBot + # factory parameters which can be used as the params payload. + # Note that the attributes are "cleaned" so no created_by_id etc. + # is present. + # + # @see FactoryBot#attributes_for + # + # @example + # attributes_params_for(:admin_user, email: 'custom@example.com') + # # => {firstname: 'Nicole', email: 'custom@example.com', ...} + # + # @return [Hash{Symbol => }] request cleaned attributes + def attributes_params_for(*args) + filter_unused_params(attributes_for(*args)) + end + + # Provides a Hash of attributes for the given Model instance which can + # be used as the params payload. + # Note that the attributes are "cleaned" so no created_by_id etc. + # is present. + # + # @param [Hash] instance An ActiveRecord instance + # + # @example + # cleaned_params_for(some_admin_user) + # # => {firstname: 'Nicole', email: 'admin@example.com', ...} + # + # @return [Hash{Symbol => }] request cleaned attributes + def cleaned_params_for(instance) + filter_unused_params(instance.attributes) + end + + # This is a self explaining internal method. + # + # @see ApplicationModel#filter_unused_params + def filter_unused_params(unfiltered) + # let's get private + ApplicationModel.send(:filter_unused_params, unfiltered) + end + + def self.included(base) + + # Execute in RSpec class context + base.class_exec do + + # This method disables the CSRF token validation for all controller + # examples. It's possible to re-enable the check by adding the + # meta tag `verify_csrf_token` to the needing example: + # + # @example + # it 'does stuff with verified CSRF token', verify_csrf_token: true do + # + before(:each) do |example| + if !example.metadata[:verify_csrf_token] + allow(controller).to receive(:verify_csrf_token).and_return(true) + end + end + + # This method disables the user device check for all controller + # examples. It's possible to re-enable the check by adding the + # meta tag `perform_user_device_check` to the needing example: + # + # @example + # it 'does stuff with user device check', perform_user_device_check: true do + # + before(:each) do |example| + if !example.metadata[:perform_user_device_check] + session[:user_device_updated_at] = Time.zone.now + end + end + end + end +end + +RSpec.configure do |config| + config.include ZammadSpecSupportController, type: :controller +end diff --git a/spec/support/custom_matchers.rb b/spec/support/custom_matchers.rb new file mode 100644 index 000000000..d497f5008 --- /dev/null +++ b/spec/support/custom_matchers.rb @@ -0,0 +1,7 @@ +# taken from https://makandracards.com/makandra/1080-rspec-matcher-to-check-if-an-activerecord-exists-in-the-database +RSpec::Matchers.define :exist_in_database do + + match do |actual| + actual.class.exists?(actual.id) + end +end diff --git a/spec/support/user_info.rb b/spec/support/user_info.rb new file mode 100644 index 000000000..5f2f0d152 --- /dev/null +++ b/spec/support/user_info.rb @@ -0,0 +1,28 @@ +# This module registers a before and after each hook callback that +# resets the stored current_user_id in the UserInfo which will otherwise +# persists across multiple examples. +# This can lead to issues where actions were performed by a user created +# via a FactoryBot factory which will get removed after the example is +# completed. The UserInfo.current_user_id will persist which leads to e.g. +# DB ForeignKey violation errors. +module ZammadSpecSupportUserInfo + + def self.included(base) + + # Execute in RSpec class context + base.class_exec do + + before(:each) do |_example| + UserInfo.current_user_id = nil + end + + after(:each) do |_example| + UserInfo.current_user_id = nil + end + end + end +end + +RSpec.configure do |config| + config.include ZammadSpecSupportUserInfo +end diff --git a/test/browser/agent_ticket_attachment_test.rb b/test/browser/agent_ticket_attachment_test.rb index e309ca91c..87aec7d59 100644 --- a/test/browser/agent_ticket_attachment_test.rb +++ b/test/browser/agent_ticket_attachment_test.rb @@ -38,6 +38,10 @@ class AgentTicketAttachmentTest < TestCase #alert.accept() #alert = alert.text + # since selenium webdriver with firefox is not able to upload files, skipp here + # https://github.com/w3c/webdriver/issues/1230 + return if browser == 'firefox' + # add attachment, attachment check should quiet file_upload( css: '.content.active .attachmentPlaceholder-inputHolder input', @@ -112,7 +116,7 @@ class AgentTicketAttachmentTest < TestCase ) # check content and edit screen in instance 1 - match( + watch_for( css: '.content.active div.ticket-article', value: 'test 6 - ticket 1-1', ) diff --git a/test/browser/chat_test.rb b/test/browser/chat_test.rb index f7ea8a66e..7238a3271 100644 --- a/test/browser/chat_test.rb +++ b/test/browser/chat_test.rb @@ -138,7 +138,7 @@ class ChatTest < TestCase # init chat click( browser: customer, - css: '.js-chat-open', + css: '.zammad-chat .js-chat-open', ) exists( browser: customer, @@ -156,7 +156,7 @@ class ChatTest < TestCase ) click( browser: customer, - css: '.js-chat-toggle', + css: '.zammad-chat .js-chat-toggle .zammad-chat-header-icon', ) watch_for_disappear( browser: customer, @@ -274,7 +274,7 @@ class ChatTest < TestCase ) click( browser: customer, - css: '.js-chat-toggle', + css: '.js-chat-toggle .zammad-chat-header-icon', ) watch_for( browser: agent, @@ -401,7 +401,7 @@ class ChatTest < TestCase ) click( browser: customer, - css: '.zammad-chat .js-chat-toggle', + css: '.zammad-chat .js-chat-toggle .zammad-chat-header-icon', ) watch_for_disappear( browser: customer, @@ -412,7 +412,7 @@ class ChatTest < TestCase sleep 2 click( browser: customer, - css: '.js-chat-open', + css: '.zammad-chat .js-chat-open', ) exists( browser: customer, @@ -581,7 +581,7 @@ class ChatTest < TestCase ) click( browser: customer, - css: '.js-chat-toggle', + css: '.zammad-chat .js-chat-toggle .zammad-chat-header-icon', ) watch_for( browser: agent, diff --git a/test/browser/switch_to_user_test.rb b/test/browser/switch_to_user_test.rb index 6c953d111..b060c3d4e 100644 --- a/test/browser/switch_to_user_test.rb +++ b/test/browser/switch_to_user_test.rb @@ -20,7 +20,8 @@ class SwitchToUserTest < TestCase ) sleep 3 - @browser.mouse.move_to(@browser.find_elements({ css: '.content.active .table-overview tbody tr:first-child' } )[0]) + @browser.action.move_to(@browser.find_elements({ css: '.content.active .table-overview tbody tr:first-child' } )[0]).release.perform + sleep 0.5 click( css: '.content.active .icon-switchView', diff --git a/test/browser/user_access_permissions_test.rb b/test/browser/user_access_permissions_test.rb new file mode 100644 index 000000000..b1de8db0f --- /dev/null +++ b/test/browser/user_access_permissions_test.rb @@ -0,0 +1,376 @@ +require 'browser_test_helper' + +class AgentProfilePermissionsTest < TestCase + def test_agent_to_edit_customer_profile + @browser = browser_instance + login( + username: 'agent1@example.com', + password: 'test', + url: browser_url, + ) + tasks_close_all() + + # search and open user + user_open_by_search(value: 'Braun') + + verify_task( + data: { + title: 'Nicole Braun', + } + ) + + watch_for( + css: '.content.active .profile-window', + value: 'note', + ) + + watch_for( + css: '.content.active .profile-window', + value: 'email', + ) + + # update note + set( + css: '.content.active [data-name="note"]', + value: 'some note 123', + ) + empty_search() + + # check and change note again in edit screen + click(css: '.content.active .js-action .icon-arrow-down', fast: true) + click(css: '.content.active .js-action [data-type="edit"]') + + watch_for( + css: '.content.active .modal', + value: 'note', + ) + watch_for( + css: '.content.active .modal', + value: 'some note 123', + ) + + set( + css: '.modal [name="lastname"]', + value: 'B2', + ) + set( + css: '.modal [data-name="note"]', + value: 'some note abc', + ) + click(css: '.content.active .modal button.js-submit') + + watch_for( + css: '.content.active .profile-window', + value: 'some note abc', + ) + + verify_task( + data: { + title: 'Nicole B2', + } + ) + + # change lastname back + click(css: '.content.active .js-action .icon-arrow-down', fast: true) + click(css: '.content.active .js-action [data-type="edit"]') + watch_for( + css: '.content.active .modal', + value: 'note', + ) + set( + css: '.modal [name="lastname"]', + value: 'Braun', + ) + click(css: '.content.active .modal button.js-submit') + + verify_task( + data: { + title: 'Nicole Braun', + } + ) + end + + def test_agent_edit_admin_profile + @browser = browser_instance + login( + username: 'agent1@example.com', + password: 'test', + url: browser_url, + ) + tasks_close_all() + + # search and open user + user_open_by_search(value: 'Test Master') + + verify_task( + data: { + title: 'Test Master Agent', + } + ) + + watch_for( + css: '.content.active .profile-window', + value: 'note', + ) + watch_for( + css: '.content.active .profile-window', + value: 'email', + ) + + empty_search() + sleep 2 + + click(css: '.content.active .js-action .icon-arrow-down', fast: true) + + exists_not( + css: '.content.active .js-action [data-type="edit"]' + ) + end + + def test_agent_to_edit_admin_ticket_user_details + @browser = browser_instance + login( + username: 'master@example.com', + password: 'test', + url: browser_url, + ) + tasks_close_all() + + ticket1 = ticket_create( + data: { + customer: 'master', + group: 'Users', + title: 'test_auto_assignment_1 - ticket 1', + body: 'test_auto_assignment_1 - ticket 1 - no auto assignment', + }, + ) + + tasks_close_all() + + logout() + + login( + username: 'agent1@example.com', + password: 'test', + url: browser_url, + ) + tasks_close_all() + + # open ticket#1 + ticket_open_by_search( + number: ticket1[:number], + ) + + watch_for( + css: '.content.active .tabsSidebar-holder', + value: ticket1[:title], + ) + + click(css: '.content.active .tabsSidebar .tabsSidebar-tab[data-tab="customer"]') + click(css: '.content.active .sidebar[data-tab="customer"] .js-actions .dropdown-toggle') + exists_not(css: '.content.active .sidebar[data-tab="customer"] .js-actions [data-type="customer-edit"]') + end + + def test_agent_to_edit_customer_ticket + @browser = browser_instance + + login( + username: 'agent1@example.com', + password: 'test', + url: browser_url, + ) + tasks_close_all() + + ticket1 = ticket_create( + data: { + customer: 'nico', + group: 'Users', + title: 'test_auto_assignment_2 - ticket 2', + body: 'test_auto_assignment_2 - ticket 2 - no auto assignment', + }, + ) + + # open ticket#1 + ticket_open_by_search( + number: ticket1[:number], + ) + + click(css: '.content.active .tabsSidebar .tabsSidebar-tab[data-tab="customer"]') + click(css: '.content.active .sidebar[data-tab="customer"] .js-actions .dropdown-toggle') + click(css: '.content.active .sidebar[data-tab="customer"] .js-actions [data-type="customer-edit"]') + + set( + css: '.modal [name="lastname"]', + value: 'B2', + ) + + set( + css: '.modal [data-name="note"]', + value: 'some note abc', + ) + + click(css: '.content.active .modal button.js-submit') + + watch_for( + css: '.content.active .sidebar[data-tab="customer"] .sidebar-block [data-name="note"]', + value: 'some note abc', + ) + + watch_for( + css: '.content.active .sidebar[data-tab="customer"] .sidebar-block h3[title="Name"]', + value: 'Nicole B2', + ) + + sleep 2 + # change lastname back + click(css: '.content.active .sidebar[data-tab="customer"] .js-actions') + click(css: 'li[data-type="customer-edit"]') + + watch_for( + css: '.content.active .modal', + value: 'note', + ) + + set( + css: '.modal [name="lastname"]', + value: 'Braun', + ) + set( + css: '.modal [data-name="note"]', + value: 'some note abc', + ) + click(css: '.content.active .modal button.js-submit') + + watch_for( + css: '.content.active .sidebar[data-tab="customer"] .sidebar-block [data-name="note"]', + value: 'some note abc', + ) + + watch_for( + css: '.content.active .sidebar[data-tab="customer"] .sidebar-block [title="Name"]', + value: 'Nicole Braun', + ) + end + + def test_agent_to_edit_customer_ticket_details + @browser = browser_instance + + login( + username: 'agent1@example.com', + password: 'test', + url: browser_url, + ) + tasks_close_all() + + ticket1 = ticket_create( + data: { + customer: 'nico', + group: 'Users', + title: 'test_auto_assignment_2 - ticket 2', + body: 'test_auto_assignment_2 - ticket 2 - no auto assignment', + }, + ) + + # open ticket#1 + ticket_open_by_search( + number: ticket1[:number], + ) + + exists(css: '.content.active .tabsSidebar .tabsSidebar-tab[data-tab="customer"]') + exists(css: '.content.active .sidebar[data-tab="customer"] .js-actions .dropdown-toggle') + exists(css: '.content.active .sidebar[data-tab="customer"] .js-actions [data-type="customer-edit"]') + + click(css: '.content.active .tabsSidebar-holder .js-avatar') + + # check and change note again in edit screen + click(css: '.content.active .js-action .dropdown-toggle') + click(css: '.content.active .js-action [data-type="edit"]') + + watch_for( + css: '.content.active .modal', + value: 'note', + ) + + set( + css: '.modal [name="lastname"]', + value: 'B2', + ) + set( + css: '.modal [data-name="note"]', + value: 'some note abc', + ) + click(css: '.content.active .modal button.js-submit') + + watch_for( + css: '.content.active .profile-window', + value: 'some note abc', + ) + + verify_task( + data: { + title: 'Nicole B2', + } + ) + + # change lastname back + click(css: '.content.active .js-action .dropdown-toggle') + click(css: '.content.active .js-action [data-type="edit"]') + + watch_for( + css: '.content.active .modal', + value: 'note', + ) + set( + css: '.modal [name="lastname"]', + value: 'Braun', + ) + set( + css: '.modal [data-name="note"]', + value: 'note', + ) + click(css: '.content.active .modal button.js-submit') + + verify_task( + data: { + title: 'Nicole Braun', + } + ) + end + + def test_agent_to_edit_admin_ticket_details + @browser = browser_instance + + login( + username: 'agent1@example.com', + password: 'test', + url: browser_url, + ) + tasks_close_all() + + ticket1 = ticket_create( + data: { + customer: 'master', + group: 'Users', + title: 'test_auto_assignment_2 - ticket 2', + body: 'test_auto_assignment_2 - ticket 2 - no auto assignment', + }, + ) + + # open ticket#1 + ticket_open_by_search( + number: ticket1[:number], + ) + + exists(css: '.content.active .tabsSidebar .tabsSidebar-tab[data-tab="customer"]') + exists(css: '.content.active .sidebar[data-tab="customer"] .js-actions .dropdown-toggle') + exists_not(css: '.content.active .sidebar[data-tab="customer"] .js-actions [data-type="customer-edit"]') + + click(css: '.content.active .tabsSidebar-holder .js-avatar') + + # check and change note again in edit screen + click(css: '.content.active .js-action .icon-arrow-down', fast: true) + exists_not( + css: '.content.active .js-action [data-type="edit"]' + ) + end +end diff --git a/test/browser_test_helper.rb b/test/browser_test_helper.rb index 962ae30c9..a6c3744eb 100644 --- a/test/browser_test_helper.rb +++ b/test/browser_test_helper.rb @@ -420,9 +420,9 @@ class TestCase < Test::Unit::TestCase begin element = instance.find_elements(css: params[:css])[0] - return if !element && params[:only_if_exists] == true + return if !element && params[:only_if_exists] == true #if element - # instance.mouse.move_to(element) + # instance.action.move_to(element).release.perform #end element.click rescue => e @@ -431,10 +431,11 @@ class TestCase < Test::Unit::TestCase # just try again log('click', { rescure: true }) element = instance.find_elements(css: params[:css])[0] - return if !element && params[:only_if_exists] == true + return if !element && params[:only_if_exists] == true #if element - # instance.mouse.move_to(element) + # instance.action.move_to(element).release.perform #end + raise "No such element '#{params[:css]}'" if !element element.click end @@ -447,7 +448,9 @@ class TestCase < Test::Unit::TestCase # just try again log('click', { rescure: true }) - instance.find_elements(partial_link_text: params[:text])[0].click + element = instance.find_elements(partial_link_text: params[:text])[0] + raise "No such element '#{params[:text]}'" if !element + element.click end end sleep 0.2 if !params[:fast] @@ -632,26 +635,38 @@ class TestCase < Test::Unit::TestCase end element.clear - if !params[:slow] - element.send_keys(params[:value]) - else - element.send_keys('') - keys = params[:value].to_s.split('') - keys.each do |key| - instance.action.send_keys(key).perform + begin + if !params[:slow] + element.send_keys(params[:value]) + else + element.send_keys('') + keys = params[:value].to_s.split('') + keys.each do |key| + instance.action.send_keys(key).perform + end + end + rescue => e + sleep 0.5 + + # just try again + log('set', { rescure: true }) + element = instance.find_elements(css: params[:css])[0] + raise "No such element '#{params[:css]}'" if !element + if !params[:slow] + element.send_keys(params[:value]) + else + element.send_keys('') + keys = params[:value].to_s.split('') + keys.each do |key| + instance.action.send_keys(key).perform + end end end # it's not working stable with ff via selenium, use js if browser =~ /firefox/i && params[:css] =~ /\[data-name=/ - log('set_ff_check', params) - value = instance.find_elements(css: params[:css])[0].text - if value != params[:value] - log('set_ff_check_failed_use_js', params) - value_quoted = quote(params[:value]) - puts "DEBUG $('#{params[:css]}').html('#{value_quoted}').trigger('focusout')" - instance.execute_script("$('#{params[:css]}').html('#{value_quoted}').trigger('focusout')") - end + log('set_ff_trigger_workaround', params) + instance.execute_script("$('#{params[:css]}').trigger('focusout')") end if params[:blur] @@ -1159,12 +1174,15 @@ set type of task (closeTab, closeNextInOverview, stayOnTab) instance = params[:browser] || @browser data = params[:data] - element = instance.find_elements(partial_link_text: data[:title])[0] + element = instance.find_element(css: '#navigation').find_element(partial_link_text: data[:title]) if !element screenshot(browser: instance, comment: 'open_task_failed') raise "no task with title '#{data[:title]}' found" end - element.click + # firefix/marionette issue with Selenium::WebDriver::Error::ElementNotInteractableError: could not be scrolled into view + # use js workaround instead of native click + instance.execute_script("$('#navigation .tasks .task:contains(\"#{data[:title]}\") .nav-tab-name').click()") + #element.click true end @@ -1187,15 +1205,15 @@ set type of task (closeTab, closeNextInOverview, stayOnTab) instance = params[:browser] || @browser data = params[:data] - element = instance.find_elements(partial_link_text: data[:title])[0] + element = instance.find_element(css: '#navigation').find_element(partial_link_text: data[:title]) if !element screenshot(browser: instance, comment: 'close_task_failed') raise "no task with title '#{data[:title]}' found" end - instance.mouse.move_to(element) + instance.action.move_to(element).release.perform sleep 0.1 - instance.execute_script("$('.navigation .tasks .task:contains(\"#{data[:title]}\") .js-close').click()") + instance.execute_script("$('#navigation .tasks .task:contains(\"#{data[:title]}\") .js-close').click()") # accept task close warning if params[:discard_changes] @@ -1405,10 +1423,9 @@ wait untill text in selector disabppears 99.times do #sleep 0.5 begin - if instance.find_elements(css: '.navigation .tasks .task:first-child')[0] - instance.mouse.move_to(instance.find_elements(css: '.navigation .tasks .task:first-child')[0]) - sleep 0.1 - click_element = instance.find_elements(css: '.navigation .tasks .task:first-child .js-close')[0] + if instance.find_elements(css: '#navigation .tasks .task:first-child')[0] + instance.action.move_to(instance.find_elements(css: '#navigation .tasks .task:first-child')[0]).release.perform + click_element = instance.find_elements(css: '#navigation .tasks .task:first-child .js-close')[0] if click_element click_element.click @@ -1453,7 +1470,7 @@ wait untill text in selector disabppears screenshot(browser: instance, comment: 'close_online_notitifcation') raise "no online notification with title '#{data[:title]}' found" end - instance.mouse.move_to(element) + instance.action.move_to(element).release.perform sleep 0.1 instance.execute_script("$('.js-notificationsContainer .js-items .js-item .activity-text:contains(\"#{data[:title]}\") .js-remove').first().click()") @@ -1465,7 +1482,7 @@ wait untill text in selector disabppears raise "no online notification with postion '#{css}' found" end - instance.mouse.move_to(element) + instance.action.move_to(element).release.perform sleep 0.1 instance.find_elements(css: "#{css} .js-remove")[0].click end @@ -1491,7 +1508,7 @@ wait untill text in selector disabppears sleep 0.5 begin if instance.find_elements(css: '.js-notificationsContainer .js-item:first-child')[0] - instance.action.move_to(instance.find_elements(css: '.js-notificationsContainer .js-item:first-child')[0]) + instance.action.move_to(instance.find_elements(css: '.js-notificationsContainer .js-item:first-child')[0]).perform sleep 0.1 click_element = instance.find_elements(css: '.js-notificationsContainer .js-item:first-child .js-remove')[0] click_element&.click @@ -2361,7 +2378,7 @@ wait untill text in selector disabppears ) screenshot(browser: instance, comment: 'ticket_open_by_overview_search') if params[:title] - element = instance.find_elements(partial_link_text: params[:title])[0] + element = instance.find_element(css: '.content.active').find_element(partial_link_text: params[:title]) if !element screenshot(browser: instance, comment: 'ticket_open_by_overview_no_ticket_failed') raise "unable to find ticket #{params[:title]} in overview #{params[:link]}!"