Merge branch '22481-honour-issue-visibility-for-groups' into 'security'

Honour issue and merge request visibility in their respective finders

This MR fixes a security issue with the IssuesFinder and MergeRequestFinder where they would return items the user did not have permission to see. This was most visible on the issue and merge requests page for a group containing projects that had set their issues or merge requests to "private".

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/22481

See merge request !2000

8 files changed, 139 insertions, 25 deletions

diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb
index cc2073081b5..6297b2db369 100644
--- a/app/finders/issuable_finder.rb
+++ b/app/finders/issuable_finder.rb
@@ -61,31 +61,26 @@ class IssuableFinder
   def project
     return @project if defined?(@project)
 
-    if project?
-      @project = Project.find(params[:project_id])
+    project = Project.find(params[:project_id])
+    project = nil unless Ability.allowed?(current_user, :"read_#{klass.to_ability_name}", project)
 
-      unless Ability.allowed?(current_user, :read_project, @project)
-        @project = nil
-      end
-    else
-      @project = nil
-    end
-
-    @project
+    @project = project
   end
 
   def projects
     return @projects if defined?(@projects)
+    return @projects = project if project?
 
-    if project?
-      @projects = project
-    elsif current_user && params[:authorized_only].presence && !current_user_related?
-      @projects = current_user.authorized_projects.reorder(nil)
-    elsif group
-      @projects = GroupProjectsFinder.new(group).execute(current_user).reorder(nil)
-    else
-      @projects = ProjectsFinder.new.execute(current_user).reorder(nil)
-    end
+    projects =
+      if current_user && params[:authorized_only].presence && !current_user_related?
+        current_user.authorized_projects
+      elsif group
+        GroupProjectsFinder.new(group).execute(current_user)
+      else
+        ProjectsFinder.new.execute(current_user)
+      end
+
+    @projects = projects.with_feature_available_for_user(klass, current_user).reorder(nil)
   end
 
   def search
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 93a6b3122e0..664bb594aa9 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -183,6 +183,10 @@ module Issuable
 
       grouping_columns
     end
+
+    def to_ability_name
+      model_name.singular
+    end
   end
 
   def today?
@@ -244,7 +248,7 @@ module Issuable
   #   issuable.class           # => MergeRequest
   #   issuable.to_ability_name # => "merge_request"
   def to_ability_name
-    self.class.to_s.underscore
+    self.class.to_ability_name
   end
 
   # Returns a Hash of attributes to be used for Twitter card metadata
diff --git a/app/models/project.rb b/app/models/project.rb
index 4c9c7c001dd..bbe590b5a8a 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -207,8 +207,38 @@ class Project < ActiveRecord::Base
   scope :for_milestones, ->(ids) { joins(:milestones).where('milestones.id' => ids).distinct }
   scope :with_push, -> { joins(:events).where('events.action = ?', Event::PUSHED) }
 
-  scope :with_builds_enabled, -> { joins('LEFT JOIN project_features ON projects.id = project_features.project_id').where('project_features.builds_access_level IS NULL or project_features.builds_access_level > 0') }
-  scope :with_issues_enabled, -> { joins('LEFT JOIN project_features ON projects.id = project_features.project_id').where('project_features.issues_access_level IS NULL or project_features.issues_access_level > 0') }
+  scope :with_project_feature, -> { joins('LEFT JOIN project_features ON projects.id = project_features.project_id') }
+
+  # "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] })
+  }
+
+  # Picks a feature where the level is exactly that given.
+  scope :with_feature_access_level, ->(feature, level) {
+    access_level_attribute = ProjectFeature.access_level_attribute(feature)
+    with_project_feature.where(project_features: { access_level_attribute => level })
+  }
+
+  scope :with_builds_enabled, -> { with_feature_enabled(:builds) }
+  scope :with_issues_enabled, -> { with_feature_enabled(:issues) }
+
+  # project features may be "disabled", "internal" or "enabled". If "internal",
+  # they are only available to team members. This scope returns projects where
+  # the feature is either enabled, or internal with permission for the user.
+  def self.with_feature_available_for_user(feature, user)
+    return with_feature_enabled(feature) if user.try(:admin?)
+
+    unconditional = with_feature_access_level(feature, [nil, ProjectFeature::ENABLED])
+    return unconditional if user.nil?
+
+    conditional = with_feature_access_level(feature, ProjectFeature::PRIVATE)
+    authorized = user.authorized_projects.merge(conditional.reorder(nil))
+
+    union = Gitlab::SQL::Union.new([unconditional.select(:id), authorized.select(:id)])
+    where(arel_table[:id].in(Arel::Nodes::SqlLiteral.new(union.to_sql)))
+  end
 
   scope :active, -> { joins(:issues, :notes, :merge_requests).order('issues.created_at, notes.created_at, merge_requests.created_at DESC') }
   scope :abandoned, -> { where('projects.last_activity_at < ?', 6.months.ago) }
