diff options
author | Bob Van Landuyt <bob@gitlab.com> | 2018-09-24 14:47:23 +0000 |
---|---|---|
committer | Bob Van Landuyt <bob@vanlanduyt.co> | 2018-09-24 16:54:48 +0200 |
commit | 5a20bb1db2c46c2f06ebbb9401e8bf0e6787d856 (patch) | |
tree | a84cadacb0b3b3ad4c7eb1bf238ea47b08b1db61 /spec | |
parent | d6df5da26ac8ccf0ea4e81a9949f4a0512fd9a57 (diff) | |
download | gitlab-ce-5a20bb1db2c46c2f06ebbb9401e8bf0e6787d856.tar.gz |
Merge branch 'security-11-3-6881-project-group-approvers-leaks-private-group-info-ce' into 'security-11-3'
[11.3] CE: Project group approvers leaks private group info
See merge request gitlab/gitlabhq!2525
Diffstat (limited to 'spec')
-rw-r--r-- | spec/models/group_spec.rb | 42 | ||||
-rw-r--r-- | spec/requests/api/groups_spec.rb | 14 | ||||
-rw-r--r-- | spec/rubocop/cop/group_public_or_visible_to_user_spec.rb | 28 | ||||
-rw-r--r-- | spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb | 1 | ||||
-rw-r--r-- | spec/views/projects/merge_requests/edit.html.haml_spec.rb | 1 |
5 files changed, 70 insertions, 16 deletions
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 0729eb99e78..1bf8f89e126 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -169,22 +169,42 @@ describe Group do end end - describe '.visible_to_user' do - let!(:group) { create(:group) } - let!(:user) { create(:user) } + describe '.public_or_visible_to_user' do + let!(:private_group) { create(:group, :private) } + let!(:internal_group) { create(:group, :internal) } - subject { described_class.visible_to_user(user) } + subject { described_class.public_or_visible_to_user(user) } - describe 'when the user has access to a group' do - before do - group.add_user(user, Gitlab::Access::MAINTAINER) - end + context 'when user is nil' do + let!(:user) { nil } - it { is_expected.to eq([group]) } + it { is_expected.to match_array([group]) } end - describe 'when the user does not have access to any groups' do - it { is_expected.to eq([]) } + context 'when user' do + let!(:user) { create(:user) } + + context 'when user does not have access to any private group' do + it { is_expected.to match_array([internal_group, group]) } + end + + context 'when user is a member of private group' do + before do + private_group.add_user(user, Gitlab::Access::DEVELOPER) + end + + it { is_expected.to match_array([private_group, internal_group, group]) } + end + + context 'when user is a member of private subgroup', :postgresql do + let!(:private_subgroup) { create(:group, :private, parent: private_group) } + + before do + private_subgroup.add_user(user, Gitlab::Access::DEVELOPER) + end + + it { is_expected.to match_array([private_subgroup, internal_group, group]) } + end end end diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index 3a8948f8477..3802b5c6848 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -155,7 +155,7 @@ describe API::Groups do expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers expect(json_response).to be_an Array - expect(response_groups).to eq(Group.visible_to_user(user1).order(:name).pluck(:name)) + expect(response_groups).to eq(groups_visible_to_user(user1).order(:name).pluck(:name)) end it "sorts in descending order when passed" do @@ -164,7 +164,7 @@ describe API::Groups do expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers expect(json_response).to be_an Array - expect(response_groups).to eq(Group.visible_to_user(user1).order(name: :desc).pluck(:name)) + expect(response_groups).to eq(groups_visible_to_user(user1).order(name: :desc).pluck(:name)) end it "sorts by path in order_by param" do @@ -173,7 +173,7 @@ describe API::Groups do expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers expect(json_response).to be_an Array - expect(response_groups).to eq(Group.visible_to_user(user1).order(:path).pluck(:name)) + expect(response_groups).to eq(groups_visible_to_user(user1).order(:path).pluck(:name)) end it "sorts by id in the order_by param" do @@ -182,7 +182,7 @@ describe API::Groups do expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers expect(json_response).to be_an Array - expect(response_groups).to eq(Group.visible_to_user(user1).order(:id).pluck(:name)) + expect(response_groups).to eq(groups_visible_to_user(user1).order(:id).pluck(:name)) end it "sorts also by descending id with pagination fix" do @@ -191,7 +191,7 @@ describe API::Groups do expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers expect(json_response).to be_an Array - expect(response_groups).to eq(Group.visible_to_user(user1).order(id: :desc).pluck(:name)) + expect(response_groups).to eq(groups_visible_to_user(user1).order(id: :desc).pluck(:name)) end it "sorts identical keys by id for good pagination" do @@ -211,6 +211,10 @@ describe API::Groups do expect(json_response).to be_an Array expect(response_groups_ids).to eq(Group.select { |group| group['name'] == 'same-name' }.map { |group| group['id'] }.sort) end + + def groups_visible_to_user(user) + Group.where(id: user.authorized_groups.select(:id).reorder(nil)) + end end context 'when using owned in the request' do diff --git a/spec/rubocop/cop/group_public_or_visible_to_user_spec.rb b/spec/rubocop/cop/group_public_or_visible_to_user_spec.rb new file mode 100644 index 00000000000..7b5235a3da7 --- /dev/null +++ b/spec/rubocop/cop/group_public_or_visible_to_user_spec.rb @@ -0,0 +1,28 @@ +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../rubocop/cop/group_public_or_visible_to_user' + +describe RuboCop::Cop::GroupPublicOrVisibleToUser do + include CopHelper + + subject(:cop) { described_class.new } + + it 'flags the use of Group.public_or_visible_to_user with a constant receiver' do + inspect_source('Group.public_or_visible_to_user') + + expect(cop.offenses.size).to eq(1) + end + + it 'does not flat the use of public_or_visible_to_user with a constant that is not Group' do + inspect_source('Project.public_or_visible_to_user') + + expect(cop.offenses.size).to eq(0) + end + + it 'does not flag the use of Group.public_or_visible_to_user with a send receiver' do + inspect_source('foo.public_or_visible_to_user') + + expect(cop.offenses.size).to eq(0) + end +end diff --git a/spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb b/spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb index 8befae39d3a..0206928a211 100644 --- a/spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb +++ b/spec/views/projects/merge_requests/creations/_new_submit.html.haml_spec.rb @@ -12,6 +12,7 @@ describe 'projects/merge_requests/creations/_new_submit.html.haml' do assign(:hidden_commit_count, 0) assign(:total_commit_count, merge_request.commits.count) assign(:project, merge_request.target_project) + assign(:mr_presenter, merge_request.present(current_user: merge_request.author)) allow(view).to receive(:can?).and_return(true) allow(view).to receive(:url_for).and_return('#') diff --git a/spec/views/projects/merge_requests/edit.html.haml_spec.rb b/spec/views/projects/merge_requests/edit.html.haml_spec.rb index 9b74a7e1946..c13eab30054 100644 --- a/spec/views/projects/merge_requests/edit.html.haml_spec.rb +++ b/spec/views/projects/merge_requests/edit.html.haml_spec.rb @@ -24,6 +24,7 @@ describe 'projects/merge_requests/edit.html.haml' do before do assign(:project, project) assign(:merge_request, closed_merge_request) + assign(:mr_presenter, closed_merge_request.present(current_user: user)) allow(view).to receive(:can?).and_return(true) allow(view).to receive(:current_user) |