Commit 3a5df1d8 authored by Robert Speicher's avatar Robert Speicher Committed by Robert Speicher

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

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

See merge request !2053
parent d7755ede
---
title: Don't allow project guests to subscribe to merge requests through the API
merge_request:
author: Robert Schilling
...@@ -90,6 +90,12 @@ module API ...@@ -90,6 +90,12 @@ module API
MergeRequestsFinder.new(current_user, project_id: user_project.id).find(id) MergeRequestsFinder.new(current_user, project_id: user_project.id).find(id)
end 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 authenticate! def authenticate!
unauthorized! unless current_user unauthorized! unless current_user
end end
......
...@@ -15,10 +15,8 @@ module API ...@@ -15,10 +15,8 @@ module API
end end
get ":id/merge_requests/:merge_request_id/versions" do get ":id/merge_requests/:merge_request_id/versions" do
merge_request = user_project.merge_requests. merge_request = find_merge_request_with_access(params[:merge_request_id])
find(params[:merge_request_id])
authorize! :read_merge_request, merge_request
present merge_request.merge_request_diffs, with: Entities::MergeRequestDiff present merge_request.merge_request_diffs, with: Entities::MergeRequestDiff
end end
...@@ -34,10 +32,8 @@ module API ...@@ -34,10 +32,8 @@ module API
end end
get ":id/merge_requests/:merge_request_id/versions/:version_id" do get ":id/merge_requests/:merge_request_id/versions/:version_id" do
merge_request = user_project.merge_requests. merge_request = find_merge_request_with_access(params[:merge_request_id])
find(params[:merge_request_id])
authorize! :read_merge_request, merge_request
present merge_request.merge_request_diffs.find(params[:version_id]), with: Entities::MergeRequestDiffFull present merge_request.merge_request_diffs.find(params[:version_id]), with: Entities::MergeRequestDiffFull
end end
end end
......
...@@ -118,8 +118,8 @@ module API ...@@ -118,8 +118,8 @@ module API
success Entities::MergeRequest success Entities::MergeRequest
end end
get path do get path do
merge_request = find_project_merge_request(params[:merge_request_id]) merge_request = find_merge_request_with_access(params[:merge_request_id])
authorize! :read_merge_request, merge_request
present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project
end end
...@@ -127,8 +127,8 @@ module API ...@@ -127,8 +127,8 @@ module API
success Entities::RepoCommit success Entities::RepoCommit
end end
get "#{path}/commits" do get "#{path}/commits" do
merge_request = find_project_merge_request(params[:merge_request_id]) merge_request = find_merge_request_with_access(params[:merge_request_id])
authorize! :read_merge_request, merge_request
present merge_request.commits, with: Entities::RepoCommit present merge_request.commits, with: Entities::RepoCommit
end end
...@@ -136,8 +136,8 @@ module API ...@@ -136,8 +136,8 @@ module API
success Entities::MergeRequestChanges success Entities::MergeRequestChanges
end end
get "#{path}/changes" do get "#{path}/changes" do
merge_request = find_project_merge_request(params[:merge_request_id]) merge_request = find_merge_request_with_access(params[:merge_request_id])
authorize! :read_merge_request, merge_request
present merge_request, with: Entities::MergeRequestChanges, current_user: current_user present merge_request, with: Entities::MergeRequestChanges, current_user: current_user
end end
...@@ -155,8 +155,7 @@ module API ...@@ -155,8 +155,7 @@ module API
:remove_source_branch :remove_source_branch
end end
put path do put path do
merge_request = find_project_merge_request(params.delete(:merge_request_id)) merge_request = find_merge_request_with_access(params.delete(:merge_request_id), :update_merge_request)
authorize! :update_merge_request, merge_request
mr_params = declared_params(include_missing: false) mr_params = declared_params(include_missing: false)
mr_params[:force_remove_source_branch] = mr_params.delete(:remove_source_branch) if mr_params[:remove_source_branch].present? mr_params[:force_remove_source_branch] = mr_params.delete(:remove_source_branch) if mr_params[:remove_source_branch].present?
...@@ -235,10 +234,7 @@ module API ...@@ -235,10 +234,7 @@ module API
use :pagination use :pagination
end end
get "#{path}/comments" do get "#{path}/comments" do
merge_request = find_project_merge_request(params[:merge_request_id]) merge_request = find_merge_request_with_access(params[:merge_request_id])
authorize! :read_merge_request, merge_request
present paginate(merge_request.notes.fresh), with: Entities::MRNote present paginate(merge_request.notes.fresh), with: Entities::MRNote
end end
...@@ -250,8 +246,7 @@ module API ...@@ -250,8 +246,7 @@ module API
requires :note, type: String, desc: 'The text of the comment' requires :note, type: String, desc: 'The text of the comment'
end end
post "#{path}/comments" do post "#{path}/comments" do
merge_request = find_project_merge_request(params[:merge_request_id]) merge_request = find_merge_request_with_access(params[:merge_request_id], :create_note)
authorize! :create_note, merge_request
opts = { opts = {
note: params[:note], note: params[:note],
...@@ -275,7 +270,7 @@ module API ...@@ -275,7 +270,7 @@ module API
use :pagination use :pagination
end end
get "#{path}/closes_issues" do get "#{path}/closes_issues" do
merge_request = find_project_merge_request(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)) issues = ::Kaminari.paginate_array(merge_request.closes_issues(current_user))
present paginate(issues), with: issue_entity(user_project), current_user: current_user present paginate(issues), with: issue_entity(user_project), current_user: current_user
end end
......
...@@ -3,8 +3,8 @@ module API ...@@ -3,8 +3,8 @@ module API
before { authenticate! } before { authenticate! }
subscribable_types = { subscribable_types = {
'merge_request' => 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| user_project.merge_requests.find(id) }, 'merge_requests' => proc { |id| find_merge_request_with_access(id, :update_merge_request) },
'issues' => proc { |id| find_project_issue(id) }, 'issues' => proc { |id| find_project_issue(id) },
'labels' => proc { |id| find_project_label(id) }, 'labels' => proc { |id| find_project_label(id) },
} }
......
...@@ -5,7 +5,7 @@ module API ...@@ -5,7 +5,7 @@ module API
before { authenticate! } before { authenticate! }
ISSUABLE_TYPES = { 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) } 'issues' => ->(id) { find_project_issue(id) }
} }
......
...@@ -627,6 +627,17 @@ describe API::MergeRequests, api: true do ...@@ -627,6 +627,17 @@ describe API::MergeRequests, api: true do
expect(json_response.first['title']).to eq(issue.title) expect(json_response.first['title']).to eq(issue.title)
expect(json_response.first['id']).to eq(issue.id) expect(json_response.first['id']).to eq(issue.id)
end end
it 'returns 403 if the user has no access to the merge request' do
project = create(:empty_project, :private)
merge_request = create(:merge_request, :simple, source_project: project)
guest = create(:user)
project.team << [guest, :guest]
get api("/projects/#{project.id}/merge_requests/#{merge_request.id}/closes_issues", guest)
expect(response).to have_http_status(403)
end
end end
describe 'POST :id/merge_requests/:merge_request_id/subscription' do describe 'POST :id/merge_requests/:merge_request_id/subscription' do
...@@ -648,6 +659,15 @@ describe API::MergeRequests, api: true do ...@@ -648,6 +659,15 @@ describe API::MergeRequests, api: true do
expect(response).to have_http_status(404) expect(response).to have_http_status(404)
end end
it 'returns 403 if user has no access to read code' do
guest = create(:user)
project.team << [guest, :guest]
post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/subscription", guest)
expect(response).to have_http_status(403)
end
end end
describe 'DELETE :id/merge_requests/:merge_request_id/subscription' do describe 'DELETE :id/merge_requests/:merge_request_id/subscription' do
...@@ -669,6 +689,15 @@ describe API::MergeRequests, api: true do ...@@ -669,6 +689,15 @@ describe API::MergeRequests, api: true do
expect(response).to have_http_status(404) expect(response).to have_http_status(404)
end end
it 'returns 403 if user has no access to read code' do
guest = create(:user)
project.team << [guest, :guest]
delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}/subscription", guest)
expect(response).to have_http_status(403)
end
end end
describe 'Time tracking' do describe 'Time tracking' do
......
...@@ -183,12 +183,25 @@ describe API::Todos, api: true do ...@@ -183,12 +183,25 @@ describe API::Todos, api: true do
expect(response.status).to eq(404) expect(response.status).to eq(404)
end end
it 'returns an error if the issuable is not accessible' do
guest = create(:user)
project_1.team << [guest, :guest]
post api("/projects/#{project_1.id}/#{issuable_type}/#{issuable.id}/todo", guest)
if issuable_type == 'merge_requests'
expect(response).to have_http_status(403)
else
expect(response).to have_http_status(404)
end
end
end end
describe 'POST :id/issuable_type/:issueable_id/todo' do describe 'POST :id/issuable_type/:issueable_id/todo' do
context 'for an issue' do context 'for an issue' do
it_behaves_like 'an issuable', 'issues' do it_behaves_like 'an issuable', 'issues' do
let(:issuable) { create(:issue, author: author_1, project: project_1) } let(:issuable) { create(:issue, :confidential, author: author_1, project: project_1) }
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment