Merge pull request #2955 from nextcloud/make-share-by-mail-work-without-linkshares

share by mail should continue to work, even if public links are disabled
2017-01-10 13:06:20 +01:00 · 2017-01-10 13:06:20 +01:00 · a5bf14ada3
parent 3fa715c2ab 0c11209d33
commit a5bf14ada3
5 changed files with 171 additions and 8 deletions
--- a/apps/files_sharing/lib/AppInfo/Application.php
+++ b/apps/files_sharing/lib/AppInfo/Application.php
@ -111,7 +111,9 @@ class Application extends App {
 				$c->query('AppName'),
 				$server->getConfig(),
 				$server->getAppManager(),
-				$c['ControllerMethodReflector']
+				$c['ControllerMethodReflector'],
+				$server->getShareManager(),
+				$server->getRequest()
 			);
 		});

--- a/apps/files_sharing/lib/Middleware/SharingCheckMiddleware.php
+++ b/apps/files_sharing/lib/Middleware/SharingCheckMiddleware.php
@ -25,6 +25,8 @@

 namespace OCA\Files_Sharing\Middleware;

+use OCA\Files_Sharing\Controller\ExternalSharesController;
+use OCA\Files_Sharing\Controller\ShareController;
 use OCP\App\IAppManager;
 use OCP\AppFramework\Http\NotFoundResponse;
 use OCP\AppFramework\Middleware;
@ -33,6 +35,8 @@ use OCP\IConfig;
 use OCP\AppFramework\Utility\IControllerMethodReflector;
 use OCA\Files_Sharing\Exceptions\S2SException;
 use OCP\AppFramework\Http\JSONResponse;
+use OCP\IRequest;
+use OCP\Share\IManager;

 /**
 * Checks whether the "sharing check" is enabled
@ -49,21 +53,32 @@ class SharingCheckMiddleware extends Middleware {
 	protected $appManager;
 	/** @var IControllerMethodReflector */
 	protected $reflector;
