Merge branch '292443-find-records-for-auth-refresh-via-new-service' into 'master'

Find project_authorizations records due for refresh via a service class [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!56624

app/services/authorized_project_update/find_records_due_for_refresh_service.rb

+# frozen_string_literal: true
+
+module AuthorizedProjectUpdate
+  # Service for finding the authorized_projects records of a user that needs addition or removal.
+  #
+  # Usage:
+  #
+  #     user = User.find_by(username: 'alice')
+  #     service = AuthorizedProjectUpdate::FindRecordsDueForRefreshService.new(some_user)
+  #     service.execute
+  class FindRecordsDueForRefreshService
+    def initialize(user, source: nil, incorrect_auth_found_callback: nil, missing_auth_found_callback: nil)
+      @user = user
+      @source = source
+      @incorrect_auth_found_callback = incorrect_auth_found_callback
+      @missing_auth_found_callback = missing_auth_found_callback
+    end
+
+    def execute
+      current = current_authorizations_per_project
+      fresh = fresh_access_levels_per_project
+
+      # Projects that have more than one authorizations associated with
+      # the user needs to be deleted.
+      # The correct authorization is added to the ``add`` array in the
+      # next stage.
+      remove = projects_with_duplicates
+      current.except!(*projects_with_duplicates)
+
+      remove |= current.each_with_object([]) do |(project_id, row), array|
+        # rows not in the new list or with a different access level should be
+        # removed.
+        if !fresh[project_id] || fresh[project_id] != row.access_level
+          if incorrect_auth_found_callback
+            incorrect_auth_found_callback.call(project_id, row.access_level)
+          end
+
+          array << row.project_id
+        end
+      end
+
+      add = fresh.each_with_object([]) do |(project_id, level), array|
+        # rows not in the old list or with a different access level should be
+        # added.
+        if !current[project_id] || current[project_id].access_level != level
+          if missing_auth_found_callback
+            missing_auth_found_callback.call(project_id, level)
+          end
+
+          array << [user.id, project_id, level]
+        end
+      end
+
+      [remove, add]
+    end
+
+    def fresh_access_levels_per_project
+      fresh_authorizations.each_with_object({}) do |row, hash|
+        hash[row.project_id] = row.access_level
+      end
+    end
+
+    def current_authorizations_per_project
+      current_authorizations.index_by(&:project_id)
+    end
+
+    def current_authorizations
+      @current_authorizations ||= user.project_authorizations.select(:project_id, :access_level)
+    end
+
+    def fresh_authorizations
+      Gitlab::ProjectAuthorizations.new(user).calculate
+    end
+
+    private
+
+    attr_reader :user, :source, :incorrect_auth_found_callback, :missing_auth_found_callback
+
+    def projects_with_duplicates
+      @projects_with_duplicates ||= current_authorizations
+                                      .group_by(&:project_id)
+                                      .select { |project_id, authorizations| authorizations.count > 1 }
+                                      .keys
+    end
+  end
+end

app/services/users/refresh_authorized_projects_service.rb

@@ -51,38 +51,12 @@ module Users
 
     # This method returns the updated User object.
     def execute_without_lease
-      current = current_authorizations_per_project
-      fresh = fresh_access_levels_per_project
-
-      # Delete projects that have more than one authorizations associated with
-      # the user. The correct authorization is added to the ``add`` array in the
-      # next stage.
-      remove = projects_with_duplicates
-      current.except!(*projects_with_duplicates)
-
-      remove |= current.each_with_object([]) do |(project_id, row), array|
-        # rows not in the new list or with a different access level should be
-        # removed.
-        if !fresh[project_id] || fresh[project_id] != row.access_level
-          if incorrect_auth_found_callback
-            incorrect_auth_found_callback.call(project_id, row.access_level)
-          end
-
-          array << row.project_id
-        end
-      end
-
-      add = fresh.each_with_object([]) do |(project_id, level), array|
-        # rows not in the old list or with a different access level should be
-        # added.
-        if !current[project_id] || current[project_id].access_level != level
-          if missing_auth_found_callback
-            missing_auth_found_callback.call(project_id, level)
-          end
-
-          array << [user.id, project_id, level]
-        end
-      end
+      remove, add = AuthorizedProjectUpdate::FindRecordsDueForRefreshService.new(
+        user,
+        source: source,
+        incorrect_auth_found_callback: incorrect_auth_found_callback,
+        missing_auth_found_callback: missing_auth_found_callback
+      ).execute
 
       update_authorizations(remove, add)
     end
@@ -104,6 +78,10 @@ module Users
       user.reset
     end
 
+    private
+
+    attr_reader :incorrect_auth_found_callback, :missing_auth_found_callback
+
     def log_refresh_details(remove, add)
       Gitlab::AppJsonLogger.info(event: 'authorized_projects_refresh',
                                  user_id: user.id,
@@ -115,34 +93,5 @@ module Users
                                  'authorized_projects_refresh.rows_deleted_slice': remove.first(5),
                                  'authorized_projects_refresh.rows_added_slice': add.first(5))
     end
-
-    def fresh_access_levels_per_project
-      fresh_authorizations.each_with_object({}) do |row, hash|
-        hash[row.project_id] = row.access_level
-      end
-    end
-
-    def current_authorizations_per_project
-      current_authorizations.index_by(&:project_id)
-    end
-
-    def current_authorizations
-      @current_authorizations ||= user.project_authorizations.select(:project_id, :access_level)
-    end
-
-    def fresh_authorizations
-      Gitlab::ProjectAuthorizations.new(user).calculate
-    end
-
-    private
-
-    attr_reader :incorrect_auth_found_callback, :missing_auth_found_callback
-
-    def projects_with_duplicates
-      @projects_with_duplicates ||= current_authorizations
-                                      .group_by(&:project_id)
-                                      .select { |project_id, authorizations| authorizations.count > 1 }
-                                      .keys
-    end
   end
 end

spec/services/authorized_project_update/find_records_due_for_refresh_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService do
+  # We're using let! here so that any expectations for the service class are not
+  # triggered twice.
+  let!(:project) { create(:project) }
+
+  let(:user) { project.namespace.owner }
+  let(:service) { described_class.new(user) }
+
+  describe '#execute' do
+    context 'callbacks' do
+      let(:callback) { double('callback') }
+
+      context 'incorrect_auth_found_callback callback' do
+        let(:user) { create(:user) }
+        let(:service) do
+          described_class.new(user,
+                              incorrect_auth_found_callback: callback)
+        end
+
+        it 'is called' do
+          access_level = Gitlab::Access::DEVELOPER
+          create(:project_authorization, user: user, project: project, access_level: access_level)
+
+          expect(callback).to receive(:call).with(project.id, access_level).once
+
+          service.execute
+        end
+      end
+
+      context 'missing_auth_found_callback callback' do
+        let(:service) do
+          described_class.new(user,
+                              missing_auth_found_callback: callback)
+        end
+
+        it 'is called' do
+          ProjectAuthorization.delete_all
+
+          expect(callback).to receive(:call).with(project.id, Gitlab::Access::MAINTAINER).once
+
+          service.execute
+        end
+      end
+    end
+
+    context 'finding project authorizations due for refresh' do
+      context 'when there are changes to be made' do
+        before do
+          user.project_authorizations.delete_all
+        end
+
+        it 'finds projects authorizations that needs to be refreshed' do
+          project2 = create(:project)
+          user.project_authorizations
+            .create!(project: project2, access_level: Gitlab::Access::MAINTAINER)
+
+          to_be_removed = [project2.id]
+          to_be_added = [[user.id, project.id, Gitlab::Access::MAINTAINER]]
+
+          expect(service.execute).to eq([to_be_removed, to_be_added])
+        end
+
+        it 'finds duplicate entries that has to be removed' do
+          [Gitlab::Access::MAINTAINER, 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, project.id, Gitlab::Access::MAINTAINER]]
+
+          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)
+
+          to_be_removed = [project.id]
+          to_be_added = [[user.id, project.id, Gitlab::Access::MAINTAINER]]
+
+          expect(service.execute).to eq([to_be_removed, to_be_added])
+        end
+      end
+
+      context 'when there are no changes to be made' do
+        it 'returns empty arrays' do
+          expect(service.execute).to eq([[], []])
+        end
+      end
+    end
+  end
+
+  describe '#fresh_access_levels_per_project' do
+    let(:hash) { service.fresh_access_levels_per_project }
+
+    it 'returns a Hash' do
+      expect(hash).to be_an_instance_of(Hash)
+    end
+
+    it 'sets the keys to the project IDs' do
+      expect(hash.keys).to eq([project.id])
+    end
+
+    it 'sets the values to the access levels' do
+      expect(hash.values).to eq([Gitlab::Access::MAINTAINER])
+    end
+
+    context 'personal projects' do
+      it 'includes the project with the right access level' do
+        expect(hash[project.id]).to eq(Gitlab::Access::MAINTAINER)
+      end
+    end
+
+    context 'projects the user is a member of' do
+      let!(:other_project) { create(:project) }
+
+      before do
+        other_project.team.add_reporter(user)
+      end
+
+      it 'includes the project with the right access level' do
+        expect(hash[other_project.id]).to eq(Gitlab::Access::REPORTER)
+      end
+    end
+
+    context 'projects of groups the user is a member of' do
+      let(:group) { create(:group) }
+      let!(:other_project) { create(:project, group: group) }
+
+      before do
+        group.add_owner(user)
+      end
+
+      it 'includes the project with the right access level' do
+        expect(hash[other_project.id]).to eq(Gitlab::Access::OWNER)
+      end
+    end
+
+    context 'projects of subgroups of groups the user is a member of' do
+      let(:group) { create(:group) }
+      let(:nested_group) { create(:group, parent: group) }
+      let!(:other_project) { create(:project, group: nested_group) }
+
+      before do
+        group.add_maintainer(user)
+      end
+
+      it 'includes the project with the right access level' do
+        expect(hash[other_project.id]).to eq(Gitlab::Access::MAINTAINER)
+      end
+    end
+
+    context 'projects shared with groups the user is a member of' do
+      let(:group) { create(:group) }
+      let(:other_project) { create(:project) }
+      let!(:project_group_link) { create(:project_group_link, project: other_project, group: group, group_access: Gitlab::Access::GUEST) }
+
+      before do
+        group.add_maintainer(user)
+      end
+
+      it 'includes the project with the right access level' do
+        expect(hash[other_project.id]).to eq(Gitlab::Access::GUEST)
+      end
+    end
+
+    context 'projects shared with subgroups of groups the user is a member of' do
+      let(:group) { create(:group) }
+      let(:nested_group) { create(:group, parent: group) }
+      let(:other_project) { create(:project) }
+      let!(:project_group_link) { create(:project_group_link, project: other_project, group: nested_group, group_access: Gitlab::Access::DEVELOPER) }
+
+      before do
+        group.add_maintainer(user)
+      end
+
+      it 'includes the project with the right access level' do
+        expect(hash[other_project.id]).to eq(Gitlab::Access::DEVELOPER)
+      end
+    end
+  end
+
+  describe '#current_authorizations_per_project' do
+    let(:hash) { service.current_authorizations_per_project }
+
+    it 'returns a Hash' do
+      expect(hash).to be_an_instance_of(Hash)
+    end
+
+    it 'sets the keys to the project IDs' do
+      expect(hash.keys).to eq([project.id])
+    end
+
+    it 'sets the values to the project authorization rows' do
+      expect(hash.values.length).to eq(1)
+
+      value = hash.values[0]
+
+      expect(value.project_id).to eq(project.id)
+      expect(value.access_level).to eq(Gitlab::Access::MAINTAINER)
+    end
+  end
+
+  describe '#current_authorizations' do
+    context 'without authorizations' do
+      it 'returns an empty list' do
+        user.project_authorizations.delete_all
+
+        expect(service.current_authorizations.empty?).to eq(true)
+      end
+    end
+
+    context 'with an authorization' do
+      let(:row) { service.current_authorizations.take }
+
+      it 'returns the currently authorized projects' do
+        expect(service.current_authorizations.length).to eq(1)
+      end
+
+      it 'includes the project ID for every row' do
+        expect(row.project_id).to eq(project.id)
+      end
+
+      it 'includes the access level for every row' do
+        expect(row.access_level).to eq(Gitlab::Access::MAINTAINER)
+      end
+    end
+  end
+
+  describe '#fresh_authorizations' do
+    it 'returns the new authorized projects' do
+      expect(service.fresh_authorizations.length).to eq(1)
+    end
+
+    it 'returns the highest access level' do
+      project.team.add_guest(user)
+
+      rows = service.fresh_authorizations.to_a
+
+      expect(rows.length).to eq(1)
+      expect(rows.first.access_level).to eq(Gitlab::Access::MAINTAINER)
+    end
+
+    context 'every returned row' do
+      let(:row) { service.fresh_authorizations.take }
+
+      it 'includes the project ID' do
+        expect(row.project_id).to eq(project.id)
+      end
+
+      it 'includes the access level' do
+        expect(row.access_level).to eq(Gitlab::Access::MAINTAINER)
+      end
+    end
+  end
+end

