Fix permissions in EpicIssues::DestroyService

- Fix tests

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

@@ -17,7 +17,7 @@ module EE
 
         def resolve(**args)
           super
-        rescue ::Gitlab::Access::AccessDeniedError, ::Issues::BaseService::EpicAssignmentError
+        rescue ::Gitlab::Access::AccessDeniedError
           raise_resource_not_available_error!
         end
       end

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_access!(issue, 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,11 +35,10 @@ module Mutations
 
       private
 
-      def authorize_access!(issue, epic)
-        return unless issue.present? && epic.present?
+      def authorize_read_rights!(epic)
+        return unless epic.present?
 
-        raise_resource_not_available_error! unless Ability.allowed?(current_user, :admin_issue, issue) &&
-          Ability.allowed?(current_user, :read_epic, epic.group)
+        raise_resource_not_available_error! unless Ability.allowed?(current_user, :read_epic, epic.group)
       end
     end
   end

ee/app/services/epic_issues/create_service.rb

@@ -34,9 +34,9 @@ module EpicIssues
     end
 
     def linkable_issuables(issues)
-      return [] unless can?(current_user, :read_epic, issuable.group)
-
       @linkable_issues ||= begin
+        return [] unless can?(current_user, :read_epic, issuable.group)
+
         issues.select do |issue|
           linkable_issue?(issue)
         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

@@ -88,8 +88,9 @@ module API
       end
       # rubocop: disable CodeReuse/ActiveRecord
       post ':id/(-/)epics/:epic_iid/issues/:issue_id' do
+        authorize_can_read!
         issue = Issue.find(params[:issue_id])
-        authorize_can_assign_to_epic!(issue)
+        authorize!(:admin_issue, issue)
 
         create_params = { target_issuable: issue }
 

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
@@ -32,12 +32,29 @@ RSpec.describe Mutations::Boards::Issues::IssueMoveList do
   end
 
   describe '#resolve' do
-    it 'moves and repositions issue and sets epic' do
-      subject
+    context 'when user has access to the epic' do
+      before do
+        group.add_guest(user)
+      end
+
+      it 'moves and repositions issue' do
+        subject
 
-      expect(issue1.reload.epic).to eq(epic)
-      expect(issue1.relative_position).to be < existing_issue2.relative_position
-      expect(issue1.relative_position).to be > existing_issue1.relative_position
+        expect(issue1.reload.epic).to eq(epic)
+        expect(issue1.relative_position).to be < existing_issue2.relative_position
+        expect(issue1.relative_position).to be > existing_issue1.relative_position
+      end
+    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
+
+        expect(issue1.reload.epic).to be_nil
+        expect(issue1.relative_position).to eq(3)
+      end
     end
 
     context 'when user cannot be assigned to issue' do

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_guest(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,7 +39,8 @@ 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
@@ -54,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)
@@ -88,14 +89,6 @@ RSpec.describe Mutations::Issues::Update do
             expect(mutated_issue.epic).to be_nil
           end
         end
-
-        context 'the epic belongs to an external group' do
-          let(:epic) { create(:epic) }
-
-          it 'does not set the epic' do
-            expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
-          end
-        end
       end
     end
   end

ee/spec/models/issue_spec.rb

@@ -813,16 +813,19 @@ 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_truthy
+        before do
+          project.add_reporter(user)
         end
-      end
 
-      context 'when a user is not a group member' do
-        it 'returns false' do
-          expect(subject).to be_falsey
+        it { is_expected.to be_truthy }
+
+        context 'when a user can not read epic' do
+          before do
+            allow(Ability).to receive(:allowed?).with(user, :read_epic, group).and_return(false)
+            allow(Ability).to receive(:allowed?).with(user, :admin_issue, project).and_call_original
+          end
+
+          it { is_expected.to be_falsey }
         end
       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,7 +118,7 @@ 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
 
         context 'without permissions to admin the issue' do
@@ -133,10 +134,10 @@ RSpec.describe API::EpicIssues do
         end
 
         context 'without permissions to read the epic' do
-          let(:epic) { create(:epic, group: create(:group, :private)) }
+          let(:epic) { create(:epic, :confidential, group: create(:group, :private)) }
 
           before do
-            project.add_developer(user)
+            project.add_reporter(user)
           end
 
           it 'returns 403 forbidden error' do
@@ -166,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
@@ -237,6 +238,19 @@ RSpec.describe API::EpicIssues do
           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
           before do
             other_group = create(:group)

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

@@ -37,8 +37,8 @@ RSpec.describe 'Reposition and move issue within board lists' do
 
   context 'when user can admin issue' do
     before do
-      project.add_maintainer(user)
-      group.add_maintainer(user)
+      project.add_reporter(user)
+      group.add_guest(user)
     end
 
     it 'updates issue position and epic' do
@@ -75,6 +75,18 @@ RSpec.describe 'Reposition and move issue within board lists' do
         expect(response_issue['epic']).to be_nil
       end
     end
+
+    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)
+
+        response_issue = graphql_mutation_response(:issue_move_list)['issue']
+        expect(response_issue['epic']).to be_nil
+      end
+    end
   end
 
   def mutation(additional_params = {})

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

