From c3748cfee35e0c8122ea8fa73d452e7796f1a0e5 Mon Sep 17 00:00:00 2001 From: Georg Ehrke Date: Tue, 26 Nov 2019 16:37:57 +0100 Subject: [PATCH] respect shareapi_allow_share_dialog_user_enumeration in Principal backend for Sabre/DAV Signed-off-by: Georg Ehrke --- apps/dav/appinfo/v1/caldav.php | 1 + apps/dav/appinfo/v1/carddav.php | 1 + apps/dav/lib/Command/CreateCalendar.php | 3 +- apps/dav/lib/Connector/Sabre/Principal.php | 21 ++++ apps/dav/lib/RootCollection.php | 5 +- .../unit/CalDAV/AbstractCalDavBackend.php | 1 + .../tests/unit/CardDAV/CardDavBackendTest.php | 3 +- .../unit/Connector/Sabre/PrincipalTest.php | 102 ++++++++++++++++-- .../lib/AppInfo/Application.php | 3 +- .../lib/AppInfo/Application.php | 3 +- 10 files changed, 128 insertions(+), 15 deletions(-) diff --git a/apps/dav/appinfo/v1/caldav.php b/apps/dav/appinfo/v1/caldav.php index 8116453b11..f1086cc62b 100644 --- a/apps/dav/appinfo/v1/caldav.php +++ b/apps/dav/appinfo/v1/caldav.php @@ -48,6 +48,7 @@ $principalBackend = new Principal( \OC::$server->getUserSession(), \OC::$server->getAppManager(), \OC::$server->query(\OCA\DAV\CalDAV\Proxy\ProxyMapper::class), + \OC::$server->getConfig(), 'principals/' ); $db = \OC::$server->getDatabaseConnection(); diff --git a/apps/dav/appinfo/v1/carddav.php b/apps/dav/appinfo/v1/carddav.php index 40ee12f194..8c6b6fb201 100644 --- a/apps/dav/appinfo/v1/carddav.php +++ b/apps/dav/appinfo/v1/carddav.php @@ -49,6 +49,7 @@ $principalBackend = new Principal( \OC::$server->getUserSession(), \OC::$server->getAppManager(), \OC::$server->query(\OCA\DAV\CalDAV\Proxy\ProxyMapper::class), + \OC::$server->getConfig(), 'principals/' ); $db = \OC::$server->getDatabaseConnection(); diff --git a/apps/dav/lib/Command/CreateCalendar.php b/apps/dav/lib/Command/CreateCalendar.php index c6bfffb242..1efe420236 100644 --- a/apps/dav/lib/Command/CreateCalendar.php +++ b/apps/dav/lib/Command/CreateCalendar.php @@ -81,7 +81,8 @@ class CreateCalendar extends Command { \OC::$server->getShareManager(), \OC::$server->getUserSession(), \OC::$server->getAppManager(), - \OC::$server->query(ProxyMapper::class) + \OC::$server->query(ProxyMapper::class), + \OC::$server->getConfig() ); $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 5c61b8371f..880f082ec4 100644 --- a/apps/dav/lib/Connector/Sabre/Principal.php +++ b/apps/dav/lib/Connector/Sabre/Principal.php @@ -40,6 +40,7 @@ use OCA\DAV\CalDAV\Proxy\ProxyMapper; use OCA\DAV\Traits\PrincipalProxyTrait; use OCP\App\IAppManager; use OCP\AppFramework\QueryException; +use OCP\IConfig; use OCP\IGroup; use OCP\IGroupManager; use OCP\IUser; @@ -79,6 +80,9 @@ class Principal implements BackendInterface { /** @var ProxyMapper */ private $proxyMapper; + /** @var IConfig */ + private $config; + /** * Principal constructor. * @@ -88,6 +92,7 @@ class Principal implements BackendInterface { * @param IUserSession $userSession * @param IAppManager $appManager * @param ProxyMapper $proxyMapper + * @param IConfig $config * @param string $principalPrefix */ public function __construct(IUserManager $userManager, @@ -96,6 +101,7 @@ class Principal implements BackendInterface { IUserSession $userSession, IAppManager $appManager, ProxyMapper $proxyMapper, + IConfig $config, string $principalPrefix = 'principals/users/') { $this->userManager = $userManager; $this->groupManager = $groupManager; @@ -105,6 +111,7 @@ class Principal implements BackendInterface { $this->principalPrefix = trim($principalPrefix, '/'); $this->hasGroups = $this->hasCircles = ($principalPrefix === 'principals/users/'); $this->proxyMapper = $proxyMapper; + $this->config = $config; } use PrincipalProxyTrait { @@ -240,6 +247,8 @@ class Principal implements BackendInterface { return []; } + $allowEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes'; + // If sharing is restricted to group members only, // return only members that have groups in common $restrictGroups = false; @@ -257,6 +266,12 @@ class Principal implements BackendInterface { case '{http://sabredav.org/ns}email-address': $users = $this->userManager->getByEmail($value); + if (!$allowEnumeration) { + $users = \array_filter($users, static function(IUser $user) use ($value) { + return $user->getEMailAddress() === $value; + }); + } + $results[] = array_reduce($users, function(array $carry, IUser $user) use ($restrictGroups) { // is sharing restricted to groups only? if ($restrictGroups !== false) { @@ -274,6 +289,12 @@ class Principal implements BackendInterface { case '{DAV:}displayname': $users = $this->userManager->searchDisplayName($value); + if (!$allowEnumeration) { + $users = \array_filter($users, static function(IUser $user) use ($value) { + return $user->getDisplayName() === $value; + }); + } + $results[] = array_reduce($users, function(array $carry, IUser $user) use ($restrictGroups) { // is sharing restricted to groups only? if ($restrictGroups !== false) { diff --git a/apps/dav/lib/RootCollection.php b/apps/dav/lib/RootCollection.php index 8c66609ed6..6458f2e1dc 100644 --- a/apps/dav/lib/RootCollection.php +++ b/apps/dav/lib/RootCollection.php @@ -63,9 +63,10 @@ class RootCollection extends SimpleCollection { $shareManager, \OC::$server->getUserSession(), \OC::$server->getAppManager(), - $proxyMapper + $proxyMapper, + \OC::$server->getConfig() ); - $groupPrincipalBackend = new GroupPrincipalBackend($groupManager, $userSession, $shareManager, $l10n); + $groupPrincipalBackend = new GroupPrincipalBackend($groupManager, $userSession, $shareManager); $calendarResourcePrincipalBackend = new ResourcePrincipalBackend($db, $userSession, $groupManager, $logger, $proxyMapper); $calendarRoomPrincipalBackend = new RoomPrincipalBackend($db, $userSession, $groupManager, $logger, $proxyMapper); // as soon as debug mode is enabled we allow listing of principals diff --git a/apps/dav/tests/unit/CalDAV/AbstractCalDavBackend.php b/apps/dav/tests/unit/CalDAV/AbstractCalDavBackend.php index 8772f51148..9449ab27b0 100644 --- a/apps/dav/tests/unit/CalDAV/AbstractCalDavBackend.php +++ b/apps/dav/tests/unit/CalDAV/AbstractCalDavBackend.php @@ -86,6 +86,7 @@ abstract class AbstractCalDavBackend extends TestCase { $this->createMock(IUserSession::class), $this->createMock(IAppManager::class), $this->createMock(ProxyMapper::class), + $this->createMock(IConfig::class), ]) ->setMethods(['getPrincipalByPath', 'getGroupMembership']) ->getMock(); diff --git a/apps/dav/tests/unit/CardDAV/CardDavBackendTest.php b/apps/dav/tests/unit/CardDAV/CardDavBackendTest.php index f2a85cb9db..d00d03d894 100644 --- a/apps/dav/tests/unit/CardDAV/CardDavBackendTest.php +++ b/apps/dav/tests/unit/CardDAV/CardDavBackendTest.php @@ -134,6 +134,7 @@ class CardDavBackendTest extends TestCase { $this->createMock(IUserSession::class), $this->createMock(IAppManager::class), $this->createMock(ProxyMapper::class), + $this->createMock(IConfig::class), ]) ->setMethods(['getPrincipalByPath', 'getGroupMembership']) ->getMock(); @@ -396,7 +397,7 @@ class CardDavBackendTest extends TestCase { // create a card $uri0 = $this->getUniqueID('card'); $this->backend->createCard($bookId, $uri0, $this->vcardTest0); - + // create another card with same uid $uri1 = $this->getUniqueID('card'); $this->expectException(BadRequest::class); diff --git a/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php b/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php index 7836191450..cb268a943e 100644 --- a/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php @@ -65,6 +65,9 @@ class PrincipalTest extends TestCase { /** @var ProxyMapper | \PHPUnit_Framework_MockObject_MockObject */ private $proxyMapper; + /** @var IConfig | \PHPUnit_Framework_MockObject_MockObject */ + private $config; + protected function setUp(): void { $this->userManager = $this->createMock(IUserManager::class); $this->groupManager = $this->createMock(IGroupManager::class); @@ -72,6 +75,7 @@ class PrincipalTest extends TestCase { $this->userSession = $this->createMock(IUserSession::class); $this->appManager = $this->createMock(IAppManager::class); $this->proxyMapper = $this->createMock(ProxyMapper::class); + $this->config = $this->createMock(IConfig::class); $this->connector = new \OCA\DAV\Connector\Sabre\Principal( $this->userManager, @@ -79,7 +83,8 @@ class PrincipalTest extends TestCase { $this->shareManager, $this->userSession, $this->appManager, - $this->proxyMapper + $this->proxyMapper, + $this->config ); parent::setUp(); } @@ -209,7 +214,7 @@ class PrincipalTest extends TestCase { $this->assertSame([], $response); } - + public function testGetGroupMemberSetEmpty() { $this->expectException(\Sabre\DAV\Exception::class); $this->expectExceptionMessage('Principal not found'); @@ -334,7 +339,7 @@ class PrincipalTest extends TestCase { $this->assertSame($expectedResponse, $response); } - + public function testGetGroupMembershipEmpty() { $this->expectException(\Sabre\DAV\Exception::class); $this->expectExceptionMessage('Principal not found'); @@ -348,7 +353,7 @@ class PrincipalTest extends TestCase { $this->connector->getGroupMembership('principals/users/foo'); } - + public function testSetGroupMembership() { $this->expectException(\Sabre\DAV\Exception::class); $this->expectExceptionMessage('Setting members of the group is not supported yet'); @@ -403,11 +408,6 @@ class PrincipalTest extends TestCase { $this->connector->setGroupMemberSet('principals/users/foo/calendar-proxy-write', ['principals/users/bar']); } - - - - - public function testUpdatePrincipal() { $this->assertSame(0, $this->connector->updatePrincipal('foo', new PropPatch(array()))); } @@ -430,6 +430,11 @@ class PrincipalTest extends TestCase { ->will($this->returnValue($sharingEnabled)); if ($sharingEnabled) { + $this->config->expects($this->once()) + ->method('getAppValue') + ->with('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') + ->willReturn('yes'); + $this->shareManager->expects($this->once()) ->method('shareWithGroupMembersOnly') ->will($this->returnValue($groupsOnly)); @@ -446,6 +451,8 @@ class PrincipalTest extends TestCase { ->will($this->returnValue(['group1', 'group2', 'group5'])); } } else { + $this->config->expects($this->never()) + ->method('getAppValue'); $this->shareManager->expects($this->never()) ->method('shareWithGroupMembersOnly'); $this->groupManager->expects($this->never()) @@ -518,6 +525,11 @@ class PrincipalTest extends TestCase { ->method('shareAPIEnabled') ->will($this->returnValue(true)); + $this->config->expects($this->exactly(2)) + ->method('getAppValue') + ->with('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') + ->willReturn('yes'); + $this->shareManager->expects($this->exactly(2)) ->method('shareWithGroupMembersOnly') ->will($this->returnValue(false)); @@ -539,6 +551,78 @@ class PrincipalTest extends TestCase { ['{urn:ietf:params:xml:ns:caldav}calendar-user-address-set' => 'user@example.com'])); } + public function testSearchPrincipalWithEnumerationDisabledDisplayname() { + $this->shareManager->expects($this->once()) + ->method('shareAPIEnabled') + ->will($this->returnValue(true)); + + $this->config->expects($this->once()) + ->method('getAppValue') + ->with('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') + ->willReturn('no'); + + $this->shareManager->expects($this->once()) + ->method('shareWithGroupMembersOnly') + ->will($this->returnValue(false)); + + $user2 = $this->createMock(IUser::class); + $user2->method('getUID')->will($this->returnValue('user2')); + $user2->method('getDisplayName')->will($this->returnValue('User 2')); + $user2->method('getEMailAddress')->will($this->returnValue('user2@foo.bar')); + $user3 = $this->createMock(IUser::class); + $user3->method('getUID')->will($this->returnValue('user3')); + $user2->method('getDisplayName')->will($this->returnValue('User 22')); + $user2->method('getEMailAddress')->will($this->returnValue('user2@foo.bar123')); + $user4 = $this->createMock(IUser::class); + $user4->method('getUID')->will($this->returnValue('user4')); + $user2->method('getDisplayName')->will($this->returnValue('User 222')); + $user2->method('getEMailAddress')->will($this->returnValue('user2@foo.bar456')); + + $this->userManager->expects($this->at(0)) + ->method('searchDisplayName') + ->with('User 2') + ->will($this->returnValue([$user2, $user3, $user4])); + + $this->assertEquals(['principals/users/user2'], $this->connector->searchPrincipals('principals/users', + ['{DAV:}displayname' => 'User 2'])); + } + + public function testSearchPrincipalWithEnumerationDisabledEmail() { + $this->shareManager->expects($this->once()) + ->method('shareAPIEnabled') + ->will($this->returnValue(true)); + + $this->config->expects($this->once()) + ->method('getAppValue') + ->with('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') + ->willReturn('no'); + + $this->shareManager->expects($this->once()) + ->method('shareWithGroupMembersOnly') + ->will($this->returnValue(false)); + + $user2 = $this->createMock(IUser::class); + $user2->method('getUID')->will($this->returnValue('user2')); + $user2->method('getDisplayName')->will($this->returnValue('User 2')); + $user2->method('getEMailAddress')->will($this->returnValue('user2@foo.bar')); + $user3 = $this->createMock(IUser::class); + $user3->method('getUID')->will($this->returnValue('user3')); + $user2->method('getDisplayName')->will($this->returnValue('User 22')); + $user2->method('getEMailAddress')->will($this->returnValue('user2@foo.bar123')); + $user4 = $this->createMock(IUser::class); + $user4->method('getUID')->will($this->returnValue('user4')); + $user2->method('getDisplayName')->will($this->returnValue('User 222')); + $user2->method('getEMailAddress')->will($this->returnValue('user2@foo.bar456')); + + $this->userManager->expects($this->at(0)) + ->method('getByEmail') + ->with('user2@foo.bar') + ->will($this->returnValue([$user2, $user3, $user4])); + + $this->assertEquals(['principals/users/user2'], $this->connector->searchPrincipals('principals/users', + ['{http://sabredav.org/ns}email-address' => 'user2@foo.bar'])); + } + public function testFindByUriSharingApiDisabled() { $this->shareManager->expects($this->once()) ->method('shareApiEnabled') diff --git a/apps/files_trashbin/lib/AppInfo/Application.php b/apps/files_trashbin/lib/AppInfo/Application.php index 08fdae18d9..dde400da5c 100644 --- a/apps/files_trashbin/lib/AppInfo/Application.php +++ b/apps/files_trashbin/lib/AppInfo/Application.php @@ -58,7 +58,8 @@ class Application extends App { \OC::$server->getShareManager(), \OC::$server->getUserSession(), \OC::$server->getAppManager(), - \OC::$server->query(ProxyMapper::class) + \OC::$server->query(ProxyMapper::class), + \OC::$server->getConfig() ); }); diff --git a/apps/files_versions/lib/AppInfo/Application.php b/apps/files_versions/lib/AppInfo/Application.php index 25dc7fe597..a7e88faedf 100644 --- a/apps/files_versions/lib/AppInfo/Application.php +++ b/apps/files_versions/lib/AppInfo/Application.php @@ -66,7 +66,8 @@ class Application extends App { $server->getShareManager(), $server->getUserSession(), $server->getAppManager(), - $server->query(ProxyMapper::class) + $server->query(ProxyMapper::class), + \OC::$server->getConfig() ); });