Merge branch...

Merge branch '208425-user-without-parent-group-permission-cannot-assign-epic-to-issue' into 'master'

Change permissions to set epic of an issue

See merge request gitlab-org/gitlab!66865

ee/app/graphql/mutations/issues/set_epic.rb

@@ -16,11 +16,13 @@ module Mutations
         issue = authorized_find!(project_path: project_path, iid: iid)
         project = issue.project
 
-        authorize_admin_rights!(epic)
+        authorize_read_rights!(epic)
 
         begin
           ::Issues::UpdateService.new(project: project, current_user: current_user, params: { epic: epic })
             .execute(issue)
+        rescue ::Gitlab::Access::AccessDeniedError
+          raise_resource_not_available_error!
         rescue EE::Issues::BaseService::EpicAssignmentError => error
           issue.errors.add(:base, error.message)
         end
@@ -33,10 +35,10 @@ module Mutations
 
       private
 
-      def authorize_admin_rights!(epic)
+      def authorize_read_rights!(epic)
         return unless epic.present?
 
-        raise_resource_not_available_error! unless Ability.allowed?(current_user, :admin_epic, epic.group)
+        raise_resource_not_available_error! unless Ability.allowed?(current_user, :read_epic, epic.group)
       end
     end
   end

ee/app/models/ee/issue.rb

@@ -150,7 +150,7 @@ module EE
     end
 
     def can_assign_epic?(user)
-      user&.can?(:admin_epic, project.group)
+      user&.can?(:read_epic, project.group) && user&.can?(:admin_issue, project)
     end
 
     def can_be_promoted_to_epic?(user, group = nil)

ee/app/services/ee/issues/base_service.rb

@@ -52,7 +52,7 @@ module EE
 
         return unless epic
 
-        unless can?(current_user, :admin_epic, epic)
+        unless can?(current_user, :read_epic, epic) && can?(current_user, :admin_issue, issue)
           raise ::Gitlab::Access::AccessDeniedError
         end
 

ee/app/services/epic_issues/create_service.rb

@@ -35,16 +35,25 @@ module EpicIssues
 
     def linkable_issuables(issues)
       @linkable_issues ||= begin
-        return [] unless can?(current_user, :admin_epic, issuable.group)
+        return [] unless can?(current_user, :read_epic, issuable.group)
+
+        Preloaders::UserMaxAccessLevelInProjectsPreloader
+          .new(issues.map(&:project).compact, current_user)
+          .execute
 
         issues.select do |issue|
-          issue.supports_epic? &&
-            issuable_group_descendants.include?(issue.project.group) &&
-            !previous_related_issuables.include?(issue)
+          linkable_issue?(issue)
         end
       end
     end
 
+    def linkable_issue?(issue)
+      issue.supports_epic? &&
+        can?(current_user, :admin_issue, issue.project) &&
+        issuable_group_descendants.include?(issue.project.group) &&
+        !previous_related_issuables.include?(issue)
+    end
+
     def previous_related_issuables
       @related_issues ||= issuable.issues.to_a
     end

ee/app/services/epic_issues/destroy_service.rb

@@ -27,7 +27,7 @@ module EpicIssues
     end
 
     def permission_to_remove_relation?
-      can?(current_user, :admin_epic_issue, target) && can?(current_user, :admin_epic, source)
+      can?(current_user, :admin_epic_issue, target) && can?(current_user, :read_epic, source)
     end
 
     def create_notes

ee/lib/api/epic_issues.rb

@@ -23,6 +23,10 @@ module API
           .with_api_entity_associations
           .sorted_by_epic_position
       end
+
+      def authorize_can_assign_to_epic!(issue)
+        forbidden! unless can?(current_user, :read_epic, epic) && can?(current_user, :admin_issue, issue)
+      end
     end
 
     params do
@@ -40,7 +44,7 @@ module API
         use :pagination
       end
       put ':id/(-/)epics/:epic_iid/issues/:epic_issue_id' do
