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

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

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

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) }

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?

spec/features/groups/issues_spec.rb

+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

spec/features/groups/merge_requests_spec.rb

+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

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

spec/support/project_features_apply_to_issuables_shared_examples.rb

+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