summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/controllers/projects/milestones_controller.rb21
-rw-r--r--app/helpers/milestones_helper.rb13
-rw-r--r--app/views/shared/milestones/_milestone.html.haml4
-rw-r--r--changelogs/unreleased/security-issue_51301.yml5
-rw-r--r--spec/controllers/projects/milestones_controller_spec.rb33
-rw-r--r--spec/features/milestones/user_promotes_milestone_spec.rb32
6 files changed, 96 insertions, 12 deletions
diff --git a/app/controllers/projects/milestones_controller.rb b/app/controllers/projects/milestones_controller.rb
index 20998c97730..8e68014a30d 100644
--- a/app/controllers/projects/milestones_controller.rb
+++ b/app/controllers/projects/milestones_controller.rb
@@ -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)
diff --git a/app/helpers/milestones_helper.rb b/app/helpers/milestones_helper.rb
index 94a030d9d57..9666080092b 100644
--- a/app/helpers/milestones_helper.rb
+++ b/app/helpers/milestones_helper.rb
@@ -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
diff --git a/app/views/shared/milestones/_milestone.html.haml b/app/views/shared/milestones/_milestone.html.haml
index 3dd2842be4f..ed7fefba56d 100644
--- a/app/views/shared/milestones/_milestone.html.haml
+++ b/app/views/shared/milestones/_milestone.html.haml
@@ -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',
diff --git a/changelogs/unreleased/security-issue_51301.yml b/changelogs/unreleased/security-issue_51301.yml
new file mode 100644
index 00000000000..cf8ebb54b1c
--- /dev/null
+++ b/changelogs/unreleased/security-issue_51301.yml
@@ -0,0 +1,5 @@
+---
+title: Fix milestone promotion authorization check
+merge_request:
+author:
+type: security
diff --git a/spec/controllers/projects/milestones_controller_spec.rb b/spec/controllers/projects/milestones_controller_spec.rb
index 3190f1ce9d4..500df0ca9b6 100644
--- a/spec/controllers/projects/milestones_controller_spec.rb
+++ b/spec/controllers/projects/milestones_controller_spec.rb
@@ -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
diff --git a/spec/features/milestones/user_promotes_milestone_spec.rb b/spec/features/milestones/user_promotes_milestone_spec.rb
new file mode 100644
index 00000000000..df1bc502134
--- /dev/null
+++ b/spec/features/milestones/user_promotes_milestone_spec.rb
@@ -0,0 +1,32 @@
+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