Add support for room shares to ShareAPIController

In some cases, the ShareAPIController requires explicit handling of each
type of share (for example, to format a share for a DataResponse). Room
shares are implemented in an external app (Nextcloud Talk), so in order
to keep the controller as isolated as possible from room share specifics
all that explicit handling is done in a helper class provided by the
Talk app.

In other cases it is just enough to call the share manager specifying a
room share type; note that the share manager is guarded against share
types for which there is no provider, so it is not necessary to
explicitly check that before passing room shares to the share manager.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

3 files changed, 547 insertions, 15 deletions

diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php
index bda0047e66c..15dda8928d4 100644
--- a/apps/files_sharing/lib/Controller/ShareAPIController.php
+++ b/apps/files_sharing/lib/Controller/ShareAPIController.php
@@ -37,6 +37,7 @@ use OCP\AppFramework\OCS\OCSException;
 use OCP\AppFramework\OCS\OCSForbiddenException;
 use OCP\AppFramework\OCS\OCSNotFoundException;
 use OCP\AppFramework\OCSController;
+use OCP\AppFramework\QueryException;
 use OCP\Constants;
 use OCP\Files\Folder;
 use OCP\Files\Node;
@@ -46,6 +47,7 @@ use OCP\IGroupManager;
 use OCP\IL10N;
 use OCP\IUserManager;
 use OCP\IRequest;
+use OCP\IServerContainer;
 use OCP\IURLGenerator;
 use OCP\Files\IRootFolder;
 use OCP\Lock\LockedException;