-        authorize_can_admin_epic!
+        authorize_can_assign_to_epic!(link.issue)
 
         update_params = {
           move_before_id: params[:move_before_id],
@@ -84,9 +88,9 @@ module API
       end
       # rubocop: disable CodeReuse/ActiveRecord
       post ':id/(-/)epics/:epic_iid/issues/:issue_id' do
-        authorize_can_admin_epic!
-
+        authorize_can_read!
         issue = Issue.find(params[:issue_id])
+        authorize!(:admin_issue, issue)
 
         create_params = { target_issuable: issue }
 
@@ -110,8 +114,7 @@ module API
         requires :epic_issue_id, type: Integer, desc: 'The ID of the association'
       end
       delete ':id/(-/)epics/:epic_iid/issues/:epic_issue_id' do
-        authorize_can_admin_epic!
-
+        authorize_can_assign_to_epic!(link.issue)
         result = ::EpicIssues::DestroyService.new(link, current_user).execute
 
         if result[:status] == :success

ee/spec/graphql/ee/mutations/boards/issues/issue_move_list_spec.rb

@@ -24,7 +24,7 @@ RSpec.describe Mutations::Boards::Issues::IssueMoveList do
 
   before do
     stub_licensed_features(epics: true)
-    project.add_maintainer(user)
+    project.add_reporter(user)
   end
 
   subject do
@@ -34,7 +34,7 @@ RSpec.describe Mutations::Boards::Issues::IssueMoveList do
   describe '#resolve' do
     context 'when user has access to the epic' do
       before do
-        group.add_developer(user)
+        group.add_guest(user)
       end
 
       it 'moves and repositions issue' do
@@ -47,6 +47,8 @@ RSpec.describe Mutations::Boards::Issues::IssueMoveList do
     end
 
     context 'when user does not have access to the epic' do
+      let(:epic) { create(:epic, :confidential, group: group) }
+
       it 'does not update issue' do
         subject
 

ee/spec/graphql/mutations/issues/set_epic_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe Mutations::Issues::SetEpic do
-  let_it_be(:group) { create(:group) }
+  let_it_be(:group) { create(:group, :public) }
   let_it_be(:project) { create(:project, group: group) }
   let_it_be(:user) { create(:user) }
   let_it_be_with_reload(:issue) { create(:issue, project: project) }
@@ -12,6 +12,7 @@ RSpec.describe Mutations::Issues::SetEpic do
 
   describe '#resolve' do
     let_it_be_with_reload(:epic) { create(:epic, group: group) }
+    let_it_be_with_reload(:confidential_epic) { create(:epic, group: group, confidential: true) }
 
     let(:mutated_issue) { subject[:issue] }
 
@@ -22,18 +23,11 @@ RSpec.describe Mutations::Issues::SetEpic do
     context 'when the user can update the issue' do
       before do
         stub_licensed_features(epics: true)
-        project.add_developer(user)
+        project.add_reporter(user)
+        group.add_guest(user)
       end
 
-      it 'raises an error if the epic is not accessible to the user' do
-        expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
-      end
-
-      context 'when user can admin epic' do
-        before do
-          group.add_owner(user)
-        end
-
+      context 'when user can read epic' do
         it 'returns the issue with the epic' do
           expect(mutated_issue).to eq(issue)
           expect(mutated_issue.epic).to eq(epic)
@@ -61,12 +55,32 @@ RSpec.describe Mutations::Issues::SetEpic do
         end
 
         context 'when epic is confidential but issue is public' do
-          let(:epic) { create(:epic, group: group, confidential: true) }
+          let(:epic) { confidential_epic }
 
           it 'returns an error with appropriate message' do
+            group.add_reporter(user)
+
             expect(subject[:errors].first).to include("Cannot assign a confidential epic to a non-confidential issue. Make the issue confidential and try again")
           end
         end
+
+        context 'with assigning epic error' do
+          let(:mock_service) { double('service', execute: { status: :error, message: 'failed to assign epic' }) }
+
+          it 'returns an error with appropriate message' do
+            expect(EpicIssues::CreateService).to receive(:new).and_return(mock_service)
+
+            expect(subject[:errors].first).to include('failed to assign epic')
+          end
+        end
+      end
+
+      context 'when user can not read epic' do
+        let(:epic) { confidential_epic }
+
+        it 'raises an error' do
+          expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
+        end
       end
     end
   end

ee/spec/graphql/mutations/issues/update_spec.rb

@@ -39,13 +39,12 @@ RSpec.describe Mutations::Issues::Update do
 
     before do
       group.clear_memoization(:feature_available)
-      group.add_developer(user)
+      project.add_reporter(user)
+      group.add_guest(user)
     end
 
     context 'when epics feature is disabled' do
       it 'raises an error' do
-        group.add_developer(user)
-
         expect { subject }.to raise_error(::Gitlab::Graphql::Errors::ResourceNotAvailable)
       end
     end
@@ -56,7 +55,7 @@ RSpec.describe Mutations::Issues::Update do
       end
 
       context 'for user without permissions' do
-        let(:current_user) { create(:user) }
+        let(:epic) { create(:epic, :confidential, group: group) }
 
         it 'raises an error' do
           expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)

