Merge branch 'id-move-approvals-check-in-service' into 'master'

Add validation checks for approvals services

See merge request gitlab-org/gitlab!36279

app/policies/merge_request_policy.rb

@@ -10,6 +10,10 @@ class MergeRequestPolicy < IssuablePolicy
   # it would not be safe to prevent :create_note there, since
   # note permissions are shared, and this would apply too broadly.
   rule { ~can?(:read_merge_request) }.prevent :create_note
+
+  rule { can?(:update_merge_request) }.policy do
+    enable :approve_merge_request
+  end
 end
 
 MergeRequestPolicy.prepend_if_ee('EE::MergeRequestPolicy')

app/services/merge_requests/approval_service.rb

@@ -3,19 +3,27 @@
 module MergeRequests
   class ApprovalService < MergeRequests::BaseService
     def execute(merge_request)
+      return unless can_be_approved?(merge_request)
+
       approval = merge_request.approvals.new(user: current_user)
 
-      return unless save_approval(approval)
+      return success unless save_approval(approval)
 
       reset_approvals_cache(merge_request)
       create_event(merge_request)
       create_approval_note(merge_request)
       mark_pending_todos_as_done(merge_request)
       execute_approval_hooks(merge_request, current_user)
+
+      success
     end
 
     private
 
+    def can_be_approved?(merge_request)
+      current_user.can?(:approve_merge_request, merge_request)
+    end
+
     def reset_approvals_cache(merge_request)
       merge_request.approvals.reset
     end

app/services/merge_requests/remove_approval_service.rb

@@ -4,6 +4,8 @@ module MergeRequests
   class RemoveApprovalService < MergeRequests::BaseService
     # rubocop: disable CodeReuse/ActiveRecord
     def execute(merge_request)
+      return unless approved_by_user?(merge_request)
+
       # paranoid protection against running wrong deletes
       return unless merge_request.id && current_user.id
 
@@ -15,11 +17,17 @@ module MergeRequests
         reset_approvals_cache(merge_request)
         create_note(merge_request)
       end
+
+      success
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
     private
 
+    def approved_by_user?(merge_request)
+      merge_request.approved_by_users.include?(current_user)
+    end
+
     def reset_approvals_cache(merge_request)
       merge_request.approvals.reset
     end

ee/app/policies/ee/merge_request_policy.rb

@@ -13,7 +13,6 @@ module EE
       rule { ~can_override_approvers }.prevent :update_approvers
       rule { can?(:update_merge_request) }.policy do
         enable :update_approvers
-        enable :approve_merge_request
       end
     end
   end

ee/app/services/ee/merge_requests/approval_service.rb

@@ -9,15 +9,18 @@ module EE
 
       override :execute
       def execute(merge_request)
-        if incorrect_approval_password?(merge_request)
-          raise IncorrectApprovalPasswordError
-        end
+        return if incorrect_approval_password?(merge_request)
 
         super
       end
 
       private
 
+      override :can_be_approved?
+      def can_be_approved?(merge_request)
+        merge_request.can_approve?(current_user)
+      end
+
       override :reset_approvals_cache
       def reset_approvals_cache(merge_request)
         merge_request.reset_approval_cache!

ee/app/services/ee/merge_requests/remove_approval_service.rb

@@ -7,6 +7,11 @@ module EE
 
       private
 
+      override :approved_by_user?
+      def approved_by_user?(merge_request)
+        merge_request.has_approved?(current_user)
+      end
+
       override :reset_approvals_cache
       def reset_approvals_cache(merge_request)
         merge_request.reset_approval_cache!

ee/lib/api/merge_request_approvals.rb

@@ -144,17 +144,15 @@ module API
         post 'approve' do
           merge_request = find_project_merge_request(params[:merge_request_iid])
 
-          unauthorized! unless merge_request.can_approve?(current_user)
-
           check_sha_param!(params, merge_request)
 
-          begin
+          success =
             ::MergeRequests::ApprovalService
               .new(user_project, current_user, params)
               .execute(merge_request)