@@ -23,116 +23,123 @@ RSpec.describe 'Update of an existing issue' do
 
   before do
     stub_licensed_features(issuable_health_status: true)
+    project.add_reporter(current_user)
   end
 
-  context 'when user can update issue' do
+  it 'updates the issue' do
+    post_graphql_mutation(mutation, current_user: current_user)
+
+    expect(response).to have_gitlab_http_status(:success)
+    expect(mutated_issue).to include(input)
+  end
+
+  context 'setting epic' do
+    let(:epic) { create(:epic, group: group) }
+
+    let(:input) do
+      { iid: issue.iid.to_s, epic_id: global_id_of(epic) }
+    end
+
     before do
-      project.add_developer(current_user)
+      stub_licensed_features(epics: true)
+      group.add_guest(current_user)
     end
 
-    it 'updates the issue' do
+    it 'sets the epic' do
       post_graphql_mutation(mutation, current_user: current_user)
 
       expect(response).to have_gitlab_http_status(:success)
-      expect(mutated_issue).to include(input)
+      expect(graphql_errors).to be_blank
+      expect(mutated_issue).to include(
+        'epic' => include('id' => global_id_of(epic))
+      )
     end
 
-    context 'setting epic' do
-      let(:epic) { create(:epic, group: group) }
+    context 'the epic is not readable to the current user' do
+      let(:epic) { create(:epic, :confidential, group: group) }
 
-      let(:input) do
-        { iid: issue.iid.to_s, epic_id: global_id_of(epic) }
-      end
+      it 'does not set the epic' do
+        post_graphql_mutation(mutation, current_user: current_user)
 
-      before do
-        stub_licensed_features(epics: true)
+        expect(response).to have_gitlab_http_status(:success)
+        expect(graphql_errors).not_to be_blank
       end
+    end
+
+    context 'the epic is not an epic' do
+      let(:epic) { create(:user) }
 
-      it 'sets the epic' do
+      it 'does not set the epic' do
         post_graphql_mutation(mutation, current_user: current_user)
 
         expect(response).to have_gitlab_http_status(:success)
-        expect(graphql_errors).to be_blank
-        expect(mutated_issue).to include(
-          'epic' => include('id' => global_id_of(epic))
-        )
+        expect(graphql_errors).not_to be_blank
       end
+    end
 
-      context 'the epic is not an epic' do
-        let(:epic) { create(:user) }
+    # TODO: remove when the global-ID compatibility layer is removed,
+    # at which point this error becomes unreachable because the query will
+    # be rejected as ill-typed.
+    context 'the epic is not an epic, using the ID type' do
+      let(:mutation) do
+        query = <<~GQL
+        mutation($epic: ID!, $path: ID!, $iid: String!) {
+          updateIssue(input: { epicId: $epic, projectPath: $path, iid: $iid }) {
+            errors
+          }
+        }
+        GQL
+
+        ::GraphqlHelpers::MutationDefinition.new(query, {
+          epic: global_id_of(create(:user)),
+          iid: issue.iid.to_s,
+          path: project.full_path
+        })
+      end
 
-        it 'does not set the epic' do
-          post_graphql_mutation(mutation, current_user: current_user)
+      it 'does not set the epic' do
+        post_graphql_mutation(mutation, current_user: current_user)
 
