Add latest changes from gitlab-org/security/gitlab@14-2-stable-ee

8 files changed, 121 insertions, 8 deletions

diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb
index 3e1e5faee54..60e1dde17b9 100644
--- a/app/models/concerns/resolvable_discussion.rb
+++ b/app/models/concerns/resolvable_discussion.rb
@@ -81,8 +81,7 @@ module ResolvableDiscussion
     return false unless current_user
     return false unless resolvable?
 
-    current_user == self.noteable.try(:author) ||
-      current_user.can?(:resolve_note, self.project)
+    current_user.can?(:resolve_note, self.noteable)
   end
 
   def resolve!(current_user)
diff --git a/app/models/project_group_link.rb b/app/models/project_group_link.rb
index d704f4c2c87..8394ebe1df4 100644
--- a/app/models/project_group_link.rb
+++ b/app/models/project_group_link.rb
@@ -15,6 +15,7 @@ class ProjectGroupLink < ApplicationRecord
   validate :different_group
 
   scope :non_guests, -> { where('group_access > ?', Gitlab::Access::GUEST) }
+  scope :in_group, -> (group_ids) { where(group_id: group_ids) }
 
   alias_method :shared_with_group, :group
 
diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb
index 61263e47d7c..39ce26526e6 100644
--- a/app/policies/issuable_policy.rb
+++ b/app/policies/issuable_policy.rb
@@ -11,6 +11,8 @@ class IssuablePolicy < BasePolicy
     @user && @subject.assignee_or_author?(@user)
   end
 
+  condition(:is_author) { @subject&.author == @user }
+
   rule { can?(:guest_access) & assignee_or_author }.policy do
     enable :read_issue
     enable :update_issue
@@ -20,6 +22,10 @@ class IssuablePolicy < BasePolicy
     enable :reopen_merge_request
   end
 
+  rule { is_author }.policy do
+    enable :resolve_note
+  end
+
   rule { locked & ~is_project_member }.policy do
     prevent :create_note
     prevent :admin_note
diff --git a/app/services/groups/transfer_service.rb b/app/services/groups/transfer_service.rb
index b7eae06b963..d0aea848af9 100644
--- a/app/services/groups/transfer_service.rb
+++ b/app/services/groups/transfer_service.rb
@@ -177,6 +177,14 @@ module Groups
 
       # schedule refreshing projects for all the members of the group
       @group.refresh_members_authorized_projects
+
+      # When a group is transferred, it also affects who gets access to the projects shared to
+      # the subgroups within its hierarchy, so we also schedule jobs that refresh authorizations for all such shared projects.
+      project_group_shares_within_the_hierarchy = ProjectGroupLink.in_group(group.self_and_descendants.select(:id))
+
+      project_group_shares_within_the_hierarchy.find_each do |project_group_link|
+        AuthorizedProjectUpdate::ProjectRecalculateWorker.perform_async(project_group_link.project_id)
+      end
     end
 
     def raise_transfer_error(message)
diff --git a/spec/graphql/mutations/discussions/toggle_resolve_spec.rb b/spec/graphql/mutations/discussions/toggle_resolve_spec.rb
index b03c6cb094f..8c11279a80a 100644
--- a/spec/graphql/mutations/discussions/toggle_resolve_spec.rb
+++ b/spec/graphql/mutations/discussions/toggle_resolve_spec.rb
@@ -7,6 +7,7 @@ RSpec.describe Mutations::Discussions::ToggleResolve do
     described_class.new(object: nil, context: { current_user: user }, field: nil)
   end
 
+  let_it_be(:author) { create(:user) }
   let_it_be(:project) { create(:project, :repository) }
 
   describe '#resolve' do
@@ -136,20 +137,37 @@ RSpec.describe Mutations::Discussions::ToggleResolve do
           end
         end
       end
+
+      context 'when user is the author and discussion is locked' do
+        let(:user) { author }
+
+        before do
+          issuable.update!(discussion_locked: true)
+        end
+
+        it 'raises an error' do
+          expect { mutation.resolve(id: id_arg, resolve: resolve_arg) }.to raise_error(
+            Gitlab::Graphql::Errors::ResourceNotAvailable,
+            "The resource that you are attempting to access does not exist or you don't have permission to perform this action"
+          )
+        end
+      end
     end
 
     context 'when discussion is on a merge request' do
-      let_it_be(:noteable) { create(:merge_request, source_project: project) }
+      let_it_be(:noteable) { create(:merge_request, source_project: project, author: author) }
 
       let(:discussion) { create(:diff_note_on_merge_request, noteable: noteable, project: project).to_discussion }
+      let(:issuable) { noteable }
 
       it_behaves_like 'a working resolve method'
     end
 
     context 'when discussion is on a design' do
-      let_it_be(:noteable) { create(:design, :with_file, issue: create(:issue, project: project)) }
+      let_it_be(:noteable) { create(:design, :with_file, issue: create(:issue, project: project, author: author)) }
 
       let(:discussion) { create(:diff_note_on_design, noteable: noteable, project: project).to_discussion }
+      let(:issuable) { noteable.issue }
 
       it_behaves_like 'a working resolve method'
     end
