diff options
| -rw-r--r-- | CHANGELOG | 3 | ||||
| -rw-r--r-- | app/controllers/dashboard/groups_controller.rb | 2 | ||||
| -rw-r--r-- | app/models/group.rb | 2 | ||||
| -rw-r--r-- | app/models/snippet.rb | 11 | ||||
| -rw-r--r-- | app/models/user.rb | 4 | ||||
| -rw-r--r-- | app/views/admin/users/groups.html.haml | 5 | ||||
| -rw-r--r-- | spec/features/groups/members/user_requests_access_spec.rb | 15 | ||||
| -rw-r--r-- | spec/models/snippet_spec.rb | 42 | ||||
| -rw-r--r-- | spec/models/user_spec.rb | 20 | ||||
| -rw-r--r-- | spec/services/search/snippet_service_spec.rb | 59 |
10 files changed, 154 insertions, 9 deletions
diff --git a/CHANGELOG b/CHANGELOG index 32d31bfa54e..6506f49174a 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -16,8 +16,9 @@ v 8.10.0 (unreleased) - Allow [ci skip] to be in any case and allow [skip ci]. !4785 (simon_w) v 8.9.2 + - Fix visibility of snippets when searching. + - Fix an information disclosure when requesting access to a group containing private projects. - Update omniauth-saml to 1.6.0 !4951 - - Fix rendering of commit notes !4953 v 8.9.1 - Refactor labels documentation. !3347 diff --git a/app/controllers/dashboard/groups_controller.rb b/app/controllers/dashboard/groups_controller.rb index 71ba6153021..de6bc689bb7 100644 --- a/app/controllers/dashboard/groups_controller.rb +++ b/app/controllers/dashboard/groups_controller.rb @@ -1,5 +1,5 @@ class Dashboard::GroupsController < Dashboard::ApplicationController def index - @group_members = current_user.group_members.page(params[:page]) + @group_members = current_user.group_members.includes(:source).page(params[:page]) end end diff --git a/app/models/group.rb b/app/models/group.rb index e66e04371b2..c70c719e338 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -11,7 +11,7 @@ class Group < Namespace has_many :users, -> { where(members: { requested_at: nil }) }, through: :group_members has_many :owners, - -> { where(members: { access_level: Gitlab::Access::OWNER }) }, + -> { where(members: { requested_at: nil, access_level: Gitlab::Access::OWNER }) }, through: :group_members, source: :user diff --git a/app/models/snippet.rb b/app/models/snippet.rb index f8034cb5e6b..51f6ae7b25c 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -135,7 +135,16 @@ class Snippet < ActiveRecord::Base end def accessible_to(user) - where('visibility_level IN (?) OR author_id = ?', [Snippet::INTERNAL, Snippet::PUBLIC], user) + return are_public unless user.present? + return all if user.admin? + + where( + 'visibility_level IN (:visibility_levels) + OR author_id = :author_id + OR project_id IN (:project_ids)', + visibility_levels: [Snippet::PUBLIC, Snippet::INTERNAL], + author_id: user.id, + project_ids: user.authorized_projects.select(:id)) end end end diff --git a/app/models/user.rb b/app/models/user.rb index 04b220ee13c..599b2fb1191 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -57,7 +57,7 @@ class User < ActiveRecord::Base # Groups has_many :members, dependent: :destroy - has_many :group_members, dependent: :destroy, source: 'GroupMember' + has_many :group_members, -> { where(requested_at: nil) }, dependent: :destroy, source: 'GroupMember' has_many :groups, through: :group_members has_many :owned_groups, -> { where members: { access_level: Gitlab::Access::OWNER } }, through: :group_members, source: :group has_many :masters_groups, -> { where members: { access_level: Gitlab::Access::MASTER } }, through: :group_members, source: :group @@ -65,7 +65,7 @@ class User < ActiveRecord::Base # Projects has_many :groups_projects, through: :groups, source: :projects has_many :personal_projects, through: :namespace, source: :projects - has_many :project_members, dependent: :destroy, class_name: 'ProjectMember' + has_many :project_members, -> { where(requested_at: nil) }, dependent: :destroy, class_name: 'ProjectMember' has_many :projects, through: :project_members has_many :created_projects, foreign_key: :creator_id, class_name: 'Project' has_many :users_star_projects, dependent: :destroy diff --git a/app/views/admin/users/groups.html.haml b/app/views/admin/users/groups.html.haml index b0a709a568a..8f6d13b881a 100644 --- a/app/views/admin/users/groups.html.haml +++ b/app/views/admin/users/groups.html.haml @@ -1,11 +1,12 @@ - page_title "Groups", @user.name, "Users" = render 'admin/users/head' -- if @user.group_members.present? +- group_members = @user.group_members.includes(:source) +- if group_members.any? .panel.panel-default .panel-heading Groups: %ul.well-list - - @user.group_members.each do |group_member| + - group_members.each do |group_member| - group = group_member.group %li.group_member %span{class: ("list-item-name" unless group_member.owner?)} diff --git a/spec/features/groups/members/user_requests_access_spec.rb b/spec/features/groups/members/user_requests_access_spec.rb index 1ea607cbca0..4944301c938 100644 --- a/spec/features/groups/members/user_requests_access_spec.rb +++ b/spec/features/groups/members/user_requests_access_spec.rb @@ -4,6 +4,7 @@ feature 'Groups > Members > User requests access', feature: true do let(:user) { create(:user) } let(:owner) { create(:user) } let(:group) { create(:group, :public) } + let!(:project) { create(:project, :private, namespace: group) } background do group.add_owner(owner) @@ -24,6 +25,20 @@ feature 'Groups > Members > User requests access', feature: true do expect(page).not_to have_content 'Leave Group' end + scenario 'user does not see private projects' do + perform_enqueued_jobs { click_link 'Request Access' } + + expect(page).not_to have_content project.name + end + + scenario 'user does not see group in the Dashboard > Groups page' do + perform_enqueued_jobs { click_link 'Request Access' } + + visit dashboard_groups_path + + expect(page).not_to have_content group.name + end + scenario 'user is not listed in the group members page' do click_link 'Request Access' diff --git a/spec/models/snippet_spec.rb b/spec/models/snippet_spec.rb index 789816bf2c7..0621c6a06ce 100644 --- a/spec/models/snippet_spec.rb +++ b/spec/models/snippet_spec.rb @@ -72,7 +72,7 @@ describe Snippet, models: true do end end - describe '#search_code' do + describe '.search_code' do let(:snippet) { create(:snippet, content: 'class Foo; end') } it 'returns snippets with matching content' do @@ -88,6 +88,46 @@ describe Snippet, models: true do end end + describe '.accessible_to' do + let(:author) { create(:author) } + let(:project) { create(:empty_project) } + + let!(:public_snippet) { create(:snippet, :public) } + let!(:internal_snippet) { create(:snippet, :internal) } + let!(:private_snippet) { create(:snippet, :private, author: author) } + + let!(:project_public_snippet) { create(:snippet, :public, project: project) } + let!(:project_internal_snippet) { create(:snippet, :internal, project: project) } + let!(:project_private_snippet) { create(:snippet, :private, project: project) } + + it 'returns only public snippets when user is blank' do + expect(described_class.accessible_to(nil)).to match_array [public_snippet, project_public_snippet] + end + + it 'returns only public, and internal snippets for regular users' do + user = create(:user) + + expect(described_class.accessible_to(user)).to match_array [public_snippet, internal_snippet, project_public_snippet, project_internal_snippet] + end + + it 'returns public, internal snippets and project private snippets for project members' do + member = create(:user) + project.team << [member, :developer] + + expect(described_class.accessible_to(member)).to match_array [public_snippet, internal_snippet, project_public_snippet, project_internal_snippet, project_private_snippet] + end + + it 'returns private snippets where the user is the author' do + expect(described_class.accessible_to(author)).to match_array [public_snippet, internal_snippet, private_snippet, project_public_snippet, project_internal_snippet] + end + + it 'returns all snippets when for admins' do + admin = create(:admin) + + expect(described_class.accessible_to(admin)).to match_array [public_snippet, internal_snippet, private_snippet, project_public_snippet, project_internal_snippet, project_private_snippet] + end + end + describe '#participants' do let(:project) { create(:project, :public) } let(:snippet) { create(:snippet, content: 'foo', project: project) } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 73bee535fe3..328254ed56b 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -31,6 +31,26 @@ describe User, models: true do it { is_expected.to have_many(:spam_logs).dependent(:destroy) } it { is_expected.to have_many(:todos).dependent(:destroy) } it { is_expected.to have_many(:award_emoji).dependent(:destroy) } + + describe '#group_members' do + it 'does not include group memberships for which user is a requester' do + user = create(:user) + group = create(:group, :public) + group.request_access(user) + + expect(user.group_members).to be_empty + end + end + + describe '#project_members' do + it 'does not include project memberships for which user is a requester' do + user = create(:user) + project = create(:project, :public) + project.request_access(user) + + expect(user.project_members).to be_empty + end + end end describe 'validations' do diff --git a/spec/services/search/snippet_service_spec.rb b/spec/services/search/snippet_service_spec.rb new file mode 100644 index 00000000000..14f3301d9f4 --- /dev/null +++ b/spec/services/search/snippet_service_spec.rb @@ -0,0 +1,59 @@ +require 'spec_helper' + +describe Search::SnippetService, services: true do + let(:author) { create(:author) } + let(:project) { create(:empty_project) } + + let!(:public_snippet) { create(:snippet, :public, content: 'password: XXX') } + let!(:internal_snippet) { create(:snippet, :internal, content: 'password: XXX') } + let!(:private_snippet) { create(:snippet, :private, content: 'password: XXX', author: author) } + + let!(:project_public_snippet) { create(:snippet, :public, project: project, content: 'password: XXX') } + let!(:project_internal_snippet) { create(:snippet, :internal, project: project, content: 'password: XXX') } + let!(:project_private_snippet) { create(:snippet, :private, project: project, content: 'password: XXX') } + + describe '#execute' do + context 'unauthenticated' do + it 'returns public snippets only' do + search = described_class.new(nil, search: 'password') + results = search.execute + + expect(results.objects('snippet_blobs')).to match_array [public_snippet, project_public_snippet] + end + end + + context 'authenticated' do + it 'returns only public & internal snippets for regular users' do + user = create(:user) + search = described_class.new(user, search: 'password') + results = search.execute + + expect(results.objects('snippet_blobs')).to match_array [public_snippet, internal_snippet, project_public_snippet, project_internal_snippet] + end + + it 'returns public, internal snippets and project private snippets for project members' do + member = create(:user) + project.team << [member, :developer] + search = described_class.new(member, search: 'password') + results = search.execute + + expect(results.objects('snippet_blobs')).to match_array [public_snippet, internal_snippet, project_public_snippet, project_internal_snippet, project_private_snippet] + end + + it 'returns public, internal and private snippets where user is the author' do + search = described_class.new(author, search: 'password') + results = search.execute + + expect(results.objects('snippet_blobs')).to match_array [public_snippet, internal_snippet, private_snippet, project_public_snippet, project_internal_snippet] + end + + it 'returns all snippets when user is admin' do + admin = create(:admin) + search = described_class.new(admin, search: 'password') + results = search.execute + + expect(results.objects('snippet_blobs')).to match_array [public_snippet, internal_snippet, private_snippet, project_public_snippet, project_internal_snippet, project_private_snippet] + end + end + end +end |