ee/spec/models/issue_spec.rb

@@ -813,19 +813,11 @@ RSpec.describe Issue do
       end
 
       context 'when a user is a project member' do
-        it 'returns false' do
-          project.add_developer(user)
-
-          expect(subject).to be_falsey
+        before do
+          project.add_reporter(user)
         end
-      end
 
-      context 'when a user is a group member' do
-        it 'returns true' do
-          group.add_developer(user)
-
-          expect(subject).to be_truthy
-        end
+        it { is_expected.to be_truthy }
       end
     end
 

ee/spec/requests/api/epic_issues_spec.rb

@@ -102,6 +102,7 @@ RSpec.describe API::EpicIssues do
     context 'when epics feature is enabled' do
       before do
         stub_licensed_features(epics: true)
+        group.add_guest(user)
       end
 
       context 'when an error occurs' do
@@ -117,13 +118,33 @@ RSpec.describe API::EpicIssues do
 
           post api(url, user)
 
-          expect(response).to have_gitlab_http_status(:not_found)
+          expect(response).to have_gitlab_http_status(:forbidden)
         end
 
-        it 'returns 403 forbidden error for a user without permissions to admin the epic' do
-          post api(url, user)
+        context 'without permissions to admin the issue' do
+          before do
+            project.add_guest(user)
+          end
 
-          expect(response).to have_gitlab_http_status(:forbidden)
+          it 'returns 403 forbidden error' do
+            post api(url, user)
+
+            expect(response).to have_gitlab_http_status(:forbidden)
+          end
+        end
+
+        context 'without permissions to read the epic' do
+          let(:epic) { create(:epic, :confidential, group: create(:group, :private)) }
+
+          before do
+            project.add_reporter(user)
+          end
+
+          it 'returns 403 forbidden error' do
+            post api(url, user)
+
+            expect(response).to have_gitlab_http_status(:forbidden)
+          end
         end
 
         context 'when issue project is not under the epic group' do
@@ -146,7 +167,7 @@ RSpec.describe API::EpicIssues do
 
       context 'when the request is correct' do
         before do
-          group.add_developer(user)
+          project.add_reporter(user)
 
           post api(url, user)
         end
@@ -205,10 +226,29 @@ RSpec.describe API::EpicIssues do
           expect(response).to have_gitlab_http_status(:not_found)
         end
 
-        it 'returns 403 forbidden error for a user without permissions to admin the epic' do
-          delete api(url, user)
+        context 'With user without permissions to admin the issue' do
+          before do
+            project.add_guest(user)
+          end
 