diff --git a/spec/models/concerns/resolvable_discussion_spec.rb b/spec/models/concerns/resolvable_discussion_spec.rb
index c0e5ddc23b1..fc154738f11 100644
--- a/spec/models/concerns/resolvable_discussion_spec.rb
+++ b/spec/models/concerns/resolvable_discussion_spec.rb
@@ -188,6 +188,16 @@ RSpec.describe Discussion, ResolvableDiscussion do
           it "returns true" do
             expect(subject.can_resolve?(current_user)).to be true
           end
+
+          context 'when noteable is locked' do
+            before do
+              allow(subject.noteable).to receive(:discussion_locked?).and_return(true)
+            end
+
+            it 'returns false' do
+              expect(subject.can_resolve?(current_user)).to be_falsey
+            end
+          end
         end
 
         context "when the signed in user can push to the project" do
@@ -200,8 +210,11 @@ RSpec.describe Discussion, ResolvableDiscussion do
           end
 
           context "when the noteable has no author" do
+            before do
+              subject.noteable.author = nil
+            end
+
             it "returns true" do
-              expect(noteable).to receive(:author).and_return(nil)
               expect(subject.can_resolve?(current_user)).to be true
             end
           end
@@ -213,8 +226,11 @@ RSpec.describe Discussion, ResolvableDiscussion do
           end
 
           context "when the noteable has no author" do
+            before do
+              subject.noteable.author = nil
+            end
+
             it "returns false" do
-              expect(noteable).to receive(:author).and_return(nil)
               expect(subject.can_resolve?(current_user)).to be false
             end
           end
diff --git a/spec/policies/issuable_policy_spec.rb b/spec/policies/issuable_policy_spec.rb
index 86b04ccda57..eeb298e853e 100644
--- a/spec/policies/issuable_policy_spec.rb
+++ b/spec/policies/issuable_policy_spec.rb
@@ -13,6 +13,10 @@ RSpec.describe IssuablePolicy, models: true do
       let(:merge_request) { create(:merge_request, source_project: project, author: user) }
       let(:policies) { described_class.new(user, merge_request) }
 
+      it 'allows resolving notes' do
+        expect(policies).to be_allowed(:resolve_note)
+      end
+
       context 'when user is able to read project' do
         it 'enables user to read and update issuables' do
           expect(policies).to be_allowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request, :reopen_merge_request)
@@ -43,16 +47,24 @@ RSpec.describe IssuablePolicy, models: true do
         it 'can not create a note nor award emojis' do
           expect(policies).to be_disallowed(:create_note, :award_emoji)
         end
+
+        it 'does not allow resolving note' do
+          expect(policies).to be_disallowed(:resolve_note)
+        end
       end
 
       context 'when the user is a project member' do
         before do
-          project.add_guest(user)
+          project.add_developer(user)
         end
 
         it 'can create a note and award emojis' do
           expect(policies).to be_allowed(:create_note, :award_emoji)
         end
+
+        it 'allows resolving notes' do
+          expect(policies).to be_allowed(:resolve_note)
+        end
       end
     end
   end
diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb
index 889b5551746..83337aad6f9 100644
--- a/spec/services/groups/transfer_service_spec.rb
+++ b/spec/services/groups/transfer_service_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Groups::TransferService do
+RSpec.describe Groups::TransferService, :sidekiq_inline do
   let_it_be(:user) { create(:user) }
   let_it_be(:new_parent_group) { create(:group, :public) }
 
@@ -594,6 +594,59 @@ RSpec.describe Groups::TransferService do
               transfer_service.execute(new_parent_group)
             end
           end
+
+          context 'transferring groups with shared_projects' do
+            let_it_be_with_reload(:shared_project) { create(:project, :public) }
+
+            shared_examples_for 'drops the authorizations of ancestor members from the old hierarchy' do
+              it 'drops the authorizations of ancestor members from the old hierarchy' do
+                expect { transfer_service.execute(new_parent_group) }.to change {
+                  ProjectAuthorization.where(project: shared_project, user: old_group_member).size
+                }.from(1).to(0)
+              end
+            end
+
+            context 'when the group that has existing project share is transferred' do
+              before do
+                create(:project_group_link, :maintainer, project: shared_project, group: group)
+              end
+
+              it_behaves_like 'drops the authorizations of ancestor members from the old hierarchy'
+            end
+
+            context 'when the group whose subgroup has an existing project share is transferred' do
+              let_it_be_with_reload(:subgroup) { create(:group, :private, parent: group) }
+
+              before do
+                create(:project_group_link, :maintainer, project: shared_project, group: subgroup)
+              end
+
+              it_behaves_like 'drops the authorizations of ancestor members from the old hierarchy'
+            end
+          end
+
+          context 'when a group that has existing group share is transferred' do
+            let(:shared_with_group) { group }
+
+            let_it_be(:member_of_shared_with_group) { create(:user) }
+            let_it_be(:shared_group) { create(:group, :private) }
+            let_it_be(:project_in_shared_group) { create(:project, namespace: shared_group) }
+
+            before do
+              shared_with_group.add_developer(member_of_shared_with_group)
+              create(:group_group_link, :maintainer, shared_group: shared_group, shared_with_group: shared_with_group)
+              shared_with_group.refresh_members_authorized_projects
+            end
+
+            it 'retains the authorizations of direct members' do
+              expect { transfer_service.execute(new_parent_group) }.not_to change {
+                ProjectAuthorization.where(
+                  project: project_in_shared_group,
+                  user: member_of_shared_with_group,
+                  access_level: Gitlab::Access::DEVELOPER).size
+              }.from(1)
+            end
+          end
         end
       end