@@ -84,6 +86,8 @@ class ShareAPIController extends OCSController {
 	private $config;
 	/** @var IAppManager */
 	private $appManager;
+	/** @var IServerContainer */
+	private $serverContainer;
 
 	/**
 	 * Share20OCS constructor.
@@ -99,6 +103,7 @@ class ShareAPIController extends OCSController {
 	 * @param IL10N $l10n
 	 * @param IConfig $config
 	 * @param IAppManager $appManager
+	 * @param IServerContainer $serverContainer
 	 */
 	public function __construct(
 		string $appName,
@@ -111,7 +116,8 @@ class ShareAPIController extends OCSController {
 		string $userId,
 		IL10N $l10n,
 		IConfig $config,
-		IAppManager $appManager
+		IAppManager $appManager,
+		IServerContainer $serverContainer
 	) {
 		parent::__construct($appName, $request);
 
@@ -125,6 +131,7 @@ class ShareAPIController extends OCSController {
 		$this->l = $l10n;
 		$this->config = $config;
 		$this->appManager = $appManager;
+		$this->serverContainer = $serverContainer;
 	}
 
 	/**
@@ -231,6 +238,14 @@ class ShareAPIController extends OCSController {
 			$shareWithStart = ($hasCircleId? strrpos($share->getSharedWith(), '[') + 1: 0);
 			$shareWithLength = ($hasCircleId? -1: strpos($share->getSharedWith(), ' '));
 			$result['share_with'] = substr($share->getSharedWith(), $shareWithStart, $shareWithLength);
+		} else if ($share->getShareType() === Share::SHARE_TYPE_ROOM) {
+			$result['share_with'] = $share->getSharedWith();
+			$result['share_with_displayname'] = '';
+
+			try {
+				$result = array_merge($result, $this->getRoomShareHelper()->formatShare($share));
+			} catch (QueryException $e) {
+			}
 		}
 
 
@@ -315,7 +330,8 @@ class ShareAPIController extends OCSController {
 			throw new OCSNotFoundException($this->l->t('Could not delete share'));
 		}
 
-		if ($share->getShareType() === Share::SHARE_TYPE_GROUP &&
+		if (($share->getShareType() === Share::SHARE_TYPE_GROUP ||
+				$share->getShareType() === Share::SHARE_TYPE_ROOM) &&
 			$share->getShareOwner() !== $this->currentUser &&
 			$share->getSharedBy() !== $this->currentUser) {
 			$this->shareManager->deleteFromSelf($share, $this->currentUser);
@@ -515,6 +531,12 @@ class ShareAPIController extends OCSController {
 			}
 			$share->setSharedWith($shareWith);
 			$share->setPermissions($permissions);
+		} else if ($shareType === Share::SHARE_TYPE_ROOM) {
+			try {
+				$this->getRoomShareHelper()->createShare($share, $shareWith, $permissions, $expireDate);
+			} catch (QueryException $e) {
+				throw new OCSForbiddenException($this->l->t('Sharing %s failed because the back end does not support room shares', [$path->getPath()]));
+			}
 		} else {
 			throw new OCSBadRequestException($this->l->t('Unknown share type'));
 		}
@@ -546,8 +568,9 @@ class ShareAPIController extends OCSController {
 		$userShares = $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_USER, $node, -1, 0);
 		$groupShares = $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_GROUP, $node, -1, 0);
 		$circleShares = $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_CIRCLE, $node, -1, 0);
+		$roomShares = $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_ROOM, $node, -1, 0);
 
-		$shares = array_merge($userShares, $groupShares, $circleShares);
+		$shares = array_merge($userShares, $groupShares, $circleShares, $roomShares);
 
 		$shares = array_filter($shares, function (IShare $share) {
 			return $share->getShareOwner() !== $this->currentUser;
@@ -594,6 +617,7 @@ class ShareAPIController extends OCSController {
 			if ($this->shareManager->outgoingServer2ServerSharesAllowed()) {
 				$shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_REMOTE, $node, false, -1, 0));
 			}
+			$shares = array_merge($shares, $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_ROOM, $node, false, -1, 0));
 		}
 
 		$formatted = [];
@@ -679,8 +703,9 @@ class ShareAPIController extends OCSController {
 		} else {
 			$circleShares = [];
 		}
+		$roomShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_ROOM, $path, $reshares, -1, 0);
 
-		$shares = array_merge($userShares, $groupShares, $linkShares, $mailShares, $circleShares);
+		$shares = array_merge($userShares, $groupShares, $linkShares, $mailShares, $circleShares, $roomShares);
 
 		if ($this->shareManager->outgoingServer2ServerSharesAllowed()) {
 			$federatedShares = $this->shareManager->getSharesBy($this->currentUser, Share::SHARE_TYPE_REMOTE, $path, $reshares, -1, 0);
@@ -864,6 +889,7 @@ class ShareAPIController extends OCSController {
 			/* Check if this is an incomming share */
 			$incomingShares = $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_USER, $share->getNode(), -1, 0);
 			$incomingShares = array_merge($incomingShares, $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_GROUP, $share->getNode(), -1, 0));
+			$incomingShares = array_merge($incomingShares, $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_ROOM, $share->getNode(), -1, 0));
 
 			/** @var \OCP\Share\IShare[] $incomingShares */
 			if (!empty($incomingShares)) {
@@ -921,6 +947,14 @@ class ShareAPIController extends OCSController {
 			return true;
 		}
 
+		if ($share->getShareType() === Share::SHARE_TYPE_ROOM) {
+			try {
+				return $this->getRoomShareHelper()->canAccessShare($share, $this->currentUser);
+			} catch (QueryException $e) {
+				return false;
+			}
+		}
+
 		return false;
 	}
 
@@ -988,6 +1022,13 @@ class ShareAPIController extends OCSController {
 			// Do nothing, just try the other share type
 		}
 
+		try {
+			$share = $this->shareManager->getShareById('ocRoomShare:' . $id, $this->currentUser);
+			return $share;
+		} catch (ShareNotFound $e) {
+			// Do nothing, just try the other share type
+		}
+
 		if (!$this->shareManager->outgoingServer2ServerSharesAllowed()) {
 			throw new ShareNotFound();
 		}
@@ -1016,4 +1057,21 @@ class ShareAPIController extends OCSController {
 			$this->lockedNode->unlock(ILockingProvider::LOCK_SHARED);
 		}
 	}
+
+	/**
+	 * Returns the helper of ShareAPIController for room shares.
+	 *
+	 * If the Talk application is not enabled or the helper is not available
+	 * a QueryException is thrown instead.
+	 *
+	 * @return \OCA\Spreed\Share\Helper\ShareAPIController
+	 * @throws QueryException
+	 */
+	private function getRoomShareHelper() {
+		if (!$this->appManager->isEnabledForUser('spreed')) {
+			throw new QueryException();
+		}
+
+		return $this->serverContainer->query('\OCA\Spreed\Share\Helper\ShareAPIController');
+	}
 }
diff --git a/apps/files_sharing/tests/ApiTest.php b/apps/files_sharing/tests/ApiTest.php
index a68ec7de1f0..0616daed62d 100644
--- a/apps/files_sharing/tests/ApiTest.php
+++ b/apps/files_sharing/tests/ApiTest.php
@@ -41,6 +41,7 @@ use OCP\AppFramework\OCS\OCSNotFoundException;
 use OCP\IConfig;
 use OCP\IL10N;
 use OCP\IRequest;
+use OCP\IServerContainer;
 
 /**
  * Class ApiTest
@@ -109,6 +110,7 @@ class ApiTest extends TestCase {
 			}));
 		$config = $this->createMock(IConfig::class);
 		$appManager = $this->createMock(IAppManager::class);
+		$serverContainer = $this->createMock(IServerContainer::class);
 
 		return new ShareAPIController(
 			self::APP_NAME,
@@ -121,7 +123,8 @@ class ApiTest extends TestCase {
 			$userId,
 			$l,
 			$config,
-			$appManager
+			$appManager,
+			$serverContainer
 		);
 	}
 
diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
index 4fd8162db3d..15c4071bc46 100644
--- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
+++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
@@ -38,6 +38,7 @@ use OCA\Files_Sharing\Controller\ShareAPIController;
 use OCP\Files\NotFoundException;
 use OCP\IGroupManager;
 use OCP\IUserManager;
+use OCP\IServerContainer;
 use OCP\IRequest;
 use OCP\IURLGenerator;
 use OCP\IUser;
@@ -92,6 +93,9 @@ class ShareAPIControllerTest extends TestCase {
 	/** @var IAppManager|\PHPUnit_Framework_MockObject_MockObject */
 	private $appManager;
 
+	/** @var IServerContainer|\PHPUnit_Framework_MockObject_MockObject */
+	private $serverContainer;
+
 	protected function setUp() {
 		$this->shareManager = $this->createMock(IManager::class);
 		$this->shareManager
@@ -112,6 +116,7 @@ class ShareAPIControllerTest extends TestCase {
 			}));
 		$this->config = $this->createMock(IConfig::class);
 		$this->appManager = $this->createMock(IAppManager::class);
+		$this->serverContainer = $this->createMock(IServerContainer::class);
 
 		$this->ocs = new ShareAPIController(
 			$this->appName,
@@ -124,7 +129,8 @@ class ShareAPIControllerTest extends TestCase {
 			$this->currentUser,
 			$this->l,
 			$this->config,
-			$this->appManager
+			$this->appManager,
+			$this->serverContainer
 		);
 	}
 
@@ -144,7 +150,8 @@ class ShareAPIControllerTest extends TestCase {
 				$this->currentUser,
 				$this->l,
 				$this->config,
-				$this->appManager
+				$this->appManager,
+				$this->serverContainer
 			])->setMethods(['formatShare'])
 			->getMock();
 	}
