Commit 57781294 authored by Tiago Botelho's avatar Tiago Botelho Committed by Yorick Peterse

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
parent 740f07b1
......@@ -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
......
......@@ -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
......
......@@ -754,8 +754,12 @@ class User < ApplicationRecord
#
# 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
......
---
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
......@@ -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
......
......@@ -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) }
......
......@@ -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
......
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