Merge branch 'zj-fix-label-creation-non-members' into 'security'

Fix label creation non members

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/23416

See merge request !2006

app/services/issuable_base_service.rb

@@ -85,14 +85,15 @@ class IssuableBaseService < BaseService
 
   def find_or_create_label_ids
     labels = params.delete(:labels)
+
     return unless labels
 
-    params[:label_ids] = labels.split(',').map do |label_name|
+    params[:label_ids] = labels.split(",").map do |label_name|
       service = Labels::FindOrCreateService.new(current_user, project, title: label_name.strip)
       label   = service.execute
 
-      label.id
-    end
+      label.try(:id)
+    end.compact
   end
 
   def process_label_ids(attributes, existing_label_ids: nil)
@@ -140,6 +141,7 @@ class IssuableBaseService < BaseService
 
     params.delete(:state_event)
     params[:author] ||= current_user
+
     label_ids = process_label_ids(params)
 
     issuable.assign_attributes(params)

app/services/labels/find_or_create_service.rb

@@ -22,9 +22,14 @@ module Labels
       ).execute(skip_authorization: skip_authorization)
     end
 
+    # Only creates the label if current_user can do so, if the label does not exist
+    # and the user can not create the label, nil is returned
     def find_or_create_label
       new_label = available_labels.find_by(title: title)
-      new_label ||= project.labels.create(params)
+
+      if new_label.nil? && (skip_authorization || Ability.allowed?(current_user, :admin_label, project))
+        new_label = project.labels.create(params)
+      end
 
       new_label
     end

changelogs/unreleased/zj-fix-label-creation-non-members.yml

+---
+title: Non members cannot create labels through the API
+merge_request: 
+author: 

lib/api/helpers.rb

@@ -192,20 +192,6 @@ module API
       ActionController::Parameters.new(attrs).permit!
     end
 
-    # Helper method for validating all labels against its names
-    def validate_label_params(params)
-      errors = {}
-
-      params[:labels].to_s.split(',').each do |label_name|
-        label = available_labels.find_or_initialize_by(title: label_name.strip)
-        next if label.valid?
-
-        errors[label.title] = label.errors
-      end
-
-      errors
-    end
-
     # Checks the occurrences of datetime attributes, each attribute if present in the params hash must be in ISO 8601
     # format (YYYY-MM-DDTHH:MM:SSZ) or a Bad Request error is invoked.
     #

lib/api/issues.rb

@@ -19,6 +19,15 @@ module API
       def filter_issues_milestone(issues, milestone)
         issues.includes(:milestone).where('milestones.title' => milestone)
       end
+
+      def issue_params
+        new_params = declared(params, include_parent_namespace: false, include_missing: false).to_h
+        new_params = new_params.with_indifferent_access
+        new_params.delete(:id)
+        new_params.delete(:issue_id)
+
+        new_params
+      end
     end
 
     resource :issues do
@@ -86,6 +95,10 @@ module API
       end
     end
 
+    params do
+      requires :id, type: String, desc: 'The ID of a project'
+    end
+
     resource :projects do
       # Get a list of project issues
       #
@@ -152,17 +165,10 @@ module API
       post ':id/issues' do
         required_attributes! [:title]
 
-        keys = [:title, :description, :assignee_id, :milestone_id, :due_date, :confidential]
+        keys = [:title, :description, :assignee_id, :milestone_id, :due_date, :confidential, :labels]
         keys << :created_at if current_user.admin? || user_project.owner == current_user
         attrs = attributes_for_keys(keys)
 
-        # Validate label names in advance
-        if (errors = validate_label_params(params)).any?
-          render_api_error!({ labels: errors }, 400)
-        end
-
-        attrs[:labels] = params[:labels] if params[:labels]
-
         # Convert and filter out invalid confidential flags
         attrs['confidential'] = to_boolean(attrs['confidential'])
         attrs.delete('confidential') if attrs['confidential'].nil?
@@ -180,41 +186,35 @@ module API
         end
       end
 
