Group Guests are no longer able to see merge requests

Group guests will only be displayed merge requests to
projects they have a access level to, higher than Reporter.

Visible projects will still display the merge requests to Guests

7 files changed, 154 insertions, 20 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 15465d9b356..ccf4ab5171b 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -377,8 +377,10 @@ class Project < ActiveRecord::Base
 
   # "enabled" here means "not disabled". It includes private features!
   scope :with_feature_enabled, ->(feature) {
-    access_level_attribute = ProjectFeature.access_level_attribute(feature)
-    with_project_feature.where(project_features: { access_level_attribute => [nil, ProjectFeature::PRIVATE, ProjectFeature::ENABLED, ProjectFeature::PUBLIC] })
+    access_level_attribute = ProjectFeature.arel_table[ProjectFeature.access_level_attribute(feature)]
+    enabled_feature = access_level_attribute.gt(ProjectFeature::DISABLED).or(access_level_attribute.eq(nil))
+
+    with_project_feature.where(enabled_feature)
   }
 
   # Picks a feature where the level is exactly that given.
@@ -465,7 +467,8 @@ class Project < ActiveRecord::Base
   # logged in users to more efficiently get private projects with the given
   # feature.
   def self.with_feature_available_for_user(feature, user)
-    visible = [nil, ProjectFeature::ENABLED, ProjectFeature::PUBLIC]
+    visible = [ProjectFeature::ENABLED, ProjectFeature::PUBLIC]
+    min_access_level = ProjectFeature.required_minimum_access_level(feature)
 
     if user&.admin?
       with_feature_enabled(feature)
@@ -473,10 +476,15 @@ class Project < ActiveRecord::Base
       column = ProjectFeature.quoted_access_level_column(feature)
 
       with_project_feature
-        .where("#{column} IN (?) OR (#{column} = ? AND EXISTS (?))",
-              visible,
-              ProjectFeature::PRIVATE,
-              user.authorizations_for_projects)
+        .where(
+          "(projects.visibility_level > :private AND (#{column} IS NULL OR #{column} >= (:public_visible) OR (#{column} = :private_visible AND EXISTS(:authorizations))))"\
+          " OR (projects.visibility_level = :private AND (#{column} IS NULL OR #{column} >= :private_visible) AND EXISTS(:authorizations))",
+          {
+            private: Gitlab::VisibilityLevel::PRIVATE,
+            public_visible: ProjectFeature::ENABLED,
+            private_visible: ProjectFeature::PRIVATE,
+            authorizations: user.authorizations_for_projects(min_access_level: min_access_level)
+          })
     else
       with_feature_access_level(feature, visible)
     end
diff --git a/app/models/project_feature.rb b/app/models/project_feature.rb
index 39f2b8fe0de..f700090a493 100644
--- a/app/models/project_feature.rb
+++ b/app/models/project_feature.rb
@@ -23,11 +23,11 @@ class ProjectFeature < ActiveRecord::Base
   PUBLIC   = 30
 
   FEATURES = %i(issues merge_requests wiki snippets builds repository pages).freeze
+  PRIVATE_FEATURES_MIN_ACCESS_LEVEL = { merge_requests: Gitlab::Access::REPORTER }.freeze
 
   class << self
     def access_level_attribute(feature)
-      feature = feature.model_name.plural.to_sym if feature.respond_to?(:model_name)
-      raise ArgumentError, "invalid project feature: #{feature}" unless FEATURES.include?(feature)
+      feature = ensure_feature!(feature)
 
       "#{feature}_access_level".to_sym
     end
@@ -38,6 +38,21 @@ class ProjectFeature < ActiveRecord::Base
 
       "#{table}.#{attribute}"
     end
+
+    def required_minimum_access_level(feature)
+      feature = ensure_feature!(feature)
+
+      PRIVATE_FEATURES_MIN_ACCESS_LEVEL.fetch(feature, Gitlab::Access::GUEST)
+    end
+
+    private
+
+    def ensure_feature!(feature)
+      feature = feature.model_name.plural.to_sym if feature.respond_to?(:model_name)
+      raise ArgumentError, "invalid project feature: #{feature}" unless FEATURES.include?(feature)
+
+      feature
+    end
   end
 
   # Default scopes force us to unscope here since a service may need to check
diff --git a/app/models/user.rb b/app/models/user.rb
index 26fd2d903a1..262b1835e98 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -754,8 +754,12 @@ class User < ActiveRecord::Base
   #
   # Example use:
   # `Project.where('EXISTS(?)', user.authorizations_for_projects)`
-  def authorizations_for_projects
-    project_authorizations.select(1).where('project_authorizations.project_id = projects.id')
+  def authorizations_for_projects(min_access_level: nil)
+    authorizations = project_authorizations.select(1).where('project_authorizations.project_id = projects.id')
+
+    return authorizations unless min_access_level.present?
+
+    authorizations.where('project_authorizations.access_level >= ?', min_access_level)
   end
 
   # Returns the projects this user has reporter (or greater) access to, limited
