From c75b5a56140605308237d94038947b1b27941521 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 9 Jan 2017 21:20:55 +0100 Subject: [PATCH] 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 --- apps/dav/lib/Connector/Sabre/Principal.php | 2 +- apps/dav/lib/DAV/GroupPrincipalBackend.php | 6 +-- .../unit/Connector/Sabre/PrincipalTest.php | 45 +++++++++---------- .../dav/tests/unit/DAV/GroupPrincipalTest.php | 29 +++++++++--- 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 a592ed8091..9da416312d 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 f8dc1e6088..f6295d3b38 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 0385680702..8d7485769f 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 e532ed164e..180fc31040 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; } }