-      # Update an existing issue
-      #
-      # Parameters:
-      #   id (required) - The ID of a project
-      #   issue_id (required) - The ID of a project issue
-      #   title (optional) - The title of an issue
-      #   description (optional) - The description of an issue
-      #   assignee_id (optional) - The ID of a user to assign issue
-      #   milestone_id (optional) - The ID of a milestone to assign issue
-      #   labels (optional) - The labels of an issue
-      #   state_event (optional) - The state event of an issue (close|reopen)
-      #   updated_at (optional) - Date time string, ISO 8601 formatted
-      #   due_date (optional)     - Date time string in the format YEAR-MONTH-DAY
-      #   confidential (optional) - Boolean parameter if the issue should be confidential
-      # Example Request:
-      #   PUT /projects/:id/issues/:issue_id
+      desc 'Update an existing issue' do
+        success Entities::Issue
+      end
+      params do
+        requires :id, type: String, desc: 'The ID of a project'
+        requires :issue_id, type: Integer, desc: "The ID of a project issue"
+        optional :title, type: String, desc: 'The new title of the issue'
+        optional :description, type: String, desc: 'The description of an issue'
+        optional :assignee_id, type: Integer, desc: 'The ID of a user to assign issue'
+        optional :milestone_id, type: Integer, desc: 'The ID of a milestone to assign issue'
+        optional :labels, type: String, desc: 'The labels of an issue'
+        optional :state_event, type: String, values: ['close', 'reopen'], desc: 'The state event of an issue'
+        # TODO 9.0, use the Grape DateTime type here
+        optional :updated_at, type: String, desc: 'Date time string, ISO 8601 formatted'
+        optional :due_date, type: String, desc: 'Date time string in the format YEAR-MONTH-DAY'
+        # TODO 9.0, use the Grape boolean type here
+        optional :confidential, type: String, desc: 'Boolean parameter if the issue should be confidential'
+      end
       put ':id/issues/:issue_id' do
         issue = user_project.issues.find(params[:issue_id])
         authorize! :update_issue, issue
-        keys = [:title, :description, :assignee_id, :milestone_id, :state_event, :due_date, :confidential]
-        keys << :updated_at if current_user.admin? || user_project.owner == current_user
-        attrs = attributes_for_keys(keys)
-
-        # Validate label names in advance
-        if (errors = validate_label_params(params)).any?
-          render_api_error!({ labels: errors }, 400)
-        end
-
-        attrs[:labels] = params[:labels] if params[:labels]
 
         # Convert and filter out invalid confidential flags
-        attrs['confidential'] = to_boolean(attrs['confidential'])
-        attrs.delete('confidential') if attrs['confidential'].nil?
+        params[:confidential] = to_boolean(params[:confidential])
+        params.delete(:confidential) if params[:confidential].nil?
+
+        params.delete(:updated_at) unless current_user.admin? || user_project.owner == current_user
 
-        issue = ::Issues::UpdateService.new(user_project, current_user, attrs).execute(issue)
+        issue = ::Issues::UpdateService.new(user_project, current_user, issue_params).execute(issue)
 
         if issue.valid?
           present issue, with: Entities::Issue, current_user: current_user

lib/api/merge_requests.rb

@@ -79,12 +79,7 @@ module API
       post ":id/merge_requests" do
         authorize! :create_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, :milestone_id]
-
-        # Validate label names in advance
-        if (errors = validate_label_params(params)).any?
-          render_api_error!({ labels: errors }, 400)
-        end
+        attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :target_project_id, :description, :milestone_id, :labels]
 
         attrs[:labels] = params[:labels] if params[:labels]
 
@@ -112,6 +107,9 @@ module API
       # Routing "merge_request/:merge_request_id/..." is DEPRECATED and WILL BE REMOVED in version 9.0
       # Use "merge_requests/:merge_request_id/..." instead.
       #
+      params do
+        requires :id, type: String, desc: 'The ID of a project'
+      end
       [":id/merge_request/:merge_request_id", ":id/merge_requests/:merge_request_id"].each do |path|
         # Show MR
         #
@@ -162,23 +160,20 @@ module API
           present merge_request, with: Entities::MergeRequestChanges, current_user: current_user
         end
 
-        # Update MR
-        #
-        # Parameters:
-        #   id (required)               - The ID of a project
-        #   merge_request_id (required) - ID of MR
-        #   target_branch               - The target branch
-        #   assignee_id                 - Assignee user ID
-        #   title                       - Title of MR
-        #   state_event                 - Status of MR. (close|reopen|merge)
-        #   description                 - Description of MR
-        #   labels (optional)           - Labels for a MR as a comma-separated list
-        #   milestone_id (optional)     - Milestone ID
-        # Example:
-        #   PUT /projects/:id/merge_requests/:merge_request_id
-        #
+        desc 'Update a merge request' do
+          success Entities::MergeRequest
+        end
+        params do
+          requires :merge_request_id, type: Integer, desc: 'The ID of a merge request'
+          optional :target_branch, type: String, desc: 'The new target branch'
+          optional :assignee_id, type: Integer, desc: 'The assignees user ID'
+          optional :title, type: String, desc: 'The new title for the merge request'
+          optional :state_event, type: String, values: ['close', 'reopen', 'merge'], desc: 'The state of the merge request'
+          optional :description, type: String, desc: 'The description, with markdown support'
+          optional :labels, type: String, desc: 'Labels for a MR as a comma-separated list'
+          optional :milestone_id, type: Integer, desc: 'The ID of the new milestone'
+        end
         put path do
