Merge branch 'update_user_highest_roles_table' into 'master'

Update user highest roles table

See merge request gitlab-org/gitlab!27231

app/models/member.rb

@@ -100,6 +100,7 @@ class Member < ApplicationRecord
   after_destroy :destroy_notification_setting
   after_destroy :post_destroy_hook, unless: :pending?
   after_commit :refresh_member_authorized_projects
+  after_commit :update_highest_role
 
   default_value_for :notification_level, NotificationSetting.levels[:global]
 
@@ -459,6 +460,22 @@ class Member < ApplicationRecord
       errors.add(:access_level, s_("should be greater than or equal to %{access} inherited membership from group %{group_name}") % error_parameters)
     end
   end
+
+  # Triggers the service to schedule a Sidekiq job to update the highest role
+  # for a User
+  #
+  # The job will be called outside of a transaction in order to ensure the changes
+  # for a Member to be commited before attempting to update the highest role.
+  # rubocop: disable CodeReuse/ServiceClass
+  def update_highest_role
+    return unless user_id.present?
+    return unless previous_changes[:access_level].present?
+
+    run_after_commit_or_now do
+      Members::UpdateHighestRoleService.new(user_id).execute
+    end
+  end
+  # rubocop: enable CodeReuse/ServiceClass
 end
 
 Member.prepend_if_ee('EE::Member')

app/models/user.rb

@@ -1694,6 +1694,11 @@ class User < ApplicationRecord
     end
   end
 
+  # Load the current highest access by looking directly at the user's memberships
+  def current_highest_access_level
+    members.non_request.maximum(:access_level)
+  end
+
   protected
 
   # override, from Devise::Validatable

app/services/members/update_highest_role_service.rb

+# frozen_string_literal: true
+
+module Members
+  class UpdateHighestRoleService < ::BaseService
+    include ExclusiveLeaseGuard
+
+    LEASE_TIMEOUT = 30.minutes.to_i
+
+    attr_reader :user_id
+
+    def initialize(user_id)
+      @user_id = user_id
+    end
+
+    def execute
+      try_obtain_lease do
+        UpdateHighestRoleWorker.perform_async(user_id)
+      end
+    end
+
+    private
+
+    def lease_key
+      "update_highest_role:#{user_id}"
+    end
+
+    def lease_timeout
+      LEASE_TIMEOUT
+    end
+
+    # Do not release the lease before the timeout to
+    # prevent multiple jobs being executed during the
+    # defined timeout
+    def lease_release?
+      false
+    end
+  end
+end

app/services/users/update_highest_member_role_service.rb

+# frozen_string_literal: true
+
+module Users
+  class UpdateHighestMemberRoleService < BaseService
+    attr_reader :user, :identity_params
+
+    def initialize(user)
+      @user = user
+    end
+
+    def execute
+      return true if user_highest_role.persisted? && highest_access_level == user_highest_role.highest_access_level
+
+      user_highest_role.update!(highest_access_level: highest_access_level)
+    end
+
+    private
+
+    def user_highest_role
+      @user_highest_role ||= begin
+        @user.user_highest_role || @user.build_user_highest_role
+      end
+    end
+
+    def highest_access_level
+      @highest_access_level ||= @user.current_highest_access_level
+    end
+  end
+end

app/workers/all_queues.yml

@@ -1333,6 +1333,13 @@
   :resource_boundary: :unknown
   :weight: 3
   :idempotent: 
+- :name: update_highest_role
+  :feature_category: :authentication_and_authorization
+  :has_external_dependencies: 
+  :urgency: :high
+  :resource_boundary: :unknown
+  :weight: 2
+  :idempotent: true
 - :name: update_merge_requests
   :feature_category: :source_code_management
   :has_external_dependencies: 

app/workers/update_highest_role_worker.rb

