From 54f79a28f6650ec5cb4fbd9e152eb1d6fb0aa0cb Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 19 Aug 2016 08:15:30 +0200 Subject: [PATCH] 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 | 15 ++- .../tests/Controller/ViewControllerTest.php | 91 ++++++------------- core/routes.php | 4 +- 3 files changed, 35 insertions(+), 75 deletions(-) diff --git a/apps/files/lib/Controller/ViewController.php b/apps/files/lib/Controller/ViewController.php index 779a2c7aadc..9d26c048368 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; @@ -67,7 +67,7 @@ class ViewController extends Controller { protected $userSession; /** @var IAppManager */ protected $appManager; - /** @var \OCP\Files\Folder */ + /** @var IRootFolder */ protected $rootFolder; /** @@ -80,7 +80,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 +91,7 @@ class ViewController extends Controller { EventDispatcherInterface $eventDispatcherInterface, IUserSession $userSession, IAppManager $appManager, - Folder $rootFolder + IRootFolder $rootFolder ) { parent::__construct($appName, $request); $this->appName = $appName; @@ -267,13 +267,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 12ff779d6f1..0ffe66c5592 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->getMockBuilder('\OCP\Files\Folder')->getMock(); + $this->rootFolder = $this->getMockBuilder('\OCP\Files\IRootFolder')->getMock(); $this->viewController = $this->getMockBuilder('\OCA\Files\Controller\ViewController') ->setConstructorArgs([ 'files', @@ -265,17 +264,7 @@ class ViewControllerTest extends TestCase { $this->assertEquals($expected, $this->viewController->index('MyDir', 'MyView')); } - public function showFileMethodProvider() { - return [ - [true], - [false], - ]; - } - - /** - * @dataProvider showFileMethodProvider - */ - public function testShowFileRouteWithFolder($useShowFile) { + public function testShowFileRouteWithFolder() { $node = $this->getMockBuilder('\OCP\Files\Folder')->getMock(); $node->expects($this->once()) ->method('getPath') @@ -284,8 +273,8 @@ class ViewControllerTest extends TestCase { $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)) @@ -304,17 +293,10 @@ 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) { + public function testShowFileRouteWithFile() { $parentNode = $this->getMockBuilder('\OCP\Files\Folder')->getMock(); $parentNode->expects($this->once()) ->method('getPath') @@ -323,8 +305,8 @@ class ViewControllerTest extends TestCase { $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)); $node = $this->getMockBuilder('\OCP\Files\File')->getMock(); @@ -351,21 +333,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) { + 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)) @@ -373,21 +348,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') @@ -402,8 +369,8 @@ class ViewControllerTest extends TestCase { $baseFolderTrash = $this->getMockBuilder('\OCP\Files\Folder')->getMock(); $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') @@ -439,10 +406,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 b4868c14cf3..a148ee091e6 100644 --- a/core/routes.php +++ b/core/routes.php @@ -121,9 +121,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 -- 2.39.5