-          expect(response).to have_gitlab_http_status(:forbidden)
+          it 'returns 403 forbidden error' do
+            delete api(url, user)
+
+            expect(response).to have_gitlab_http_status(:forbidden)
+          end
+        end
+
+        context 'without permissions to read the epic' do
+          before do
+            [issue, epic].map { |issuable| issuable.update!(confidential: true) }
+            project.add_reporter(user)
+          end
+
+          it 'returns 403 forbidden error' do
+            delete api(url, user)
+
+            expect(response).to have_gitlab_http_status(:forbidden)
+          end
         end
 
         context 'when epic_issue association does not include the epic in the url' do

ee/spec/requests/api/graphql/mutations/boards/issues/issue_move_list_spec.rb

@@ -33,38 +33,19 @@ RSpec.describe 'Reposition and move issue within board lists' do
 
   before do
     stub_licensed_features(epics: true)
-    project.add_maintainer(user)
   end
 
-  context 'when user can admin epic' do
+  context 'when user can admin issue' do
     before do
-      group.add_maintainer(user)
+      project.add_reporter(user)
     end
 
-    it 'updates issue position and epic' do
-      post_graphql_mutation(mutation(params), current_user: user)
-
-      expect(response).to have_gitlab_http_status(:success)
-      response_issue = graphql_mutation_response(:issue_move_list)['issue']
-      expect(response_issue['iid']).to eq(issue1.iid.to_s)
-      expect(response_issue['relativePosition']).to be > existing_issue1.relative_position
-      expect(response_issue['relativePosition']).to be < existing_issue2.relative_position
-      expect(response_issue['epic']['id']).to eq(epic.to_global_id.to_s)
-      expect(response_issue['labels']['nodes'].first['title']).to eq(testing.title)
-    end
-
-    context 'when user sets nil epic' do
-      let_it_be(:epic_issue) { create(:epic_issue, issue: issue1, epic: epic) }
-
-      let(:issue_move_params) do
-        {
-          epic_id: nil,
-          move_before_id: existing_issue2.id,
-          move_after_id: existing_issue1.id
-        }
+    context 'when user can read epic' do
+      before do
+        group.add_guest(user)
       end
 
-      it 'updates issue position and epic is unassigned' do
+      it 'updates issue position and epic' do
         post_graphql_mutation(mutation(params), current_user: user)
 
         expect(response).to have_gitlab_http_status(:success)
@@ -72,19 +53,46 @@ RSpec.describe 'Reposition and move issue within board lists' do
         expect(response_issue['iid']).to eq(issue1.iid.to_s)
         expect(response_issue['relativePosition']).to be > existing_issue1.relative_position
         expect(response_issue['relativePosition']).to be < existing_issue2.relative_position
-        expect(response_issue['epic']).to be_nil
+        expect(response_issue['epic']['id']).to eq(epic.to_global_id.to_s)
+        expect(response_issue['labels']['nodes'].first['title']).to eq(testing.title)
+      end
+
+      context 'when user sets nil epic' do
+        let_it_be(:epic_issue) { create(:epic_issue, issue: issue1, epic: epic) }
+
+        let(:issue_move_params) do
+          {
+            epic_id: nil,
+            move_before_id: existing_issue2.id,
+            move_after_id: existing_issue1.id
+          }
+        end
+
+        it 'updates issue position and epic is unassigned' do
+          post_graphql_mutation(mutation(params), current_user: user)
+
+          expect(response).to have_gitlab_http_status(:success)
+          response_issue = graphql_mutation_response(:issue_move_list)['issue']
+          expect(response_issue['iid']).to eq(issue1.iid.to_s)
+          expect(response_issue['relativePosition']).to be > existing_issue1.relative_position
+          expect(response_issue['relativePosition']).to be < existing_issue2.relative_position
+          expect(response_issue['epic']).to be_nil
+        end
       end
     end
-  end
 
