From 6802e2b59a8973b1e7b9c3aaafc5073df51677f9 Mon Sep 17 00:00:00 2001 From: Georg Ehrke Date: Mon, 4 Dec 2017 15:02:55 +0100 Subject: [PATCH] Principal search: Take sharing settings into account Signed-off-by: Georg Ehrke --- apps/dav/appinfo/v1/caldav.php | 2 + apps/dav/appinfo/v1/carddav.php | 2 + apps/dav/lib/Command/CreateCalendar.php | 4 +- apps/dav/lib/Connector/Sabre/Principal.php | 109 ++++++++-- apps/dav/lib/RootCollection.php | 5 +- .../unit/Connector/Sabre/PrincipalTest.php | 188 ++++++++++++++++-- 6 files changed, 275 insertions(+), 35 deletions(-) diff --git a/apps/dav/appinfo/v1/caldav.php b/apps/dav/appinfo/v1/caldav.php index d9851bf92c..b4907ce94c 100644 --- a/apps/dav/appinfo/v1/caldav.php +++ b/apps/dav/appinfo/v1/caldav.php @@ -44,6 +44,8 @@ $authBackend = new Auth( $principalBackend = new Principal( \OC::$server->getUserManager(), \OC::$server->getGroupManager(), + \OC::$server->getShareManager(), + \OC::$server->getUserSession(), 'principals/' ); $db = \OC::$server->getDatabaseConnection(); diff --git a/apps/dav/appinfo/v1/carddav.php b/apps/dav/appinfo/v1/carddav.php index 8633b4211e..eddc7a794a 100644 --- a/apps/dav/appinfo/v1/carddav.php +++ b/apps/dav/appinfo/v1/carddav.php @@ -45,6 +45,8 @@ $authBackend = new Auth( $principalBackend = new Principal( \OC::$server->getUserManager(), \OC::$server->getGroupManager(), + \OC::$server->getShareManager(), + \OC::$server->getUserSession(), 'principals/' ); $db = \OC::$server->getDatabaseConnection(); diff --git a/apps/dav/lib/Command/CreateCalendar.php b/apps/dav/lib/Command/CreateCalendar.php index 190e4aa339..1cbd7b6094 100644 --- a/apps/dav/lib/Command/CreateCalendar.php +++ b/apps/dav/lib/Command/CreateCalendar.php @@ -75,7 +75,9 @@ class CreateCalendar extends Command { } $principalBackend = new Principal( $this->userManager, - $this->groupManager + $this->groupManager, + \OC::$server->getShareManager(), + \OC::$server->getUserSession() ); $random = \OC::$server->getSecureRandom(); $logger = \OC::$server->getLogger(); diff --git a/apps/dav/lib/Connector/Sabre/Principal.php b/apps/dav/lib/Connector/Sabre/Principal.php index 1741204835..06fb10f7e2 100644 --- a/apps/dav/lib/Connector/Sabre/Principal.php +++ b/apps/dav/lib/Connector/Sabre/Principal.php @@ -34,6 +34,8 @@ use OCP\IGroup; use OCP\IGroupManager; use OCP\IUser; use OCP\IUserManager; +use OCP\IUserSession; +use OCP\Share\IManager as IShareManager; use Sabre\DAV\Exception; use \Sabre\DAV\PropPatch; use Sabre\DAVACL\PrincipalBackend\BackendInterface; @@ -47,6 +49,12 @@ class Principal implements BackendInterface { /** @var IGroupManager */ private $groupManager; + /** @var IShareManager */ + private $shareManager; + + /** @var IUserSession */ + private $userSession; + /** @var string */ private $principalPrefix; @@ -56,13 +64,19 @@ class Principal implements BackendInterface { /** * @param IUserManager $userManager * @param IGroupManager $groupManager + * @param IShareManager $shareManager + * @param IUserSession $userSession * @param string $principalPrefix */ public function __construct(IUserManager $userManager, IGroupManager $groupManager, + IShareManager $shareManager, + IUserSession $userSession, $principalPrefix = 'principals/users/') { $this->userManager = $userManager; $this->groupManager = $groupManager; + $this->shareManager = $shareManager; + $this->userSession = $userSession; $this->principalPrefix = trim($principalPrefix, '/'); $this->hasGroups = ($principalPrefix === 'principals/users/'); } @@ -106,7 +120,7 @@ class Principal implements BackendInterface { if ($prefix === $this->principalPrefix) { $user = $this->userManager->get($name); - if (!is_null($user)) { + if ($user !== null) { return $this->userToPrincipal($user); } } @@ -189,37 +203,65 @@ class Principal implements BackendInterface { * @param string $test * @return array */ - function searchUserPrincipals(array $searchProperties, $test = 'allof') { + protected function searchUserPrincipals(array $searchProperties, $test = 'allof') { $results = []; - //TODO: If more properties should be supported, hook this up to a db query + // If sharing is disabled, return the empty array + if (!$this->shareManager->shareApiEnabled()) { + return []; + } + + // If sharing is restricted to group members only, + // return only members that have groups in common + $restrictGroups = false; + if ($this->shareManager->shareWithGroupMembersOnly()) { + $user = $this->userSession->getUser(); + if (!$user) { + return []; + } + + $restrictGroups = $this->groupManager->getUserGroupIds($user); + } + foreach ($searchProperties as $prop => $value) { switch ($prop) { case '{http://sabredav.org/ns}email-address': $users = $this->userManager->getByEmail($value); - $results[] = array_map(function ($user) { - return $this->principalPrefix . '/' . $user->getUID(); - }, $users); + + $results[] = array_reduce($users, function(array $carry, IUser $user) use ($restrictGroups) { + // is sharing restricted to groups only? + if ($restrictGroups !== false) { + $userGroups = $this->groupManager->getUserGroupIds($user); + if (count(array_intersect($userGroups, $restrictGroups)) === 0) { + return $carry; + } + } + + $carry[] = $this->principalPrefix . '/' . $user->getUID(); + return $carry; + }, []); break; + default: $results[] = []; break; } } - if (count($results) == 1) { + // results is an array of arrays, so this is not the first search result + // but the results of the first searchProperty + if (count($results) === 1) { return $results[0]; } switch ($test) { - case 'allof': - - return array_intersect(...$results); case 'anyof': return array_unique(array_merge(...$results)); - } - return []; + case 'allof': + default: + return array_intersect(...$results); + } } /** @@ -229,13 +271,14 @@ class Principal implements BackendInterface { * @return array */ function searchPrincipals($prefixPath, array $searchProperties, $test = 'allof') { - if (count($searchProperties) == 0) { + if (count($searchProperties) === 0) { return []; } switch ($prefixPath) { case 'principals/users': return $this->searchUserPrincipals($searchProperties, $test); + default: return []; } @@ -247,17 +290,43 @@ class Principal implements BackendInterface { * @return string */ function findByUri($uri, $principalPrefix) { - if (substr($uri, 0, 7) === 'mailto:') { - if ($principalPrefix === principals/users) { - $email = substr($uri, 7); - $users = $this->userManager->getByEmail($email); - if (count($users) === 1) { - return $this->principalPrefix . '/' . $users[0]->getUID(); + // If sharing is disabled, return null as in user not found + if (!$this->shareManager->shareApiEnabled()) { + return null; + } + + // If sharing is restricted to group members only, + // return only members that have groups in common + $restrictGroups = false; + if ($this->shareManager->shareWithGroupMembersOnly()) { + $user = $this->userSession->getUser(); + if (!$user) { + return null; + } + + $restrictGroups = $this->groupManager->getUserGroupIds($user); + } + + if (strpos($uri, 'mailto:') === 0) { + if ($principalPrefix === 'principals/users') { + $users = $this->userManager->getByEmail(substr($uri, 7)); + if (count($users) !== 1) { + return null; } + $user = $users[0]; + + if ($restrictGroups !== false) { + $userGroups = $this->groupManager->getUserGroupIds($user); + if (count(array_intersect($userGroups, $restrictGroups)) === 0) { + return null; + } + } + + return $this->principalPrefix . '/' . $user->getUID(); } } - return ''; + return null; } /** diff --git a/apps/dav/lib/RootCollection.php b/apps/dav/lib/RootCollection.php index c0f7b1e497..f2e9350c10 100644 --- a/apps/dav/lib/RootCollection.php +++ b/apps/dav/lib/RootCollection.php @@ -43,11 +43,14 @@ class RootCollection extends SimpleCollection { $logger = \OC::$server->getLogger(); $userManager = \OC::$server->getUserManager(); $groupManager = \OC::$server->getGroupManager(); + $shareManager = \OC::$server->getShareManager(); $db = \OC::$server->getDatabaseConnection(); $dispatcher = \OC::$server->getEventDispatcher(); $userPrincipalBackend = new Principal( $userManager, - $groupManager + $groupManager, + $shareManager, + \OC::$server->getUserSession() ); $groupPrincipalBackend = new GroupPrincipalBackend($groupManager); // as soon as debug mode is enabled we allow listing of principals diff --git a/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php b/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php index 5b77a0694f..7b9929bc4f 100644 --- a/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php @@ -29,6 +29,9 @@ namespace OCA\DAV\Tests\unit\Connector\Sabre; use OC\User\User; use OCP\IGroup; use OCP\IGroupManager; +use OCP\IUser; +use OCP\IUserSession; +use OCP\Share\IManager; use \Sabre\DAV\PropPatch; use OCP\IUserManager; use Test\TestCase; @@ -40,14 +43,22 @@ class PrincipalTest extends TestCase { private $connector; /** @var IGroupManager | \PHPUnit_Framework_MockObject_MockObject */ private $groupManager; + /** @var IManager | \PHPUnit_Framework_MockObject_MockObject */ + private $shareManager; + /** @var IUserSession | \PHPUnit_Framework_MockObject_MockObject */ + private $userSession; public function setUp() { $this->userManager = $this->createMock(IUserManager::class); $this->groupManager = $this->createMock(IGroupManager::class); + $this->shareManager = $this->createMock(IManager::class); + $this->userSession = $this->createMock(IUserSession::class); $this->connector = new \OCA\DAV\Connector\Sabre\Principal( $this->userManager, - $this->groupManager); + $this->groupManager, + $this->shareManager, + $this->userSession); parent::setUp(); } @@ -255,21 +266,172 @@ class PrincipalTest extends TestCase { $this->assertSame(0, $this->connector->updatePrincipal('foo', new PropPatch(array()))); } - public function testSearchPrincipals() { + public function testSearchPrincipalsWithEmptySearchProperties() { $this->assertSame([], $this->connector->searchPrincipals('principals/users', [])); } - public function testFindByUri() { - $fooUser = $this->createMock(User::class); - $fooUser - ->expects($this->exactly(1)) - ->method('getUID') - ->will($this->returnValue('foo')); + public function testSearchPrincipalsWithWrongPrefixPath() { + $this->assertSame([], $this->connector->searchPrincipals('principals/groups', + ['{http://sabredav.org/ns}email-address' => 'foo'])); + } - $this->userManager->expects($this->once())->method('getByEmail')->willReturn([ - $fooUser - ]); - $ret = $this->connector->findByUri('mailto:foo@bar.net', 'principals/users'); - $this->assertSame('principals/users/foo', $ret); + /** + * @dataProvider searchPrincipalsDataProvider + */ + public function testSearchPrincipals($sharingEnabled, $groupsOnly, $result) { + $this->shareManager->expects($this->once()) + ->method('shareAPIEnabled') + ->will($this->returnValue($sharingEnabled)); + + if ($sharingEnabled) { + $this->shareManager->expects($this->once()) + ->method('shareWithGroupMembersOnly') + ->will($this->returnValue($groupsOnly)); + + if ($groupsOnly) { + $user = $this->createMock(IUser::class); + $this->userSession->expects($this->once()) + ->method('getUser') + ->will($this->returnValue($user)); + + $this->groupManager->expects($this->at(0)) + ->method('getUserGroupIds') + ->with($user) + ->will($this->returnValue(['group1', 'group2'])); + } + } else { + $this->shareManager->expects($this->never()) + ->method('shareWithGroupMembersOnly'); + $this->groupManager->expects($this->never()) + ->method($this->anything()); + } + + $user2 = $this->createMock(IUser::class); + $user2->method('getUID')->will($this->returnValue('user2')); + $user3 = $this->createMock(IUser::class); + $user3->method('getUID')->will($this->returnValue('user3')); + + if ($sharingEnabled) { + $this->userManager->expects($this->at(0)) + ->method('getByEmail') + ->with('user') + ->will($this->returnValue([$user2, $user3])); + } + + if ($sharingEnabled && $groupsOnly) { + $this->groupManager->expects($this->at(1)) + ->method('getUserGroupIds') + ->with($user2) + ->will($this->returnValue(['group1', 'group3'])); + $this->groupManager->expects($this->at(2)) + ->method('getUserGroupIds') + ->with($user3) + ->will($this->returnValue(['group3', 'group4'])); + } + + $this->assertEquals($result, $this->connector->searchPrincipals('principals/users', + ['{http://sabredav.org/ns}email-address' => 'user'])); + } + + public function searchPrincipalsDataProvider() { + return [ + [true, false, ['principals/users/user2', 'principals/users/user3']], + [true, true, ['principals/users/user2']], + [false, false, []], + ]; + } + + public function testFindByUriSharingApiDisabled() { + $this->shareManager->expects($this->once()) + ->method('shareApiEnabled') + ->will($this->returnValue(false)); + + $this->assertEquals(null, $this->connector->findByUri('mailto:user@foo.com', 'principals/users')); + } + + /** + * @dataProvider findByUriWithGroupRestrictionDataProvider + */ + public function testFindByUriWithGroupRestriction($uri, $email, $expects) { + $this->shareManager->expects($this->once()) + ->method('shareApiEnabled') + ->will($this->returnValue(true)); + + $this->shareManager->expects($this->once()) + ->method('shareWithGroupMembersOnly') + ->will($this->returnValue(true)); + + $user = $this->createMock(IUser::class); + $this->userSession->expects($this->once()) + ->method('getUser') + ->will($this->returnValue($user)); + + $this->groupManager->expects($this->at(0)) + ->method('getUserGroupIds') + ->with($user) + ->will($this->returnValue(['group1', 'group2'])); + + $user2 = $this->createMock(IUser::class); + $user2->method('getUID')->will($this->returnValue('user2')); + $user3 = $this->createMock(IUser::class); + $user3->method('getUID')->will($this->returnValue('user3')); + + $this->userManager->expects($this->once()) + ->method('getByEmail') + ->with($email) + ->will($this->returnValue([$email === 'user2@foo.bar' ? $user2 : $user3])); + + if ($email === 'user2@foo.bar') { + $this->groupManager->expects($this->at(1)) + ->method('getUserGroupIds') + ->with($user2) + ->will($this->returnValue(['group1', 'group3'])); + } else { + $this->groupManager->expects($this->at(1)) + ->method('getUserGroupIds') + ->with($user3) + ->will($this->returnValue(['group3', 'group3'])); + } + + $this->assertEquals($expects, $this->connector->findByUri($uri, 'principals/users')); + } + + public function findByUriWithGroupRestrictionDataProvider() { + return [ + ['mailto:user2@foo.bar', 'user2@foo.bar', 'principals/users/user2'], + ['mailto:user3@foo.bar', 'user3@foo.bar', null], + ]; + } + + /** + * @dataProvider findByUriWithoutGroupRestrictionDataProvider + */ + public function testFindByUriWithoutGroupRestriction($uri, $email, $expects) { + $this->shareManager->expects($this->once()) + ->method('shareApiEnabled') + ->will($this->returnValue(true)); + + $this->shareManager->expects($this->once()) + ->method('shareWithGroupMembersOnly') + ->will($this->returnValue(false)); + + $user2 = $this->createMock(IUser::class); + $user2->method('getUID')->will($this->returnValue('user2')); + $user3 = $this->createMock(IUser::class); + $user3->method('getUID')->will($this->returnValue('user3')); + + $this->userManager->expects($this->once()) + ->method('getByEmail') + ->with($email) + ->will($this->returnValue([$email === 'user2@foo.bar' ? $user2 : $user3])); + + $this->assertEquals($expects, $this->connector->findByUri($uri, 'principals/users')); + } + + public function findByUriWithoutGroupRestrictionDataProvider() { + return [ + ['mailto:user2@foo.bar', 'user2@foo.bar', 'principals/users/user2'], + ['mailto:user3@foo.bar', 'user3@foo.bar', 'principals/users/user3'], + ]; } }