diff --git a/changelogs/unreleased/security-guests-can-see-list-of-merge-requests.yml b/changelogs/unreleased/security-guests-can-see-list-of-merge-requests.yml
new file mode 100644
index 00000000000..f5b74011829
--- /dev/null
+++ b/changelogs/unreleased/security-guests-can-see-list-of-merge-requests.yml
@@ -0,0 +1,6 @@
+---
+title: Group guests are no longer able to see merge requests they don't have access
+  to at group level
+merge_request:
+author:
+type: security
diff --git a/spec/finders/merge_requests_finder_spec.rb b/spec/finders/merge_requests_finder_spec.rb
index ff4c6b8dd42..107da08a0a9 100644
--- a/spec/finders/merge_requests_finder_spec.rb
+++ b/spec/finders/merge_requests_finder_spec.rb
@@ -68,20 +68,34 @@ describe MergeRequestsFinder do
       expect(merge_requests.size).to eq(2)
     end
 
-    it 'filters by group' do
-      params = { group_id: group.id }
+    context 'filtering by group' do
+      it 'includes all merge requests when user has access' do
+        params = { group_id: group.id }
 
-      merge_requests = described_class.new(user, params).execute
+        merge_requests = described_class.new(user, params).execute
 
-      expect(merge_requests.size).to eq(3)
-    end
+        expect(merge_requests.size).to eq(3)
+      end
 
-    it 'filters by group including subgroups', :nested_groups do
-      params = { group_id: group.id, include_subgroups: true }
+      it 'excludes merge requests from projects the user does not have access to' do
+        private_project = create_project_without_n_plus_1(:private, group: group)
+        private_mr = create(:merge_request, :simple, author: user, source_project: private_project, target_project: private_project)
+        params = { group_id: group.id }
 
-      merge_requests = described_class.new(user, params).execute
+        private_project.add_guest(user)
+        merge_requests = described_class.new(user, params).execute
 
-      expect(merge_requests.size).to eq(6)
+        expect(merge_requests.size).to eq(3)
+        expect(merge_requests).not_to include(private_mr)
+      end
+
+      it 'filters by group including subgroups', :nested_groups do
+        params = { group_id: group.id, include_subgroups: true }
+
+        merge_requests = described_class.new(user, params).execute
+
+        expect(merge_requests.size).to eq(6)
+      end
     end
 
     it 'filters by non_archived' do
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 7a8dc59039e..585dfe46189 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3074,6 +3074,66 @@ describe Project do
     end
   end
 
+  describe '.with_feature_available_for_user' do
+    let!(:user) { create(:user) }
+    let!(:feature) { MergeRequest }
+    let!(:project) { create(:project, :public, :merge_requests_enabled) }
+
+    subject { described_class.with_feature_available_for_user(feature, user) }
+
+    context 'when user has access to project' do
+      subject { described_class.with_feature_available_for_user(feature, user) }
+
+      before do
+        project.add_guest(user)
+      end
+
+      context 'when public project' do
+        context 'when feature is public' do
+          it 'returns project' do
+            is_expected.to include(project)
+          end
+        end
+
+        context 'when feature is private' do
+          let!(:project) { create(:project, :public, :merge_requests_private) }
+
+          it 'returns project when user has access to the feature' do
+            project.add_maintainer(user)
+
+            is_expected.to include(project)
+          end
+
+          it 'does not return project when user does not have the minimum access level required' do
+            is_expected.not_to include(project)
+          end
+        end
+      end
+
+      context 'when private project' do
+        let!(:project) { create(:project) }
+
+        it 'returns project when user has access to the feature' do
+          project.add_maintainer(user)
+
+          is_expected.to include(project)
+        end
+
+        it 'does not return project when user does not have the minimum access level required' do
+          is_expected.not_to include(project)
+        end
+      end
+    end
+
+    context 'when user does not have access to project' do
+      let!(:project) { create(:project) }
+
+      it 'does not return project when user cant access project' do
+        is_expected.not_to include(project)
+      end
+    end
+  end
+
   describe '#pages_available?' do
     let(:project) { create(:project, group: group) }
 
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 33842e74b92..78477ab0a5a 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1997,6 +1997,33 @@ describe User do
       expect(subject).to include(accessible)
       expect(subject).not_to include(other)
     end
+
+    context 'with min_access_level' do
+      let!(:user) { create(:user) }
+      let!(:project) { create(:project, :private, namespace: user.namespace) }
+
+      before do
+        project.add_developer(user)
+      end
+
+      subject { Project.where("EXISTS (?)", user.authorizations_for_projects(min_access_level: min_access_level)) }
+
+      context 'when developer access' do
+        let(:min_access_level) { Gitlab::Access::DEVELOPER }
+
+        it 'includes projects a user has access to' do
+          expect(subject).to include(project)
+        end
+      end
+
+      context 'when owner access' do
+        let(:min_access_level) { Gitlab::Access::OWNER }
+
+        it 'does not include projects with higher access level' do
+          expect(subject).not_to include(project)
+        end
+      end
+    end
   end
 
   describe '#authorized_projects', :delete do