Skip to content

Commit

Permalink
respect shareapi_allow_share_dialog_user_enumeration in Principal bac…
Browse files Browse the repository at this point in the history
…kend for Sabre/DAV

Signed-off-by: Georg Ehrke <[email protected]>
  • Loading branch information
georgehrke committed Nov 27, 2019
1 parent b607e3e commit 9964030
Show file tree
Hide file tree
Showing 10 changed files with 125 additions and 12 deletions.
1 change: 1 addition & 0 deletions apps/dav/appinfo/v1/caldav.php
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
\OC::$server->getUserSession(),
\OC::$server->getAppManager(),
\OC::$server->query(\OCA\DAV\CalDAV\Proxy\ProxyMapper::class),
\OC::$server->getConfig(),
'principals/'
);
$db = \OC::$server->getDatabaseConnection();
Expand Down
1 change: 1 addition & 0 deletions apps/dav/appinfo/v1/carddav.php
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
\OC::$server->getUserSession(),
\OC::$server->getAppManager(),
\OC::$server->query(\OCA\DAV\CalDAV\Proxy\ProxyMapper::class),
\OC::$server->getConfig(),
'principals/'
);
$db = \OC::$server->getDatabaseConnection();
Expand Down
3 changes: 2 additions & 1 deletion apps/dav/lib/Command/CreateCalendar.php
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@ protected function execute(InputInterface $input, OutputInterface $output) {
\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();
Expand Down
21 changes: 21 additions & 0 deletions apps/dav/lib/Connector/Sabre/Principal.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
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;
Expand Down Expand Up @@ -79,6 +80,9 @@ class Principal implements BackendInterface {
/** @var ProxyMapper */
private $proxyMapper;

/** @var IConfig */
private $config;

/**
* Principal constructor.
*
Expand All @@ -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,
Expand All @@ -96,6 +101,7 @@ public function __construct(IUserManager $userManager,
IUserSession $userSession,
IAppManager $appManager,
ProxyMapper $proxyMapper,
IConfig $config,
string $principalPrefix = 'principals/users/') {
$this->userManager = $userManager;
$this->groupManager = $groupManager;
Expand All @@ -105,6 +111,7 @@ public function __construct(IUserManager $userManager,
$this->principalPrefix = trim($principalPrefix, '/');
$this->hasGroups = $this->hasCircles = ($principalPrefix === 'principals/users/');
$this->proxyMapper = $proxyMapper;
$this->config = $config;
}

use PrincipalProxyTrait {
Expand Down Expand Up @@ -240,6 +247,8 @@ protected function searchUserPrincipals(array $searchProperties, $test = 'allof'
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;
Expand All @@ -257,6 +266,12 @@ protected function searchUserPrincipals(array $searchProperties, $test = 'allof'
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) {
Expand All @@ -274,6 +289,12 @@ protected function searchUserPrincipals(array $searchProperties, $test = 'allof'
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) {
Expand Down
5 changes: 3 additions & 2 deletions apps/dav/lib/RootCollection.php
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,10 @@ public function __construct() {
$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
Expand Down
1 change: 1 addition & 0 deletions apps/dav/tests/unit/CalDAV/AbstractCalDavBackend.php
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ public function setUp() {
$this->createMock(IUserSession::class),
$this->createMock(IAppManager::class),
$this->createMock(ProxyMapper::class),
$this->createMock(IConfig::class),
])
->setMethods(['getPrincipalByPath', 'getGroupMembership'])
->getMock();
Expand Down
3 changes: 2 additions & 1 deletion apps/dav/tests/unit/CardDAV/CardDavBackendTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ public function setUp() {
$this->createMock(IUserSession::class),
$this->createMock(IAppManager::class),
$this->createMock(ProxyMapper::class),
$this->createMock(IConfig::class),
])
->setMethods(['getPrincipalByPath', 'getGroupMembership'])
->getMock();
Expand Down Expand Up @@ -396,7 +397,7 @@ public function testMultipleUIDDenied() {
// 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);
Expand Down
96 changes: 90 additions & 6 deletions apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -65,21 +65,26 @@ class PrincipalTest extends TestCase {
/** @var ProxyMapper | \PHPUnit_Framework_MockObject_MockObject */
private $proxyMapper;

/** @var IConfig | \PHPUnit_Framework_MockObject_MockObject */
private $config;

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->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,
$this->groupManager,
$this->shareManager,
$this->userSession,
$this->appManager,
$this->proxyMapper
$this->proxyMapper,
$this->config
);
parent::setUp();
}
Expand Down Expand Up @@ -403,11 +408,6 @@ public function testSetGroupMembershipProxy() {
$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())));
}
Expand All @@ -430,6 +430,11 @@ public function testSearchPrincipals($sharingEnabled, $groupsOnly, $test, $resul
->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));
Expand All @@ -446,6 +451,8 @@ public function testSearchPrincipals($sharingEnabled, $groupsOnly, $test, $resul
->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())
Expand Down Expand Up @@ -518,6 +525,11 @@ public function testSearchPrincipalByCalendarUserAddressSet() {
->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));
Expand All @@ -539,6 +551,78 @@ public function testSearchPrincipalByCalendarUserAddressSet() {
['{urn:ietf:params:xml:ns:caldav}calendar-user-address-set' => '[email protected]']));
}

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('[email protected]'));
$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('[email protected]'));
$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('[email protected]'));

$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('[email protected]'));
$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('[email protected]'));
$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('[email protected]'));

$this->userManager->expects($this->at(0))
->method('getByEmail')
->with('[email protected]')
->will($this->returnValue([$user2, $user3, $user4]));

$this->assertEquals(['principals/users/user2'], $this->connector->searchPrincipals('principals/users',
['{http://sabredav.org/ns}email-address' => '[email protected]']));
}

public function testFindByUriSharingApiDisabled() {
$this->shareManager->expects($this->once())
->method('shareApiEnabled')
Expand Down
3 changes: 2 additions & 1 deletion apps/files_trashbin/lib/AppInfo/Application.php
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ public function __construct (array $urlParams = []) {
\OC::$server->getShareManager(),
\OC::$server->getUserSession(),
\OC::$server->getAppManager(),
\OC::$server->query(ProxyMapper::class)
\OC::$server->query(ProxyMapper::class),
\OC::$server->getConfig()
);
});

Expand Down
3 changes: 2 additions & 1 deletion apps/files_versions/lib/AppInfo/Application.php
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ public function __construct(array $urlParams = []) {
$server->getShareManager(),
$server->getUserSession(),
$server->getAppManager(),
$server->query(ProxyMapper::class)
$server->query(ProxyMapper::class),
\OC::$server->getConfig()
);
});

Expand Down

0 comments on commit 9964030

Please sign in to comment.