From fd1b563cd0b9e73929bb915a23ae02b3bb911c7e Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 7 Apr 2022 20:28:09 +0200 Subject: [PATCH] Sharing from review Signed-off-by: Vincent Petry Co-authored-by: Carl Schwan --- .../lib/Controller/ShareAPIController.php | 35 ++++----- .../lib/Controller/ShareController.php | 77 +++++-------------- apps/sharebymail/lib/ShareByMailProvider.php | 39 +++++----- .../tests/ShareByMailProviderTest.php | 4 +- lib/private/Share20/Manager.php | 8 +- lib/private/Share20/Share.php | 7 +- .../AuthPublicShareController.php | 15 ++-- lib/public/Share/IShare.php | 10 +-- 8 files changed, 77 insertions(+), 118 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index c2467d3e0396c..2b62d60bc8d10 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -1523,30 +1523,27 @@ private function parseDate(string $expireDate): \DateTime { /** * Set the share's password expiration time - * - * @param IShare $share - * */ - private function setSharePasswordExpirationTime($share) { + private function setSharePasswordExpirationTime(IShare $share): void { if ($this->config->getSystemValue('allow_mail_share_permanent_password')) { // Sets password expiration date to NULL $share->setPasswordExpirationTime(); - } else { - // Sets password expiration date - $expirationTime = null; - try { - $now = new \DateTime(); - $expirationInterval = $this->config->getSystemValue('share_temporary_password_expiration_interval'); - if ($expirationInterval === '' || is_null($expirationInterval)) { - $expirationInterval = 'P0DT15M'; - } - $expirationTime = $now->add(new \DateInterval($expirationInterval)); - } catch (\Exception $e) { - // Catches invalid format for system value 'share_temporary_password_expiration_interval' - $expirationTime = $now->add(new \DateInterval('P0DT15M')); - } finally { - $share->setPasswordExpirationTime($expirationTime); + return; + } + // Sets password expiration date + $expirationTime = null; + try { + $now = new \DateTime(); + $expirationInterval = $this->config->getSystemValue('share_temporary_password_expiration_interval'); + if ($expirationInterval === '' || is_null($expirationInterval)) { + $expirationInterval = 'P0DT15M'; } + $expirationTime = $now->add(new \DateInterval($expirationInterval)); + } catch (\Exception $e) { + // Catches invalid format for system value 'share_temporary_password_expiration_interval' + $expirationTime = $now->add(new \DateInterval('P0DT15M')); + } finally { + $share->setPasswordExpirationTime($expirationTime); } } diff --git a/apps/files_sharing/lib/Controller/ShareController.php b/apps/files_sharing/lib/Controller/ShareController.php index 16a13bd0ae92f..a12878e6de234 100644 --- a/apps/files_sharing/lib/Controller/ShareController.php +++ b/apps/files_sharing/lib/Controller/ShareController.php @@ -85,56 +85,21 @@ * @package OCA\Files_Sharing\Controllers */ class ShareController extends AuthPublicShareController { + protected IConfig $config; + protected IUserManager $userManager; + protected ILogger $logger; + protected \OCP\Activity\IManager $activityManager; + protected IPreview $previewManager; + protected IRootFolder $rootFolder; + protected FederatedShareProvider $federatedShareProvider; + protected IAccountManager $accountManager; + protected IEventDispatcher $eventDispatcher; + protected IL10N $l10n; + protected Defaults $defaults; + protected ShareManager $shareManager; + protected ISecureRandom $secureRandom; + protected ?Share\IShare $share = null; - /** @var IConfig */ - protected $config; - /** @var IUserManager */ - protected $userManager; - /** @var ILogger */ - protected $logger; - /** @var \OCP\Activity\IManager */ - protected $activityManager; - /** @var IPreview */ - protected $previewManager; - /** @var IRootFolder */ - protected $rootFolder; - /** @var FederatedShareProvider */ - protected $federatedShareProvider; - /** @var IAccountManager */ - protected $accountManager; - /** @var IEventDispatcher */ - protected $eventDispatcher; - /** @var IL10N */ - protected $l10n; - /** @var Defaults */ - protected $defaults; - /** @var ShareManager */ - protected $shareManager; - /** @var ISecureRandom */ - protected $secureRandom; - - /** @var Share\IShare */ - protected $share; - - /** - * @param string $appName - * @param IRequest $request - * @param IConfig $config - * @param IURLGenerator $urlGenerator - * @param IUserManager $userManager - * @param ILogger $logger - * @param \OCP\Activity\IManager $activityManager - * @param \OCP\Share\IManager $shareManager - * @param ISession $session - * @param IPreview $previewManager - * @param IRootFolder $rootFolder - * @param FederatedShareProvider $federatedShareProvider - * @param IAccountManager $accountManager - * @param IEventDispatcher $eventDispatcher - * @param IL10N $l10n - * @param ISecureRandom $secureRandom - * @param Defaults $defaults - */ public function __construct(string $appName, IRequest $request, IConfig $config, @@ -237,10 +202,10 @@ protected function showIdentificationResult(bool $success = false): TemplateResp /** * Validate the identity token of a public share * - * @param string $identityToken + * @param ?string $identityToken * @return bool */ - protected function validateIdentity(string $identityToken = null): bool { + protected function validateIdentity(?string $identityToken = null): bool { if ($this->share->getShareType() !== IShare::TYPE_EMAIL) { return false; @@ -250,25 +215,19 @@ protected function validateIdentity(string $identityToken = null): bool { return false; } - if ($identityToken !== $this->share->getSharedWith()) { - return false; - } - - return true; - + return $identityToken === $this->share->getSharedWith(); } /** * Generates a password for the share, respecting any password policy defined */ - protected function generatePassword() { + protected function generatePassword(): void { $event = new \OCP\Security\Events\GenerateSecurePasswordEvent(); $this->eventDispatcher->dispatchTyped($event); $password = $event->getPassword() ?? $this->secureRandom->generate(20); $this->share->setPassword($password); $this->shareManager->updateShare($this->share); - return; } protected function verifyPassword(string $password): bool { diff --git a/apps/sharebymail/lib/ShareByMailProvider.php b/apps/sharebymail/lib/ShareByMailProvider.php index bdf1c2f938190..62b885483c8f1 100644 --- a/apps/sharebymail/lib/ShareByMailProvider.php +++ b/apps/sharebymail/lib/ShareByMailProvider.php @@ -76,8 +76,7 @@ */ class ShareByMailProvider implements IShareProvider { - /** @var IConfig */ - private $config; + private IConfig $config; /** @var IDBConnection */ private $dbConnection; @@ -684,23 +683,24 @@ public function getChildren(IShare $parent) { } /** - * add share to the database and return the ID - * - * @param int $itemSource - * @param string $itemType - * @param string $shareWith - * @param string $sharedBy - * @param string $uidOwner - * @param int $permissions - * @param string $token - * @param string $password - * @param bool $sendPasswordByTalk - * @param bool $hideDownload - * @param string $label - * @param \DateTime|null $expirationTime - * @return int + * Add share to the database and return the ID */ - protected function addShareToDB($itemSource, $itemType, $shareWith, $sharedBy, $uidOwner, $permissions, $token, $password, $passwordExpirationTime, $sendPasswordByTalk, $hideDownload, $label, $expirationTime, $note = ''): int { + protected function addShareToDB( + ?int $itemSource, + ?string $itemType, + ?string $shareWith, + ?string $sharedBy, + ?string $uidOwner, + ?int $permissions, + ?string $token, + ?string $password, + ?\DateTimeInterface $passwordExpirationTime, + ?bool $sendPasswordByTalk, + ?bool $hideDownload, + ?string $label, + ?\DateTimeInterface $expirationTime, + ?string $note = '' + ): int { $qb = $this->dbConnection->getQueryBuilder(); $qb->insert('share') ->setValue('share_type', $qb->createNamedParameter(IShare::TYPE_EMAIL)) @@ -1026,7 +1026,8 @@ protected function createShareObject($data) { $share->setShareTime($shareTime); $share->setSharedWith($data['share_with']); $share->setPassword($data['password']); - $share->setPasswordExpirationTime($data['password_expiration_time']); + $passwordExpirationTime = \DateTime::createFromFormat('Y-m-d H:i:s', $data['password_expiration_time']); + $share->setPasswordExpirationTime($passwordExpirationTime !== false? $passwordExpirationTime : null); $share->setLabel($data['label']); $share->setSendPasswordByTalk((bool)$data['password_by_talk']); $share->setHideDownload((bool)$data['hide_download']); diff --git a/apps/sharebymail/tests/ShareByMailProviderTest.php b/apps/sharebymail/tests/ShareByMailProviderTest.php index dc547a0bdd8b6..bf8821c34fa8f 100644 --- a/apps/sharebymail/tests/ShareByMailProviderTest.php +++ b/apps/sharebymail/tests/ShareByMailProviderTest.php @@ -476,6 +476,7 @@ public function testAddShareToDB() { $hideDownload = true; $label = 'label'; $expiration = new \DateTime(); + $passwordExpirationTime = new \DateTime(); $instance = $this->getInstance(); @@ -491,7 +492,7 @@ public function testAddShareToDB() { $permissions, $token, $password, - null, + $passwordExpirationTime, $sendPasswordByTalk, $hideDownload, $label, @@ -518,6 +519,7 @@ public function testAddShareToDB() { $this->assertSame($permissions, (int)$result[0]['permissions']); $this->assertSame($token, $result[0]['token']); $this->assertSame($password, $result[0]['password']); + $this->assertSame($passwordExpirationTime->getTimestamp(), \DateTime::createFromFormat('Y-m-d H:i:s', $result[0]['password_expiration_time'])->getTimestamp()); $this->assertSame($sendPasswordByTalk, (bool)$result[0]['password_by_talk']); $this->assertSame($hideDownload, (bool)$result[0]['hide_download']); $this->assertSame($label, $result[0]['label']); diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 8dbffd260a423..4e87c37fedbcb 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1554,12 +1554,8 @@ public function checkPassword(IShare $share, $password) { // Makes sure password hasn't expired $expirationTime = $share->getPasswordExpirationTime(); - if ($expirationTime !== null) { - $expirationDateTime = new \DateTime($expirationTime); - $now = new \DateTime(); - if ($expirationDateTime < $now) { - return false; - } + if ($expirationTime !== null && $expirationTime < new \DateTime()) { + return false; } $newHash = ''; diff --git a/lib/private/Share20/Share.php b/lib/private/Share20/Share.php index c237649c5139f..7ed03832e4c71 100644 --- a/lib/private/Share20/Share.php +++ b/lib/private/Share20/Share.php @@ -73,8 +73,7 @@ class Share implements IShare { private $expireDate; /** @var string */ private $password; - /** @var string */ - private $passwordExpirationTime; + private ?\DateTimeInterface $passwordExpirationTime = null; /** @var bool */ private $sendPasswordByTalk = false; /** @var string */ @@ -466,7 +465,7 @@ public function getPassword() { /** * @inheritdoc */ - public function setPasswordExpirationTime($passwordExpirationTime = null) { + public function setPasswordExpirationTime(?\DateTimeInterface $passwordExpirationTime = null): IShare { $this->passwordExpirationTime = $passwordExpirationTime; return $this; } @@ -474,7 +473,7 @@ public function setPasswordExpirationTime($passwordExpirationTime = null) { /** * @inheritdoc */ - public function getPasswordExpirationTime() { + public function getPasswordExpirationTime(): ?\DateTimeInterface { return $this->passwordExpirationTime; } diff --git a/lib/public/AppFramework/AuthPublicShareController.php b/lib/public/AppFramework/AuthPublicShareController.php index 4be5e4ed321ed..5c20125e54cba 100644 --- a/lib/public/AppFramework/AuthPublicShareController.php +++ b/lib/public/AppFramework/AuthPublicShareController.php @@ -98,21 +98,26 @@ protected function showIdentificationResult(bool $success): TemplateResponse { * * @since 24.0.0 */ - abstract protected function validateIdentity(string $identityToken): bool; + protected function validateIdentity(?string $identityToken = null): bool { + return false; + }; /** * Generates a password * - * @since 14.0.0 + * @since 24.0.0 */ - abstract protected function generatePassword(); + protected function generatePassword(): void { + }; /** * Verify the password * - * @since 14.0.0 + * @since 24.0.0 */ - abstract protected function verifyPassword(string $password): bool; + protected function verifyPassword(string $password): bool { + return false; + }; /** * Function called after failed authentication diff --git a/lib/public/Share/IShare.php b/lib/public/Share/IShare.php index cfc24a866375c..1d3cf9bbbdff7 100644 --- a/lib/public/Share/IShare.php +++ b/lib/public/Share/IShare.php @@ -451,16 +451,16 @@ public function getPassword(); /** * Set the password's expiration time of this share. * - * @return \OCP\Share\IShare The modified object + * @return self The modified object + * @since 24.0.0 */ - public function setPasswordExpirationTime($passwordExpirationTime = null); + public function setPasswordExpirationTime(?\DateTimeInterface $passwordExpirationTime = null): IShare; /** * Get the password's expiration time of this share. - * - * @return string + * @since 24.0.0 */ - public function getPasswordExpirationTime(); + public function getPasswordExpirationTime(): ?\DateTimeInterface; /** * Set if the recipient can start a conversation with the owner to get the