Merge pull request #2835 from Asquera/fixes/api

Fix API return codes

9 files changed, 142 insertions, 33 deletions

diff --git a/lib/api.rb b/lib/api.rb
index 2a9a0eb2842..d241f9b7479 100644
--- a/lib/api.rb
+++ b/lib/api.rb
@@ -8,6 +8,19 @@ module Gitlab
       rack_response({'message' => '404 Not found'}.to_json, 404)
     end
 
+    rescue_from :all do |exception|
+      # lifted from https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb#L60
+      # why is this not wrapped in something reusable?
+      trace = exception.backtrace
+
+      message = "\n#{exception.class} (#{exception.message}):\n"
+      message << exception.annoted_source_code.to_s if exception.respond_to?(:annoted_source_code)
+      message << "  " << trace.join("\n  ")
+
+      API.logger.add Logger::FATAL, message 
+      rack_response({'message' => '500 Internal Server Error'}, 500)
+    end
+
     format :json
     helpers APIHelpers
     
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index 464a2d15662..52fa8eff33c 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -20,12 +20,14 @@ module Gitlab
       # Create group. Available only for admin
       #
       # Parameters:
-      #   name (required)                   - Name
-      #   path (required)                   - Path
+      #   name (required) - The name of the group
+      #   path (required) - The path of the group
       # Example Request:
       #   POST /groups
       post do
         authenticated_as_admin!
+        required_attributes! [:name, :path]
+
         attrs = attributes_for_keys [:name, :path]
         @group = Group.new(attrs)
         @group.owner = current_user
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 6bd8111c2b2..f12fb5fdbd0 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -41,6 +41,17 @@ module Gitlab
       abilities.allowed?(object, action, subject)
     end
 
+    # Checks the occurrences of required attributes, each attribute must be present in the params hash
+    # or a Bad Request error is invoked.
+    #
+    # Parameters:
+    #   keys (required) - A hash consisting of keys that must be present
+    def required_attributes!(keys)
+      keys.each do |key|
+        bad_request!(key) unless params[key].present?
+      end
+    end
+
     def attributes_for_keys(keys)
       attrs = {}
       keys.each do |key|
@@ -55,6 +66,12 @@ module Gitlab
       render_api_error!('403 Forbidden', 403)
     end
 
+    def bad_request!(attribute)
+      message = ["400 (Bad request)"]
+      message << "\"" + attribute.to_s + "\" not given"
+      render_api_error!(message.join(' '), 400)
+    end
+
     def not_found!(resource = nil)
       message = ["404"]
       message << resource if resource
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index 70bbf47e72c..500a8551f35 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -48,6 +48,7 @@ module Gitlab
       # Example Request:
       #   POST /projects/:id/issues
       post ":id/issues" do
+        required_attributes! [:title]
         attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id]
         attrs[:label_list] = params[:labels] if params[:labels].present?
         @issue = user_project.issues.new attrs
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index 7f763eb49d5..234a005a998 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -4,6 +4,16 @@ module Gitlab
     before { authenticate! }
 
     resource :projects do
+      helpers do
+        def handle_merge_request_errors!(errors)
+          if errors[:project_access].any?
+            error!(errors[:project_access], 422)
+          elsif errors[:branch_conflict].any?
+            error!(errors[:branch_conflict], 422)
+          end
+          not_found!
+        end
+      end
 
       # List merge requests
       #
@@ -51,6 +61,7 @@ module Gitlab
       #
       post ":id/merge_requests" do
         authorize! :write_merge_request, user_project
+        required_attributes! [:source_branch, :target_branch, :title]
 
         attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title]
         merge_request = user_project.merge_requests.new(attrs)
@@ -60,7 +71,7 @@ module Gitlab
           merge_request.reload_code
           present merge_request, with: Entities::MergeRequest
         else
-          not_found!
+          handle_merge_request_errors! merge_request.errors
         end
       end
 
