share manager should emit share.link auth events to catch all accesses

owncloud · May 22, 2020 · 59dfc8e · 59dfc8e
1 parent 1d6094c
commit 59dfc8e
Show file tree

Hide file tree

Showing 5 changed files with 86 additions and 70 deletions.
diff --git a/apps/files_sharing/lib/Controllers/ShareController.php b/apps/files_sharing/lib/Controllers/ShareController.php
@@ -182,13 +182,10 @@ public function authenticate($token, $password = '') {
 	 * @return bool
 	 */
 	private function linkShareAuth(\OCP\Share\IShare $share, $password = null) {
-		$beforeEvent = new GenericEvent(null, ['shareObject' => $share]);
-		$this->eventDispatcher->dispatch('share.beforelinkauth', $beforeEvent);
 		if ($password !== null) {
 			if ($this->shareManager->checkPassword($share, $password)) {
 				$this->session->set('public_link_authenticated', (string)$share->getId());
 			} else {
-				$this->emitAccessShareHook($share, 403, 'Wrong password');
 				return false;
 			}
 		} else {
@@ -198,8 +195,6 @@ private function linkShareAuth(\OCP\Share\IShare $share, $password = null) {
 				return false;
 			}
 		}
-		$afterEvent = new GenericEvent(null, ['shareObject' => $share]);
-		$this->eventDispatcher->dispatch('share.afterlinkauth', $afterEvent);
 		return true;
 	}
 

diff --git a/apps/files_sharing/tests/Controllers/ShareControllerTest.php b/apps/files_sharing/tests/Controllers/ShareControllerTest.php
@@ -30,7 +30,6 @@
 namespace OCA\Files_Sharing\Tests\Controllers;
 
 use OC\Files\Filesystem;
-use OCA\FederatedFileSharing\FederatedShareProvider;
 use OCA\Files_Sharing\Controllers\ShareController;
 use OCP\AppFramework\Http\NotFoundResponse;
 use OCP\AppFramework\Http\RedirectResponse;
@@ -41,7 +40,6 @@
 use OCP\Security\ISecureRandom;
 use OCP\Share\Exceptions\ShareNotFound;
 use Symfony\Component\EventDispatcher\EventDispatcher;
-use Symfony\Component\EventDispatcher\GenericEvent;
 
 /**
  * @group DB
@@ -223,24 +221,9 @@ public function testAuthenticateValidPassword() {
 			->with('files_sharing.sharecontroller.showShare', ['token'=>'token'])
 			->willReturn('redirect');
 
-		$beforeLinkAuthCalled = false;
-		$this->eventDispatcher->addListener(
-			'share.beforelinkauth', function () use (&$beforeLinkAuthCalled) {
-				$beforeLinkAuthCalled = true;
-			}
-		);
-		$afterLinkAuthCalled = false;
-		$this->eventDispatcher->addListener(
-			'share.afterlinkauth', function () use (&$afterLinkAuthCalled) {
-				$afterLinkAuthCalled = true;
-			}
-		);
-
 		$response = $this->shareController->authenticate('token', 'validpassword');
 		$expectedResponse =  new RedirectResponse('redirect');
 		$this->assertEquals($expectedResponse, $response);
-		$this->assertEquals(true, $beforeLinkAuthCalled);
-		$this->assertEquals(true, $afterLinkAuthCalled);
 	}
 
 	public function testAuthenticateInvalidPassword() {
@@ -267,55 +250,9 @@ public function testAuthenticateInvalidPassword() {
 			->expects($this->never())
 			->method('set');
 
-		$hookListner = $this->getMockBuilder('Dummy')->setMethods(['access'])->getMock();
-		\OCP\Util::connectHook('OCP\Share', 'share_link_access', $hookListner, 'access');
-
-		$calledShareLinkAccess = [];
-		$this->eventDispatcher->addListener('share.linkaccess',
-			function (GenericEvent $event) use (&$calledShareLinkAccess) {
-				$calledShareLinkAccess[] = 'share.linkaccess';
-				$calledShareLinkAccess[] = $event;
-			});
-
-		$beforeLinkAuthCalled = false;
-		$this->eventDispatcher->addListener(
-			'share.beforelinkauth', function () use (&$beforeLinkAuthCalled) {
-				$beforeLinkAuthCalled = true;
-			}
-		);
-		$afterLinkAuthCalled = false;
-		$this->eventDispatcher->addListener(
-			'share.afterlinkauth', function () use (&$afterLinkAuthCalled) {
-				$afterLinkAuthCalled = true;
-			}
-		);
-
-		$hookListner->expects($this->once())
-			->method('access')
-			->with($this->callback(function (array $data) {
-				return $data['itemType'] === 'file' &&
-					$data['itemSource'] === 100 &&
-					$data['uidOwner'] === 'initiator' &&
-					$data['token'] === 'token' &&
-					$data['errorCode'] === 403 &&
-					$data['errorMessage'] === 'Wrong password';
-			}));
-
 		$response = $this->shareController->authenticate('token', 'invalidpassword');
 		$expectedResponse =  new TemplateResponse($this->appName, 'authenticate', ['wrongpw' => true], 'guest');
 		$this->assertEquals($expectedResponse, $response);
-
-		$this->assertEquals('share.linkaccess', $calledShareLinkAccess[0]);
-		$this->assertInstanceOf(GenericEvent::class, $calledShareLinkAccess[1]);
-		$this->assertArrayHasKey('shareObject', $calledShareLinkAccess[1]);
-		$this->assertEquals('42', $calledShareLinkAccess[1]->getArgument('shareObject')->getId());
-		$this->assertEquals('file', $calledShareLinkAccess[1]->getArgument('shareObject')->getNodeType());
-		$this->assertEquals('initiator', $calledShareLinkAccess[1]->getArgument('shareObject')->getSharedBy());
-		$this->assertEquals('token', $calledShareLinkAccess[1]->getArgument('shareObject')->getToken());
-		$this->assertEquals(403, $calledShareLinkAccess[1]->getArgument('errorCode'));
-		$this->assertEquals('Wrong password', $calledShareLinkAccess[1]->getArgument('errorMessage'));
-		$this->assertEquals(true, $beforeLinkAuthCalled);
-		$this->assertEquals(false, $afterLinkAuthCalled);
 	}
 
 	public function testShowShareInvalidToken() {

diff --git a/changelog/unreleased/37430 b/changelog/unreleased/37430
@@ -0,0 +1,6 @@
+Bugfix: Emit link share auth events on DAV access to password-protected links
+
+DAV access for the password-protected links was not emitting link share auth events that need to be emitted.
+This problem has been fixed.
+
+https://github.com/owncloud/core/pull/37430
diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php
@@ -1490,10 +1490,31 @@ public function checkPassword(\OCP\Share\IShare $share, $password) {
 		}
 
 		$newHash = '';
+		$beforeEvent = new GenericEvent(null, ['shareObject' => $share]);
+		$this->eventDispatcher->dispatch('share.beforelinkauth', $beforeEvent);
 		if (!$this->hasher->verify($password, $share->getPassword(), $newHash)) {
+			$token = $share->getToken();
+			$uidOwner = $share->getSharedBy();
+			$itemType = $share->getNodeType();
+			$itemSource = $share->getNodeId();
+			$errorCode = 403;
+			$errorMessage = 'Wrong password';
+			\OC_Hook::emit('OCP\Share', 'share_link_access', [
+				'itemType' => $itemType,
+				'itemSource' => $itemSource,
+				'uidOwner' => $uidOwner,
+				'token' => $token,
+				'errorCode' => $errorCode,
+				'errorMessage' => $errorMessage,
+			]);
+			$publicShareLinkAccessEvent = new GenericEvent(null,
+				['shareObject' => $share, 'errorCode' => $errorCode,
+					'errorMessage' => $errorMessage]);
+			$this->eventDispatcher->dispatch('share.linkaccess', $publicShareLinkAccessEvent);
 			return false;
 		}
-
+		$afterEvent = new GenericEvent(null, ['shareObject' => $share]);
+		$this->eventDispatcher->dispatch('share.afterlinkauth', $afterEvent);
 		if (!empty($newHash)) {
 			$share->setPassword($newHash);
 			$provider = $this->factory->getProviderForType($share->getShareType());

diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php
@@ -3148,20 +3148,77 @@ public function testCheckPasswordInvalidPassword() {
 		$share = $this->createMock('\OCP\Share\IShare');
 		$share->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_LINK);
 		$share->method('getPassword')->willReturn('password');
+		$share->method('getNodeId')->willReturn(100);
+		$share->method('getNodeType')->willReturn('file');
+		$share->method('getSharedBy')->willReturn('initiator');
+		$share->method('getToken')->willReturn('token');
+
+		$calledBeforeEvent = [];
+		$this->eventDispatcher->addListener('share.beforelinkauth',
+			function (GenericEvent $event) use (&$calledBeforeEvent) {
+				$calledBeforeEvent[] = 'share.beforelinkauth';
+				$calledBeforeEvent[] = $event;
+			});
+
+		$hookListener = $this->getMockBuilder('Dummy')->setMethods(['access'])->getMock();
+		\OCP\Util::connectHook('OCP\Share', 'share_link_access', $hookListener, 'access');
+		$calledShareLinkAccess = [];
+		$this->eventDispatcher->addListener('share.linkaccess',
+			function (GenericEvent $event) use (&$calledShareLinkAccess) {
+				$calledShareLinkAccess[] = 'share.linkaccess';
+				$calledShareLinkAccess[] = $event;
+			});
+		$hookListener->expects($this->once())
+			->method('access')
+			->with($this->callback(function (array $data) {
+				return $data['itemType'] === 'file' &&
+					$data['itemSource'] === 100 &&
+					$data['uidOwner'] === 'initiator' &&
+					$data['token'] === 'token' &&
+					$data['errorCode'] === 403 &&
+					$data['errorMessage'] === 'Wrong password';
+			}));
 
 		$this->hasher->method('verify')->with('invalidpassword', 'password', '')->willReturn(false);
 
 		$this->assertFalse($this->manager->checkPassword($share, 'invalidpassword'));
+
+		$this->assertEquals('share.linkaccess', $calledShareLinkAccess[0]);
+		$this->assertInstanceOf(GenericEvent::class, $calledShareLinkAccess[1]);
+		$this->assertArrayHasKey('shareObject', $calledShareLinkAccess[1]);
+		$this->assertEquals('100', $calledShareLinkAccess[1]->getArgument('shareObject')->getNodeId());
+		$this->assertEquals('file', $calledShareLinkAccess[1]->getArgument('shareObject')->getNodeType());
+		$this->assertEquals('initiator', $calledShareLinkAccess[1]->getArgument('shareObject')->getSharedBy());
+		$this->assertEquals('token', $calledShareLinkAccess[1]->getArgument('shareObject')->getToken());
+		$this->assertEquals(403, $calledShareLinkAccess[1]->getArgument('errorCode'));
+		$this->assertEquals('Wrong password', $calledShareLinkAccess[1]->getArgument('errorMessage'));
+		$this->assertEquals('share.beforelinkauth', $calledBeforeEvent[0]);
+		$this->assertInstanceOf(GenericEvent::class, $calledBeforeEvent[1]);
 	}
 
 	public function testCheckPasswordValidPassword() {
 		$share = $this->createMock('\OCP\Share\IShare');
 		$share->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_LINK);
 		$share->method('getPassword')->willReturn('passwordHash');
-
+		$calledBeforeEvent = [];
+		$this->eventDispatcher->addListener('share.beforelinkauth',
+			function (GenericEvent $event) use (&$calledBeforeEvent) {
+				$calledBeforeEvent[] = 'share.beforelinkauth';
+				$calledBeforeEvent[] = $event;
+			});
+		$calledAfterEvent = [];
+		$this->eventDispatcher->addListener('share.afterlinkauth',
+			function (GenericEvent $event) use (&$calledAfterEvent) {
+				$calledAfterEvent[] = 'share.afterlinkauth';
+				$calledAfterEvent[] = $event;
+			});
 		$this->hasher->method('verify')->with('password', 'passwordHash', '')->willReturn(true);
 
 		$this->assertTrue($this->manager->checkPassword($share, 'password'));
+		$this->assertEquals('share.beforelinkauth', $calledBeforeEvent[0]);
+		$this->assertEquals('share.afterlinkauth', $calledAfterEvent[0]);
+		$this->assertInstanceOf(GenericEvent::class, $calledBeforeEvent[1]);
+		$this->assertInstanceOf(GenericEvent::class, $calledAfterEvent[1]);
 	}
 
 	public function testCheckPasswordUpdateShare() {