Merge remote-tracking branch 'dev/master'

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

app/controllers/dashboard/groups_controller.rb

 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

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
 

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

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

app/views/admin/users/groups.html.haml

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

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'
 

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

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

spec/services/search/snippet_service_spec.rb

+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