@@ -88,7 +99,7 @@ module Gitlab
           merge_request.mark_as_unchecked
           present merge_request, with: Entities::MergeRequest
         else
-          not_found!
+          handle_merge_request_errors! merge_request.errors
         end
       end
 
@@ -102,6 +113,8 @@ module Gitlab
       #   POST /projects/:id/merge_request/:merge_request_id/comments
       #
       post ":id/merge_request/:merge_request_id/comments" do
+        required_attributes! [:note]
+
         merge_request = user_project.merge_requests.find(params[:merge_request_id])
         note = merge_request.notes.new(note: params[:note], project_id: user_project.id)
         note.author = current_user
diff --git a/lib/api/milestones.rb b/lib/api/milestones.rb
index eaf0d37c18b..1adeefece1f 100644
--- a/lib/api/milestones.rb
+++ b/lib/api/milestones.rb
@@ -41,6 +41,7 @@ module Gitlab
       #   POST /projects/:id/milestones
       post ":id/milestones" do
         authorize! :admin_milestone, user_project
+        required_attributes! [:title]
 
         attrs = attributes_for_keys [:title, :description, :due_date]
         @milestone = user_project.milestones.new attrs
diff --git a/lib/api/notes.rb b/lib/api/notes.rb
index 70344d6e381..097cc7ea475 100644
--- a/lib/api/notes.rb
+++ b/lib/api/notes.rb
@@ -37,12 +37,16 @@ module Gitlab
       # Example Request:
       #   POST /projects/:id/notes
       post ":id/notes" do
+        required_attributes! [:body]
+
         @note = user_project.notes.new(note: params[:body])
         @note.author = current_user
 
         if @note.save
           present @note, with: Entities::Note
         else
+          # :note is exposed as :body, but :note is set on error
+          bad_request!(:note) if @note.errors[:note].any?
           not_found!
         end
       end
@@ -89,6 +93,8 @@ module Gitlab
         #   POST /projects/:id/issues/:noteable_id/notes
         #   POST /projects/:id/snippets/:noteable_id/notes
         post ":id/#{noteables_str}/:#{noteable_id_str}/notes" do
+          required_attributes! [:body]
+
           @noteable = user_project.send(:"#{noteables_str}").find(params[:"#{noteable_id_str}"])
           @note = @noteable.notes.new(note: params[:body])
           @note.author = current_user
diff --git a/lib/api/projects.rb b/lib/api/projects.rb
index 00b70728c0e..e82cfeca45d 100644
--- a/lib/api/projects.rb
+++ b/lib/api/projects.rb
@@ -4,6 +4,15 @@ module Gitlab
     before { authenticate! }
 
     resource :projects do
+      helpers do
+        def handle_project_member_errors(errors)
+          if errors[:project_access].any?
+            error!(errors[:project_access], 422)
+          end
+          not_found!
+        end
+      end
+
       # Get a projects list for authenticated user
       #
       # Example Request:
@@ -33,9 +42,11 @@ module Gitlab
       #   wall_enabled (optional) - enabled by default
       #   merge_requests_enabled (optional) - enabled by default
       #   wiki_enabled (optional) - enabled by default
+      #   namespace_id (optional) - defaults to user namespace
       # Example Request
       #   POST /projects
       post do