diff --git a/app/models/project_feature.rb b/app/models/project_feature.rb
index b37ce1d3cf6..34fd5a57b5e 100644
--- a/app/models/project_feature.rb
+++ b/app/models/project_feature.rb
@@ -20,6 +20,15 @@ class ProjectFeature < ActiveRecord::Base
 
   FEATURES = %i(issues merge_requests wiki snippets builds repository)
 
+  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}_access_level".to_sym
+    end
+  end
+
   # Default scopes force us to unscope here since a service may need to check
   # permissions for a project in pending_delete
   # http://stackoverflow.com/questions/1540645/how-to-disable-default-scope-for-a-belongs-to
@@ -35,9 +44,8 @@ class ProjectFeature < ActiveRecord::Base
   default_value_for :repository_access_level,     value: ENABLED, allows_nil: false
 
   def feature_available?(feature, user)
-    raise ArgumentError, 'invalid project feature' unless FEATURES.include?(feature)
-
-    get_permission(user, public_send("#{feature}_access_level"))
+    access_level = public_send(ProjectFeature.access_level_attribute(feature))
+    get_permission(user, access_level)
   end
 
   def builds_enabled?
diff --git a/spec/features/groups/issues_spec.rb b/spec/features/groups/issues_spec.rb
new file mode 100644
index 00000000000..476eca17a9d
--- /dev/null
+++ b/spec/features/groups/issues_spec.rb
@@ -0,0 +1,8 @@
+require 'spec_helper'
+
+feature 'Group issues page', feature: true do
+  let(:path) { issues_group_path(group) }
+  let(:issuable) { create(:issue, project: project, title: "this is my created issuable")}
+
+  include_examples 'project features apply to issuables', Issue
+end
diff --git a/spec/features/groups/merge_requests_spec.rb b/spec/features/groups/merge_requests_spec.rb
new file mode 100644
index 00000000000..a2791b57544
--- /dev/null
+++ b/spec/features/groups/merge_requests_spec.rb
@@ -0,0 +1,8 @@
+require 'spec_helper'
+
+feature 'Group merge requests page', feature: true do
+  let(:path) { merge_requests_group_path(group) }
+  let(:issuable) { create(:merge_request, source_project: project, target_project: project, title: "this is my created issuable")}
+
+  include_examples 'project features apply to issuables', MergeRequest
+end
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index a9603074c32..6e987967ca5 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -97,6 +97,11 @@ describe Issue, "Issuable" do
     end
   end
 
+  describe '.to_ability_name' do
+    it { expect(Issue.to_ability_name).to eq("issue") }
+    it { expect(MergeRequest.to_ability_name).to eq("merge_request") }
+  end
+
   describe "#today?" do
     it "returns true when created today" do
       # Avoid timezone differences and just return exactly what we want
diff --git a/spec/support/project_features_apply_to_issuables_shared_examples.rb b/spec/support/project_features_apply_to_issuables_shared_examples.rb
new file mode 100644
index 00000000000..4621d17549b
--- /dev/null
+++ b/spec/support/project_features_apply_to_issuables_shared_examples.rb
@@ -0,0 +1,56 @@
+shared_examples 'project features apply to issuables' do |klass|
+  let(:described_class) { klass }
+
+  let(:group) { create(:group) }
+  let(:user_in_group) { create(:group_member, :developer, user: create(:user), group: group ).user }
+  let(:user_outside_group) { create(:user) }
+
+  let(:project) { create(:empty_project, :public, project_args) }
+
+  def project_args
+    feature = "#{described_class.model_name.plural}_access_level".to_sym
+
+    args = { group: group }
+    args[feature] = access_level
+
+    args
+  end
+
+  before do
+    _ = issuable
+    login_as(user)
+    visit path
+  end
+
+  context 'public access level' do
+    let(:access_level) { ProjectFeature::ENABLED }
+
+    context 'group member' do
+      let(:user) { user_in_group }
+
+      it { expect(page).to have_content(issuable.title) }
+    end
+
+    context 'non-member' do
+      let(:user) { user_outside_group }
+
+      it { expect(page).to have_content(issuable.title) }
+    end
+  end
+
+  context 'private access level' do
+    let(:access_level) { ProjectFeature::PRIVATE }
+
+    context 'group member' do
+      let(:user) { user_in_group }
+
+      it { expect(page).to have_content(issuable.title) }
+    end
+
+    context 'non-member' do
+      let(:user) { user_outside_group }
+
+      it { expect(page).not_to have_content(issuable.title) }
+    end
+  end
+end