Commit 3d7704ae authored by Douwe Maan's avatar Douwe Maan Committed by Alejandro Rodríguez

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
parent ec5d0472
...@@ -85,14 +85,15 @@ class IssuableBaseService < BaseService ...@@ -85,14 +85,15 @@ class IssuableBaseService < BaseService
def find_or_create_label_ids def find_or_create_label_ids
labels = params.delete(:labels) labels = params.delete(:labels)
return unless 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) service = Labels::FindOrCreateService.new(current_user, project, title: label_name.strip)
label = service.execute label = service.execute
label.id label.try(:id)
end end.compact
end end
def process_label_ids(attributes, existing_label_ids: nil) def process_label_ids(attributes, existing_label_ids: nil)
...@@ -140,6 +141,7 @@ class IssuableBaseService < BaseService ...@@ -140,6 +141,7 @@ class IssuableBaseService < BaseService
params.delete(:state_event) params.delete(:state_event)
params[:author] ||= current_user params[:author] ||= current_user
label_ids = process_label_ids(params) label_ids = process_label_ids(params)
issuable.assign_attributes(params) issuable.assign_attributes(params)
......
...@@ -22,9 +22,14 @@ module Labels ...@@ -22,9 +22,14 @@ module Labels
).execute(skip_authorization: skip_authorization) ).execute(skip_authorization: skip_authorization)
end 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 def find_or_create_label
new_label = available_labels.find_by(title: title) 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 new_label
end end
......
---
title: Non members cannot create labels through the API
merge_request:
author:
...@@ -198,20 +198,6 @@ module API ...@@ -198,20 +198,6 @@ module API
ActionController::Parameters.new(attrs).permit! ActionController::Parameters.new(attrs).permit!
end 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 # 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. # format (YYYY-MM-DDTHH:MM:SSZ) or a Bad Request error is invoked.
# #
......
...@@ -19,6 +19,15 @@ module API ...@@ -19,6 +19,15 @@ module API
def filter_issues_milestone(issues, milestone) def filter_issues_milestone(issues, milestone)
issues.includes(:milestone).where('milestones.title' => milestone) issues.includes(:milestone).where('milestones.title' => milestone)
end 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 end
resource :issues do resource :issues do
...@@ -86,6 +95,10 @@ module API ...@@ -86,6 +95,10 @@ module API
end end
end end
params do
requires :id, type: String, desc: 'The ID of a project'
end
resource :projects do resource :projects do
# Get a list of project issues # Get a list of project issues
# #
...@@ -152,17 +165,10 @@ module API ...@@ -152,17 +165,10 @@ module API
post ':id/issues' do post ':id/issues' do
required_attributes! [:title] 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 keys << :created_at if current_user.admin? || user_project.owner == current_user
attrs = attributes_for_keys(keys) 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 # Convert and filter out invalid confidential flags
attrs['confidential'] = to_boolean(attrs['confidential']) attrs['confidential'] = to_boolean(attrs['confidential'])
attrs.delete('confidential') if attrs['confidential'].nil? attrs.delete('confidential') if attrs['confidential'].nil?
...@@ -180,41 +186,35 @@ module API ...@@ -180,41 +186,35 @@ module API
end end
end end
# Update an existing issue desc 'Update an existing issue' do
# success Entities::Issue
# Parameters: end
# id (required) - The ID of a project params do
# issue_id (required) - The ID of a project issue requires :id, type: String, desc: 'The ID of a project'
# title (optional) - The title of an issue requires :issue_id, type: Integer, desc: "The ID of a project issue"
# description (optional) - The description of an issue optional :title, type: String, desc: 'The new title of the issue'
# assignee_id (optional) - The ID of a user to assign issue optional :description, type: String, desc: 'The description of an issue'
# milestone_id (optional) - The ID of a milestone to assign issue optional :assignee_id, type: Integer, desc: 'The ID of a user to assign issue'
# labels (optional) - The labels of an issue optional :milestone_id, type: Integer, desc: 'The ID of a milestone to assign issue'
# state_event (optional) - The state event of an issue (close|reopen) optional :labels, type: String, desc: 'The labels of an issue'
# updated_at (optional) - Date time string, ISO 8601 formatted optional :state_event, type: String, values: ['close', 'reopen'], desc: 'The state event of an issue'
# due_date (optional) - Date time string in the format YEAR-MONTH-DAY # TODO 9.0, use the Grape DateTime type here
# confidential (optional) - Boolean parameter if the issue should be confidential optional :updated_at, type: String, desc: 'Date time string, ISO 8601 formatted'
# Example Request: optional :due_date, type: String, desc: 'Date time string in the format YEAR-MONTH-DAY'
# PUT /projects/:id/issues/:issue_id # 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 put ':id/issues/:issue_id' do
issue = user_project.issues.find(params[:issue_id]) issue = user_project.issues.find(params[:issue_id])
authorize! :update_issue, issue 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 # Convert and filter out invalid confidential flags
attrs['confidential'] = to_boolean(attrs['confidential']) params[:confidential] = to_boolean(params[:confidential])
attrs.delete('confidential') if attrs['confidential'].nil? 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? if issue.valid?
present issue, with: Entities::Issue, current_user: current_user, project: user_project present issue, with: Entities::Issue, current_user: current_user, project: user_project
......
...@@ -77,11 +77,6 @@ module API ...@@ -77,11 +77,6 @@ module API
mr_params = declared_params mr_params = declared_params
# Validate label names in advance
if (errors = validate_label_params(mr_params)).any?
render_api_error!({ labels: errors }, 400)
end
merge_request = ::MergeRequests::CreateService.new(user_project, current_user, mr_params).execute merge_request = ::MergeRequests::CreateService.new(user_project, current_user, mr_params).execute
if merge_request.valid? if merge_request.valid?
...@@ -157,11 +152,6 @@ module API ...@@ -157,11 +152,6 @@ module API
mr_params = declared_params(include_missing: false) mr_params = declared_params(include_missing: false)
# Validate label names in advance
if (errors = validate_label_params(mr_params)).any?
render_api_error!({ labels: errors }, 400)
end
merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, mr_params).execute(merge_request) merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, mr_params).execute(merge_request)
if merge_request.valid? if merge_request.valid?
......
...@@ -697,6 +697,14 @@ describe API::API, api: true do ...@@ -697,6 +697,14 @@ describe API::API, api: true do
expect(Time.parse(json_response['created_at'])).to be_like_time(creation_time) expect(Time.parse(json_response['created_at'])).to be_like_time(creation_time)
end end
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 end
describe 'POST /projects/:id/issues with spam filtering' do describe 'POST /projects/:id/issues with spam filtering' do
...@@ -839,8 +847,8 @@ describe API::API, api: true do ...@@ -839,8 +847,8 @@ describe API::API, api: true do
end end
it 'removes all labels' do it 'removes all labels' do
put api("/projects/#{project.id}/issues/#{issue.id}", user), put api("/projects/#{project.id}/issues/#{issue.id}", user), labels: ''
labels: ''
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
expect(json_response['labels']).to eq([]) expect(json_response['labels']).to eq([])
end end
...@@ -892,8 +900,8 @@ describe API::API, api: true do ...@@ -892,8 +900,8 @@ describe API::API, api: true do
update_time = 2.weeks.ago update_time = 2.weeks.ago
put api("/projects/#{project.id}/issues/#{issue.id}", user), put api("/projects/#{project.id}/issues/#{issue.id}", user),
labels: 'label3', state_event: 'close', updated_at: update_time 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(json_response['labels']).to include 'label3'
expect(Time.parse(json_response['updated_at'])).to be_like_time(update_time) expect(Time.parse(json_response['updated_at'])).to be_like_time(update_time)
end end
......
...@@ -402,14 +402,6 @@ describe API::API, api: true do ...@@ -402,14 +402,6 @@ describe API::API, api: true do
end end
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 describe "PUT /projects/:id/merge_requests/:merge_request_id/merge" do
let(:pipeline) { create(:ci_pipeline_without_jobs) } let(:pipeline) { create(:ci_pipeline_without_jobs) }
...@@ -486,6 +478,15 @@ describe API::API, api: true do ...@@ -486,6 +478,15 @@ describe API::API, api: true do
end end
describe "PUT /projects/:id/merge_requests/:merge_request_id" do 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 it "updates title and returns merge_request" do
put api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user), title: "New title" put api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user), title: "New title"
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
...@@ -511,10 +512,10 @@ describe API::API, api: true do ...@@ -511,10 +512,10 @@ describe API::API, api: true do
end end
it 'allows special label names' do it 'allows special label names' do
put api("/projects/#{project.id}/merge_requests/#{merge_request.id}", put api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user),
user), title: 'new issue',
title: 'new issue', labels: 'label, label?, label&foo, ?, &'
labels: 'label, label?, label&foo, ?, &'
expect(response.status).to eq(200) expect(response.status).to eq(200)
expect(json_response['labels']).to include 'label' expect(json_response['labels']).to include 'label'
expect(json_response['labels']).to include 'label?' expect(json_response['labels']).to include 'label?'
...@@ -543,7 +544,7 @@ describe API::API, api: true do ...@@ -543,7 +544,7 @@ describe API::API, api: true do
it "returns 404 if note is attached to non existent merge request" do it "returns 404 if note is attached to non existent merge request" do
post api("/projects/#{project.id}/merge_requests/404/comments", user), post api("/projects/#{project.id}/merge_requests/404/comments", user),
note: 'My comment' note: 'My comment'
expect(response).to have_http_status(404) expect(response).to have_http_status(404)
end end
end end
......
...@@ -2,7 +2,7 @@ require 'spec_helper' ...@@ -2,7 +2,7 @@ require 'spec_helper'
describe Labels::TransferService, services: true do describe Labels::TransferService, services: true do
describe '#execute' do describe '#execute' do
let(:user) { create(:user) } let(:user) { create(:admin) }
let(:group_1) { create(:group) } let(:group_1) { create(:group) }
let(:group_2) { create(:group) } let(:group_2) { create(:group) }
let(:group_3) { create(:group) } let(:group_3) { create(:group) }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment