From 3190ba92467c1397ef89ea6ee8de3fadf07cd88a Mon Sep 17 00:00:00 2001 From: provokateurin Date: Wed, 23 Sep 2026 15:23:32 +0200 Subject: [PATCH 01/13] chore(psalm): Update baseline Signed-off-by: provokateurin --- build/psalm-baseline.xml | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index b6da12c8b93f5..ce4e99120819d 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -20,7 +20,6 @@ - @@ -201,32 +200,17 @@ - CLASS]]> - CLASS]]> CLASS]]> DTEND]]> DTEND]]> - DTEND]]> - DTSTART]]> DTSTART]]> - DTSTART]]> - DUE]]> DUE]]> DUE]]> DURATION]]> DURATION]]> - DURATION]]> - RDATE]]> - RDATE]]> RDATE]]> RDATE]]> - RDATE]]> - RDATE]]> - RRULE]]> RRULE]]> - RRULE]]> - UID]]> - UID]]> UID]]> @@ -2423,9 +2407,6 @@ - - - From 5cdeb8386f839edb989f10e71a04811b9bb2456d Mon Sep 17 00:00:00 2001 From: provokateurin Date: Wed, 2 Sep 2026 12:43:18 +0200 Subject: [PATCH 02/13] chore(Sharing): Remove some outdated TODOs Signed-off-by: provokateurin --- apps/sharing/appinfo/info.xml | 4 ++-- apps/sharing/lib/Command/AddShareRecipient.php | 1 - apps/sharing/openapi.json | 2 +- lib/private/Sharing/SharingManager.php | 2 -- lib/private/Sharing/SharingRegistry.php | 1 - 5 files changed, 3 insertions(+), 7 deletions(-) diff --git a/apps/sharing/appinfo/info.xml b/apps/sharing/appinfo/info.xml index cd653bf56fa09..b46cd273f56f8 100644 --- a/apps/sharing/appinfo/info.xml +++ b/apps/sharing/appinfo/info.xml @@ -7,8 +7,8 @@ xsi:noNamespaceSchemaLocation="https://apps.nextcloud.com/schema/apps/info.xsd"> sharing Sharing - TODO - TODO + This app provides APIs and occ commands to manage shares. + This app provides APIs and occ commands to manage shares. 2.0.0-dev.3 AGPL-3.0-or-later Kate Döen diff --git a/apps/sharing/lib/Command/AddShareRecipient.php b/apps/sharing/lib/Command/AddShareRecipient.php index 5742b0816087b..e45a347943a1d 100644 --- a/apps/sharing/lib/Command/AddShareRecipient.php +++ b/apps/sharing/lib/Command/AddShareRecipient.php @@ -16,7 +16,6 @@ use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; -// TODO: Initiator missing. final class AddShareRecipient extends SharingBase { #[\Override] public function configure(): void { diff --git a/apps/sharing/openapi.json b/apps/sharing/openapi.json index 1073b158e7ac6..0a70734e06617 100644 --- a/apps/sharing/openapi.json +++ b/apps/sharing/openapi.json @@ -3,7 +3,7 @@ "info": { "title": "sharing", "version": "0.0.1", - "description": "TODO", + "description": "This app provides APIs and occ commands to manage shares.", "license": { "name": "AGPL-3.0-or-later" } diff --git a/lib/private/Sharing/SharingManager.php b/lib/private/Sharing/SharingManager.php index 4b035e55741f2..e44446cf3f4e4 100644 --- a/lib/private/Sharing/SharingManager.php +++ b/lib/private/Sharing/SharingManager.php @@ -50,8 +50,6 @@ use Random\Randomizer; use RuntimeException; -// TODO: Add accept/reject -// TODO: Add permission masking (reshares) // TODO: Test sharing to federated users, groups and circles // TODO: Implement share transfers // TODO: Cache share owner diff --git a/lib/private/Sharing/SharingRegistry.php b/lib/private/Sharing/SharingRegistry.php index d22e447f2c69a..ad37ebaea483d 100644 --- a/lib/private/Sharing/SharingRegistry.php +++ b/lib/private/Sharing/SharingRegistry.php @@ -17,7 +17,6 @@ use NCU\Sharing\Source\IShareSourceType; use RuntimeException; -// TODO: Maybe add validate method to run all checks before using the manager final class SharingRegistry implements ISharingRegistry { private ?ISharingLegacyBackend $legacyBackend = null; From 1d1fe5d473658cb6c6d75a6e9a8528b0b9d77a56 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Wed, 2 Sep 2026 14:54:19 +0200 Subject: [PATCH 03/13] test(Sharing): Only fail test due to open transaction after teardown is done Signed-off-by: provokateurin --- tests/lib/Sharing/AbstractSharingManagerTests.php | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/tests/lib/Sharing/AbstractSharingManagerTests.php b/tests/lib/Sharing/AbstractSharingManagerTests.php index 3b96003476921..349de677d0bfd 100644 --- a/tests/lib/Sharing/AbstractSharingManagerTests.php +++ b/tests/lib/Sharing/AbstractSharingManagerTests.php @@ -244,9 +244,10 @@ public function setUp(): void { #[\Override] protected function tearDown(): void { + $openTransaction = false; if ($this->dbConnection->inTransaction()) { $this->dbConnection->rollBack(); - $this->fail('Open transaction was not committed.'); + $openTransaction = true; } $accessContext = new ShareAccessContext(overrideChecks: true); @@ -257,11 +258,13 @@ protected function tearDown(): void { $this->manager->deleteShare($accessContext, $share); } + $this->dbConnection->commit(); + $this->owner->delete(); $this->user1->delete(); $this->user2->delete(); - $this->dbConnection->commit(); + $this->registry->clear(); foreach ([ 'sharing_share', @@ -279,9 +282,11 @@ protected function tearDown(): void { $this->assertEquals(0, $qb->executeQuery()->fetchOne(), $table); } - $this->registry->clear(); - parent::tearDown(); + + if ($openTransaction) { + $this->fail('Open transaction was not committed.'); + } } private function reloadShare(ShareAccessContext $accessContext, Share $share): Share { From 8d7b8012ebe968a8a89ec0eede639d24e33873d4 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Wed, 2 Sep 2026 14:10:16 +0200 Subject: [PATCH 04/13] chore(Sharing): Remove ISharingLegacyBackend Signed-off-by: provokateurin --- lib/composer/composer/autoload_classmap.php | 1 - lib/composer/composer/autoload_static.php | 1 - lib/private/Sharing/ISharingLegacyBackend.php | 59 ------------------- lib/private/Sharing/SharingManager.php | 35 +---------- lib/private/Sharing/SharingRegistry.php | 17 ------ lib/unstable/Sharing/ISharingRegistry.php | 11 ---- 6 files changed, 1 insertion(+), 123 deletions(-) delete mode 100644 lib/private/Sharing/ISharingLegacyBackend.php diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 255465c0ffb2e..01c8fc36d96b3 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -2366,7 +2366,6 @@ 'OC\\Share20\\UserRemovedListener' => $baseDir . '/lib/private/Share20/UserRemovedListener.php', 'OC\\Share\\Constants' => $baseDir . '/lib/private/Share/Constants.php', 'OC\\Sharing\\ClassMapper' => $baseDir . '/lib/private/Sharing/ClassMapper.php', - 'OC\\Sharing\\ISharingLegacyBackend' => $baseDir . '/lib/private/Sharing/ISharingLegacyBackend.php', 'OC\\Sharing\\SharingBackend' => $baseDir . '/lib/private/Sharing/SharingBackend.php', 'OC\\Sharing\\SharingManager' => $baseDir . '/lib/private/Sharing/SharingManager.php', 'OC\\Sharing\\SharingRegistry' => $baseDir . '/lib/private/Sharing/SharingRegistry.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 046d2cd20b693..0687a45a56211 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -2407,7 +2407,6 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Share20\\UserRemovedListener' => __DIR__ . '/../../..' . '/lib/private/Share20/UserRemovedListener.php', 'OC\\Share\\Constants' => __DIR__ . '/../../..' . '/lib/private/Share/Constants.php', 'OC\\Sharing\\ClassMapper' => __DIR__ . '/../../..' . '/lib/private/Sharing/ClassMapper.php', - 'OC\\Sharing\\ISharingLegacyBackend' => __DIR__ . '/../../..' . '/lib/private/Sharing/ISharingLegacyBackend.php', 'OC\\Sharing\\SharingBackend' => __DIR__ . '/../../..' . '/lib/private/Sharing/SharingBackend.php', 'OC\\Sharing\\SharingManager' => __DIR__ . '/../../..' . '/lib/private/Sharing/SharingManager.php', 'OC\\Sharing\\SharingRegistry' => __DIR__ . '/../../..' . '/lib/private/Sharing/SharingRegistry.php', diff --git a/lib/private/Sharing/ISharingLegacyBackend.php b/lib/private/Sharing/ISharingLegacyBackend.php deleted file mode 100644 index 5aa9f45271e75..0000000000000 --- a/lib/private/Sharing/ISharingLegacyBackend.php +++ /dev/null @@ -1,59 +0,0 @@ -> - */ - public function getCompatibleSourceTypes(): array; - - /** - * @return list> - */ - public function getCompatibleRecipientTypes(): array; - - /** - * Update a share. - */ - public function updateShare(Share $share): void; - - /** - * Delete a share. - * - * @throws ShareNotFoundException - */ - public function deleteShare(string $id): void; - - /** - * Get a share. - * - * @throws ShareNotFoundException - */ - public function getShare(ShareAccessContext $accessContext, string $id): Share; - - /** - * Get multiple shares. - * - * @param ?class-string $filterSourceTypeClass - * @param ?positive-int $limit - * @return list - */ - public function getShares(ShareAccessContext $accessContext, ?string $filterSourceTypeClass, ?string $filterSourceTypeValue, ?string $lastShareID, ?int $limit): array; -} diff --git a/lib/private/Sharing/SharingManager.php b/lib/private/Sharing/SharingManager.php index e44446cf3f4e4..0190d1408864a 100644 --- a/lib/private/Sharing/SharingManager.php +++ b/lib/private/Sharing/SharingManager.php @@ -186,14 +186,7 @@ public function onOwnerDeleted(ShareAccessContext $accessContext, ShareUser $own // No need to update the last updated timestamp, because the share will be deleted anyway. - $ids = $this->backend->onOwnerDeleted($owner); - - $legacyBackend = $this->registry->getLegacyBackend(); - if ($legacyBackend instanceof ISharingLegacyBackend) { - foreach ($ids as $id) { - $legacyBackend->deleteShare($id); - } - } + $this->backend->onOwnerDeleted($owner); } #[\Override] @@ -757,11 +750,6 @@ public function deleteShare(ShareAccessContext $accessContext, Share $share): vo $this->validateShareEditPermissions($accessContext, $share); $this->backend->deleteShare($share->id); - - $legacyBackend = $this->registry->getLegacyBackend(); - if ($legacyBackend instanceof ISharingLegacyBackend) { - $legacyBackend->deleteShare($share->id); - } } #[\Override] @@ -986,27 +974,6 @@ private function processShareUpdates(array $shares): array { ); } } - - $legacyBackend = $this->registry->getLegacyBackend(); - if ($legacyBackend instanceof ISharingLegacyBackend) { - $compatibleSourceTypes = array_fill_keys($legacyBackend->getCompatibleSourceTypes(), true); - foreach ($share->sources as $source) { - if (!isset($compatibleSourceTypes[$source->class])) { - throw new RuntimeException('The legacy backend ' . $legacyBackend::class . ' does not support this source type: ' . $source->class); - } - } - - $compatibleRecipientTypes = array_fill_keys($legacyBackend->getCompatibleRecipientTypes(), true); - foreach ($share->recipients as $recipient) { - if (!isset($compatibleRecipientTypes[$recipient->class])) { - throw new RuntimeException( - 'The legacy backend ' . $legacyBackend::class . ' does not support this recipient type: ' . $recipient->class - ); - } - } - - $legacyBackend->updateShare($share); - } } return $shares; diff --git a/lib/private/Sharing/SharingRegistry.php b/lib/private/Sharing/SharingRegistry.php index ad37ebaea483d..d2c15b5ce9150 100644 --- a/lib/private/Sharing/SharingRegistry.php +++ b/lib/private/Sharing/SharingRegistry.php @@ -18,8 +18,6 @@ use RuntimeException; final class SharingRegistry implements ISharingRegistry { - private ?ISharingLegacyBackend $legacyBackend = null; - /** @var array, IShareSourceType> */ private array $sourceTypes = []; @@ -58,7 +56,6 @@ final class SharingRegistry implements ISharingRegistry { #[\Override] public function clear(): void { - $this->legacyBackend = null; $this->sourceTypes = []; $this->recipientTypes = []; $this->propertyTypes = []; @@ -73,20 +70,6 @@ public function clear(): void { $this->permissionPresetCompatiblePermissionTypes = []; } - #[\Override] - public function registerLegacyBackend(ISharingLegacyBackend $legacyBackend): void { - if ($this->legacyBackend instanceof ISharingLegacyBackend) { - throw new RuntimeException('A sharing legacy backend is already registered'); - } - - $this->legacyBackend = $legacyBackend; - } - - #[\Override] - public function getLegacyBackend(): ?ISharingLegacyBackend { - return $this->legacyBackend; - } - #[\Override] public function registerSourceType(IShareSourceType $sourceType): void { $class = $sourceType::class; diff --git a/lib/unstable/Sharing/ISharingRegistry.php b/lib/unstable/Sharing/ISharingRegistry.php index 7e56c194f00c1..77bb566bdc59f 100644 --- a/lib/unstable/Sharing/ISharingRegistry.php +++ b/lib/unstable/Sharing/ISharingRegistry.php @@ -14,7 +14,6 @@ use NCU\Sharing\Property\ISharePropertyType; use NCU\Sharing\Recipient\IShareRecipientType; use NCU\Sharing\Source\IShareSourceType; -use OC\Sharing\ISharingLegacyBackend; use OCP\AppFramework\Attribute\Consumable; /** @@ -27,16 +26,6 @@ interface ISharingRegistry { */ public function clear(): void; - /** - * @experimental 35.0.0 - */ - public function registerLegacyBackend(ISharingLegacyBackend $legacyBackend): void; - - /** - * @experimental 35.0.0 - */ - public function getLegacyBackend(): ?ISharingLegacyBackend; - /** * @experimental 35.0.0 */ From 76251efa80021a7f552e7e20ac3d8f2b3c9bec0e Mon Sep 17 00:00:00 2001 From: provokateurin Date: Wed, 23 Sep 2026 14:33:41 +0200 Subject: [PATCH 05/13] fix(DefaultShareProvider): Use existing share status if available Signed-off-by: provokateurin --- lib/private/Share20/DefaultShareProvider.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index c7a47188f587b..b61db90e8f975 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -110,7 +110,10 @@ public function create(IShare $share) { if ($share->getShareType() === IShare::TYPE_USER) { //Set the UID of the user we share with $qb->setValue('share_with', $qb->createNamedParameter($share->getSharedWith())); - $qb->setValue('accepted', $qb->createNamedParameter(IShare::STATUS_PENDING)); + if ($share->getStatus() === null) { + $share->setStatus(IShare::STATUS_PENDING); + } + $qb->setValue('accepted', $qb->createNamedParameter($share->getStatus())); //If an expiration date is set store it if ($expirationDate !== null) { From 20ba55b1ee4e7ecba888dd27f0985cd91a171b61 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Thu, 3 Sep 2026 15:51:49 +0200 Subject: [PATCH 06/13] fix(DefaultShareProvider): Use existing share time if available Signed-off-by: provokateurin --- lib/private/Share20/DefaultShareProvider.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index b61db90e8f975..e9e26276083a2 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -188,9 +188,9 @@ public function create(IShare $share) { $qb->setValue('note', $qb->createNamedParameter($share->getNote())); } - // Set the time this share was created - $shareTime = $this->timeFactory->now(); + $shareTime = $share->getShareTime() ?? \DateTime::createFromImmutable($this->timeFactory->now()); $qb->setValue('stime', $qb->createNamedParameter($shareTime->getTimestamp())); + $share->setShareTime($shareTime); // insert the data and fetch the id of the share $qb->executeStatement(); @@ -200,8 +200,6 @@ public function create(IShare $share) { $share->setId((string)$id); $share->setProviderId($this->identifier()); - $share->setShareTime(\DateTime::createFromImmutable($shareTime)); - $mailSendValue = $share->getMailSend(); $share->setMailSend(($mailSendValue === null) ? true : $mailSendValue); From 898ca1c783bd7265146ff3c7f6a635493a8e2c16 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Wed, 23 Sep 2026 14:32:02 +0200 Subject: [PATCH 07/13] fix(ShareByMailProvider): Use existing share time if available Signed-off-by: provokateurin --- apps/sharebymail/lib/ShareByMailProvider.php | 5 ++++- apps/sharebymail/tests/ShareByMailProviderTest.php | 3 ++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/apps/sharebymail/lib/ShareByMailProvider.php b/apps/sharebymail/lib/ShareByMailProvider.php index b96a61288517f..e0c918e463721 100644 --- a/apps/sharebymail/lib/ShareByMailProvider.php +++ b/apps/sharebymail/lib/ShareByMailProvider.php @@ -7,6 +7,7 @@ namespace OCA\ShareByMail; +use DateTime; use OC\Share20\DefaultShareProvider; use OC\Share20\Exception\InvalidShare; use OC\Share20\Share; @@ -241,6 +242,7 @@ protected function createMailShare(IShare $share): string { $share->getHideDownload(), $share->getLabel(), $share->getExpirationDate(), + $share->getShareTime(), $share->getNote(), $share->getAttributes(), $share->getMailSend(), @@ -699,6 +701,7 @@ protected function addShareToDB( ?bool $hideDownload, ?string $label, ?\DateTimeInterface $expirationTime, + ?DateTime $shareTime, ?string $note = '', ?IAttributes $attributes = null, ?bool $mailSend = true, @@ -717,7 +720,7 @@ protected function addShareToDB( ->setValue('password', $qb->createNamedParameter($password)) ->setValue('password_expiration_time', $qb->createNamedParameter($passwordExpirationTime, IQueryBuilder::PARAM_DATETIME_MUTABLE)) ->setValue('password_by_talk', $qb->createNamedParameter($sendPasswordByTalk, IQueryBuilder::PARAM_BOOL)) - ->setValue('stime', $qb->createNamedParameter(time())) + ->setValue('stime', $qb->createNamedParameter($shareTime?->getTimestamp() ?? time())) ->setValue('hide_download', $qb->createNamedParameter((int)$hideDownload, IQueryBuilder::PARAM_INT)) ->setValue('label', $qb->createNamedParameter($label)) ->setValue('note', $qb->createNamedParameter($note)) diff --git a/apps/sharebymail/tests/ShareByMailProviderTest.php b/apps/sharebymail/tests/ShareByMailProviderTest.php index c87748f6657ce..1f177f32f68e1 100644 --- a/apps/sharebymail/tests/ShareByMailProviderTest.php +++ b/apps/sharebymail/tests/ShareByMailProviderTest.php @@ -777,7 +777,8 @@ public function testAddShareToDB(): void { $sendPasswordByTalk, $hideDownload, $label, - $expiration + $expiration, + null, ] ); From 8266e5aced7313835661e06adad25b44b664c20d Mon Sep 17 00:00:00 2001 From: provokateurin Date: Wed, 23 Sep 2026 14:37:46 +0200 Subject: [PATCH 08/13] fix(OC\Share20\Manager): Use existing share target if available Signed-off-by: provokateurin --- lib/private/Share20/Manager.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index e0f1a97a70542..92a2f1352fd3b 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -588,9 +588,11 @@ public function createShare(IShare $share): IShare { } } - $target = $shareFolder . '/' . $share->getNode()->getName(); - $target = Filesystem::normalizePath($target); - $share->setTarget($target); + if ($share->getTarget() === null) { + $target = $shareFolder . '/' . $share->getNode()->getName(); + $target = Filesystem::normalizePath($target); + $share->setTarget($target); + } // Pre share event $event = new BeforeShareCreatedEvent($share); From b0dd753818d550ec87353723e66a53cd891abd44 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Thu, 10 Sep 2026 11:07:32 +0200 Subject: [PATCH 09/13] refactor(IGroup): Set correct return type for getUsers() Signed-off-by: provokateurin --- lib/private/Group/Group.php | 5 ----- lib/public/IGroup.php | 2 +- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/lib/private/Group/Group.php b/lib/private/Group/Group.php index 7499db4046f07..a27c4ef89c536 100644 --- a/lib/private/Group/Group.php +++ b/lib/private/Group/Group.php @@ -98,11 +98,6 @@ public function setDisplayName(string $displayName): bool { return false; } - /** - * get all users in the group - * - * @return array - */ #[\Override] public function getUsers(): array { if ($this->usersLoaded) { diff --git a/lib/public/IGroup.php b/lib/public/IGroup.php index 448b9f4c8f8ce..33da553d1ee38 100644 --- a/lib/public/IGroup.php +++ b/lib/public/IGroup.php @@ -41,7 +41,7 @@ public function setDisplayName(string $displayName): bool; /** * get all users in the group * - * @return IUser[] + * @return array * @since 8.0.0 */ public function getUsers(): array; From 40f53781b17b12d3f8ba3e6dc8762776c2356ba1 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Sun, 20 Sep 2026 15:36:10 +0200 Subject: [PATCH 10/13] feat(ISnowflakeGenerator): Allow passing a timestamp to nextId() Signed-off-by: provokateurin --- lib/private/Snowflake/SnowflakeGenerator.php | 16 ++++++---------- lib/public/Snowflake/ISnowflakeGenerator.php | 4 +++- tests/lib/Snowflake/GeneratorTest.php | 12 ++++++++++++ 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/lib/private/Snowflake/SnowflakeGenerator.php b/lib/private/Snowflake/SnowflakeGenerator.php index 6e07bd72d6f61..a73253d246c7f 100644 --- a/lib/private/Snowflake/SnowflakeGenerator.php +++ b/lib/private/Snowflake/SnowflakeGenerator.php @@ -9,6 +9,7 @@ namespace OC\Snowflake; +use DateTimeImmutable; use OCP\AppFramework\Utility\ITimeFactory; use OCP\IServerInfo; use OCP\Snowflake\ISnowflakeGenerator; @@ -31,9 +32,12 @@ public function __construct( } #[Override] - public function nextId(): string { + public function nextId(?DateTimeImmutable $timestamp = null): string { + $timestamp ??= $this->timeFactory->now(); + // Relative time - [$seconds, $milliseconds] = $this->getCurrentTime(); + $seconds = $timestamp->getTimestamp() - self::TS_OFFSET; + $milliseconds = (int)$timestamp->format('v'); $serverId = $this->serverInfo->getServerId(); $isCli = (int)$this->isCli(); // 1 bit @@ -124,14 +128,6 @@ private function convertToDecimal(array $bytes): string { return $digits; } - private function getCurrentTime(): array { - $time = $this->timeFactory->now(); - return [ - $time->getTimestamp() - self::TS_OFFSET, - (int)$time->format('v'), - ]; - } - private function isCli(): bool { return PHP_SAPI === 'cli'; } diff --git a/lib/public/Snowflake/ISnowflakeGenerator.php b/lib/public/Snowflake/ISnowflakeGenerator.php index ee9a02bc486fa..475d376788313 100644 --- a/lib/public/Snowflake/ISnowflakeGenerator.php +++ b/lib/public/Snowflake/ISnowflakeGenerator.php @@ -9,6 +9,7 @@ namespace OCP\Snowflake; +use DateTimeImmutable; use OCP\AppFramework\Attribute\Consumable; /** @@ -39,11 +40,12 @@ interface ISnowflakeGenerator { * * Each call to this method is guaranteed to return a different ID. * + * @param ?DateTimeImmutable $timestamp Generate the Snowflake ID for a specific time. This should only be used in very special cases. * @return non-empty-string * * @since 33.0 */ - public function nextId(): string; + public function nextId(?DateTimeImmutable $timestamp = null): string; /** * Return the smallest possible Snowflake ID for a given timestamp diff --git a/tests/lib/Snowflake/GeneratorTest.php b/tests/lib/Snowflake/GeneratorTest.php index 3323773105a0d..a3d60f7b93efe 100644 --- a/tests/lib/Snowflake/GeneratorTest.php +++ b/tests/lib/Snowflake/GeneratorTest.php @@ -99,6 +99,18 @@ public function testGeneratorWithFixedTime(string $date, int $expectedSeconds, i $this->assertEquals($this->serverInfo->getServerId(), $data->getServerId()); } + #[DataProvider('provideSnowflakeData')] + public function testGeneratorWithTimestampParameter(string $date, int $expectedSeconds, int $expectedMilliseconds): void { + $dt = new \DateTimeImmutable($date); + + $generator = new SnowflakeGenerator(new TimeFactory(), $this->sequence, $this->serverInfo); + $data = $this->decoder->decode($generator->nextId($dt)); + + $this->assertEquals($expectedSeconds, $data->getCreatedAt()->format('U') - ISnowflakeGenerator::TS_OFFSET); + $this->assertEquals($expectedMilliseconds, (int)$data->getCreatedAt()->format('v')); + $this->assertEquals($this->serverInfo->getServerId(), $data->getServerId()); + } + public static function provideSnowflakeData(): array { $tests = [ ['2025-10-01 00:00:00.000000', 0, 0], From 7d34160066b1d13b835d40c6e1a0f8a086f2941b Mon Sep 17 00:00:00 2001 From: provokateurin Date: Mon, 21 Sep 2026 15:29:07 +0200 Subject: [PATCH 11/13] feat(IShare): Add setPasswordHash() and isPasswordHashed() Signed-off-by: provokateurin --- lib/private/Share20/Share.php | 19 +++++++++++++++++++ lib/public/Share/IShare.php | 15 +++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/lib/private/Share20/Share.php b/lib/private/Share20/Share.php index 3ea440a885a68..35c70a1464bfc 100644 --- a/lib/private/Share20/Share.php +++ b/lib/private/Share20/Share.php @@ -16,6 +16,7 @@ use OCP\Files\Node; use OCP\Files\NotFoundException; use OCP\IUserManager; +use OCP\Security\IHasher; use OCP\Server; use OCP\Share\Exceptions\IllegalIDChangeException; use OCP\Share\IAttributes; @@ -57,6 +58,7 @@ class Share implements IShare { private $expireDate; /** @var string */ private $password; + private bool $isPasswordHashed = false; private ?\DateTimeInterface $passwordExpirationTime = null; /** @var bool */ private $sendPasswordByTalk = false; @@ -501,6 +503,18 @@ public function getShareOwner() { #[\Override] public function setPassword($password) { $this->password = $password; + $this->isPasswordHashed = false; + return $this; + } + + #[\Override] + public function setPasswordHash(string $passwordHash): IShare { + if (!Server::get(IHasher::class)->validate($passwordHash)) { + throw new \InvalidArgumentException(); + } + + $this->password = $passwordHash; + $this->isPasswordHashed = true; return $this; } @@ -512,6 +526,11 @@ public function getPassword() { return $this->password; } + #[\Override] + public function isPasswordHashed(): bool { + return $this->isPasswordHashed; + } + /** * @inheritdoc */ diff --git a/lib/public/Share/IShare.php b/lib/public/Share/IShare.php index 5e5647f52aa10..6c92f30ef0ccd 100644 --- a/lib/public/Share/IShare.php +++ b/lib/public/Share/IShare.php @@ -453,6 +453,14 @@ public function getShareOwner(); */ public function setPassword($password); + /** + * Sets the password for the shared, but in it's already hashed form. + * Use {@see isPasswordHashed} to check if the return value of {@see getPassword} is already hashed. + * + * @since 35.0.0 + */ + public function setPasswordHash(string $passwordHash): IShare; + /** * Get the password of this share. * If this share is obtained via a shareprovider the password is @@ -463,6 +471,13 @@ public function setPassword($password); */ public function getPassword(); + /** + * Returns whether the return value of {@see getPassword} is already hashed. + * + * @since 35.0.0 + */ + public function isPasswordHashed(): bool; + /** * Returns whether the share is password protected by any means (e.g. password or OTP) * @return bool From ee1feb833bfca9f3d89bd924b69f154f19410749 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Mon, 21 Sep 2026 15:27:41 +0200 Subject: [PATCH 12/13] refactor: Cleanup share password hash handling Signed-off-by: provokateurin --- apps/sharebymail/lib/ShareByMailProvider.php | 27 ++- .../tests/ShareByMailProviderTest.php | 53 +++--- lib/private/Security/Hasher.php | 20 +- lib/private/Share20/DefaultShareProvider.php | 19 +- lib/private/Share20/Manager.php | 21 ++- .../lib/Share20/DefaultShareProviderTest.php | 42 +++-- tests/lib/Share20/ManagerTest.php | 174 +++++++++--------- 7 files changed, 204 insertions(+), 152 deletions(-) diff --git a/apps/sharebymail/lib/ShareByMailProvider.php b/apps/sharebymail/lib/ShareByMailProvider.php index e0c918e463721..3a41fc790fb98 100644 --- a/apps/sharebymail/lib/ShareByMailProvider.php +++ b/apps/sharebymail/lib/ShareByMailProvider.php @@ -41,6 +41,7 @@ use OCP\User\Exceptions\UserNotFoundException; use OCP\Util; use Psr\Log\LoggerInterface; +use RuntimeException; /** * Class ShareByMail @@ -99,14 +100,10 @@ public function create(IShare $share): IShare { // if the admin enforces a password for all mail shares we create a // random password and send it to the recipient - $password = $share->getPassword() ?: ''; - $passwordEnforced = $this->shareManager->shareApiLinkEnforcePassword(); - if ($passwordEnforced && empty($password)) { + $password = $share->getPassword(); + if ($password === null && $this->shareManager->shareApiLinkEnforcePassword()) { $password = $this->autoGeneratePassword($share); - } - - if (!empty($password)) { - $share->setPassword($this->hasher->hash($password)); + $share->setPasswordHash($this->hasher->hash($password)); } $shareId = $this->createMailShare($share); @@ -117,7 +114,7 @@ public function create(IShare $share): IShare { // Temporary set the clear password again to send it by mail // This need to be done after the share was created in the database // as the password is hashed in between. - if (!empty($password)) { + if ($password !== null) { $data['password'] = $password; } @@ -228,6 +225,11 @@ protected function createMailShare(IShare $share): string { if ($share->getToken() === '') { $share->setToken($this->generateToken()); } + + if ($share->getPassword() !== null && !$share->isPasswordHashed()) { + throw new RuntimeException('The password must be hashed already.'); + } + return $this->addShareToDB( $share->getNodeId(), $share->getNodeType(), @@ -770,6 +772,11 @@ public function update(IShare $share, ?string $plainTextPassword = null): IShare $expiration = \DateTime::createFromInterface($expiration); $expiration->setTimezone(new \DateTimeZone(date_default_timezone_get())); } + + if ($share->getPassword() !== null && !$share->isPasswordHashed()) { + throw new RuntimeException('The password must be hashed already.'); + } + $qb->update('share') ->where($qb->expr()->eq('id', $qb->createNamedParameter($share->getId()))) ->set('item_source', $qb->createNamedParameter($share->getNodeId())) @@ -1055,7 +1062,9 @@ protected function createShareObject(array $data): IShare { $shareTime->setTimestamp((int)$data['stime']); $share->setShareTime($shareTime); $share->setSharedWith($data['share_with'] ?? ''); - $share->setPassword($data['password']); + if (($password = $data['password']) !== null) { + $share->setPasswordHash($password); + } $passwordExpirationTime = \DateTime::createFromFormat('Y-m-d H:i:s', $data['password_expiration_time'] ?? ''); $share->setPasswordExpirationTime($passwordExpirationTime !== false ? $passwordExpirationTime : null); $share->setLabel($data['label'] ?? ''); diff --git a/apps/sharebymail/tests/ShareByMailProviderTest.php b/apps/sharebymail/tests/ShareByMailProviderTest.php index 1f177f32f68e1..59971ce925324 100644 --- a/apps/sharebymail/tests/ShareByMailProviderTest.php +++ b/apps/sharebymail/tests/ShareByMailProviderTest.php @@ -59,7 +59,6 @@ class ShareByMailProviderTest extends TestCase { private IShare&MockObject $share; private IConfig&MockObject $config; private IMailer&MockObject $mailer; - private IHasher&MockObject $hasher; private Defaults&MockObject $defaults; private IManager&MockObject $shareManager; private LoggerInterface&MockObject $logger; @@ -92,7 +91,6 @@ protected function setUp(): void { $this->activityManager = $this->createMock('OCP\Activity\IManager'); $this->settingsManager = $this->createMock(SettingsManager::class); $this->defaults = $this->createMock(Defaults::class); - $this->hasher = $this->createMock(IHasher::class); $this->eventDispatcher = $this->createMock(IEventDispatcher::class); $this->shareManager = $this->createMock(IManager::class); @@ -122,7 +120,7 @@ private function getInstance(array $mockedMethods = []) { $this->activityManager, $this->settingsManager, $this->defaults, - $this->hasher, + Server::get(IHasher::class), $this->eventDispatcher, $this->shareManager, $this->getEmailValidatorWithStrictEmailCheck(), @@ -144,7 +142,7 @@ private function getInstance(array $mockedMethods = []) { $this->activityManager, $this->settingsManager, $this->defaults, - $this->hasher, + Server::get(IHasher::class), $this->eventDispatcher, $this->shareManager, $this->getEmailValidatorWithStrictEmailCheck(), @@ -253,8 +251,8 @@ public function testCreateSendPasswordByMailWithPasswordAndWithoutEnforcedPasswo $share->expects($this->any())->method('getNode')->willReturn($node); $share->expects($this->any())->method('getPassword')->willReturn('password'); - $this->hasher->expects($this->once())->method('hash')->with('password')->willReturn('passwordHashed'); - $share->expects($this->once())->method('setPassword')->with('passwordHashed'); + $share->expects($this->never())->method('setPassword'); + $share->expects($this->never())->method('setPasswordHash'); // The given password (but not the autogenerated password) should not be // mailed to the receiver of the share because permanent passwords are not enforced. @@ -300,8 +298,8 @@ public function testCreateSendPasswordByMailWithPasswordAndWithoutEnforcedPasswo $share->expects($this->any())->method('getNode')->willReturn($node); $share->expects($this->any())->method('getPassword')->willReturn('password'); - $this->hasher->expects($this->once())->method('hash')->with('password')->willReturn('passwordHashed'); - $share->expects($this->once())->method('setPassword')->with('passwordHashed'); + $share->expects($this->never())->method('setPassword'); + $share->expects($this->never())->method('setPasswordHash'); // No password is generated, so no emails need to be sent // aside from the main email notification. @@ -336,8 +334,8 @@ public function testCreateSendPasswordToOwnerWhenSendPasswordByMailIsDisabled(): $share->method('getPassword')->willReturn('password'); $this->mailer->method('validateMailAddress')->willReturn(true); - $this->hasher->expects($this->once())->method('hash')->with('password')->willReturn('passwordHashed'); - $share->expects($this->once())->method('setPassword')->with('passwordHashed'); + $share->expects($this->never())->method('setPassword'); + $share->expects($this->never())->method('setPasswordHash'); $instance = $this->getInstance([ 'getSharedWith', 'createMailShare', 'getRawShare', 'createShareObject', @@ -399,8 +397,8 @@ public function testCreateSendPasswordByMailWithEnforcedPasswordProtectionWithPe // Initially not set, but will be set by the autoGeneratePassword method. $share->expects($this->exactly(3))->method('getPassword')->willReturnOnConsecutiveCalls(null, 'autogeneratedPassword', 'autogeneratedPassword'); - $this->hasher->expects($this->once())->method('hash')->with('autogeneratedPassword')->willReturn('autogeneratedPasswordHashed'); - $share->expects($this->once())->method('setPassword')->with('autogeneratedPasswordHashed'); + $share->expects($this->never())->method('setPassword'); + $share->expects($this->once())->method('setPasswordHash')->with($this->callback(fn (string $hash): bool => Server::get(IHasher::class)->verify('autogeneratedPassword', $hash))); // The autogenerated password should be mailed to the receiver of the share because permanent passwords are enforced. $this->shareManager->expects($this->any())->method('shareApiLinkEnforcePassword')->willReturn(true); @@ -478,8 +476,8 @@ public function testCreateSendPasswordByMailWithPasswordAndWithEnforcedPasswordP $instance->expects($this->once())->method('createShareObject')->with(['rawShare', 'password' => 'password'])->willReturn($expectedShare); $share->expects($this->exactly(3))->method('getPassword')->willReturn('password'); - $this->hasher->expects($this->once())->method('hash')->with('password')->willReturn('passwordHashed'); - $share->expects($this->once())->method('setPassword')->with('passwordHashed'); + $share->expects($this->never())->method('setPassword'); + $share->expects($this->never())->method('setPasswordHash'); // The given password (but not the autogenerated password) should be // mailed to the receiver of the share. @@ -566,8 +564,8 @@ public function testCreateSendPasswordByTalkWithEnforcedPasswordProtectionWithPe $instance->expects($this->once())->method('createShareObject')->with(['rawShare', 'password' => 'autogeneratedPassword'])->willReturn($expectedShare); $share->expects($this->exactly(3))->method('getPassword')->willReturnOnConsecutiveCalls(null, 'autogeneratedPassword', 'autogeneratedPassword'); - $this->hasher->expects($this->once())->method('hash')->with('autogeneratedPassword')->willReturn('autogeneratedPasswordHashed'); - $share->expects($this->once())->method('setPassword')->with('autogeneratedPasswordHashed'); + $share->expects($this->never())->method('setPassword'); + $share->expects($this->once())->method('setPasswordHash')->with($this->callback(fn (string $hash): bool => Server::get(IHasher::class)->verify('autogeneratedPassword', $hash))); // The autogenerated password should be mailed to the owner of the share. $this->shareManager->expects($this->any())->method('shareApiLinkEnforcePassword')->willReturn(true); @@ -662,8 +660,8 @@ public function sendNotificationToMultipleEmails() { $share->expects($this->any())->method('getNode')->willReturn($node); $share->expects($this->any())->method('getPassword')->willReturn('password'); - $this->hasher->expects($this->once())->method('hash')->with('password')->willReturn('passwordHashed'); - $share->expects($this->once())->method('setPassword')->with('passwordHashed'); + $share->expects($this->never())->method('setPassword'); + $share->expects($this->never())->method('setPasswordHash'); // The given password (but not the autogenerated password) should not be // mailed to the receiver of the share because permanent passwords are not enforced. @@ -856,14 +854,18 @@ public function testUpdate(): void { } public static function dataUpdateSendPassword(): array { + $hasher = Server::get(IHasher::class); + $hash = $hasher->hash('password'); + $hashNew = $hasher->hash('password new'); + return [ - ['password', 'hashed', 'hashed new', false, false, true], - ['', 'hashed', 'hashed new', false, false, false], - [null, 'hashed', 'hashed new', false, false, false], - ['password', 'hashed', 'hashed', false, false, false], - ['password', 'hashed', 'hashed new', false, true, false], - ['password', 'hashed', 'hashed new', true, false, true], - ['password', 'hashed', 'hashed', true, false, true], + ['password', $hash, $hashNew, false, false, true], + ['', $hash, $hashNew, false, false, false], + [null, $hash, $hashNew, false, false, false], + ['password', $hash, $hash, false, false, false], + ['password', $hash, $hashNew, false, true, false], + ['password', $hash, $hashNew, true, false, true], + ['password', $hash, $hash, true, false, true], ]; } @@ -885,6 +887,7 @@ public function testUpdateSendPassword(?string $plainTextPassword, string $origi $share->expects($this->any())->method('getSharedWith')->willReturn('receiver@example.com'); $share->expects($this->any())->method('getNode')->willReturn($node); $share->expects($this->any())->method('getId')->willReturn('42'); + $share->expects($this->any())->method('isPasswordHashed')->willReturn(true); $share->expects($this->any())->method('getPassword')->willReturn($newPassword); $share->expects($this->any())->method('getSendPasswordByTalk')->willReturn($newSendPasswordByTalk); diff --git a/lib/private/Security/Hasher.php b/lib/private/Security/Hasher.php index 418c8b88a58b8..2bbcd2fa6fa22 100644 --- a/lib/private/Security/Hasher.php +++ b/lib/private/Security/Hasher.php @@ -36,6 +36,8 @@ class Hasher implements IHasher { private array $options = []; /** Salt used for legacy passwords */ private ?string $legacySalt = null; + /** Only used for testing */ + private ?string $forcedAlgorithm = null; public function __construct( private IConfig $config, @@ -72,15 +74,13 @@ public function __construct( public function hash(string $message): string { $alg = $this->getPrefferedAlgorithm(); - if (\defined('PASSWORD_ARGON2ID') && $alg === PASSWORD_ARGON2ID) { - return 3 . '|' . password_hash($message, PASSWORD_ARGON2ID, $this->options); - } - - if (\defined('PASSWORD_ARGON2I') && $alg === PASSWORD_ARGON2I) { - return 2 . '|' . password_hash($message, PASSWORD_ARGON2I, $this->options); - } + $version = match ($alg) { + PASSWORD_ARGON2ID => 3, + PASSWORD_ARGON2I => 2, + PASSWORD_BCRYPT => 1, + }; - return 1 . '|' . password_hash($message, PASSWORD_BCRYPT, $this->options); + return $version . '|' . password_hash($message, $alg, $this->options); } /** @@ -182,6 +182,10 @@ private function needsRehash(string $hash): bool { } private function getPrefferedAlgorithm(): string { + if ($this->forcedAlgorithm !== null) { + return $this->forcedAlgorithm; + } + $default = PASSWORD_BCRYPT; if (\defined('PASSWORD_ARGON2I')) { $default = PASSWORD_ARGON2I; diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index e9e26276083a2..0fff3fd0644bc 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -46,6 +46,7 @@ use OCP\Share\IShareProviderWithNotification; use OCP\Util; use Psr\Log\LoggerInterface; +use RuntimeException; use function str_starts_with; use function strlen; @@ -136,8 +137,11 @@ public function create(IShare $share) { $qb->setValue('token', $qb->createNamedParameter($share->getToken())); //If a password is set store it - if ($share->getPassword() !== null) { - $qb->setValue('password', $qb->createNamedParameter($share->getPassword())); + if (($password = $share->getPassword()) !== null) { + if (!$share->isPasswordHashed()) { + throw new RuntimeException('The password must be hashed already.'); + } + $qb->setValue('password', $qb->createNamedParameter($password)); } $qb->setValue('password_by_talk', $qb->createNamedParameter($share->getSendPasswordByTalk(), IQueryBuilder::PARAM_BOOL)); @@ -288,10 +292,15 @@ public function update(IShare $share) { ->set('attributes', $qb->createNamedParameter($shareAttributes)) ->executeStatement(); } elseif ($share->getShareType() === IShare::TYPE_LINK) { + $password = $share->getPassword(); + if ($password !== null && !$share->isPasswordHashed()) { + throw new RuntimeException('The password must be hashed already.'); + } + $qb = $this->dbConn->getQueryBuilder(); $qb->update('share') ->where($qb->expr()->eq('id', $qb->createNamedParameter($share->getId()))) - ->set('password', $qb->createNamedParameter($share->getPassword())) + ->set('password', $qb->createNamedParameter($password)) ->set('password_by_talk', $qb->createNamedParameter($share->getSendPasswordByTalk(), IQueryBuilder::PARAM_BOOL)) ->set('uid_owner', $qb->createNamedParameter($share->getShareOwner())) ->set('uid_initiator', $qb->createNamedParameter($share->getSharedBy())) @@ -1130,7 +1139,9 @@ private function createShare($data): IShare { $share->setSharedWith($data['share_with']); $share->setSharedWithDisplayNameCallback(fn (IShare $share) => $this->groupManager->getDisplayName($share->getSharedWith())); } elseif ($share->getShareType() === IShare::TYPE_LINK) { - $share->setPassword($data['password']); + if (($password = $data['password']) !== null) { + $share->setPasswordHash($password); + } $share->setSendPasswordByTalk((bool)$data['password_by_talk']); $share->setToken($data['token']); } diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 92a2f1352fd3b..a6dfcb66fbfe9 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -82,6 +82,7 @@ use OCP\Share\IShareProviderWithNotification; use Override; use Psr\Log\LoggerInterface; +use RuntimeException; /** * This class is the communication hub for all sharing related operations. @@ -567,9 +568,8 @@ public function createShare(IShare $share): IShare { $this->verifyPassword($share->getPassword()); // If a password is set. Hash it! - if ($share->getShareType() === IShare::TYPE_LINK - && $share->getPassword() !== null) { - $share->setPassword($this->hasher->hash($share->getPassword())); + if (($share->getShareType() === IShare::TYPE_LINK || $share->getShareType() === IShare::TYPE_EMAIL) && $share->getPassword() !== null && !$share->isPasswordHashed()) { + $share->setPasswordHash($this->hasher->hash($share->getPassword())); } } @@ -834,7 +834,7 @@ private function updateSharePasswordIfNeeded(IShare $share, IShare $originalShar // If a password is set. Hash it! if (!empty($share->getPassword())) { - $share->setPassword($this->hasher->hash($share->getPassword())); + $share->setPasswordHash($this->hasher->hash($share->getPassword())); if ($share->getShareType() === IShare::TYPE_EMAIL) { // Shares shared by email have temporary passwords $this->setSharePasswordExpirationTime($share); @@ -852,7 +852,12 @@ private function updateSharePasswordIfNeeded(IShare $share, IShare $originalShar } else { // Reset the password to the original one, as it is either the same // as the "new" password or a hashed version of it. - $share->setPassword($originalShare->getPassword()); + $password = $originalShare->getPassword(); + if ($password !== null && $originalShare->isPasswordHashed()) { + $share->setPasswordHash($password); + } else { + $share->setPassword($password); + } } return false; @@ -1475,13 +1480,17 @@ public function checkPassword(IShare $share, ?string $password): bool { return false; } + if (!$share->isPasswordHashed()) { + throw new RuntimeException('The password must be hashed already.'); + } + $newHash = ''; if (!$this->hasher->verify($password, $share->getPassword(), $newHash)) { return false; } if (!empty($newHash)) { - $share->setPassword($newHash); + $share->setPasswordHash($newHash); $provider = $this->factory->getProviderForType($share->getShareType()); $provider->update($share); } diff --git a/tests/lib/Share20/DefaultShareProviderTest.php b/tests/lib/Share20/DefaultShareProviderTest.php index c10542a1433ed..65cd34652f322 100644 --- a/tests/lib/Share20/DefaultShareProviderTest.php +++ b/tests/lib/Share20/DefaultShareProviderTest.php @@ -31,6 +31,7 @@ use OCP\IUserManager; use OCP\L10N\IFactory; use OCP\Mail\IMailer; +use OCP\Security\IHasher; use OCP\Server; use OCP\Share\Exceptions\ShareNotFound; use OCP\Share\IManager as IShareManager; @@ -407,12 +408,14 @@ public function testGetShareByIdUserGroupShare(): void { } public function testGetShareByIdLinkShare(): void { + $passwordHash = Server::get(IHasher::class)->hash('password'); + $qb = $this->dbConn->getQueryBuilder(); $qb->insert('share') ->values([ 'share_type' => $qb->expr()->literal(IShare::TYPE_LINK), - 'password' => $qb->expr()->literal('password'), + 'password' => $qb->expr()->literal($passwordHash), 'password_by_talk' => $qb->expr()->literal(true), 'uid_owner' => $qb->expr()->literal('shareOwner'), 'uid_initiator' => $qb->expr()->literal('sharedBy'), @@ -442,7 +445,8 @@ public function testGetShareByIdLinkShare(): void { $this->assertEquals($id, $share->getId()); $this->assertEquals(IShare::TYPE_LINK, $share->getShareType()); $this->assertNull($share->getSharedWith()); - $this->assertEquals('password', $share->getPassword()); + $this->assertTrue($share->isPasswordHashed()); + $this->assertEquals($passwordHash, $share->getPassword()); $this->assertEquals(true, $share->getSendPasswordByTalk()); $this->assertEquals('sharedBy', $share->getSharedBy()); $this->assertEquals('shareOwner', $share->getShareOwner()); @@ -834,6 +838,8 @@ public function testCreateGroupShare(): void { } public function testCreateLinkShare(): void { + $passwordHash = Server::get(IHasher::class)->hash('password'); + $share = new Share($this->rootFolder, $this->userManager); $shareOwner = $this->createMock(IUser::class); @@ -864,7 +870,7 @@ public function testCreateLinkShare(): void { $share->setShareOwner('shareOwner'); $share->setNode($path); $share->setPermissions(1); - $share->setPassword('password'); + $share->setPasswordHash($passwordHash); $share->setSendPasswordByTalk(true); $share->setToken('token'); $expireDate = new \DateTime(); @@ -882,19 +888,22 @@ public function testCreateLinkShare(): void { $this->assertSame('/target', $share2->getTarget()); $this->assertLessThanOrEqual(new \DateTime(), $share2->getShareTime()); $this->assertSame($path, $share2->getNode()); - $this->assertSame('password', $share2->getPassword()); + $this->assertTrue($share2->isPasswordHashed()); + $this->assertSame($passwordHash, $share2->getPassword()); $this->assertSame(true, $share2->getSendPasswordByTalk()); $this->assertSame('token', $share2->getToken()); $this->assertEquals($expireDate->getTimestamp(), $share2->getExpirationDate()->getTimestamp()); } public function testGetShareByToken(): void { + $passwordHash = Server::get(IHasher::class)->hash('password'); + $qb = $this->dbConn->getQueryBuilder(); $qb->insert('share') ->values([ 'share_type' => $qb->expr()->literal(IShare::TYPE_LINK), - 'password' => $qb->expr()->literal('password'), + 'password' => $qb->expr()->literal($passwordHash), 'password_by_talk' => $qb->expr()->literal(true), 'uid_owner' => $qb->expr()->literal('shareOwner'), 'uid_initiator' => $qb->expr()->literal('sharedBy'), @@ -919,7 +928,8 @@ public function testGetShareByToken(): void { $this->assertSame('shareOwner', $share->getShareOwner()); $this->assertSame('sharedBy', $share->getSharedBy()); $this->assertSame('secrettoken', $share->getToken()); - $this->assertSame('password', $share->getPassword()); + $this->assertTrue($share->isPasswordHashed()); + $this->assertSame($passwordHash, $share->getPassword()); $this->assertSame('the label', $share->getLabel()); $this->assertSame(true, $share->getSendPasswordByTalk()); $this->assertSame(null, $share->getSharedWith()); @@ -930,12 +940,14 @@ public function testGetShareByToken(): void { * as types on IShare, a string and not null */ public function testGetShareByTokenNullLabel(): void { + $passwordHash = Server::get(IHasher::class)->hash('password'); + $qb = $this->dbConn->getQueryBuilder(); $qb->insert('share') ->values([ 'share_type' => $qb->expr()->literal(IShare::TYPE_LINK), - 'password' => $qb->expr()->literal('password'), + 'password' => $qb->expr()->literal($passwordHash), 'password_by_talk' => $qb->expr()->literal(true), 'uid_owner' => $qb->expr()->literal('shareOwner'), 'uid_initiator' => $qb->expr()->literal('sharedBy'), @@ -1924,6 +1936,8 @@ function ($userId) use ($users) { } public function testUpdateLink(): void { + $passwordHash = Server::get(IHasher::class)->hash('password'); + $id = $this->addShareToDB(IShare::TYPE_LINK, null, 'user1', 'user2', 'file', 42, 'target', 31, null, null); @@ -1957,7 +1971,7 @@ function ($userId) use ($users) { $share = $this->provider->getShareById($id); - $share->setPassword('password'); + $share->setPasswordHash($passwordHash); $share->setSendPasswordByTalk(true); $share->setSharedBy('user4'); $share->setShareOwner('user5'); @@ -1967,7 +1981,8 @@ function ($userId) use ($users) { $share2 = $this->provider->update($share); $this->assertEquals($id, $share2->getId()); - $this->assertEquals('password', $share2->getPassword()); + $this->assertTrue($share2->isPasswordHashed()); + $this->assertEquals($passwordHash, $share2->getPassword()); $this->assertSame(true, $share2->getSendPasswordByTalk()); $this->assertSame('user4', $share2->getSharedBy()); $this->assertSame('user5', $share2->getShareOwner()); @@ -1976,7 +1991,8 @@ function ($userId) use ($users) { $share2 = $this->provider->getShareById($id); $this->assertEquals($id, $share2->getId()); - $this->assertEquals('password', $share2->getPassword()); + $this->assertTrue($share2->isPasswordHashed()); + $this->assertEquals($passwordHash, $share2->getPassword()); $this->assertSame(true, $share2->getSendPasswordByTalk()); $this->assertSame('user4', $share2->getSharedBy()); $this->assertSame('user5', $share2->getShareOwner()); @@ -1984,13 +2000,15 @@ function ($userId) use ($users) { } public function testUpdateLinkRemovePassword(): void { + $passwordHash = Server::get(IHasher::class)->hash('password'); + $id = $this->addShareToDB(IShare::TYPE_LINK, 'foo', 'user1', 'user2', 'file', 42, 'target', 31, null, null); $qb = $this->dbConn->getQueryBuilder(); $qb->update('share'); $qb->where($qb->expr()->eq('id', $qb->createNamedParameter($id))); - $qb->set('password', $qb->createNamedParameter('password')); + $qb->set('password', $qb->createNamedParameter($passwordHash)); $this->assertEquals(1, $qb->executeStatement()); $users = []; @@ -2032,6 +2050,7 @@ function ($userId) use ($users) { $share2 = $this->provider->update($share); $this->assertEquals($id, $share2->getId()); + $this->assertFalse($share2->isPasswordHashed()); $this->assertEquals(null, $share2->getPassword()); $this->assertSame('user4', $share2->getSharedBy()); $this->assertSame('user5', $share2->getShareOwner()); @@ -2040,6 +2059,7 @@ function ($userId) use ($users) { $share2 = $this->provider->getShareById($id); $this->assertEquals($id, $share2->getId()); + $this->assertFalse($share2->isPasswordHashed()); $this->assertEquals(null, $share2->getPassword()); $this->assertSame('user4', $share2->getSharedBy()); $this->assertSame('user5', $share2->getShareOwner()); diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 5759c0f22a506..56e43bd17a360 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -90,7 +90,6 @@ class ManagerTest extends \Test\TestCase { protected LoggerInterface&MockObject $logger; protected IConfig&MockObject $config; protected ISecureRandom&MockObject $secureRandom; - protected IHasher&MockObject $hasher; protected IShareProvider&MockObject $defaultProvider; protected IMountManager&MockObject $mountManager; protected IGroupManager&MockObject $groupManager; @@ -115,7 +114,6 @@ protected function setUp(): void { $this->logger = $this->createMock(LoggerInterface::class); $this->config = $this->createMock(IConfig::class); $this->secureRandom = $this->createMock(ISecureRandom::class); - $this->hasher = $this->createMock(IHasher::class); $this->mountManager = $this->createMock(IMountManager::class); $this->groupManager = $this->createMock(IGroupManager::class); $this->userManager = $this->createMock(IUserManager::class); @@ -179,7 +177,7 @@ private function createManager(IProviderFactory $factory): Manager { $this->logger, $this->config, $this->secureRandom, - $this->hasher, + Server::get(IHasher::class), $this->mountManager, $this->groupManager, $this->l10nFactory, @@ -205,7 +203,7 @@ private function createManagerMock(): MockBuilder { $this->logger, $this->config, $this->secureRandom, - $this->hasher, + Server::get(IHasher::class), $this->mountManager, $this->groupManager, $this->l10nFactory, @@ -3347,11 +3345,6 @@ public function testCreateShareLink(): void { ->method('setLinkParent') ->with($share); - $this->hasher->expects($this->once()) - ->method('hash') - ->with('password') - ->willReturn('hashed'); - $this->secureRandom->method('generate') ->willReturn('token'); @@ -3395,7 +3388,8 @@ public function testCreateShareLink(): void { $this->assertEquals('/target', $share->getTarget()); $this->assertSame($date, $share->getExpirationDate()); $this->assertEquals('token', $share->getToken()); - $this->assertEquals('hashed', $share->getPassword()); + $this->assertTrue($share->isPasswordHashed()); + $this->assertNotEmpty($share->getPassword()); } public function testCreateShareMail(): void { @@ -4193,46 +4187,106 @@ public function testCheckPasswordNoPassword(): void { } public function testCheckPasswordInvalidPassword(): void { - $share = $this->createMock(IShare::class); - $share->method('getShareType')->willReturn(IShare::TYPE_LINK); - $share->method('getPassword')->willReturn('password'); - $share->method('isPasswordProtected')->willReturn(true); + $passwordHash = Server::get(IHasher::class)->hash('password'); - $this->hasher->method('verify')->with('invalidpassword', 'password', '')->willReturn(false); + $share = $this->manager->newShare() + ->setShareType(IShare::TYPE_LINK) + ->setPasswordHash($passwordHash); $this->assertFalse($this->manager->checkPassword($share, 'invalidpassword')); } public function testCheckPasswordValidPassword(): void { - $share = $this->createMock(IShare::class); - $share->method('getShareType')->willReturn(IShare::TYPE_LINK); - $share->method('getPassword')->willReturn('passwordHash'); - $share->method('isPasswordProtected')->willReturn(true); + $passwordHash = Server::get(IHasher::class)->hash('password'); - $this->hasher->method('verify')->with('password', 'passwordHash', '')->willReturn(true); + $share = $this->manager->newShare() + ->setShareType(IShare::TYPE_LINK) + ->setPasswordHash($passwordHash); $this->assertTrue($this->manager->checkPassword($share, 'password')); } - public function testCheckPasswordUpdateShare(): void { - $share = $this->manager->newShare(); - $share->setShareType(IShare::TYPE_LINK) - ->setPassword('passwordHash'); + public static function dataHasherAlgorithm(): array { + $algorithms = [ + PASSWORD_BCRYPT, + ]; - $this->hasher->method('verify')->with('password', 'passwordHash', '') - ->willReturnCallback(function ($pass, $hash, &$newHash) { - $newHash = 'newHash'; + if (\defined('PASSWORD_ARGON2I')) { + $algorithms[] = PASSWORD_ARGON2I; + } - return true; - }); + if (\defined('PASSWORD_ARGON2ID')) { + $algorithms[] = PASSWORD_ARGON2ID; + } + + return array_map(static fn (string $algorithm): array => [$algorithm], $algorithms); + } + + #[DataProvider('dataHasherAlgorithm')] + public function testCheckPasswordUpdateShareNoRehash(string $algorithm): void { + $hasher = Server::get(IHasher::class); + $this->invokePrivate($hasher, 'forcedAlgorithm', [$algorithm]); + + $passwordHash = $hasher->hash('password'); + $this->assertEquals((match ($algorithm) { + PASSWORD_ARGON2ID => 3, + PASSWORD_ARGON2I => 2, + PASSWORD_BCRYPT => 1, + }), (int)(explode('|', $passwordHash)[0])); + + $share = $this->manager->newShare() + ->setShareType(IShare::TYPE_LINK) + ->setPasswordHash($passwordHash); + + $this->defaultProvider->expects($this->never())->method('update'); + + $this->assertTrue($this->manager->checkPassword($share, 'password')); + + $this->invokePrivate($hasher, 'forcedAlgorithm', [null]); + } + + #[DataProvider('dataHasherAlgorithm')] + public function testCheckPasswordUpdateShare(string $algorithm): void { + $hasher = Server::get(IHasher::class); + $this->invokePrivate($hasher, 'forcedAlgorithm', [$algorithm]); + + $passwordHash = $hasher->hash('password'); + $this->assertEquals((match ($algorithm) { + PASSWORD_ARGON2ID => 3, + PASSWORD_ARGON2I => 2, + PASSWORD_BCRYPT => 1, + }), (int)(explode('|', $passwordHash)[0])); + + $previousHasherOptions = $this->invokePrivate($hasher, 'options'); + + // Make sure the hasher wants to rehash the password + $this->invokePrivate( + $hasher, + 'options', + [ + [ + 'threads' => $previousHasherOptions['threads'] ?? 1, + 'memory_cost' => ($previousHasherOptions['memory_cost'] ?? PASSWORD_ARGON2_DEFAULT_MEMORY_COST) + 1, + 'time_cost' => ($previousHasherOptions['time_cost'] ?? PASSWORD_ARGON2_DEFAULT_TIME_COST) + 1, + 'cost' => ($previousHasherOptions['cost'] ?? PASSWORD_BCRYPT_DEFAULT_COST) + 1, + ], + ], + ); + + $share = $this->manager->newShare() + ->setShareType(IShare::TYPE_LINK) + ->setPasswordHash($passwordHash); $this->defaultProvider->expects($this->once()) ->method('update') - ->with($this->callback(function (IShare $share) { - return $share->getPassword() === 'newHash'; + ->with($this->callback(function (IShare $share) use ($passwordHash) { + return $share->getPassword() !== null && $share->isPasswordHashed() && $share->getPassword() !== $passwordHash; })); $this->assertTrue($this->manager->checkPassword($share, 'password')); + + $this->invokePrivate($hasher, 'options', [$previousHasherOptions]); + $this->invokePrivate($hasher, 'forcedAlgorithm', [null]); } public function testUpdateShareCantChangeShareType(): void { @@ -4458,11 +4512,6 @@ public function testUpdateShareLink(): void { $manager->expects($this->once())->method('validateExpirationDateLink')->with($share); $manager->expects($this->once())->method('verifyPassword')->with('password'); - $this->hasher->expects($this->once()) - ->method('hash') - ->with('password') - ->willReturn('hashed'); - $this->defaultProvider->expects($this->once()) ->method('update') ->with($share) @@ -4538,9 +4587,6 @@ public function testUpdateShareLinkEnableSendPasswordByTalkWithNoPassword(): voi $manager->expects($this->never())->method('pathCreateChecks'); $manager->expects($this->never())->method('validateExpirationDateLink'); - $this->hasher->expects($this->never()) - ->method('hash'); - $this->defaultProvider->expects($this->never()) ->method('update'); @@ -4599,11 +4645,6 @@ public function testUpdateShareMail(): void { $manager->expects($this->once())->method('pathCreateChecks')->with($file); $manager->expects($this->once())->method('validateExpirationDateLink'); - $this->hasher->expects($this->once()) - ->method('hash') - ->with('password') - ->willReturn('hashed'); - $this->defaultProvider->expects($this->once()) ->method('update') ->with($share, 'password') @@ -4678,11 +4719,6 @@ public function testUpdateShareMailEnableSendPasswordByTalk(): void { $manager->expects($this->once())->method('pathCreateChecks')->with($file); $manager->expects($this->once())->method('validateExpirationDateLink'); - $this->hasher->expects($this->once()) - ->method('hash') - ->with('password') - ->willReturn('hashed'); - $this->defaultProvider->expects($this->once()) ->method('update') ->with($share, 'password') @@ -4757,16 +4793,6 @@ public function testUpdateShareMailEnableSendPasswordByTalkWithDifferentPassword $manager->expects($this->once())->method('pathCreateChecks')->with($file); $manager->expects($this->once())->method('validateExpirationDateLink'); - $this->hasher->expects($this->once()) - ->method('verify') - ->with('password', 'anotherPasswordHash') - ->willReturn(false); - - $this->hasher->expects($this->once()) - ->method('hash') - ->with('password') - ->willReturn('hashed'); - $this->defaultProvider->expects($this->once()) ->method('update') ->with($share, 'password') @@ -4844,10 +4870,6 @@ public function testUpdateShareMailEnableSendPasswordByTalkWithNoPassword(): voi $manager->expects($this->never())->method('pathCreateChecks'); $manager->expects($this->never())->method('validateExpirationDateLink'); - // If the password is empty, we have nothing to hash - $this->hasher->expects($this->never()) - ->method('hash'); - $this->defaultProvider->expects($this->never()) ->method('update'); @@ -4912,10 +4934,6 @@ public function testUpdateShareMailEnableSendPasswordByTalkRemovingPassword(): v $manager->expects($this->never())->method('pathCreateChecks'); $manager->expects($this->never())->method('validateExpirationDateLink'); - // If the password is empty, we have nothing to hash - $this->hasher->expects($this->never()) - ->method('hash'); - $this->defaultProvider->expects($this->never()) ->method('update'); @@ -4980,10 +4998,6 @@ public function testUpdateShareMailEnableSendPasswordByTalkRemovingPasswordWithE $manager->expects($this->never())->method('pathCreateChecks'); $manager->expects($this->never())->method('validateExpirationDateLink'); - // If the password is empty, we have nothing to hash - $this->hasher->expects($this->never()) - ->method('hash'); - $this->defaultProvider->expects($this->never()) ->method('update'); @@ -5048,12 +5062,6 @@ public function testUpdateShareMailEnableSendPasswordByTalkWithPreviousPassword( $manager->expects($this->never())->method('pathCreateChecks'); $manager->expects($this->never())->method('validateExpirationDateLink'); - // If the old & new passwords are the same, we don't do anything - $this->hasher->expects($this->never()) - ->method('verify'); - $this->hasher->expects($this->never()) - ->method('hash'); - $this->defaultProvider->expects($this->never()) ->method('update'); @@ -5118,12 +5126,6 @@ public function testUpdateShareMailDisableSendPasswordByTalkWithPreviousPassword $manager->expects($this->never())->method('pathCreateChecks'); $manager->expects($this->never())->method('validateExpirationDateLink'); - // If the old & new passwords are the same, we don't do anything - $this->hasher->expects($this->never()) - ->method('verify'); - $this->hasher->expects($this->never()) - ->method('hash'); - $this->defaultProvider->expects($this->never()) ->method('update'); @@ -5188,12 +5190,6 @@ public function testUpdateShareMailDisableSendPasswordByTalkWithoutChangingPassw $manager->expects($this->never())->method('pathCreateChecks'); $manager->expects($this->never())->method('validateExpirationDateLink'); - // If the old & new passwords are the same, we don't do anything - $this->hasher->expects($this->never()) - ->method('verify'); - $this->hasher->expects($this->never()) - ->method('hash'); - $this->defaultProvider->expects($this->never()) ->method('update'); From a7d51a0f3520ea71eb9ab48b085cce8b3d7cc086 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Wed, 23 Sep 2026 14:26:20 +0200 Subject: [PATCH 13/13] fix(OC\Core\Sharing\Recipient): Listen to Before*DeletedEvent to delete the recipients Signed-off-by: provokateurin --- core/Sharing/Recipient/GroupShareRecipientType.php | 6 +++--- core/Sharing/Recipient/UserShareRecipientType.php | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/core/Sharing/Recipient/GroupShareRecipientType.php b/core/Sharing/Recipient/GroupShareRecipientType.php index d2539c72399b2..01490505dcce4 100644 --- a/core/Sharing/Recipient/GroupShareRecipientType.php +++ b/core/Sharing/Recipient/GroupShareRecipientType.php @@ -20,7 +20,7 @@ use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventDispatcher; use OCP\EventDispatcher\IEventListener; -use OCP\Group\Events\GroupDeletedEvent; +use OCP\Group\Events\BeforeGroupDeletedEvent; use OCP\IDBConnection; use OCP\IGroupManager; use OCP\Interaction\InteractionReceiver; @@ -30,7 +30,7 @@ use OCP\Share\IShare; /** - * @template-implements IEventListener + * @template-implements IEventListener */ final class GroupShareRecipientType extends AShareRecipientTypeSearchCollaborator implements IEventListener { public function __construct( @@ -39,7 +39,7 @@ public function __construct( private readonly IGroupManager $groupManager, private readonly ISharingManager $manager, ) { - $eventDispatcher->addServiceListener(GroupDeletedEvent::class, self::class); + $eventDispatcher->addServiceListener(BeforeGroupDeletedEvent::class, self::class); } #[\Override] diff --git a/core/Sharing/Recipient/UserShareRecipientType.php b/core/Sharing/Recipient/UserShareRecipientType.php index 3c69a7ab804b1..61927b0b1f8c0 100644 --- a/core/Sharing/Recipient/UserShareRecipientType.php +++ b/core/Sharing/Recipient/UserShareRecipientType.php @@ -26,10 +26,10 @@ use OCP\IUserManager; use OCP\L10N\IFactory; use OCP\Share\IShare; -use OCP\User\Events\UserDeletedEvent; +use OCP\User\Events\BeforeUserDeletedEvent; /** - * @template-implements IEventListener + * @template-implements IEventListener */ final class UserShareRecipientType extends AShareRecipientTypeSearchCollaborator implements IEventListener { @@ -39,7 +39,7 @@ public function __construct( private readonly IUserManager $userManager, private readonly ISharingManager $manager, ) { - $eventDispatcher->addServiceListener(UserDeletedEvent::class, self::class); + $eventDispatcher->addServiceListener(BeforeUserDeletedEvent::class, self::class); } #[\Override]