Merge branch '218747-unique-project-auths' into 'master'

Project authorization is unique per user, project

See merge request gitlab-org/gitlab!82460

app/models/project_authorization.rb

@@ -9,7 +9,7 @@ class ProjectAuthorization < ApplicationRecord
 
   validates :project, presence: true
   validates :access_level, inclusion: { in: Gitlab::Access.all_values }, presence: true
-  validates :user, uniqueness: { scope: [:project, :access_level] }, presence: true
+  validates :user, uniqueness: { scope: :project }, presence: true
 
   def self.select_from_union(relations)
     from_union(relations)

db/post_migrate/20210812013042_remove_duplicate_project_authorizations.rb

+# frozen_string_literal: true
+
+class RemoveDuplicateProjectAuthorizations < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::MigrationHelpers
+
+  BATCH_SIZE = 10_000
+  OLD_INDEX_NAME = 'index_project_authorizations_on_project_id_user_id'
+  INDEX_NAME = 'index_unique_project_authorizations_on_project_id_user_id'
+
+  class ProjectAuthorization < ActiveRecord::Base
+    self.table_name = 'project_authorizations'
+  end
+
+  disable_ddl_transaction!
+
+  def up
+    batch do |first_record, last_record|
+      break if first_record.blank?
+
+      # construct a range query where we filter records between the first and last records
+      rows = ActiveRecord::Base.connection.execute <<~SQL
+        SELECT user_id, project_id
+        FROM project_authorizations
+        WHERE
+        #{start_condition(first_record)}
+        #{end_condition(last_record)}
+        GROUP BY user_id, project_id
+        HAVING COUNT(*) > 1
+      SQL
+
+      rows.each do |row|
+        deduplicate_item(row['project_id'], row['user_id'])
+      end
+    end
+
+    add_concurrent_index :project_authorizations, [:project_id, :user_id], unique: true, name: INDEX_NAME
+    remove_concurrent_index_by_name :project_authorizations, OLD_INDEX_NAME
+  end
+
+  def down
+    add_concurrent_index(:project_authorizations, [:project_id, :user_id], name: OLD_INDEX_NAME)
+    remove_concurrent_index_by_name(:project_authorizations, INDEX_NAME)
+  end
+
+  private
+
+  def start_condition(record)
+    "(user_id, project_id) >= (#{Integer(record.user_id)}, #{Integer(record.project_id)})"
+  end
+
+  def end_condition(record)
+    return "" unless record
+
+    "AND (user_id, project_id) <= (#{Integer(record.user_id)}, #{Integer(record.project_id)})"
+  end
+
+  def batch(&block)
+    order = Gitlab::Pagination::Keyset::Order.build([
+        Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
+          attribute_name: 'user_id',
+          order_expression: ProjectAuthorization.arel_table[:user_id].asc,
+          nullable: :not_nullable,
+          distinct: false
+        ),
+        Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
+          attribute_name: 'project_id',
+          order_expression: ProjectAuthorization.arel_table[:project_id].asc,
+          nullable: :not_nullable,
+          distinct: false
+        ),
+        Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
+          attribute_name: 'access_level',
+          order_expression: ProjectAuthorization.arel_table[:access_level].asc,
+          nullable: :not_nullable,
+          distinct: true
+        )
+      ])
+
+    scope = ProjectAuthorization.order(order)
+    cursor = {}
+    loop do
+      current_scope = scope.dup
+
+      relation = order.apply_cursor_conditions(current_scope, cursor)
+      first_record = relation.take
+      last_record = relation.offset(BATCH_SIZE).take
+
+      yield first_record, last_record
+
+      break if last_record.blank?
+
+      cursor = order.cursor_attributes_for_node(last_record)
+    end
+  end
+
+  def deduplicate_item(project_id, user_id)
+    auth_records = ProjectAuthorization.where(project_id: project_id, user_id: user_id).order(access_level: :desc).to_a
+
+    ActiveRecord::Base.transaction do
+      # Keep the highest access level and destroy the rest.
+      auth_records[1..].each do |record|
+        ProjectAuthorization
+        .where(
+          project_id: record.project_id,
+          user_id: record.user_id,
+          access_level: record.access_level
+        ).delete_all
+      end
+    end
+  end
+end

