From ac9f75e1d9e6473a9aaa33f6e72f2d2517470de2 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 19 Aug 2016 08:15:30 +0200 Subject: [PATCH 1/2] When using permalinks don't error out if file id can't be found Fixes #952 * Use only the index route (since it went to showFile anyways) * Fix tests * Use getUserFolder to force init of users mounts --- apps/files/lib/Controller/ViewController.php | 16 ++- .../tests/Controller/ViewControllerTest.php | 97 ++++++------------- core/routes.php | 4 +- 3 files changed, 39 insertions(+), 78 deletions(-) diff --git a/apps/files/lib/Controller/ViewController.php b/apps/files/lib/Controller/ViewController.php index 5e342e6589..776f7502d8 100644 --- a/apps/files/lib/Controller/ViewController.php +++ b/apps/files/lib/Controller/ViewController.php @@ -27,11 +27,11 @@ namespace OCA\Files\Controller; -use OC\AppFramework\Http\Request; use OCP\AppFramework\Controller; use OCP\AppFramework\Http\ContentSecurityPolicy; use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Http\TemplateResponse; +use OCP\Files\IRootFolder; use OCP\Files\NotFoundException; use OCP\IConfig; use OCP\IL10N; @@ -42,6 +42,7 @@ use OCP\IUserSession; use Symfony\Component\EventDispatcher\EventDispatcherInterface; use OCP\Files\Folder; use OCP\App\IAppManager; +use OC\AppFramework\Http\Request; /** * Class ViewController @@ -67,7 +68,7 @@ class ViewController extends Controller { protected $userSession; /** @var IAppManager */ protected $appManager; - /** @var \OCP\Files\Folder */ + /** @var IRootFolder */ protected $rootFolder; /** @@ -80,7 +81,7 @@ class ViewController extends Controller { * @param EventDispatcherInterface $eventDispatcherInterface * @param IUserSession $userSession * @param IAppManager $appManager - * @param Folder $rootFolder + * @param IRootFolder $rootFolder */ public function __construct($appName, IRequest $request, @@ -91,7 +92,7 @@ class ViewController extends Controller { EventDispatcherInterface $eventDispatcherInterface, IUserSession $userSession, IAppManager $appManager, - Folder $rootFolder + IRootFolder $rootFolder ) { parent::__construct($appName, $request); $this->appName = $appName; @@ -277,13 +278,10 @@ class ViewController extends Controller { * @param string $fileId file id to show * @return RedirectResponse redirect response or not found response * @throws \OCP\Files\NotFoundException - * - * @NoCSRFRequired - * @NoAdminRequired */ - public function showFile($fileId) { + private function showFile($fileId) { $uid = $this->userSession->getUser()->getUID(); - $baseFolder = $this->rootFolder->get($uid . '/files/'); + $baseFolder = $this->rootFolder->getUserFolder($uid); $files = $baseFolder->getById($fileId); $params = []; diff --git a/apps/files/tests/Controller/ViewControllerTest.php b/apps/files/tests/Controller/ViewControllerTest.php index ceb48a2241..5313593afd 100644 --- a/apps/files/tests/Controller/ViewControllerTest.php +++ b/apps/files/tests/Controller/ViewControllerTest.php @@ -27,7 +27,7 @@ namespace OCA\Files\Tests\Controller; use OCA\Files\Controller\ViewController; use OCP\AppFramework\Http; -use OCP\Files\NotFoundException; +use OCP\Files\IRootFolder; use OCP\IUser; use OCP\Template; use Test\TestCase; @@ -39,7 +39,6 @@ use OCP\IL10N; use OCP\IConfig; use OCP\IUserSession; use Symfony\Component\EventDispatcher\EventDispatcherInterface; -use OCP\Files\Folder; use OCP\App\IAppManager; /** @@ -48,27 +47,27 @@ use OCP\App\IAppManager; * @package OCA\Files\Tests\Controller */ class ViewControllerTest extends TestCase { - /** @var IRequest */ + /** @var IRequest|\PHPUnit_Framework_MockObject_MockObject */ private $request; - /** @var IURLGenerator */ + /** @var IURLGenerator|\PHPUnit_Framework_MockObject_MockObject */ private $urlGenerator; /** @var INavigationManager */ private $navigationManager; /** @var IL10N */ private $l10n; - /** @var IConfig */ + /** @var IConfig|\PHPUnit_Framework_MockObject_MockObject */ private $config; /** @var EventDispatcherInterface */ private $eventDispatcher; - /** @var ViewController */ + /** @var ViewController|\PHPUnit_Framework_MockObject_MockObject */ private $viewController; /** @var IUser */ private $user; /** @var IUserSession */ private $userSession; - /** @var IAppManager */ + /** @var IAppManager|\PHPUnit_Framework_MockObject_MockObject */ private $appManager; - /** @var Folder */ + /** @var IRootFolder|\PHPUnit_Framework_MockObject_MockObject */ private $rootFolder; public function setUp() { @@ -88,7 +87,7 @@ class ViewControllerTest extends TestCase { $this->userSession->expects($this->any()) ->method('getUser') ->will($this->returnValue($this->user)); - $this->rootFolder = $this->getMock('\OCP\Files\Folder'); + $this->rootFolder = $this->getMockBuilder('\OCP\Files\IRootFolder')->getMock(); $this->viewController = $this->getMockBuilder('\OCA\Files\Controller\ViewController') ->setConstructorArgs([ 'files', @@ -305,18 +304,8 @@ class ViewControllerTest extends TestCase { $this->assertEquals($expected, $this->viewController->index('MyDir', 'MyView')); } - public function showFileMethodProvider() { - return [ - [true], - [false], - ]; - } - - /** - * @dataProvider showFileMethodProvider - */ - public function testShowFileRouteWithFolder($useShowFile) { - $node = $this->getMock('\OCP\Files\Folder'); + public function testShowFileRouteWithFolder() { + $node = $this->getMockBuilder('\OCP\Files\Folder')->getMock(); $node->expects($this->once()) ->method('getPath') ->will($this->returnValue('/testuser1/files/test/sub')); @@ -324,8 +313,8 @@ class ViewControllerTest extends TestCase { $baseFolder = $this->getMock('\OCP\Files\Folder'); $this->rootFolder->expects($this->once()) - ->method('get') - ->with('testuser1/files/') + ->method('getUserFolder') + ->with('testuser1') ->will($this->returnValue($baseFolder)); $baseFolder->expects($this->at(0)) @@ -344,18 +333,11 @@ class ViewControllerTest extends TestCase { ->will($this->returnValue('/apps/files/?dir=/test/sub')); $expected = new Http\RedirectResponse('/apps/files/?dir=/test/sub'); - if ($useShowFile) { - $this->assertEquals($expected, $this->viewController->showFile(123)); - } else { - $this->assertEquals($expected, $this->viewController->index('/whatever', '', '123')); - } + $this->assertEquals($expected, $this->viewController->index('/whatever', '', '123')); } - /** - * @dataProvider showFileMethodProvider - */ - public function testShowFileRouteWithFile($useShowFile) { - $parentNode = $this->getMock('\OCP\Files\Folder'); + public function testShowFileRouteWithFile() { + $parentNode = $this->getMockBuilder('\OCP\Files\Folder')->getMock(); $parentNode->expects($this->once()) ->method('getPath') ->will($this->returnValue('testuser1/files/test')); @@ -363,8 +345,8 @@ class ViewControllerTest extends TestCase { $baseFolder = $this->getMock('\OCP\Files\Folder'); $this->rootFolder->expects($this->once()) - ->method('get') - ->with('testuser1/files/') + ->method('getUserFolder') + ->with('testuser1') ->will($this->returnValue($baseFolder)); $node = $this->getMock('\OCP\Files\File'); @@ -391,21 +373,14 @@ class ViewControllerTest extends TestCase { ->will($this->returnValue('/apps/files/?dir=/test/sub&scrollto=somefile.txt')); $expected = new Http\RedirectResponse('/apps/files/?dir=/test/sub&scrollto=somefile.txt'); - if ($useShowFile) { - $this->assertEquals($expected, $this->viewController->showFile(123)); - } else { - $this->assertEquals($expected, $this->viewController->index('/whatever', '', '123')); - } + $this->assertEquals($expected, $this->viewController->index('/whatever', '', '123')); } - /** - * @dataProvider showFileMethodProvider - */ - public function testShowFileRouteWithInvalidFileId($useShowFile) { - $baseFolder = $this->getMock('\OCP\Files\Folder'); + public function testShowFileRouteWithInvalidFileId() { + $baseFolder = $this->getMockBuilder('\OCP\Files\Folder')->getMock(); $this->rootFolder->expects($this->once()) - ->method('get') - ->with('testuser1/files/') + ->method('getUserFolder') + ->with('testuser1') ->will($this->returnValue($baseFolder)); $baseFolder->expects($this->at(0)) @@ -413,21 +388,13 @@ class ViewControllerTest extends TestCase { ->with(123) ->will($this->returnValue([])); - if ($useShowFile) { - $this->setExpectedException('OCP\Files\NotFoundException'); - $this->viewController->showFile(123); - } else { - $response = $this->viewController->index('MyDir', 'MyView', '123'); - $this->assertInstanceOf('OCP\AppFramework\Http\TemplateResponse', $response); - $params = $response->getParams(); - $this->assertEquals(1, $params['fileNotFound']); - } + $response = $this->viewController->index('MyDir', 'MyView', '123'); + $this->assertInstanceOf('OCP\AppFramework\Http\TemplateResponse', $response); + $params = $response->getParams(); + $this->assertEquals(1, $params['fileNotFound']); } - /** - * @dataProvider showFileMethodProvider - */ - public function testShowFileRouteWithTrashedFile($useShowFile) { + public function testShowFileRouteWithTrashedFile() { $this->appManager->expects($this->once()) ->method('isEnabledForUser') ->with('files_trashbin') @@ -442,8 +409,8 @@ class ViewControllerTest extends TestCase { $baseFolderTrash = $this->getMock('\OCP\Files\Folder'); $this->rootFolder->expects($this->at(0)) - ->method('get') - ->with('testuser1/files/') + ->method('getUserFolder') + ->with('testuser1') ->will($this->returnValue($baseFolderFiles)); $this->rootFolder->expects($this->at(1)) ->method('get') @@ -479,10 +446,6 @@ class ViewControllerTest extends TestCase { ->will($this->returnValue('/apps/files/?view=trashbin&dir=/test.d1462861890/sub&scrollto=somefile.txt')); $expected = new Http\RedirectResponse('/apps/files/?view=trashbin&dir=/test.d1462861890/sub&scrollto=somefile.txt'); - if ($useShowFile) { - $this->assertEquals($expected, $this->viewController->showFile(123)); - } else { - $this->assertEquals($expected, $this->viewController->index('/whatever', '', '123')); - } + $this->assertEquals($expected, $this->viewController->index('/whatever', '', '123')); } } diff --git a/core/routes.php b/core/routes.php index 98454946d4..faf5aa498d 100644 --- a/core/routes.php +++ b/core/routes.php @@ -117,9 +117,9 @@ $this->create('core_ajax_update', '/core/ajax/update.php') ->actionInclude('core/ajax/update.php'); // File routes -$this->create('files.viewcontroller.showFile', '/f/{fileId}')->action(function($urlParams) { +$this->create('files.viewcontroller.showFile', '/f/{fileid}')->action(function($urlParams) { $app = new \OCA\Files\AppInfo\Application($urlParams); - $app->dispatch('ViewController', 'showFile'); + $app->dispatch('ViewController', 'index'); }); // Sharing routes From 53a3ec2f18e995b700e5c1025df9303e70de9c4a Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 19 Aug 2016 10:10:19 +0200 Subject: [PATCH 2/2] When requesting a permalink to an invalid file redirect We need to do the redirect to update address bar. --- apps/files/lib/Controller/ViewController.php | 7 +++---- apps/files/tests/Controller/ViewControllerTest.php | 10 +++++++--- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/apps/files/lib/Controller/ViewController.php b/apps/files/lib/Controller/ViewController.php index 776f7502d8..03ac20aa88 100644 --- a/apps/files/lib/Controller/ViewController.php +++ b/apps/files/lib/Controller/ViewController.php @@ -144,15 +144,14 @@ class ViewController extends Controller { * @param string $dir * @param string $view * @param string $fileid - * @return TemplateResponse + * @return TemplateResponse|RedirectResponse */ - public function index($dir = '', $view = '', $fileid = null) { - $fileNotFound = false; + public function index($dir = '', $view = '', $fileid = null, $fileNotFound = false) { if ($fileid !== null) { try { return $this->showFile($fileid); } catch (NotFoundException $e) { - $fileNotFound = true; + return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.index', ['fileNotFound' => true])); } } diff --git a/apps/files/tests/Controller/ViewControllerTest.php b/apps/files/tests/Controller/ViewControllerTest.php index 5313593afd..eb4ac1555c 100644 --- a/apps/files/tests/Controller/ViewControllerTest.php +++ b/apps/files/tests/Controller/ViewControllerTest.php @@ -388,10 +388,14 @@ class ViewControllerTest extends TestCase { ->with(123) ->will($this->returnValue([])); + $this->urlGenerator->expects($this->once()) + ->method('linkToRoute') + ->with('files.view.index', ['fileNotFound' => true]) + ->willReturn('redirect.url'); + $response = $this->viewController->index('MyDir', 'MyView', '123'); - $this->assertInstanceOf('OCP\AppFramework\Http\TemplateResponse', $response); - $params = $response->getParams(); - $this->assertEquals(1, $params['fileNotFound']); + $this->assertInstanceOf('OCP\AppFramework\Http\RedirectResponse', $response); + $this->assertEquals('redirect.url', $response->getRedirectURL()); } public function testShowFileRouteWithTrashedFile() {