Fix ProjectAuthorization calculation for shared groups

Calculation joined GroupGroupLinks table to Members table without
limiting the Member#source_type to Namespaces. As a result unrelated
Users were given access to Projects.

app/services/users/refresh_authorized_projects_service.rb

@@ -19,8 +19,10 @@ module Users
     LEASE_TIMEOUT = 1.minute.to_i
 
     # user - The User for which to refresh the authorized projects.
-    def initialize(user)
+    def initialize(user, incorrect_auth_found_callback: nil, missing_auth_found_callback: nil)
       @user = user
+      @incorrect_auth_found_callback = incorrect_auth_found_callback
+      @missing_auth_found_callback = missing_auth_found_callback
 
       # We need an up to date User object that has access to all relations that
       # may have been created earlier. The only way to ensure this is to reload
@@ -55,6 +57,10 @@ module Users
         # 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
@@ -63,6 +69,10 @@ module Users
         # 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
@@ -104,5 +114,9 @@ module Users
     def fresh_authorizations
       Gitlab::ProjectAuthorizations.new(user).calculate
     end
+
+    private
+
+    attr_reader :incorrect_auth_found_callback, :missing_auth_found_callback
   end
 end

changelogs/unreleased/202520-fix_project_auth_calculation_for_group_shares.yml

+---
+title: Fix ProjectAuthorization calculation for shared groups
+merge_request:
+author:
+type: security

db/post_migrate/20200204113223_schedule_recalculate_project_authorizations.rb

+# frozen_string_literal: true
+
+class ScheduleRecalculateProjectAuthorizations < ActiveRecord::Migration[5.1]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  MIGRATION = 'RecalculateProjectAuthorizations'
+  BATCH_SIZE = 2_500
+  DELAY_INTERVAL = 2.minutes.to_i
+
+  disable_ddl_transaction!
+
+  class Namespace < ActiveRecord::Base
+    include ::EachBatch
+
+    self.table_name = 'namespaces'
+  end
+
+  class ProjectAuthorization < ActiveRecord::Base
+    include ::EachBatch
+
+    self.table_name = 'project_authorizations'
+  end
+
+  def up
+    say "Scheduling #{MIGRATION} jobs"
+
+    max_group_id = Namespace.where(type: 'Group').maximum(:id)
+    project_authorizations = ProjectAuthorization.where('project_id <= ?', max_group_id)
+                                                 .select(:user_id)
+                                                 .distinct
+
+    project_authorizations.each_batch(of: BATCH_SIZE, column: :user_id) do |authorizations, index|
+      delay = index * DELAY_INTERVAL
+      user_ids = authorizations.map(&:user_id)
+      BackgroundMigrationWorker.perform_in(delay, MIGRATION, [user_ids])
+    end
+  end
+
+  def down
+  end
+end

lib/gitlab/background_migration/recalculate_project_authorizations.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # rubocop:disable Style/Documentation
+    class RecalculateProjectAuthorizations
+      def perform(user_ids)
+        user_ids.each do |user_id|
+          user = User.find_by(id: user_id)
+
+          next unless user
+
+          service = Users::RefreshAuthorizedProjectsService.new(
+            user,
+            incorrect_auth_found_callback:
+              ->(project_id, access_level) do
+                logger.info(message: 'Removing ProjectAuthorizations',
+                            user_id: user.id,
+                            project_id: project_id,
+                            access_level: access_level)
+              end,
+            missing_auth_found_callback:
+              ->(project_id, access_level) do
+                logger.info(message: 'Creating ProjectAuthorizations',
+                            user_id: user.id,
+                            project_id: project_id,
+                            access_level: access_level)
+              end
+          )
+
+          service.execute
+        end
+      end
+
+      private
+
+      def logger
+        @logger ||= Gitlab::BackgroundMigration::Logger.build
+      end
+    end
+  end
+end

lib/gitlab/project_authorizations.rb

@@ -68,12 +68,10 @@ module Gitlab
         .select([namespaces[:id], members[:access_level]])
         .except(:order)
 