-          expect(response).to have_gitlab_http_status(:success)
-          expect(graphql_errors).not_to be_blank
-        end
+        expect(response).to have_gitlab_http_status(:success)
+        expect(graphql_errors).to contain_exactly(
+          a_hash_including('message' => /No object found/)
+        )
       end
+    end
+  end
 
-      # TODO: remove when the global-ID compatibility layer is removed,
-      # at which point this error becomes unreachable because the query will
-      # be rejected as ill-typed.
-      context 'the epic is not an epic, using the ID type' do
-        let(:mutation) do
-          query = <<~GQL
-          mutation($epic: ID!, $path: ID!, $iid: String!) {
-            updateIssue(input: { epicId: $epic, projectPath: $path, iid: $iid }) {
-              errors
-            }
-          }
-          GQL
-
-          ::GraphqlHelpers::MutationDefinition.new(query, {
-            epic: global_id_of(create(:user)),
-            iid: issue.iid.to_s,
-            path: project.full_path
-          })
-        end
-
-        it 'does not set the epic' do
-          post_graphql_mutation(mutation, current_user: current_user)
-
-          expect(response).to have_gitlab_http_status(:success)
-          expect(graphql_errors).to contain_exactly(
-            a_hash_including('message' => /No object found/)
-          )
-        end
-      end
+  context 'removing epic' do
+    let(:epic) { create(:epic, group: group) }
+
+    let(:input) do
+      { iid: issue.iid.to_s, epic_id: nil }
     end
 
-    context 'removing epic' do
-      let(:epic) { create(:epic, group: group) }
+    before do
+      stub_licensed_features(epics: true)
+      group.add_guest(current_user)
+      issue.update!(epic: epic)
+    end
 
-      let(:input) do
-        { iid: issue.iid.to_s, epic_id: nil }
-      end
+    it 'removes the epic' do
+      post_graphql_mutation(mutation, current_user: current_user)
 
-      before do
-        stub_licensed_features(epics: true)
-        group.add_developer(current_user)
-        issue.update!(epic: epic)
+      expect(response).to have_gitlab_http_status(:success)
+      expect(graphql_errors).to be_blank
+      expect(mutated_issue).to include('epic' => be_nil)
+    end
+
+    context 'the epic argument is not provided' do
+      let(:input) do
+        { iid: issue.iid.to_s, weight: 1 }
       end
 
-      it 'removes the epic' do
+      it 'does not remove the epic' do
         post_graphql_mutation(mutation, current_user: current_user)
 
         expect(response).to have_gitlab_http_status(:success)
         expect(graphql_errors).to be_blank
-        expect(mutated_issue).to include('epic' => be_nil)
-      end
-
-      context 'the epic argument is not provided' do
-        let(:input) do
-          { iid: issue.iid.to_s, weight: 1 }
-        end
-
-        it 'does not remove the epic' do
-          post_graphql_mutation(mutation, current_user: current_user)
-
-          expect(response).to have_gitlab_http_status(:success)
-          expect(graphql_errors).to be_blank
-          expect(mutated_issue).to include('epic' => be_present)
-        end
+        expect(mutated_issue).to include('epic' => be_present)
       end
     end
   end

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

@@ -263,7 +263,7 @@ RSpec.describe Issues::UpdateService do
 
       context 'when a user has permissions to assign an epic' do
         before do
-          group.add_reporter(user)
+          group.add_guest(user)
         end
 
         context 'when EpicIssues::CreateService returns failure', :aggregate_failures do

ee/spec/services/epic_issues/create_service_spec.rb

@@ -90,7 +90,8 @@ RSpec.describe EpicIssues::CreateService do
       subject { assign_issue([valid_reference]) }
 
       before do
-        group.add_developer(user)
+        group.add_guest(user)
+        project.add_reporter(user)
       end
 
       include_examples 'returns an error'
@@ -103,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
@@ -150,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) } }
@@ -160,7 +162,7 @@ RSpec.describe EpicIssues::CreateService do
               # and we insert 5 issues instead of 1 which we do for control count
               expect { described_class.new(epic, user, params).execute }
                 .not_to exceed_query_limit(control_count)
-                .with_threshold(30)
+                .with_threshold(29)
             end
           end
 
@@ -262,7 +264,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
@@ -292,7 +295,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
@@ -365,8 +369,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