Commit d83bafe5 authored by Cindy Pallares's avatar Cindy Pallares

Merge branch 'security-issue_51301' into 'master'

[master] Resolve: Promoting a milestone is missing an authorization check

See merge request gitlab/gitlabhq!2598
parents 7de01597 6dcfae54
...@@ -11,7 +11,10 @@ class Projects::MilestonesController < Projects::ApplicationController ...@@ -11,7 +11,10 @@ class Projects::MilestonesController < Projects::ApplicationController
before_action :authorize_read_milestone! before_action :authorize_read_milestone!
# Allow admin milestone # Allow admin milestone
before_action :authorize_admin_milestone!, except: [:index, :show, :merge_requests, :participants, :labels, :promote] before_action :authorize_admin_milestone!, except: [:index, :show, :merge_requests, :participants, :labels]
# Allow to promote milestone
before_action :authorize_promote_milestone!, only: :promote
respond_to :html respond_to :html
...@@ -78,7 +81,7 @@ class Projects::MilestonesController < Projects::ApplicationController ...@@ -78,7 +81,7 @@ class Projects::MilestonesController < Projects::ApplicationController
def promote def promote
promoted_milestone = Milestones::PromoteService.new(project, current_user).execute(milestone) promoted_milestone = Milestones::PromoteService.new(project, current_user).execute(milestone)
flash[:notice] = flash_notice_for(promoted_milestone, project.group) flash[:notice] = flash_notice_for(promoted_milestone, project_group)
respond_to do |format| respond_to do |format|
format.html do format.html do
...@@ -109,6 +112,12 @@ class Projects::MilestonesController < Projects::ApplicationController ...@@ -109,6 +112,12 @@ class Projects::MilestonesController < Projects::ApplicationController
protected protected
def project_group
strong_memoize(:project_group) do
project.group
end
end
def milestones def milestones
strong_memoize(:milestones) do strong_memoize(:milestones) do
MilestonesFinder.new(search_params).execute MilestonesFinder.new(search_params).execute
...@@ -125,13 +134,17 @@ class Projects::MilestonesController < Projects::ApplicationController ...@@ -125,13 +134,17 @@ class Projects::MilestonesController < Projects::ApplicationController
return render_404 unless can?(current_user, :admin_milestone, @project) return render_404 unless can?(current_user, :admin_milestone, @project)
end end
def authorize_promote_milestone!
return render_404 unless can?(current_user, :admin_milestone, project_group)
end
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 def search_params
if request.format.json? && @project.group && can?(current_user, :read_group, @project.group) if request.format.json? && project_group && can?(current_user, :read_group, project_group)
groups = @project.group.self_and_ancestors_ids groups = project_group.self_and_ancestors_ids
end end
params.permit(:state).merge(project_ids: @project.id, group_ids: groups) params.permit(:state).merge(project_ids: @project.id, group_ids: groups)
......
...@@ -2,6 +2,7 @@ ...@@ -2,6 +2,7 @@
module MilestonesHelper module MilestonesHelper
include EntityDateHelper include EntityDateHelper
include Gitlab::Utils::StrongMemoize
def milestones_filter_path(opts = {}) def milestones_filter_path(opts = {})
if @project if @project
...@@ -243,4 +244,16 @@ module MilestonesHelper ...@@ -243,4 +244,16 @@ module MilestonesHelper
dashboard_milestone_path(milestone.safe_title, title: milestone.title) dashboard_milestone_path(milestone.safe_title, title: milestone.title)
end end
end end
def can_admin_project_milestones?
strong_memoize(:can_admin_project_milestones) do
can?(current_user, :admin_milestone, @project)
end
end
def can_admin_group_milestones?
strong_memoize(:can_admin_group_milestones) do
can?(current_user, :admin_milestone, @project.group)
end
end
end end
...@@ -35,8 +35,8 @@ ...@@ -35,8 +35,8 @@
.col-sm-2 .col-sm-2
.milestone-actions.d-flex.justify-content-sm-start.justify-content-md-end .milestone-actions.d-flex.justify-content-sm-start.justify-content-md-end
- if @project - if @project
- if can?(current_user, :admin_milestone, milestone.project) and milestone.active? - if can_admin_project_milestones? and milestone.active?
- if @project.group - if can_admin_group_milestones?
%button.js-promote-project-milestone-button.btn.btn-blank.btn-sm.btn-grouped.has-tooltip{ title: _('Promote to Group Milestone'), %button.js-promote-project-milestone-button.btn.btn-blank.btn-sm.btn-grouped.has-tooltip{ title: _('Promote to Group Milestone'),
disabled: true, disabled: true,
type: 'button', type: 'button',
......
---
title: Fix milestone promotion authorization check
merge_request:
author:
type: security
...@@ -143,11 +143,27 @@ describe Projects::MilestonesController do ...@@ -143,11 +143,27 @@ describe Projects::MilestonesController do
end end
describe '#promote' do describe '#promote' do
let(:group) { create(:group) }
before do
project.update(namespace: group)
end
context 'when user does not have permission to promote milestone' do
before do
group.add_guest(user)
end
it 'renders 404' do
post :promote, namespace_id: project.namespace.id, project_id: project.id, id: milestone.iid
expect(response).to have_gitlab_http_status(404)
end
end
context 'promotion succeeds' do context 'promotion succeeds' do
before do before do
group = create(:group)
group.add_developer(user) group.add_developer(user)
milestone.project.update(namespace: group)
end end
it 'shows group milestone' do it 'shows group milestone' do
...@@ -166,12 +182,17 @@ describe Projects::MilestonesController do ...@@ -166,12 +182,17 @@ describe Projects::MilestonesController do
end end
end end
context 'promotion fails' do context 'when user cannot admin group milestones' do
it 'shows project milestone' do before do
project.add_developer(user)
end
it 'renders 404' do
project.update(namespace: user.namespace)
post :promote, namespace_id: project.namespace.id, project_id: project.id, id: milestone.iid post :promote, namespace_id: project.namespace.id, project_id: project.id, id: milestone.iid
expect(response).to redirect_to(project_milestone_path(project, milestone)) expect(response).to have_gitlab_http_status(404)
expect(flash[:alert]).to eq('Promotion failed - Project does not belong to a group.')
end end
end end
end end
......
require 'rails_helper'
describe 'User promotes milestone' do
set(:group) { create(:group) }
set(:user) { create(:user) }
set(:project) { create(:project, namespace: group) }
set(:milestone) { create(:milestone, project: project) }
context 'when user can admin group milestones' do
before do
group.add_developer(user)
sign_in(user)
visit(project_milestones_path(project))
end
it "shows milestone promote button" do
expect(page).to have_selector('.js-promote-project-milestone-button')
end
end
context 'when user cannot admin group milestones' do
before do
project.add_developer(user)
sign_in(user)
visit(project_milestones_path(project))
end
it "does not show milestone promote button" do
expect(page).not_to have_selector('.js-promote-project-milestone-button')
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