From 4b58d85d823800119b2e9b341ebfe159ebe6701b Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Tue, 17 Feb 2026 21:04:18 +0100 Subject: [PATCH] refactor: clean up ShareService a little - make class properties readonly private - clean up imports - clean up PHP doc - remove unnecessary vars Signed-off-by: Arthur Schiwon --- lib/Controller/ApiPublicColumnsController.php | 6 +- lib/Controller/PublicRowOCSController.php | 4 +- lib/Controller/PublicSharePageController.php | 13 ++-- lib/Controller/ShareController.php | 8 ++- ....php => AssertShareAccessIsAccessible.php} | 2 +- lib/Middleware/ShareControlMiddleware.php | 50 +++++++++++-- lib/Service/ShareService.php | 71 ++++++++++--------- openapi.json | 3 +- 8 files changed, 98 insertions(+), 59 deletions(-) rename lib/Middleware/Attribute/{AssertShareToken.php => AssertShareAccessIsAccessible.php} (87%) diff --git a/lib/Controller/ApiPublicColumnsController.php b/lib/Controller/ApiPublicColumnsController.php index 44858e5c71..f622ab8bc7 100644 --- a/lib/Controller/ApiPublicColumnsController.php +++ b/lib/Controller/ApiPublicColumnsController.php @@ -13,7 +13,7 @@ use OCA\Tables\Errors\InternalError; use OCA\Tables\Errors\NotFoundError; use OCA\Tables\Errors\PermissionError; -use OCA\Tables\Middleware\Attribute\AssertShareToken; +use OCA\Tables\Middleware\Attribute\AssertShareAccessIsAccessible; use OCA\Tables\ResponseDefinitions; use OCA\Tables\Service\ColumnService; use OCA\Tables\Service\ShareService; @@ -58,8 +58,8 @@ public function __construct( * 500: Internal error */ #[PublicPage] - #[AssertShareToken] - #[ApiRoute(verb: 'GET', url: '/api/2/public/{token}/columns')] + #[AssertShareAccessIsAccessible] + #[ApiRoute(verb: 'GET', url: '/api/2/public/{token}/columns', requirements: ['token' => '[a-zA-Z0-9]{16}'])] #[OpenAPI] #[AnonRateLimit(limit: 10, period: 10)] public function indexByPublicLink(string $token): DataResponse { diff --git a/lib/Controller/PublicRowOCSController.php b/lib/Controller/PublicRowOCSController.php index 7b8fd03a75..bfb348c5a3 100644 --- a/lib/Controller/PublicRowOCSController.php +++ b/lib/Controller/PublicRowOCSController.php @@ -14,7 +14,7 @@ use OCA\Tables\Errors\NotFoundError; use OCA\Tables\Errors\PermissionError; use OCA\Tables\Helper\ConversionHelper; -use OCA\Tables\Middleware\Attribute\AssertShareToken; +use OCA\Tables\Middleware\Attribute\AssertShareAccessIsAccessible; use OCA\Tables\ResponseDefinitions; use OCA\Tables\Service\RowService; use OCA\Tables\Service\ShareService; @@ -59,7 +59,7 @@ public function __construct( * 500: Internal error */ #[PublicPage] - #[AssertShareToken] + #[AssertShareAccessIsAccessible] #[ApiRoute(verb: 'GET', url: '/api/2/public/{token}/rows', requirements: ['token' => '[a-zA-Z0-9]{16}'])] #[OpenAPI] #[AnonRateLimit(limit: 20, period: 30)] diff --git a/lib/Controller/PublicSharePageController.php b/lib/Controller/PublicSharePageController.php index 64fb8e3695..dda37e5af2 100644 --- a/lib/Controller/PublicSharePageController.php +++ b/lib/Controller/PublicSharePageController.php @@ -40,10 +40,10 @@ public function __construct( IRequest $request, ISession $session, IURLGenerator $urlGenerator, - private ShareService $shareService, - private NodeService $nodeService, - private IInitialState $initialState, - private IEventDispatcher $eventDispatcher, + private readonly ShareService $shareService, + private readonly NodeService $nodeService, + private readonly IInitialState $initialState, + private readonly IEventDispatcher $eventDispatcher, ) { parent::__construct($appName, $request, $session, $urlGenerator); $token = $request->getParam('token'); @@ -86,8 +86,7 @@ public function showShare(): TemplateResponse { public function showAuthenticate(): TemplateResponse { $templateParameters = ['share' => new SharePageShareDataDecorator($this->share)]; - $response = new TemplateResponse('core', 'publicshareauth', $templateParameters, 'guest'); - return $response; + return new TemplateResponse('core', 'publicshareauth', $templateParameters, 'guest'); } protected function showAuthFailed(): TemplateResponse { @@ -112,7 +111,7 @@ protected function isPasswordProtected(): bool { } protected function verifyPassword(string $password): bool { - return $password === $this->share->getPassword(); + return $this->shareService->checkPassword($this->share, $password); } protected function loadStyles(): void { diff --git a/lib/Controller/ShareController.php b/lib/Controller/ShareController.php index 8cef76afc0..527c233c23 100644 --- a/lib/Controller/ShareController.php +++ b/lib/Controller/ShareController.php @@ -42,7 +42,8 @@ public function __construct( #[RequirePermission(permission: Application::PERMISSION_READ, type: Application::NODE_TYPE_TABLE, idParam: 'tableId')] public function index(int $tableId): DataResponse { return $this->handleError(function () use ($tableId) { - return $this->service->findAll('table', $tableId); + $shares = $this->service->findAll('table', $tableId); + return array_map([$this->service, 'formatForOutput'], $shares); }); } @@ -50,14 +51,15 @@ public function index(int $tableId): DataResponse { #[RequirePermission(permission: Application::PERMISSION_READ, type: Application::NODE_TYPE_VIEW, idParam: 'viewId')] public function indexView(int $viewId): DataResponse { return $this->handleError(function () use ($viewId) { - return $this->service->findAll('view', $viewId); + $shares = $this->service->findAll('view', $viewId); + return array_map([$this->service, 'formatForOutput'], $shares); }); } #[NoAdminRequired] public function show(int $id): DataResponse { return $this->handleError(function () use ($id) { - return $this->service->find($id); + return $this->service->formatForOutput($this->service->find($id)); }); } diff --git a/lib/Middleware/Attribute/AssertShareToken.php b/lib/Middleware/Attribute/AssertShareAccessIsAccessible.php similarity index 87% rename from lib/Middleware/Attribute/AssertShareToken.php rename to lib/Middleware/Attribute/AssertShareAccessIsAccessible.php index 4fa8df9711..dbb7d59872 100644 --- a/lib/Middleware/Attribute/AssertShareToken.php +++ b/lib/Middleware/Attribute/AssertShareAccessIsAccessible.php @@ -11,6 +11,6 @@ use Attribute; #[Attribute(Attribute::TARGET_METHOD)] -class AssertShareToken { +class AssertShareAccessIsAccessible { } diff --git a/lib/Middleware/ShareControlMiddleware.php b/lib/Middleware/ShareControlMiddleware.php index 160aa0a265..84a3496405 100644 --- a/lib/Middleware/ShareControlMiddleware.php +++ b/lib/Middleware/ShareControlMiddleware.php @@ -9,31 +9,64 @@ namespace OCA\Tables\Middleware; use InvalidArgumentException; +use OCA\Tables\Db\Share; use OCA\Tables\Errors\NotFoundError; -use OCA\Tables\Middleware\Attribute\AssertShareToken; +use OCA\Tables\Errors\PermissionError; +use OCA\Tables\Middleware\Attribute\AssertShareAccessIsAccessible; use OCA\Tables\Service\ShareService; use OCA\Tables\Service\ValueObject\ShareToken; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\Middleware; +use OCP\AppFramework\PublicShareController; use OCP\IRequest; +use OCP\ISession; use ReflectionMethod; class ShareControlMiddleware extends Middleware { + private Share $share; + public function __construct( - protected IRequest $request, - protected ShareService $shareService, + private readonly IRequest $request, + private readonly ShareService $shareService, + private readonly ISession $session, ) { } + /** + * @throws \ReflectionException + * @throws NotFoundError + * @throws PermissionError + */ public function beforeController(Controller $controller, string $methodName): void { $reflectionMethod = new ReflectionMethod($controller, $methodName); - $shallAssertShareToken = $reflectionMethod->getAttributes(AssertShareToken::class); - if ($shallAssertShareToken) { - $this->assertShareTokenIsValidAndExisting($this->request->getParam('token', '')); + $shallAssertShareAccessible = $reflectionMethod->getAttributes(AssertShareAccessIsAccessible::class); + if ($shallAssertShareAccessible) { + $tokenInput = $this->request->getParam('token', ''); + $this->assertShareTokenIsValidAndExisting($tokenInput); + $this->assertIsAccessible($tokenInput); } + } + /** + * @throws PermissionError + * @see PublicShareController + */ + private function assertIsAccessible(string $tokenInput): void { + if ($this->share->getPassword() === null || $this->share->getPassword() === '') { + return; + } + + $allowedTokensJSON = $this->session->get(PublicShareController::DAV_AUTHENTICATED_FRONTEND) ?? '[]'; + $allowedTokens = json_decode($allowedTokensJSON, true); + if (!is_array($allowedTokens)) { + $allowedTokens = []; + } + + if (($allowedTokens[$tokenInput] ?? '') !== $this->share->getPassword()) { + throw new PermissionError('Share cannot be accessed'); + } } /** @@ -42,7 +75,7 @@ public function beforeController(Controller $controller, string $methodName): vo */ public function assertShareTokenIsValidAndExisting(string $tokenInput): void { $shareToken = new ShareToken($tokenInput); - $this->shareService->findByToken($shareToken); + $this->share = $this->shareService->findByToken($shareToken); } public function afterException($controller, $methodName, \Exception $exception): DataResponse { @@ -52,6 +85,9 @@ public function afterException($controller, $methodName, \Exception $exception): if ($exception instanceof NotFoundError) { return new DataResponse(['message' => $exception->getMessage()], Http::STATUS_NOT_FOUND); } + if ($exception instanceof PermissionError) { + return new DataResponse(['message' => $exception->getMessage()], Http::STATUS_FORBIDDEN); + } throw $exception; } } diff --git a/lib/Service/ShareService.php b/lib/Service/ShareService.php index a664478599..5beb281f1b 100644 --- a/lib/Service/ShareService.php +++ b/lib/Service/ShareService.php @@ -12,7 +12,6 @@ use DateTime; use InvalidArgumentException; -use OC\User\User; use OCA\Tables\AppInfo\Application; use OCA\Tables\Constants\ShareReceiverType; use OCA\Tables\Db\Context; @@ -40,6 +39,7 @@ use OCP\DB\Exception; use OCP\IDBConnection; use OCP\IUserManager; +use OCP\Security\IHasher; use OCP\Security\ISecureRandom; use Psr\Log\LoggerInterface; use Throwable; @@ -54,21 +54,21 @@ public function __construct( PermissionsService $permissionsService, LoggerInterface $logger, ?string $userId, - protected ShareMapper $mapper, - protected TableMapper $tableMapper, - protected ViewMapper $viewMapper, - protected UserHelper $userHelper, - protected GroupHelper $groupHelper, - protected CircleHelper $circleHelper, - private ContextNavigationMapper $contextNavigationMapper, - private IDBConnection $dbc, - protected ISecureRandom $secureRandom, - protected IUserManager $userManager, + private readonly ShareMapper $mapper, + private readonly TableMapper $tableMapper, + private readonly ViewMapper $viewMapper, + private readonly UserHelper $userHelper, + private readonly GroupHelper $groupHelper, + private readonly CircleHelper $circleHelper, + private readonly ContextNavigationMapper $contextNavigationMapper, + private readonly IDBConnection $dbc, + private readonly ISecureRandom $secureRandom, + private readonly IUserManager $userManager, + private readonly IHasher $hasher, ) { parent::__construct($logger, $userId, $permissionsService); } - /** * @throws InternalError * @return Share[] @@ -92,7 +92,16 @@ public function findAll(string $nodeType, int $nodeId, ?string $userId = null, b * @return TablesShare[] */ public function formatShares(array $items): array { - return array_map(fn (Share $item) => $item->jsonSerialize(), $items); + return array_map(static fn (Share $item) => $item->jsonSerialize(), $items); + } + + public function formatForOutput(Share $share): Share { + if ($share->getPassword() === null || $share->getPassword() === '') { + return $share; + } + $clone = clone $share; + $clone->setPassword('yes'); + return $clone; } /** @@ -163,6 +172,10 @@ public function createLinkShare(Table|View $node, ?string $password = null): Sha throw $e; } + public function checkPassword(Share $share, string $password): bool { + return $this->hasher->verify($password, $share->getPassword()); + } + public function hasLinkShare(Table|View $node): bool { $type = ConversionHelper::object2String($node); $nodeShares = $this->mapper->findAllSharesForNode($type, $node->getId()); @@ -238,7 +251,7 @@ private function findElementsSharedWithMe(string $elementType = 'table', ?string throw new InternalError('Cannot find element of type ' . $elementType); } // Check if en element with this id is already in the result array - $index = array_search($element->getId(), array_column($returnArray, 'id')); + $index = array_search($element->getId(), array_column($returnArray, 'id'), true); if (!$index) { $returnArray[] = $element; } @@ -254,7 +267,8 @@ private function findElementsSharedWithMe(string $elementType = 'table', ?string * @param int $elementId * @param 'table'|'view' $elementType * @param string|null $userId - * @throws NotFoundError + * @return Permissions + * @throws NotFoundError|InternalError */ public function getSharedPermissionsIfSharedWithMe(int $elementId, string $elementType = 'table', ?string $userId = null): Permissions { try { @@ -267,19 +281,9 @@ public function getSharedPermissionsIfSharedWithMe(int $elementId, string $eleme } /** - * @param int $nodeId - * @param string $nodeType - * @param string $receiver - * @param string $receiverType - * @param bool $permissionRead - * @param bool $permissionCreate - * @param bool $permissionUpdate - * @param bool $permissionDelete - * @param bool $permissionManage - * @return Share * @throws InternalError */ - public function create(int $nodeId, string $nodeType, string $receiver, string $receiverType, bool $permissionRead, bool $permissionCreate, bool $permissionUpdate, bool $permissionDelete, bool $permissionManage, int $displayMode, ?ShareToken $shareToken = null, ?string $password = null, ?string $sender = null):Share { + public function create(int $nodeId, string $nodeType, string $receiver, string $receiverType, bool $permissionRead, bool $permissionCreate, bool $permissionUpdate, bool $permissionDelete, bool $permissionManage, int $displayMode, ?ShareToken $shareToken = null, ?string $password = null, ?string $sender = null): Share { $sender = $sender ?? $this->userId; if (!$sender) { $e = new \Exception('No user given.'); @@ -306,7 +310,7 @@ public function create(int $nodeId, string $nodeType, string $receiver, string $ $item->setToken((string)$shareToken); } if ($password) { - $item->setPassword($password); + $item->setPassword($this->hasher->hash($password)); } try { @@ -502,7 +506,7 @@ private function addReceiverDisplayNames(array $shares): array { public function deleteAllForTable(Table $table):void { try { $this->mapper->deleteByNode($table->getId(), 'table'); - } catch (Exception $e) { + } catch (Exception) { $this->logger->error('something went wrong while deleting shares for table: ' . $table->getId()); } } @@ -510,7 +514,7 @@ public function deleteAllForTable(Table $table):void { public function deleteAllForView(View $view):void { try { $this->mapper->deleteByNode($view->getId(), 'view'); - } catch (Exception $e) { + } catch (Exception) { $this->logger->error('something went wrong while deleting shares for view: ' . $view->getId()); } } @@ -520,12 +524,11 @@ public function deleteAllForContext(Context $context): void { $this->atomic(function () use ($context) { $shares = $this->mapper->findAllSharesForNode('context', $context->getId(), $this->userId); foreach ($shares as $share) { - /** @var Share $share */ $this->contextNavigationMapper->deleteByShareId($share->getId()); } $this->mapper->deleteByNode($context->getId(), 'context'); }, $this->dbc); - } catch (Exception $e) { + } catch (Exception) { $this->logger->error('something went wrong while deleting shares for context: ' . $context->getId()); } } @@ -539,7 +542,6 @@ public function changeSenderForNode(string $nodeType, int $nodeId, string $newOw $newShares = []; foreach ($sharesForTable as $share) { - /* @var Share $share */ $share->setSender($newOwnerUserId); try { $this->mapper->update($share); @@ -561,7 +563,6 @@ public function findSharedWithUserIds(int $elementId, string $elementType): arra $shares = $this->mapper->findAllSharesForNode($elementType, $elementId, ''); $sharedWithUserIds = []; - /** @var Share $share */ foreach ($shares as $share) { if ($share->getReceiverType() === ShareReceiverType::USER) { $sharedWithUserIds[$share->getReceiver()] = 1; @@ -611,7 +612,7 @@ public function importShare(int $nodeId, array $share, string $userId): void { if (!empty($share['token'])) { try { $shareToken = new ShareToken($share['token']); - } catch (\Throwable $e) { + } catch (Throwable $e) { $this->logger->warning('Invalid share token during import: ' . $e->getMessage(), ['token' => $share['token'], 'exception' => $e]); } } @@ -632,7 +633,7 @@ public function importShare(int $nodeId, array $share, string $userId): void { $password, $sender ); - } catch (\Throwable $e) { + } catch (Throwable $e) { $this->logger->error('Failed to import share: ' . $e->getMessage(), ['exception' => $e, 'share' => $share]); } } diff --git a/openapi.json b/openapi.json index 9ea4a10c40..0cb07e5e4f 100644 --- a/openapi.json +++ b/openapi.json @@ -10303,7 +10303,8 @@ "description": "The share token", "required": true, "schema": { - "type": "string" + "type": "string", + "pattern": "^[a-zA-Z0-9]{16}$" } }, {