-          attrs = attributes_for_keys [:target_branch, :assignee_id, :title, :state_event, :description, :milestone_id]
           merge_request = user_project.merge_requests.find(params[:merge_request_id])
           authorize! :update_merge_request, merge_request
 
@@ -187,14 +182,10 @@ module API
             render_api_error!('Source branch cannot be changed', 400)
           end
 
-          # Validate label names in advance
-          if (errors = validate_label_params(params)).any?
-            render_api_error!({ labels: errors }, 400)
-          end
-
-          attrs[:labels] = params[:labels] if params[:labels]
+          mr_params = declared(params, include_missing: false, include_parent_namespace: false).with_indifferent_access
+          mr_params.delete(:merge_request_id)
 
-          merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, attrs).execute(merge_request)
+          merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, mr_params).execute(merge_request)
 
           if merge_request.valid?
             present merge_request, with: Entities::MergeRequest, current_user: current_user

spec/requests/api/issues_spec.rb

@@ -697,6 +697,14 @@ describe API::API, api: true  do
         expect(Time.parse(json_response['created_at'])).to be_like_time(creation_time)
       end
     end
+
+    context 'the user can only read the issue' do
+      it 'cannot create new labels' do
+        expect do
+          post api("/projects/#{project.id}/issues", non_member), title: 'new issue', labels: 'label, label2'
+        end.not_to change { project.labels.count }
+      end
+    end
   end
 
   describe 'POST /projects/:id/issues with spam filtering' do
@@ -839,8 +847,8 @@ describe API::API, api: true  do
     end
 
     it 'removes all labels' do
-      put api("/projects/#{project.id}/issues/#{issue.id}", user),
-          labels: ''
+      put api("/projects/#{project.id}/issues/#{issue.id}", user), labels: ''
+
       expect(response).to have_http_status(200)
       expect(json_response['labels']).to eq([])
     end
@@ -892,8 +900,8 @@ describe API::API, api: true  do
         update_time = 2.weeks.ago
         put api("/projects/#{project.id}/issues/#{issue.id}", user),
           labels: 'label3', state_event: 'close', updated_at: update_time
-        expect(response).to have_http_status(200)
 
+        expect(response).to have_http_status(200)
         expect(json_response['labels']).to include 'label3'
         expect(Time.parse(json_response['updated_at'])).to be_like_time(update_time)
       end

spec/requests/api/merge_requests_spec.rb

@@ -392,14 +392,6 @@ describe API::API, api: true  do
     end
   end
 
-  describe "PUT /projects/:id/merge_requests/:merge_request_id to close MR" do
-    it "returns merge_request" do
-      put api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user), state_event: "close"
-      expect(response).to have_http_status(200)
-      expect(json_response['state']).to eq('closed')
-    end
-  end
-
   describe "PUT /projects/:id/merge_requests/:merge_request_id/merge" do
     let(:pipeline) { create(:ci_pipeline_without_jobs) }
 
@@ -476,6 +468,15 @@ describe API::API, api: true  do
   end
 
   describe "PUT /projects/:id/merge_requests/:merge_request_id" do
+    context "to close a MR" do
+      it "returns merge_request" do
+        put api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user), state_event: "close"
+
+        expect(response).to have_http_status(200)
+        expect(json_response['state']).to eq('closed')
+      end
+    end
+
     it "updates title and returns merge_request" do
       put api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user), title: "New title"
       expect(response).to have_http_status(200)
@@ -496,7 +497,7 @@ describe API::API, api: true  do
 
     it "returns 400 when source_branch is specified" do
       put api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user),
-      source_branch: "master", target_branch: "master"
+        source_branch: "master", target_branch: "master"
       expect(response).to have_http_status(400)
     end
 
@@ -507,10 +508,10 @@ describe API::API, api: true  do
     end
 
     it 'allows special label names' do
-      put api("/projects/#{project.id}/merge_requests/#{merge_request.id}",
-              user),
-          title: 'new issue',
-          labels: 'label, label?, label&foo, ?, &'
+      put api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user),
+        title: 'new issue',
+        labels: 'label, label?, label&foo, ?, &'
+
       expect(response.status).to eq(200)
       expect(json_response['labels']).to include 'label'
       expect(json_response['labels']).to include 'label?'
@@ -539,7 +540,7 @@ describe API::API, api: true  do
 
     it "returns 404 if note is attached to non existent merge request" do
       post api("/projects/#{project.id}/merge_requests/404/comments", user),
-           note: 'My comment'
+        note: 'My comment'
       expect(response).to have_http_status(404)
     end
   end

spec/services/labels/transfer_service_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Labels::TransferService, services: true do
   describe '#execute' do
-    let(:user)    { create(:user) }
+    let(:user)    { create(:admin) }
     let(:group_1) { create(:group) }
     let(:group_2) { create(:group) }
     let(:group_3) { create(:group) }