Merge branch 'mmj-project-member-auth-recalculation' into 'master'

Use specialized service to refresh authorizations when a member is added/removed/updated in a project

See merge request gitlab-org/gitlab!67477

app/models/member.rb

@@ -393,11 +393,6 @@ class Member < ApplicationRecord
   # error or not doing any meaningful work.
   # rubocop: disable CodeReuse/ServiceClass
   def refresh_member_authorized_projects
-    # If user/source is being destroyed, project access are going to be
-    # destroyed eventually because of DB foreign keys, so we shouldn't bother
-    # with refreshing after each member is destroyed through association
-    return if destroyed_by_association.present?
-
     UserProjectAccessChangedService.new(user_id).execute
   end
   # rubocop: enable CodeReuse/ServiceClass

app/models/members/group_member.rb

 # frozen_string_literal: true
 
 class GroupMember < Member
+  extend ::Gitlab::Utils::Override
   include FromUnion
   include CreatedAtFilterable
 
@@ -49,6 +50,19 @@ class GroupMember < Member
     { group: group }
   end
 
+  override :refresh_member_authorized_projects
+  def refresh_member_authorized_projects
+    # Here, `destroyed_by_association` will be present if the
+    # GroupMember is being destroyed due to the `dependent: :destroy`
+    # callback on Group. In this case, there is no need to refresh the
+    # authorizations, because whenever a Group is being destroyed,
+    # its projects are also destroyed, so the removal of project_authorizations
+    # will happen behind the scenes via DB foreign keys anyway.
+    return if destroyed_by_association.present?
+
+    super
+  end
+
   private
 
   def access_level_inclusion

app/models/members/project_member.rb

 # frozen_string_literal: true
 
 class ProjectMember < Member
+  extend ::Gitlab::Utils::Override
   SOURCE_TYPE = 'Project'
 
   belongs_to :project, foreign_key: 'source_id'
@@ -89,6 +90,22 @@ class ProjectMember < Member
     { project: project }
   end
 
+  override :refresh_member_authorized_projects
+  def refresh_member_authorized_projects
+    return super unless Feature.enabled?(:specialized_service_for_project_member_auth_refresh)
+    return unless user
+
+    # rubocop:disable CodeReuse/ServiceClass
+    AuthorizedProjectUpdate::ProjectRecalculatePerUserService.new(project, user).execute
+
+    # Until we compare the inconsistency rates of the new, specialized service and
+    # the old approach, we still run AuthorizedProjectsWorker
+    # but with some delay and lower urgency as a safety net.
+    UserProjectAccessChangedService.new(user_id)
+      .execute(blocking: false, priority: UserProjectAccessChangedService::LOW_PRIORITY)
+    # rubocop:enable CodeReuse/ServiceClass
+  end
+
   private
 
   def send_invite

app/services/projects/create_service.rb

@@ -65,7 +65,7 @@ module Projects
 
       save_project_and_import_data
 
-      Gitlab::ApplicationContext.with_context(related_class: "Projects::CreateService", project: @project) do
+      Gitlab::ApplicationContext.with_context(project: @project) do
         after_create_actions if @project.persisted?
 
         import_schedule

config/feature_flags/development/specialized_service_for_project_member_auth_refresh.yml

+---
+name: specialized_service_for_project_member_auth_refresh
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/67477
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/337730
+milestone: '14.2'
+type: development
+group: group::access
+default_enabled: false

ee/spec/graphql/resolvers/vulnerabilities_resolver_spec.rb

@@ -8,7 +8,7 @@ RSpec.describe Resolvers::VulnerabilitiesResolver do
   describe '#resolve' do
     subject { resolve(described_class, obj: vulnerable, args: params, ctx: { current_user: current_user }) }
 
-    let_it_be(:project) { create(:project) }
+    let_it_be_with_reload(:project) { create(:project) }
     let_it_be(:user) { create(:user, security_dashboard_projects: [project]) }
 
     let_it_be(:low_vulnerability) do

ee/spec/lib/gitlab/code_owners/loader_spec.rb

@@ -81,7 +81,8 @@ RSpec.describe Gitlab::CodeOwners::Loader do
         another_group = create(:group, parent: test_group, path: 'nested-group')
         another_group.add_developer(create(:user))
 
-        project.invited_groups << [test_group, another_group]
+        create(:project_group_link, project: project, group: test_group)
+        create(:project_group_link, project: project, group: another_group)
 
         # - 1 query for users
         # - 1 for the emails to later divide them across the entries
@@ -98,7 +99,7 @@ RSpec.describe Gitlab::CodeOwners::Loader do
 
       it 'loads group members as code owners' do
         test_group = create(:group, path: 'test-group')
-        project.invited_groups << test_group
+        create(:project_group_link, project: project, group: test_group)
 
         group_user = create(:user)
 

