Commit 8721c9cd authored by Robert Speicher's avatar Robert Speicher Committed by Oswaldo Ferreira

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

Remove order param from the MilestoneFinder

See merge request gitlab/gitlabhq!2259
parent 15e6d87f
......@@ -75,8 +75,6 @@ class Groups::MilestonesController < Groups::ApplicationController
end
def milestones
search_params = params.merge(group_ids: group.id)
milestones = MilestonesFinder.new(search_params).execute
legacy_milestones =
......@@ -100,4 +98,8 @@ class Groups::MilestonesController < Groups::ApplicationController
render_404 unless @milestone
end
def search_params
params.permit(:state).merge(group_ids: group.id)
end
end
......@@ -97,12 +97,6 @@ class Projects::MilestonesController < Projects::ApplicationController
def milestones
@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
end
end
......@@ -118,4 +112,12 @@ class Projects::MilestonesController < Projects::ApplicationController
def milestone_params
params.require(:milestone).permit(:title, :description, :start_date, :due_date, :state_event)
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
......@@ -46,11 +46,7 @@ class MilestonesFinder
end
def order(items)
if params.has_key?(:order)
items.reorder(params[:order])
else
order_statement = Gitlab::Database.nulls_last_order('due_date', 'ASC')
items.reorder(order_statement)
end
order_statement = Gitlab::Database.nulls_last_order('due_date', 'ASC')
items.reorder(order_statement).order('title ASC')
end
end
......@@ -46,10 +46,10 @@ class GlobalMilestone
def self.group_milestones_states_count(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
grouped_by_state = relation.group(:state).count
grouped_by_state = relation.reorder(nil).group(:state).count
{
opened: grouped_by_state['active'] || 0,
......@@ -62,10 +62,10 @@ class GlobalMilestone
def self.legacy_group_milestone_states_count(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
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')
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
expect(result).to contain_exactly(milestone_1, milestone_2)
end
it 'returns milestones for groups and projects' do
result = described_class.new(project_ids: [project_1.id, project_2.id], group_ids: group.id, state: 'all').execute
context 'milestones for groups and project' do
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
context 'with filters' do
......@@ -61,30 +70,4 @@ describe MilestonesFinder do
expect(result.to_a).to contain_exactly(milestone_1)
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
......@@ -93,26 +93,27 @@ describe Projects::AutocompleteService do
let(:user) { create(:user) }
let(:group) { create(:group) }
let(:project) { create(:project, group: group) }
let!(:group_milestone) { create(:milestone, group: group) }
let!(:project_milestone) { create(:milestone, project: project) }
let!(:group_milestone1) { create(:milestone, group: group, due_date: '2017-01-01', title: 'Second Title') }
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) }
it 'includes project and group milestones' do
expect(milestone_titles).to eq([group_milestone.title, project_milestone.title])
it 'includes project and group milestones and sorts them correctly' do
expect(milestone_titles).to eq([project_milestone.title, group_milestone2.title, group_milestone1.title])
end
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
it 'does not include milestones from other projects in the group' do
other_project = create(:project, group: group)
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
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