Merge branch 'security-rc-insights-unallowed-private-project-usage' into 'master'

Ensure current user can read the Insights configuration project

See merge request gitlab/gitlab-ee!1032

ee/app/controllers/concerns/insights_actions.rb

@@ -44,7 +44,7 @@ module InsightsActions
   end
 
   def query_param
-    @query_param ||= params[:query]
+    @query_param ||= params[:query] || {}
   end
 
   def period_param

ee/app/controllers/groups/insights_controller.rb

@@ -4,6 +4,7 @@ class Groups::InsightsController < Groups::ApplicationController
   include InsightsActions
 
   before_action :authorize_read_group!
+  before_action :authorize_read_insights_config_project!
 
   private
 
@@ -11,6 +12,12 @@ class Groups::InsightsController < Groups::ApplicationController
     render_404 unless can?(current_user, :read_group, group)
   end
 
+  def authorize_read_insights_config_project!
+    insights_config_project = group.insights_config_project
+
+    render_404 if insights_config_project && !can?(current_user, :read_project, insights_config_project)
+  end
+
   def insights_entity
     group
   end

ee/app/models/concerns/insights_feature.rb

@@ -10,32 +10,32 @@ module InsightsFeature
     feature_available?(:insights)
   end
 
-  def insights_config(follow_group: true)
-    case self
-    when Group
-      # When there's a config file, we use it regardless it's valid or not
-      if insight&.project&.project_insights_config_yaml
-        insight.project.insights_config(follow_group: false)
-      else # When there's nothing, then we use the default
-        default_insights_config
+  def insights_config_project
+    strong_memoize(:insights_config_project) do
+      case self
+      when Group
+        insight&.project
+      when Project
+        if insights_config_yaml
+          self
+        else
+          group&.insights_config_project
+        end
       end
-    when Project
-      yaml = project_insights_config_yaml
+    end
+  end
+
+  def insights_config
+    strong_memoize(:insights_config) do
+      yaml = insights_config_project&.insights_config_yaml
 
-      # When there's a config file, we use it regardless it's valid or not
       if yaml
-        strong_memoize(:insights_config) do
-          ::Gitlab::Config::Loader::Yaml.new(yaml).load!
-        rescue Gitlab::Config::Loader::FormatError
-          nil
-        end
-      # When we're following the group and there's a group then we use it
-      elsif follow_group && group
-        group.insights_config
-      # A project might not have a group, then we just use the default
+        ::Gitlab::Config::Loader::Yaml.new(yaml).load!
       else
         default_insights_config
       end
+    rescue Gitlab::Config::Loader::FormatError
+      nil
     end
   end
 
@@ -50,8 +50,10 @@ module InsightsFeature
 
   protected
 
-  def project_insights_config_yaml
-    strong_memoize(:project_insights_config_yaml) do
+  def insights_config_yaml
+    raise NotImplementedError unless is_a?(Project)
+
+    strong_memoize(:insights_config_yaml) do
       next if repository.empty?
 
       repository.insights_config_for(repository.root_ref)

ee/app/services/ee/groups/update_service.rb

@@ -35,6 +35,12 @@ module EE
           params.delete(:extra_shared_runners_minutes_limit)
         end
 
+        insight_project_id = params.dig(:insight_attributes, :project_id)
+        if insight_project_id
+          group_projects = GroupProjectsFinder.new(group: group, current_user: current_user, options: { only_owned: true, include_subgroups: true }).execute
+          params.delete(:insight_attributes) unless group_projects.exists?(insight_project_id) # rubocop:disable CodeReuse/ActiveRecord
+        end
+
         super
       end
 

ee/changelogs/unreleased/security-rc-insights-unallowed-private-project-usage.yml

+---
+title: Ensure the Insights configuration project is part of the group and is accessible to the current user
+merge_request:
+author:
+type: security

ee/lib/gitlab/insights/finders/issuable_finder.rb

@@ -54,7 +54,7 @@ module Gitlab
         attr_reader :entity, :current_user, :opts
 
         def finder
-          issuable_type = opts[:issuable_type].to_sym
+          issuable_type = opts[:issuable_type]&.to_sym
 
           FINDERS[issuable_type] ||
             raise(InvalidIssuableTypeError, "Invalid `:issuable_type` option: `#{opts[:issuable_type]}`. Allowed values are #{FINDERS.keys}!")

ee/spec/controllers/groups/insights_controller_spec.rb

