From bdcfbf8f424fbb3a48c39b3c6b22ed371fa02fe1 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 24 Jan 2019 12:49:01 +0000 Subject: Merge branch 'security-guests-can-see-list-of-merge-requests-11-6' into 'security-11-6' [11.6] Group Guests are no longer able to see merge requests See merge request gitlab/gitlabhq!2815 (cherry picked from commit a662cfdb80a9d7fe6eacbc1a40fb24b5a7b9272e) f7a2dabd Group Guests are no longer able to see merge requests --- app/models/project.rb | 22 +++++--- app/models/project_feature.rb | 19 ++++++- app/models/user.rb | 8 ++- ...urity-guests-can-see-list-of-merge-requests.yml | 6 +++ spec/finders/merge_requests_finder_spec.rb | 32 ++++++++---- spec/models/project_spec.rb | 60 ++++++++++++++++++++++ spec/models/user_spec.rb | 27 ++++++++++ 7 files changed, 154 insertions(+), 20 deletions(-) create mode 100644 changelogs/unreleased/security-guests-can-see-list-of-merge-requests.yml diff --git a/app/models/project.rb b/app/models/project.rb index 8194dd7a458..5cad53120a8 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -374,8 +374,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. @@ -462,7 +464,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) @@ -470,10 +473,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 dbd754dd25a..cc5d81b9483 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -753,8 +753,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 diff --git a/changelogs/unreleased/security-guests-can-see-list-of-merge-requests.yml b/changelogs/unreleased/security-guests-can-see-list-of-merge-requests.yml new file mode 100644 index 00000000000..f5b74011829 --- /dev/null +++ b/changelogs/unreleased/security-guests-can-see-list-of-merge-requests.yml @@ -0,0 +1,6 @@ +--- +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 diff --git a/spec/finders/merge_requests_finder_spec.rb b/spec/finders/merge_requests_finder_spec.rb index ff4c6b8dd42..107da08a0a9 100644 --- a/spec/finders/merge_requests_finder_spec.rb +++ b/spec/finders/merge_requests_finder_spec.rb @@ -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 diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 8dd42539422..d5ace2fe7a7 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2986,6 +2986,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) } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index ff075e65c76..82c0363b5a5 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1996,6 +1996,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 -- cgit v1.2.1