Merge pull request #40803 from owncloud/fix/fedsharing-permission-creep

fix: Do not allow to set higher permissions on a federated share for …
owncloud · Jun 7, 2023 · 86ca60e · 86ca60e
2 parents 4b39db6 + 24c4292
commit 86ca60e
Show file tree

Hide file tree

Showing 9 changed files with 50 additions and 65 deletions.
diff --git a/apps/federatedfilesharing/lib/AppInfo/Application.php b/apps/federatedfilesharing/lib/AppInfo/Application.php
@@ -148,16 +148,7 @@ function ($c) use ($server) {
 		$container->registerService(
 			'NotificationManager',
 			function ($c) {
-				return new NotificationManager(
-					$c->query('Permissions')
-				);
-			}
-		);
-
-		$container->registerService(
-			'Permissions',
-			function ($c) {
-				return new Permissions();
+				return new NotificationManager();
 			}
 		);
 
@@ -208,9 +199,7 @@ protected function initFederatedShareProvider() {
 			\OC::$server->getMemCacheFactory(),
 			\OC::$server->getHTTPClientService()
 		);
-		$notificationManager = new NotificationManager(
-			new Permissions()
-		);
+		$notificationManager = new NotificationManager();
 		$notifications = new Notifications(
 			$addressHandler,
 			\OC::$server->getHTTPClientService(),

diff --git a/apps/federatedfilesharing/lib/FedShareManager.php b/apps/federatedfilesharing/lib/FedShareManager.php
@@ -69,11 +69,6 @@ class FedShareManager {
 	 */
 	private $addressHandler;
 
-	/**
-	 * @var Permissions
-	 */
-	private $permissions;
-
 	/**
 	 * @var EventDispatcherInterface
 	 */
@@ -88,7 +83,6 @@ class FedShareManager {
 	 * @param ActivityManager $activityManager
 	 * @param NotificationManager $notificationManager
 	 * @param AddressHandler $addressHandler
-	 * @param Permissions $permissions
 	 * @param EventDispatcherInterface $eventDispatcher
 	 */
 	public function __construct(
@@ -98,7 +92,6 @@ public function __construct(
 		ActivityManager $activityManager,
 		NotificationManager $notificationManager,
 		AddressHandler $addressHandler,
-		Permissions $permissions,
 		EventDispatcherInterface $eventDispatcher
 	) {
 		$this->federatedShareProvider = $federatedShareProvider;
@@ -107,7 +100,6 @@ public function __construct(
 		$this->activityManager = $activityManager;
 		$this->notificationManager = $notificationManager;
 		$this->addressHandler = $addressHandler;
-		$this->permissions = $permissions;
 		$this->eventDispatcher = $eventDispatcher;
 	}
 
@@ -316,7 +308,7 @@ public function undoReshare(IShare $share) {
 	 * @return void
 	 */
 	public function updateOcmPermissions(IShare $share, $ocmPermissions) {
-		$permissions = $this->permissions->toOcPermissions($ocmPermissions);
+		$permissions = Permissions::toOcPermissions($ocmPermissions);
 		$this->updatePermissions($share, $permissions);
 	}
 
@@ -328,8 +320,9 @@ public function updateOcmPermissions(IShare $share, $ocmPermissions) {
 	 *
 	 * @return void
 	 */
-	public function updatePermissions(IShare $share, $permissions) {
-		if ($share->getPermissions() !== $permissions) {
+	public function updatePermissions(IShare $share, int $permissions): void {
+		# permissions can only be reduced but not upgraded
+		if (Permissions::isNewPermissionHigher($share->getPermissions(), $permissions)) {
 			$share->setPermissions($permissions);
 			$this->federatedShareProvider->update($share);
 		}

diff --git a/apps/federatedfilesharing/lib/Ocm/NotificationManager.php b/apps/federatedfilesharing/lib/Ocm/NotificationManager.php
@@ -29,20 +29,6 @@
  * @package OCA\FederatedFileSharing\Ocm
  */
 class NotificationManager {
-	/**
-	 * @var Permissions
-	 */
-	protected $permissions;
-
-	/**
-	 * NotificationManager constructor.
-	 *
-	 * @param Permissions $permissions
-	 */
-	public function __construct(Permissions $permissions) {
-		$this->permissions = $permissions;
-	}
-
 	/**
 	 * @param string $type
 	 *
@@ -87,7 +73,7 @@ public function convertToOcmFileNotification($remoteId, $token, $action, $data =
 		$notification->addNotificationData('message', $messages[$action]);
 
 		if ($action === 'permissions') {
-			$ocmPermissions = $this->permissions->toOcmPermissions($data['permissions']);
+			$ocmPermissions = Permissions::toOcmPermissions($data['permissions']);
 			$notification->addNotificationData('permission', $ocmPermissions);
 		}
 

diff --git a/apps/federatedfilesharing/lib/Ocm/Permissions.php b/apps/federatedfilesharing/lib/Ocm/Permissions.php
@@ -40,11 +40,11 @@ class Permissions {
 	 *
 	 * @return array
 	 */
-	public function toOcmPermissions($ocPermissions) {
+	public static function toOcmPermissions(int $ocPermissions): array {
 		$ocPermissions = (int) $ocPermissions;
 		$ocmPermissions = [];
 		if ($ocPermissions & Constants::PERMISSION_READ) {
-			$ocmPermissions[] = self::OCM_PERMISSION_READ . '';
+			$ocmPermissions[] = self::OCM_PERMISSION_READ;
 		}
 		if (($ocPermissions & Constants::PERMISSION_CREATE)
 			|| ($ocPermissions & Constants::PERMISSION_UPDATE)
@@ -62,7 +62,7 @@ public function toOcmPermissions($ocPermissions) {
 	 *
 	 * @return int
 	 */
-	public function toOcPermissions($ocmPermissions) {
+	public static function toOcPermissions(array $ocmPermissions): int {
 		$permissionMap = [
 			self::OCM_PERMISSION_READ => Constants::PERMISSION_READ,
 			self::OCM_PERMISSION_WRITE => Constants::PERMISSION_CREATE + Constants::PERMISSION_UPDATE,
@@ -77,4 +77,8 @@ public function toOcPermissions($ocmPermissions) {
 
 		return $ocPermissions;
 	}
+
+	public static function isNewPermissionHigher(int $existingPermission, int $newPermission): bool {
+		return ($existingPermission | $newPermission) === $existingPermission;
+	}
 }
diff --git a/apps/federatedfilesharing/tests/Controller/RequestHandlerTest.php b/apps/federatedfilesharing/tests/Controller/RequestHandlerTest.php
@@ -456,6 +456,7 @@ public function testUpdatePermissions(): void {
 		$this->ocmMiddleware->expects($this->once())
 			->method('getValidShare')
 			->willReturn($share);
+		$this->ocmMiddleware->method('normalizePermissions')->willReturnArgument(0);
 		$this->fedShareManager->expects($this->once())
 			->method('isFederatedReShare')
 			->willReturn(true);

diff --git a/apps/federatedfilesharing/tests/FedShareManagerTest.php b/apps/federatedfilesharing/tests/FedShareManagerTest.php
@@ -64,9 +64,6 @@ class FedShareManagerTest extends TestCase {
 	/** @var AddressHandler | \PHPUnit\Framework\MockObject\MockObject */
 	private $addressHandler;
 
-	/** @var Permissions | \PHPUnit\Framework\MockObject\MockObject */
-	private $permissions;
-
 	/** @var EventDispatcherInterface | \PHPUnit\Framework\MockObject\MockObject */
 	private $eventDispatcher;
 
@@ -87,8 +84,6 @@ protected function setUp(): void {
 		$this->addressHandler = $this->getMockBuilder(AddressHandler::class)
 			->disableOriginalConstructor()->getMock();
 
-		$this->permissions = $this->createMock(Permissions::class);
-
 		$this->eventDispatcher = $this->getMockBuilder(EventDispatcherInterface::class)
 			->getMock();
 
@@ -101,7 +96,6 @@ protected function setUp(): void {
 					$this->activityManager,
 					$this->notificationManager,
 					$this->addressHandler,
-					$this->permissions,
 					$this->eventDispatcher
 				]
 			)
@@ -298,4 +292,31 @@ public function testIsFederatedReShare() {
 			$isFederatedShared
 		);
 	}
+
+	public function providesPermissions(): \Generator {
+		$read = Permissions::toOcPermissions(['read']);
+		$readWrite = Permissions::toOcPermissions(['read', 'write']);
+		$readShare = Permissions::toOcPermissions(['read', 'share']);
+		$readWriteShare = Permissions::toOcPermissions(['read', 'write', 'share']);
+
+		yield 'read -> write is not allowed' => [$read, $readWrite, false];
+		yield 'write -> read is allowed' => [$readWrite, $read, true];
+		yield 'read+share -> write is not allowed' => [$readShare, $readWrite, false];
+		yield 'write+share -> read is allowed' => [$readWriteShare, $read, true];
+	}
+
+	/**
+	 * @dataProvider providesPermissions
+	 */
+	public function testGrantingHigherPermission(int $currentPermissions, int $newPermissions, bool $allowed): void {
+		$share = $this->getMockBuilder(IShare::class)
+			->disableOriginalConstructor()->getMock();
+		$share->method('getPermissions')->willReturn($currentPermissions);
+
+		$this->federatedShareProvider->expects($allowed ? $this->once() : $this->never())
+			->method('update')
+			->with($share);
+
+		$this->fedShareManager->updatePermissions($share, $newPermissions);
+	}
 }
diff --git a/apps/federatedfilesharing/tests/NotificationsTest.php b/apps/federatedfilesharing/tests/NotificationsTest.php
@@ -65,9 +65,7 @@ public function setUp(): void {
 		$this->config = $this->createMock(IConfig::class);
 		$this->discoveryManager = $this->getMockBuilder(DiscoveryManager::class)
 			->disableOriginalConstructor()->getMock();
-		$this->notificationManager = new NotificationManager(
-			$this->createMock(Permissions::class)
-		);
+		$this->notificationManager = new NotificationManager();
 		$this->httpClientService = $this->createMock(IClientService::class);
 		$this->addressHandler = $this->getMockBuilder(AddressHandler::class)
 			->disableOriginalConstructor()->getMock();

diff --git a/apps/federatedfilesharing/tests/Ocm/PermissionsTest.php b/apps/federatedfilesharing/tests/Ocm/PermissionsTest.php
@@ -32,24 +32,14 @@
  * @group DB
  */
 class PermissionsTest extends TestCase {
-	/**
-	 * @var Permissions
-	 */
-	private $permissions;
-
-	protected function setUp(): void {
-		parent::setUp();
-		$this->permissions = new Permissions();
-	}
-
 	/**
 	 * @dataProvider dataTestToOcPermissions
 	 *
 	 * @param string[] $ocmPermissions
 	 * @param int $expectedOcPermissions
 	 */
-	public function testToOcPermissions($ocmPermissions, $expectedOcPermissions) {
-		$ocPermissions = $this->permissions->toOcPermissions($ocmPermissions);
+	public function testToOcPermissions(array $ocmPermissions, int $expectedOcPermissions): void {
+		$ocPermissions = Permissions::toOcPermissions($ocmPermissions);
 		$this->assertEquals($expectedOcPermissions, $ocPermissions);
 	}
 
@@ -91,8 +81,8 @@ public function dataTestToOcPermissions() {
 	 * @param int $ocPermissions
 	 * @param string[] $expectedOcmPermissions
 	 */
-	public function testToOcmPermissions($ocPermissions, $expectedOcmPermissions) {
-		$ocmPermissions = $this->permissions->toOcmPermissions($ocPermissions);
+	public function testToOcmPermissions(int $ocPermissions, array $expectedOcmPermissions): void {
+		$ocmPermissions = Permissions::toOcmPermissions($ocPermissions);
 		$this->assertEquals($expectedOcmPermissions, $ocmPermissions);
 	}
 

diff --git a/changelog/unreleased/40803 b/changelog/unreleased/40803
@@ -0,0 +1,3 @@
+Bugfix: Disallow permission tobe upgraded via federated sharing
+
+https://github.com/owncloud/core/pull/40803