db/schema_migrations/20210812013042

+0af6e6e56967cef9d1160dbfd95456428337843d893307c69505e1a2d3c2074a
\ No newline at end of file

db/structure.sql

@@ -28522,8 +28522,6 @@ CREATE UNIQUE INDEX index_project_aliases_on_name ON project_aliases USING btree
 
 CREATE INDEX index_project_aliases_on_project_id ON project_aliases USING btree (project_id);
 
-CREATE INDEX index_project_authorizations_on_project_id_user_id ON project_authorizations USING btree (project_id, user_id);
-
 CREATE UNIQUE INDEX index_project_auto_devops_on_project_id ON project_auto_devops USING btree (project_id);
 
 CREATE UNIQUE INDEX index_project_ci_cd_settings_on_project_id ON project_ci_cd_settings USING btree (project_id);
@@ -29160,6 +29158,8 @@ CREATE UNIQUE INDEX index_unique_ci_runner_projects_on_runner_id_and_project_id
 
 CREATE UNIQUE INDEX index_unique_issue_metrics_issue_id ON issue_metrics USING btree (issue_id);
 
+CREATE UNIQUE INDEX index_unique_project_authorizations_on_project_id_user_id ON project_authorizations USING btree (project_id, user_id);
+
 CREATE INDEX index_unit_test_failures_failed_at ON ci_unit_test_failures USING btree (failed_at DESC);
 
 CREATE UNIQUE INDEX index_unit_test_failures_unique_columns ON ci_unit_test_failures USING btree (unit_test_id, failed_at DESC, build_id);

spec/migrations/20210812013042_remove_duplicate_project_authorizations_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require_migration!('remove_duplicate_project_authorizations')
+
+RSpec.describe RemoveDuplicateProjectAuthorizations, :migration do
+  let(:users) { table(:users) }
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:project_authorizations) { table(:project_authorizations) }
+
+  let!(:user_1) { users.create! email: 'user1@example.com', projects_limit: 0 }
+  let!(:user_2) { users.create! email: 'user2@example.com', projects_limit: 0 }
+  let!(:namespace_1) { namespaces.create! name: 'namespace 1', path: 'namespace1' }
+  let!(:namespace_2) { namespaces.create! name: 'namespace 2', path: 'namespace2' }
+  let!(:project_1) { projects.create! namespace_id: namespace_1.id }
+  let!(:project_2) { projects.create! namespace_id: namespace_2.id }
+
+  before do
+    stub_const("#{described_class.name}::BATCH_SIZE", 2)
+  end
+
+  describe '#up' do
+    subject { migrate! }
+
+    context 'User with multiple projects' do
+      before do
+        project_authorizations.create! project_id: project_1.id, user_id: user_1.id, access_level: Gitlab::Access::DEVELOPER
+        project_authorizations.create! project_id: project_2.id, user_id: user_1.id, access_level: Gitlab::Access::DEVELOPER
+      end
+
+      it { expect { subject }.not_to change { ProjectAuthorization.count } }
+    end
+
+    context 'Project with multiple users' do
+      before do
+        project_authorizations.create! project_id: project_1.id, user_id: user_1.id, access_level: Gitlab::Access::DEVELOPER
+        project_authorizations.create! project_id: project_1.id, user_id: user_2.id, access_level: Gitlab::Access::DEVELOPER
+      end
+
+      it { expect { subject }.not_to change { ProjectAuthorization.count } }
+    end
+
+    context 'Same project and user but different access level' do
+      before do
+        project_authorizations.create! project_id: project_1.id, user_id: user_1.id, access_level: Gitlab::Access::DEVELOPER
+        project_authorizations.create! project_id: project_1.id, user_id: user_1.id, access_level: Gitlab::Access::MAINTAINER
+        project_authorizations.create! project_id: project_1.id, user_id: user_1.id, access_level: Gitlab::Access::REPORTER
+      end
+
+      it { expect { subject }.to change { ProjectAuthorization.count}.from(3).to(1) }
+
+      it 'retains the highest access level' do
+        subject
+
+        all_records = ProjectAuthorization.all.to_a
+        expect(all_records.count).to eq 1
+        expect(all_records.first.access_level).to eq Gitlab::Access::MAINTAINER
+      end
+    end
+  end
+end

