Properly handle groups with a /

If a group contains a slash the principal URI becomes
principals/groups/foo/bar. Now the URI is plit on '/' so this creates
issues ;)

Fixes #2957

* Add tests for groups with /

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

4 files changed, 48 insertions, 34 deletions

diff --git a/apps/dav/lib/Connector/Sabre/Principal.php b/apps/dav/lib/Connector/Sabre/Principal.php
index a592ed80918..9da416312d0 100644
--- a/apps/dav/lib/Connector/Sabre/Principal.php
+++ b/apps/dav/lib/Connector/Sabre/Principal.php
@@ -150,7 +150,7 @@ class Principal implements BackendInterface {
 				$groups = $this->groupManager->getUserGroups($user);
 				$groups = array_map(function($group) {
 					/** @var IGroup $group */
-					return 'principals/groups/' . $group->getGID();
+					return 'principals/groups/' . urlencode($group->getGID());
 				}, $groups);
 
 				return $groups;
diff --git a/apps/dav/lib/DAV/GroupPrincipalBackend.php b/apps/dav/lib/DAV/GroupPrincipalBackend.php
index f8dc1e60881..f6295d3b386 100644
--- a/apps/dav/lib/DAV/GroupPrincipalBackend.php
+++ b/apps/dav/lib/DAV/GroupPrincipalBackend.php
@@ -76,14 +76,14 @@ class GroupPrincipalBackend implements BackendInterface {
 	 * @return array
 	 */
 	public function getPrincipalByPath($path) {
-		$elements = explode('/', $path);
+		$elements = explode('/', $path,  3);
 		if ($elements[0] !== 'principals') {
 			return null;
 		}
 		if ($elements[1] !== 'groups') {
 			return null;
 		}
-		$name = $elements[2];
+		$name = urldecode($elements[2]);
 		$group = $this->groupManager->get($name);
 
 		if (!is_null($group)) {
@@ -179,7 +179,7 @@ class GroupPrincipalBackend implements BackendInterface {
 	protected function groupToPrincipal($group) {
 		$groupId = $group->getGID();
 		$principal = [
-			'uri' => "principals/groups/$groupId",
+			'uri' => 'principals/groups/' . urlencode($groupId),
 			'{DAV:}displayname' => $groupId,
 		];
 
diff --git a/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php b/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php
index 03856807026..8d7485769fa 100644
--- a/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php
+++ b/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php
@@ -25,6 +25,8 @@
 
 namespace OCA\DAV\Tests\unit\Connector\Sabre;
 
+use OC\User\User;
+use OCP\IGroup;
 use OCP\IGroupManager;
 use \Sabre\DAV\PropPatch;
 use OCP\IUserManager;
@@ -39,10 +41,8 @@ class PrincipalTest extends TestCase {
 	private $groupManager;
 
 	public function setUp() {
-		$this->userManager = $this->getMockBuilder('\OCP\IUserManager')
-			->disableOriginalConstructor()->getMock();
-		$this->groupManager = $this->getMockBuilder('\OCP\IGroupManager')
-			->disableOriginalConstructor()->getMock();
+		$this->userManager = $this->createMock(IUserManager::class);
+		$this->groupManager = $this->createMock(IGroupManager::class);
 
 		$this->connector = new \OCA\DAV\Connector\Sabre\Principal(
 			$this->userManager,
@@ -56,8 +56,7 @@ class PrincipalTest extends TestCase {
 	}
 
 	public function testGetPrincipalsByPrefixWithUsers() {
-		$fooUser = $this->getMockBuilder('\OC\User\User')
-			->disableOriginalConstructor()->getMock();
+		$fooUser = $this->createMock(User::class);
 		$fooUser
 				->expects($this->exactly(1))
 				->method('getUID')
@@ -70,8 +69,7 @@ class PrincipalTest extends TestCase {
 				->expects($this->exactly(1))
 				->method('getEMailAddress')
 				->will($this->returnValue(''));
-		$barUser = $this->getMockBuilder('\OC\User\User')
-			->disableOriginalConstructor()->getMock();
+		$barUser = $this->createMock(User::class);
 		$barUser
 			->expects($this->exactly(1))
 			->method('getUID')
@@ -113,8 +111,7 @@ class PrincipalTest extends TestCase {
 	}
 
 	public function testGetPrincipalsByPathWithoutMail() {
-		$fooUser = $this->getMockBuilder('\OC\User\User')
-			->disableOriginalConstructor()->getMock();
+		$fooUser = $this->createMock(User::class);
 		$fooUser
 			->expects($this->exactly(1))
 			->method('getUID')
@@ -134,8 +131,7 @@ class PrincipalTest extends TestCase {
 	}
 
 	public function testGetPrincipalsByPathWithMail() {
-		$fooUser = $this->getMockBuilder('\OC\User\User')
-			->disableOriginalConstructor()->getMock();
+		$fooUser = $this->createMock(User::class);
 		$fooUser
 				->expects($this->exactly(1))
 				->method('getEMailAddress')
@@ -171,8 +167,7 @@ class PrincipalTest extends TestCase {
 	}
 
 	public function testGetGroupMemberSet() {
-		$fooUser = $this->getMockBuilder('\OC\User\User')
-			->disableOriginalConstructor()->getMock();
+		$fooUser = $this->createMock(User::class);
 		$fooUser
 			->expects($this->exactly(1))
 			->method('getUID')
@@ -202,13 +197,15 @@ class PrincipalTest extends TestCase {
 	}
 
 	public function testGetGroupMembership() {
-		$fooUser = $this->getMockBuilder('\OC\User\User')
-			->disableOriginalConstructor()->getMock();
-		$group = $this->getMockBuilder('\OCP\IGroup')
-			->disableOriginalConstructor()->getMock();
-		$group->expects($this->once())
+		$fooUser = $this->createMock(User::class);
+		$group1 = $this->createMock(IGroup::class);
+		$group1->expects($this->once())
 			->method('getGID')
 			->willReturn('group1');
+		$group2 = $this->createMock(IGroup::class);
+		$group2->expects($this->once())
+			->method('getGID')
+			->willReturn('foo/bar');
 		$this->userManager
 			->expects($this->once())
 			->method('get')
@@ -217,12 +214,15 @@ class PrincipalTest extends TestCase {
 		$this->groupManager
 			->expects($this->once())
 			->method('getUserGroups')
+			->with($fooUser)
 			->willReturn([
-				$group
+				$group1,
+				$group2,
 			]);
 
 		$expectedResponse = [
-			'principals/groups/group1'
+			'principals/groups/group1',
+			'principals/groups/foo%2Fbar',
 		];
 		$response = $this->connector->getGroupMembership('principals/users/foo');
 		$this->assertSame($expectedResponse, $response);
@@ -259,8 +259,7 @@ class PrincipalTest extends TestCase {
 	}
 
 	public function testFindByUri() {
-		$fooUser = $this->getMockBuilder('\OC\User\User')
-			->disableOriginalConstructor()->getMock();
+		$fooUser = $this->createMock(User::class);
 		$fooUser
 			->expects($this->exactly(1))
 			->method('getUID')
diff --git a/apps/dav/tests/unit/DAV/GroupPrincipalTest.php b/apps/dav/tests/unit/DAV/GroupPrincipalTest.php
index e532ed164e9..180fc31040a 100644
--- a/apps/dav/tests/unit/DAV/GroupPrincipalTest.php
+++ b/apps/dav/tests/unit/DAV/GroupPrincipalTest.php
@@ -23,6 +23,7 @@
 
 namespace OCA\DAV\Tests\unit\DAV;
 
+use OC\Group\Group;
 use OCA\DAV\DAV\GroupPrincipalBackend;
 use OCP\IGroupManager;
 use PHPUnit_Framework_MockObject_MockObject;
@@ -37,8 +38,7 @@ class GroupPrincipalTest extends \Test\TestCase {
 	private $connector;
 
 	public function setUp() {
-		$this->groupManager = $this->getMockBuilder('\OCP\IGroupManager')
-			->disableOriginalConstructor()->getMock();
+		$this->groupManager = $this->createMock(IGroupManager::class);
 
 		$this->connector = new GroupPrincipalBackend($this->groupManager);
 		parent::setUp();
@@ -126,6 +126,22 @@ class GroupPrincipalTest extends \Test\TestCase {
 		$this->assertSame(null, $response);
 	}
 
+	public function testGetPrincipalsByPathGroupWithSlash() {
+		$group1 = $this->mockGroup('foo/bar');
+		$this->groupManager
+			->expects($this->once())
+			->method('get')
+			->with('foo/bar')
+			->will($this->returnValue($group1));
+
+		$expectedResponse = [
+			'uri' => 'principals/groups/foo%2Fbar',
+			'{DAV:}displayname' => 'foo/bar'
+		];
+		$response = $this->connector->getPrincipalByPath('principals/groups/foo/bar');
+		$this->assertSame($expectedResponse, $response);
+	}
+
 	public function testGetGroupMemberSet() {
 		$response = $this->connector->getGroupMemberSet('principals/groups/foo');
 		$this->assertSame([], $response);
@@ -153,15 +169,14 @@ class GroupPrincipalTest extends \Test\TestCase {
 	}
 
 	/**
-	 * @return PHPUnit_Framework_MockObject_MockObject
+	 * @return Group|\PHPUnit_Framework_MockObject_MockObject
 	 */
 	private function mockGroup($gid) {
-		$fooUser = $this->getMockBuilder('\OC\Group\Group')
-			->disableOriginalConstructor()->getMock();
-		$fooUser
+		$fooGroup = $this->createMock(Group::class);
+		$fooGroup
 			->expects($this->exactly(1))
 			->method('getGID')
 			->will($this->returnValue($gid));
-		return $fooUser;
+		return $fooGroup;
 	}
 }