+# frozen_string_literal: true
+
+class UpdateHighestRoleWorker
+  include ApplicationWorker
+
+  feature_category :authentication_and_authorization
+  urgency :high
+  weight 2
+
+  idempotent!
+
+  # rubocop: disable CodeReuse/ActiveRecord
+  def perform(user_id)
+    user = User.active.find_by(id: user_id)
+
+    Users::UpdateHighestMemberRoleService.new(user).execute if user.present?
+  end
+  # rubocop: enable CodeReuse/ActiveRecord
+end

changelogs/unreleased/update_user_highest_roles_table.yml

+---
+title: Update user's highest role to keep the users statistics up to date
+merge_request: 27231
+author:
+type: added

config/sidekiq_queues.yml

@@ -244,6 +244,8 @@
   - 1
 - - update_external_pull_requests
   - 3
+- - update_highest_role
+  - 2
 - - update_merge_requests
   - 3
 - - update_namespace_statistics

ee/spec/services/geo/design_repository_sync_service_spec.rb

@@ -8,7 +8,8 @@ describe Geo::DesignRepositorySyncService do
 
   let_it_be(:primary) { create(:geo_node, :primary) }
   let_it_be(:secondary) { create(:geo_node) }
-  let_it_be(:project) { create(:project_empty_repo) }
+  let(:user) { create(:user) }
+  let(:project) { create(:project_empty_repo, namespace: create(:namespace, owner: user)) }
 
   let(:repository) { project.design_repository }
   let(:lease_key) { "geo_sync_service:design:#{project.id}" }
@@ -27,6 +28,8 @@ describe Geo::DesignRepositorySyncService do
     let(:url_to_repo) { "#{primary.url}#{project.full_path}.design.git" }
 
     before do
+      allow_any_instance_of(Member).to receive(:update_highest_role) # avoid stubbing exclusive lease for this method
+
       stub_exclusive_lease(lease_key, lease_uuid)
       stub_exclusive_lease("geo_project_housekeeping:#{project.id}")
 

spec/factories/user_highest_roles.rb

@@ -2,14 +2,13 @@
 
 FactoryBot.define do
   factory :user_highest_role do
+    highest_access_level { nil }
     user
 
-    trait :maintainer do
-      highest_access_level { Gitlab::Access::MAINTAINER }
-    end
-
-    trait :developer do
-      highest_access_level { Gitlab::Access::DEVELOPER }
-    end
+    trait(:guest)      { highest_access_level { GroupMember::GUEST } }
+    trait(:reporter)   { highest_access_level { GroupMember::REPORTER } }
+    trait(:developer)  { highest_access_level { GroupMember::DEVELOPER } }
+    trait(:maintainer) { highest_access_level { GroupMember::MAINTAINER } }
+    trait(:owner)      { highest_access_level { GroupMember::OWNER } }
   end
 end

spec/models/member_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe Member do
+  using RSpec::Parameterized::TableSyntax
+
   describe "Associations" do
     it { is_expected.to belong_to(:user) }
   end
@@ -582,4 +584,54 @@ describe Member do
       expect(user.authorized_projects).not_to include(project)
     end
   end
+
+  context 'when after_commit :update_highest_role' do
+    where(:member_type, :source_type) do
+      :project_member | :project
+      :group_member   | :group
+    end
+
+    with_them do
+      describe 'create member' do
+        it 'initializes a new Members::UpdateHighestRoleService object' do
+          source = create(source_type) # source owner initializes a new service object too
+          user = create(:user)
+
+          expect(Members::UpdateHighestRoleService).to receive(:new).with(user.id).and_call_original
+
+          create(member_type, :guest, user: user, source_type => source)
+        end
+      end
+
+      context 'when member exists' do
+        let!(:member) { create(member_type) }
+
+        describe 'update member' do
+          context 'when access level was changed' do
+            it 'initializes a new Members::UpdateHighestRoleService object' do
+              expect(Members::UpdateHighestRoleService).to receive(:new).with(member.user_id).and_call_original
+
+              member.update(access_level: Gitlab::Access::GUEST)
+            end
+          end
+
+          context 'when access level was not changed' do
+            it 'does not initialize a new Members::UpdateHighestRoleService object' do
+              expect(Members::UpdateHighestRoleService).not_to receive(:new).with(member.user_id)
+
+              member.update(notification_level: NotificationSetting.levels[:disabled])
+            end
+          end
+        end
+
+        describe 'destroy member' do
+          it 'initializes a new Members::UpdateHighestRoleService object' do
+            expect(Members::UpdateHighestRoleService).to receive(:new).with(member.user_id).and_call_original
+
+            member.destroy
+          end
+        end
+      end
+    end
+  end
 end

