Merge branch 'security-guests-can-see-list-of-merge-requests-11-7' into 'security-11-7'

[11.7] Group Guests are no longer able to see merge requests

See merge request gitlab/gitlabhq!2814

(cherry picked from commit 190167d542fab9bfe8d41b6f87f5be4fbeb699f7)

fe6504ed Group Guests are no longer able to see merge requests

3 files changed, 38 insertions, 11 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index cab173503ce..3216884d49f 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