Merge branch 'osw-10-3-security-milestone-data-exposure' into 'security-10-3-ee'

[10.3] Check milestone access before persisting it on boards

See merge request gitlab/gitlab-ee!566

app/services/boards/base_service.rb

@@ -11,5 +11,26 @@ module Boards
       assignee = User.find_by(id: params.delete(:assignee_id))
       params.merge!(assignee: assignee)
     end
+
+    def set_milestone
+      milestone_id = params[:milestone_id]
+
+      return unless milestone_id
+      return if [::Milestone::None.id,
+                 ::Milestone::Upcoming.id,
+                 ::Milestone::Started.id].include?(milestone_id)
+
+      finder_params =
+        case parent
+        when Group
+          { group_ids: [parent.id] }
+        when Project
+          { project_ids: [parent.id], group_ids: [parent.group&.id] }
+        end
+
+      milestone = MilestonesFinder.new(finder_params).execute.find_by_id(milestone_id)
+
+      params[:milestone_id] = milestone&.id
+    end
   end
 end

app/services/boards/create_service.rb

@@ -14,6 +14,8 @@ module Boards
 
     def create_board!
       set_assignee
+      set_milestone
+
       board = parent.boards.create(params)
 
       if board.persisted?

changelogs/unreleased-ee/osw-10-3-security-milestone-data-exposure.yml

+---
+title: Deny persisting milestones from outside project/group scope on boards
+merge_request:
+author:
+type: security

ee/app/services/boards/update_service.rb

@@ -9,6 +9,7 @@ module Boards
       end
 
       set_assignee
+      set_milestone
 
       board.update(params)
     end

spec/ee/spec/controllers/projects/boards_controller_spec.rb

@@ -37,7 +37,7 @@ describe Projects::BoardsController do
 
       context 'with valid params' do
         let(:user) { create(:user) }
-        let(:milestone) { create(:milestone) }
+        let(:milestone) { create(:milestone, project: project) }
         let(:label) { create(:label) }
 
         let(:create_params) do
@@ -113,7 +113,7 @@ describe Projects::BoardsController do
   describe 'PATCH update' do
     let(:board) { create(:board, project: project, name: 'Backend') }
     let(:user) { create(:user) }
-    let(:milestone) { create(:milestone) }
+    let(:milestone) { create(:milestone, project: project) }
     let(:label) { create(:label) }
 
     let(:update_params) do

spec/ee/spec/services/boards/create_service_spec.rb

 require 'spec_helper'
 
 describe Boards::CreateService, services: true do
+  shared_examples 'board with milestone predefined scope' do
+    let(:project) { create(:project) }
+
+    it 'creates board with correct milestone' do
+      stub_licensed_features(scoped_issue_board: true)
+
+      board = described_class
+        .new(project, double, milestone_id: milestone_class.id)
+        .execute
+
+      expect(board.reload.milestone).to eq(milestone_class)
+    end
+  end
+
   shared_examples 'boards create service' do
     context 'With the feature available' do
       before do
@@ -71,5 +85,77 @@ describe Boards::CreateService, services: true do
     it_behaves_like 'boards create service' do
       let(:parent) { create(:group) }
     end
+
+    it_behaves_like 'board with milestone predefined scope' do
+      let(:milestone_class) { ::Milestone::Upcoming }
+    end
+
+    it_behaves_like 'board with milestone predefined scope' do
+      let(:milestone_class) { ::Milestone::Started }
+    end
+
+    context 'group board milestone' do
+      let(:group) { create(:group) }
+      let!(:milestone) { create(:milestone) }
+
+      before do
+        stub_licensed_features(scoped_issue_board: true)
+      end
+
+      it 'is not persisted if it is not within group milestones' do
+        service = described_class.new(group, double, milestone_id: milestone.id)
+
+        group_board = service.execute
+
+        expect(group_board.milestone).to be_nil
+      end
+
+      it 'is persisted if it is within group milestones' do
+        milestone.update!(project: nil, group: group)
+        service = described_class.new(group, double, milestone_id: milestone.id)
+
+        group_board = service.execute
+
+        expect(group_board.reload.milestone).to eq(milestone)
+      end
+    end
+
+    context 'project board milestone' do
+      let(:project) { create(:project, :private) }
+      let!(:milestone) { create(:milestone) }
+
+      before do
+        stub_licensed_features(scoped_issue_board: true)
+      end
+
+      it 'is not persisted if it is not within project milestones' do
+        service = described_class.new(project, double, milestone_id: milestone.id)
+
+        board = service.execute
+
+        expect(board.reload.milestone).to be_nil
+      end
+
+      it 'is persisted if it is within project milestones' do
+        milestone.update!(project: project)
+        service = described_class.new(project, double, milestone_id: milestone.id)
+
+        board = service.execute
+
+        expect(board.reload.milestone).to eq(milestone)
+      end
+
+      it 'is persisted if it is within project group milestones' do
+        project_group = create(:group)
+        project.update(group: project_group)
+        milestone.update!(project: nil, group: project_group)
+
+        service = described_class.new(project_group, double, milestone_id: milestone.id)
+
+        board = service.execute
+
+        expect(board.reload.milestone).to eq(milestone)
+      end
+    end
   end
 end

