From 0dc650b5d811f4e3a5af44c6b09522abd360de97 Mon Sep 17 00:00:00 2001 From: Dominik Klein Date: Thu, 5 Aug 2021 10:11:35 +0200 Subject: [PATCH] Fixes #2565 - Visualise locked users in UI and make them unlock-able for admin --- .../_application_controller/table.coffee | 15 +++++-- .../javascripts/app/controllers/user.coffee | 40 ++++++++++++++++++- app/assets/javascripts/app/models/user.coffee | 3 ++ .../app/views/generic/table_row.jst.eco | 3 ++ app/assets/stylesheets/zammad.scss | 4 ++ app/controllers/users_controller.rb | 20 +++++++++- app/models/user.rb | 4 +- .../controllers/users_controller_policy.rb | 2 +- config/routes/user.rb | 1 + ...etting_update_password_max_login_failed.rb | 12 ++++++ db/seeds/settings.rb | 5 ++- ...g_update_password_max_login_failed_spec.rb | 25 ++++++++++++ spec/requests/user_spec.rb | 37 +++++++++++++++++ spec/system/manage/users_spec.rb | 20 ++++++++++ 14 files changed, 181 insertions(+), 10 deletions(-) create mode 100644 db/migrate/20210731132202_setting_update_password_max_login_failed.rb create mode 100644 spec/db/migrate/setting_update_password_max_login_failed_spec.rb diff --git a/app/assets/javascripts/app/controllers/_application_controller/table.coffee b/app/assets/javascripts/app/controllers/_application_controller/table.coffee index 4971a20ba..2b0b43c0d 100644 --- a/app/assets/javascripts/app/controllers/_application_controller/table.coffee +++ b/app/assets/javascripts/app/controllers/_application_controller/table.coffee @@ -502,6 +502,8 @@ class App.ControllerTable extends App.Controller tableBody = [] objectsToShow = @objectsOfPage(@shownPage) for object in objectsToShow + objectActions = [] + if object position++ if @groupBy @@ -509,7 +511,14 @@ class App.ControllerTable extends App.Controller if groupLastName isnt groupByName groupLastName = groupByName tableBody.push @renderTableGroupByRow(object, position, groupByName) - tableBody.push @renderTableRow(object, position) + for action in @actions + # Check if the available key is used, it can be a Boolean or a function which should be called. + if !action.available? || action.available == true + objectActions.push action + else if typeof action.available is 'function' && action.available(object) == true + objectActions.push action + + tableBody.push @renderTableRow(object, position, objectActions) tableBody renderTableGroupByRow: (object, position, groupByName) => @@ -531,7 +540,7 @@ class App.ControllerTable extends App.Controller columnsLength: @columnsLength ) - renderTableRow: (object, position) => + renderTableRow: (object, position, actions) => App.view('generic/table_row')( headers: @headers attributes: @attributesList @@ -541,7 +550,7 @@ class App.ControllerTable extends App.Controller sortable: @dndCallback position: position object: object - actions: @actions + actions: actions ) tableHeadersHasChanged: => diff --git a/app/assets/javascripts/app/controllers/user.coffee b/app/assets/javascripts/app/controllers/user.coffee index 0242b590e..0044f1753 100644 --- a/app/assets/javascripts/app/controllers/user.coffee +++ b/app/assets/javascripts/app/controllers/user.coffee @@ -9,6 +9,7 @@ class User extends App.ControllerSubContent constructor: -> super + @render() show: => @@ -95,6 +96,17 @@ class User extends App.ControllerSubContent container: @el.closest('.content') ) + + callbackLoginAttribute = (value, object, attribute, attributes) -> + attribute.prefixIcon = null + attribute.title = null + + if object.maxLoginFailedReached() + attribute.title = App.i18n.translateContent('The user is locked, because of too many failed login attempts.') + attribute.prefixIcon = 'lock' + + value + users = [] for user_id in user_ids user = App.User.find(user_id) @@ -129,7 +141,7 @@ class User extends App.ControllerSubContent ) 800 ) - } + }, { name: 'delete' display: 'Delete' @@ -138,7 +150,33 @@ class User extends App.ControllerSubContent callback: (id) => @navigate "#system/data_privacy/#{id}" }, + { + name: 'unlock' + display: 'Unlock' + icon: 'lock-open' + class: 'unlock' + available: (user) -> + user.maxLoginFailedReached() + callback: (id) => + @ajax( + id: "user_unlock_#{id}" + type: 'PUT' + url: "#{@apiPath}/users/unlock/#{id}" + success: => + App.User.full(id, + => @notify( + type: 'success' + msg: App.i18n.translateContent('User successfully unlocked!') + + @renderResult(user_ids) + ), + true) + ) + } ] + callbackAttributes: { + login: [ callbackLoginAttribute ] + } bindRow: events: 'click': edit diff --git a/app/assets/javascripts/app/models/user.coffee b/app/assets/javascripts/app/models/user.coffee index 8f1f6aff8..6c1a4beb1 100644 --- a/app/assets/javascripts/app/models/user.coffee +++ b/app/assets/javascripts/app/models/user.coffee @@ -121,6 +121,9 @@ class App.User extends App.Model return true false + maxLoginFailedReached: -> + return @login_failed > (App.Config.get('password_max_login_failed') || 10) + imageUrl: -> return if !@image # set image url diff --git a/app/assets/javascripts/app/views/generic/table_row.jst.eco b/app/assets/javascripts/app/views/generic/table_row.jst.eco index c12bd3802..a5ec74a2d 100644 --- a/app/assets/javascripts/app/views/generic/table_row.jst.eco +++ b/app/assets/javascripts/app/views/generic/table_row.jst.eco @@ -66,6 +66,9 @@ <% if header.raw: %> <%- header.raw %> <% else: %> + <% if header.prefixIcon: %> + <%- @Icon(header.prefixIcon) %> + <% end %> <% if header.class || header.data: %> class="<%= header.class %>"<% end %> <% if header.data: %><% for data_key, data_header of header.data: %>data-<%- data_key %>="<%= data_header %>" <% end %><% end %>> <% end %> diff --git a/app/assets/stylesheets/zammad.scss b/app/assets/stylesheets/zammad.scss index 97b867b27..87ea77794 100644 --- a/app/assets/stylesheets/zammad.scss +++ b/app/assets/stylesheets/zammad.scss @@ -1353,6 +1353,10 @@ td .icon-trash { //fill: hsl(240,1%,77%); } +td .prefix-icon > .icon { + vertical-align: top; +} + .table-checkbox, .table-radio { padding: 0 !important; diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index bfe1e4b7b..ce28c6d91 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -3,7 +3,7 @@ class UsersController < ApplicationController include ChecksUserAttributesByCurrentUserPermission - prepend_before_action -> { authorize! }, only: %i[import_example import_start search history] + prepend_before_action -> { authorize! }, only: %i[import_example import_start search history unlock] prepend_before_action :authentication_check, except: %i[create password_reset_send password_reset_verify image email_verify email_verify_send] prepend_before_action :authentication_check_only, only: %i[create] @@ -338,6 +338,24 @@ class UsersController < ApplicationController render json: user.history_get(true) end + # @path [PUT] /users/unlock/{id} + # + # @summary Unlocks the User record matching the identifier. + # @notes The requester have 'admin.user' permissions to be able to unlock a user. + # + # @parameter id(required) [Integer] The identifier matching the requested User record. + # + # @response_message 200 Unlocked User record. + # @response_message 403 Forbidden / Invalid session. + def unlock + user = User.find(params[:id]) + + user.with_lock do + user.update!(login_failed: 0) + end + render json: { message: 'ok' }, status: :ok + end + =begin Resource: diff --git a/app/models/user.rb b/app/models/user.rb index 753439337..c028e1f6c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -49,7 +49,7 @@ class User < ApplicationModel before_validation :check_name, :check_email, :check_login, :ensure_uniq_email, :ensure_password, :ensure_roles, :ensure_identifier before_validation :check_mail_delivery_failed, on: :update before_create :check_preferences_default, :validate_preferences, :validate_ooo, :domain_based_assignment, :set_locale - before_update :check_preferences_default, :validate_preferences, :validate_ooo, :reset_login_failed, :validate_agent_limit_by_attributes, :last_admin_check_by_attribute + before_update :check_preferences_default, :validate_preferences, :validate_ooo, :reset_login_failed_after_password_change, :validate_agent_limit_by_attributes, :last_admin_check_by_attribute before_destroy :destroy_longer_required_objects, :destroy_move_dependency_ownership after_commit :update_caller_id @@ -1203,7 +1203,7 @@ raise 'Minimum one user need to have admin permissions' end # reset login_failed if password is changed - def reset_login_failed + def reset_login_failed_after_password_change return true if !will_save_change_to_attribute?('password') self.login_failed = 0 diff --git a/app/policies/controllers/users_controller_policy.rb b/app/policies/controllers/users_controller_policy.rb index 72cd3c8e7..0539f27c0 100644 --- a/app/policies/controllers/users_controller_policy.rb +++ b/app/policies/controllers/users_controller_policy.rb @@ -1,6 +1,6 @@ # Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ class Controllers::UsersControllerPolicy < Controllers::ApplicationControllerPolicy - permit! %i[import_example import_start], to: 'admin.user' + permit! %i[import_example import_start unlock], to: 'admin.user' permit! %i[search history create update], to: ['ticket.agent', 'admin.user'] end diff --git a/config/routes/user.rb b/config/routes/user.rb index b1f918102..ea4bd2abb 100644 --- a/config/routes/user.rb +++ b/config/routes/user.rb @@ -29,6 +29,7 @@ Zammad::Application.routes.draw do match api_path + '/users/:id', to: 'users#update', via: :put, as: 'api_v1_update_user' match api_path + '/users/:id', to: 'users#destroy', via: :delete, as: 'api_v1_delete_user' match api_path + '/users/image/:hash', to: 'users#image', via: :get + match api_path + '/users/unlock/:id', to: 'users#unlock', via: :put match api_path + '/users/email_verify', to: 'users#email_verify', via: :post match api_path + '/users/email_verify_send', to: 'users#email_verify_send', via: :post diff --git a/db/migrate/20210731132202_setting_update_password_max_login_failed.rb b/db/migrate/20210731132202_setting_update_password_max_login_failed.rb new file mode 100644 index 000000000..abe509115 --- /dev/null +++ b/db/migrate/20210731132202_setting_update_password_max_login_failed.rb @@ -0,0 +1,12 @@ +# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ + +class SettingUpdatePasswordMaxLoginFailed < ActiveRecord::Migration[6.0] + def change + return if !Setting.exists?(name: 'system_init_done') + + setting = Setting.find_by(name: 'password_max_login_failed') + setting.preferences[:authentication] = true + setting.frontend = true + setting.save! + end +end diff --git a/db/seeds/settings.rb b/db/seeds/settings.rb index 4ce862d11..6f3b761e4 100644 --- a/db/seeds/settings.rb +++ b/db/seeds/settings.rb @@ -1869,9 +1869,10 @@ Setting.create_if_not_exists( }, state: 5, preferences: { - permission: ['admin.security'], + authentication: true, + permission: ['admin.security'], }, - frontend: false + frontend: true ) Setting.create_if_not_exists( diff --git a/spec/db/migrate/setting_update_password_max_login_failed_spec.rb b/spec/db/migrate/setting_update_password_max_login_failed_spec.rb new file mode 100644 index 000000000..cc752e1a5 --- /dev/null +++ b/spec/db/migrate/setting_update_password_max_login_failed_spec.rb @@ -0,0 +1,25 @@ +# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ + +require 'rails_helper' + +RSpec.describe SettingUpdatePasswordMaxLoginFailed, type: :db_migration do + context 'when having old password max login failed setting values' do + before do + setting.preferences = { + permission: ['admin.security'], + } + setting.frontend = false + setting.save! + end + + let(:setting) { Setting.find_by(name: 'password_max_login_failed') } + + it 'add authentication to preferences' do + expect { migrate }.to change { setting.reload.preferences[:authentication] }.to(true) + end + + it 'change frontend flag to true' do + expect { migrate }.to change { setting.reload.frontend }.to(true) + end + end +end diff --git a/spec/requests/user_spec.rb b/spec/requests/user_spec.rb index a402e6fdb..1bc0147da 100644 --- a/spec/requests/user_spec.rb +++ b/spec/requests/user_spec.rb @@ -1506,4 +1506,41 @@ RSpec.describe 'User', type: :request do .to change { Avatar.list('User', user.id) } end end + + describe 'PUT /api/v1/users/unlock/{id}' do + let(:admin) { create(:admin) } + let(:agent) { create(:agent) } + let(:customer) { create(:customer, login_failed: 2) } + + def make_request(id) + put "/api/v1/users/unlock/#{id}", params: {}, as: :json + end + + context 'with authenticated admin user', authenticated_as: :admin do + it 'returns success' do + make_request(customer.id) + expect(response).to have_http_status(:ok) + end + + it 'check that login failed was reseted' do + expect { make_request(customer.id) }.to change { customer.reload.login_failed }.from(2).to(0) + end + + it 'fail with not existing user id' do + make_request(99_999) + expect(response).to have_http_status(:not_found) + end + end + + context 'with authenticated agent user', authenticated_as: :agent do + it 'fail without admin permission' do + make_request(customer.id) + expect(response).to have_http_status(:forbidden) + end + + it 'check that login failed was not changed' do + expect { make_request(customer.id) }.not_to change { customer.reload.login_failed } + end + end + end end diff --git a/spec/system/manage/users_spec.rb b/spec/system/manage/users_spec.rb index 807265796..8072ba604 100644 --- a/spec/system/manage/users_spec.rb +++ b/spec/system/manage/users_spec.rb @@ -90,4 +90,24 @@ RSpec.describe 'Manage > Users', type: :system do end end end + + describe 'show/unlock a user', authenticated_as: -> { user } do + let(:user) { create(:admin) } + let!(:locked_user) { create(:user, login_failed: 6) } + + it 'check marked locked user and execute unlock action' do + visit '#manage/users' + + within(:active_content) do + row = find("tr[data-id=\"#{locked_user.id}\"]") + + expect(row).to have_css('.icon-lock') + + row.find('.js-action').click + row.find('li.unlock').click + + expect(row).to have_no_css('.icon-lock') + end + end + end end