Merge branch 'fix-api-mr-permissions' into 'security'

Ensure that only privileged users can access merge requests in the API

See merge request !2053

5 files changed, 20 insertions, 28 deletions

diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 4edea99d0d5..a5f1d88bb23 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -97,6 +97,12 @@ module API
       IssuesFinder.new(current_user, project_id: user_project.id).find(id)
     end
 
+    def find_merge_request_with_access(id, access_level = :read_merge_request)
+      merge_request = user_project.merge_requests.find(id)
+      authorize! access_level, merge_request
+      merge_request
+    end
+
     def paginate(relation)
       relation.page(params[:page]).per(params[:per_page].to_i).tap do |data|
         add_pagination_headers(data)
diff --git a/lib/api/merge_request_diffs.rb b/lib/api/merge_request_diffs.rb
index 07435d78468..bc3d69f6904 100644
--- a/lib/api/merge_request_diffs.rb
+++ b/lib/api/merge_request_diffs.rb
@@ -15,10 +15,8 @@ module API
       end
 
       get ":id/merge_requests/:merge_request_id/versions" do
-        merge_request = user_project.merge_requests.
-          find(params[:merge_request_id])
+        merge_request = find_merge_request_with_access(params[:merge_request_id])
 
-        authorize! :read_merge_request, merge_request
         present merge_request.merge_request_diffs, with: Entities::MergeRequestDiff
       end
 
@@ -34,10 +32,8 @@ module API
       end
 
       get ":id/merge_requests/:merge_request_id/versions/:version_id" do
-        merge_request = user_project.merge_requests.
-          find(params[:merge_request_id])
+        merge_request = find_merge_request_with_access(params[:merge_request_id])
 
-        authorize! :read_merge_request, merge_request
         present merge_request.merge_request_diffs.find(params[:version_id]), with: Entities::MergeRequestDiffFull
       end
     end
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index 20bb9e73dc6..0cbfd517a42 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -121,9 +121,7 @@ module API
         #   GET /projects/:id/merge_requests/:merge_request_id
         #
         get path do
-          merge_request = user_project.merge_requests.find(params[:merge_request_id])
-
-          authorize! :read_merge_request, merge_request
+          merge_request = find_merge_request_with_access(params[:merge_request_id])
 
           present merge_request, with: Entities::MergeRequest, current_user: current_user
         end
@@ -138,9 +136,8 @@ module API
         #   GET /projects/:id/merge_requests/:merge_request_id/commits
         #
         get "#{path}/commits" do
-          merge_request = user_project.merge_requests.
-            find(params[:merge_request_id])
-          authorize! :read_merge_request, merge_request
+          merge_request = find_merge_request_with_access(params[:merge_request_id])
+
           present merge_request.commits, with: Entities::RepoCommit
         end
 
@@ -154,9 +151,8 @@ module API
         #   GET /projects/:id/merge_requests/:merge_request_id/changes
         #
         get "#{path}/changes" do
-          merge_request = user_project.merge_requests.
-            find(params[:merge_request_id])
-          authorize! :read_merge_request, merge_request
+          merge_request = find_merge_request_with_access(params[:merge_request_id])
+
           present merge_request, with: Entities::MergeRequestChanges, current_user: current_user
         end
 
@@ -174,8 +170,7 @@ module API
           optional :milestone_id, type: Integer, desc: 'The ID of the new milestone'
         end
         put path do
-          merge_request = user_project.merge_requests.find(params[:merge_request_id])
-          authorize! :update_merge_request, merge_request
+          merge_request = find_merge_request_with_access(params.delete(:merge_request_id), :update_merge_request)
 
           # Ensure source_branch is not specified
           if params[:source_branch].present?
@@ -262,10 +257,7 @@ module API
         #   GET /projects/:id/merge_requests/:merge_request_id/comments
         #
         get "#{path}/comments" do
-          merge_request = user_project.merge_requests.find(params[:merge_request_id])
-
-          authorize! :read_merge_request, merge_request
-
+          merge_request = find_merge_request_with_access(params[:merge_request_id])
           present paginate(merge_request.notes.fresh), with: Entities::MRNote
         end
 
@@ -284,9 +276,7 @@ module API
         post "#{path}/comments" do
           required_attributes! [:note]
 
-          merge_request = user_project.merge_requests.find(params[:merge_request_id])
-
-          authorize! :create_note, merge_request
+          merge_request = find_merge_request_with_access(params[:merge_request_id], :create_note)
 
           opts = {
             note: params[:note],
@@ -311,7 +301,7 @@ module API
         # Examples:
         #   GET /projects/:id/merge_requests/:merge_request_id/closes_issues
         get "#{path}/closes_issues" do
-          merge_request = user_project.merge_requests.find(params[:merge_request_id])
+          merge_request = find_merge_request_with_access(params[:merge_request_id])
           issues = ::Kaminari.paginate_array(merge_request.closes_issues(current_user))
           present paginate(issues), with: issue_entity(user_project), current_user: current_user
         end
diff --git a/lib/api/subscriptions.rb b/lib/api/subscriptions.rb
index c49e2a21b82..2ff272d41b3 100644
--- a/lib/api/subscriptions.rb
+++ b/lib/api/subscriptions.rb
@@ -3,8 +3,8 @@ module API
     before { authenticate! }
 
     subscribable_types = {
-      'merge_request' => proc { |id| user_project.merge_requests.find(id) },
-      'merge_requests' => proc { |id| user_project.merge_requests.find(id) },
+      'merge_request' => proc { |id| find_merge_request_with_access(id, :update_merge_request) },
+      'merge_requests' => proc { |id| find_merge_request_with_access(id, :update_merge_request) },
       'issues' => proc { |id| find_project_issue(id) },
       'labels' => proc { |id| find_project_label(id) },
     }
diff --git a/lib/api/todos.rb b/lib/api/todos.rb
index 832b04a3bb1..86d79d60247 100644
--- a/lib/api/todos.rb
+++ b/lib/api/todos.rb
@@ -4,7 +4,7 @@ module API
     before { authenticate! }
 
     ISSUABLE_TYPES = {
-      'merge_requests' => ->(id) { user_project.merge_requests.find(id) },
+      'merge_requests' => ->(id) { find_merge_request_with_access(id) },
       'issues' => ->(id) { find_project_issue(id) }
     }