refactor: clean up ShareService a little

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 {

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)]

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 {

lib/Controller/ShareController.php

@@ -42,22 +42,24 @@ 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);
 		});
 	}
 
 	#[NoAdminRequired]
 	#[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));
 		});
 	}
 

lib/Middleware/Attribute/AssertShareToken.php → lib/Middleware/Attribute/AssertShareAccessIsAccessible.php

@@ -11,6 +11,6 @@
 use Attribute;
 
 #[Attribute(Attribute::TARGET_METHOD)]
-class AssertShareToken {
+class AssertShareAccessIsAccessible {
 
 }

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;
 	}
 }

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,15 +506,15 @@ 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());
 		}
 	}
 
 	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]);
 		}
 	}