diff options
author | Zeger-Jan van de Weg <zegerjan@gitlab.com> | 2016-03-18 19:11:25 +0100 |
---|---|---|
committer | Zeger-Jan van de Weg <zegerjan@gitlab.com> | 2016-03-19 21:24:05 +0100 |
commit | 1d7782281693a94b2d8efcdd9d05f81fefae75f9 (patch) | |
tree | c145ed956047668a69043ab1cc6946f307d1e996 | |
parent | 7342a4566cc2eef0e434f3aea0eac48674baaaf1 (diff) | |
download | gitlab-ce-1d7782281693a94b2d8efcdd9d05f81fefae75f9.tar.gz |
minor improvements and fixed specs
-rw-r--r-- | lib/api/helpers.rb | 4 | ||||
-rw-r--r-- | lib/api/issues.rb | 3 | ||||
-rw-r--r-- | lib/api/merge_requests.rb | 4 | ||||
-rw-r--r-- | spec/controllers/projects/merge_requests_controller_spec.rb | 2 | ||||
-rw-r--r-- | spec/requests/api/issues_spec.rb | 28 | ||||
-rw-r--r-- | spec/requests/api/merge_requests_spec.rb | 22 |
6 files changed, 34 insertions, 29 deletions
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index a72044e8058..4921ae99e78 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -118,9 +118,7 @@ module API end def authorize!(action, subject) - unless abilities.allowed?(current_user, action, subject) - forbidden! - end + forbidden! unless abilities.allowed?(current_user, action, subject) end def authorize_push_project diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 539f00d37fe..8c753e9f2ff 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -200,7 +200,8 @@ module API # DELETE /projects/:id/issues/:issue_id delete ":id/issues/:issue_id" do issue = user_project.issues.find(params[:issue_id]) - !JLJsdf sdfijsf current_user.can?(:remove_issue, issue) + + authorize!(:remove_issue, issue) issue = user_project.issues.find(params[:issue_id]) issue.destroy diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 09ce02b0912..dc94cc5c85f 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -106,9 +106,9 @@ module API # id (required) - The ID of the project # merge_request_id (required) - The MR id delete ":id/merge_requests/:merge_request_id" do - authenticated_as_admin! - merge_request = user_project.merge_requests.find(params[:merge_request_id]) + + authorize!(:remove_merge_request, merge_request) merge_request.destroy present merge_request, with: Entities::MergeRequest diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 99d1e1c595e..af154fadb6f 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -164,7 +164,7 @@ describe Projects::MergeRequestsController do expect(response.status).to eq 404 end - context "user is an admin" do + context "user is an admin or owner" do before do user.admin = true user.save diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 7d6ebc9b76e..6298771925e 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -2,12 +2,12 @@ require 'spec_helper' describe API::API, api: true do include ApiHelpers - let(:user) { create(:user) } - let(:non_member) { create(:user) } - let(:author) { create(:author) } - let(:assignee) { create(:assignee) } - let(:admin) { create(:admin) } - let!(:project) { create(:project, :public, namespace: user.namespace ) } + let(:user) { create(:user) } + let(:non_member) { create(:user) } + let(:author) { create(:author) } + let(:assignee) { create(:assignee) } + let(:admin) { create(:user, :admin) } + let!(:project) { create(:project, :public, namespace: user.namespace ) } let!(:closed_issue) do create :closed_issue, author: user, @@ -469,16 +469,18 @@ describe API::API, api: true do end describe "DELETE /projects/:id/issues/:issue_id" do - it "should reject non admins form deleting an issue" do - delete api("/projects/#{project.id}/issues/#{issue.id}", user) - expect(response.status).to eq(403) + it "should reject a non member from deleting an issue" do + delete api("/projects/#{project.id}/issues/#{issue.id}", non_member) + expect(response.status).to be(403) end - it "deletes the issue if an admin requests it" do - user.admin = true - user.save + it "should reject a developer from deleting an issue" do + delete api("/projects/#{project.id}/issues/#{issue.id}", author) + expect(response.status).to be(403) + end - delete api("/projects/#{project.id}/issues/#{issue.id}", user) + it "deletes the issue if an admin requests it" do + delete api("/projects/#{project.id}/issues/#{issue.id}", admin) expect(response.status).to eq(200) expect(json_response['state']).to eq 'opened' end diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 5a0bc4ff076..43e6a59d1ee 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -3,8 +3,10 @@ require "spec_helper" describe API::API, api: true do include ApiHelpers let(:base_time) { Time.now } - let(:user) { create(:user) } - let!(:project) {create(:project, creator_id: user.id, namespace: user.namespace) } + let(:user) { create(:user) } + let(:admin) { create(:user, :admin) } + let(:non_member) { create(:user) } + let!(:project) { create(:project, creator_id: user.id, namespace: user.namespace) } let!(:merge_request) { create(:merge_request, :simple, author: user, assignee: user, source_project: project, target_project: project, title: "Test", created_at: base_time) } let!(:merge_request_closed) { create(:merge_request, state: "closed", author: user, assignee: user, source_project: project, target_project: project, title: "Closed test", created_at: base_time + 1.second) } let!(:merge_request_merged) { create(:merge_request, state: "merged", author: user, assignee: user, source_project: project, target_project: project, title: "Merged test", created_at: base_time + 2.seconds) } @@ -316,21 +318,23 @@ describe API::API, api: true do end describe "DELETE /projects/:id/merge_request/:merge_request_id" do - it "rejects non admin users from deletions" do + it "owners can destroy" do delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user) - expect(response.status).to eq(403) + expect(response.status).to eq(200) end - it "let's Admins delete a merge request" do - user.admin = true - user.save - - delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user) + it "let's Admins and owners delete a merge request" do + delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}", admin) expect(response.status).to eq(200) expect(json_response['id']).to eq merge_request.id end + + it "rejects removal from other users" do + delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}", non_member) + expect(response.status).to eq(404) + end end describe "PUT /projects/:id/merge_requests/:merge_request_id to close MR" do |