Merge branch 'api_rework' into 'master'

API refactoring && better test coverage

See merge request !87

lib/api/projects.rb

@@ -16,15 +16,15 @@ module API
         project = Project.find(params[:project_id])
-        if project.present? && current_user.can_access_project?(project.gitlab_id)
-          web_hook = project.web_hooks.new({ url: params[:web_hook] })
-
-          if web_hook.save
-            present web_hook, with: Entities::WebHook
-          else
-            errors = web_hook.errors.full_messages.join(", ")
-            render_api_error!(errors, 400)
-          end
+        unauthorized! unless current_user.can_manage_project?(project.gitlab_id)
+        
+        web_hook = project.web_hooks.new({ url: params[:web_hook] })
+
+        if web_hook.save
+          present web_hook, with: Entities::WebHook
+        else
+          errors = web_hook.errors.full_messages.join(", ")
+          render_api_error!(errors, 400)
         end
       end
@@ -37,7 +37,6 @@ module API
       get ":id/jobs" do
         project = Project.find(params[:id])
-        not_found! if project.blank?
         unauthorized! unless current_user.can_manage_project?(project.gitlab_id)
         project.jobs
@@ -60,7 +59,6 @@ module API
         project = Project.find(params[:id])
-        not_found! if project.blank?
         unauthorized! unless current_user.can_manage_project?(project.gitlab_id)
         job_params =
@@ -99,7 +97,6 @@ module API
         project = Project.find(params[:id])
-        not_found! if project.blank?
         unauthorized! unless current_user.can_manage_project?(project.gitlab_id)
         job_params =
@@ -133,11 +130,11 @@ module API
         required_attributes! [:job_id]
         project = Project.find(params[:id])
-        job     = project.jobs.find(params[:job_id])
-        not_found! if project.blank? || job.blank?
         unauthorized! unless current_user.can_manage_project?(project.gitlab_id)
+        job = project.jobs.find(params[:job_id])
+
         job.destroy
       end
@@ -178,11 +175,9 @@ module API
       get ":id" do
         project = Project.find(params[:id])
-        if current_user.can_access_project?(project.gitlab_id)
-          present project, with: Entities::Project
-        else
-          unauthorized!
-        end
+        unauthorized! unless current_user.can_access_project?(project.gitlab_id)
+
+        present project, with: Entities::Project
       end
       # Create Gitlab CI project using Gitlab project info
@@ -232,17 +227,15 @@ module API
       put ":id" do
         project = Project.find(params[:id])
-        if project.present? && current_user.can_manage_project?(project.gitlab_id)
-          attrs = attributes_for_keys [:name, :gitlab_id, :gitlab_url, :default_ref, :ssh_url_to_repo]
-          if project.update_attributes(attrs)
-            present project, with: Entities::Project
-          else
-            errors = project.errors.full_messages.join(", ")
-            render_api_error!(errors, 400)
-          end
+        unauthorized! unless current_user.can_manage_project?(project.gitlab_id)
+        attrs = attributes_for_keys [:name, :gitlab_id, :gitlab_url, :default_ref, :ssh_url_to_repo]
+
+        if project.update_attributes(attrs)
+          present project, with: Entities::Project
         else
-          not_found!
+          errors = project.errors.full_messages.join(", ")
+          render_api_error!(errors, 400)
         end
       end
@@ -255,11 +248,9 @@ module API
       delete ":id" do
         project = Project.find(params[:id])
-        if project.present? && current_user.can_manage_project?(project.gitlab_id)
-          project.destroy
-        else
-          not_found!
-        end
+        unauthorized! unless current_user.can_manage_project?(project.gitlab_id)
+
+        project.destroy
       end
       # Link a Gitlab CI project to a runner
@@ -270,12 +261,10 @@ module API
       # Example Request:
       #   POST /projects/:id/runners/:runner_id
       post ":id/runners/:runner_id" do