+        required_attributes! [:name]
         attrs = attributes_for_keys [:name,
                                     :description,
                                     :default_branch,
@@ -48,6 +59,9 @@ module Gitlab
         if @project.saved?
           present @project, with: Entities::Project
         else
+          if @project.errors[:limit_reached].present?
+            error!(@project.errors[:limit_reached], 403)
+          end
           not_found!
         end
       end
@@ -122,16 +136,22 @@ module Gitlab
       #   POST /projects/:id/members
       post ":id/members" do
         authorize! :admin_project, user_project
-        users_project = user_project.users_projects.new(
-          user_id: params[:user_id],
-          project_access: params[:access_level]
-        )
+        required_attributes! [:user_id, :access_level]
+
+        # either the user is already a team member or a new one
+        team_member = user_project.team_member_by_id(params[:user_id])
+        if team_member.nil?
+          team_member = user_project.users_projects.new(
+            user_id: params[:user_id],
+            project_access: params[:access_level]
+          )
+        end
 
-        if users_project.save
-          @member = users_project.user
+        if team_member.save
+          @member = team_member.user
           present @member, with: Entities::ProjectMember, project: user_project
         else
-          not_found!
+          handle_project_member_errors team_member.errors
         end
       end
 
@@ -145,13 +165,16 @@ module Gitlab
       #   PUT /projects/:id/members/:user_id
       put ":id/members/:user_id" do
         authorize! :admin_project, user_project
-        users_project = user_project.users_projects.find_by_user_id params[:user_id]
+        required_attributes! [:access_level]
+
+        team_member = user_project.users_projects.find_by_user_id(params[:user_id])
+        not_found!("User can not be found") if team_member.nil?
 
-        if users_project.update_attributes(project_access: params[:access_level])
-          @member = users_project.user
+        if team_member.update_attributes(project_access: params[:access_level])
+          @member = team_member.user
           present @member, with: Entities::ProjectMember, project: user_project
         else
-          not_found!
+          handle_project_member_errors team_member.errors
         end
       end
 
@@ -164,8 +187,12 @@ module Gitlab
       #   DELETE /projects/:id/members/:user_id
       delete ":id/members/:user_id" do
         authorize! :admin_project, user_project
-        users_project = user_project.users_projects.find_by_user_id params[:user_id]
-        users_project.destroy
+        team_member = user_project.users_projects.find_by_user_id(params[:user_id])
+        unless team_member.nil?
+          team_member.destroy
+        else
+          {:message => "Access revoked", :id => params[:user_id].to_i}
+        end
       end
 
       # Get project hooks
@@ -203,11 +230,16 @@ module Gitlab
       #   POST /projects/:id/hooks
       post ":id/hooks" do
         authorize! :admin_project, user_project
+        required_attributes! [:url]
+
         @hook = user_project.hooks.new({"url" => params[:url]})
         if @hook.save
           present @hook, with: Entities::Hook
         else
-          error!({'message' => '404 Not found'}, 404)
+          if @hook.errors[:url].present?
+            error!("Invalid url given", 422)
+          end
+          not_found!
         end
       end
 
@@ -222,27 +254,36 @@ module Gitlab
       put ":id/hooks/:hook_id" do
         @hook = user_project.hooks.find(params[:hook_id])
         authorize! :admin_project, user_project
+        required_attributes! [:url]
 
         attrs = attributes_for_keys [:url]
-
         if @hook.update_attributes attrs
           present @hook, with: Entities::Hook
         else
+          if @hook.errors[:url].present?
+            error!("Invalid url given", 422)
+          end
           not_found!
         end
       end
 
-      # Delete project hook
+      # Deletes project hook. This is an idempotent function.
       #
       # Parameters:
       #   id (required) - The ID of a project
       #   hook_id (required) - The ID of hook to delete
       # Example Request:
       #   DELETE /projects/:id/hooks/:hook_id
-      delete ":id/hooks/:hook_id" do
+      delete ":id/hooks" do
         authorize! :admin_project, user_project
-        @hook = user_project.hooks.find(params[:hook_id])
-        @hook.destroy
+        required_attributes! [:hook_id]
+
+        begin
+          @hook = ProjectHook.find(params[:hook_id])
+          @hook.destroy
+        rescue
+          # ProjectHook can raise Error if hook_id not found
+        end
       end
 
       # Get a project repository branches
@@ -277,6 +318,7 @@ module Gitlab
       #   PUT /projects/:id/repository/branches/:branch/protect
       put ":id/repository/branches/:branch/protect" do
         @branch = user_project.repo.heads.find { |item| item.name == params[:branch] }
+        not_found! unless @branch
         protected = user_project.protected_branches.find_by_name(@branch.name)
 
         unless protected
@@ -295,6 +337,7 @@ module Gitlab
       #   PUT /projects/:id/repository/branches/:branch/unprotect
       put ":id/repository/branches/:branch/unprotect" do
         @branch = user_project.repo.heads.find { |item| item.name == params[:branch] }
+        not_found! unless @branch
         protected = user_project.protected_branches.find_by_name(@branch.name)
 
         if protected
@@ -318,7 +361,7 @@ module Gitlab
       #
       # Parameters:
       #   id (required) - The ID of a project
-      #   ref_name (optional) - The name of a repository branch or tag
+      #   ref_name (optional) - The name of a repository branch or tag, if not given the default branch is used
       # Example Request:
       #   GET /projects/:id/repository/commits
       get ":id/repository/commits" do
@@ -366,6 +409,7 @@ module Gitlab
       #   POST /projects/:id/snippets
       post ":id/snippets" do
         authorize! :write_snippet, user_project
+        required_attributes! [:title, :file_name, :code]
 
         attrs = attributes_for_keys [:title, :file_name]
         attrs[:expires_at] = params[:lifetime] if params[:lifetime].present?
@@ -414,10 +458,12 @@ module Gitlab
       # Example Request:
       #   DELETE /projects/:id/snippets/:snippet_id
       delete ":id/snippets/:snippet_id" do
-        @snippet = user_project.snippets.find(params[:snippet_id])
-        authorize! :modify_snippet, @snippet
-
-        @snippet.destroy
+        begin
+          @snippet = user_project.snippets.find(params[:snippet_id])
+          authorize! :modify_snippet, user_project
+          @snippet.destroy
+        rescue
+        end
       end
 
       # Get a raw project snippet
@@ -443,6 +489,7 @@ module Gitlab
       #   GET /projects/:id/repository/commits/:sha/blob
       get ":id/repository/commits/:sha/blob" do
         authorize! :download_code, user_project
+        required_attributes! [:filepath]
 
         ref = params[:sha]
 
diff --git a/lib/api/users.rb b/lib/api/users.rb
index 7399d1a5034..6cc3a7e52c9 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -41,6 +41,8 @@ module Gitlab
       #   POST /users
       post do
         authenticated_as_admin!
+        required_attributes! [:email, :password, :name, :username]
+
         attrs = attributes_for_keys [:email, :name, :password, :skype, :linkedin, :twitter, :projects_limit, :username, :extern_uid, :provider, :bio]
         user = User.new attrs, as: :admin
         if user.save
@@ -67,10 +69,12 @@ module Gitlab
       #   PUT /users/:id
       put ":id" do
         authenticated_as_admin!
+
         attrs = attributes_for_keys [:email, :name, :password, :skype, :linkedin, :twitter, :projects_limit, :username, :extern_uid, :provider, :bio]
-        user = User.find_by_id(params[:id])
+        user = User.find(params[:id])
+        not_found!("User not found") unless user
 
-        if user && user.update_attributes(attrs)
+        if user.update_attributes(attrs)
           present user, with: Entities::User
         else
           not_found!
@@ -147,6 +151,8 @@ module Gitlab
       # Example Request:
       #   POST /user/keys
       post "keys" do
+        required_attributes! [:title, :key]
+
         attrs = attributes_for_keys [:title, :key]
         key = current_user.keys.new attrs
         if key.save
@@ -156,15 +162,18 @@ module Gitlab
         end
       end
 
-      # Delete existed ssh key of currently authenticated user
+      # Delete existing ssh key of currently authenticated user
       #
       # Parameters:
       #   id (required) - SSH Key ID
       # Example Request:
       #   DELETE /user/keys/:id
       delete "keys/:id" do
-        key = current_user.keys.find params[:id]
-        key.delete
+        begin
+          key = current_user.keys.find params[:id]
+          key.delete
+        rescue
+        end
       end
     end
   end