spec/services/authorized_project_update/recalculate_for_user_range_service_spec.rb

@@ -7,12 +7,14 @@ RSpec.describe AuthorizedProjectUpdate::RecalculateForUserRangeService do
     let_it_be(:users) { create_list(:user, 2) }
 
     it 'calls Users::RefreshAuthorizedProjectsService' do
-      users.each do |user|
+      user_ids = users.map(&:id)
+
+      User.where(id: user_ids).select(:id).each do |user|
         expect(Users::RefreshAuthorizedProjectsService).to(
           receive(:new).with(user, source: described_class.name).and_call_original)
       end
 
-      range = users.map(&:id).minmax
+      range = user_ids.minmax
       described_class.new(*range).execute
     end
   end

spec/services/users/refresh_authorized_projects_service_spec.rb

@@ -163,168 +163,4 @@ RSpec.describe Users::RefreshAuthorizedProjectsService do
       service.update_authorizations([], [[user.id, project.id, Gitlab::Access::MAINTAINER]])
     end
   end
-
-  describe '#fresh_access_levels_per_project' do
-    let(:hash) { service.fresh_access_levels_per_project }
-
-    it 'returns a Hash' do
-      expect(hash).to be_an_instance_of(Hash)
-    end
-
-    it 'sets the keys to the project IDs' do
-      expect(hash.keys).to eq([project.id])
-    end
-
-    it 'sets the values to the access levels' do
-      expect(hash.values).to eq([Gitlab::Access::MAINTAINER])
-    end
-
-    context 'personal projects' do
-      it 'includes the project with the right access level' do
-        expect(hash[project.id]).to eq(Gitlab::Access::MAINTAINER)
-      end
-    end
-
-    context 'projects the user is a member of' do
-      let!(:other_project) { create(:project) }
-
-      before do
-        other_project.team.add_reporter(user)
-      end
-
-      it 'includes the project with the right access level' do
-        expect(hash[other_project.id]).to eq(Gitlab::Access::REPORTER)
-      end
-    end
-
-    context 'projects of groups the user is a member of' do
-      let(:group) { create(:group) }
-      let!(:other_project) { create(:project, group: group) }
-
-      before do
-        group.add_owner(user)
-      end
-
-      it 'includes the project with the right access level' do
-        expect(hash[other_project.id]).to eq(Gitlab::Access::OWNER)
-      end
-    end
-
-    context 'projects of subgroups of groups the user is a member of' do
-      let(:group) { create(:group) }
-      let(:nested_group) { create(:group, parent: group) }
-      let!(:other_project) { create(:project, group: nested_group) }
-
-      before do
-        group.add_maintainer(user)
-      end
-
-      it 'includes the project with the right access level' do
-        expect(hash[other_project.id]).to eq(Gitlab::Access::MAINTAINER)
-      end
-    end
-
-    context 'projects shared with groups the user is a member of' do
-      let(:group) { create(:group) }
-      let(:other_project) { create(:project) }
-      let!(:project_group_link) { create(:project_group_link, project: other_project, group: group, group_access: Gitlab::Access::GUEST) }
-
-      before do
-        group.add_maintainer(user)
-      end
-
-      it 'includes the project with the right access level' do
-        expect(hash[other_project.id]).to eq(Gitlab::Access::GUEST)
-      end
-    end
-
-    context 'projects shared with subgroups of groups the user is a member of' do
-      let(:group) { create(:group) }
-      let(:nested_group) { create(:group, parent: group) }
-      let(:other_project) { create(:project) }
-      let!(:project_group_link) { create(:project_group_link, project: other_project, group: nested_group, group_access: Gitlab::Access::DEVELOPER) }
-
-      before do
-        group.add_maintainer(user)
-      end
-
-      it 'includes the project with the right access level' do
-        expect(hash[other_project.id]).to eq(Gitlab::Access::DEVELOPER)
-      end
-    end
-  end
-
-  describe '#current_authorizations_per_project' do
-    let(:hash) { service.current_authorizations_per_project }
-
-    it 'returns a Hash' do
-      expect(hash).to be_an_instance_of(Hash)
-    end
-
-    it 'sets the keys to the project IDs' do
-      expect(hash.keys).to eq([project.id])
-    end
-
-    it 'sets the values to the project authorization rows' do
-      expect(hash.values.length).to eq(1)
-
-      value = hash.values[0]
-
-      expect(value.project_id).to eq(project.id)
-      expect(value.access_level).to eq(Gitlab::Access::MAINTAINER)
-    end
-  end
-
-  describe '#current_authorizations' do
-    context 'without authorizations' do
-      it 'returns an empty list' do
-        user.project_authorizations.delete_all
-
-        expect(service.current_authorizations.empty?).to eq(true)
-      end
-    end
-
-    context 'with an authorization' do
-      let(:row) { service.current_authorizations.take }
-
-      it 'returns the currently authorized projects' do
-        expect(service.current_authorizations.length).to eq(1)
-      end
-
-      it 'includes the project ID for every row' do
-        expect(row.project_id).to eq(project.id)
-      end
-
-      it 'includes the access level for every row' do
-        expect(row.access_level).to eq(Gitlab::Access::MAINTAINER)
-      end
-    end
-  end
-
-  describe '#fresh_authorizations' do
-    it 'returns the new authorized projects' do
-      expect(service.fresh_authorizations.length).to eq(1)
-    end
-
-    it 'returns the highest access level' do
-      project.team.add_guest(user)
-
-      rows = service.fresh_authorizations.to_a
-
-      expect(rows.length).to eq(1)
-      expect(rows.first.access_level).to eq(Gitlab::Access::MAINTAINER)
-    end
-
-    context 'every returned row' do
-      let(:row) { service.fresh_authorizations.take }
-
-      it 'includes the project ID' do
-        expect(row.project_id).to eq(project.id)
-      end
-
-      it 'includes the access level' do
-        expect(row.access_level).to eq(Gitlab::Access::MAINTAINER)
-      end
-    end
-  end
 end