-        project = Project.find_by_id(params[:id])
-        runner  = Runner.find_by_id(params[:runner_id])
-
-        not_found! if project.blank? or runner.blank?
-        unauthorized! unless current_user.can_access_project?(project.gitlab_id)
+        project = Project.find(params[:id])
+        runner  = Runner.find(params[:runner_id])
+        unauthorized! unless current_user.can_manage_project?(project.gitlab_id)
         options = {
           project_id: project.id,
@@ -300,18 +289,17 @@ module API
       # Example Request:
       #   DELETE /projects/:id/runners/:runner_id
       delete ":id/runners/:runner_id" do
-        project = Project.find_by_id(params[:id])
-        runner  = Runner.find_by_id(params[:runner_id])
-        not_found! if project.blank? or runner.blank?
-        unauthorized! unless current_user.can_access_project?(project.gitlab_id)
+        project = Project.find(params[:id])
+        runner  = Runner.find(params[:runner_id])
+        unauthorized! unless current_user.can_manage_project?(project.gitlab_id)
         options = {
           project_id: project.id,
           runner_id:  runner.id
         }
-        runner_project = RunnerProject.where(options).first
+        runner_project = RunnerProject.find_by(options)
         if runner_project.present?
           runner_project.destroy

lib/api/runners.rb

@@ -10,11 +10,7 @@ module API
         authenticate!
         runners = Runner.all
-        if runners.present?
-          present runners, with: Entities::Runner
-        else
-          not_found!
-        end
+        present runners, with: Entities::Runner
       end
       # Delete runner

spec/requests/api/projects_spec.rb

@@ -92,6 +92,12 @@ describe API::API do
         post api("/projects/non-existant-id/jobs"), options
         response.status.should == 404
       end
+
+      it "non-manager is not authorized" do
+        User.any_instance.stub(:can_manage_project?).and_return(false)
+        post api("/projects/#{project.id}/jobs"), options
+        response.status.should == 401
+      end
     end
   end
@@ -139,6 +145,12 @@ describe API::API do
         post api("/projects/non-existant-id/deploy_jobs"), options
         response.status.should == 404
       end
+
+      it "non-manager is not authorized" do
+        User.any_instance.stub(:can_manage_project?).and_return(false)
+        post api("/projects/#{project.id}/deploy_jobs"), options
+        response.status.should == 401
+      end
     end
   end
@@ -204,12 +216,9 @@ describe API::API do
     end
     it "should delete a project job" do
-      post api("/projects/#{project.id}/jobs"), options
-      response.status.should == 201
-      json_response["name"].should == job_info[:name]
-      json_response["commands"].should == job_info[:commands]
-      job_id = json_response["id"]
-      delete api("/projects/#{project.id}/jobs/#{job_id}"), options
+      job = FactoryGirl.create(:job, project: project)
+
+      delete api("/projects/#{project.id}/jobs/#{job.id}"), options
       response.status.should == 200
     end
@@ -222,6 +231,15 @@ describe API::API do
       delete api("/projects/#{project.id}/jobs/non-existant-job-id"), options
       response.status.should == 404
     end
+
+    it "non-manager is not authorized" do
+      User.any_instance.stub(:can_manage_project?).and_return(false)
+      job = FactoryGirl.create(:job, project: project)
+
+      delete api("/projects/#{project.id}/jobs/#{job.id}"), options
+
+      response.status.should == 401
+    end
   end
   describe "POST /projects/:project_id/webhooks" do
@@ -245,6 +263,11 @@ describe API::API do
         response.status.should == 404
       end
+      it "non-manager is not authorized" do
+        User.any_instance.stub(:can_manage_project?).and_return(false)
+        post api("/projects/#{project.id}/webhooks"), options
+        response.status.should == 401
+      end
     end
     context "Invalid Webhook URL" do
@@ -305,6 +328,12 @@ describe API::API do
       put api("/projects/non-existant-id"), options
       response.status.should == 404
     end
+
+    it "non-manager is not authorized" do
+      User.any_instance.stub(:can_manage_project?).and_return(false)
+      put api("/projects/#{project.id}"), options
+      response.status.should == 401
+    end
   end
   describe "DELETE /projects/:id" do
@@ -316,6 +345,17 @@ describe API::API do
       expect { project.reload }.to raise_error
     end
+
+    it "non-manager is not authorized" do
+      User.any_instance.stub(:can_manage_project?).and_return(false)
+      delete api("/projects/#{project.id}"), options
+      response.status.should == 401
+    end
+
+    it "is getting not found error" do
+      delete api("/projects/not-existing_id"), options
+      response.status.should == 404
+    end
   end
   describe "POST /projects" do
@@ -372,6 +412,12 @@ describe API::API do
         post api("/projects/non-existing/runners/#{runner.id}"), options
         response.status.should == 404
       end
+
+      it "non-manager is not authorized" do
+        User.any_instance.stub(:can_manage_project?).and_return(false)
+        post api("/projects/#{project.id}/runners/#{runner.id}"), options
+        response.status.should == 401
+      end
     end
     describe "DELETE /projects/:id/runners/:id" do
@@ -390,6 +436,12 @@ describe API::API do
         project.reload
         project.runners.should be_empty
       end
+
+      it "non-manager is not authorized" do
+        User.any_instance.stub(:can_manage_project?).and_return(false)
+        post api("/projects/#{project.id}/runners/#{runner.id}"), options
+        response.status.should == 401
+      end
     end
   end
 end