Fixes #2565 - Visualise locked users in UI and make them unlock-able for admin

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: =>

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

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

app/assets/javascripts/app/views/generic/table_row.jst.eco

@@ -66,6 +66,9 @@
       <% if header.raw: %>
         <%- header.raw %>
       <% else: %>
+        <% if header.prefixIcon: %>
+          <span class="prefix-icon"><%- @Icon(header.prefixIcon) %></span>
+        <% end %>
         <% if header.class || header.data: %>
         <span <% if header.class: %>class="<%= header.class %>"<% end %> <% if header.data: %><% for data_key, data_header of header.data: %>data-<%- data_key %>="<%= data_header %>" <% end %><% end %>>
         <% end %>

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;

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:

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

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

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

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

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(

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

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

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