diff --git a/apps/federatedfilesharing/lib/AppInfo/Application.php b/apps/federatedfilesharing/lib/AppInfo/Application.php index 287626b69303..ccbee8ba8214 100644 --- 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 index a60d323194f2..4e101e1033a4 100644 --- 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 index 9f810798ffbd..5577c9eb09aa 100644 --- 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 index f75118d79c27..c8d6d6aaf2b7 100644 --- 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 index 932a5c531631..fe9df1709d13 100644 --- 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 index a46c8cc05847..8bd482900baf 100644 --- 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 index 5ff29603c02a..8e129b450bdf 100644 --- 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 index 107b2500c529..24934370e7c6 100644 --- 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 new file mode 100644 index 000000000000..ebdfe19037b5 --- /dev/null +++ b/changelog/unreleased/40803 @@ -0,0 +1,3 @@ +Bugfix: Disallow permission tobe upgraded via federated sharing + +https://github.com/owncloud/core/pull/40803