From ae92511e74b11619166c0303b125a86b77f3d7c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Mon, 11 Feb 2019 12:53:58 +0100 Subject: [PATCH 1/2] Secure vulerability and add specs --- app/policies/group_policy.rb | 1 - .../security-shared-project-private-group.yml | 5 +++ .../projects/group_links_controller_spec.rb | 1 + .../security/group/private_access_spec.rb | 32 +++++++++++++-- spec/policies/group_policy_spec.rb | 40 ++++++++++++++++--- 5 files changed, 68 insertions(+), 11 deletions(-) create mode 100644 changelogs/unreleased/security-shared-project-private-group.yml diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index 6b4e56ef5e4..09e0ce67b2d 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -47,7 +47,6 @@ class GroupPolicy < BasePolicy rule { admin } .enable :read_group rule { has_projects }.policy do - enable :read_group enable :read_label end diff --git a/changelogs/unreleased/security-shared-project-private-group.yml b/changelogs/unreleased/security-shared-project-private-group.yml new file mode 100644 index 00000000000..3b21daa5491 --- /dev/null +++ b/changelogs/unreleased/security-shared-project-private-group.yml @@ -0,0 +1,5 @@ +--- +title: Fixed ability to see private groups by users not belonging to given group +merge_request: +author: +type: security diff --git a/spec/controllers/projects/group_links_controller_spec.rb b/spec/controllers/projects/group_links_controller_spec.rb index 6b12a6ae720..eec876583da 100644 --- a/spec/controllers/projects/group_links_controller_spec.rb +++ b/spec/controllers/projects/group_links_controller_spec.rb @@ -85,6 +85,7 @@ describe Projects::GroupLinksController do project_id: project, link_group_id: group2.id, link_group_access: ProjectGroupLink.default_access) + end it 'does not share project with selected group' do diff --git a/spec/features/security/group/private_access_spec.rb b/spec/features/security/group/private_access_spec.rb index 4705cd12d23..3238e07fe15 100644 --- a/spec/features/security/group/private_access_spec.rb +++ b/spec/features/security/group/private_access_spec.rb @@ -27,7 +27,7 @@ describe 'Private Group access' do it { is_expected.to be_allowed_for(:developer).of(group) } it { is_expected.to be_allowed_for(:reporter).of(group) } it { is_expected.to be_allowed_for(:guest).of(group) } - it { is_expected.to be_allowed_for(project_guest) } + it { is_expected.to be_denied_for(project_guest) } it { is_expected.to be_denied_for(:user) } it { is_expected.to be_denied_for(:external) } it { is_expected.to be_denied_for(:visitor) } @@ -42,7 +42,7 @@ describe 'Private Group access' do it { is_expected.to be_allowed_for(:developer).of(group) } it { is_expected.to be_allowed_for(:reporter).of(group) } it { is_expected.to be_allowed_for(:guest).of(group) } - it { is_expected.to be_allowed_for(project_guest) } + it { is_expected.to be_denied_for(project_guest) } it { is_expected.to be_denied_for(:user) } it { is_expected.to be_denied_for(:external) } it { is_expected.to be_denied_for(:visitor) } @@ -58,7 +58,7 @@ describe 'Private Group access' do it { is_expected.to be_allowed_for(:developer).of(group) } it { is_expected.to be_allowed_for(:reporter).of(group) } it { is_expected.to be_allowed_for(:guest).of(group) } - it { is_expected.to be_allowed_for(project_guest) } + it { is_expected.to be_denied_for(project_guest) } it { is_expected.to be_denied_for(:user) } it { is_expected.to be_denied_for(:external) } it { is_expected.to be_denied_for(:visitor) } @@ -73,7 +73,7 @@ describe 'Private Group access' do it { is_expected.to be_allowed_for(:developer).of(group) } it { is_expected.to be_allowed_for(:reporter).of(group) } it { is_expected.to be_allowed_for(:guest).of(group) } - it { is_expected.to be_allowed_for(project_guest) } + it { is_expected.to be_denied_for(project_guest) } it { is_expected.to be_denied_for(:user) } it { is_expected.to be_denied_for(:external) } it { is_expected.to be_denied_for(:visitor) } @@ -93,4 +93,28 @@ describe 'Private Group access' do it { is_expected.to be_denied_for(:visitor) } it { is_expected.to be_denied_for(:external) } end + + describe 'GET /groups/:path for shared projects' do + let(:project) { create(:project, :public) } + before do + Projects::GroupLinks::CreateService.new( + project, + create(:user), + link_group_access: ProjectGroupLink::DEVELOPER + ).execute(group) + end + + subject { group_path(group) } + + it { is_expected.to be_allowed_for(:admin) } + it { is_expected.to be_allowed_for(:owner).of(group) } + it { is_expected.to be_allowed_for(:maintainer).of(group) } + it { is_expected.to be_allowed_for(:developer).of(group) } + it { is_expected.to be_allowed_for(:reporter).of(group) } + it { is_expected.to be_allowed_for(:guest).of(group) } + it { is_expected.to be_denied_for(project_guest) } + it { is_expected.to be_denied_for(:user) } + it { is_expected.to be_denied_for(:external) } + it { is_expected.to be_denied_for(:visitor) } + end end diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb index 9d0093e8159..e5d1bfac5f7 100644 --- a/spec/policies/group_policy_spec.rb +++ b/spec/policies/group_policy_spec.rb @@ -73,6 +73,38 @@ describe GroupPolicy do end end + context 'with no user and public project' do + let(:project) { create(:project, :public) } + let(:user) { create(:user) } + let(:current_user) { nil } + + before do + Projects::GroupLinks::CreateService.new( + project, + user, + link_group_access: ProjectGroupLink::DEVELOPER + ).execute(group) + end + + it { expect_disallowed(:read_group) } + end + + context 'with foreign user and public project' do + let(:project) { create(:project, :public) } + let(:user) { create(:user) } + let(:current_user) { create(:user) } + + before do + Projects::GroupLinks::CreateService.new( + project, + user, + link_group_access: ProjectGroupLink::DEVELOPER + ).execute(group) + end + + it { expect_disallowed(:read_group) } + end + context 'has projects' do let(:current_user) { create(:user) } let(:project) { create(:project, namespace: group) } @@ -81,17 +113,13 @@ describe GroupPolicy do project.add_developer(current_user) end - it do - expect_allowed(:read_group, :read_label) - end + it { expect_allowed(:read_label) } context 'in subgroups', :nested_groups do let(:subgroup) { create(:group, :private, parent: group) } let(:project) { create(:project, namespace: subgroup) } - it do - expect_allowed(:read_group, :read_label) - end + it { expect_allowed(:read_label) } end end -- GitLab From f6f61c325373f47b8c119136319fc324422f3a3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Thu, 28 Feb 2019 17:11:47 +0100 Subject: [PATCH 2/2] Remove whitespace --- spec/controllers/projects/group_links_controller_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/controllers/projects/group_links_controller_spec.rb b/spec/controllers/projects/group_links_controller_spec.rb index eec876583da..6b12a6ae720 100644 --- a/spec/controllers/projects/group_links_controller_spec.rb +++ b/spec/controllers/projects/group_links_controller_spec.rb @@ -85,7 +85,6 @@ describe Projects::GroupLinksController do project_id: project, link_group_id: group2.id, link_group_access: ProjectGroupLink.default_access) - end it 'does not share project with selected group' do -- GitLab