From fb890807c0c17549566690512549c8d4ae5813e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20Molakvo=C3=A6=20=28skjnldsv=29?= Date: Wed, 28 Feb 2018 17:05:55 +0100 Subject: [PATCH 1/8] Sharing: redirect to download after authentification if requested MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: John Molakvoæ (skjnldsv) --- apps/files_sharing/lib/Controller/ShareController.php | 11 +++++++---- core/routes.php | 4 ++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareController.php b/apps/files_sharing/lib/Controller/ShareController.php index 1f8864fc5f3..856f001bbb4 100644 --- a/apps/files_sharing/lib/Controller/ShareController.php +++ b/apps/files_sharing/lib/Controller/ShareController.php @@ -170,10 +170,11 @@ class ShareController extends Controller { * * Authenticates against password-protected shares * @param string $token + * @param string $redirect * @param string $password * @return RedirectResponse|TemplateResponse|NotFoundResponse */ - public function authenticate($token, $password = '') { + public function authenticate($token, $redirect, $password = '') { // Check whether share exists try { @@ -184,7 +185,9 @@ class ShareController extends Controller { $authenticate = $this->linkShareAuth($share, $password); - if($authenticate === true) { + if ($authenticate === true && $redirect === 'download') { + return new RedirectResponse($this->urlGenerator->linkToRoute('files_sharing.sharecontroller.downloadShare', array('token' => $token))); + } else if ($authenticate === true) { return new RedirectResponse($this->urlGenerator->linkToRoute('files_sharing.sharecontroller.showShare', array('token' => $token))); } @@ -294,7 +297,7 @@ class ShareController extends Controller { // Share is password protected - check whether the user is permitted to access the share if ($share->getPassword() !== null && !$this->linkShareAuth($share)) { return new RedirectResponse($this->urlGenerator->linkToRoute('files_sharing.sharecontroller.authenticate', - array('token' => $token))); + array('token' => $token, 'redirect' => 'preview'))); } if (!$this->validateShare($share)) { @@ -480,7 +483,7 @@ class ShareController extends Controller { // Share is password protected - check whether the user is permitted to access the share if ($share->getPassword() !== null && !$this->linkShareAuth($share)) { return new RedirectResponse($this->urlGenerator->linkToRoute('files_sharing.sharecontroller.authenticate', - ['token' => $token])); + ['token' => $token, 'redirect' => 'download'])); } $files_list = null; diff --git a/core/routes.php b/core/routes.php index 97a8621fc39..d357fd45f96 100644 --- a/core/routes.php +++ b/core/routes.php @@ -116,7 +116,7 @@ $this->create('files_sharing.sharecontroller.showShare', '/s/{token}')->action(f throw new \OC\HintException('App file sharing is not enabled'); } }); -$this->create('files_sharing.sharecontroller.authenticate', '/s/{token}/authenticate')->post()->action(function($urlParams) { +$this->create('files_sharing.sharecontroller.authenticate', '/s/{token}/authenticate/{redirect}')->post()->action(function($urlParams) { if (class_exists(\OCA\Files_Sharing\AppInfo\Application::class, false)) { $app = new \OCA\Files_Sharing\AppInfo\Application($urlParams); $app->dispatch('ShareController', 'authenticate'); @@ -124,7 +124,7 @@ $this->create('files_sharing.sharecontroller.authenticate', '/s/{token}/authenti throw new \OC\HintException('App file sharing is not enabled'); } }); -$this->create('files_sharing.sharecontroller.showAuthenticate', '/s/{token}/authenticate')->get()->action(function($urlParams) { +$this->create('files_sharing.sharecontroller.showAuthenticate', '/s/{token}/authenticate/{redirect}')->get()->action(function($urlParams) { if (class_exists(\OCA\Files_Sharing\AppInfo\Application::class, false)) { $app = new \OCA\Files_Sharing\AppInfo\Application($urlParams); $app->dispatch('ShareController', 'showAuthenticate'); From a0641e43dc0db58bd31bc097b15c19784e09f50b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20Molakvo=C3=A6=20=28skjnldsv=29?= Date: Wed, 28 Feb 2018 17:08:25 +0100 Subject: [PATCH 2/8] fixup! Sharing: redirect to download after authentification if requested MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: John Molakvoæ (skjnldsv) --- apps/files_sharing/lib/Controller/ShareController.php | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareController.php b/apps/files_sharing/lib/Controller/ShareController.php index 856f001bbb4..f793d35e3ae 100644 --- a/apps/files_sharing/lib/Controller/ShareController.php +++ b/apps/files_sharing/lib/Controller/ShareController.php @@ -185,10 +185,17 @@ class ShareController extends Controller { $authenticate = $this->linkShareAuth($share, $password); + // if download was requested before auth, redirect to download if ($authenticate === true && $redirect === 'download') { - return new RedirectResponse($this->urlGenerator->linkToRoute('files_sharing.sharecontroller.downloadShare', array('token' => $token))); + return new RedirectResponse($this->urlGenerator->linkToRoute( + 'files_sharing.sharecontroller.downloadShare', + array('token' => $token)) + ); } else if ($authenticate === true) { - return new RedirectResponse($this->urlGenerator->linkToRoute('files_sharing.sharecontroller.showShare', array('token' => $token))); + return new RedirectResponse($this->urlGenerator->linkToRoute( + 'files_sharing.sharecontroller.showShare', + array('token' => $token)) + ); } $response = new TemplateResponse($this->appName, 'authenticate', array('wrongpw' => true), 'guest'); From 8c69d783e0f32394d1d37c059bd55e648dcbba93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20Molakvo=C3=A6=20=28skjnldsv=29?= Date: Wed, 28 Feb 2018 17:35:42 +0100 Subject: [PATCH 3/8] Fixed tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: John Molakvoæ (skjnldsv) --- apps/files_sharing/lib/Controller/ShareController.php | 2 +- .../tests/Controller/ShareControllerTest.php | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareController.php b/apps/files_sharing/lib/Controller/ShareController.php index f793d35e3ae..c4d9e739658 100644 --- a/apps/files_sharing/lib/Controller/ShareController.php +++ b/apps/files_sharing/lib/Controller/ShareController.php @@ -174,7 +174,7 @@ class ShareController extends Controller { * @param string $password * @return RedirectResponse|TemplateResponse|NotFoundResponse */ - public function authenticate($token, $redirect, $password = '') { + public function authenticate($token, $redirect = 'preview', $password = '') { // Check whether share exists try { diff --git a/apps/files_sharing/tests/Controller/ShareControllerTest.php b/apps/files_sharing/tests/Controller/ShareControllerTest.php index 6dc577a354c..28f81395b9d 100644 --- a/apps/files_sharing/tests/Controller/ShareControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareControllerTest.php @@ -249,7 +249,7 @@ class ShareControllerTest extends \Test\TestCase { ->with('files_sharing.sharecontroller.showShare', ['token'=>'token']) ->willReturn('redirect'); - $response = $this->shareController->authenticate('token', 'validpassword'); + $response = $this->shareController->authenticate('token', 'preview', 'validpassword'); $expectedResponse = new RedirectResponse('redirect'); $this->assertEquals($expectedResponse, $response); } @@ -292,7 +292,7 @@ class ShareControllerTest extends \Test\TestCase { $data['errorMessage'] === 'Wrong password'; })); - $response = $this->shareController->authenticate('token', 'invalidpassword'); + $response = $this->shareController->authenticate('token', 'preview', 'invalidpassword'); $expectedResponse = new TemplateResponse($this->appName, 'authenticate', array('wrongpw' => true), 'guest'); $expectedResponse->throttle(); $this->assertEquals($expectedResponse, $response); @@ -323,7 +323,7 @@ class ShareControllerTest extends \Test\TestCase { $this->urlGenerator->expects($this->once()) ->method('linkToRoute') - ->with('files_sharing.sharecontroller.authenticate', ['token' => 'validtoken']) + ->with('files_sharing.sharecontroller.authenticate', ['token' => 'validtoken', 'redirect' => 'preview']) ->willReturn('redirect'); // Test without a not existing token @@ -505,12 +505,12 @@ class ShareControllerTest extends \Test\TestCase { $this->urlGenerator->expects($this->once()) ->method('linkToRoute') - ->with('files_sharing.sharecontroller.authenticate', ['token' => 'validtoken']) + ->with('files_sharing.sharecontroller.authenticate', ['token' => 'validtoken', 'redirect' => 'download']) ->willReturn('redirect'); // Test with a password protected share and no authentication $response = $this->shareController->downloadShare('validtoken'); - $expectedResponse = new RedirectResponse('redirect'); + $expectedResponse = new RedirectResponse('redirect', ''); $this->assertEquals($expectedResponse, $response); } From 7bcc82164221f5f6c807af4e1309220a1f852a0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20Molakvo=C3=A6=20=28skjnldsv=29?= Date: Wed, 28 Feb 2018 19:03:00 +0100 Subject: [PATCH 4/8] fixup! Fixed tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: John Molakvoæ (skjnldsv) --- .../tests/Controller/ShareControllerTest.php | 34 +++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/apps/files_sharing/tests/Controller/ShareControllerTest.php b/apps/files_sharing/tests/Controller/ShareControllerTest.php index 28f81395b9d..dc0c7709a77 100644 --- a/apps/files_sharing/tests/Controller/ShareControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareControllerTest.php @@ -254,6 +254,37 @@ class ShareControllerTest extends \Test\TestCase { $this->assertEquals($expectedResponse, $response); } + public function testAuthenticateValidPasswordAndDownload() { + $share = \OC::$server->getShareManager()->newShare(); + $share->setId(42); + + $this->shareManager + ->expects($this->once()) + ->method('getShareByToken') + ->with('token') + ->willReturn($share); + + $this->shareManager + ->expects($this->once()) + ->method('checkPassword') + ->with($share, 'validpassword') + ->willReturn(true); + + $this->session + ->expects($this->once()) + ->method('set') + ->with('public_link_authenticated', '42'); + + $this->urlGenerator->expects($this->once()) + ->method('linkToRoute') + ->with('files_sharing.sharecontroller.downloadShare', ['token'=>'token']) + ->willReturn('redirect'); + + $response = $this->shareController->authenticate('token', 'download', 'validpassword'); + $expectedResponse = new RedirectResponse('redirect'); + $this->assertEquals($expectedResponse, $response); + } + public function testAuthenticateInvalidPassword() { $share = \OC::$server->getShareManager()->newShare(); $share->setNodeId(100) @@ -510,7 +541,7 @@ class ShareControllerTest extends \Test\TestCase { // Test with a password protected share and no authentication $response = $this->shareController->downloadShare('validtoken'); - $expectedResponse = new RedirectResponse('redirect', ''); + $expectedResponse = new RedirectResponse('redirect'); $this->assertEquals($expectedResponse, $response); } @@ -533,5 +564,4 @@ class ShareControllerTest extends \Test\TestCase { $expectedResponse = new DataResponse('Share is read-only'); $this->assertEquals($expectedResponse, $response); } - } From 484568e995fe3586b823a1493505b4c39aa1ea23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20Molakvo=C3=A6=20=28skjnldsv=29?= Date: Thu, 1 Mar 2018 10:45:56 +0100 Subject: [PATCH 5/8] Acceptance fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: John Molakvoæ (skjnldsv) --- tests/acceptance/features/app-files.feature | 11 ++++++++ .../bootstrap/FilesSharingAppContext.php | 27 ++++++++++++++++++- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/tests/acceptance/features/app-files.feature b/tests/acceptance/features/app-files.feature index ef3d07ae499..eeef6de601f 100644 --- a/tests/acceptance/features/app-files.feature +++ b/tests/acceptance/features/app-files.feature @@ -71,6 +71,17 @@ Feature: app-files Then I see that the current page is the Authenticate page for the shared link I wrote down And I see that a wrong password for the shared file message is shown + Scenario: access a direct download shared link protected by password with a valid password + Given I act as John + And I am logged in + And I share the link for "welcome.txt" protected by the password "abcdef" + And I write down the shared link + When I act as Jane + And I visit the direct download shared link I wrote down + And I see that the current page is the Authenticate page for the direct download shared link I wrote down + And I authenticate with password "abcdef" + Then I see that the current page is the direct download shared link I wrote down + Scenario: show the input field for tags in the details view Given I am logged in And I open the details view for "welcome.txt" diff --git a/tests/acceptance/features/bootstrap/FilesSharingAppContext.php b/tests/acceptance/features/bootstrap/FilesSharingAppContext.php index 5c5d23887cd..4b7dd08c83e 100644 --- a/tests/acceptance/features/bootstrap/FilesSharingAppContext.php +++ b/tests/acceptance/features/bootstrap/FilesSharingAppContext.php @@ -109,6 +109,13 @@ class FilesSharingAppContext implements Context, ActorAwareInterface { $this->actor->getSession()->visit($this->actor->getSharedNotebook()["shared link"]); } + /** + * @When I visit the direct download shared link I wrote down + */ + public function iVisitTheDirectDownloadSharedLinkIWroteDown() { + $this->actor->getSession()->visit($this->actor->getSharedNotebook()["shared link"] . "/download"); + } + /** * @When I authenticate with password :password */ @@ -129,7 +136,16 @@ class FilesSharingAppContext implements Context, ActorAwareInterface { */ public function iSeeThatTheCurrentPageIsTheAuthenticatePageForTheSharedLinkIWroteDown() { PHPUnit_Framework_Assert::assertEquals( - $this->actor->getSharedNotebook()["shared link"] . "/authenticate", + $this->actor->getSharedNotebook()["shared link"] . "/authenticate/preview", + $this->actor->getSession()->getCurrentUrl()); + } + + /** + * @Then I see that the current page is the Authenticate page for the direct download shared link I wrote down + */ + public function iSeeThatTheCurrentPageIsTheAuthenticatePageForTheDirectDownloadSharedLinkIWroteDown() { + PHPUnit_Framework_Assert::assertEquals( + $this->actor->getSharedNotebook()["shared link"] . "/authenticate/download", $this->actor->getSession()->getCurrentUrl()); } @@ -142,6 +158,15 @@ class FilesSharingAppContext implements Context, ActorAwareInterface { $this->actor->getSession()->getCurrentUrl()); } + /** + * @Then I see that the current page is the direct download shared link I wrote down + */ + public function iSeeThatTheCurrentPageIsTheDirectDownloadSharedLinkIWroteDown() { + PHPUnit_Framework_Assert::assertEquals( + $this->actor->getSharedNotebook()["shared link"] . "/download", + $this->actor->getSession()->getCurrentUrl()); + } + /** * @Then I see that a wrong password for the shared file message is shown */ From 2d4896e2a733be04555f64deea06577f61c2f0b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20Molakvo=C3=A6=20=28skjnldsv=29?= Date: Thu, 1 Mar 2018 12:57:00 +0100 Subject: [PATCH 6/8] fixup! Acceptance fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: John Molakvoæ (skjnldsv) --- tests/acceptance/features/app-files.feature | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/acceptance/features/app-files.feature b/tests/acceptance/features/app-files.feature index eeef6de601f..dd5340d6374 100644 --- a/tests/acceptance/features/app-files.feature +++ b/tests/acceptance/features/app-files.feature @@ -80,7 +80,8 @@ Feature: app-files And I visit the direct download shared link I wrote down And I see that the current page is the Authenticate page for the direct download shared link I wrote down And I authenticate with password "abcdef" - Then I see that the current page is the direct download shared link I wrote down + # download starts no page redirection + And I see that the current page is the Authenticate page for the direct download shared link I wrote down Scenario: show the input field for tags in the details view Given I am logged in From 4c2aff0807f668c03d4b426fbe98c2216665f133 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20Molakvo=C3=A6=20=28skjnldsv=29?= Date: Thu, 1 Mar 2018 13:12:24 +0100 Subject: [PATCH 7/8] fixup! Sharing: redirect to download after authentification if requested MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: John Molakvoæ (skjnldsv) --- apps/files_sharing/lib/Controller/ShareController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/files_sharing/lib/Controller/ShareController.php b/apps/files_sharing/lib/Controller/ShareController.php index c4d9e739658..f793d35e3ae 100644 --- a/apps/files_sharing/lib/Controller/ShareController.php +++ b/apps/files_sharing/lib/Controller/ShareController.php @@ -174,7 +174,7 @@ class ShareController extends Controller { * @param string $password * @return RedirectResponse|TemplateResponse|NotFoundResponse */ - public function authenticate($token, $redirect = 'preview', $password = '') { + public function authenticate($token, $redirect, $password = '') { // Check whether share exists try { From 3824e6f631e3345401ac5720d35ec53e8983dcaa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20Molakvo=C3=A6=20=28skjnldsv=29?= Date: Thu, 1 Mar 2018 13:17:08 +0100 Subject: [PATCH 8/8] fixup! fixup! Sharing: redirect to download after authentification if requested MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: John Molakvoæ (skjnldsv) --- apps/files_sharing/tests/Controller/ShareControllerTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/files_sharing/tests/Controller/ShareControllerTest.php b/apps/files_sharing/tests/Controller/ShareControllerTest.php index dc0c7709a77..a977a422e7d 100644 --- a/apps/files_sharing/tests/Controller/ShareControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareControllerTest.php @@ -218,7 +218,7 @@ class ShareControllerTest extends \Test\TestCase { ->with('token') ->will($this->throwException(new \OCP\Share\Exceptions\ShareNotFound())); - $response = $this->shareController->authenticate('token'); + $response = $this->shareController->authenticate('token', 'preview'); $expectedResponse = new NotFoundResponse(); $this->assertEquals($expectedResponse, $response); }