Welcome to mirror list, hosted at ThFree Co, Russian Federation.

gitlab.com/gitlab-org/gitlab-foss.git - Unnamed repository; edit this file 'description' to name the repository.
summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDouwe Maan <douwe@gitlab.com>2016-11-10 13:23:44 +0300
committerRémy Coutable <remy@rymai.me>2016-11-24 19:36:02 +0300
commitcf262a102f4c8255b0c947cf1c704c34557035c9 (patch)
treef913310f1a785efa4b1a573a2e8e6dd1695ac511
parentb73edc5e171d50d86135ebb9e89b5eb46a232279 (diff)
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
-rw-r--r--app/services/issuable_base_service.rb8
-rw-r--r--app/services/labels/find_or_create_service.rb7
-rw-r--r--changelogs/unreleased/zj-fix-label-creation-non-members.yml4
-rw-r--r--lib/api/helpers.rb14
-rw-r--r--lib/api/issues.rb74
-rw-r--r--lib/api/merge_requests.rb49
-rw-r--r--spec/requests/api/issues_spec.rb14
-rw-r--r--spec/requests/api/merge_requests_spec.rb29
-rw-r--r--spec/services/labels/transfer_service_spec.rb2
9 files changed, 99 insertions, 102 deletions
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index bb92cd80cc9..0511d1dbf3c 100644
--- a/app/services/issuable_base_service.rb
+++ b/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)
diff --git a/app/services/labels/find_or_create_service.rb b/app/services/labels/find_or_create_service.rb
index d622f9edd33..cf4f7606c94 100644
--- a/app/services/labels/find_or_create_service.rb
+++ b/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
diff --git a/changelogs/unreleased/zj-fix-label-creation-non-members.yml b/changelogs/unreleased/zj-fix-label-creation-non-members.yml
new file mode 100644
index 00000000000..ae4824f82fa
--- /dev/null
+++ b/changelogs/unreleased/zj-fix-label-creation-non-members.yml
@@ -0,0 +1,4 @@
+---
+title: Non members cannot create labels through the API
+merge_request:
+author:
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 3c9d7b1aaef..2a88b55aef3 100644
--- a/lib/api/helpers.rb
+++ b/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.
#
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index c9689e6f8ef..5edcacfeae8 100644
--- a/lib/api/issues.rb
+++ b/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
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index bf8504e1101..20bb9e73dc6 100644
--- a/lib/api/merge_requests.rb
+++ b/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
diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb
index beed53d1e5c..8f695438ea2 100644
--- a/spec/requests/api/issues_spec.rb
+++ b/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
diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb
index b813ee967f8..d5303dfc130 100644
--- a/spec/requests/api/merge_requests_spec.rb
+++ b/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
diff --git a/spec/services/labels/transfer_service_spec.rb b/spec/services/labels/transfer_service_spec.rb
index ddf3527dc0f..13654a0881c 100644
--- a/spec/services/labels/transfer_service_spec.rb
+++ b/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) }