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>

diff --git a/apps/dav/lib/Connector/Sabre/Principal.php b/apps/dav/lib/Connector/Sabre/Principal.php
index a592ed80918094fc97772f3e13c4f9c4b87b0fff..9da416312d00ea6ce49f5877d21583c4d5a6e4ef 100644 (file)
--- 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 f8dc1e6088136a8153b674434683dee301b0bfbd..f6295d3b386ffc7617216d704b565e15742502fb 100644 (file)
--- 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 03856807026183adc0bf33d514974847136c1ed5..8d7485769faed174658e8d0b1e2bfd05e4ca4d96 100644 (file)
--- 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 e532ed164e92acc07aadedd27a696e8e2312cae3..180fc31040a4cbb07f36e0d56ab392113d05818d 100644 (file)
--- 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;
        }
 }