spec/models/user_spec.rb

@@ -4360,4 +4360,24 @@ describe User, :do_not_mock_admin_mode do
       it { is_expected.to be expected_result }
     end
   end
+
+  describe '#current_highest_access_level' do
+    let_it_be(:user) { create(:user) }
+
+    context 'when no memberships exist' do
+      it 'returns nil' do
+        expect(user.current_highest_access_level).to be_nil
+      end
+    end
+
+    context 'when memberships exist' do
+      it 'returns the highest access level for non requested memberships' do
+        create(:group_member, :reporter, user_id: user.id)
+        create(:project_member, :guest, user_id: user.id)
+        create(:project_member, :maintainer, user_id: user.id, requested_at: Time.current)
+
+        expect(user.current_highest_access_level).to eq(Gitlab::Access::REPORTER)
+      end
+    end
+  end
 end

spec/services/members/update_highest_role_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require 'sidekiq/testing'
+
+describe Members::UpdateHighestRoleService, :clean_gitlab_redis_shared_state do
+  include ExclusiveLeaseHelpers
+
+  let_it_be(:user) { create(:user) }
+  let_it_be(:lease_key) { "update_highest_role:#{user.id}" }
+  let(:service) { described_class.new(user.id) }
+
+  describe '#perform' do
+    subject { service.execute }
+
+    context 'when lease is obtained' do
+      it 'takes the lease but does not release it', :aggregate_failures do
+        expect_to_obtain_exclusive_lease(lease_key, 'uuid', timeout: described_class::LEASE_TIMEOUT)
+
+        subject
+
+        expect(service.exclusive_lease.exists?).to be_truthy
+      end
+
+      it 'schedules a job' do
+        Sidekiq::Testing.fake! do
+          expect { subject }.to change(UpdateHighestRoleWorker.jobs, :size).by(1)
+        end
+      end
+    end
+
+    context 'when lease cannot be obtained' do
+      it 'only schedules one job' do
+        Sidekiq::Testing.fake! do
+          stub_exclusive_lease_taken(lease_key, timeout: described_class::LEASE_TIMEOUT)
+
+          expect { subject }.not_to change(UpdateHighestRoleWorker.jobs, :size)
+        end
+      end
+    end
+  end
+end

spec/services/users/update_highest_member_role_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Users::UpdateHighestMemberRoleService do
+  let(:user) { create(:user) }
+  let(:execute_service) { described_class.new(user).execute }
+
+  describe '#execute' do
+    context 'when user_highest_role already exists' do
+      let!(:user_highest_role) { create(:user_highest_role, :guest, user: user) }
+
+      context 'when the current highest access level equals the already stored highest access level' do
+        it 'does not update the highest access level' do
+          create(:group_member, :guest, user: user)
+
+          expect { execute_service }.not_to change { user_highest_role.reload.highest_access_level }
+        end
+      end
+
+      context 'when the current highest access level does not equal the already stored highest access level' do
+        it 'updates the highest access level' do
+          create(:group_member, :developer, user: user)
+
+          expect { execute_service }
+            .to change { user_highest_role.reload.highest_access_level }
+            .from(Gitlab::Access::GUEST)
+            .to(Gitlab::Access::DEVELOPER)
+        end
+      end
+    end
+
+    context 'when user_highest_role does not exist' do
+      it 'creates an user_highest_role object to store the highest access level' do
+        create(:group_member, :guest, user: user)
+
+        expect { execute_service }.to change { UserHighestRole.count }.from(0).to(1)
+      end
+    end
+  end
+end

spec/workers/cluster_update_app_worker_spec.rb

@@ -47,11 +47,11 @@ describe ClusterUpdateAppWorker do
     end
 
     context 'with exclusive lease' do
+      let_it_be(:user) { create(:user) }
       let(:application) { create(:clusters_applications_prometheus, :installed) }
       let(:lease_key) { "#{described_class.name.underscore}-#{application.id}" }
 
       before do
-        allow(Gitlab::ExclusiveLease).to receive(:new)
         stub_exclusive_lease_taken(lease_key)
       end
 
@@ -61,8 +61,10 @@ describe ClusterUpdateAppWorker do
         subject.perform(application.name, application.id, project.id, Time.now)
       end
 
-      it 'does not allow same app to be updated concurrently by different project' do
-        project1 = create(:project)
+      it 'does not allow same app to be updated concurrently by different project', :aggregate_failures do
+        stub_exclusive_lease("refresh_authorized_projects:#{user.id}")
+        stub_exclusive_lease("update_highest_role:#{user.id}")
+        project1 = create(:project, namespace: create(:namespace, owner: user))
 
         expect(Clusters::Applications::PrometheusUpdateService).not_to receive(:new)
 
@@ -81,10 +83,13 @@ describe ClusterUpdateAppWorker do
         subject.perform(application2.name, application2.id, project.id, Time.now)
       end
 
-      it 'allows different app to be updated by different project' do
+      it 'allows different app to be updated by different project', :aggregate_failures do
         application2 = create(:clusters_applications_prometheus, :installed)
         lease_key2 = "#{described_class.name.underscore}-#{application2.id}"
-        project2 = create(:project)
+
+        stub_exclusive_lease("refresh_authorized_projects:#{user.id}")
+        stub_exclusive_lease("update_highest_role:#{user.id}")
+        project2 = create(:project, namespace: create(:namespace, owner: user))
 
         stub_exclusive_lease(lease_key2)
 

spec/workers/update_highest_role_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe UpdateHighestRoleWorker, :clean_gitlab_redis_shared_state do
+  include ExclusiveLeaseHelpers
+
+  let(:worker) { described_class.new }
+
+  describe '#perform' do
+    let(:active_scope_attributes) do
+      {
+        state: 'active',
+        ghost: false,
+        user_type: nil
+      }
+    end
+    let(:user) { create(:user, attributes) }
+
+    subject { worker.perform(user.id) }
+
+    context 'when user is found' do
+      let(:attributes) { active_scope_attributes }
+
+      it 'updates the highest role for the user' do
+        user_highest_role = create(:user_highest_role, user: user)
+        create(:group_member, :developer, user: user)
+
+        expect { subject }
+          .to change { user_highest_role.reload.highest_access_level }
+          .from(nil)
+          .to(Gitlab::Access::DEVELOPER)
+      end
+    end
+
+    context 'when user is not found' do
+      shared_examples 'no update' do
+        it 'does not update any highest role' do
+          expect(Users::UpdateHighestMemberRoleService).not_to receive(:new)
+
+          worker.perform(user.id)
+        end
+      end
+
+      context 'when user is blocked' do
+        let(:attributes) { active_scope_attributes.merge(state: 'blocked') }
+
+        it_behaves_like 'no update'
+      end
+
+      context 'when user is a ghost' do
+        let(:attributes) { active_scope_attributes.merge(ghost: true) }
+
+        it_behaves_like 'no update'
+      end
+
+      context 'when user has a user type' do
+        let(:attributes) { active_scope_attributes.merge(user_type: :alert_bot) }
+
+        it_behaves_like 'no update'
+      end
+    end
+  end
+end