Validate uniqnuess of member role name

Changelog: other

db/post_migrate/20240210104125_ensure_member_roles_names_uniq.rb

+# frozen_string_literal: true
+
+# See https://docs.gitlab.com/ee/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class EnsureMemberRolesNamesUniq < Gitlab::Database::Migration[2.2]
+  milestone '16.10'
+
+  disable_ddl_transaction!
+
+  restrict_gitlab_migration gitlab_schema: :gitlab_main
+
+  def up
+    sql = <<~SQL
+      UPDATE member_roles SET name = CONCAT(name, ' (', id, ')')
+      WHERE id IN (
+        SELECT mr.id FROM member_roles mr
+        WHERE EXISTS (SELECT mr_duplicates.id
+          FROM member_roles mr_duplicates
+          WHERE mr_duplicates.name = mr.name
+          AND (
+            mr_duplicates.namespace_id = mr.namespace_id
+            OR (mr_duplicates.namespace_id IS NULL AND mr.namespace_id IS NULL)
+          )
+          AND mr_duplicates.id < mr.id))
+    SQL
+
+    execute(sql)
+  end
+
+  def down; end
+end

db/post_migrate/20240221134504_add_name_unique_index_to_member_roles.rb

+# frozen_string_literal: true
+
+class AddNameUniqueIndexToMemberRoles < Gitlab::Database::Migration[2.2]
+  disable_ddl_transaction!
+  milestone '16.10'
+
+  INDEX_WITH_NAMESPACE_NAME = 'index_member_roles_on_namespace_id_name_unique'
+  INDEX_NO_NAMESPACE_NAME = 'index_member_roles_on_name_unique'
+
+  def up
+    add_concurrent_index :member_roles, [:namespace_id, :name], name: INDEX_WITH_NAMESPACE_NAME,
+      unique: true, where: 'namespace_id IS NOT NULL'
+    add_concurrent_index :member_roles, [:name], name: INDEX_NO_NAMESPACE_NAME,
+      unique: true, where: 'namespace_id IS NULL'
+  end
+
+  def down
+    remove_concurrent_index_by_name :member_roles, INDEX_WITH_NAMESPACE_NAME
+    remove_concurrent_index_by_name :member_roles, INDEX_NO_NAMESPACE_NAME
+  end
+end

db/schema_migrations/20240210104125

+3eb5dbfdae669848eaf552cd6eb097fb139b41d18b614095ac8de67737b7c796
\ No newline at end of file

db/schema_migrations/20240221134504

+e73649f6285a6ede1741169c62eecb474da9ddaa3c8c03e4f571d4621e3471d2
\ No newline at end of file

db/structure.sql

@@ -25521,8 +25521,12 @@ CREATE INDEX index_member_approval_on_requested_by_id ON member_approvals USING
 CREATE INDEX index_member_approval_on_reviewed_by_id ON member_approvals USING btree (reviewed_by_id);
+CREATE UNIQUE INDEX index_member_roles_on_name_unique ON member_roles USING btree (name) WHERE (namespace_id IS NULL);
+
 CREATE INDEX index_member_roles_on_namespace_id ON member_roles USING btree (namespace_id);
+CREATE UNIQUE INDEX index_member_roles_on_namespace_id_name_unique ON member_roles USING btree (namespace_id, name) WHERE (namespace_id IS NOT NULL);
+
 CREATE INDEX index_member_roles_on_occupies_seat ON member_roles USING btree (occupies_seat);
 CREATE INDEX index_members_on_access_level ON members USING btree (access_level);

ee/app/models/members/member_role.rb

@@ -24,6 +24,7 @@ class MemberRole < ApplicationRecord # rubocop:disable Gitlab/NamespacedClass
 
   validates :namespace, presence: true, if: :gitlab_com_subscription?
   validates :name, presence: true
+  validates :name, uniqueness: { scope: :namespace_id }, on: :create
   validates :base_access_level, presence: true, inclusion: { in: LEVELS }
   validate :belongs_to_top_level_namespace
   validate :max_count_per_group_hierarchy, on: :create

ee/spec/factories/member_roles.rb

@@ -5,6 +5,7 @@
     namespace { association(:group) }
     base_access_level { Gitlab::Access::DEVELOPER }
     read_code { true }
+    name { generate(:title) }
 
     trait(:developer) { base_access_level { Gitlab::Access::DEVELOPER } }
     trait(:maintainer) { base_access_level { Gitlab::Access::MAINTAINER } }

ee/spec/finders/member_roles/roles_finder_spec.rb

@@ -211,7 +211,7 @@
             end
 
             it 'returns both instance member roles and group member roles' do
