From 91f43587a8c05a5c2955f0b5c464f03688552cb6 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Tue, 28 Mar 2017 11:09:44 +0000
Subject: [PATCH] Merge branch 'jej-group-name-disclosure' into 'security'

Prevent private group disclosure via parent_id

See merge request !2077
---
 app/finders/group_finder.rb                     | 17 +++++++++++++++++
 app/services/groups/update_service.rb           |  8 ++++++++
 app/views/shared/_group_form.html.haml          |  2 +-
 .../unreleased/jej-group-name-disclosure.yml    |  4 ++++
 spec/features/groups_spec.rb                    | 10 ++++++++++
 spec/services/groups/update_service_spec.rb     | 14 ++++++++++++++
 6 files changed, 54 insertions(+), 1 deletion(-)
 create mode 100644 app/finders/group_finder.rb
 create mode 100644 changelogs/unreleased/jej-group-name-disclosure.yml

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