From 3a321c80031630c3687cfdc08699bb0824a3dbfa 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] Secure vulerability and add specs --- app/policies/group_policy.rb | 1 - .../security-shared-project-private-group.yml | 5 +++ .../projects/group_links_controller_spec.rb | 2 + .../security/group/private_access_spec.rb | 32 +++++++++++++-- spec/policies/group_policy_spec.rb | 40 ++++++++++++++++--- 5 files changed, 69 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 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/controllers/projects/group_links_controller_spec.rb b/spec/controllers/projects/group_links_controller_spec.rb index 675eeff8d12..b985a9f2846 100644 --- a/spec/controllers/projects/group_links_controller_spec.rb +++ b/spec/controllers/projects/group_links_controller_spec.rb @@ -67,6 +67,8 @@ context 'when project group id equal link group id' do before do + group2.add_developer(user) + post(:create, params: { namespace_id: project.namespace, project_id: project, 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ def expect_disallowed(*permissions) 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 @@ def expect_disallowed(*permissions) 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