+require 'spec_helper'
+
+describe Groups::InsightsController do
+  set(:parent_group) { create(:group, :private) }
+  set(:nested_group) { create(:group, :private, parent: parent_group) }
+  set(:project) { create(:project, :private) }
+  set(:insight) { create(:insight, group: parent_group, project: project) }
+  set(:user) { create(:user) }
+  let(:query_params) { { chart_type: 'bar', query: { issuable_type: 'issue', collection_labels: ['bug'] } } }
+
+  before do
+    stub_licensed_features(insights: true)
+    sign_in(user)
+    parent_group.add_developer(user)
+    nested_group.add_developer(user)
+  end
+
+  shared_examples '404 status' do
+    it 'returns 404 status' do
+      subject
+
+      expect(response).to have_gitlab_http_status(404)
+    end
+  end
+
+  shared_examples '200 status' do
+    it 'returns 200 status' do
+      subject
+
+      expect(response).to have_gitlab_http_status(200)
+    end
+  end
+
+  context 'when insights configuration project cannot be read by current user' do
+    context 'when visiting the parent group' do
+      describe 'GET #show.html' do
+        subject { get :show, params: { group_id: parent_group.to_param } }
+
+        it_behaves_like '404 status'
+      end
+
+      describe 'GET #show.json' do
+        subject { get :show, params: { group_id: parent_group.to_param }, format: :json }
+
+        it_behaves_like '404 status'
+      end
+
+      describe 'POST #query' do
+        subject { post :query, params: query_params.merge(group_id: parent_group.to_param) }
+
+        it_behaves_like '404 status'
+      end
+    end
+
+    context 'when visiting a nested group' do
+      describe 'GET #show.html' do
+        subject { get :show, params: { group_id: nested_group.to_param } }
+
+        # The following expectation should be changed to
+        # it_behaves_like '404 status'
+        # once https://gitlab.com/gitlab-org/gitlab-ee/issues/11340 is implemented.
+        it_behaves_like '200 status'
+      end
+
+      describe 'GET #show.json' do
+        subject { get :show, params: { group_id: nested_group.to_param }, format: :json }
+
+        # The following expectation should be changed to
+        # it_behaves_like '404 status'
+        # once https://gitlab.com/gitlab-org/gitlab-ee/issues/11340 is implemented.
+        it_behaves_like '200 status'
+
+        # The following expectation should be removed
+        # once https://gitlab.com/gitlab-org/gitlab-ee/issues/11340 is implemented.
+        it 'does return the default config' do
+          subject
+
+          expect(response.parsed_body).to eq(parent_group.default_insights_config.to_json)
+        end
+      end
+
+      describe 'POST #query.json' do
+        subject { post :query, params: query_params.merge(group_id: nested_group.to_param), format: :json }
+
+        # The following expectation should be changed to
+        # it_behaves_like '404 status'
+        # once https://gitlab.com/gitlab-org/gitlab-ee/issues/11340 is implemented.
+        it_behaves_like '200 status'
+      end
+    end
+  end
+
+  context 'when insights configuration project can be read by current user' do
+    before do
+      project.add_reporter(user)
+    end
+
+    context 'when the configuration is attached to the current group' do
+      describe 'GET #show.html' do
+        subject { get :show, params: { group_id: parent_group.to_param } }
+
+        it_behaves_like '200 status'
+      end
+
+      describe 'GET #show.sjon' do
+        subject { get :show, params: { group_id: parent_group.to_param }, format: :json }
+
+        it_behaves_like '200 status'
+      end
+
+      describe 'POST #query.json' do
+        subject { post :query, params: query_params.merge(group_id: parent_group.to_param), format: :json }
+
+        it_behaves_like '200 status'
+      end
+    end
+
+    context 'when the configuration is attached to a nested group' do
+      describe 'GET #show.html' do
+        subject { get :show, params: { group_id: nested_group.to_param } }
+
+        it_behaves_like '200 status'
+      end
+
+      describe 'GET #show.json' do
+        subject { get :show, params: { group_id: nested_group.to_param }, format: :json }
+
+        it_behaves_like '200 status'
+      end
+
+      describe 'POST #query.json' do
+        subject { post :query, params: query_params.merge(group_id: nested_group.to_param), format: :json }
+
+        it_behaves_like '200 status'
+      end
+    end
+  end
+end

ee/spec/factories/insights.rb

 # frozen_string_literal: true
 
 FactoryBot.define do