spec/models/project_authorization_spec.rb

@@ -3,6 +3,56 @@
 require 'spec_helper'
 
 RSpec.describe ProjectAuthorization do
+  describe 'unique user, project authorizations' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:project_1) { create(:project) }
+
+    let!(:project_auth) do
+      create(
+        :project_authorization,
+        user: user,
+        project: project_1,
+        access_level: Gitlab::Access::DEVELOPER
+      )
+    end
+
+    context 'with duplicate user and project authorization' do
+      subject { project_auth.dup }
+
+      it { is_expected.to be_invalid }
+
+      context 'after validation' do
+        before do
+          subject.valid?
+        end
+
+        it 'contains duplicate error' do
+          expect(subject.errors[:user]).to include('has already been taken')
+        end
+      end
+    end
+
+    context 'with multiple access levels for the same user and project' do
+      subject do
+        project_auth.dup.tap do |auth|
+          auth.access_level = Gitlab::Access::MAINTAINER
+        end
+      end
+
+      it { is_expected.to be_invalid }
+
+      context 'after validation' do
+        before do
+          subject.valid?
+        end
+
+        it 'contains duplicate error' do
+          expect(subject.errors[:user]).to include('has already been taken')
+        end
+      end
+    end
+  end
+
   describe 'relations' do
     it { is_expected.to belong_to(:user) }
     it { is_expected.to belong_to(:project) }

spec/services/authorized_project_update/find_records_due_for_refresh_service_spec.rb

@@ -66,19 +66,6 @@ RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService do
           expect(service.execute).to eq([to_be_removed, to_be_added])
         end
 
-        it 'finds duplicate entries that has to be removed' do
-          [Gitlab::Access::OWNER, Gitlab::Access::REPORTER].each do |access_level|
-            user.project_authorizations.create!(project: project, access_level: access_level)
-          end
-
-          to_be_removed = [project.id]
-          to_be_added = [
-            { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::OWNER }
-          ]
-
-          expect(service.execute).to eq([to_be_removed, to_be_added])
-        end
-
         it 'finds entries with wrong access levels' do
           user.project_authorizations
             .create!(project: project, access_level: Gitlab::Access::DEVELOPER)

spec/services/users/refresh_authorized_projects_service_spec.rb

@@ -82,31 +82,6 @@ RSpec.describe Users::RefreshAuthorizedProjectsService do
       service.execute_without_lease
     end
 
-    it 'removes duplicate entries' do
-      [Gitlab::Access::OWNER, Gitlab::Access::REPORTER].each do |access_level|
-        user.project_authorizations.create!(project: project, access_level: access_level)
-      end
-
-      to_be_removed = [project.id]
-
-      to_be_added = [
-        { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::OWNER }
-      ]
-      expect(service).to(
-        receive(:update_authorizations)
-          .with(to_be_removed, to_be_added)
-          .and_call_original)
-
-      service.execute_without_lease
-
-      expect(user.project_authorizations.count).to eq(1)
-      project_authorization = ProjectAuthorization.where(
-        project_id: project.id,
-        user_id: user.id,
-        access_level: Gitlab::Access::OWNER)
-      expect(project_authorization).to exist
-    end
-
     it 'sets the access level of a project to the highest available level' do
       user.project_authorizations.delete_all