+	/** @var IManager */
+	protected $shareManager;
+	/** @var IRequest */
+	protected $request;

 	/***
 	 * @param string $appName
 	 * @param IConfig $config
 	 * @param IAppManager $appManager
+	 * @param IControllerMethodReflector $reflector
+	 * @param IManager $shareManager
+	 * @param IRequest $request
 	 */
 	public function __construct($appName,
 								IConfig $config,
 								IAppManager $appManager,
-								IControllerMethodReflector $reflector
+								IControllerMethodReflector $reflector,
+								IManager $shareManager,
+								IRequest $request
 								) {
 		$this->appName = $appName;
 		$this->config = $config;
 		$this->appManager = $appManager;
 		$this->reflector = $reflector;
+		$this->shareManager = $shareManager;
+		$this->request = $request;
 	}

 	/**
@ -72,18 +87,23 @@ class SharingCheckMiddleware extends Middleware {
 	 * @param \OCP\AppFramework\Controller $controller
 	 * @param string $methodName
 	 * @throws NotFoundException
+	 * @throws S2SException
 	 */
 	public function beforeController($controller, $methodName) {
 		if(!$this->isSharingEnabled()) {
 			throw new NotFoundException('Sharing is disabled.');
 		}

-		if ($controller instanceof \OCA\Files_Sharing\Controller\ExternalSharesController &&
+		if ($controller instanceof ExternalSharesController &&
 			!$this->externalSharesChecks()) {
 			throw new S2SException('Federated sharing not allowed');
-		} else if ($controller instanceof \OCA\Files_Sharing\Controller\ShareController &&
-			!$this->isLinkSharingEnabled()) {
-			throw new NotFoundException('Link sharing is disabled');
+		} else if ($controller instanceof ShareController) {
+			$token = $this->request->getParam('token');
+			$share = $this->shareManager->getShareByToken($token);
+			if ($share->getShareType() === \OCP\Share::SHARE_TYPE_LINK
+				&& !$this->isLinkSharingEnabled()) {
+				throw new NotFoundException('Link sharing is disabled');
+			}
 		}
 	}

--- a/apps/files_sharing/tests/Middleware/SharingCheckMiddlewareTest.php
+++ b/apps/files_sharing/tests/Middleware/SharingCheckMiddlewareTest.php
@ -34,6 +34,9 @@ use OCP\AppFramework\Utility\IControllerMethodReflector;
 use OCA\Files_Sharing\Exceptions\S2SException;
 use OCP\AppFramework\Http\JSONResponse;
 use OCP\IConfig;
+use OCP\IRequest;
+use OCP\Share\IManager;
+use OCP\Share\IShare;

 /**
 * @package OCA\Files_Sharing\Middleware\SharingCheckMiddleware
@ -50,6 +53,10 @@ class SharingCheckMiddlewareTest extends \Test\TestCase {
 	private $controllerMock;
 	/** @var IControllerMethodReflector|\PHPUnit_Framework_MockObject_MockObject */
 	private $reflector;
+	/** @var  IManager | \PHPUnit_Framework_MockObject_MockObject */
+	private $shareManager;
+	/** @var  IRequest | \PHPUnit_Framework_MockObject_MockObject */
+	private $request;

 	protected function setUp() {
 		parent::setUp();
@ -58,12 +65,16 @@ class SharingCheckMiddlewareTest extends \Test\TestCase {
 		$this->appManager = $this->createMock(IAppManager::class);
 		$this->controllerMock = $this->createMock(Controller::class);
 		$this->reflector = $this->createMock(IControllerMethodReflector::class);
+		$this->shareManager = $this->createMock(IManager::class);
+		$this->request = $this->createMock(IRequest::class);

 		$this->sharingCheckMiddleware = new SharingCheckMiddleware(
 			'files_sharing',
 			$this->config,
 			$this->appManager,
-			$this->reflector);
+			$this->reflector,
+			$this->shareManager,
+			$this->request);
 	}

 	public function testIsSharingEnabledWithAppEnabled() {
@ -215,6 +226,9 @@ class SharingCheckMiddlewareTest extends \Test\TestCase {
 	}

 	public function testBeforeControllerWithShareControllerWithSharingEnabled() {
+
+		$share = $this->createMock(IShare::class);
+
 		$this->appManager
 			->expects($this->once())
 			->method('isEnabledForUser')
@ -233,6 +247,13 @@ class SharingCheckMiddlewareTest extends \Test\TestCase {
 			->with('core', 'shareapi_allow_links', 'yes')
 			->will($this->returnValue('yes'));

+		$this->request->expects($this->once())->method('getParam')->with('token')
+			->willReturn('token');
+		$this->shareManager->expects($this->once())->method('getShareByToken')
+			->with('token')->willReturn($share);
+
+		$share->expects($this->once())->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_LINK);
+
 		$controller = $this->createMock(ShareController::class);

 		$this->sharingCheckMiddleware->beforeController($controller, 'myMethod');
@ -243,6 +264,9 @@ class SharingCheckMiddlewareTest extends \Test\TestCase {
 	 * @expectedExceptionMessage Link sharing is disabled
 	 */
 	public function testBeforeControllerWithShareControllerWithSharingEnabledAPIDisabled() {
+
+		$share = $this->createMock(IShare::class);
+
 		$this->appManager
 			->expects($this->once())
 			->method('isEnabledForUser')
@ -251,6 +275,14 @@ class SharingCheckMiddlewareTest extends \Test\TestCase {

 		$controller = $this->createMock(ShareController::class);

+		$this->request->expects($this->once())->method('getParam')->with('token')
+			->willReturn('token');
+		$this->shareManager->expects($this->once())->method('getShareByToken')
+			->with('token')->willReturn($share);
+
+		$share->expects($this->once())->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_LINK);
+
+
 		$this->sharingCheckMiddleware->beforeController($controller, 'myMethod');
 	}

--- a/lib/private/Share20/Manager.php
+++ b/lib/private/Share20/Manager.php
@ -587,7 +587,6 @@ class Manager implements IManager {
 				$share->setPassword($this->hasher->hash($share->getPassword()));
 			}
 		} else if ($share->getShareType() === \OCP\Share::SHARE_TYPE_EMAIL) {
-			$this->linkCreateChecks($share);
 			$share->setToken(
 				$this->secureRandom->generate(
 					\OC\Share\Constants::TOKEN_LENGTH,
--- a/tests/lib/Share20/ManagerTest.php
+++ b/tests/lib/Share20/ManagerTest.php
@ -1768,6 +1768,116 @@ class ManagerTest extends \Test\TestCase {
 		$this->assertEquals('hashed', $share->getPassword());
 	}

+	public function testCreateShareMail() {
+		$manager = $this->createManagerMock()
+			->setMethods([
+				'canShare',
+				'generalCreateChecks',
+				'linkCreateChecks',
+				'pathCreateChecks',
+				'validateExpirationDate',
+				'verifyPassword',
+				'setLinkParent',
+			])
+			->getMock();
+
+		$shareOwner = $this->createMock(IUser::class);
+		$shareOwner->method('getUID')->willReturn('shareOwner');
+
+		$storage = $this->createMock(Storage::class);
+		$path = $this->createMock(File::class);
+		$path->method('getOwner')->willReturn($shareOwner);
+		$path->method('getName')->willReturn('target');
+		$path->method('getId')->willReturn(1);
+		$path->method('getStorage')->willReturn($storage);
+
+		$share = $this->manager->newShare();
+		$share->setShareType(\OCP\Share::SHARE_TYPE_EMAIL)
+			->setNode($path)
+			->setSharedBy('sharedBy')
+			->setPermissions(\OCP\Constants::PERMISSION_ALL);
+
+		$manager->expects($this->once())
+			->method('canShare')
+			->with($share)
+			->willReturn(true);
+		$manager->expects($this->once())
+			->method('generalCreateChecks')
+			->with($share);;
+		$manager->expects($this->never())
+			->method('linkCreateChecks');
+		$manager->expects($this->once())
+			->method('pathCreateChecks')
+			->with($path);
+		$manager->expects($this->never())
+			->method('validateExpirationDate');
+		$manager->expects($this->never())
+			->method('verifyPassword');
+		$manager->expects($this->never())
+			->method('setLinkParent');
+
+		$this->secureRandom->method('getMediumStrengthGenerator')
+			->will($this->returnSelf());
+		$this->secureRandom->method('generate')
+			->willReturn('token');
+
+		$this->defaultProvider
+			->expects($this->once())
+			->method('create')
+			->with($share)
+			->will($this->returnCallback(function(Share $share) {
+				return $share->setId(42);
+			}));
+
+		$hookListner = $this->getMockBuilder('Dummy')->setMethods(['pre', 'post'])->getMock();
+		\OCP\Util::connectHook('OCP\Share', 'pre_shared',  $hookListner, 'pre');
+		\OCP\Util::connectHook('OCP\Share', 'post_shared', $hookListner, 'post');
+
+		$hookListnerExpectsPre = [
+			'itemType' => 'file',
+			'itemSource' => 1,
+			'shareType' => \OCP\Share::SHARE_TYPE_EMAIL,
+			'uidOwner' => 'sharedBy',
+			'permissions' => 31,
+			'fileSource' => 1,
+			'expiration' => null,
+			'token' => 'token',
+			'run' => true,
+			'error' => '',
+			'itemTarget' => '/target',
+			'shareWith' => null,
+		];
+
+		$hookListnerExpectsPost = [
+			'itemType' => 'file',
+			'itemSource' => 1,
+			'shareType' => \OCP\Share::SHARE_TYPE_EMAIL,
+			'uidOwner' => 'sharedBy',
+			'permissions' => 31,
+			'fileSource' => 1,
+			'expiration' => null,
+			'token' => 'token',
+			'id' => 42,
+			'itemTarget' => '/target',
+			'fileTarget' => '/target',
+			'shareWith' => null,
+		];
+
+		$hookListner->expects($this->once())
+			->method('pre')
+			->with($this->equalTo($hookListnerExpectsPre));
+		$hookListner->expects($this->once())
+			->method('post')
+			->with($this->equalTo($hookListnerExpectsPost));
+
+		/** @var IShare $share */
+		$share = $manager->createShare($share);
+
+		$this->assertSame('shareOwner', $share->getShareOwner());
+		$this->assertEquals('/target', $share->getTarget());
+		$this->assertEquals('token', $share->getToken());
+	}
+
 	/**
 	 * @expectedException Exception
 	 * @expectedExceptionMessage I won't let you share