From 30900ae751bd653260d8129f1ee353d46bae8067 Mon Sep 17 00:00:00 2001
From: fenn-cs <fenn25.fn@gmail.com>
Date: Mon, 15 Apr 2024 20:38:26 +0100
Subject: [PATCH 1/3] refactor(shareApiController): use contrusctor property
promotion & DI logger
Signed-off-by: fenn-cs <fenn25.fn@gmail.com>
---
.../lib/Controller/ShareAPIController.php | 72 ++++----------
apps/files_sharing/tests/ApiTest.php | 9 +-
.../Controller/ShareAPIControllerTest.php | 98 ++++++++-----------
3 files changed, 64 insertions(+), 115 deletions(-)
diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php
index 77faa2ab0c97a..10e3028a59f50 100644
--- a/apps/files_sharing/lib/Controller/ShareAPIController.php
+++ b/apps/files_sharing/lib/Controller/ShareAPIController.php
@@ -75,7 +75,6 @@
use OCP\IL10N;
use OCP\IPreview;
use OCP\IRequest;
-use OCP\IServerContainer;
use OCP\IURLGenerator;
use OCP\IUserManager;
use OCP\Lock\ILockingProvider;
@@ -87,6 +86,7 @@
use OCP\Share\IShare;
use OCP\UserStatus\IManager as IUserStatusManager;
use Psr\Container\ContainerExceptionInterface;
+use Psr\Container\ContainerInterface;
use Psr\Log\LoggerInterface;
/**
@@ -96,32 +96,8 @@
*/
class ShareAPIController extends OCSController {
- /** @var IManager */
- private $shareManager;
- /** @var IGroupManager */
- private $groupManager;
- /** @var IUserManager */
- private $userManager;
- /** @var IRootFolder */
- private $rootFolder;
- /** @var IURLGenerator */
- private $urlGenerator;
- /** @var string */
- private $currentUser;
- /** @var IL10N */
- private $l;
- /** @var \OCP\Files\Node */
- private $lockedNode;
- /** @var IConfig */
- private $config;
- /** @var IAppManager */
- private $appManager;
- /** @var IServerContainer */
- private $serverContainer;
- /** @var IUserStatusManager */
- private $userStatusManager;
- /** @var IPreview */
- private $previewManager;
+ private ?Node $lockedNode = null;
+ private string $currentUser;
/**
* Share20OCS constructor.
@@ -129,35 +105,23 @@ class ShareAPIController extends OCSController {
public function __construct(
string $appName,
IRequest $request,
- IManager $shareManager,
- IGroupManager $groupManager,
- IUserManager $userManager,
- IRootFolder $rootFolder,
- IURLGenerator $urlGenerator,
- string $userId = null,
- IL10N $l10n,
- IConfig $config,
- IAppManager $appManager,
- IServerContainer $serverContainer,
- IUserStatusManager $userStatusManager,
- IPreview $previewManager,
+ private IManager $shareManager,
+ private IGroupManager $groupManager,
+ private IUserManager $userManager,
+ private IRootFolder $rootFolder,
+ private IURLGenerator $urlGenerator,
+ private IL10N $l,
+ private IConfig $config,
+ private IAppManager $appManager,
+ private ContainerInterface $serverContainer,
+ private IUserStatusManager $userStatusManager,
+ private IPreview $previewManager,
private IDateTimeZone $dateTimeZone,
+ private LoggerInterface $logger,
+ ?string $userId = null
) {
parent::__construct($appName, $request);
-
- $this->shareManager = $shareManager;
- $this->userManager = $userManager;
- $this->groupManager = $groupManager;
- $this->request = $request;
- $this->rootFolder = $rootFolder;
- $this->urlGenerator = $urlGenerator;
$this->currentUser = $userId;
- $this->l = $l10n;
- $this->config = $config;
- $this->appManager = $appManager;
- $this->serverContainer = $serverContainer;
- $this->userStatusManager = $userStatusManager;
- $this->previewManager = $previewManager;
}
/**
@@ -376,7 +340,7 @@ private function getDisplayNameFromAddressBook(string $query, string $property):
'strict_search' => true,
]);
} catch (Exception $e) {
- Server::get(LoggerInterface::class)->error(
+ $this->logger->error(
$e->getMessage(),
['exception' => $e]
);
@@ -458,7 +422,7 @@ private function retrieveFederatedDisplayName(array $userIds, bool $cacheOnly =
try {
$slaveService = Server::get(\OCA\GlobalSiteSelector\Service\SlaveService::class);
} catch (\Throwable $e) {
- Server::get(LoggerInterface::class)->error(
+ $this->logger->error(
$e->getMessage(),
['exception' => $e]
);
diff --git a/apps/files_sharing/tests/ApiTest.php b/apps/files_sharing/tests/ApiTest.php
index 98f87c96f3128..cdb6d2ddcca0c 100644
--- a/apps/files_sharing/tests/ApiTest.php
+++ b/apps/files_sharing/tests/ApiTest.php
@@ -48,9 +48,10 @@
use OCP\IL10N;
use OCP\IPreview;
use OCP\IRequest;
-use OCP\IServerContainer;
use OCP\Share\IShare;
use OCP\UserStatus\IManager as IUserStatusManager;
+use Psr\Container\ContainerInterface;
+use Psr\Log\LoggerInterface;
/**
* Class ApiTest
@@ -120,10 +121,11 @@ private function createOCS($userId) {
});
$config = $this->createMock(IConfig::class);
$appManager = $this->createMock(IAppManager::class);
- $serverContainer = $this->createMock(IServerContainer::class);
+ $serverContainer = $this->createMock(ContainerInterface::class);
$userStatusManager = $this->createMock(IUserStatusManager::class);
$previewManager = $this->createMock(IPreview::class);
$dateTimeZone = $this->createMock(IDateTimeZone::class);
+ $logger = $this->createMock(LoggerInterface::class);
$dateTimeZone->method('getTimeZone')->willReturn(new \DateTimeZone(date_default_timezone_get()));
return new ShareAPIController(
@@ -134,7 +136,6 @@ private function createOCS($userId) {
\OC::$server->getUserManager(),
\OC::$server->getRootFolder(),
\OC::$server->getURLGenerator(),
- $userId,
$l,
$config,
$appManager,
@@ -142,6 +143,8 @@ private function createOCS($userId) {
$userStatusManager,
$previewManager,
$dateTimeZone,
+ $logger,
+ $userId,
);
}
diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
index 22b1da0f285ab..0a0e267857313 100644
--- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
+++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
@@ -53,7 +53,6 @@
use OCP\IL10N;
use OCP\IPreview;
use OCP\IRequest;
-use OCP\IServerContainer;
use OCP\IURLGenerator;
use OCP\IUser;
use OCP\IUserManager;
@@ -63,6 +62,8 @@
use OCP\Share\IManager;
use OCP\Share\IShare;
use OCP\UserStatus\IManager as IUserStatusManager;
+use Psr\Container\ContainerInterface;
+use Psr\Log\LoggerInterface;
use Test\TestCase;
/**
@@ -73,53 +74,23 @@
*/
class ShareAPIControllerTest extends TestCase {
- /** @var string */
- private $appName = 'files_sharing';
-
- /** @var \OC\Share20\Manager|\PHPUnit\Framework\MockObject\MockObject */
- private $shareManager;
-
- /** @var IGroupManager|\PHPUnit\Framework\MockObject\MockObject */
- private $groupManager;
-
- /** @var IUserManager|\PHPUnit\Framework\MockObject\MockObject */
- private $userManager;
-
- /** @var IRequest|\PHPUnit\Framework\MockObject\MockObject */
- private $request;
-
- /** @var IRootFolder|\PHPUnit\Framework\MockObject\MockObject */
- private $rootFolder;
-
- /** @var IURLGenerator|\PHPUnit\Framework\MockObject\MockObject */
- private $urlGenerator;
-
- /** @var string|\PHPUnit\Framework\MockObject\MockObject */
- private $currentUser;
-
- /** @var ShareAPIController */
- private $ocs;
-
- /** @var IL10N|\PHPUnit\Framework\MockObject\MockObject */
- private $l;
-
- /** @var IConfig|\PHPUnit\Framework\MockObject\MockObject */
- private $config;
-
- /** @var IAppManager|\PHPUnit\Framework\MockObject\MockObject */
- private $appManager;
-
- /** @var IServerContainer|\PHPUnit\Framework\MockObject\MockObject */
- private $serverContainer;
-
- /** @var IUserStatusManager|\PHPUnit\Framework\MockObject\MockObject */
- private $userStatusManager;
-
- /** @var IPreview|\PHPUnit\Framework\MockObject\MockObject */
- private $previewManager;
-
- /** @var IDateTimeZone|\PHPUnit\Framework\MockObject\MockObject */
- private $dateTimeZone;
+ private string $appName = 'files_sharing';
+ private \OC\Share20\Manager|\PHPUnit\Framework\MockObject\MockObject $shareManager;
+ private IGroupManager|\PHPUnit\Framework\MockObject\MockObject $groupManager;
+ private IUserManager|\PHPUnit\Framework\MockObject\MockObject $userManager;
+ private IRequest|\PHPUnit\Framework\MockObject\MockObject $request;
+ private IRootFolder|\PHPUnit\Framework\MockObject\MockObject $rootFolder;
+ private IURLGenerator|\PHPUnit\Framework\MockObject\MockObject $urlGenerator;
+ private string|\PHPUnit\Framework\MockObject\MockObject $currentUser;
+ private ShareAPIController $ocs;
+ private IL10N|\PHPUnit\Framework\MockObject\MockObject $l;
+ private IConfig|\PHPUnit\Framework\MockObject\MockObject $config;
+ private IAppManager|\PHPUnit\Framework\MockObject\MockObject $appManager;
+ private IServerContainer|\PHPUnit\Framework\MockObject\MockObject $serverContainer;
+ private IUserStatusManager|\PHPUnit\Framework\MockObject\MockObject $userStatusManager;
+ private IPreview|\PHPUnit\Framework\MockObject\MockObject $previewManager;
+ private IDateTimeZone|\PHPUnit\Framework\MockObject\MockObject $dateTimeZone;
+ private LoggerInterface $logger;
protected function setUp(): void {
$this->shareManager = $this->createMock(IManager::class);
@@ -144,7 +115,7 @@ protected function setUp(): void {
});
$this->config = $this->createMock(IConfig::class);
$this->appManager = $this->createMock(IAppManager::class);
- $this->serverContainer = $this->createMock(IServerContainer::class);
+ $this->serverContainer = $this->createMock(ContainerInterface::class);
$this->userStatusManager = $this->createMock(IUserStatusManager::class);
$this->previewManager = $this->createMock(IPreview::class);
$this->previewManager->method('isAvailable')
@@ -152,6 +123,7 @@ protected function setUp(): void {
return $fileInfo->getMimeType() === 'mimeWithPreview';
});
$this->dateTimeZone = $this->createMock(IDateTimeZone::class);
+ $this->logger = $this->createMock(LoggerInterface::class);
$this->ocs = new ShareAPIController(
$this->appName,
@@ -161,7 +133,6 @@ protected function setUp(): void {
$this->userManager,
$this->rootFolder,
$this->urlGenerator,
- $this->currentUser,
$this->l,
$this->config,
$this->appManager,
@@ -169,6 +140,8 @@ protected function setUp(): void {
$this->userStatusManager,
$this->previewManager,
$this->dateTimeZone,
+ $this->logger,
+ $this->currentUser,
);
}
@@ -185,7 +158,6 @@ private function mockFormatShare() {
$this->userManager,
$this->rootFolder,
$this->urlGenerator,
- $this->currentUser,
$this->l,
$this->config,
$this->appManager,
@@ -193,6 +165,8 @@ private function mockFormatShare() {
$this->userStatusManager,
$this->previewManager,
$this->dateTimeZone,
+ $this->logger,
+ $this->currentUser,
])->setMethods(['formatShare'])
->getMock();
}
@@ -783,7 +757,6 @@ public function testGetShare(\OCP\Share\IShare $share, array $result) {
$this->userManager,
$this->rootFolder,
$this->urlGenerator,
- $this->currentUser,
$this->l,
$this->config,
$this->appManager,
@@ -791,6 +764,9 @@ public function testGetShare(\OCP\Share\IShare $share, array $result) {
$this->userStatusManager,
$this->previewManager,
$this->dateTimeZone,
+ $this->logger,
+ $this->currentUser,
+
])->setMethods(['canAccessShare'])
->getMock();
@@ -1409,7 +1385,6 @@ public function testGetShares(array $getSharesParameters, array $shares, array $
$this->userManager,
$this->rootFolder,
$this->urlGenerator,
- $this->currentUser,
$this->l,
$this->config,
$this->appManager,
@@ -1417,6 +1392,8 @@ public function testGetShares(array $getSharesParameters, array $shares, array $
$this->userStatusManager,
$this->previewManager,
$this->dateTimeZone,
+ $this->logger,
+ $this->currentUser,
])->setMethods(['formatShare'])
->getMock();
@@ -1749,7 +1726,6 @@ public function testCreateShareUser() {
$this->userManager,
$this->rootFolder,
$this->urlGenerator,
- $this->currentUser,
$this->l,
$this->config,
$this->appManager,
@@ -1757,6 +1733,8 @@ public function testCreateShareUser() {
$this->userStatusManager,
$this->previewManager,
$this->dateTimeZone,
+ $this->logger,
+ $this->currentUser,
])->setMethods(['formatShare'])
->getMock();
@@ -1844,7 +1822,6 @@ public function testCreateShareGroup() {
$this->userManager,
$this->rootFolder,
$this->urlGenerator,
- $this->currentUser,
$this->l,
$this->config,
$this->appManager,
@@ -1852,6 +1829,8 @@ public function testCreateShareGroup() {
$this->userStatusManager,
$this->previewManager,
$this->dateTimeZone,
+ $this->logger,
+ $this->currentUser,
])->setMethods(['formatShare'])
->getMock();
@@ -2254,7 +2233,6 @@ public function testCreateShareRemote() {
$this->userManager,
$this->rootFolder,
$this->urlGenerator,
- $this->currentUser,
$this->l,
$this->config,
$this->appManager,
@@ -2262,6 +2240,8 @@ public function testCreateShareRemote() {
$this->userStatusManager,
$this->previewManager,
$this->dateTimeZone,
+ $this->logger,
+ $this->currentUser,
])->setMethods(['formatShare'])
->getMock();
@@ -2321,7 +2301,6 @@ public function testCreateShareRemoteGroup() {
$this->userManager,
$this->rootFolder,
$this->urlGenerator,
- $this->currentUser,
$this->l,
$this->config,
$this->appManager,
@@ -2329,6 +2308,8 @@ public function testCreateShareRemoteGroup() {
$this->userStatusManager,
$this->previewManager,
$this->dateTimeZone,
+ $this->logger,
+ $this->currentUser,
])->setMethods(['formatShare'])
->getMock();
@@ -2561,7 +2542,6 @@ public function testCreateReshareOfFederatedMountNoDeletePermissions() {
$this->userManager,
$this->rootFolder,
$this->urlGenerator,
- $this->currentUser,
$this->l,
$this->config,
$this->appManager,
@@ -2569,6 +2549,8 @@ public function testCreateReshareOfFederatedMountNoDeletePermissions() {
$this->userStatusManager,
$this->previewManager,
$this->dateTimeZone,
+ $this->logger,
+ $this->currentUser,
])->setMethods(['formatShare'])
->getMock();
From 3c507773b7ec550fe47bb7222df732a06f5fb281 Mon Sep 17 00:00:00 2001
From: fenn-cs <fenn25.fn@gmail.com>
Date: Mon, 15 Apr 2024 21:38:55 +0100
Subject: [PATCH 2/3] fix(shareApiController): avoid duplicated expiryDate
operations
`expireDate` can be set once and used anywhere needed, the current implementation,
duplicates this behavior which leads to `parseDate` receiving an a date object it
parsed and returend earlier in the createShare method.
Signed-off-by: fenn-cs <fenn25.fn@gmail.com>
---
.../lib/Controller/ShareAPIController.php | 37 +++++--------------
1 file changed, 10 insertions(+), 27 deletions(-)
diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php
index 10e3028a59f50..63e124a909f9d 100644
--- a/apps/files_sharing/lib/Controller/ShareAPIController.php
+++ b/apps/files_sharing/lib/Controller/ShareAPIController.php
@@ -646,6 +646,16 @@ public function createShare(
$share = $this->setShareAttributes($share, $attributes);
}
+ //Expire date
+ if ($expireDate !== '') {
+ try {
+ $expireDate = $this->parseDate($expireDate);
+ $share->setExpirationDate($expireDate);
+ } catch (\Exception $e) {
+ throw new OCSNotFoundException($this->l->t('Invalid date, date format must be YYYY-MM-DD'));
+ }
+ }
+
$share->setSharedBy($this->currentUser);
$this->checkInheritedAttributes($share);
@@ -732,15 +742,6 @@ public function createShare(
$share->setSharedWith($shareWith);
$share->setPermissions($permissions);
- if ($expireDate !== '') {
- try {
- $expireDate = $this->parseDate($expireDate);
- $share->setExpirationDate($expireDate);
- } catch (\Exception $e) {
- throw new OCSNotFoundException($this->l->t('Invalid date, date format must be YYYY-MM-DD'));
- }
- }
-
$share->setSharedWithDisplayName($this->getCachedFederatedDisplayName($shareWith, false));
} elseif ($shareType === IShare::TYPE_REMOTE_GROUP) {
if (!$this->shareManager->outgoingServer2ServerGroupSharesAllowed()) {
@@ -753,14 +754,6 @@ public function createShare(
$share->setSharedWith($shareWith);
$share->setPermissions($permissions);
- if ($expireDate !== '') {
- try {
- $expireDate = $this->parseDate($expireDate);
- $share->setExpirationDate($expireDate);
- } catch (\Exception $e) {
- throw new OCSNotFoundException($this->l->t('Invalid date, date format must be YYYY-MM-DD'));
- }
- }
} elseif ($shareType === IShare::TYPE_CIRCLE) {
if (!\OC::$server->getAppManager()->isEnabledForUser('circles') || !class_exists('\OCA\Circles\ShareByCircleProvider')) {
throw new OCSNotFoundException($this->l->t('You cannot share to a Circle if the app is not enabled'));
@@ -796,16 +789,6 @@ public function createShare(
throw new OCSBadRequestException($this->l->t('Unknown share type'));
}
- //Expire date
- if ($expireDate !== '') {
- try {
- $expireDate = $this->parseDate($expireDate);
- $share->setExpirationDate($expireDate);
- } catch (\Exception $e) {
- throw new OCSNotFoundException($this->l->t('Invalid date, date format must be YYYY-MM-DD'));
- }
- }
-
$share->setShareType($shareType);
if ($note !== '') {
From 4d748b9f5a69a9925e42522eaa7dc9b4d62142a2 Mon Sep 17 00:00:00 2001
From: Joas Schilling <coding@schilljs.com>
Date: Thu, 18 Apr 2024 14:31:45 +0200
Subject: [PATCH 3/3] fix(sharing): Don't change the type of the controller
argument
[EA] New value type (\DateTime) is not matching the resolved parameter type and might introduce types-related false-positives.
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: fenn-cs <fenn25.fn@gmail.com>
---
apps/files_sharing/lib/Controller/ShareAPIController.php | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php
index 63e124a909f9d..cdf370c82fd35 100644
--- a/apps/files_sharing/lib/Controller/ShareAPIController.php
+++ b/apps/files_sharing/lib/Controller/ShareAPIController.php
@@ -649,8 +649,8 @@ public function createShare(
//Expire date
if ($expireDate !== '') {
try {
- $expireDate = $this->parseDate($expireDate);
- $share->setExpirationDate($expireDate);
+ $expireDateTime = $this->parseDate($expireDate);
+ $share->setExpirationDate($expireDateTime);
} catch (\Exception $e) {
throw new OCSNotFoundException($this->l->t('Invalid date, date format must be YYYY-MM-DD'));
}