diff options
| -rw-r--r-- | app/policies/group_policy.rb | 1 | ||||
| -rw-r--r-- | changelogs/unreleased/security-shared-project-private-group.yml | 5 | ||||
| -rw-r--r-- | spec/features/security/group/private_access_spec.rb | 32 | ||||
| -rw-r--r-- | spec/policies/group_policy_spec.rb | 40 |
4 files changed, 67 insertions, 11 deletions
diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index c25766a5af8..55d8da78dc8 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -53,7 +53,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/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 be1804c5ce0..4c31ff30fc6 100644 --- a/spec/policies/group_policy_spec.rb +++ b/spec/policies/group_policy_spec.rb @@ -74,6 +74,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) } @@ -82,17 +114,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 |