spec/ee/spec/services/boards/update_service_spec.rb

 require 'spec_helper'
 
 describe Boards::UpdateService, services: true do
+  shared_examples 'board with milestone predefined scope' do
+    let(:project) { create(:project) }
+    let!(:board)  { create(:board) }
+
+    it 'updates board to milestone id' do
+      stub_licensed_features(scoped_issue_board: true)
+
+      described_class
+        .new(project, double, milestone_id: milestone_class.id)
+        .execute(board)
+
+      expect(board.reload.milestone).to eq(milestone_class)
+    end
+  end
+
   describe '#execute' do
+    let(:project) { create(:project) }
     let(:group) { create(:group) }
     let!(:board)  { create(:board, group: group, name: 'Backend') }
 
@@ -24,5 +40,106 @@ describe Boards::UpdateService, services: true do
 
       expect(service.execute(board)).to eq false
     end
+
+    it 'updates the configuration params when scoped issue board is enabled' do
+      stub_licensed_features(scoped_issue_board: true)
+      assignee = create(:user)
+      milestone = create(:milestone, project: project)
+      label = create(:label, project: project)
+
+      service = described_class.new(project, double,
+                                    milestone_id: milestone.id,
+                                    assignee_id: assignee.id,
+                                    label_ids: [label.id])
+      service.execute(board)
+
+      expect(board.reload).to have_attributes(milestone: milestone,
+                                              assignee: assignee,
+                                              labels: [label])
+    end
+
+    it 'filters unpermitted params when scoped issue board is not enabled' do
+      stub_licensed_features(scoped_issue_board: false)
+      params = { milestone_id: double, assignee_id: double, label_ids: double, weight: double }
+
+      service = described_class.new(project, double, params)
+      service.execute(board)
+
+      expect(board.reload).to have_attributes(milestone: nil,
+                                              assignee: nil,
+                                              labels: [])
+    end
+
+    it_behaves_like 'board with milestone predefined scope' do
+      let(:milestone_class) { ::Milestone::Upcoming }
+    end
+
+    it_behaves_like 'board with milestone predefined scope' do
+      let(:milestone_class) { ::Milestone::Started }
+    end
+
+    context 'group board milestone' do
+      let(:group) { create(:group) }
+      let(:group_board) { create(:board, group: group, name: 'Backend Group') }
+      let!(:milestone) { create(:milestone) }
+
+      before do
+        stub_licensed_features(scoped_issue_board: true)
+      end
+
+      it 'is not updated if it is not within group milestones' do
+        service = described_class.new(group, double, milestone_id: milestone.id)
+
+        service.execute(group_board)
+
+        expect(group_board.reload.milestone).to be_nil
+      end
+
+      it 'is updated if it is within group milestones' do
+        milestone.update!(project: nil, group: group)
+        service = described_class.new(group, double, milestone_id: milestone.id)
+
+        service.execute(group_board)
+
+        expect(group_board.reload.milestone).to eq(milestone)
+      end
+    end
+
+    context 'project board milestone' do
+      let!(:milestone) { create(:milestone) }
+
+      before do
+        stub_licensed_features(scoped_issue_board: true)
+      end
+
+      it 'is not updated if it is not within project milestones' do
+        service = described_class.new(project, double, milestone_id: milestone.id)
+
+        service.execute(board)
+
+        expect(board.reload.milestone).to be_nil
+      end
+
+      it 'is updated if it is within project milestones' do
+        milestone.update!(project: project)
+        service = described_class.new(project, double, milestone_id: milestone.id)
+
+        service.execute(board)
+
+        expect(board.reload.milestone).to eq(milestone)
+      end
+
+      it 'is updated if it is within project group milestones' do
+        project_group = create(:group)
+        project.update(group: project_group)
+        milestone.update!(project: nil, group: project_group)
+
+        service = described_class.new(project_group, double, milestone_id: milestone.id)
+
+        service.execute(board)
+
+        expect(board.reload.milestone).to eq(milestone)
+      end
+    end
   end
 end

spec/services/boards/update_service_spec.rb

@@ -24,34 +24,5 @@ describe Boards::UpdateService do
 
       expect(service.execute(board)).to eq false
     end
-
-    it 'updates the configuration params when scoped issue board is enabled' do
-      stub_licensed_features(scoped_issue_board: true)
-      assignee = create(:user)
-      milestone = create(:milestone, project: project)
-      label = create(:label, project: project)
-
-      service = described_class.new(project, double,
-                                    milestone_id: milestone.id,
-                                    assignee_id: assignee.id,
-                                    label_ids: [label.id])
-      service.execute(board)
-
-      expect(board.reload).to have_attributes(milestone: milestone,
-                                              assignee: assignee,
-                                              labels: [label])
-    end
-
-    it 'filters unpermitted params when scoped issue board is not enabled' do
-      stub_licensed_features(scoped_issue_board: false)
-      params = { milestone_id: double, assignee_id: double, label_ids: double, weight: double }
-
-      service = described_class.new(project, double, params)
-      service.execute(board)
-
-      expect(board.reload).to have_attributes(milestone: nil,
-                                              assignee: nil,
-                                              labels: [])
-    end
   end
 end