Do label validation for issues/merge requests API

9 files changed, 108 insertions, 8 deletions

diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 517e4548624..0a5fe24b5af 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -140,7 +140,8 @@ module Issuable
 
   def add_labels_by_names(label_names)
     label_names.each do |label_name|
-      label = project.labels.find_or_create_by(title: label_name.strip)
+      label = project.labels.create_with(
+        color: Label::DEFAULT_COLOR).find_or_create_by(title: label_name.strip)
       self.labels << label
     end
   end
diff --git a/app/models/label.rb b/app/models/label.rb
index 515ed447f00..a511b7940ed 100644
--- a/app/models/label.rb
+++ b/app/models/label.rb
@@ -1,4 +1,6 @@
 class Label < ActiveRecord::Base
+  DEFAULT_COLOR = '#82C5FF'
+
   belongs_to :project
   has_many :label_links, dependent: :destroy
   has_many :issues, through: :label_links, source: :target, source_type: 'Issue'
diff --git a/doc/api/issues.md b/doc/api/issues.md
index f775d502a6d..a4b3b3e9918 100644
--- a/doc/api/issues.md
+++ b/doc/api/issues.md
@@ -157,6 +157,9 @@ Parameters:
 - `milestone_id` (optional) - The ID of a milestone to assign issue
 - `labels` (optional) - Comma-separated label names for an issue
 
+If the operation is successful, 200 and the newly created issue is returned.
+If an error occurs, an error number and a message explaining the reason is returned.
+
 ## Edit issue
 
 Updates an existing project issue. This function is also used to mark an issue as closed.
@@ -176,6 +179,9 @@ Parameters:
 - `labels` (optional) - Comma-separated label names for an issue
 - `state_event` (optional) - The state event of an issue ('close' to close issue and 'reopen' to reopen it)
 
+If the operation is successful, 200 and the updated issue is returned.
+If an error occurs, an error number and a message explaining the reason is returned.
+
 ## Delete existing issue (**Deprecated**)
 
 The function is deprecated and returns a `405 Method Not Allowed` error if called. An issue gets now closed and is done by calling `PUT /projects/:id/issues/:issue_id` with parameter `closed` set to 1.
diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md
index a46472a0812..f56e968e7c2 100644
--- a/doc/api/merge_requests.md
+++ b/doc/api/merge_requests.md
@@ -136,6 +136,9 @@ Parameters:
 }
 ```
 
+If the operation is successful, 200 and the newly created merge request is returned.
+If an error occurs, an error number and a message explaining the reason is returned.
+
 ## Update MR
 
 Updates an existing merge request. You can change branches, title, or even close the MR.
@@ -183,15 +186,18 @@ Parameters:
 }
 ```
 
+If the operation is successful, 200 and the updated merge request is returned.
+If an error occurs, an error number and a message explaining the reason is returned.
+
 ## Accept MR
 
-Merge changes submitted with MR usign this API.
+Merge changes submitted with MR using this API.
 
 If merge success you get 200 OK.
 
 If it has some conflicts and can not be merged - you get 405 and error message 'Branch cannot be merged'
 
-If merge request is already merged or closed - you get 405 and error message 'Method Not Allowed' 
+If merge request is already merged or closed - you get 405 and error message 'Method Not Allowed'
 
 If you dont have permissions to accept this merge request - you get 401
 
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 8189e433789..d36b29a00b1 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -112,6 +112,21 @@ module API
       ActionController::Parameters.new(attrs).permit!
     end
 
+    # Helper method for validating all labels against its names
+    def validate_label_params(params)
+      if params[:labels].present?
+        params[:labels].split(',').each do |label_name|
+          label = user_project.labels.create_with(
+            color: Label::DEFAULT_COLOR).find_or_initialize_by(
+              title: label_name.strip)
+          if label.invalid?
+            return true
+          end
+        end
+      end
+      false
+    end
+
     # error helpers
 
     def forbidden!
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index b29118b2fd8..055529ccbd8 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -51,12 +51,18 @@ module API
         required_attributes! [:title]
         attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id]
 
+        # Validate label names in advance
+        if validate_label_params(params)
+          return render_api_error!('Label names invalid', 405)
+        end
+
         issue = ::Issues::CreateService.new(user_project, current_user, attrs).execute
 
         if issue.valid?
-          # Find or create labels and attach to issue
+          # Find or create labels and attach to issue. Labels are valid because
+          # we already checked its name, so there can't be an error here
           if params[:labels].present?
-            issue.add_labels_by_names(params[:labels].split(","))
+            issue.add_labels_by_names(params[:labels].split(','))
           end
 
           present issue, with: Entities::Issue
@@ -83,12 +89,19 @@ module API
         authorize! :modify_issue, issue
         attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id, :state_event]
 
+        # Validate label names in advance
+        if validate_label_params(params)
+          return render_api_error!('Label names invalid', 405)
+        end
+
         issue = ::Issues::UpdateService.new(user_project, current_user, attrs).execute(issue)
 
         if issue.valid?