spec/tooling/danger/project_helper_spec.rb

@@ -140,6 +140,7 @@ RSpec.describe Tooling::Danger::ProjectHelper do
       'ee/db/geo/post_migrate/foo'                                | [:database, :migration]
       'app/models/project_authorization.rb'                       | [:database]
       'app/services/users/refresh_authorized_projects_service.rb' | [:database]
+      'app/services/authorized_project_update/find_records_due_for_refresh_service.rb' | [:database]
       'lib/gitlab/background_migration.rb'                        | [:database]
       'lib/gitlab/background_migration/foo'                       | [:database]
       'ee/lib/gitlab/background_migration/foo'                    | [:database]

tooling/danger/project_helper.rb

@@ -78,6 +78,7 @@ module Tooling
         %r{\A(ee/)?db/(geo/)?(migrate|post_migrate)/} => [:database, :migration],
         %r{\A(ee/)?db/(?!fixtures)[^/]+} => :database,
         %r{\A(ee/)?lib/gitlab/(database|background_migration|sql|github_import)(/|\.rb)} => :database,
+        %r{\A(app/services/authorized_project_update/find_records_due_for_refresh_service)(/|\.rb)} => :database,
         %r{\A(app/models/project_authorization|app/services/users/refresh_authorized_projects_service)(/|\.rb)} => :database,
         %r{\A(ee/)?app/finders/} => :database,
         %r{\Arubocop/cop/migration(/|\.rb)} => :database,