Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[stable27] fix: avoid douple expireDate parsing #44911

Closed
wants to merge 2 commits into from
Closed

[stable27] fix: avoid douple expireDate parsing #44911

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1bd9fe6
refactor(shareApiController): use contrusctor property promotion & DI…
Fenn-CS Apr 15, 2024
c902c22
fix(shareApiController): avoid duplicated expiryDate operations
Fenn-CS Apr 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 17 additions & 69 deletions apps/files_sharing/lib/Controller/ShareAPIController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@
use OCP\IL10N;
use OCP\IPreview;
use OCP\IRequest;
use OCP\IServerContainer;
use OCP\IURLGenerator;
use OCP\IUserManager;
use OCP\Lock\ILockingProvider;
Expand All @@ -83,6 +82,7 @@
use OCP\Share\IShare;
use OCP\UserStatus\IManager as IUserStatusManager;
use Psr\Container\ContainerExceptionInterface;
use Psr\Container\ContainerInterface;
use Psr\Log\LoggerInterface;

/**
Expand All @@ -92,32 +92,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.
Expand All @@ -134,26 +110,15 @@
IL10N $l10n,
IConfig $config,
IAppManager $appManager,
IServerContainer $serverContainer,

Check failure on line 113 in apps/files_sharing/lib/Controller/ShareAPIController.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

UndefinedClass 

apps/files_sharing/lib/Controller/ShareAPIController.php:113:3: UndefinedClass: Class, interface or enum named OCA\Files_Sharing\Controller\IServerContainer does not exist (see https://psalm.dev/019)
IUserStatusManager $userStatusManager,
IPreview $previewManager,
private IDateTimeZone $dateTimeZone,
private LoggerInterface $logger,
?string $userId = null

Check failure on line 118 in apps/files_sharing/lib/Controller/ShareAPIController.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

DuplicateParam 

apps/files_sharing/lib/Controller/ShareAPIController.php:118:3: DuplicateParam: Duplicate param $userId in docblock for OCA\Files_Sharing\Controller\ShareAPIController::__construct (see https://psalm.dev/065)

Check failure

Code scanning / Psalm

DuplicateParam Error

Duplicate param $userId in docblock for OCA\Files_Sharing\Controller\ShareAPIController::__construct
) {
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;
}

/**
Expand All @@ -167,7 +132,7 @@
* @suppress PhanUndeclaredClassMethod
*/
protected function formatShare(IShare $share, Node $recipientNode = null): array {
$sharedBy = $this->userManager->get($share->getSharedBy());

Check failure on line 135 in apps/files_sharing/lib/Controller/ShareAPIController.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

UndefinedThisPropertyFetch 

apps/files_sharing/lib/Controller/ShareAPIController.php:135:15: UndefinedThisPropertyFetch: Instance property OCA\Files_Sharing\Controller\ShareAPIController::$userManager is not defined (see https://psalm.dev/041)
$shareOwner = $this->userManager->get($share->getShareOwner());

$result = [
Expand All @@ -190,7 +155,7 @@
'displayname_file_owner' => $shareOwner !== null ? $shareOwner->getDisplayName() : $share->getShareOwner(),
];

$userFolder = $this->rootFolder->getUserFolder($this->currentUser);

Check failure on line 158 in apps/files_sharing/lib/Controller/ShareAPIController.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

UndefinedThisPropertyFetch 

apps/files_sharing/lib/Controller/ShareAPIController.php:158:17: UndefinedThisPropertyFetch: Instance property OCA\Files_Sharing\Controller\ShareAPIController::$rootFolder is not defined (see https://psalm.dev/041)
if ($recipientNode) {
$node = $recipientNode;
} else {
Expand All @@ -214,7 +179,7 @@
}

$result['mimetype'] = $node->getMimetype();
$result['has_preview'] = $this->previewManager->isAvailable($node);

Check failure on line 182 in apps/files_sharing/lib/Controller/ShareAPIController.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

UndefinedThisPropertyFetch 

apps/files_sharing/lib/Controller/ShareAPIController.php:182:28: UndefinedThisPropertyFetch: Instance property OCA\Files_Sharing\Controller\ShareAPIController::$previewManager is not defined (see https://psalm.dev/041)
$result['storage_id'] = $node->getStorage()->getId();
$result['storage'] = $node->getStorage()->getCache()->getNumericStorageId();
$result['item_source'] = $node->getId();
Expand All @@ -237,7 +202,7 @@
) : $share->getSharedWith();
$result['status'] = [];

$userStatuses = $this->userStatusManager->getUserStatuses([$share->getSharedWith()]);

Check failure on line 205 in apps/files_sharing/lib/Controller/ShareAPIController.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

UndefinedThisPropertyFetch 

apps/files_sharing/lib/Controller/ShareAPIController.php:205:20: UndefinedThisPropertyFetch: Instance property OCA\Files_Sharing\Controller\ShareAPIController::$userStatusManager is not defined (see https://psalm.dev/041)
$userStatus = array_shift($userStatuses);
if ($userStatus) {
$result['status'] = [
Expand All @@ -250,7 +215,7 @@
];
}
} elseif ($share->getShareType() === IShare::TYPE_GROUP) {
$group = $this->groupManager->get($share->getSharedWith());

Check failure on line 218 in apps/files_sharing/lib/Controller/ShareAPIController.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

UndefinedThisPropertyFetch 

apps/files_sharing/lib/Controller/ShareAPIController.php:218:13: UndefinedThisPropertyFetch: Instance property OCA\Files_Sharing\Controller\ShareAPIController::$groupManager is not defined (see https://psalm.dev/041)
$result['share_with'] = $share->getSharedWith();
$result['share_with_displayname'] = $group !== null ? $group->getDisplayName() : $share->getSharedWith();
} elseif ($share->getShareType() === IShare::TYPE_LINK) {
Expand All @@ -258,14 +223,14 @@
// "share_with" and "share_with_displayname" for passwords of link
// shares was deprecated in Nextcloud 15, use "password" instead.
$result['share_with'] = $share->getPassword();
$result['share_with_displayname'] = '(' . $this->l->t('Shared link') . ')';

Check failure on line 226 in apps/files_sharing/lib/Controller/ShareAPIController.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

UndefinedThisPropertyFetch 

apps/files_sharing/lib/Controller/ShareAPIController.php:226:46: UndefinedThisPropertyFetch: Instance property OCA\Files_Sharing\Controller\ShareAPIController::$l is not defined (see https://psalm.dev/041)

$result['password'] = $share->getPassword();

$result['send_password_by_talk'] = $share->getSendPasswordByTalk();

$result['token'] = $share->getToken();
$result['url'] = $this->urlGenerator->linkToRouteAbsolute('files_sharing.sharecontroller.showShare', ['token' => $share->getToken()]);

Check failure on line 233 in apps/files_sharing/lib/Controller/ShareAPIController.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

UndefinedThisPropertyFetch 

apps/files_sharing/lib/Controller/ShareAPIController.php:233:21: UndefinedThisPropertyFetch: Instance property OCA\Files_Sharing\Controller\ShareAPIController::$urlGenerator is not defined (see https://psalm.dev/041)
} elseif ($share->getShareType() === IShare::TYPE_REMOTE) {
$result['share_with'] = $share->getSharedWith();
$result['share_with_displayname'] = $this->getCachedFederatedDisplayName($share->getSharedWith());
Expand Down Expand Up @@ -355,7 +320,7 @@
'strict_search' => true,
]);
} catch (Exception $e) {
Server::get(LoggerInterface::class)->error(
$this->logger->error(
$e->getMessage(),
['exception' => $e]
);
Expand Down Expand Up @@ -429,7 +394,7 @@
private function retrieveFederatedDisplayName(array $userIds, bool $cacheOnly = false): array {
// check if gss is enabled and available
if (count($userIds) === 0
|| !$this->appManager->isInstalled('globalsiteselector')

Check failure on line 397 in apps/files_sharing/lib/Controller/ShareAPIController.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

UndefinedThisPropertyFetch 

apps/files_sharing/lib/Controller/ShareAPIController.php:397:8: UndefinedThisPropertyFetch: Instance property OCA\Files_Sharing\Controller\ShareAPIController::$appManager is not defined (see https://psalm.dev/041)
|| !class_exists('\OCA\GlobalSiteSelector\Service\SlaveService')) {
return [];
}
Expand All @@ -437,7 +402,7 @@
try {
$slaveService = Server::get(\OCA\GlobalSiteSelector\Service\SlaveService::class);
} catch (\Throwable $e) {
Server::get(LoggerInterface::class)->error(
$this->logger->error(
$e->getMessage(),
['exception' => $e]
);
Expand Down Expand Up @@ -653,6 +618,16 @@
$share = $this->setShareAttributes($share, $attributes);
}

//Expire date
if ($expireDate !== '') {
try {
$expireDate = $this->parseDate($expireDate);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs #44916 otherwise it breaks opencloudmesh, deck and talk

$share->setExpirationDate($expireDate);
} catch (\Exception $e) {
throw new OCSNotFoundException($this->l->t('Invalid date, date format must be YYYY-MM-DD'));

Check failure

Code scanning / Psalm

UndefinedThisPropertyFetch Error

Instance property OCA\Files_Sharing\Controller\ShareAPIController::$l is not defined
}
}

$share->setSharedBy($this->currentUser);
$this->checkInheritedAttributes($share);

Expand Down Expand Up @@ -739,15 +714,6 @@

$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()) {
Expand All @@ -760,14 +726,6 @@

$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'));
Expand Down Expand Up @@ -803,16 +761,6 @@
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 !== '') {
Expand Down
9 changes: 6 additions & 3 deletions apps/files_sharing/tests/ApiTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,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
Expand Down Expand Up @@ -121,10 +122,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(
Expand All @@ -135,14 +137,15 @@ private function createOCS($userId) {
\OC::$server->getUserManager(),
\OC::$server->getRootFolder(),
\OC::$server->getURLGenerator(),
$userId,
$l,
$config,
$appManager,
$serverContainer,
$userStatusManager,
$previewManager,
$dateTimeZone,
$logger,
$userId,
);
}

Expand Down
Loading
Loading