Commit 5614d724 authored by Douwe Maan's avatar Douwe Maan Committed by Alejandro Rodríguez

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
parent 311b59d9
...@@ -69,31 +69,26 @@ class IssuableFinder ...@@ -69,31 +69,26 @@ class IssuableFinder
def project def project
return @project if defined?(@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 = project
@project = nil
end
else
@project = nil
end
@project
end end
def projects def projects
return @projects if defined?(@projects) return @projects if defined?(@projects)
return @projects = project if project?
if project? projects =
@projects = project if current_user && params[:authorized_only].presence && !current_user_related?
elsif current_user && params[:authorized_only].presence && !current_user_related? current_user.authorized_projects
@projects = current_user.authorized_projects.reorder(nil) elsif group
elsif group GroupProjectsFinder.new(group).execute(current_user)
@projects = GroupProjectsFinder.new(group).execute(current_user).reorder(nil) else
else ProjectsFinder.new.execute(current_user)
@projects = ProjectsFinder.new.execute(current_user).reorder(nil) end
end
@projects = projects.with_feature_available_for_user(klass, current_user).reorder(nil)
end end
def search def search
......
...@@ -172,6 +172,10 @@ module Issuable ...@@ -172,6 +172,10 @@ module Issuable
grouping_columns grouping_columns
end end
def to_ability_name
model_name.singular
end
end end
def today? def today?
...@@ -245,7 +249,7 @@ module Issuable ...@@ -245,7 +249,7 @@ module Issuable
# issuable.class # => MergeRequest # issuable.class # => MergeRequest
# issuable.to_ability_name # => "merge_request" # issuable.to_ability_name # => "merge_request"
def to_ability_name def to_ability_name
self.class.to_s.underscore self.class.to_ability_name
end end
# Returns a Hash of attributes to be used for Twitter card metadata # Returns a Hash of attributes to be used for Twitter card metadata
......
...@@ -195,8 +195,38 @@ class Project < ActiveRecord::Base ...@@ -195,8 +195,38 @@ class Project < ActiveRecord::Base
scope :for_milestones, ->(ids) { joins(:milestones).where('milestones.id' => ids).distinct } scope :for_milestones, ->(ids) { joins(:milestones).where('milestones.id' => ids).distinct }
scope :with_push, -> { joins(:events).where('events.action = ?', Event::PUSHED) } 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_project_feature, -> { joins('LEFT JOIN project_features ON projects.id = project_features.project_id') }
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') }
# "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 :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) } scope :abandoned, -> { where('projects.last_activity_at < ?', 6.months.ago) }
......
...@@ -20,6 +20,15 @@ class ProjectFeature < ActiveRecord::Base ...@@ -20,6 +20,15 @@ class ProjectFeature < ActiveRecord::Base
FEATURES = %i(issues merge_requests wiki snippets builds) FEATURES = %i(issues merge_requests wiki snippets builds)
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 # Default scopes force us to unscope here since a service may need to check
# permissions for a project in pending_delete # permissions for a project in pending_delete
# http://stackoverflow.com/questions/1540645/how-to-disable-default-scope-for-a-belongs-to # http://stackoverflow.com/questions/1540645/how-to-disable-default-scope-for-a-belongs-to
...@@ -32,9 +41,8 @@ class ProjectFeature < ActiveRecord::Base ...@@ -32,9 +41,8 @@ class ProjectFeature < ActiveRecord::Base
default_value_for :wiki_access_level, value: ENABLED, allows_nil: false default_value_for :wiki_access_level, value: ENABLED, allows_nil: false
def feature_available?(feature, user) def feature_available?(feature, user)
raise ArgumentError, 'invalid project feature' unless FEATURES.include?(feature) access_level = public_send(ProjectFeature.access_level_attribute(feature))
get_permission(user, access_level)
get_permission(user, public_send("#{feature}_access_level"))
end end
def builds_enabled? def builds_enabled?
......
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
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
...@@ -97,6 +97,11 @@ describe Issue, "Issuable" do ...@@ -97,6 +97,11 @@ describe Issue, "Issuable" do
end end
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 describe "#today?" do
it "returns true when created today" do it "returns true when created today" do
# Avoid timezone differences and just return exactly what we want # Avoid timezone differences and just return exactly what we want
......
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment