From 5a21b2fb15ed9c675594f0f990765bd4f97155c7 Mon Sep 17 00:00:00 2001
From: Mateusz Uzdowski <mateusz@silverstripe.com>
Date: Fri, 11 Dec 2015 13:55:09 +1300
Subject: [PATCH] BUG Guard against users being added to all groups on unsaved
 Group.

If ->Members()->add() is called on an unsaved group (with ID 0), the
collateFamilyIDs() will errorneously return all root Groups thinking
it's looking for Groups with ParentID=0. As a result, the Member will be
added to all root groups, instead of just the selected group and all its
children.
---
 security/Group.php           |  9 +++++++++
 tests/security/GroupTest.php | 14 ++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/security/Group.php b/security/Group.php
index 2a6f77a3a..6b80cb744 100755
--- a/security/Group.php
+++ b/security/Group.php
@@ -263,6 +263,10 @@ class Group extends DataObject {
 
 		// First get direct members as a base result
 		$result = $this->DirectMembers();
+
+		// Unsaved group cannot have child groups because its ID is still 0.
+		if(!$this->exists()) return $result;
+
 		// Remove the default foreign key filter in prep for re-applying a filter containing all children groups.
 		// Filters are conjunctive in DataQuery by default, so this filter would otherwise overrule any less specific
 		// ones.
@@ -288,9 +292,14 @@ class Group extends DataObject {
 	/**
 	 * Return a set of this record's "family" of IDs - the IDs of
 	 * this record and all its descendants.
+	 *
 	 * @return array
 	 */
 	public function collateFamilyIDs() {
+		if (!$this->exists()) {
+			throw new \InvalidArgumentException("Cannot call collateFamilyIDs on unsaved Group.");
+		}
+
 		$familyIDs = array();
 		$chunkToAdd = array($this->ID);
 
diff --git a/tests/security/GroupTest.php b/tests/security/GroupTest.php
index ef9dd7006..dd5d0eaea 100644
--- a/tests/security/GroupTest.php
+++ b/tests/security/GroupTest.php
@@ -69,6 +69,20 @@ class GroupTest extends FunctionalTest {
 
 	}
 
+	public function testUnsavedGroups() {
+		$member = $this->objFromFixture('GroupTest_Member', 'admin');
+		$group = new Group();
+
+		// Can save user to unsaved group
+		$group->Members()->add($member);
+		$this->assertEquals(array($member->ID), array_values($group->Members()->getIDList()));
+
+		// Persists after writing to DB
+		$group->write();
+		$group = Group::get()->byID($group->ID);
+		$this->assertEquals(array($member->ID), array_values($group->Members()->getIDList()));
+	}
+
 	public function testCollateAncestorIDs() {
 		$parentGroup = $this->objFromFixture('Group', 'parentgroup');
 		$childGroup = $this->objFromFixture('Group', 'childgroup');