From 599161f881e5300af079b1ccd8bea6a92f38f7fd Mon Sep 17 00:00:00 2001 From: Semih Serhat Karakaya Date: Thu, 21 May 2020 16:19:43 -0700 Subject: [PATCH] share manager should emit share.link auth events to catch all accesses --- .../lib/Controllers/ShareController.php | 5 -- .../tests/Controllers/ShareControllerTest.php | 63 ------------------- lib/private/Share20/Manager.php | 23 ++++++- tests/lib/Share20/ManagerTest.php | 59 ++++++++++++++++- 4 files changed, 80 insertions(+), 70 deletions(-) diff --git a/apps/files_sharing/lib/Controllers/ShareController.php b/apps/files_sharing/lib/Controllers/ShareController.php index feee1b6decb7..2656c24dff21 100644 --- 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 index 91135223d2b6..cd138c371daa 100644 --- 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/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 34f58733ce72..3ffb1e0fd86e 100644 --- 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 index 4389517d0305..e0016e555f6e 100644 --- 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() {