-              expect(find_member_roles).to eq([member_role_instance, member_role_2, member_role_1])
+              expect(find_member_roles).to match_array([member_role_instance, member_role_2, member_role_1])
             end
           end
         end

ee/spec/models/members/member_role_spec.rb

@@ -19,6 +19,42 @@
     it { is_expected.to validate_presence_of(:base_access_level) }
     it { is_expected.to validate_inclusion_of(:base_access_level).in_array(described_class::LEVELS) }
 
+    describe 'name uniqueness validation' do
+      let_it_be(:group) { create(:group) }
+      let_it_be(:existing_member_role) { create(:member_role, name: 'foo', namespace: group) }
+
+      context 'when creating a new record' do
+        it 'is invalid when name already exists for a namespace' do
+          member_role = build(:member_role, name: 'foo', namespace: group)
+
+          expect(member_role).not_to be_valid
+          expect(member_role.errors[:name]).to include('has already been taken')
+        end
+
+        it 'is valid when name exists for another namespace' do
+          member_role = build(:member_role, name: 'foo', namespace: create(:namespace))
+
+          expect(member_role).to be_valid
+        end
+
+        it 'is invalid creating a duplicate name for instance' do
+          create(:member_role, :instance, name: 'foo')
+          member_role = build(:member_role, :instance, name: 'foo')
+
+          expect(member_role).not_to be_valid
+        end
+      end
+
+      context 'when updating an old record' do
+        it 'is invalid when name already exists for a namespace' do
+          member_role = create(:member_role, name: 'foo 2', namespace: group)
+          member_role.name = 'foo'
+
+          expect(member_role).to be_valid
+        end
+      end
+    end
+
     context 'when running on Gitlab.com' do
       before do
         stub_saas_features(gitlab_com_subscriptions: true)

ee/spec/presenters/member_presenter_spec.rb

@@ -36,7 +36,7 @@
 
       context 'when user has a custom role' do
         it 'returns custom roles' do
-          member_role = create(:member_role, :guest, namespace: root_group)
+          member_role = create(:member_role, :guest, name: 'Custom', namespace: root_group)
           member_root.member_role = member_role
           member_root.access_level = Gitlab::Access::GUEST
 

spec/migrations/20240210104125_ensure_member_roles_names_uniq_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require_migration!
+
+RSpec.describe EnsureMemberRolesNamesUniq, feature_category: :permissions do
+  let(:migration) { described_class.new }
+
+  let(:namespaces) { table(:namespaces) }
+  let(:member_roles) { table(:member_roles) }
+
+  let(:namespace) { namespaces.create!(name: 'foo', path: 'foo') }
+
+  let!(:member_role_1) { member_roles.create!(name: 'foo', namespace_id: namespace.id, base_access_level: 10) }
+  let!(:member_role_2) { member_roles.create!(name: 'other', namespace_id: namespace.id, base_access_level: 10) }
+  let!(:member_role_3) { member_roles.create!(name: 'other', namespace_id: nil, base_access_level: 10) }
+  let!(:member_role_4) { member_roles.create!(name: 'no namespace', namespace_id: nil, base_access_level: 10) }
+  let!(:member_role_1_duplicated) do
+    member_roles.create!(name: 'foo', namespace_id: namespace.id, base_access_level: 10)
+  end
+
+  let!(:member_role_4_duplicated) do
+    member_roles.create!(name: 'no namespace', namespace_id: nil, base_access_level: 10)
+  end
+
+  describe '#up' do
+    it 'updates the duplicated names with higher id', :aggregate_failures do
+      migration.up
+
+      expect(member_role_1.reload.name).to eq('foo')
+      expect(member_role_1_duplicated.reload.name).to eq("foo (#{member_role_1_duplicated.id})")
+      expect(member_role_2.reload.name).to eq('other')
+      expect(member_role_3.reload.name).to eq('other')
+      expect(member_role_4.reload.name).to eq('no namespace')
+      expect(member_role_4_duplicated.reload.name).to eq("no namespace (#{member_role_4_duplicated.id})")
+
+      migration.down
+      migration.up
+
+      expect(member_role_1.reload.name).to eq('foo')
+      expect(member_role_1_duplicated.reload.name).to eq("foo (#{member_role_1_duplicated.id})")
+      expect(member_role_2.reload.name).to eq('other')
+      expect(member_role_3.reload.name).to eq('other')
+      expect(member_role_4.reload.name).to eq('no namespace')
+      expect(member_role_4_duplicated.reload.name).to eq("no namespace (#{member_role_4_duplicated.id})")
+    end
+  end
+end