Make dav respect disallowing sharing with groups

Closes #25390

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

3 files changed, 87 insertions, 53 deletions

diff --git a/apps/dav/lib/DAV/GroupPrincipalBackend.php b/apps/dav/lib/DAV/GroupPrincipalBackend.php
index ae66d85f4ca..1b96f2ee8ba 100644
--- a/apps/dav/lib/DAV/GroupPrincipalBackend.php
+++ b/apps/dav/lib/DAV/GroupPrincipalBackend.php
@@ -196,9 +196,8 @@ class GroupPrincipalBackend implements BackendInterface {
 		if ($prefixPath !== self::PRINCIPAL_PREFIX) {
 			return [];
 		}
-		// If sharing is disabled, return the empty array
-		$shareAPIEnabled = $this->shareManager->shareApiEnabled();
-		if (!$shareAPIEnabled) {
+		// If sharing or group sharing is disabled, return the empty array
+		if (!$this->groupSharingEnabled()) {
 			return [];
 		}
 
@@ -273,8 +272,7 @@ class GroupPrincipalBackend implements BackendInterface {
 	 */
 	public function findByUri($uri, $principalPrefix) {
 		// If sharing is disabled, return the empty array
-		$shareAPIEnabled = $this->shareManager->shareApiEnabled();
-		if (!$shareAPIEnabled) {
+		if (!$this->groupSharingEnabled()) {
 			return null;
 		}
 
@@ -340,4 +338,11 @@ class GroupPrincipalBackend implements BackendInterface {
 
 		return $principal;
 	}
+
+	/**
+	 * @return bool
+	 */
+	private function groupSharingEnabled(): bool {
+		return $this->shareManager->shareApiEnabled() && $this->shareManager->allowGroupSharing();
+	}
 }
diff --git a/apps/dav/tests/unit/DAV/GroupPrincipalTest.php b/apps/dav/tests/unit/DAV/GroupPrincipalTest.php
index ba079fb9cad..7e80930bc76 100644
--- a/apps/dav/tests/unit/DAV/GroupPrincipalTest.php
+++ b/apps/dav/tests/unit/DAV/GroupPrincipalTest.php
@@ -38,19 +38,20 @@ use OCP\IGroupManager;
 use OCP\IUser;
 use OCP\IUserSession;
 use OCP\Share\IManager;
+use PHPUnit\Framework\MockObject\MockObject;
 use Sabre\DAV\PropPatch;
 
 class GroupPrincipalTest extends \Test\TestCase {
-	/** @var IConfig|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var IConfig|MockObject */
 	private $config;
 
-	/** @var IGroupManager | \PHPUnit\Framework\MockObject\MockObject */
+	/** @var IGroupManager | MockObject */
 	private $groupManager;
 
-	/** @var IUserSession | \PHPUnit\Framework\MockObject\MockObject */
+	/** @var IUserSession | MockObject */
 	private $userSession;
 
-	/** @var IManager | \PHPUnit\Framework\MockObject\MockObject */
+	/** @var IManager | MockObject */
 	private $shareManager;
 
 	/** @var GroupPrincipalBackend */
@@ -224,13 +225,22 @@ class GroupPrincipalTest extends \Test\TestCase {
 
 	/**
 	 * @dataProvider searchPrincipalsDataProvider
+	 * @param bool $sharingEnabled
+	 * @param bool $groupSharingEnabled
+	 * @param bool $groupsOnly
+	 * @param string $test
+	 * @param array $result
 	 */
-	public function testSearchPrincipals($sharingEnabled, $groupsOnly, $test, $result) {
+	public function testSearchPrincipals(bool $sharingEnabled, bool $groupSharingEnabled, bool $groupsOnly, string $test, array $result): void {
 		$this->shareManager->expects($this->once())
 			->method('shareAPIEnabled')
 			->willReturn($sharingEnabled);
 
-		if ($sharingEnabled) {
+		$this->shareManager->expects($sharingEnabled ? $this->once() : $this->never())
+			->method('allowGroupSharing')
+			->willReturn($groupSharingEnabled);
+
+		if ($sharingEnabled && $groupSharingEnabled) {
 			$this->shareManager->expects($this->once())
 				->method('shareWithGroupMembersOnly')
 				->willReturn($groupsOnly);
@@ -264,7 +274,7 @@ class GroupPrincipalTest extends \Test\TestCase {
 		$group5 = $this->createMock(IGroup::class);
 		$group5->method('getGID')->willReturn('group5');
 
-		if ($sharingEnabled) {
+		if ($sharingEnabled && $groupSharingEnabled) {
 			$this->groupManager->expects($this->once())
 				->method('search')
 				->with('Foo')
@@ -280,24 +290,35 @@ class GroupPrincipalTest extends \Test\TestCase {
 
 	public function searchPrincipalsDataProvider() {
 		return [
-			[true, false, 'allof', ['principals/groups/group1', 'principals/groups/group2', 'principals/groups/group3', 'principals/groups/group4', 'principals/groups/group5']],
-			[true, false, 'anyof', ['principals/groups/group1', 'principals/groups/group2', 'principals/groups/group3', 'principals/groups/group4', 'principals/groups/group5']],
-			[true, true, 'allof', ['principals/groups/group1', 'principals/groups/group2', 'principals/groups/group5']],
-			[true, true, 'anyof', ['principals/groups/group1', 'principals/groups/group2', 'principals/groups/group5']],
-			[false, false, 'allof', []],
-			[false, false, 'anyof', []],
+			[true, true, false, 'allof', ['principals/groups/group1', 'principals/groups/group2', 'principals/groups/group3', 'principals/groups/group4', 'principals/groups/group5']],
+			[true, true, false, 'anyof', ['principals/groups/group1', 'principals/groups/group2', 'principals/groups/group3', 'principals/groups/group4', 'principals/groups/group5']],
+			[true, true, true, 'allof', ['principals/groups/group1', 'principals/groups/group2', 'principals/groups/group5']],
+			[true, true, true, 'anyof', ['principals/groups/group1', 'principals/groups/group2', 'principals/groups/group5']],
+			[true, false, false, 'allof', []],
+			[false, true, false, 'anyof', []],
+			[false, false, false, 'allof', []],
+			[false, false, false, 'anyof', []],
 		];
 	}
 
 	/**
 	 * @dataProvider findByUriDataProvider
+	 * @param bool $sharingEnabled
+	 * @param bool $groupSharingEnabled
+	 * @param bool $groupsOnly
+	 * @param string $findUri
+	 * @param string|null $result
 	 */
-	public function testFindByUri($sharingEnabled, $groupsOnly, $findUri, $result) {
+	public function testFindByUri(bool $sharingEnabled, bool $groupSharingEnabled, bool $groupsOnly, string $findUri, ?string $result): void {
 		$this->shareManager->expects($this->once())
 			->method('shareAPIEnabled')
 			->willReturn($sharingEnabled);
 
-		if ($sharingEnabled) {
+		$this->shareManager->expects($sharingEnabled ? $this->once() : $this->never())
+			->method('allowGroupSharing')
+			->willReturn($groupSharingEnabled);
+
+		if ($sharingEnabled && $groupSharingEnabled) {
 			$this->shareManager->expects($this->once())
 				->method('shareWithGroupMembersOnly')
 				->willReturn($groupsOnly);
@@ -325,19 +346,23 @@ class GroupPrincipalTest extends \Test\TestCase {
 
 	public function findByUriDataProvider() {
 		return [
-			[false, false, 'principal:principals/groups/group1', null],
-			[false, false, 'principal:principals/groups/group3', null],
-			[false, true, 'principal:principals/groups/group1', null],
-			[false, true, 'principal:principals/groups/group3', null],
-			[true, true, 'principal:principals/groups/group1', 'principals/groups/group1'],
-			[true, true, 'principal:principals/groups/group3', null],
-			[true, false, 'principal:principals/groups/group1', 'principals/groups/group1'],
-			[true, false, 'principal:principals/groups/group3', 'principals/groups/group3'],
+			[false, false, false, 'principal:principals/groups/group1', null],
+			[false, false, false, 'principal:principals/groups/group3', null],
+			[false, true, false, 'principal:principals/groups/group1', null],
+			[false, true, false, 'principal:principals/groups/group3', null],
+			[false, false, true, 'principal:principals/groups/group1', null],
+			[false, false, true, 'principal:principals/groups/group3', null],
+			[true, false, true, 'principal:principals/groups/group1', null],
+			[true, false, true, 'principal:principals/groups/group3', null],
+			[true, true, true, 'principal:principals/groups/group1', 'principals/groups/group1'],
+			[true, true, true, 'principal:principals/groups/group3', null],
+			[true, true, false, 'principal:principals/groups/group1', 'principals/groups/group1'],
+			[true, true, false, 'principal:principals/groups/group3', 'principals/groups/group3'],
 		];
 	}
 
 	/**
-	 * @return Group|\PHPUnit\Framework\MockObject\MockObject
+	 * @return Group|MockObject
 	 */
 	private function mockGroup($gid) {
 		$fooGroup = $this->createMock(Group::class);
diff --git a/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php
index 85267726068..f057bc1b892 100644
--- a/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php
+++ b/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php
@@ -41,6 +41,7 @@ use OCP\IRequest;
 use OCP\IURLGenerator;
 use OCP\Share\IShare;
 use OCP\Share\IManager;
+use PHPUnit\Framework\MockObject\MockObject;
 
 /**
  * Class ShareesTest
@@ -56,13 +57,13 @@ class ShareesAPIControllerTest extends TestCase {
 	/** @var string */
 	protected $uid;
 
-	/** @var IRequest|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var IRequest|MockObject */
 	protected $request;
 
-	/** @var IManager|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var IManager|MockObject */
 	protected $shareManager;
 
-	/** @var  ISearch|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var  ISearch|MockObject */
 	protected $collaboratorSearch;
 
 	protected function setUp(): void {
@@ -72,10 +73,10 @@ class ShareesAPIControllerTest extends TestCase {
 		$this->request = $this->createMock(IRequest::class);
 		$this->shareManager = $this->createMock(IManager::class);
 
-		/** @var IConfig|\PHPUnit\Framework\MockObject\MockObject $configMock */
+		/** @var IConfig|MockObject $configMock */
 		$configMock = $this->createMock(IConfig::class);
 
-		/** @var IURLGenerator|\PHPUnit\Framework\MockObject\MockObject $urlGeneratorMock */
+		/** @var IURLGenerator|MockObject $urlGeneratorMock */
 		$urlGeneratorMock = $this->createMock(IURLGenerator::class);
 
 		$this->collaboratorSearch = $this->createMock(ISearch::class);
@@ -91,7 +92,7 @@ class ShareesAPIControllerTest extends TestCase {
 		);
 	}
 
-	public function dataSearch() {
+	public function dataSearch(): array {
 		$noRemote = [IShare::TYPE_USER, IShare::TYPE_GROUP, IShare::TYPE_EMAIL];
 		$allTypes = [IShare::TYPE_USER, IShare::TYPE_GROUP, IShare::TYPE_REMOTE, IShare::TYPE_REMOTE_GROUP, IShare::TYPE_EMAIL];
 
@@ -124,6 +125,9 @@ class ShareesAPIControllerTest extends TestCase {
 				'itemType' => 'call',
 			], '', 'yes', true, true, true, $noRemote, false, true, true],
 			[[
+				'itemType' => 'call',
+			], '', 'yes', true, true, true, [0, 4], false, true, false],
+			[[
 				'itemType' => 'folder',
 			], '', 'yes', true, true, true, $allTypes, false, true, true],
 			[[
@@ -230,14 +234,14 @@ class ShareesAPIControllerTest extends TestCase {
 	 * @param bool $allowGroupSharing
 	 * @throws OCSBadRequestException
 	 */
-	public function testSearch($getData, $apiSetting, $enumSetting, $remoteSharingEnabled, $isRemoteGroupSharingEnabled, $emailSharingEnabled, $shareTypes, $shareWithGroupOnly, $shareeEnumeration, $allowGroupSharing) {
-		$search = isset($getData['search']) ? $getData['search'] : '';
-		$itemType = isset($getData['itemType']) ? $getData['itemType'] : 'irrelevant';
-		$page = isset($getData['page']) ? $getData['page'] : 1;
-		$perPage = isset($getData['perPage']) ? $getData['perPage'] : 200;
-		$shareType = isset($getData['shareType']) ? $getData['shareType'] : null;
-
-		/** @var IConfig|\PHPUnit\Framework\MockObject\MockObject $config */
+	public function testSearch(array $getData, string $apiSetting, string $enumSetting, bool $remoteSharingEnabled, bool $isRemoteGroupSharingEnabled, bool $emailSharingEnabled, array $shareTypes, bool $shareWithGroupOnly, bool $shareeEnumeration, bool $allowGroupSharing) {
+		$search = $getData['search'] ?? '';
+		$itemType = $getData['itemType'] ?? 'irrelevant';
+		$page = $getData['page'] ?? 1;
+		$perPage = $getData['perPage'] ?? 200;
+		$shareType = $getData['shareType'] ?? null;
+
+		/** @var IConfig|MockObject $config */
 		$config = $this->createMock(IConfig::class);
 		$config->expects($this->exactly(1))
 			->method('getAppValue')
@@ -246,19 +250,19 @@ class ShareesAPIControllerTest extends TestCase {
 				['files_sharing', 'lookupServerEnabled', 'yes', 'yes'],
 			]);
 
-		$this->shareManager->expects($itemType === 'file' || $itemType === 'folder' ? $this->once() : $this->never())
+		$this->shareManager->expects($this->once())
 			->method('allowGroupSharing')
 			->willReturn($allowGroupSharing);
 
 		/** @var string */
 		$uid = 'test123';
-		/** @var IRequest|\PHPUnit\Framework\MockObject\MockObject $request */
+		/** @var IRequest|MockObject $request */
 		$request = $this->createMock(IRequest::class);
-		/** @var IURLGenerator|\PHPUnit\Framework\MockObject\MockObject $urlGenerator */
+		/** @var IURLGenerator|MockObject $urlGenerator */
 		$urlGenerator = $this->createMock(IURLGenerator::class);
 
-		/** @var \PHPUnit\Framework\MockObject\MockObject|\OCA\Files_Sharing\Controller\ShareesAPIController $sharees */
-		$sharees = $this->getMockBuilder('\OCA\Files_Sharing\Controller\ShareesAPIController')
+		/** @var MockObject|ShareesAPIController $sharees */
+		$sharees = $this->getMockBuilder(ShareesAPIController::class)
 			->setConstructorArgs([
 				$uid,
 				'files_sharing',
@@ -304,7 +308,7 @@ class ShareesAPIControllerTest extends TestCase {
 		$this->assertInstanceOf(Http\DataResponse::class, $sharees->search($search, $itemType, $page, $perPage, $shareType));
 	}
 
-	public function dataSearchInvalid() {
+	public function dataSearchInvalid(): array {
 		return [
 			// Test invalid pagination
 			[[
@@ -340,19 +344,19 @@ class ShareesAPIControllerTest extends TestCase {
 		$page = isset($getData['page']) ? $getData['page'] : 1;
 		$perPage = isset($getData['perPage']) ? $getData['perPage'] : 200;
 
-		/** @var IConfig|\PHPUnit\Framework\MockObject\MockObject $config */
+		/** @var IConfig|MockObject $config */
 		$config = $this->createMock(IConfig::class);
 		$config->expects($this->never())
 			->method('getAppValue');
 
 		/** @var string */
 		$uid = 'test123';
-		/** @var IRequest|\PHPUnit\Framework\MockObject\MockObject $request */
+		/** @var IRequest|MockObject $request */
 		$request = $this->createMock(IRequest::class);
-		/** @var IURLGenerator|\PHPUnit\Framework\MockObject\MockObject $urlGenerator */
+		/** @var IURLGenerator|MockObject $urlGenerator */
 		$urlGenerator = $this->createMock(IURLGenerator::class);
 
-		/** @var \PHPUnit\Framework\MockObject\MockObject|\OCA\Files_Sharing\Controller\ShareesAPIController $sharees */
+		/** @var MockObject|ShareesAPIController $sharees */
 		$sharees = $this->getMockBuilder('\OCA\Files_Sharing\Controller\ShareesAPIController')
 			->setConstructorArgs([
 				$uid,