diff --git a/apps/dav/lib/Connector/Sabre/CorsPlugin.php b/apps/dav/lib/Connector/Sabre/CorsPlugin.php index cc8ed00d7edf..f36b140ceca6 100644 --- a/apps/dav/lib/Connector/Sabre/CorsPlugin.php +++ b/apps/dav/lib/Connector/Sabre/CorsPlugin.php @@ -50,6 +50,10 @@ class CorsPlugin extends ServerPlugin { * @var string[] */ private $extraHeaders; + /** + * @var bool + */ + private $alreadyExecuted = false; /** * @param IUserSession $userSession @@ -107,9 +111,13 @@ public function initialize(\Sabre\DAV\Server $server) { } $this->server->on('beforeMethod:*', [$this, 'setCorsHeaders']); + $this->server->on('exception', [$this, 'onException']); $this->server->on('beforeMethod:OPTIONS', [$this, 'setOptionsRequestHeaders'], 5); } + public function onException(\Throwable $ex) { + $this->setCorsHeaders($this->server->httpRequest, $this->server->httpResponse); + } /** * This method sets the cors headers for all requests * @@ -118,18 +126,23 @@ public function initialize(\Sabre\DAV\Server $server) { * @return void */ public function setCorsHeaders(RequestInterface $request, ResponseInterface $response) { - if ($request->getHeader('origin') !== null) { - $requesterDomain = $request->getHeader('origin'); - // unauthenticated request shall add cors headers as well - $userId = null; - if ($this->userSession->getUser() !== null) { - $userId = $this->userSession->getUser()->getUID(); - } + if ($request->getHeader('origin') === null) { + return; + } + if ($this->alreadyExecuted) { + return; + } + $this->alreadyExecuted = true; + $requesterDomain = $request->getHeader('origin'); + // unauthenticated request shall add cors headers as well + $userId = null; + if ($this->userSession->getUser() !== null) { + $userId = $this->userSession->getUser()->getUID(); + } - $headers = \OC_Response::setCorsHeaders($userId, $requesterDomain, null, $this->getExtraHeaders($request)); - foreach ($headers as $key => $value) { - $response->addHeader($key, \implode(',', $value)); - } + $headers = \OC_Response::setCorsHeaders($userId, $requesterDomain, null, $this->getExtraHeaders($request)); + foreach ($headers as $key => $value) { + $response->addHeader($key, \implode(',', $value)); } } diff --git a/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php b/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php index f3bedae4c30f..76485ec2f683 100644 --- a/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php +++ b/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php @@ -32,6 +32,7 @@ use OCA\FederatedFileSharing\FedShareManager; use OCA\FederatedFileSharing\Middleware\OcmMiddleware; use OCA\FederatedFileSharing\Ocm\Exception\BadRequestException; +use OCA\FederatedFileSharing\Ocm\Exception\ForbiddenException; use OCA\FederatedFileSharing\Ocm\Exception\NotImplementedException; use OCA\FederatedFileSharing\Ocm\Exception\OcmException; use OCP\AppFramework\Http; @@ -255,6 +256,18 @@ public function acceptShare($id) { $token = $this->request->getParam('token', null); $share = $this->ocmMiddleware->getValidShare($id, $token); $this->fedShareManager->acceptShare($share); + } catch (BadRequestException $e) { + return new Result( + null, + Http::STATUS_GONE, + $e->getMessage() + ); + } catch (ForbiddenException $e) { + return new Result( + null, + Http::STATUS_FORBIDDEN, + $e->getMessage() + ); } catch (NotImplementedException $e) { return new Result( null, @@ -281,6 +294,18 @@ public function declineShare($id) { $this->ocmMiddleware->assertOutgoingSharingEnabled(); $share = $this->ocmMiddleware->getValidShare($id, $token); $this->fedShareManager->declineShare($share); + } catch (BadRequestException $e) { + return new Result( + null, + Http::STATUS_GONE, + $e->getMessage() + ); + } catch (ForbiddenException $e) { + return new Result( + null, + Http::STATUS_FORBIDDEN, + $e->getMessage() + ); } catch (NotImplementedException $e) { return new Result( null, diff --git a/apps/federatedfilesharing/tests/Controller/RequestHandlerTest.php b/apps/federatedfilesharing/tests/Controller/RequestHandlerTest.php index a13dcc700c33..ee7a1dc82d6d 100644 --- a/apps/federatedfilesharing/tests/Controller/RequestHandlerTest.php +++ b/apps/federatedfilesharing/tests/Controller/RequestHandlerTest.php @@ -308,6 +308,42 @@ public function testAcceptSuccess() { ); } + public function testAcceptFailedWhenInvalidShareId() { + $this->request->expects($this->any()) + ->method('getParam') + ->willReturn(self::DEFAULT_TOKEN); + + $this->ocmMiddleware->method('getValidShare') + ->willThrowException(new BadRequestException()); + + $this->fedShareManager->expects($this->never()) + ->method('acceptShare'); + + $response = $this->requestHandlerController->acceptShare(2); + $this->assertEquals( + Http::STATUS_GONE, + $response->getStatusCode() + ); + } + + public function testAcceptFailedWhenShareIdHasInvalidSecret() { + $this->request->expects($this->any()) + ->method('getParam') + ->willReturn(self::DEFAULT_TOKEN); + + $this->ocmMiddleware->method('getValidShare') + ->willThrowException(new ForbiddenException()); + + $this->fedShareManager->expects($this->never()) + ->method('acceptShare'); + + $response = $this->requestHandlerController->acceptShare(2); + $this->assertEquals( + Http::STATUS_FORBIDDEN, + $response->getStatusCode() + ); + } + public function testDeclineFailedWhenSharingIsDisabled() { $this->ocmMiddleware->method('assertOutgoingSharingEnabled') ->willThrowException(new NotImplementedException()); @@ -346,6 +382,42 @@ public function testDeclineSuccess() { ); } + public function testDeclineFailedWhenInvalidShareId() { + $this->request->expects($this->any()) + ->method('getParam') + ->willReturn(self::DEFAULT_TOKEN); + + $this->ocmMiddleware->method('getValidShare') + ->willThrowException(new BadRequestException()); + + $this->fedShareManager->expects($this->never()) + ->method('declineShare'); + + $response = $this->requestHandlerController->declineShare(2); + $this->assertEquals( + Http::STATUS_GONE, + $response->getStatusCode() + ); + } + + public function testDeclineFailedWhenShareIdHasInvalidSecret() { + $this->request->expects($this->any()) + ->method('getParam') + ->willReturn(self::DEFAULT_TOKEN); + + $this->ocmMiddleware->method('getValidShare') + ->willThrowException(new ForbiddenException()); + + $this->fedShareManager->expects($this->never()) + ->method('declineShare'); + + $response = $this->requestHandlerController->declineShare(2); + $this->assertEquals( + Http::STATUS_FORBIDDEN, + $response->getStatusCode() + ); + } + public function testUnshareFailedWhenSharingIsDisabled() { $this->ocmMiddleware->method('assertOutgoingSharingEnabled') ->willThrowException(new NotImplementedException()); diff --git a/apps/files_sharing/lib/AppInfo/Application.php b/apps/files_sharing/lib/AppInfo/Application.php index cb754e0d889d..9ce2c3cac5fc 100644 --- a/apps/files_sharing/lib/AppInfo/Application.php +++ b/apps/files_sharing/lib/AppInfo/Application.php @@ -86,7 +86,7 @@ public function __construct(array $urlParams = []) { $server->getUserManager(), $server->getRootFolder(), $server->getURLGenerator(), - $server->getUserSession()->getUser(), + $server->getUserSession(), $server->getL10N('files_sharing'), $server->getConfig(), $c->query(NotificationPublisher::class), diff --git a/apps/files_sharing/lib/Controller/Share20OcsController.php b/apps/files_sharing/lib/Controller/Share20OcsController.php index 26a64db9f168..728edd1ec90a 100644 --- a/apps/files_sharing/lib/Controller/Share20OcsController.php +++ b/apps/files_sharing/lib/Controller/Share20OcsController.php @@ -34,6 +34,7 @@ use OCP\IURLGenerator; use OCP\IUser; use OCP\IUserManager; +use OCP\IUserSession; use OCP\Lock\ILockingProvider; use OCP\Lock\LockedException; use OCP\Share; @@ -61,10 +62,10 @@ class Share20OcsController extends OCSController { private $userManager; /** @var IRootFolder */ private $rootFolder; + /** @var IUserSession */ + private $userSession; /** @var IURLGenerator */ private $urlGenerator; - /** @var IUser */ - private $currentUser; /** @var IL10N */ private $l; /** @var IConfig */ @@ -73,9 +74,9 @@ class Share20OcsController extends OCSController { private $notificationPublisher; /** @var EventDispatcher */ private $eventDispatcher; + /** @var SharingBlacklist */ private $sharingBlacklist; - /** * @var string */ @@ -89,7 +90,7 @@ public function __construct( IUserManager $userManager, IRootFolder $rootFolder, IURLGenerator $urlGenerator, - IUser $currentUser, + IUserSession $userSession, IL10N $l10n, IConfig $config, NotificationPublisher $notificationPublisher, @@ -103,13 +104,13 @@ public function __construct( $this->userManager = $userManager; $this->rootFolder = $rootFolder; $this->urlGenerator = $urlGenerator; - $this->currentUser = $currentUser; $this->l = $l10n; $this->config = $config; $this->notificationPublisher = $notificationPublisher; $this->eventDispatcher = $eventDispatcher; $this->sharingBlacklist = $sharingBlacklist; $this->additionalInfoField = $this->config->getAppValue('core', 'user_additional_info_field', ''); + $this->userSession = $userSession; } /** @@ -158,14 +159,14 @@ protected function formatShare(IShare $share, $received = false) { $result['state'] = $share->getState(); // can only fetch path info if mounted already or if owner - if ($share->getState() === Share::STATE_ACCEPTED || $share->getShareOwner() === $this->currentUser->getUID()) { - $userFolder = $this->rootFolder->getUserFolder($this->currentUser->getUID()); + if ($share->getState() === Share::STATE_ACCEPTED || $share->getShareOwner() === $this->userSession->getUser()->getUID()) { + $userFolder = $this->rootFolder->getUserFolder($this->userSession->getUser()->getUID()); } else { // need to go through owner user for pending shares $userFolder = $this->rootFolder->getUserFolder($share->getShareOwner()); } } else { - $userFolder = $this->rootFolder->getUserFolder($this->currentUser->getUID()); + $userFolder = $this->rootFolder->getUserFolder($this->userSession->getUser()->getUID()); } $nodes = $userFolder->getById($share->getNodeId(), true); @@ -315,7 +316,7 @@ public function createShare() { return new Result(null, 404, $this->l->t('Please specify a file or folder path')); } - $userFolder = $this->rootFolder->getUserFolder($this->currentUser->getUID()); + $userFolder = $this->rootFolder->getUserFolder($this->userSession->getUser()->getUID()); try { $path = $userFolder->get($path); @@ -337,16 +338,16 @@ public function createShare() { $permissions = $this->request->getParam('permissions', null); if ($permissions === null) { if ($shareType !== Share::SHARE_TYPE_LINK) { - $permissions = $this->config->getAppValue('core', 'shareapi_default_permissions', \OCP\Constants::PERMISSION_ALL); - $permissions |= \OCP\Constants::PERMISSION_READ; + $permissions = $this->config->getAppValue('core', 'shareapi_default_permissions', Constants::PERMISSION_ALL); + $permissions |= Constants::PERMISSION_READ; } else { - $permissions = \OCP\Constants::PERMISSION_ALL; + $permissions = Constants::PERMISSION_ALL; } } else { $permissions = (int)$permissions; } - if ($permissions < 0 || $permissions > \OCP\Constants::PERMISSION_ALL) { + if ($permissions < 0 || $permissions > Constants::PERMISSION_ALL) { $share->getNode()->unlock(ILockingProvider::LOCK_SHARED); return new Result(null, 404, 'invalid permissions'); } @@ -356,15 +357,15 @@ public function createShare() { } // link shares can have create-only without read (anonymous upload) - if ($shareType !== Share::SHARE_TYPE_LINK && $permissions !== \OCP\Constants::PERMISSION_CREATE) { + if ($shareType !== Share::SHARE_TYPE_LINK && $permissions !== Constants::PERMISSION_CREATE) { // Shares always require read permissions - $permissions |= \OCP\Constants::PERMISSION_READ; + $permissions |= Constants::PERMISSION_READ; } if ($path instanceof \OCP\Files\File) { // Single file shares should never have delete or create permissions - $permissions &= ~\OCP\Constants::PERMISSION_DELETE; - $permissions &= ~\OCP\Constants::PERMISSION_CREATE; + $permissions &= ~Constants::PERMISSION_DELETE; + $permissions &= ~Constants::PERMISSION_CREATE; } /* @@ -424,7 +425,7 @@ public function createShare() { // legacy way, expecting that this won't be used together with "create-only" shares $publicUpload = $this->request->getParam('publicUpload', null); // a few permission checks - if ($publicUpload === 'true' || $permissions === \OCP\Constants::PERMISSION_CREATE) { + if ($publicUpload === 'true' || $permissions === Constants::PERMISSION_CREATE) { // Check if public upload is allowed if (!$this->shareManager->shareApiLinkAllowPublicUpload()) { $share->getNode()->unlock(ILockingProvider::LOCK_SHARED); @@ -441,18 +442,18 @@ public function createShare() { // convert to permissions if ($publicUpload === 'true') { $share->setPermissions( - \OCP\Constants::PERMISSION_READ | - \OCP\Constants::PERMISSION_CREATE | - \OCP\Constants::PERMISSION_UPDATE | - \OCP\Constants::PERMISSION_DELETE + Constants::PERMISSION_READ | + Constants::PERMISSION_CREATE | + Constants::PERMISSION_UPDATE | + Constants::PERMISSION_DELETE ); - } elseif ($permissions === \OCP\Constants::PERMISSION_CREATE || - $permissions === (\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE | \OCP\Constants::PERMISSION_DELETE)) { + } elseif ($permissions === Constants::PERMISSION_CREATE || + $permissions === (Constants::PERMISSION_READ | Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE | Constants::PERMISSION_DELETE)) { $share->setPermissions($permissions); } else { // because when "publicUpload" is passed usually no permissions are set, // which defaults to ALL. But in the case of link shares we default to READ... - $share->setPermissions(\OCP\Constants::PERMISSION_READ); + $share->setPermissions(Constants::PERMISSION_READ); } // set name only if passed as parameter, empty string is allowed @@ -493,7 +494,7 @@ public function createShare() { } $share->setShareType($shareType); - $share->setSharedBy($this->currentUser->getUID()); + $share->setSharedBy($this->userSession->getUser()->getUID()); try { $share = $this->shareManager->createShare($share); @@ -520,13 +521,13 @@ public function createShare() { * @return Result */ private function getSharedWithMe($node = null, $includeTags, $stateFilter = 0) { - $userShares = $this->shareManager->getSharedWith($this->currentUser->getUID(), Share::SHARE_TYPE_USER, $node, -1, 0); - $groupShares = $this->shareManager->getSharedWith($this->currentUser->getUID(), Share::SHARE_TYPE_GROUP, $node, -1, 0); + $userShares = $this->shareManager->getSharedWith($this->userSession->getUser()->getUID(), Share::SHARE_TYPE_USER, $node, -1, 0); + $groupShares = $this->shareManager->getSharedWith($this->userSession->getUser()->getUID(), Share::SHARE_TYPE_GROUP, $node, -1, 0); $shares = \array_merge($userShares, $groupShares); $shares = \array_filter($shares, function (IShare $share) { - return $share->getShareOwner() !== $this->currentUser->getUID(); + return $share->getShareOwner() !== $this->userSession->getUser()->getUID(); }); $formatted = []; @@ -538,7 +539,7 @@ private function getSharedWithMe($node = null, $includeTags, $stateFilter = 0) { * Check if the group to which the user belongs is not allowed * to reshare */ - if ($this->shareManager->sharingDisabledForUser($this->currentUser->getUID())) { + if ($this->shareManager->sharingDisabledForUser($this->userSession->getUser()->getUID())) { /** * Now set the permission to 15. Which will allow not to reshare. */ @@ -572,11 +573,11 @@ private function getSharesInDir($folder) { /** @var IShare[] $shares */ $shares = []; foreach ($nodes as $node) { - $shares = \array_merge($shares, $this->shareManager->getSharesBy($this->currentUser->getUID(), Share::SHARE_TYPE_USER, $node, false, -1, 0)); - $shares = \array_merge($shares, $this->shareManager->getSharesBy($this->currentUser->getUID(), Share::SHARE_TYPE_GROUP, $node, false, -1, 0)); - $shares = \array_merge($shares, $this->shareManager->getSharesBy($this->currentUser->getUID(), Share::SHARE_TYPE_LINK, $node, false, -1, 0)); + $shares = \array_merge($shares, $this->shareManager->getSharesBy($this->userSession->getUser()->getUID(), Share::SHARE_TYPE_USER, $node, false, -1, 0)); + $shares = \array_merge($shares, $this->shareManager->getSharesBy($this->userSession->getUser()->getUID(), Share::SHARE_TYPE_GROUP, $node, false, -1, 0)); + $shares = \array_merge($shares, $this->shareManager->getSharesBy($this->userSession->getUser()->getUID(), Share::SHARE_TYPE_LINK, $node, false, -1, 0)); if ($this->shareManager->outgoingServer2ServerSharesAllowed()) { - $shares = \array_merge($shares, $this->shareManager->getSharesBy($this->currentUser->getUID(), Share::SHARE_TYPE_REMOTE, $node, false, -1, 0)); + $shares = \array_merge($shares, $this->shareManager->getSharesBy($this->userSession->getUser()->getUID(), Share::SHARE_TYPE_REMOTE, $node, false, -1, 0)); } } @@ -619,7 +620,7 @@ public function getShares() { $includeTags = $this->request->getParam('include_tags', false); if ($path !== null) { - $userFolder = $this->rootFolder->getUserFolder($this->currentUser->getUID()); + $userFolder = $this->rootFolder->getUserFolder($this->userSession->getUser()->getUID()); try { $path = $userFolder->get($path); $path->lock(ILockingProvider::LOCK_SHARED); @@ -661,13 +662,13 @@ public function getShares() { } // Get all shares - $userShares = $this->shareManager->getSharesBy($this->currentUser->getUID(), Share::SHARE_TYPE_USER, $path, $reshares, -1, 0); - $groupShares = $this->shareManager->getSharesBy($this->currentUser->getUID(), Share::SHARE_TYPE_GROUP, $path, $reshares, -1, 0); - $linkShares = $this->shareManager->getSharesBy($this->currentUser->getUID(), Share::SHARE_TYPE_LINK, $path, $reshares, -1, 0); + $userShares = $this->shareManager->getSharesBy($this->userSession->getUser()->getUID(), Share::SHARE_TYPE_USER, $path, $reshares, -1, 0); + $groupShares = $this->shareManager->getSharesBy($this->userSession->getUser()->getUID(), Share::SHARE_TYPE_GROUP, $path, $reshares, -1, 0); + $linkShares = $this->shareManager->getSharesBy($this->userSession->getUser()->getUID(), Share::SHARE_TYPE_LINK, $path, $reshares, -1, 0); $shares = \array_merge($userShares, $groupShares, $linkShares); if ($this->shareManager->outgoingServer2ServerSharesAllowed()) { - $federatedShares = $this->shareManager->getSharesBy($this->currentUser->getUID(), Share::SHARE_TYPE_REMOTE, $path, $reshares, -1, 0); + $federatedShares = $this->shareManager->getSharesBy($this->userSession->getUser()->getUID(), Share::SHARE_TYPE_REMOTE, $path, $reshares, -1, 0); $shares = \array_merge($shares, $federatedShares); } @@ -733,9 +734,9 @@ public function updateShare($id) { $newPermissions = null; if ($publicUpload === 'true') { - $newPermissions = \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE | \OCP\Constants::PERMISSION_DELETE; + $newPermissions = Constants::PERMISSION_READ | Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE | Constants::PERMISSION_DELETE; } elseif ($publicUpload === 'false') { - $newPermissions = \OCP\Constants::PERMISSION_READ; + $newPermissions = Constants::PERMISSION_READ; } if ($permissions !== null) { @@ -743,12 +744,12 @@ public function updateShare($id) { } if ($newPermissions !== null && - $newPermissions !== \OCP\Constants::PERMISSION_READ && - $newPermissions !== \OCP\Constants::PERMISSION_CREATE && + $newPermissions !== Constants::PERMISSION_READ && + $newPermissions !== Constants::PERMISSION_CREATE && // legacy - $newPermissions !== (\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE) && + $newPermissions !== (Constants::PERMISSION_READ | Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE) && // correct - $newPermissions !== (\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE | \OCP\Constants::PERMISSION_DELETE) + $newPermissions !== (Constants::PERMISSION_READ | Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE | Constants::PERMISSION_DELETE) ) { $share->getNode()->unlock(ILockingProvider::LOCK_SHARED); return new Result(null, 400, $this->l->t('Can\'t change permissions for public share links')); @@ -756,9 +757,9 @@ public function updateShare($id) { if ( // legacy - $newPermissions === (\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE) || + $newPermissions === (Constants::PERMISSION_READ | Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE) || // correct - $newPermissions === (\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE | \OCP\Constants::PERMISSION_DELETE) + $newPermissions === (Constants::PERMISSION_READ | Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE | Constants::PERMISSION_DELETE) ) { if (!$this->shareManager->shareApiLinkAllowPublicUpload()) { $share->getNode()->unlock(ILockingProvider::LOCK_SHARED); @@ -771,12 +772,12 @@ public function updateShare($id) { } // normalize to correct public upload permissions - $newPermissions = \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE | \OCP\Constants::PERMISSION_DELETE; + $newPermissions = Constants::PERMISSION_READ | Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE | Constants::PERMISSION_DELETE; } // create-only (upload-only) if ( - $newPermissions === \OCP\Constants::PERMISSION_CREATE + $newPermissions === Constants::PERMISSION_CREATE ) { if (!$this->shareManager->shareApiLinkAllowPublicUpload()) { $share->getNode()->unlock(ILockingProvider::LOCK_SHARED); @@ -828,10 +829,10 @@ public function updateShare($id) { } } - if ($permissions !== null && $share->getShareOwner() !== $this->currentUser->getUID()) { + if ($permissions !== null && $share->getShareOwner() !== $this->userSession->getUser()->getUID()) { /* Check if this is an incoming share */ - $incomingShares = $this->shareManager->getSharedWith($this->currentUser->getUID(), Share::SHARE_TYPE_USER, $share->getNode(), -1, 0); - $incomingShares = \array_merge($incomingShares, $this->shareManager->getSharedWith($this->currentUser->getUID(), Share::SHARE_TYPE_GROUP, $share->getNode(), -1, 0)); + $incomingShares = $this->shareManager->getSharedWith($this->userSession->getUser()->getUID(), Share::SHARE_TYPE_USER, $share->getNode(), -1, 0); + $incomingShares = \array_merge($incomingShares, $this->shareManager->getSharedWith($this->userSession->getUser()->getUID(), Share::SHARE_TYPE_GROUP, $share->getNode(), -1, 0)); if (!empty($incomingShares)) { $maxPermissions = 0; @@ -907,13 +908,13 @@ public function notifyRecipients($itemSource, $shareType, $recipient) { // don't send a mail to the user who shared the file $recipientList = \array_filter($recipientList, function ($user) { /** @var IUser $user */ - return $user->getUID() !== $this->currentUser->getUID(); + return $user->getUID() !== $this->userSession->getUser()->getUID(); }); $defaults = new \OCP\Defaults(); $mailNotification = new \OC\Share\MailNotifications( $this->shareManager, - $this->currentUser, + $this->userSession->getUser(), \OC::$server->getL10N('lib'), \OC::$server->getMailer(), $this->config, @@ -923,7 +924,7 @@ public function notifyRecipients($itemSource, $shareType, $recipient) { $this->eventDispatcher ); - $userFolder = $this->rootFolder->getUserFolder($this->currentUser->getUID()); + $userFolder = $this->rootFolder->getUserFolder($this->userSession->getUser()->getUID()); $nodes = $userFolder->getById($itemSource, true); $node = $nodes[0] ?? null; $result = $mailNotification->sendInternalShareMail($node, $shareType, $recipientList); @@ -961,7 +962,7 @@ public function notifyRecipients($itemSource, $shareType, $recipient) { * @return Result */ public function notifyRecipientsDisabled($itemSource, $shareType, $recipient) { - $userFolder = $this->rootFolder->getUserFolder($this->currentUser->getUID()); + $userFolder = $this->rootFolder->getUserFolder($this->userSession->getUser()->getUID()); $nodes = $userFolder->getById($itemSource, true); $node = $nodes[0] ?? null; @@ -992,7 +993,7 @@ private function updateShareState($id, $state) { } try { - $share = $this->getShareById($id, $this->currentUser->getUID()); + $share = $this->getShareById($id, $this->userSession->getUser()->getUID()); $this->eventDispatcher->dispatch('share.before' . $eventName, new GenericEvent(null, ['share' => $share])); } catch (ShareNotFound $e) { return new Result(null, 404, $this->l->t('Wrong share ID, share doesn\'t exist')); @@ -1008,8 +1009,8 @@ private function updateShareState($id, $state) { } // only recipient can accept/reject share - if ($share->getShareOwner() === $this->currentUser->getUID() || - $share->getSharedBy() === $this->currentUser->getUID()) { + if ($share->getShareOwner() === $this->userSession->getUser()->getUID() || + $share->getSharedBy() === $this->userSession->getUser()->getUID()) { $node->unlock(ILockingProvider::LOCK_SHARED); return new Result(null, 403, $this->l->t('Only recipient can change accepted state')); } @@ -1025,8 +1026,8 @@ private function updateShareState($id, $state) { // we actually want to update all shares related to the node in case there are multiple // incoming shares for the same node (ex: receiving simultaneously through group share and user share) - $allShares = $this->shareManager->getSharedWith($this->currentUser->getUID(), Share::SHARE_TYPE_USER, $node, -1, 0); - $allShares = \array_merge($allShares, $this->shareManager->getSharedWith($this->currentUser->getUID(), Share::SHARE_TYPE_GROUP, $node, -1, 0)); + $allShares = $this->shareManager->getSharedWith($this->userSession->getUser()->getUID(), Share::SHARE_TYPE_USER, $node, -1, 0); + $allShares = \array_merge($allShares, $this->shareManager->getSharedWith($this->userSession->getUser()->getUID(), Share::SHARE_TYPE_GROUP, $node, -1, 0)); // resolve and deduplicate target if accepting if ($state === Share::STATE_ACCEPTED) { @@ -1039,7 +1040,7 @@ private function updateShareState($id, $state) { foreach ($allShares as $aShare) { $aShare->setState($share->getState()); $aShare->setTarget($share->getTarget()); - $this->shareManager->updateShareForRecipient($aShare, $this->currentUser->getUID()); + $this->shareManager->updateShareForRecipient($aShare, $this->userSession->getUser()->getUID()); } } catch (\Exception $e) { $share->getNode()->unlock(ILockingProvider::LOCK_SHARED); @@ -1052,9 +1053,9 @@ private function updateShareState($id, $state) { \OC\Files\Filesystem::tearDown(); // FIXME: trigger mount for user to make sure the new node is mounted already // before formatShare resolves it - $this->rootFolder->getUserFolder($this->currentUser->getUID()); + $this->rootFolder->getUserFolder($this->userSession->getUser()->getUID()); - $this->notificationPublisher->discardNotificationForUser($share, $this->currentUser->getUID()); + $this->notificationPublisher->discardNotificationForUser($share, $this->userSession->getUser()->getUID()); if ($eventName !== '') { $this->eventDispatcher->dispatch('share.after' . $eventName, new GenericEvent(null, ['share' => $share])); @@ -1070,7 +1071,7 @@ private function updateShareState($id, $state) { * @return IShare same share with target updated if necessary */ private function deduplicateShareTarget(IShare $share) { - $userFolder = $this->rootFolder->getUserFolder($this->currentUser->getUID()); + $userFolder = $this->rootFolder->getUserFolder($this->userSession->getUser()->getUID()); $mountPoint = \basename($share->getTarget()); $parentDir = \dirname($share->getTarget()); if (!$userFolder->nodeExists($parentDir)) { @@ -1108,21 +1109,21 @@ protected function canAccessShare(IShare $share) { } // Owner of the file and the sharer of the file can always get share - if ($share->getShareOwner() === $this->currentUser->getUID() || - $share->getSharedBy() === $this->currentUser->getUID() + if ($share->getShareOwner() === $this->userSession->getUser()->getUID() || + $share->getSharedBy() === $this->userSession->getUser()->getUID() ) { return true; } // If the share is shared with you (or a group you are a member of) if ($share->getShareType() === Share::SHARE_TYPE_USER && - $share->getSharedWith() === $this->currentUser->getUID()) { + $share->getSharedWith() === $this->userSession->getUser()->getUID()) { return true; } if ($share->getShareType() === Share::SHARE_TYPE_GROUP) { $sharedWith = $this->groupManager->get($share->getSharedWith()); - if ($sharedWith !== null && $sharedWith->inGroup($this->currentUser)) { + if ($sharedWith !== null && $sharedWith->inGroup($this->userSession->getUser())) { return true; } } diff --git a/apps/files_sharing/tests/ApiTest.php b/apps/files_sharing/tests/ApiTest.php index 67977d1d675e..4a6e0279aad9 100644 --- a/apps/files_sharing/tests/ApiTest.php +++ b/apps/files_sharing/tests/ApiTest.php @@ -31,6 +31,7 @@ use OCP\Constants; use OCP\IL10N; use OCP\IRequest; +use OCP\IUserSession; use OCP\Share; use OCA\Files_Sharing\Service\NotificationPublisher; use OCA\Files_Sharing\SharingBlacklist; @@ -108,6 +109,8 @@ private function createRequest(array $data) { */ private function createOCS($request, $userId) { $currentUser = \OC::$server->getUserManager()->get($userId); + $userSession = $this->createMock(IUserSession::class); + $userSession->method('getUser')->willReturn($currentUser); $l = $this->createMock(IL10N::class); $l->method('t') @@ -123,7 +126,7 @@ private function createOCS($request, $userId) { \OC::$server->getUserManager(), \OC::$server->getRootFolder(), \OC::$server->getURLGenerator(), - $currentUser, + $userSession, $l, \OC::$server->getConfig(), \OC::$server->getAppContainer('files_sharing')->query(NotificationPublisher::class), diff --git a/apps/files_sharing/tests/Controller/Share20OcsControllerTest.php b/apps/files_sharing/tests/Controller/Share20OcsControllerTest.php index b094429d5fda..41d06ac99b5c 100644 --- a/apps/files_sharing/tests/Controller/Share20OcsControllerTest.php +++ b/apps/files_sharing/tests/Controller/Share20OcsControllerTest.php @@ -39,6 +39,7 @@ use OCP\IUser; use OCP\IGroup; use OCP\IUserManager; +use OCP\IUserSession; use OCP\Lock\ILockingProvider; use OCP\Lock\LockedException; use OCP\Share; @@ -87,6 +88,9 @@ class Share20OcsControllerTest extends TestCase { /** @var IUser */ private $currentUser; + /** @var IUserSession */ + private $userSession; + /** @var Share20OcsController */ private $ocs; @@ -115,8 +119,10 @@ protected function setUp() { $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->currentUser = $this->createMock(IUser::class); $this->currentUser->method('getUID')->willReturn('currentUser'); + $this->userSession = $this->createMock(IUserSession::class); + $this->userSession->method('getUser')->willReturn($this->currentUser); - $this->userManager->expects($this->any())->method('userExists')->willReturn(true); + $this->userManager->method('userExists')->willReturn(true); $this->l = $this->createMock(IL10N::class); $this->l->method('t') @@ -143,7 +149,7 @@ protected function setUp() { $this->userManager, $this->rootFolder, $this->urlGenerator, - $this->currentUser, + $this->userSession, $this->l, $this->config, $this->notificationPublisher, @@ -169,7 +175,7 @@ private function mockFormatShare() { $this->userManager, $this->rootFolder, $this->urlGenerator, - $this->currentUser, + $this->userSession, $this->l, $this->config, $this->notificationPublisher, @@ -495,7 +501,7 @@ public function testGetShare(\OCP\Share\IShare $share, array $result) { $this->userManager, $this->rootFolder, $this->urlGenerator, - $this->currentUser, + $this->userSession, $this->l, $this->config, $this->notificationPublisher, @@ -2794,7 +2800,7 @@ public function getOcsDisabledAPI() { $this->userManager, $this->rootFolder, $this->urlGenerator, - $this->currentUser, + $this->userSession, $this->l, $this->config, $this->notificationPublisher, @@ -2888,7 +2894,7 @@ public function testGetShareAdditionalInfo($configValue, $expectedInfo) { $this->userManager, $this->rootFolder, $this->urlGenerator, - $this->currentUser, + $this->userSession, $this->l, $config, $this->notificationPublisher, diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index 79a6c1b79d7e..445c0cb398eb 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -31,7 +31,6 @@ namespace OC\AppFramework\DependencyInjection; use OC; -use OC\AppFramework\Core\API; use OC\AppFramework\Http; use OC\AppFramework\Http\Dispatcher; use OC\AppFramework\Http\Output; diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index 88d048542bd4..78726ed6f99d 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -33,13 +33,16 @@ use OC\AppFramework\Middleware\Security\Exceptions\NotLoggedInException; use OC\AppFramework\Utility\ControllerMethodReflector; use OC\Core\Controller\LoginController; +use OC\OCS\Result; use OC\Security\CSP\ContentSecurityPolicyManager; +use OCP\API; use OCP\AppFramework\Http\ContentSecurityPolicy; use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Http\TemplateResponse; use OCP\AppFramework\Middleware; use OCP\AppFramework\Http\Response; use OCP\AppFramework\Http\JSONResponse; +use OCP\AppFramework\OCSController; use OCP\INavigationManager; use OCP\IURLGenerator; use OCP\IRequest; @@ -184,6 +187,13 @@ public function afterController($controller, $methodName, Response $response) { * @return Response a Response object or null in case that the exception could not be handled */ public function afterException($controller, $methodName, \Exception $exception) { + if ($controller instanceof OCSController) { + if ($exception instanceof NotLoggedInException) { + return $controller->buildResponse(new Result(null, API::RESPOND_UNAUTHORISED, 'Unauthorised')); + } + return $controller->buildResponse(new Result(null, API::RESPOND_SERVER_ERROR, $exception->getMessage())); + } + if ($exception instanceof SecurityException) { if (\stripos($this->request->getHeader('Accept'), 'html') === false) { $response = new JSONResponse( diff --git a/lib/public/AppFramework/OCSController.php b/lib/public/AppFramework/OCSController.php index f6c06518fbf8..7a3a06c48036 100644 --- a/lib/public/AppFramework/OCSController.php +++ b/lib/public/AppFramework/OCSController.php @@ -33,6 +33,7 @@ namespace OCP\AppFramework; use OC\OCS\Result; +use OCP\API; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\Http\OCSResponse; use OCP\AppFramework\Http\Response; @@ -147,6 +148,11 @@ public function buildResponse($response, $format = 'json') { } } + if ($resp->getStatusCode() === API::RESPOND_UNAUTHORISED) { + // HTTP code + $resp->setStatus(401); + } + return $resp; } } diff --git a/tests/acceptance/features/apiAuth/ocsDELETEAuth.feature b/tests/acceptance/features/apiAuth/ocsDELETEAuth.feature index c2b2b2fad71c..eff986186c06 100644 --- a/tests/acceptance/features/apiAuth/ocsDELETEAuth.feature +++ b/tests/acceptance/features/apiAuth/ocsDELETEAuth.feature @@ -28,18 +28,8 @@ Feature: auth | 2 |/cloud/users/user0/groups | 997 | 401 | | 1 |/cloud/users/user0/subadmins | 997 | 401 | | 2 |/cloud/users/user0/subadmins | 997 | 401 | + | 1 |/apps/files_sharing/api/v1/shares/123 | 997 | 401 | + | 2 |/apps/files_sharing/api/v1/shares/123 | 997 | 401 | + | 1 |/apps/files_sharing/api/v1/shares/pending/123 | 997 | 401 | + | 2 |/apps/files_sharing/api/v1/shares/pending/123 | 997 | 401 | - #merge into previous scenario when fixed - @issue-34626 - Scenario Outline: send DELETE requests to OCS endpoints as admin with wrong password - Given using OCS API version "" - When the administrator sends HTTP method "DELETE" to OCS API endpoint "" using password "invalid" - Then the HTTP status code should be "200" - And the body of the response should be empty - #And the OCS status code should be "997" - Examples: - | ocs_api_version | endpoint | - | 1 | /apps/files_sharing/api/v1/shares/123 | - | 2 | /apps/files_sharing/api/v1/shares/123 | - | 1 | /apps/files_sharing/api/v1/shares/pending/123 | - | 2 | /apps/files_sharing/api/v1/shares/pending/123 | diff --git a/tests/acceptance/features/apiAuth/ocsGETAuth.feature b/tests/acceptance/features/apiAuth/ocsGETAuth.feature index c13f2828e060..ffd6f3d3b2a0 100644 --- a/tests/acceptance/features/apiAuth/ocsGETAuth.feature +++ b/tests/acceptance/features/apiAuth/ocsGETAuth.feature @@ -27,14 +27,8 @@ Feature: auth |/ocs/v2.php/config | 200 | 200 | |/ocs/v1.php/privatedata/getattribute | 997 | 401 | |/ocs/v2.php/privatedata/getattribute | 997 | 401 | - - #merge into previous scenario when fixed - @issue-34626 - Scenario: using OCS anonymously - When a user requests "/ocs/v1.php/apps/files_sharing/api/v1/shares" with "GET" and no authentication - Then the HTTP status code should be "200" - #Then the HTTP status code should be "401" - #And the OCS status code should be "997" + |/ocs/v1.php/apps/files_sharing/api/v1/shares | 997 | 401 | + |/ocs/v2.php/apps/files_sharing/api/v1/shares | 997 | 401 | @issue-32068 Scenario Outline: using OCS with non-admin basic auth @@ -86,19 +80,8 @@ Feature: auth | 2 |/config | 200 | 200 | | 1 |/privatedata/getattribute | 997 | 401 | | 2 |/privatedata/getattribute | 997 | 401 | - - #merge into previous scenario when fixed - @issue-34626 - Scenario Outline: using OCS as normal user with wrong password - Given using OCS API version "" - When user "user0" sends HTTP method "GET" to OCS API endpoint "/apps/files_sharing/api/v1/shares" using password "invalid" - Then the HTTP status code should be "200" - And the body of the response should be empty - #And the OCS status code should be "997" - Examples: - | ocs_api_version | - | 1 | - | 2 | + | 1 |/apps/files_sharing/api/v1/shares | 997 | 401 | + | 2 |/apps/files_sharing/api/v1/shares | 997 | 401 | Scenario Outline: using OCS with admin basic auth When the administrator requests "" with "GET" using basic auth @@ -134,19 +117,8 @@ Feature: auth | 2 |/cloud/users | 997 | 401 | | 1 |/privatedata/getattribute | 997 | 401 | | 2 |/privatedata/getattribute | 997 | 401 | - - #merge into previous scenario when fixed - @issue-34626 - Scenario Outline: using OCS as admin user with wrong password - Given using OCS API version "" - When the administrator sends HTTP method "GET" to OCS API endpoint "/apps/files_sharing/api/v1/shares" using password "invalid" - Then the HTTP status code should be "200" - And the body of the response should be empty - #And the OCS status code should be "997" - Examples: - | ocs_api_version | - | 1 | - | 2 | + | 1 |/apps/files_sharing/api/v1/shares | 997 | 401 | + | 2 |/apps/files_sharing/api/v1/shares | 997 | 401 | Scenario Outline: using OCS with token auth of a normal user When user "user0" requests "" with "GET" using basic token auth diff --git a/tests/acceptance/features/apiAuth/ocsPOSTAuth.feature b/tests/acceptance/features/apiAuth/ocsPOSTAuth.feature index 550f006f3394..470ea62e1e4e 100644 --- a/tests/acceptance/features/apiAuth/ocsPOSTAuth.feature +++ b/tests/acceptance/features/apiAuth/ocsPOSTAuth.feature @@ -33,19 +33,8 @@ Feature: auth | 2 |/privatedata/deleteattribute/testing/test | 997 | 401 | | 1 |/privatedata/setattribute/testing/test | 997 | 401 | | 2 |/privatedata/setattribute/testing/test | 997 | 401 | + | 1 |/apps/files_sharing/api/v1/shares | 997 | 401 | + | 2 |/apps/files_sharing/api/v1/shares | 997 | 401 | + | 1 |/apps/files_sharing/api/v1/shares/pending/123 | 997 | 401 | + | 2 |/apps/files_sharing/api/v1/shares/pending/123 | 997 | 401 | - #merge into previous scenario when fixed - @issue-34626 - Scenario Outline: send POST requests to OCS endpoints as normal user with wrong password - Given using OCS API version "" - When user "user0" sends HTTP method "POST" to OCS API endpoint "" with body using password "invalid" - | data | doesnotmatter | - Then the HTTP status code should be "200" - And the body of the response should be empty - #And the OCS status code should be "997" - Examples: - | ocs_api_version | endpoint | - | 1 | /apps/files_sharing/api/v1/shares | - | 2 | /apps/files_sharing/api/v1/shares | - | 1 | /apps/files_sharing/api/v1/shares/pending/123 | - | 2 | /apps/files_sharing/api/v1/shares/pending/123 | diff --git a/tests/acceptance/features/apiAuth/ocsPUTAuth.feature b/tests/acceptance/features/apiAuth/ocsPUTAuth.feature index f73dc6c5b5e1..a92409a25a0d 100644 --- a/tests/acceptance/features/apiAuth/ocsPUTAuth.feature +++ b/tests/acceptance/features/apiAuth/ocsPUTAuth.feature @@ -14,23 +14,12 @@ Feature: auth And the HTTP status code should be "" Examples: | ocs_api_version |endpoint | ocs-code | http-code | - | 1 |/cloud/users/user0 | 997 | 401 | - | 2 |/cloud/users/user0 | 997 | 401 | - | 1 |/cloud/users/user0/disable | 997 | 401 | - | 2 |/cloud/users/user0/disable | 997 | 401 | - | 1 |/cloud/users/user0/enable | 997 | 401 | - | 2 |/cloud/users/user0/enable | 997 | 401 | + | 1 |/cloud/users/user0 | 997 | 401 | + | 2 |/cloud/users/user0 | 997 | 401 | + | 1 |/cloud/users/user0/disable | 997 | 401 | + | 2 |/cloud/users/user0/disable | 997 | 401 | + | 1 |/cloud/users/user0/enable | 997 | 401 | + | 2 |/cloud/users/user0/enable | 997 | 401 | + | 1 |/apps/files_sharing/api/v1/shares/123 | 997 | 401 | + | 2 |/apps/files_sharing/api/v1/shares/123 | 997 | 401 | - #merge into previous scenario when fixed - @issue-34626 - Scenario Outline: send PUT requests to OCS endpoints as admin with wrong password - Given using OCS API version "" - When the administrator sends HTTP method "PUT" to OCS API endpoint "/apps/files_sharing/api/v1/shares/123" with body using password "invalid" - | data | doesnotmatter | - Then the HTTP status code should be "200" - And the body of the response should be empty - #And the OCS status code should be "997" - Examples: - | ocs_api_version | - | 1 | - | 2 |