-          rescue ::MergeRequests::ApprovalService::IncorrectApprovalPasswordError
-            unauthorized!
-          end
+
+          unauthorized! unless success
+
           present_approval(merge_request)
         end
 
@@ -164,12 +162,12 @@ module API
         post 'unapprove' do
           merge_request = find_project_merge_request(params[:merge_request_iid])
 
-          not_found! unless merge_request.has_approved?(current_user)
-
-          ::MergeRequests::RemoveApprovalService
+          success = ::MergeRequests::RemoveApprovalService
             .new(user_project, current_user)
             .execute(merge_request)
 
+          not_found! unless success
+
           present_approval(merge_request)
         end
       end

ee/spec/services/merge_requests/approval_service_spec.rb

@@ -11,6 +11,10 @@ RSpec.describe MergeRequests::ApprovalService do
 
     subject(:service) { described_class.new(project, user) }
 
+    before do
+      project.add_developer(user)
+    end
+
     context 'with invalid approval' do
       before do
         allow(merge_request.approvals).to receive(:new).and_return(double(save: false))
@@ -133,8 +137,8 @@ RSpec.describe MergeRequests::ApprovalService do
         user.update(password: 'password')
       end
       context 'when password not specified' do
-        it 'raises an error' do
-          expect { service.execute(merge_request) }.to raise_error(::MergeRequests::ApprovalService::IncorrectApprovalPasswordError)
+        it 'does not update the approvals' do
+          expect { service.execute(merge_request) }.not_to change { merge_request.approvals.size }
         end
       end
 
@@ -143,9 +147,10 @@ RSpec.describe MergeRequests::ApprovalService do
           { approval_password: 'incorrect' }
         end
 
-        it 'raises an error' do
+        it 'does not update the approvals' do
           service_with_params = described_class.new(project, user, params)
-          expect { service_with_params.execute(merge_request) }.to raise_error(::MergeRequests::ApprovalService::IncorrectApprovalPasswordError)
+
+          expect { service_with_params.execute(merge_request) }.not_to change { merge_request.approvals.size }
         end
       end
 
@@ -154,9 +159,10 @@ RSpec.describe MergeRequests::ApprovalService do
           { approval_password: 'password' }
         end
 
-        it 'does not raise an error' do
+        it 'approves the merge request' do
           service_with_params = described_class.new(project, user, params)
-          expect { service_with_params.execute(merge_request) }.not_to raise_error(::MergeRequests::ApprovalService::IncorrectApprovalPasswordError)
+
+          expect { service_with_params.execute(merge_request) }.to change { merge_request.approvals.size }
         end
       end
     end

spec/policies/merge_request_policy_spec.rb

@@ -24,6 +24,7 @@ RSpec.describe MergeRequestPolicy do
   mr_perms = %i[create_merge_request_in
                 create_merge_request_from
                 read_merge_request
+                approve_merge_request
                 create_note].freeze
 
   shared_examples_for 'a denied user' do

spec/services/merge_requests/approval_service_spec.rb

@@ -11,6 +11,10 @@ RSpec.describe MergeRequests::ApprovalService do
 
     subject(:service) { described_class.new(project, user) }
 
+    before do
+      project.add_developer(user)
+    end
+
     context 'with invalid approval' do
       before do
         allow(merge_request.approvals).to receive(:new).and_return(double(save: false))
@@ -56,5 +60,15 @@ RSpec.describe MergeRequests::ApprovalService do
         end
       end
     end
+
+    context 'user cannot update the merge request' do
+      before do
+        project.add_guest(user)
+      end
+
+      it 'does not update approvals' do
+        expect { service.execute(merge_request) }.not_to change { merge_request.approvals.size }
+      end
+    end
   end
 end

spec/services/merge_requests/remove_approval_service_spec.rb

@@ -33,5 +33,14 @@ RSpec.describe MergeRequests::RemoveApprovalService do
         execute!
       end
     end
+
+    context 'with a user who has not approved' do
+      it 'does not create an unapproval note and triggers web hook' do
+        expect(service).not_to receive(:execute_hooks)
+        expect(SystemNoteService).not_to receive(:unapprove_mr)
+
+        execute!
+      end
+    end
   end
 end