+  factory :insight do
+    group
+    project
+  end
+
   factory :insights_issues_by_team, class: Hash do
     initialize_with do
       {

ee/spec/models/group_spec.rb

@@ -425,10 +425,26 @@ describe Group do
       context 'with an invalid config file' do
         let(:insights_file_content) { ': foo bar' }
 
-        it 'returns the insights config data' do
+        it 'returns nil' do
           expect(group.insights_config).to be_nil
         end
       end
     end
+
+    context 'when group has an Insights project configured which is in a nested group' do
+      before do
+        nested_group = create(:group, parent: group)
+        project = create(:project, :custom_repo, group: nested_group, files: { ::Gitlab::Insights::CONFIG_FILE_PATH => insights_file_content })
+        group.create_insight!(project: project)
+      end
+
+      let(:insights_file_content) { 'key: monthlyBugsCreated' }
+
+      it 'returns the insights config data' do
+        insights_config = group.insights_config
+
+        expect(insights_config).to eq(key: 'monthlyBugsCreated')
+      end
+    end
   end
 end

ee/spec/models/project_spec.rb

@@ -1747,7 +1747,7 @@ describe Project do
 
         context 'when the group has an Insights config from another project' do
           let(:config_project) do
-            create(:project, :custom_repo, files: { ::Gitlab::Insights::CONFIG_FILE_PATH => insights_file_content })
+            create(:project, :custom_repo, group: group, files: { ::Gitlab::Insights::CONFIG_FILE_PATH => insights_file_content })
           end
 
           before do
@@ -1761,6 +1761,18 @@ describe Project do
               expect(project.insights_config).to eq(config_project.insights_config)
               expect(project.insights_config).to eq(group.insights_config)
             end
+
+            context 'when the project is inside a nested group' do
+              let(:nested_group) { create(:group, parent: group) }
+              let(:project) { create(:project, group: nested_group) }
+
+              # The following expectaction should be changed to
+              # expect(project.insights_config).to eq(config_project.insights_config)
+              # once https://gitlab.com/gitlab-org/gitlab-ee/issues/11340 is implemented.
+              it 'returns the project default config' do
+                expect(project.insights_config).to eq(project.default_insights_config)
+              end
+            end
           end
 
           context 'with an invalid config file' do
@@ -1783,25 +1795,22 @@ describe Project do
         let(:insights_file_content) { 'key: monthlyBugsCreated' }
 
         it 'returns the insights config data' do
-          insights_config = project.insights_config
-
-          expect(insights_config).to eq(key: 'monthlyBugsCreated')
+          expect(project.insights_config).to eq(key: 'monthlyBugsCreated')
         end
 
         context 'when the project is inside a group having another config' do
+          let(:group) { create(:group) }
           let(:config_project) do
-            create(:project, :custom_repo, files: { ::Gitlab::Insights::CONFIG_FILE_PATH => ': foo bar' })
+            create(:project, :custom_repo, group: group, files: { ::Gitlab::Insights::CONFIG_FILE_PATH => ': foo bar' })
           end
 
           before do
-            project.group = create(:group)
+            project.group = group
             project.group.create_insight!(project: config_project)
           end
 
           it 'returns the project insights config data' do
-            insights_config = project.insights_config
-
-            expect(insights_config).to eq(key: 'monthlyBugsCreated')
+            expect(project.insights_config).to eq(key: 'monthlyBugsCreated')
           end
         end
       end
@@ -1814,12 +1823,13 @@ describe Project do
         end
 
         context 'when the project is inside a group having another config' do
+          let(:group) { create(:group) }
           let(:config_project) do
-            create(:project, :custom_repo, files: { ::Gitlab::Insights::CONFIG_FILE_PATH => 'key: monthlyBugsCreated' })
+            create(:project, :custom_repo, group: group, files: { ::Gitlab::Insights::CONFIG_FILE_PATH => 'key: monthlyBugsCreated' })
           end
 
           before do
-            project.group = create(:group)
+            project.group = group
             project.group.create_insight!(project: config_project)
           end
 

ee/spec/services/groups/update_service_spec.rb

@@ -204,6 +204,58 @@ describe Groups::UpdateService, '#execute' do
     end
   end
 
+  context 'updating insight_attributes.project_id param' do
+    let(:attrs) { { insight_attributes: { project_id: private_project.id } } }
+
+    shared_examples 'successful update of the Insights project' do
+      it 'updates the Insights project' do
+        update_group(group, user, attrs)
+
+        expect(group.insight.project).to eq(private_project)
+      end
+    end
+
+    shared_examples 'ignorance of the Insights project ID' do
+      it 'ignores the Insights project ID' do
+        update_group(group, user, attrs)
+
+        expect(group.insight).to be_nil
+      end
+    end
+
+    context 'when project is not in the group' do
+      let(:private_project) { create(:project, :private) }
+
+      context 'when user can read the project' do
+        before do
+          private_project.add_maintainer(user)
+        end
+
+        it_behaves_like 'ignorance of the Insights project ID'
+      end
+
+      context 'when user cannot read the project' do
+        it_behaves_like 'ignorance of the Insights project ID'
+      end
+    end
+
+    context 'when project is in the group' do
+      let(:private_project) { create(:project, :private, group: group) }
+
+      context 'when user can read the project' do
+        before do
+          private_project.add_maintainer(user)
+        end
+
+        it_behaves_like 'successful update of the Insights project'
+      end
+
+      context 'when user cannot read the project' do
+        it_behaves_like 'ignorance of the Insights project ID'
+      end
+    end
+  end
+
   def update_group(group, user, opts)
     Groups::UpdateService.new(group, user, opts).execute
   end