Fix milestone promotion authorization

Promoting milestone was missing an authorization check, guest
users were being able to promote project milestones to group milestones.

6 files changed, 96 insertions, 12 deletions

diff --git a/app/controllers/projects/milestones_controller.rb b/app/controllers/projects/milestones_controller.rb
index b9b3dcd5a85..445a4867799 100644
--- a/app/controllers/projects/milestones_controller.rb
+++ b/app/controllers/projects/milestones_controller.rb
@@ -9,7 +9,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
 
@@ -76,7 +79,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
@@ -112,6 +115,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
@@ -126,13 +135,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 95da8f00aff..df949478239 100644
--- a/app/helpers/milestones_helper.rb
+++ b/app/helpers/milestones_helper.rb
@@ -1,5 +1,6 @@
 module MilestonesHelper
   include EntityDateHelper
+  include Gitlab::Utils::StrongMemoize
 
   def milestones_filter_path(opts = {})
     if @project
@@ -241,4 +242,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