Commit 91f43587 authored by Sean McGivern's avatar Sean McGivern Committed by DJ Mountney

Merge branch 'jej-group-name-disclosure' into 'security'

Prevent private group disclosure via parent_id

See merge request !2077
parent 60c0c0f3
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
module Groups module Groups
class UpdateService < Groups::BaseService class UpdateService < Groups::BaseService
def execute def execute
reject_parent_id!
# check that user is allowed to set specified visibility_level # check that user is allowed to set specified visibility_level
new_visibility = params[:visibility_level] new_visibility = params[:visibility_level]
if new_visibility && new_visibility.to_i != group.visibility_level if new_visibility && new_visibility.to_i != group.visibility_level
...@@ -22,5 +24,11 @@ module Groups ...@@ -22,5 +24,11 @@ module Groups
false false
end end
end end
private
def reject_parent_id!
params.except!(:parent_id)
end
end end
end end
- 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 = root_url
- group_path << parent.full_path + '/' if parent - group_path << parent.full_path + '/' if parent
- if @group.persisted? - if @group.persisted?
......
---
title: Fixed private group name disclosure via new/update forms
merge_request:
author:
...@@ -100,6 +100,16 @@ feature 'Group', feature: true do ...@@ -100,6 +100,16 @@ feature 'Group', feature: true do
end end
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 describe 'group edit' do
let(:group) { create(:group) } let(:group) { create(:group) }
let(:path) { edit_group_path(group) } let(:path) { edit_group_path(group) }
......
...@@ -36,6 +36,20 @@ describe Groups::UpdateService, services: true do ...@@ -36,6 +36,20 @@ describe Groups::UpdateService, services: true do
end end
end 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 end
context "unauthorized visibility_level validation" do context "unauthorized visibility_level validation" do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment