Merge branch 'security-issue_51301' into 'master'

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

See merge request gitlab/gitlabhq!2598
parent 94ab2d5f
......@@ -11,7 +11,10 @@ class Projects::MilestonesController < Projects::ApplicationController
before_action :authorize_read_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
......@@ -78,7 +81,7 @@ class Projects::MilestonesController < Projects::ApplicationController
def promote
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|
format.html do
......@@ -109,6 +112,12 @@ class Projects::MilestonesController < Projects::ApplicationController
protected
def project_group
strong_memoize(:project_group) do
project.group
end
end
def milestones
strong_memoize(:milestones) do
MilestonesFinder.new(search_params).execute
......@@ -125,13 +134,17 @@ class Projects::MilestonesController < Projects::ApplicationController
return render_404 unless can?(current_user, :admin_milestone, @project)
end
def authorize_promote_milestone!
return render_404 unless can?(current_user, :admin_milestone, project_group)
end
def milestone_params
params.require(:milestone).permit(:title, :description, :start_date, :due_date, :state_event)
end
def search_params
if request.format.json? && @project.group && can?(current_user, :read_group, @project.group)
groups = @project.group.self_and_ancestors_ids
if request.format.json? && project_group && can?(current_user, :read_group, project_group)
groups = project_group.self_and_ancestors_ids
end
params.permit(:state).merge(project_ids: @project.id, group_ids: groups)
......
......@@ -2,6 +2,7 @@
module MilestonesHelper
include EntityDateHelper
include Gitlab::Utils::StrongMemoize
def milestones_filter_path(opts = {})
if @project
......@@ -243,4 +244,16 @@ module MilestonesHelper
dashboard_milestone_path(milestone.safe_title, title: milestone.title)
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
......@@ -35,8 +35,8 @@
.col-sm-2
.milestone-actions.d-flex.justify-content-sm-start.justify-content-md-end
- if @project
- if can?(current_user, :admin_milestone, milestone.project) and milestone.active?
- if @project.group
- if can_admin_project_milestones? and milestone.active?
- 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'),
disabled: true,
type: 'button',
......
---
title: Fix milestone promotion authorization check
merge_request:
author:
type: security
......@@ -143,11 +143,27 @@ describe Projects::MilestonesController do
end
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
before do
group = create(:group)
group.add_developer(user)
milestone.project.update(namespace: group)
end
it 'shows group milestone' do
......@@ -166,12 +182,17 @@ describe Projects::MilestonesController do
end
end
context 'promotion fails' do
it 'shows project milestone' do
context 'when user cannot admin group milestones' 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
expect(response).to redirect_to(project_milestone_path(project, milestone))
expect(flash[:alert]).to eq('Promotion failed - Project does not belong to a group.')
expect(response).to have_gitlab_http_status(404)
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