-  context 'when user can not admin epic' do
-    it 'fails with error' do
-      post_graphql_mutation(mutation(params), current_user: user)
+    context 'when user can not read epic' do
+      let(:confidential_epic) { create(:epic, :confidential, group: group) }
+
+      it 'does not set epic' do
+        params[:epic_id] = confidential_epic.to_global_id.to_s
+        post_graphql_mutation(mutation(params), current_user: user)
 
-      mutation_response = graphql_mutation_response(:issue_move_list)
-      expect(mutation_response['errors']).to eq(['You are not allowed to move the issue'])
-      expect(mutation_response['issue']['epic']).to eq(nil)
-      expect(mutation_response['issue']['relativePosition']).to eq(3)
+        response_issue = graphql_mutation_response(:issue_move_list)['issue']
+        expect(response_issue['epic']).to be_nil
+        expect(response_issue['relativePosition']).to eq(3)
+        expect(graphql_mutation_response(:issue_move_list)['errors']).to include('Resource not found')
+      end
     end
   end
 

ee/spec/requests/api/graphql/mutations/issues/update_spec.rb

@@ -23,7 +23,7 @@ RSpec.describe 'Update of an existing issue' do
 
   before do
     stub_licensed_features(issuable_health_status: true)
-    project.add_developer(current_user)
+    project.add_reporter(current_user)
   end
 
   it 'updates the issue' do
@@ -42,7 +42,7 @@ RSpec.describe 'Update of an existing issue' do
 
     before do
       stub_licensed_features(epics: true)
-      group.add_developer(current_user)
+      group.add_guest(current_user)
     end
 
     it 'sets the epic' do
@@ -56,7 +56,7 @@ RSpec.describe 'Update of an existing issue' do
     end
 
     context 'the epic is not readable to the current user' do
-      let(:epic) { create(:epic) }
+      let(:epic) { create(:epic, :confidential, group: group) }
 
       it 'does not set the epic' do
         post_graphql_mutation(mutation, current_user: current_user)
@@ -117,7 +117,7 @@ RSpec.describe 'Update of an existing issue' do
 
     before do
       stub_licensed_features(epics: true)
-      group.add_developer(current_user)
+      group.add_guest(current_user)
       issue.update!(epic: epic)
     end
 

ee/spec/services/ee/issues/update_service_spec.rb

@@ -252,6 +252,10 @@ RSpec.describe Issues::UpdateService do
       subject { update_issue(params) }
 
       context 'when a user does not have permissions to assign an epic' do
+        before do
+          project.add_guest(author)
+        end
+
         it 'raises an exception' do
           expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError)
         end
@@ -259,7 +263,7 @@ RSpec.describe Issues::UpdateService do
 
       context 'when a user has permissions to assign an epic' do
         before do
-          group.add_maintainer(user)
+          group.add_guest(user)
         end
 
         context 'when EpicIssues::CreateService returns failure', :aggregate_failures do

ee/spec/services/epic_issues/create_service_spec.rb

@@ -89,6 +89,11 @@ RSpec.describe EpicIssues::CreateService do
     context 'when epics feature is disabled' do
       subject { assign_issue([valid_reference]) }
 
+      before do
+        group.add_guest(user)
+        project.add_reporter(user)
+      end
+
       include_examples 'returns an error'
     end
 
@@ -99,7 +104,8 @@ RSpec.describe EpicIssues::CreateService do
 
       context 'when user has permissions to link the issue' do
         before do
-          group.add_developer(user)
+          group.add_guest(user)
+          project.add_reporter(user)
         end
 
         context 'when the reference list is empty' do
@@ -124,7 +130,7 @@ RSpec.describe EpicIssues::CreateService do
 
             include_examples 'returns success'
 
-            it 'does not perform N + 1 queries' do
+            it 'does not perform N + 1 queries', :request_store do
               allow(SystemNoteService).to receive(:epic_issue)
               allow(SystemNoteService).to receive(:issue_on_epic)
 
