summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--CHANGELOG1
-rw-r--r--app/controllers/projects/application_controller.rb3
-rw-r--r--app/helpers/diff_helper.rb8
-rw-r--r--app/services/issuable_base_service.rb28
-rw-r--r--app/views/projects/merge_requests/_show.html.haml2
-rw-r--r--doc/workflow/cherry_pick_changes.md3
-rw-r--r--spec/controllers/projects/merge_requests_controller_spec.rb8
-rw-r--r--spec/helpers/diff_helper_spec.rb20
-rw-r--r--spec/services/issues/bulk_update_service_spec.rb2
-rw-r--r--spec/services/issues/create_service_spec.rb63
-rw-r--r--spec/services/issues/update_service_spec.rb11
-rw-r--r--spec/services/merge_requests/update_service_spec.rb11
12 files changed, 124 insertions, 36 deletions
diff --git a/CHANGELOG b/CHANGELOG
index d78833d2204..d707c126c98 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -4,6 +4,7 @@ v 8.8.0 (unreleased)
v 8.7.0 (unreleased)
- Gitlab::GitAccess and Gitlab::GitAccessWiki are now instrumented
+ - Fix vulnerability that made it possible to gain access to private labels and milestones
- The number of InfluxDB points stored per UDP packet can now be configured
- Fix error when cross-project label reference used with non-existent project
- Transactions for /internal/allowed now have an "action" tag set
diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb
index 74150ad606b..be872a93fee 100644
--- a/app/controllers/projects/application_controller.rb
+++ b/app/controllers/projects/application_controller.rb
@@ -83,8 +83,7 @@ class Projects::ApplicationController < ApplicationController
end
def apply_diff_view_cookie!
- view = params[:view] || cookies[:diff_view]
- cookies.permanent[:diff_view] = params[:view] = view if view
+ cookies.permanent[:diff_view] = params.delete(:view) if params[:view].present?
end
def builds_enabled
diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb
index 6a3ec83b8c0..97466d532f4 100644
--- a/app/helpers/diff_helper.rb
+++ b/app/helpers/diff_helper.rb
@@ -9,7 +9,13 @@ module DiffHelper
end
def diff_view
- params[:view] == 'parallel' ? 'parallel' : 'inline'
+ diff_views = %w(inline parallel)
+
+ if diff_views.include?(cookies[:diff_view])
+ cookies[:diff_view]
+ else
+ diff_views.first
+ end
end
def diff_hard_limit_enabled?
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 18f76d3f650..2b16089df1b 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -37,8 +37,9 @@ class IssuableBaseService < BaseService
end
def filter_params(issuable_ability_name = :issue)
- params[:assignee_id] = "" if params[:assignee_id] == IssuableFinder::NONE
- params[:milestone_id] = "" if params[:milestone_id] == IssuableFinder::NONE
+ filter_assignee
+ filter_milestone
+ filter_labels
ability = :"admin_#{issuable_ability_name}"
@@ -49,6 +50,29 @@ class IssuableBaseService < BaseService
end
end
+ def filter_assignee
+ if params[:assignee_id] == IssuableFinder::NONE
+ params[:assignee_id] = ''
+ end
+ end
+
+ def filter_milestone
+ milestone_id = params[:milestone_id]
+ return unless milestone_id
+
+ if milestone_id == IssuableFinder::NONE ||
+ project.milestones.find_by(id: milestone_id).nil?
+ params[:milestone_id] = ''
+ end
+ end
+
+ def filter_labels
+ return if params[:label_ids].to_a.empty?
+
+ params[:label_ids] =
+ project.labels.where(id: params[:label_ids]).pluck(:id)
+ end
+
def update(issuable)
change_state(issuable)
filter_params
diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml
index 8d05060f563..290753d57c6 100644
--- a/app/views/projects/merge_requests/_show.html.haml
+++ b/app/views/projects/merge_requests/_show.html.haml
@@ -3,7 +3,7 @@
- page_card_attributes @merge_request.card_attributes
- header_title project_title(@project, "Merge Requests", namespace_project_merge_requests_path(@project.namespace, @project))
-- if params[:view] == 'parallel'
+- if diff_view == 'parallel'
- fluid_layout true
.merge-request{'data-url' => merge_request_path(@merge_request)}
diff --git a/doc/workflow/cherry_pick_changes.md b/doc/workflow/cherry_pick_changes.md
index b0ca0879643..4a499009842 100644
--- a/doc/workflow/cherry_pick_changes.md
+++ b/doc/workflow/cherry_pick_changes.md
@@ -1,6 +1,7 @@
# Cherry-pick changes
-_**Note:** This feature was [introduced][ce-3514] in GitLab 8.7._
+>**Note:**
+This feature was [introduced][ce-3514] in GitLab 8.7.
---
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index c54e83339a1..c0a1f45195f 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -300,14 +300,6 @@ describe Projects::MergeRequestsController do
expect(response.cookies['diff_view']).to eq('parallel')
end
-
- it 'assigns :view param based on cookie' do
- request.cookies['diff_view'] = 'parallel'
-
- go
-
- expect(controller.params[:view]).to eq 'parallel'
- end
end
describe 'GET commits' do
diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb
index 982c113e84b..b7810185d16 100644
--- a/spec/helpers/diff_helper_spec.rb
+++ b/spec/helpers/diff_helper_spec.rb
@@ -11,6 +11,26 @@ describe DiffHelper do
let(:diff_refs) { [commit.parent, commit] }
let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs) }
+ describe 'diff_view' do
+ it 'returns a valid value when cookie is set' do
+ helper.request.cookies[:diff_view] = 'parallel'
+
+ expect(helper.diff_view).to eq 'parallel'
+ end
+
+ it 'returns a default value when cookie is invalid' do
+ helper.request.cookies[:diff_view] = 'invalid'
+
+ expect(helper.diff_view).to eq 'inline'
+ end
+
+ it 'returns a default value when cookie is nil' do
+ expect(helper.request.cookies).to be_empty
+
+ expect(helper.diff_view).to eq 'inline'
+ end
+ end
+
describe 'diff_hard_limit_enabled?' do
it 'should return true if param is provided' do
allow(controller).to receive(:params) { { force_show_diff: true } }
diff --git a/spec/services/issues/bulk_update_service_spec.rb b/spec/services/issues/bulk_update_service_spec.rb
index 6a7ea4b2f44..e91906d0d49 100644
--- a/spec/services/issues/bulk_update_service_spec.rb
+++ b/spec/services/issues/bulk_update_service_spec.rb
@@ -100,7 +100,7 @@ describe Issues::BulkUpdateService, services: true do
describe :update_milestone do
before do
- @milestone = create :milestone
+ @milestone = create(:milestone, project: @project)
@params = {
issues_ids: [issue.id],
milestone_id: @milestone.id
diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb
index 5e7915db7e1..ac28b6f71f9 100644
--- a/spec/services/issues/create_service_spec.rb
+++ b/spec/services/issues/create_service_spec.rb
@@ -3,40 +3,75 @@ require 'spec_helper'
describe Issues::CreateService, services: true do
let(:project) { create(:empty_project) }
let(:user) { create(:user) }
- let(:assignee) { create(:user) }
- describe :execute do
- context 'valid params' do
+ describe '#execute' do
+ let(:issue) { described_class.new(project, user, opts).execute }
+
+ context 'when params are valid' do
+ let(:assignee) { create(:user) }
+ let(:milestone) { create(:milestone, project: project) }
+ let(:labels) { create_pair(:label, project: project) }
+
before do
project.team << [user, :master]
project.team << [assignee, :master]
+ end
- opts = {
- title: 'Awesome issue',
+ let(:opts) do
+ { title: 'Awesome issue',
description: 'please fix',
- assignee: assignee
- }
-
- @issue = Issues::CreateService.new(project, user, opts).execute
+ assignee: assignee,
+ label_ids: labels.map(&:id),
+ milestone_id: milestone.id }
end
- it { expect(@issue).to be_valid }
- it { expect(@issue.title).to eq('Awesome issue') }
- it { expect(@issue.assignee).to eq assignee }
+ it { expect(issue).to be_valid }
+ it { expect(issue.title).to eq('Awesome issue') }
+ it { expect(issue.assignee).to eq assignee }
+ it { expect(issue.labels).to match_array labels }
+ it { expect(issue.milestone).to eq milestone }
it 'creates a pending todo for new assignee' do
attributes = {
project: project,
author: user,
user: assignee,
- target_id: @issue.id,
- target_type: @issue.class.name,
+ target_id: issue.id,
+ target_type: issue.class.name,
action: Todo::ASSIGNED,
state: :pending
}
expect(Todo.where(attributes).count).to eq 1
end
+
+ context 'when label belongs to different project' do
+ let(:label) { create(:label) }
+
+ let(:opts) do
+ { title: 'Title',
+ description: 'Description',
+ label_ids: [label.id] }
+ end
+
+ it 'does not assign label'do
+ expect(issue.labels).to_not include label
+ end
+ end
+
+ context 'when milestone belongs to different project' do
+ let(:milestone) { create(:milestone) }
+
+ let(:opts) do
+ { title: 'Title',
+ description: 'Description',
+ milestone_id: milestone.id }
+ end
+
+ it 'does not assign milestone' do
+ expect(issue.milestone).to_not eq milestone
+ end
+ end
end
end
end
diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb
index 6b214a0d96b..52f69306994 100644
--- a/spec/services/issues/update_service_spec.rb
+++ b/spec/services/issues/update_service_spec.rb
@@ -4,10 +4,15 @@ describe Issues::UpdateService, services: true do
let(:user) { create(:user) }
let(:user2) { create(:user) }
let(:user3) { create(:user) }
- let(:issue) { create(:issue, title: 'Old title', assignee_id: user3.id) }
- let(:label) { create(:label) }
+ let(:project) { create(:empty_project) }
+ let(:label) { create(:label, project: project) }
let(:label2) { create(:label) }
- let(:project) { issue.project }
+
+ let(:issue) do
+ create(:issue, title: 'Old title',
+ assignee_id: user3.id,
+ project: project)
+ end
before do
project.team << [user, :master]
diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb
index cb8cff2fa8c..213e8c2eb3a 100644
--- a/spec/services/merge_requests/update_service_spec.rb
+++ b/spec/services/merge_requests/update_service_spec.rb
@@ -1,14 +1,19 @@
require 'spec_helper'
describe MergeRequests::UpdateService, services: true do
+ let(:project) { create(:project) }
let(:user) { create(:user) }
let(:user2) { create(:user) }
let(:user3) { create(:user) }
- let(:merge_request) { create(:merge_request, :simple, title: 'Old title', assignee_id: user3.id) }
- let(:project) { merge_request.project }
- let(:label) { create(:label) }
+ let(:label) { create(:label, project: project) }
let(:label2) { create(:label) }
+ let(:merge_request) do
+ create(:merge_request, :simple, title: 'Old title',
+ assignee_id: user3.id,
+ source_project: project)
+ end
+
before do
project.team << [user, :master]
project.team << [user2, :developer]
View Lorry Controller Status