-      if Feature.enabled?(:share_group_with_group, default_enabled: true)
-        # Namespaces shared with any of the group
-        cte << Group.select([namespaces[:id], 'group_group_links.group_access AS access_level'])
-                    .joins(join_group_group_links)
-                    .joins(join_members_on_group_group_links)
-      end
+      # Namespaces shared with any of the group
+      cte << Group.select([namespaces[:id], 'group_group_links.group_access AS access_level'])
+                  .joins(join_group_group_links)
+                  .joins(join_members_on_group_group_links)
 
       # Sub groups of any groups the user is a member of.
       cte << Group.select([
@@ -114,6 +112,8 @@ module Gitlab
       members = Member.arel_table
 
       cond = group_group_links[:shared_with_group_id].eq(members[:source_id])
+                    .and(members[:source_type].eq('Namespace'))
+                    .and(members[:requested_at].eq(nil))
                     .and(members[:user_id].eq(user.id))
       Arel::Nodes::InnerJoin.new(members, Arel::Nodes::On.new(cond))
     end

spec/factories/project_authorizations.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :project_authorization do
+    user
+    project
+    access_level { Gitlab::Access::REPORTER }
+  end
+end

spec/lib/gitlab/project_authorizations_spec.rb

@@ -97,87 +97,68 @@ describe Gitlab::ProjectAuthorizations do
       create(:group_group_link, shared_group: shared_group, shared_with_group: group)
     end
 
-    context 'when feature flag share_group_with_group is enabled' do
-      before do
-        stub_feature_flags(share_group_with_group: true)
-      end
-
-      context 'group user' do
-        let(:user) { group_user }
+    context 'group user' do
+      let(:user) { group_user }
 
-        it 'creates proper authorizations' do
-          mapping = map_access_levels(authorizations)
+      it 'creates proper authorizations' do
+        mapping = map_access_levels(authorizations)
 
-          expect(mapping[project_parent.id]).to be_nil
-          expect(mapping[project.id]).to eq(Gitlab::Access::DEVELOPER)
-          expect(mapping[project_child.id]).to eq(Gitlab::Access::DEVELOPER)
-        end
+        expect(mapping[project_parent.id]).to be_nil
+        expect(mapping[project.id]).to eq(Gitlab::Access::DEVELOPER)
+        expect(mapping[project_child.id]).to eq(Gitlab::Access::DEVELOPER)
       end
+    end
 
-      context 'parent group user' do
-        let(:user) { parent_group_user }
+    context 'parent group user' do
+      let(:user) { parent_group_user }
 
-        it 'creates proper authorizations' do
-          mapping = map_access_levels(authorizations)
+      it 'creates proper authorizations' do
+        mapping = map_access_levels(authorizations)
 
-          expect(mapping[project_parent.id]).to be_nil
-          expect(mapping[project.id]).to be_nil
-          expect(mapping[project_child.id]).to be_nil
-        end
+        expect(mapping[project_parent.id]).to be_nil
+        expect(mapping[project.id]).to be_nil
+        expect(mapping[project_child.id]).to be_nil
       end
+    end
 
-      context 'child group user' do
-        let(:user) { child_group_user }
+    context 'child group user' do
+      let(:user) { child_group_user }
 
-        it 'creates proper authorizations' do
-          mapping = map_access_levels(authorizations)
+      it 'creates proper authorizations' do
+        mapping = map_access_levels(authorizations)
 
-          expect(mapping[project_parent.id]).to be_nil
-          expect(mapping[project.id]).to be_nil
-          expect(mapping[project_child.id]).to be_nil
-        end
+        expect(mapping[project_parent.id]).to be_nil
+        expect(mapping[project.id]).to be_nil
+        expect(mapping[project_child.id]).to be_nil
       end
     end
 
-    context 'when feature flag share_group_with_group is disabled' do
-      before do
-        stub_feature_flags(share_group_with_group: false)
-      end
-
-      context 'group user' do
-        let(:user) { group_user }
-
-        it 'creates proper authorizations' do
-          mapping = map_access_levels(authorizations)
+    context 'user without accepted access request' do
+      let!(:user) { create(:user) }
 
-          expect(mapping[project_parent.id]).to be_nil
-          expect(mapping[project.id]).to be_nil
-          expect(mapping[project_child.id]).to be_nil
-        end
-      end
+      it 'does not have access to group and its projects' do
+        create(:group_member, :developer, :access_request, user: user, group: group)
 
-      context 'parent group user' do
-        let(:user) { parent_group_user }
+        mapping = map_access_levels(authorizations)
 
-        it 'creates proper authorizations' do
-          mapping = map_access_levels(authorizations)
-
-          expect(mapping[project_parent.id]).to be_nil
-          expect(mapping[project.id]).to be_nil
-          expect(mapping[project_child.id]).to be_nil
-        end
+        expect(mapping[project_parent.id]).to be_nil
+        expect(mapping[project.id]).to be_nil
+        expect(mapping[project_child.id]).to be_nil
       end
+    end
 
-      context 'child group user' do
-        let(:user) { child_group_user }
+    context 'unrelated project owner' do
+      let(:common_id) { [Project.maximum(:id).to_i, Namespace.maximum(:id).to_i].max + 999 }
+      let!(:group) { create(:group, id: common_id) }
+      let!(:unrelated_project) { create(:project, id: common_id) }
+      let(:user) { unrelated_project.owner }
 
-        it 'creates proper authorizations' do
-          mapping = map_access_levels(authorizations)
+      it 'does not have access to group and its projects' do
+        mapping = map_access_levels(authorizations)
 
-          expect(mapping[project_parent.id]).to be_nil
-          expect(mapping[project.id]).to be_nil
-          expect(mapping[project_child.id]).to be_nil
-        end
+        expect(mapping[project_parent.id]).to be_nil
+        expect(mapping[project.id]).to be_nil
+        expect(mapping[project_child.id]).to be_nil
       end
     end
   end

spec/migrations/schedule_recalculate_project_authorizations_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200204113223_schedule_recalculate_project_authorizations.rb')
+
+describe ScheduleRecalculateProjectAuthorizations, :migration do
+  let(:users_table) { table(:users) }
+  let(:namespaces_table) { table(:namespaces) }
+  let(:projects_table) { table(:projects) }
+  let(:project_authorizations_table) { table(:project_authorizations) }
+
+  let(:user1) { users_table.create!(name: 'user1', email: 'user1@example.com', projects_limit: 1) }
+  let(:user2) { users_table.create!(name: 'user2', email: 'user2@example.com', projects_limit: 1) }
+  let(:group) { namespaces_table.create!(id: 1, type: 'Group', name: 'group', path: 'group') }
+  let(:project) do
+    projects_table.create!(id: 1, name: 'project', path: 'project',
+                           visibility_level: 0, namespace_id: group.id)
+  end
+
+  before do
+    stub_const("#{described_class}::BATCH_SIZE", 1)
+
+    project_authorizations_table.create!(user_id: user1.id, project_id: project.id, access_level: 30)
+    project_authorizations_table.create!(user_id: user2.id, project_id: project.id, access_level: 30)
+  end
+
+  it 'schedules background migration' do
+    Sidekiq::Testing.fake! do
+      Timecop.freeze do
+        migrate!
+
+        expect(BackgroundMigrationWorker.jobs.size).to eq(2)
+        expect(described_class::MIGRATION).to be_scheduled_migration([user1.id])
+        expect(described_class::MIGRATION).to be_scheduled_migration([user2.id])
+      end
+    end
+  end
+
+  it 'ignores projects with higher id than maximum group id' do
+    another_user = users_table.create!(name: 'another user', email: 'another-user@example.com',
+                                       projects_limit: 1)
+    ignored_project = projects_table.create!(id: 2, name: 'ignored-project', path: 'ignored-project',
+                                             visibility_level: 0, namespace_id: group.id)
+    project_authorizations_table.create!(user_id: another_user.id, project_id: ignored_project.id,
+                                         access_level: 30)
+
+    Sidekiq::Testing.fake! do
+      Timecop.freeze do
+        migrate!
+
+        expect(BackgroundMigrationWorker.jobs.size).to eq(2)
+        expect(described_class::MIGRATION).to be_scheduled_migration([user1.id])
+        expect(described_class::MIGRATION).to be_scheduled_migration([user2.id])
+      end
+    end
+  end
+end

spec/services/users/refresh_authorized_projects_service_spec.rb

@@ -22,6 +22,42 @@ describe Users::RefreshAuthorizedProjectsService do
 
       service.execute
     end
+
+    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
   end
 
   describe '#execute_without_lease' do