@@ -146,7 +152,7 @@ RSpec.describe EpicIssues::CreateService do
               project = create(:project, group: group)
               issues = create_list(:issue, 5, project: project)
               epic = create(:epic, group: group)
-              group.add_developer(user)
+              group.add_reporter(user)
 
               allow(extractor).to receive(:issues).and_return(issues)
               params = { issuable_references: issues.map { |i| i.to_reference(full: true) } }
@@ -173,6 +179,10 @@ RSpec.describe EpicIssues::CreateService do
 
             subject { assign_issue([Gitlab::Routing.url_helpers.namespace_project_issue_url(namespace_id: issue.project.namespace, project_id: issue.project, id: issue.iid)])}
 
+            before do
+              project2.add_reporter(user)
+            end
+
             include_examples 'returns success'
           end
 
@@ -196,6 +206,7 @@ RSpec.describe EpicIssues::CreateService do
 
               expect(created_link2.relative_position).to be < created_link1.relative_position
             end
+
             it 'orders the epic issues to the first place and moves the existing ones down' do
               existing_link = create(:epic_issue, epic: epic, issue: issue3)
 
@@ -258,7 +269,8 @@ RSpec.describe EpicIssues::CreateService do
 
       context 'when assigning issue(s) to the same epic' do
         before do
-          group.add_developer(user)
+          group.add_guest(user)
+          project.add_reporter(user)
           assign_issue([valid_reference])
           epic.reload
         end
@@ -288,7 +300,8 @@ RSpec.describe EpicIssues::CreateService do
 
       context 'when an issue is already assigned to another epic', :sidekiq_inline do
         before do
-          group.add_developer(user)
+          group.add_guest(user)
+          project.add_reporter(user)
           create(:epic_issue, epic: epic, issue: issue)
           issue.reload
         end
@@ -361,8 +374,8 @@ RSpec.describe EpicIssues::CreateService do
         let_it_be(:another_issue) { create :issue }
 
         before do
-          group.add_developer(user)
-          another_issue.project.add_developer(user)
+          group.add_guest(user)
+          another_issue.project.add_reporter(user)
         end
 
         include_examples 'returns an error'

ee/spec/services/epic_issues/destroy_service_spec.rb

@@ -15,7 +15,8 @@ RSpec.describe EpicIssues::DestroyService do
 
     context 'when epics feature is disabled' do
       before do
-        group.add_reporter(user)
+        project.add_reporter(user)
+        group.add_guest(user)
       end
 
       it 'returns an error' do
@@ -30,7 +31,8 @@ RSpec.describe EpicIssues::DestroyService do
 
       context 'when user has permissions to remove associations' do
         before do
-          group.add_reporter(user)
+          project.add_reporter(user)
+          group.add_guest(user)
         end
 
         it 'removes related issue' do
@@ -73,6 +75,14 @@ RSpec.describe EpicIssues::DestroyService do
 
           subject
         end
+
+        context 'refresh epic dates' do
+          it 'calls UpdateDatesService' do
+            expect(Epics::UpdateDatesService).to receive(:new).with([epic_issue.epic]).and_call_original
+
+            subject
+          end
+        end
       end
 
       context 'user does not have permissions to remove associations' do
@@ -90,16 +100,6 @@ RSpec.describe EpicIssues::DestroyService do
           subject
         end
       end
-
-      context 'refresh epic dates' do
-        it 'calls UpdateDatesService' do
-          group.add_reporter(user)
-
-          expect(Epics::UpdateDatesService).to receive(:new).with([epic_issue.epic]).and_call_original
-
-          subject
-        end
-      end
     end
   end
 end

ee/spec/support/shared_examples/services/issue_epic_shared_examples.rb

@@ -26,7 +26,7 @@ RSpec.shared_examples 'issue with epic_id parameter' do
 
   context 'when user can not add issues to the epic' do
     before do
-      project.add_maintainer(user)
+      project.add_guest(user)
     end
 
     let(:params) { { title: 'issue1', epic_id: epic.id } }