@@ -159,10 +166,10 @@ class ShareAPIControllerTest extends TestCase {
 	 */
 	public function testDeleteShareShareNotFound() {
 		$this->shareManager
-			->expects($this->exactly(2))
+			->expects($this->exactly(3))
 			->method('getShareById')
 			->will($this->returnCallback(function($id) {
-				if ($id === 'ocinternal:42' || $id === 'ocFederatedSharing:42') {
+				if ($id === 'ocinternal:42' || $id === 'ocRoomShare:42' || $id === 'ocFederatedSharing:42') {
 					throw new \OCP\Share\Exceptions\ShareNotFound();
 				} else {
 					throw new \Exception();
@@ -461,7 +468,8 @@ class ShareAPIControllerTest extends TestCase {
 					$this->currentUser,
 					$this->l,
 					$this->config,
-					$this->appManager
+					$this->appManager,
+					$this->serverContainer
 				])->setMethods(['canAccessShare'])
 				->getMock();
 
@@ -596,6 +604,65 @@ class ShareAPIControllerTest extends TestCase {
 		$this->assertFalse($this->invokePrivate($this->ocs, 'canAccessShare', [$share]));
 	}
 
+	public function dataCanAccessRoomShare() {
+		$result = [];
+
+		$share = $this->createMock(IShare::class);
+		$share->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_ROOM);
+		$share->method('getSharedWith')->willReturn('recipientRoom');
+
+		$result[] = [
+			false, $share, false, false
+		];
+
+		$result[] = [
+			false, $share, false, true
+		];
+
+		$result[] = [
+			true, $share, true, true
+		];
+
+		$result[] = [
+			false, $share, true, false
+		];
+
+		return $result;
+	}
+
+	/**
+	 * @dataProvider dataCanAccessRoomShare
+	 *
+	 * @param bool $expects
+	 * @param \OCP\Share\IShare $share
+	 * @param bool helperAvailable
+	 * @param bool canAccessShareByHelper
+	 */
+	public function testCanAccessRoomShare(bool $expected, \OCP\Share\IShare $share, bool $helperAvailable, bool $canAccessShareByHelper) {
+		if (!$helperAvailable) {
+			$this->appManager->method('isEnabledForUser')
+				->with('spreed')
+				->willReturn(false);
+		} else {
+			$this->appManager->method('isEnabledForUser')
+				->with('spreed')
+				->willReturn(true);
+
+			$helper = $this->getMockBuilder('\OCA\Spreed\Share\Helper\ShareAPIController')
+				->setMethods(array('canAccessShare'))
+				->getMock();
+			$helper->method('canAccessShare')
+				->with($share, $this->currentUser)
+				->willReturn($canAccessShareByHelper);
+
+			$this->serverContainer->method('query')
+				->with('\OCA\Spreed\Share\Helper\ShareAPIController')
+				->willReturn($helper);
+		}
+
+		$this->assertEquals($expected, $this->invokePrivate($this->ocs, 'canAccessShare', [$share]));
+	}
+
 	/**
 	 * @expectedException \OCP\AppFramework\OCS\OCSNotFoundException
 	 * @expectedExceptionMessage Please specify a file or folder path
@@ -733,7 +800,8 @@ class ShareAPIControllerTest extends TestCase {
 				$this->currentUser,
 				$this->l,
 				$this->config,
-				$this->appManager
+				$this->appManager,
+				$this->serverContainer
 			])->setMethods(['formatShare'])
 			->getMock();
 
@@ -832,7 +900,8 @@ class ShareAPIControllerTest extends TestCase {
 				$this->currentUser,
 				$this->l,
 				$this->config,
-				$this->appManager
+				$this->appManager,
+				$this->serverContainer
 			])->setMethods(['formatShare'])
 			->getMock();
 
@@ -1128,6 +1197,186 @@ class ShareAPIControllerTest extends TestCase {
 		$ocs->createShare('valid-path', \OCP\Constants::PERMISSION_ALL, \OCP\Share::SHARE_TYPE_LINK, null, 'false', '', null, 'a1b2d3');
 	}
 
+	public function testCreateShareRoom() {
+		$ocs = $this->mockFormatShare();
+
+		$share = $this->newShare();
+		$this->shareManager->method('newShare')->willReturn($share);
+
+		$userFolder = $this->getMockBuilder(Folder::class)->getMock();
+		$this->rootFolder->expects($this->once())
+				->method('getUserFolder')
+				->with('currentUser')
+				->willReturn($userFolder);
+
+		$path = $this->getMockBuilder(File::class)->getMock();
+		$storage = $this->getMockBuilder(Storage::class)->getMock();
+		$storage->method('instanceOfStorage')
+			->with('OCA\Files_Sharing\External\Storage')
+			->willReturn(false);
+		$path->method('getStorage')->willReturn($storage);
+		$userFolder->expects($this->once())
+				->method('get')
+				->with('valid-path')
+				->willReturn($path);
+
+		$path->expects($this->once())
+			->method('lock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+
+		$this->appManager->method('isEnabledForUser')
+			->with('spreed')
+			->willReturn(true);
+
+		$helper = $this->getMockBuilder('\OCA\Spreed\Share\Helper\ShareAPIController')
+			->setMethods(array('createShare'))
+			->getMock();
+		$helper->method('createShare')
+			->with(
+				$share,
+				'recipientRoom',
+				\OCP\Constants::PERMISSION_ALL &
+				~\OCP\Constants::PERMISSION_DELETE &
+				~\OCP\Constants::PERMISSION_CREATE,
+				''
+			)->will($this->returnCallback(
+				function ($share) {
+					$share->setSharedWith('recipientRoom');
+					$share->setPermissions(
+						\OCP\Constants::PERMISSION_ALL &
+						~\OCP\Constants::PERMISSION_DELETE &
+						~\OCP\Constants::PERMISSION_CREATE
+					);
+				}
+			));
+
+		$this->serverContainer->method('query')
+			->with('\OCA\Spreed\Share\Helper\ShareAPIController')
+			->willReturn($helper);
+
+		$this->shareManager->method('createShare')
+			->with($this->callback(function (\OCP\Share\IShare $share) use ($path) {
+				return $share->getNode() === $path &&
+					$share->getPermissions() === (
+						\OCP\Constants::PERMISSION_ALL &
+						~\OCP\Constants::PERMISSION_DELETE &
+						~\OCP\Constants::PERMISSION_CREATE
+					) &&
+					$share->getShareType() === \OCP\Share::SHARE_TYPE_ROOM &&
+					$share->getSharedWith() === 'recipientRoom' &&
+					$share->getSharedBy() === 'currentUser';
+			}))
+			->will($this->returnArgument(0));
+
+		$expected = new DataResponse([]);
+		$result = $ocs->createShare('valid-path', \OCP\Constants::PERMISSION_ALL, \OCP\Share::SHARE_TYPE_ROOM, 'recipientRoom');
+
+		$this->assertInstanceOf(get_class($expected), $result);
+		$this->assertEquals($expected->getData(), $result->getData());
+	}
+
+	/**
+	 * @expectedException \OCP\AppFramework\OCS\OCSForbiddenException
+	 * @expectedExceptionMessage Sharing valid-path failed because the back end does not support room shares
+	 */
+	public function testCreateShareRoomHelperNotAvailable() {
+		$ocs = $this->mockFormatShare();
+
+		$share = $this->newShare();
+		$this->shareManager->method('newShare')->willReturn($share);
+
+		$userFolder = $this->getMockBuilder(Folder::class)->getMock();
+		$this->rootFolder->expects($this->once())
+				->method('getUserFolder')
+				->with('currentUser')
+				->willReturn($userFolder);
+
+		$path = $this->getMockBuilder(File::class)->getMock();
+		$storage = $this->getMockBuilder(Storage::class)->getMock();
+		$storage->method('instanceOfStorage')
+			->with('OCA\Files_Sharing\External\Storage')
+			->willReturn(false);
+		$path->method('getStorage')->willReturn($storage);
+		$path->method('getPath')->willReturn('valid-path');
+		$userFolder->expects($this->once())
+				->method('get')
+				->with('valid-path')
+				->willReturn($path);
+
+		$path->expects($this->once())
+			->method('lock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+
+		$this->appManager->method('isEnabledForUser')
+			->with('spreed')
+			->willReturn(false);
+
+		$this->shareManager->expects($this->never())->method('createShare');
+
+		$ocs->createShare('valid-path', \OCP\Constants::PERMISSION_ALL, \OCP\Share::SHARE_TYPE_ROOM, 'recipientRoom');
+	}
+
+	/**
+	 * @expectedException \OCP\AppFramework\OCS\OCSNotFoundException
+	 * @expectedExceptionMessage Exception thrown by the helper
+	 */
+	public function testCreateShareRoomHelperThrowException() {
+		$ocs = $this->mockFormatShare();
+
+		$share = $this->newShare();
+		$this->shareManager->method('newShare')->willReturn($share);
+
+		$userFolder = $this->getMockBuilder(Folder::class)->getMock();
+		$this->rootFolder->expects($this->once())
+				->method('getUserFolder')
+				->with('currentUser')
+				->willReturn($userFolder);
+
+		$path = $this->getMockBuilder(File::class)->getMock();
+		$storage = $this->getMockBuilder(Storage::class)->getMock();
+		$storage->method('instanceOfStorage')
+			->with('OCA\Files_Sharing\External\Storage')
+			->willReturn(false);
+		$path->method('getStorage')->willReturn($storage);
+		$userFolder->expects($this->once())
+				->method('get')
+				->with('valid-path')
+				->willReturn($path);
+
+		$path->expects($this->once())
+			->method('lock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+
+		$this->appManager->method('isEnabledForUser')
+			->with('spreed')
+			->willReturn(true);
+
+		$helper = $this->getMockBuilder('\OCA\Spreed\Share\Helper\ShareAPIController')
+			->setMethods(array('createShare'))
+			->getMock();
+		$helper->method('createShare')
+			->with(
+				$share,
+				'recipientRoom',
+				\OCP\Constants::PERMISSION_ALL &
+				~\OCP\Constants::PERMISSION_DELETE &
+				~\OCP\Constants::PERMISSION_CREATE,
+				''
+			)->will($this->returnCallback(
+				function ($share) {
+					throw new OCSNotFoundException("Exception thrown by the helper");
+				}
+			));
+
+		$this->serverContainer->method('query')
+			->with('\OCA\Spreed\Share\Helper\ShareAPIController')
+			->willReturn($helper);
+
+		$this->shareManager->expects($this->never())->method('createShare');
+
+		$ocs->createShare('valid-path', \OCP\Constants::PERMISSION_ALL, \OCP\Share::SHARE_TYPE_ROOM, 'recipientRoom');
+	}
+
 	/**
 	 * Test for https://github.com/owncloud/core/issues/22587
 	 * TODO: Remove once proper solution is in place
@@ -1149,7 +1398,8 @@ class ShareAPIControllerTest extends TestCase {
 				$this->currentUser,
 				$this->l,
 				$this->config,
-				$this->appManager
+				$this->appManager,
+				$this->serverContainer
 			])->setMethods(['formatShare'])
 			->getMock();
 
@@ -1681,7 +1931,8 @@ class ShareAPIControllerTest extends TestCase {
 			->method('getSharedWith')
 			->will($this->returnValueMap([
 				['currentUser', \OCP\Share::SHARE_TYPE_USER, $share->getNode(), -1, 0, []],
-				['currentUser', \OCP\Share::SHARE_TYPE_GROUP, $share->getNode(), -1, 0, [$incomingShare]]
+				['currentUser', \OCP\Share::SHARE_TYPE_GROUP, $share->getNode(), -1, 0, [$incomingShare]],
+				['currentUser', \OCP\Share::SHARE_TYPE_ROOM, $share->getNode(), -1, 0, []]
 			]));
 
 		$this->shareManager->expects($this->never())->method('updateShare');
@@ -1726,7 +1977,8 @@ class ShareAPIControllerTest extends TestCase {
 			->method('getSharedWith')
 			->will($this->returnValueMap([
 				['currentUser', \OCP\Share::SHARE_TYPE_USER, $share->getNode(), -1, 0, [$incomingShare]],
-				['currentUser', \OCP\Share::SHARE_TYPE_GROUP, $share->getNode(), -1, 0, []]
+				['currentUser', \OCP\Share::SHARE_TYPE_GROUP, $share->getNode(), -1, 0, []],
+				['currentUser', \OCP\Share::SHARE_TYPE_ROOM, $share->getNode(), -1, 0, []]
 			]));
 
 		$this->shareManager->expects($this->never())->method('updateShare');
@@ -1740,6 +1992,69 @@ class ShareAPIControllerTest extends TestCase {
 		}
 	}
 
+	public function testUpdateShareCannotIncreasePermissionsRoomShare() {
+		$ocs = $this->mockFormatShare();
+
+		$folder = $this->createMock(Folder::class);
+
+		$share = \OC::$server->getShareManager()->newShare();
+		$share
+			->setId(42)
+			->setSharedBy($this->currentUser)
+			->setShareOwner('anotheruser')
+			->setShareType(\OCP\Share::SHARE_TYPE_ROOM)
+			->setSharedWith('group1')
+			->setPermissions(\OCP\Constants::PERMISSION_READ)
+			->setNode($folder);
+
+		// note: updateShare will modify the received instance but getSharedWith will reread from the database,
+		// so their values will be different
+		$incomingShare = \OC::$server->getShareManager()->newShare();
+		$incomingShare
+			->setId(42)
+			->setSharedBy($this->currentUser)
+			->setShareOwner('anotheruser')
+			->setShareType(\OCP\Share::SHARE_TYPE_ROOM)
+			->setSharedWith('group1')
+			->setPermissions(\OCP\Constants::PERMISSION_READ)
+			->setNode($folder);
+
+		$this->request
+			->method('getParam')
+			->will($this->returnValueMap([
+				['permissions', null, '31'],
+			]));
+
+		$this->shareManager
+			->method('getShareById')
+			->will($this->returnCallback(
+				function ($id) use ($share) {
+					if ($id !== 'ocRoomShare:42') {
+						throw new \OCP\Share\Exceptions\ShareNotFound();
+					}
+
+					return $share;
+				}
+			));
+
+		$this->shareManager->expects($this->any())
+			->method('getSharedWith')
+			->will($this->returnValueMap([
+				['currentUser', \OCP\Share::SHARE_TYPE_USER, $share->getNode(), -1, 0, []],
+				['currentUser', \OCP\Share::SHARE_TYPE_GROUP, $share->getNode(), -1, 0, []],
+				['currentUser', \OCP\Share::SHARE_TYPE_ROOM, $share->getNode(), -1, 0, [$incomingShare]]
+			]));
+
+		$this->shareManager->expects($this->never())->method('updateShare');
+
+		try {
+			$ocs->updateShare(42, 31);
+			$this->fail();
+		} catch (OCSNotFoundException $e) {
+			$this->assertEquals('Cannot increase permissions', $e->getMessage());
+		}
+	}
+
 	public function testUpdateShareCanIncreasePermissionsIfOwner() {
 		$ocs = $this->mockFormatShare();
 
@@ -2410,4 +2725,160 @@ class ShareAPIControllerTest extends TestCase {
 			$this->assertTrue($exception);
 		}
 	}
+
+	public function dataFormatRoomShare() {
+		$file = $this->getMockBuilder(File::class)->getMock();
+		$parent = $this->getMockBuilder(Folder::class)->getMock();
+
+		$file->method('getMimeType')->willReturn('myMimeType');
+
+		$file->method('getPath')->willReturn('file');
+
+		$parent->method('getId')->willReturn(1);
+		$file->method('getId')->willReturn(3);
+
+		$file->method('getParent')->willReturn($parent);
+
+		$cache = $this->getMockBuilder('OCP\Files\Cache\ICache')->getMock();
+		$cache->method('getNumericStorageId')->willReturn(100);
+		$storage = $this->getMockBuilder(Storage::class)->getMock();
+		$storage->method('getId')->willReturn('storageId');
+		$storage->method('getCache')->willReturn($cache);
+
+		$file->method('getStorage')->willReturn($storage);
+
+		$result = [];
+
+		$share = \OC::$server->getShareManager()->newShare();
+		$share->setShareType(\OCP\Share::SHARE_TYPE_ROOM)
+			->setSharedWith('recipientRoom')
+			->setSharedBy('initiator')
+			->setShareOwner('owner')
+			->setPermissions(\OCP\Constants::PERMISSION_READ)
+			->setNode($file)
+			->setShareTime(new \DateTime('2000-01-01T00:01:02'))
+			->setTarget('myTarget')
+			->setNote('personal note')
+			->setId(42);
+
+		$result[] = [
+			[
+				'id' => 42,
+				'share_type' => \OCP\Share::SHARE_TYPE_ROOM,
+				'uid_owner' => 'initiator',
+				'displayname_owner' => 'initiator',
+				'permissions' => 1,
+				'stime' => 946684862,
+				'parent' => null,
+				'expiration' => null,
+				'token' => null,
+				'uid_file_owner' => 'owner',
+				'displayname_file_owner' => 'owner',
+				'note' => 'personal note',
+				'path' => 'file',
+				'item_type' => 'file',
+				'storage_id' => 'storageId',
+				'storage' => 100,
+				'item_source' => 3,
+				'file_source' => 3,
+				'file_parent' => 1,
+				'file_target' => 'myTarget',
+				'share_with' => 'recipientRoom',
+				'share_with_displayname' => '',
+				'mail_send' => 0,
+				'mimetype' => 'myMimeType',
+			], $share, false, []
+		];
+
+		$share = \OC::$server->getShareManager()->newShare();
+		$share->setShareType(\OCP\Share::SHARE_TYPE_ROOM)
+			->setSharedWith('recipientRoom')
+			->setSharedBy('initiator')
+			->setShareOwner('owner')
+			->setPermissions(\OCP\Constants::PERMISSION_READ)
+			->setNode($file)
+			->setShareTime(new \DateTime('2000-01-01T00:01:02'))
+			->setTarget('myTarget')
+			->setNote('personal note')
+			->setId(42);
+
+		$result[] = [
+			[
+				'id' => 42,
+				'share_type' => \OCP\Share::SHARE_TYPE_ROOM,
+				'uid_owner' => 'initiator',
+				'displayname_owner' => 'initiator',
+				'permissions' => 1,
+				'stime' => 946684862,
+				'parent' => null,
+				'expiration' => null,
+				'token' => null,
+				'uid_file_owner' => 'owner',
+				'displayname_file_owner' => 'owner',
+				'note' => 'personal note',
+				'path' => 'file',
+				'item_type' => 'file',
+				'storage_id' => 'storageId',
+				'storage' => 100,
+				'item_source' => 3,
+				'file_source' => 3,
+				'file_parent' => 1,
+				'file_target' => 'myTarget',
+				'share_with' => 'recipientRoom',
+				'share_with_displayname' => 'recipientRoomName',
+				'mail_send' => 0,
+				'mimetype' => 'myMimeType',
+			], $share, true, [
+				'share_with_displayname' => 'recipientRoomName'
+			]
+		];
+
+		return $result;
+	}
+
+	/**
+	 * @dataProvider dataFormatRoomShare
+	 *
+	 * @param array $expects
+	 * @param \OCP\Share\IShare $share
+	 * @param bool $helperAvailable
+	 * @param array $formatShareByHelper
+	 */
+	public function testFormatRoomShare(array $expects, \OCP\Share\IShare $share, bool $helperAvailable, array $formatShareByHelper) {
+		$this->rootFolder->method('getUserFolder')
+			->with($this->currentUser)
+			->will($this->returnSelf());
+
+		$this->rootFolder->method('getById')
+			->with($share->getNodeId())
+			->willReturn([$share->getNode()]);
+
+		$this->rootFolder->method('getRelativePath')
+			->with($share->getNode()->getPath())
+			->will($this->returnArgument(0));
+
+		if (!$helperAvailable) {
+			$this->appManager->method('isEnabledForUser')
+				->with('spreed')
+				->willReturn(false);
+		} else {
+			$this->appManager->method('isEnabledForUser')
+				->with('spreed')
+				->willReturn(true);
+
+			$helper = $this->getMockBuilder('\OCA\Spreed\Share\Helper\ShareAPIController')
+				->setMethods(array('formatShare'))
+				->getMock();
+			$helper->method('formatShare')
+				->with($share)
+				->willReturn($formatShareByHelper);
+
+			$this->serverContainer->method('query')
+				->with('\OCA\Spreed\Share\Helper\ShareAPIController')
+				->willReturn($helper);
+		}
+
+		$result = $this->invokePrivate($this->ocs, 'formatShare', [$share]);
+		$this->assertEquals($expects, $result);
+	}
 }