minor improvements and fixed specs

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