Fixes #5254 - Prevent an organization from being both primary and secondary.

Co-authored-by: Mantas Masalskis <mm@zammad.com>

app/models/user.rb

@@ -27,7 +27,7 @@ class User < ApplicationModel
   include User::OutOfOffice
   include User::Permissions
 
-  has_and_belongs_to_many :organizations,          after_add: %i[cache_update create_organization_add_history], after_remove: %i[cache_update create_organization_remove_history], class_name: 'Organization'
+  has_and_belongs_to_many :organizations,          after_add: %i[cache_update create_organization_add_history], after_remove: %i[cache_update create_organization_remove_history], before_add: %i[check_organization_uniqueness], class_name: 'Organization'
   has_and_belongs_to_many :overviews,              dependent: :nullify
   has_many                :tokens,                 after_add: :cache_update, after_remove: :cache_update, dependent: :destroy
   has_many                :authorizations,         after_add: :cache_update, after_remove: :cache_update, dependent: :destroy
@@ -46,7 +46,7 @@ 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_password, :ensure_roles, :ensure_organizations, :ensure_organizations_limit
+  before_validation :check_name, :check_email, :check_login, :ensure_password, :ensure_roles, :ensure_organizations, :ensure_different_organizations, :ensure_organizations_limit
   before_validation :check_mail_delivery_failed, on: :update
   before_save       :ensure_notification_preferences, if: :reset_notification_config_before_save
   before_create     :validate_preferences, :domain_based_assignment, :set_locale
@@ -900,6 +900,12 @@ try to find correct name
     errors.add :base, __('Secondary organizations are only allowed when the primary organization is given.')
   end
 
+  def ensure_different_organizations
+    return if organization_ids.exclude?(organization_id)
+
+    errors.add :base, __('Secondary organizations cannot include the primary organization.')
+  end
+
   def ensure_organizations_limit
     return if organization_ids.size <= 250
 
@@ -1137,4 +1143,12 @@ raise 'At least one user need to have admin permissions'
     # Detects if login was set from email and then iterated
     email_was.present? && login&.start_with?(email_was)
   end
+
+  def check_organization_uniqueness(new_organization)
+    return if organization != new_organization && organization_ids.exclude?(new_organization.id)
+
+    errors.add :base, __('Secondary organizations cannot include the primary organization.')
+
+    raise ActiveRecord::RecordInvalid, self
+  end
 end

i18n/zammad.pot

@@ -1387,7 +1387,7 @@ msgid "Agent idle timeout"
 msgstr ""
 
 #: app/models/role.rb:151
-#: app/models/user.rb:749
+#: app/models/user.rb:755
 msgid "Agent limit exceeded, please check your account settings."
 msgstr ""
 
@@ -1975,7 +1975,7 @@ msgid "At least one object must be selected."
 msgstr ""
 
 #: app/models/role.rb:128
-#: app/models/user.rb:728
+#: app/models/user.rb:734
 msgid "At least one user needs to have admin permissions."
 msgstr ""
 
@@ -10039,7 +10039,7 @@ msgstr ""
 msgid "More information can be found here."
 msgstr ""
 
-#: app/models/user.rb:680
+#: app/models/user.rb:686
 msgid "More than 250 secondary organizations are not allowed."
 msgstr ""
 
@@ -13661,6 +13661,10 @@ msgstr ""
 msgid "Secondary organizations are only allowed when the primary organization is given."
 msgstr ""
 
+#: app/models/user.rb:680
+msgid "Secondary organizations cannot include the primary organization."
+msgstr ""
+
 #: app/assets/javascripts/app/controllers/_manage/security.coffee:3
 #: app/assets/javascripts/app/views/agent_ticket_create.jst.eco:34
 #: app/assets/javascripts/app/views/ticket_zoom/article_new.jst.eco:70
@@ -20391,7 +20395,7 @@ msgstr ""
 msgid "is the wrong length (should be 1 character)"
 msgstr ""
 
-#: app/models/user.rb:865
+#: app/models/user.rb:871
 msgid "is too long"
 msgstr ""
 

spec/models/user_spec.rb

@@ -1428,4 +1428,26 @@ RSpec.describe User, type: :model do
       expect(user.assets({}).deep_symbolize_keys.keys).not_to include(:TicketPriority, :Role, :TicketState, :Group)
     end
   end
+
+  describe 'Prevent an organization from being both primary and secondary #5254' do
+    let(:organizations) { create_list(:organization, 3) }
+
+    it 'is not allowed to assign the same organization as primary and secondary' do
+      expect { create(:user, organization_id: organizations.first.id, organization_ids: [organizations.first.id, organizations.second.id]) }.to raise_error(ActiveRecord::RecordInvalid, 'Validation failed: Secondary organizations cannot include the primary organization.')
+    end
+
+    it 'is not allowed to add one of the secondary orgaizations as primary' do
+      user = create(:user, organization: organizations.first, organizations: [organizations.second])
+
+      expect { user.organizations << organizations.first }
+        .to raise_error(ActiveRecord::RecordInvalid, 'Validation failed: Secondary organizations cannot include the primary organization.')
+    end
+
+    it 'allows to move organization from secondary to primary' do
+      user = create(:user, organization: organizations.first, organizations: [organizations.second])
+
+      expect { user.update!(organization: organizations.second, organizations: [organizations.first]) }
+        .not_to raise_error
+    end
+  end
 end

spec/system/apps/mobile_old/tickets/ticket_information_customer_edit_spec.rb

@@ -7,6 +7,7 @@ require 'rails_helper'
 RSpec.describe 'Mobile > Ticket > Information > Customer Edit', app: :mobile, authenticated_as: :authenticate, db_strategy: :reset, type: :system do
   let(:primary_organization)    { create(:organization) }
   let(:secondary_organizations) { create_list(:organization, 4) }
+  let(:third_organization)      { create(:organization) }
   let(:customer)                { create(:customer, organization: primary_organization, organizations: secondary_organizations, address: 'Berlin') }
   let(:group)                   { create(:group) }
   let(:ticket)                  { create(:ticket, customer: customer, group: group) }
@@ -70,7 +71,7 @@ RSpec.describe 'Mobile > Ticket > Information > Customer Edit', app: :mobile, au
       find_input('First name').type('Foo')
       find_input('Last name').type('Bar')
       find_input('Address').type('München')
-      find_autocomplete('Organization').search_for_option(secondary_organizations.first.name)
+      find_autocomplete('Organization').search_for_option(third_organization.name)
 
       # # Despite the name of the action, the following DESELECTS all secondary organizations for the customer.
       # #   This works because all these values are already selected in the field.
@@ -83,7 +84,7 @@ RSpec.describe 'Mobile > Ticket > Information > Customer Edit', app: :mobile, au
 
     expect(find('[role="img"][aria-label="Avatar (Foo Bar)"]')).to have_text('FB')
     expect(find('h2')).to have_text('Foo Bar')
-    expect(find('h3')).to have_text(secondary_organizations.first.name)
+    expect(find('h3')).to have_text(third_organization.name)
     expect(find('section', text: 'Address')).to have_text('München')
     expect(find('section', text: 'Text Attribute')).to have_text('foobar')
 

spec/system/user/history_spec.rb

@@ -24,7 +24,7 @@ RSpec.describe 'User history', authenticated_as: :authenticate, time_zone: 'Euro
       country:                'Germany',
       out_of_office_start_at: current_time,
       last_login:             current_time,
-      organizations:          [organization, org_1, org_2]
+      organizations:          [org_1, org_2]
     )
 
     travel_to Time.zone.parse('2021-04-06 23:30:00')
@@ -34,7 +34,7 @@ RSpec.describe 'User history', authenticated_as: :authenticate, time_zone: 'Euro
       mobile:               '5757473827',
       out_of_office_end_at: current_time,
       last_login:           current_time,
-      organizations:        [organization, org_1]
+      organizations:        [org_1]
     )
 
     travel_back