diff options
author | Robert Speicher <robert@gitlab.com> | 2017-01-03 21:03:13 +0300 |
---|---|---|
committer | Robert Speicher <rspeicher@gmail.com> | 2017-01-20 20:23:11 +0300 |
commit | b104115a179bbf57f5440bd3ebe15cb0581f892d (patch) | |
tree | c8edb74c0546ea3aa0994d3c2d48d76c9ec9a7c5 | |
parent | e0d7fb658936d64dc688ecb72e75b48f59a00287 (diff) |
Merge branch 'fix-api-mr-permissions' into 'security'
Ensure that only privileged users can access merge requests in the API
See merge request !2053
-rw-r--r-- | changelogs/unreleased/fix-api-mr-permissions.yml | 4 | ||||
-rw-r--r-- | lib/api/helpers.rb | 6 | ||||
-rw-r--r-- | lib/api/merge_request_diffs.rb | 8 | ||||
-rw-r--r-- | lib/api/merge_requests.rb | 25 | ||||
-rw-r--r-- | lib/api/subscriptions.rb | 4 | ||||
-rw-r--r-- | lib/api/todos.rb | 2 | ||||
-rw-r--r-- | spec/requests/api/merge_requests_spec.rb | 27 | ||||
-rw-r--r-- | spec/requests/api/todos_spec.rb | 15 |
8 files changed, 66 insertions, 25 deletions
diff --git a/changelogs/unreleased/fix-api-mr-permissions.yml b/changelogs/unreleased/fix-api-mr-permissions.yml new file mode 100644 index 00000000000..33b677b1f29 --- /dev/null +++ b/changelogs/unreleased/fix-api-mr-permissions.yml @@ -0,0 +1,4 @@ +--- +title: Don't allow project guests to subscribe to merge requests through the API +merge_request: +author: Robert Schilling diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 4be659fc20b..b8ba18706b7 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -85,6 +85,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 5d1fe22f2df..f3ddd448ff0 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -116,8 +116,8 @@ module API success Entities::MergeRequest end 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, project: user_project end @@ -125,8 +125,8 @@ module API success Entities::RepoCommit end 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 @@ -134,8 +134,8 @@ module API success Entities::MergeRequestChanges end 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 @@ -153,8 +153,7 @@ module API :remove_source_branch end put path do - merge_request = user_project.merge_requests.find(params.delete(:merge_request_id)) - authorize! :update_merge_request, merge_request + merge_request = find_merge_request_with_access(params.delete(:merge_request_id), :update_merge_request) 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? @@ -233,10 +232,7 @@ module API use :pagination end 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 @@ -248,8 +244,7 @@ module API requires :note, type: String, desc: 'The text of the comment' end post "#{path}/comments" do - 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], @@ -273,7 +268,7 @@ module API use :pagination end 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 10749b34004..e11d7537cc9 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 ed8f48aa1e3..9bd077263a7 100644 --- a/lib/api/todos.rb +++ b/lib/api/todos.rb @@ -5,7 +5,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) } } diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index f032d1b683d..eecc172eb95 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -627,6 +627,15 @@ describe API::MergeRequests, api: true do expect(json_response.first['title']).to eq(issue.title) expect(json_response.first['id']).to eq(issue.id) end + + it 'returns 403 if the user has no access to the merge request' do + 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 describe 'POST :id/merge_requests/:merge_request_id/subscription' do @@ -648,6 +657,15 @@ describe API::MergeRequests, api: true do expect(response).to have_http_status(404) 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 describe 'DELETE :id/merge_requests/:merge_request_id/subscription' do @@ -669,6 +687,15 @@ describe API::MergeRequests, api: true do expect(response).to have_http_status(404) 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 def mr_with_later_created_and_updated_at_time diff --git a/spec/requests/api/todos_spec.rb b/spec/requests/api/todos_spec.rb index 887a2ba5b84..e8248086df6 100644 --- a/spec/requests/api/todos_spec.rb +++ b/spec/requests/api/todos_spec.rb @@ -183,12 +183,25 @@ describe API::Todos, api: true do expect(response.status).to eq(404) 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 describe 'POST :id/issuable_type/:issueable_id/todo' do context 'for an issue' 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 |