ee/spec/models/group_member_spec.rb

@@ -105,6 +105,41 @@ RSpec.describe GroupMember do
     end
   end
 
+  context 'refreshing project_authorizations' do
+    let_it_be_with_refind(:group) { create(:group) }
+    let_it_be_with_refind(:user) { create(:user) }
+    let_it_be(:group_member) { create(:group_member, :guest, group: group, user: user) }
+    let_it_be(:project) { create(:project, namespace: group) }
+
+    context 'when the source group of the group member is destroyed' do
+      it 'refreshes the authorization of user to the project in the group' do
+        expect { group.destroy! }.to change { user.can?(:guest_access, project) }.from(true).to(false)
+      end
+
+      it 'refreshes the authorization without calling UserProjectAccessChangedService' do
+        expect(UserProjectAccessChangedService).not_to receive(:new)
+
+        group.destroy!
+      end
+    end
+
+    context 'when the user of the group member is destroyed' do
+      it 'refreshes the authorization of user to the project in the group' do
+        expect(project.authorized_users).to include(user)
+
+        user.destroy!
+
+        expect(project.authorized_users).not_to include(user)
+      end
+
+      it 'refreshes the authorization without calling UserProjectAccessChangedService' do
+        expect(UserProjectAccessChangedService).not_to receive(:new)
+
+        user.destroy!
+      end
+    end
+  end
+
   context 'group member webhooks', :sidekiq_inline do
     let_it_be_with_refind(:group) { create(:group_with_plan, plan: :ultimate_plan) }
     let_it_be(:group_hook) { create(:group_hook, group: group, member_events: true) }

ee/spec/models/protected_environment/deploy_access_level_spec.rb

@@ -2,7 +2,7 @@
 require 'spec_helper'
 
 RSpec.describe ProtectedEnvironment::DeployAccessLevel do
-  let_it_be(:project) { create(:project) }
+  let_it_be_with_reload(:project) { create(:project) }
   let_it_be(:protected_environment) { create(:protected_environment, project: project) }
   let_it_be(:user) { create(:user) }
 

spec/models/members/project_member_spec.rb

@@ -139,4 +139,161 @@ RSpec.describe ProjectMember do
       end
     end
   end
+
+  context 'refreshing project_authorizations' do
+    let_it_be_with_refind(:project) { create(:project) }
+    let_it_be_with_refind(:user) { create(:user) }
+    let_it_be(:project_member) { create(:project_member, :guest, project: project, user: user) }
+
+    context 'when the source project of the project member is destroyed' do
+      it 'refreshes the authorization of user to the project in the group' do
+        expect { project.destroy! }.to change { user.can?(:guest_access, project) }.from(true).to(false)
+      end
+
+      it 'refreshes the authorization without calling AuthorizedProjectUpdate::ProjectRecalculatePerUserService' do
+        expect(AuthorizedProjectUpdate::ProjectRecalculatePerUserService).not_to receive(:new)
+
+        project.destroy!
+      end
+    end
+
+    context 'when the user of the project member is destroyed' do
+      it 'refreshes the authorization of user to the project in the group' do
+        expect(project.authorized_users).to include(user)
+
+        user.destroy!
+
+        expect(project.authorized_users).not_to include(user)
+      end
+
+      it 'refreshes the authorization without calling UserProjectAccessChangedService' do
+        expect(UserProjectAccessChangedService).not_to receive(:new)
+
+        user.destroy!
+      end
+    end
+  end
+
+  context 'authorization refresh on addition/updation/deletion' do
+    let_it_be(:project) { create(:project) }
+    let_it_be(:user) { create(:user) }
+
+    shared_examples_for 'calls AuthorizedProjectUpdate::ProjectRecalculatePerUserService to recalculate authorizations' do
+      it 'calls AuthorizedProjectUpdate::ProjectRecalculatePerUserService' do
+        expect_next_instance_of(AuthorizedProjectUpdate::ProjectRecalculatePerUserService, project, user) do |service|
+          expect(service).to receive(:execute)
+        end
+
+        action
+      end
+    end
+
+    shared_examples_for 'calls AuthorizedProjectUpdate::UserRefreshFromReplicaWorker with a delay to update project authorizations' do
+      it 'calls AuthorizedProjectUpdate::UserRefreshFromReplicaWorker' do
+        expect(AuthorizedProjectUpdate::UserRefreshFromReplicaWorker).to(
+          receive(:bulk_perform_in)
+            .with(1.hour,
+                  [[user.id]],
+                  batch_delay: 30.seconds, batch_size: 100)
+        )
+
+        action
+      end
+    end
+
+    context 'on create' do
+      let(:action) { project.add_user(user, Gitlab::Access::GUEST) }
+
+      it 'changes access level' do
+        expect { action }.to change { user.can?(:guest_access, project) }.from(false).to(true)
+      end
+
+      it_behaves_like 'calls AuthorizedProjectUpdate::ProjectRecalculatePerUserService to recalculate authorizations'
+      it_behaves_like 'calls AuthorizedProjectUpdate::UserRefreshFromReplicaWorker with a delay to update project authorizations'
+    end
+
+    context 'on update' do
+      let(:action) { project.members.find_by(user: user).update!(access_level: Gitlab::Access::DEVELOPER) }
+
+      before do
+        project.add_user(user, Gitlab::Access::GUEST)
+      end
+
+      it 'changes access level' do
+        expect { action }.to change { user.can?(:developer_access, project) }.from(false).to(true)
+      end
+
+      it_behaves_like 'calls AuthorizedProjectUpdate::ProjectRecalculatePerUserService to recalculate authorizations'
+      it_behaves_like 'calls AuthorizedProjectUpdate::UserRefreshFromReplicaWorker with a delay to update project authorizations'
+    end
+
+    context 'on destroy' do
+      let(:action) { project.members.find_by(user: user).destroy! }
+
+      before do
+        project.add_user(user, Gitlab::Access::GUEST)
+      end
+
+      it 'changes access level' do
+        expect { action }.to change { user.can?(:guest_access, project) }.from(true).to(false)
+      end
+
+      it_behaves_like 'calls AuthorizedProjectUpdate::ProjectRecalculatePerUserService to recalculate authorizations'
+      it_behaves_like 'calls AuthorizedProjectUpdate::UserRefreshFromReplicaWorker with a delay to update project authorizations'
+    end
+
+    context 'when the feature flag `specialized_service_for_project_member_auth_refresh` is disabled' do
+      before do
+        stub_feature_flags(specialized_service_for_project_member_auth_refresh: false)
+      end
+
+      shared_examples_for 'calls UserProjectAccessChangedService to recalculate authorizations' do
+        it 'calls UserProjectAccessChangedService' do
+          expect_next_instance_of(UserProjectAccessChangedService, user.id) do |service|
+            expect(service).to receive(:execute)
+          end
+
+          action
+        end
+      end
+
+      context 'on create' do
+        let(:action) { project.add_user(user, Gitlab::Access::GUEST) }
+
+        it 'changes access level' do
+          expect { action }.to change { user.can?(:guest_access, project) }.from(false).to(true)
+        end
+
+        it_behaves_like 'calls UserProjectAccessChangedService to recalculate authorizations'
+      end
+
+      context 'on update' do
+        let(:action) { project.members.find_by(user: user).update!(access_level: Gitlab::Access::DEVELOPER) }
+
+        before do
+          project.add_user(user, Gitlab::Access::GUEST)
+        end
+
+        it 'changes access level' do
+          expect { action }.to change { user.can?(:developer_access, project) }.from(false).to(true)
+        end
+
+        it_behaves_like 'calls UserProjectAccessChangedService to recalculate authorizations'
+      end
+
+      context 'on destroy' do
+        let(:action) { project.members.find_by(user: user).destroy! }
+
+        before do
+          project.add_user(user, Gitlab::Access::GUEST)
+        end
+
+        it 'changes access level' do
+          expect { action }.to change { user.can?(:guest_access, project) }.from(true).to(false)
+        end
+
+        it_behaves_like 'calls UserProjectAccessChangedService to recalculate authorizations'
+      end
+    end
+  end
 end

spec/requests/api/project_milestones_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 RSpec.describe API::ProjectMilestones do
   let_it_be(:user) { create(:user) }
-  let_it_be(:project) { create(:project, namespace: user.namespace ) }
+  let_it_be_with_reload(:project) { create(:project, namespace: user.namespace ) }
   let_it_be(:closed_milestone) { create(:closed_milestone, project: project, title: 'version1', description: 'closed milestone') }
   let_it_be(:milestone) { create(:milestone, project: project, title: 'version2', description: 'open milestone') }
   let_it_be(:route) { "/projects/#{project.id}/milestones" }

spec/services/notification_service_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe NotificationService, :mailer do
   include ExternalAuthorizationServiceHelpers
   include NotificationHelpers
 
-  let_it_be(:project, reload: true) { create(:project, :public) }
+  let_it_be_with_refind(:project) { create(:project, :public) }
   let_it_be_with_refind(:assignee) { create(:user) }
 
   let(:notification) { described_class.new }

spec/services/projects/create_service_spec.rb

@@ -135,7 +135,7 @@ RSpec.describe Projects::CreateService, '#execute' do
     end
 
     it_behaves_like 'storing arguments in the application context' do
-      let(:expected_params) { { project: subject.full_path, related_class: described_class.to_s } }
+      let(:expected_params) { { project: subject.full_path } }
 
       subject { create_project(user, opts) }
     end