Commit 8f4b0613 authored by Robert Speicher's avatar Robert Speicher Committed by Stan Hu

Merge branch 'milestones-finder-order-fix' into 'security-10-3'

Remove order param from the MilestoneFinder

See merge request gitlab/gitlabhq!2259

(cherry picked from commit 14408042e78f2ebc2644f956621b461dbfa3d36d)

155881e7 Remove order param from the MilestoneFinder
parent 6846b70d
...@@ -75,8 +75,6 @@ class Groups::MilestonesController < Groups::ApplicationController ...@@ -75,8 +75,6 @@ class Groups::MilestonesController < Groups::ApplicationController
end end
def milestones def milestones
search_params = params.merge(group_ids: group.id)
milestones = MilestonesFinder.new(search_params).execute milestones = MilestonesFinder.new(search_params).execute
legacy_milestones = GroupMilestone.build_collection(group, group_projects, params) legacy_milestones = GroupMilestone.build_collection(group, group_projects, params)
...@@ -94,4 +92,8 @@ class Groups::MilestonesController < Groups::ApplicationController ...@@ -94,4 +92,8 @@ class Groups::MilestonesController < Groups::ApplicationController
render_404 unless @milestone render_404 unless @milestone
end end
def search_params
params.permit(:state).merge(group_ids: group.id)
end
end end
...@@ -92,12 +92,6 @@ class Projects::MilestonesController < Projects::ApplicationController ...@@ -92,12 +92,6 @@ class Projects::MilestonesController < Projects::ApplicationController
def milestones def milestones
@milestones ||= begin @milestones ||= begin
if @project.group && can?(current_user, :read_group, @project.group)
group = @project.group
end
search_params = params.merge(project_ids: @project.id, group_ids: group&.id)
MilestonesFinder.new(search_params).execute MilestonesFinder.new(search_params).execute
end end
end end
...@@ -113,4 +107,12 @@ class Projects::MilestonesController < Projects::ApplicationController ...@@ -113,4 +107,12 @@ class Projects::MilestonesController < Projects::ApplicationController
def milestone_params def milestone_params
params.require(:milestone).permit(:title, :description, :start_date, :due_date, :state_event) params.require(:milestone).permit(:title, :description, :start_date, :due_date, :state_event)
end end
def search_params
if @project.group && can?(current_user, :read_group, @project.group)
group = @project.group
end
params.permit(:state).merge(project_ids: @project.id, group_ids: group&.id)
end
end end
...@@ -46,11 +46,7 @@ class MilestonesFinder ...@@ -46,11 +46,7 @@ class MilestonesFinder
end end
def order(items) def order(items)
if params.has_key?(:order) order_statement = Gitlab::Database.nulls_last_order('due_date', 'ASC')
items.reorder(params[:order]) items.reorder(order_statement).order('title ASC')
else
order_statement = Gitlab::Database.nulls_last_order('due_date', 'ASC')
items.reorder(order_statement)
end
end end
end end
...@@ -44,10 +44,10 @@ class GlobalMilestone ...@@ -44,10 +44,10 @@ class GlobalMilestone
def self.group_milestones_states_count(group) def self.group_milestones_states_count(group)
return STATE_COUNT_HASH unless group return STATE_COUNT_HASH unless group
params = { group_ids: [group.id], state: 'all', order: nil } params = { group_ids: [group.id], state: 'all' }
relation = MilestonesFinder.new(params).execute relation = MilestonesFinder.new(params).execute
grouped_by_state = relation.group(:state).count grouped_by_state = relation.reorder(nil).group(:state).count
{ {
opened: grouped_by_state['active'] || 0, opened: grouped_by_state['active'] || 0,
...@@ -60,10 +60,10 @@ class GlobalMilestone ...@@ -60,10 +60,10 @@ class GlobalMilestone
def self.legacy_group_milestone_states_count(projects) def self.legacy_group_milestone_states_count(projects)
return STATE_COUNT_HASH unless projects return STATE_COUNT_HASH unless projects
params = { project_ids: projects.map(&:id), state: 'all', order: nil } params = { project_ids: projects.map(&:id), state: 'all' }
relation = MilestonesFinder.new(params).execute relation = MilestonesFinder.new(params).execute
project_milestones_by_state_and_title = relation.group(:state, :title).count project_milestones_by_state_and_title = relation.reorder(nil).group(:state, :title).count
opened = count_by_state(project_milestones_by_state_and_title, 'active') opened = count_by_state(project_milestones_by_state_and_title, 'active')
closed = count_by_state(project_milestones_by_state_and_title, 'closed') closed = count_by_state(project_milestones_by_state_and_title, 'closed')
......
---
title: Prevent a SQL injection in the MilestonesFinder
merge_request:
author:
type: security
...@@ -21,10 +21,19 @@ describe MilestonesFinder do ...@@ -21,10 +21,19 @@ describe MilestonesFinder do
expect(result).to contain_exactly(milestone_1, milestone_2) expect(result).to contain_exactly(milestone_1, milestone_2)
end end
it 'returns milestones for groups and projects' do context 'milestones for groups and project' do
result = described_class.new(project_ids: [project_1.id, project_2.id], group_ids: group.id, state: 'all').execute let(:result) do
described_class.new(project_ids: [project_1.id, project_2.id], group_ids: group.id, state: 'all').execute
end
it 'returns milestones for groups and projects' do
expect(result).to contain_exactly(milestone_1, milestone_2, milestone_3, milestone_4)
end
expect(result).to contain_exactly(milestone_1, milestone_2, milestone_3, milestone_4) it 'orders milestones by due date' do
expect(result.first).to eq(milestone_1)
expect(result.second).to eq(milestone_3)
end
end end
context 'with filters' do context 'with filters' do
...@@ -61,30 +70,4 @@ describe MilestonesFinder do ...@@ -61,30 +70,4 @@ describe MilestonesFinder do
expect(result.to_a).to contain_exactly(milestone_1) expect(result.to_a).to contain_exactly(milestone_1)
end end
end end
context 'with order' do
let(:params) do
{
project_ids: [project_1.id, project_2.id],
group_ids: group.id,
state: 'all'
}
end
it "default orders by due date" do
result = described_class.new(params).execute
expect(result.first).to eq(milestone_1)
expect(result.second).to eq(milestone_3)
end
it "orders by parameter" do
result = described_class.new(params.merge(order: 'id DESC')).execute
expect(result.first).to eq(milestone_4)
expect(result.second).to eq(milestone_3)
expect(result.third).to eq(milestone_2)
expect(result.fourth).to eq(milestone_1)
end
end
end end
...@@ -93,26 +93,27 @@ describe Projects::AutocompleteService do ...@@ -93,26 +93,27 @@ describe Projects::AutocompleteService do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:group) { create(:group) } let(:group) { create(:group) }
let(:project) { create(:project, group: group) } let(:project) { create(:project, group: group) }
let!(:group_milestone) { create(:milestone, group: group) } let!(:group_milestone1) { create(:milestone, group: group, due_date: '2017-01-01', title: 'Second Title') }
let!(:project_milestone) { create(:milestone, project: project) } let!(:group_milestone2) { create(:milestone, group: group, due_date: '2017-01-01', title: 'First Title') }
let!(:project_milestone) { create(:milestone, project: project, due_date: '2016-01-01') }
let(:milestone_titles) { described_class.new(project, user).milestones.map(&:title) } let(:milestone_titles) { described_class.new(project, user).milestones.map(&:title) }
it 'includes project and group milestones' do it 'includes project and group milestones and sorts them correctly' do
expect(milestone_titles).to eq([group_milestone.title, project_milestone.title]) expect(milestone_titles).to eq([project_milestone.title, group_milestone2.title, group_milestone1.title])
end end
it 'does not include closed milestones' do it 'does not include closed milestones' do
group_milestone.close group_milestone1.close
expect(milestone_titles).to eq([project_milestone.title]) expect(milestone_titles).to eq([project_milestone.title, group_milestone2.title])
end end
it 'does not include milestones from other projects in the group' do it 'does not include milestones from other projects in the group' do
other_project = create(:project, group: group) other_project = create(:project, group: group)
project_milestone.update!(project: other_project) project_milestone.update!(project: other_project)
expect(milestone_titles).to eq([group_milestone.title]) expect(milestone_titles).to eq([group_milestone2.title, group_milestone1.title])
end end
end end
end end
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