From 81244363a9e745e35d2b6f34115884c9d795b216 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 | 97 ++++++------------- core/routes.php | 4 +- 3 files changed, 38 insertions(+), 78 deletions(-) diff --git a/apps/files/lib/Controller/ViewController.php b/apps/files/lib/Controller/ViewController.php index 5e342e6589bc4..b2f7ca6c5f4c6 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 @@ public function __construct($appName, EventDispatcherInterface $eventDispatcherInterface, IUserSession $userSession, IAppManager $appManager, - Folder $rootFolder + IRootFolder $rootFolder ) { parent::__construct($appName, $request); $this->appName = $appName; @@ -277,13 +277,10 @@ public function index($dir = '', $view = '', $fileid = null) { * @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 ceb48a2241f3f..5313593afdf60 100644 --- a/apps/files/tests/Controller/ViewControllerTest.php +++ b/apps/files/tests/Controller/ViewControllerTest.php @@ -27,7 +27,7 @@ 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\IConfig; use OCP\IUserSession; use Symfony\Component\EventDispatcher\EventDispatcherInterface; -use OCP\Files\Folder; use OCP\App\IAppManager; /** @@ -48,27 +47,27 @@ * @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 @@ public function setUp() { $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 @@ public function testIndexWithRegularBrowser() { $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 @@ public function testShowFileRouteWithFolder($useShowFile) { $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 @@ public function testShowFileRouteWithFolder($useShowFile) { ->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 @@ public function testShowFileRouteWithFile($useShowFile) { $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 @@ public function testShowFileRouteWithFile($useShowFile) { ->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 @@ public function testShowFileRouteWithInvalidFileId($useShowFile) { ->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 @@ public function testShowFileRouteWithTrashedFile($useShowFile) { $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 @@ public function testShowFileRouteWithTrashedFile($useShowFile) { ->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 98454946d45c5..faf5aa498d479 100644 --- a/core/routes.php +++ b/core/routes.php @@ -117,9 +117,9 @@ ->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