diff options
| -rw-r--r-- | app/finders/group_finder.rb | 17 | ||||
| -rw-r--r-- | app/services/groups/update_service.rb | 8 | ||||
| -rw-r--r-- | app/views/shared/_group_form.html.haml | 2 | ||||
| -rw-r--r-- | changelogs/unreleased/jej-group-name-disclosure.yml | 4 | ||||
| -rw-r--r-- | spec/features/groups_spec.rb | 10 | ||||
| -rw-r--r-- | spec/services/groups/update_service_spec.rb | 14 |
6 files changed, 54 insertions, 1 deletions
diff --git a/app/finders/group_finder.rb b/app/finders/group_finder.rb new file mode 100644 index 00000000000..24c84d2d1aa --- /dev/null +++ b/app/finders/group_finder.rb @@ -0,0 +1,17 @@ +class GroupFinder + include Gitlab::Allowable + + def initialize(current_user) + @current_user = current_user + end + + def execute(*params) + group = Group.find_by(*params) + + if can?(@current_user, :read_group, group) + group + else + nil + end + end +end diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb index 4e878ec556a..1d65c76d282 100644 --- a/app/services/groups/update_service.rb +++ b/app/services/groups/update_service.rb @@ -1,6 +1,8 @@ module Groups class UpdateService < Groups::BaseService def execute + reject_parent_id! + # check that user is allowed to set specified visibility_level new_visibility = params[:visibility_level] if new_visibility && new_visibility.to_i != group.visibility_level @@ -22,5 +24,11 @@ module Groups false end end + + private + + def reject_parent_id! + params.except!(:parent_id) + end end end diff --git a/app/views/shared/_group_form.html.haml b/app/views/shared/_group_form.html.haml index 7974eb67f0f..8869d510aef 100644 --- a/app/views/shared/_group_form.html.haml +++ b/app/views/shared/_group_form.html.haml @@ -1,4 +1,4 @@ -- parent = Group.find_by(id: params[:parent_id] || @group.parent_id) +- parent = GroupFinder.new(current_user).execute(id: params[:parent_id] || @group.parent_id) - group_path = root_url - group_path << parent.full_path + '/' if parent - if @group.persisted? diff --git a/changelogs/unreleased/jej-group-name-disclosure.yml b/changelogs/unreleased/jej-group-name-disclosure.yml new file mode 100644 index 00000000000..9b8ab7082ef --- /dev/null +++ b/changelogs/unreleased/jej-group-name-disclosure.yml @@ -0,0 +1,4 @@ +--- +title: Fixed private group name disclosure via new/update forms +merge_request: +author: diff --git a/spec/features/groups_spec.rb b/spec/features/groups_spec.rb index 144d069b632..c90cc06a8f5 100644 --- a/spec/features/groups_spec.rb +++ b/spec/features/groups_spec.rb @@ -100,6 +100,16 @@ feature 'Group', feature: true do end end + it 'checks permissions to avoid exposing groups by parent_id' do + group = create(:group, :private, path: 'secret-group') + + logout + login_as(:user) + visit new_group_path(parent_id: group.id) + + expect(page).not_to have_content('secret-group') + end + describe 'group edit' do let(:group) { create(:group) } let(:path) { edit_group_path(group) } diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 91ec224d1c4..f6ad5cebd2c 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -36,6 +36,20 @@ describe Groups::UpdateService, services: true do end end end + + context "with parent_id user doesn't have permissions for" do + let(:service) { described_class.new(public_group, user, parent_id: private_group.id) } + + before do + service.execute + end + + it 'does not update parent_id' do + updated_group = public_group.reload + + expect(updated_group.parent_id).to be_nil + end + end end context "unauthorized visibility_level validation" do |