From a6e387cf9b3c1ce1173c7716d14e60a8248fcce7 Mon Sep 17 00:00:00 2001 From: Mantas Masalskis Date: Fri, 3 Dec 2021 14:09:11 +0100 Subject: [PATCH] Fixes #2429 - Note is not being updated because of the lack of lastname --- app/assets/javascripts/app/models/user.coffee | 6 +- .../application_controller/handles_errors.rb | 4 +- app/controllers/users_controller.rb | 5 +- app/models/user.rb | 75 +++++++++---------- ...3_issue_2429_user_identifier_validation.rb | 31 ++++++++ db/seeds/object_manager_attributes.rb | 30 ++++---- i18n/zammad.pot | 8 +- ...ue_2429_user_identifier_validation_spec.rb | 39 ++++++++++ spec/models/user_spec.rb | 8 ++ spec/requests/user_spec.rb | 4 +- spec/system/manage/users_spec.rb | 46 ++++++++++-- test/unit/user_test.rb | 4 +- 12 files changed, 186 insertions(+), 74 deletions(-) create mode 100644 db/migrate/20211025093743_issue_2429_user_identifier_validation.rb create mode 100644 spec/db/migrate/issue_2429_user_identifier_validation_spec.rb diff --git a/app/assets/javascripts/app/models/user.coffee b/app/assets/javascripts/app/models/user.coffee index 4ef1697c7..e453116b7 100644 --- a/app/assets/javascripts/app/models/user.coffee +++ b/app/assets/javascripts/app/models/user.coffee @@ -6,9 +6,9 @@ class App.User extends App.Model # @hasMany 'roles', 'App.Role' @configure_attributes = [ { name: 'login', display: __('Login'), tag: 'input', type: 'text', limit: 100, null: false, autocapitalize: false, signup: false, quick: false }, - { name: 'firstname', display: __('Firstname'), tag: 'input', type: 'text', limit: 100, null: false, signup: true, info: true, invite_agent: true, invite_customer: true }, - { name: 'lastname', display: __('Lastname'), tag: 'input', type: 'text', limit: 100, null: false, signup: true, info: true, invite_agent: true, invite_customer: true }, - { name: 'email', display: __('Email'), tag: 'input', type: 'email', limit: 100, null: false, signup: true, info: true, invite_agent: true, invite_customer: true }, + { name: 'firstname', display: __('Firstname'), tag: 'input', type: 'text', limit: 100, null: true, signup: true, info: true, invite_agent: true, invite_customer: true }, + { name: 'lastname', display: __('Lastname'), tag: 'input', type: 'text', limit: 100, null: true, signup: true, info: true, invite_agent: true, invite_customer: true }, + { name: 'email', display: __('Email'), tag: 'input', type: 'email', limit: 100, null: true, signup: true, info: true, invite_agent: true, invite_customer: true }, { name: 'organization_id', display: __('Organization'), tag: 'select', multiple: false, nulloption: true, null: true, relation: 'Organization', signup: false, info: true, invite_customer: true }, { name: 'created_by_id', display: __('Created by'), relation: 'User', readonly: 1 }, { name: 'created_at', display: __('Created at'), tag: 'datetime', readonly: 1 }, diff --git a/app/controllers/application_controller/handles_errors.rb b/app/controllers/application_controller/handles_errors.rb index 4c075c0c8..3d8cfc607 100644 --- a/app/controllers/application_controller/handles_errors.rb +++ b/app/controllers/application_controller/handles_errors.rb @@ -84,8 +84,8 @@ module ApplicationController::HandlesErrors error: e.message } - if e.message =~ %r{Validation failed: (.+?)(,|$)}i - data[:error_human] = $1 + if (message = e.try(:record)&.errors&.full_messages&.first) + data[:error_human] = message elsif e.message.match?(%r{(already exists|duplicate key|duplicate entry)}i) data[:error_human] = __('Object already exists!') elsif e.message =~ %r{null value in column "(.+?)" violates not-null constraint}i || e.message =~ %r{Field '(.+?)' doesn't have a default value}i diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 11ae3c9db..4f41e74ea 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -982,9 +982,8 @@ curl http://localhost/api/v1/users/avatar -v -u #{login}:#{password} -H "Content user.source = 'signup' if email_taken_by # show fake OK response to avoid leaking that email is already in use - User.without_callback :validation, :before, :ensure_uniq_email do # skip unique email validation - user.valid? # trigger errors raised in validations - end + user.skip_ensure_uniq_email = true + user.validate! result = User.password_reset_new_token(email_taken_by.email) NotificationFactory::Mailer.notification( diff --git a/app/models/user.rb b/app/models/user.rb index 2605093d6..882590d06 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -44,13 +44,16 @@ class User < ApplicationModel has_many :data_privacy_tasks, as: :deletable belongs_to :organization, inverse_of: :members, optional: true - before_validation :check_name, :check_email, :check_login, :ensure_uniq_email, :ensure_password, :ensure_roles, :ensure_identifier + before_validation :check_name, :check_email, :check_login, :ensure_password, :ensure_roles 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_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 + validate :ensure_identifier, :ensure_email + validate :ensure_uniq_email, unless: :skip_ensure_uniq_email + # workflow checks should run after before_create and before_update callbacks include ChecksCoreWorkflow @@ -859,51 +862,49 @@ try to find correct name preferences.fetch(:locale) { Locale.default } end + attr_accessor :skip_ensure_uniq_email + private def check_name - if firstname.present? - firstname.strip! - end - if lastname.present? - lastname.strip! - end + firstname&.strip! + lastname&.strip! - return true if firstname.present? && lastname.present? + return if firstname.present? && lastname.present? if (firstname.blank? && lastname.present?) || (firstname.present? && lastname.blank?) used_name = firstname.presence || lastname (local_firstname, local_lastname) = User.name_guess(used_name, email) - elsif firstname.blank? && lastname.blank? && email.present? (local_firstname, local_lastname) = User.name_guess('', email) end - self.firstname = local_firstname if local_firstname.present? - self.lastname = local_lastname if local_lastname.present? + check_name_apply(:firstname, local_firstname) + check_name_apply(:lastname, local_lastname) + end - if firstname.present? && firstname.match(%r{^[A-z]+$}) && (firstname.downcase == firstname || firstname.upcase == firstname) - firstname.capitalize! - end - if lastname.present? && lastname.match(%r{^[A-z]+$}) && (lastname.downcase == lastname || lastname.upcase == lastname) - lastname.capitalize! - end - true + def check_name_apply(identifier, input) + self[identifier] = input if input.present? + + self[identifier].capitalize! if self[identifier]&.match? %r{^([[:upper:]]+|[[:lower:]]+)$} end def check_email - return true if Setting.get('import_mode') - return true if email.blank? + return if Setting.get('import_mode') + return if email.blank? self.email = email.downcase.strip - return true if id == 1 + end + + def ensure_email + return if email.blank? + return if id == 1 email_address_validation = EmailAddressValidation.new(email) - if !email_address_validation.valid_format? - raise Exceptions::UnprocessableEntity, "Invalid email '#{email}'" - end - true + return if email_address_validation.valid_format? + + errors.add :base, "Invalid email '#{email}'" end def check_login @@ -914,7 +915,7 @@ try to find correct name end # if email has changed, login is old email, change also login - if changes && changes['email'] && changes['email'][0] == login + if email_changed? && email_was == login self.login = email end @@ -943,27 +944,26 @@ try to find correct name end def ensure_roles - return true if role_ids.present? + return if role_ids.present? self.role_ids = Role.signup_role_ids end def ensure_identifier - return true if email.present? || firstname.present? || lastname.present? || phone.present? - return true if login.present? && !login.start_with?('auto-') + return if login.present? && !login.start_with?('auto-') + return if [email, firstname, lastname, phone].any?(&:present?) - raise Exceptions::UnprocessableEntity, __('Minimum one identifier (login, firstname, lastname, phone or email) for user is required.') + errors.add :base, __('At least one identifier (firstname, lastname, phone or email) for user is required.') end def ensure_uniq_email - return true if Setting.get('user_email_multiple_use') - return true if Setting.get('import_mode') - return true if email.blank? - return true if !changes - return true if !changes['email'] - return true if !User.exists?(email: email.downcase.strip) + return if Setting.get('user_email_multiple_use') + return if Setting.get('import_mode') + return if email.blank? + return if !email_changed? + return if !User.exists?(email: email.downcase.strip) - raise Exceptions::UnprocessableEntity, "Email address '#{email.downcase.strip}' is already used for other user." + errors.add :base, "Email address '#{email.downcase.strip}' is already used for other user." end def validate_roles(role) @@ -1166,7 +1166,6 @@ raise 'Minimum one user need to have admin permissions' def ensure_password self.password = ensured_password - true end def ensured_password diff --git a/db/migrate/20211025093743_issue_2429_user_identifier_validation.rb b/db/migrate/20211025093743_issue_2429_user_identifier_validation.rb new file mode 100644 index 000000000..df22ade1b --- /dev/null +++ b/db/migrate/20211025093743_issue_2429_user_identifier_validation.rb @@ -0,0 +1,31 @@ +# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ + +class Issue2429UserIdentifierValidation < ActiveRecord::Migration[6.0] + def up + return if !Setting.exists?(name: 'system_init_done') + + %i[firstname lastname email phone].each { |elem| update_single(elem) } + end + + def update_single(elem) + attr = ObjectManager::Attribute.for_object(User).find_by(name: elem) + + attr.screens.each do |_, value| + if value.try(:key?, 'null') + value['null'] = true + end + + next if !value.is_a? Hash + + value.each do |_, inner_value| + if inner_value.try(:key?, 'null') + inner_value['null'] = true + end + end + end + + attr.save! + rescue => e + Rails.logger.error e + end +end diff --git a/db/seeds/object_manager_attributes.rb b/db/seeds/object_manager_attributes.rb index ac3657230..dcef2651e 100644 --- a/db/seeds/object_manager_attributes.rb +++ b/db/seeds/object_manager_attributes.rb @@ -567,7 +567,7 @@ ObjectManager::Attribute.add( data_option: { type: 'text', maxlength: 150, - null: false, + null: true, item_class: 'formGroup--halfSize', }, editable: false, @@ -575,27 +575,27 @@ ObjectManager::Attribute.add( screens: { signup: { '-all-' => { - null: false, + null: true, }, }, invite_agent: { '-all-' => { - null: false, + null: true, }, }, invite_customer: { '-all-' => { - null: false, + null: true, }, }, edit: { '-all-' => { - null: false, + null: true, }, }, create: { '-all-' => { - null: false, + null: true, }, }, view: { @@ -619,7 +619,7 @@ ObjectManager::Attribute.add( data_option: { type: 'text', maxlength: 150, - null: false, + null: true, item_class: 'formGroup--halfSize', }, editable: false, @@ -627,27 +627,27 @@ ObjectManager::Attribute.add( screens: { signup: { '-all-' => { - null: false, + null: true, }, }, invite_agent: { '-all-' => { - null: false, + null: true, }, }, invite_customer: { '-all-' => { - null: false, + null: true, }, }, edit: { '-all-' => { - null: false, + null: true, }, }, create: { '-all-' => { - null: false, + null: true, }, }, view: { @@ -679,17 +679,17 @@ ObjectManager::Attribute.add( screens: { signup: { '-all-' => { - null: false, + null: true, }, }, invite_agent: { '-all-' => { - null: false, + null: true, }, }, invite_customer: { '-all-' => { - null: false, + null: true, }, }, edit: { diff --git a/i18n/zammad.pot b/i18n/zammad.pot index 5fdfe48c1..794676d3d 100644 --- a/i18n/zammad.pot +++ b/i18n/zammad.pot @@ -925,6 +925,10 @@ msgstr "" msgid "Assignment timeout in minutes if assigned agent is not working on it. Ticket will be shown as unassigend." msgstr "" +#: app/models/user.rb +msgid "At least one identifier (firstname, lastname, phone or email) for user is required." +msgstr "" + #: app/models/object_manager/attribute.rb msgid "At least one letters is needed" msgstr "" @@ -5784,10 +5788,6 @@ msgstr "" msgid "Minimum of one permission is needed!" msgstr "" -#: app/models/user.rb -msgid "Minimum one identifier (login, firstname, lastname, phone or email) for user is required." -msgstr "" - #: app/models/role.rb #: app/models/user.rb msgid "Minimum one user needs to have admin permissions." diff --git a/spec/db/migrate/issue_2429_user_identifier_validation_spec.rb b/spec/db/migrate/issue_2429_user_identifier_validation_spec.rb new file mode 100644 index 000000000..7fd404617 --- /dev/null +++ b/spec/db/migrate/issue_2429_user_identifier_validation_spec.rb @@ -0,0 +1,39 @@ +# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ + +require 'rails_helper' + +RSpec.describe Issue2429UserIdentifierValidation, type: :db_migration do + let(:elem) { ObjectManager::Attribute.for_object(User).find_by(name: 'firstname') } + + it 'resets value directly in screen' do + elem.screens = { screen1: { asd: true, null: false } } + elem.save! + + migrate + + expect(elem.reload.screens).to eq({ screen1: { asd: true, null: true } }.deep_stringify_keys) + end + + it 'resets value nested in permission' do + elem.screens = { screen1: { all: { asd: true, null: false } } } + elem.save! + + migrate + + expect(elem.reload.screens).to eq({ screen1: { all: { asd: true, null: true } } }.deep_stringify_keys) + end + + it 'does not touch when null not present directly in screen' do + elem.screens = { screen1: { all: { asd: true } } } + elem.save! + + expect { migrate }.not_to change { elem.reload.updated_at } + end + + it 'does not touch when null not present in permission' do + elem.screens = { screen1: { asd: true } } + elem.save! + + expect { migrate }.not_to change { elem.reload.updated_at } + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 0a38e381e..d9309fe69 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -603,6 +603,14 @@ RSpec.describe User, type: :model do expect(new_agent.login.sub!(agent.login, '')).to be_a_uuid end end + + describe '#check_name' do + it 'guesses user first/last name with non-ASCII characters' do + user = create(:user, firstname: 'perkūnas ąžuolas', lastname: '') + + expect(user).to have_attributes(firstname: 'Perkūnas', lastname: 'Ąžuolas') + end + end end describe 'Attributes:' do diff --git a/spec/requests/user_spec.rb b/spec/requests/user_spec.rb index 964d3b4f7..0071d7e09 100644 --- a/spec/requests/user_spec.rb +++ b/spec/requests/user_spec.rb @@ -315,7 +315,7 @@ RSpec.describe 'User', type: :request do post '/api/v1/users', params: params, as: :json expect(response).to have_http_status(:unprocessable_entity) expect(json_response).to be_truthy - expect(json_response['error']).to eq('Minimum one identifier (login, firstname, lastname, phone or email) for user is required.') + expect(json_response['error']).to eq('At least one identifier (firstname, lastname, phone or email) for user is required.') # invalid email params = { firstname: 'newfirstname123', email: 'some_what', note: 'some note' } @@ -1225,7 +1225,7 @@ RSpec.describe 'User', type: :request do it 'requires at least one identifier' do make_request({ web: 'example.com' }) - expect(json_response['error']).to start_with('Minimum one identifier') + expect(json_response['error']).to start_with('At least one identifier') end it 'takes first name as identifier' do diff --git a/spec/system/manage/users_spec.rb b/spec/system/manage/users_spec.rb index aeec624f4..065835590 100644 --- a/spec/system/manage/users_spec.rb +++ b/spec/system/manage/users_spec.rb @@ -112,16 +112,20 @@ RSpec.describe 'Manage > Users', type: :system do end context 'updating a user' do - before do - create(:admin) - end + let(:user) { create(:admin) } + let(:row) { find 'table.user-list tbody tr', text: user.firstname } + + before do + user - it 'handles permission checkboxes correctly' do visit '#manage/users' within(:active_content) do - click 'table.user-list tbody tr:first-child' + row.click end + end + + it 'handles permission checkboxes correctly' do in_modal disappears: false do scroll_into_view 'table.settings-list' within 'table.settings-list tbody tr:first-child' do @@ -136,6 +140,38 @@ RSpec.describe 'Manage > Users', type: :system do end end end + + it 'allows to update a user with no email/first/last/phone if login is present' do + in_modal do + fill_in 'Firstname', with: '' + fill_in 'Lastname', with: '' + fill_in 'Email', with: '' + fill_in 'Phone', with: '' + + click_on 'Submit' + end + + within :active_content do + expect(page).to have_no_text(user.firstname) + end + end + + context 'when user has auto login' do + let(:user) { create(:admin, login: "auto-#{SecureRandom.uuid}") } + + it 'does not allow to update a user with no email/first/last/phone' do + in_modal disappears: false do + fill_in 'Firstname', with: '' + fill_in 'Lastname', with: '' + fill_in 'Email', with: '' + fill_in 'Phone', with: '' + + click_on 'Submit' + + expect(page).to have_text('At least one identifier') + end + end + end end describe 'check user edit permissions', authenticated_as: -> { user } do diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb index c95cb651f..c89c30b77 100644 --- a/test/unit/user_test.rb +++ b/test/unit/user_test.rb @@ -496,7 +496,7 @@ class UserTest < ActiveSupport::TestCase assert(admin1.id) assert_equal(admin1.email, email1) - assert_raises(Exceptions::UnprocessableEntity) do + assert_raises(ActiveRecord::RecordInvalid) do User.create!( login: "#{email1}-1", firstname: 'Role', @@ -522,7 +522,7 @@ class UserTest < ActiveSupport::TestCase created_by_id: 1, ) - assert_raises(Exceptions::UnprocessableEntity) do + assert_raises(ActiveRecord::RecordInvalid) do admin2.email = email1 admin2.save! end