-          # Find or create labels and attach to issue
+          # Find or create labels and attach to issue. Labels are valid because
+          # we already checked its name, so there can't be an error here
           if params[:labels].present?
-            issue.add_labels_by_names(params[:labels].split(","))
+            # Create and add labels to the new created issue
+            issue.add_labels_by_names(params[:labels].split(','))
           end
 
           present issue, with: Entities::Issue
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index acca7cb6bad..0d765f9280e 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -76,6 +76,12 @@ module API
         authorize! :write_merge_request, user_project
         required_attributes! [:source_branch, :target_branch, :title]
         attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :target_project_id, :description]
+
+        # Validate label names in advance
+        if validate_label_params(params)
+          return render_api_error!('Label names invalid', 405)
+        end
+
         merge_request = ::MergeRequests::CreateService.new(user_project, current_user, attrs).execute
 
         if merge_request.valid?
@@ -109,6 +115,12 @@ module API
         attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :state_event, :description]
         merge_request = user_project.merge_requests.find(params[:merge_request_id])
         authorize! :modify_merge_request, merge_request
+
+        # Validate label names in advance
+        if validate_label_params(params)
+          return render_api_error!('Label names invalid', 405)
+        end
+
         merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, attrs).execute(merge_request)
 
         if merge_request.valid?
diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb
index dff7f20cb32..d8e8e4f5035 100644
--- a/spec/requests/api/issues_spec.rb
+++ b/spec/requests/api/issues_spec.rb
@@ -5,6 +5,10 @@ describe API::API, api: true  do
   let(:user) { create(:user) }
   let!(:project) { create(:project, namespace: user.namespace ) }
   let!(:issue) { create(:issue, author: user, assignee: user, project: project) }
+  let!(:label) do
+    create(:label, title: 'label', color: '#FFAABB', project: project)
+  end
+
   before { project.team << [user, :reporter] }
 
   describe "GET /issues" do
@@ -68,6 +72,14 @@ describe API::API, api: true  do
       post api("/projects/#{project.id}/issues", user), labels: 'label, label2'
       response.status.should == 400
     end
+
+    it 'should return 405 on invalid label names' do
+      post api("/projects/#{project.id}/issues", user),
+           title: 'new issue',
+           labels: 'label, ?'
+      response.status.should == 405
+      json_response['message'].should == 'Label names invalid'
+    end
   end
 
   describe "PUT /projects/:id/issues/:issue_id to update only title" do
@@ -84,6 +96,14 @@ describe API::API, api: true  do
         title: 'updated title'
       response.status.should == 404
     end
+
+    it 'should return 405 on invalid label names' do
+      put api("/projects/#{project.id}/issues/#{issue.id}", user),
+          title: 'updated title',
+          labels: 'label, ?'
+      response.status.should == 405
+      json_response['message'].should == 'Label names invalid'
+    end
   end
 
   describe "PUT /projects/:id/issues/:issue_id to update state and label" do
diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb
index 3611d9d6dc3..58cf7f139dc 100644
--- a/spec/requests/api/merge_requests_spec.rb
+++ b/spec/requests/api/merge_requests_spec.rb
@@ -78,9 +78,14 @@ describe API::API, api: true  do
     context 'between branches projects' do
       it "should return merge_request" do
         post api("/projects/#{project.id}/merge_requests", user),
-        title: 'Test merge_request', source_branch: "stable", target_branch: "master", author: user
+             title: 'Test merge_request',
+             source_branch: 'stable',
+             target_branch: 'master',
+             author: user,
+             labels: 'label, label2'
         response.status.should == 201
         json_response['title'].should == 'Test merge_request'
+        json_response['labels'].should == ['label', 'label2']
       end
 
       it "should return 422 when source_branch equals target_branch" do
@@ -106,6 +111,17 @@ describe API::API, api: true  do
         target_branch: 'master', source_branch: 'stable'
         response.status.should == 400
       end
+
+      it 'should return 405 on invalid label names' do
+        post api("/projects/#{project.id}/merge_requests", user),
+             title: 'Test merge_request',
+             source_branch: 'stable',
+             target_branch: 'master',
+             author: user,
+             labels: 'label, ?'
+        response.status.should == 405
+        json_response['message'].should == 'Label names invalid'
+      end
     end
 
     context 'forked projects' do
@@ -235,6 +251,15 @@ describe API::API, api: true  do
       response.status.should == 200
       json_response['target_branch'].should == 'wiki'
     end
+
+    it 'should return 405 on invalid label names' do
+      put api("/projects/#{project.id}/merge_request/#{merge_request.id}",
+              user),
+          title: 'new issue',
+          labels: 'label, ?'
+      response.status.should == 405
+      json_response['message'].should == 'Label names invalid'
+    end
   end
 
   describe "POST /projects/:id/merge_request/:merge_request_id/comments" do