Merge pull request #956 from nextcloud/fix_952

When using permalinks don't error out if file id can't be found

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;
@@ -143,15 +143,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) {
+	public function index($dir = '', $view = '', $fileid = null, $fileNotFound = false) {
-		$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]));
 			}
 		}
 
@@ -267,13 +266,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 = [];
 

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() {
+	public function testShowFileRouteWithFolder() {
-		return [
-			[true],
-			[false],
-		];
-	}
-
-	/**
-	 * @dataProvider showFileMethodProvider
-	 */
-	public function testShowFileRouteWithFolder($useShowFile) {
 		$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')
+			->method('getUserFolder')
-			->with('testuser1/files/')
+			->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->index('/whatever', '', '123'));
-			$this->assertEquals($expected, $this->viewController->showFile(123));
-		} else {
-			$this->assertEquals($expected, $this->viewController->index('/whatever', '', '123'));
-		}
 	}
 
-	/**
+	public function testShowFileRouteWithFile() {
-	 * @dataProvider showFileMethodProvider
-	 */
-	public function testShowFileRouteWithFile($useShowFile) {
 		$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')
+			->method('getUserFolder')
-			->with('testuser1/files/')
+			->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->index('/whatever', '', '123'));
-			$this->assertEquals($expected, $this->viewController->showFile(123));
-		} else {
-			$this->assertEquals($expected, $this->viewController->index('/whatever', '', '123'));
-		}
 	}
 
-	/**
+	public function testShowFileRouteWithInvalidFileId() {
-	 * @dataProvider showFileMethodProvider
-	 */
-	public function testShowFileRouteWithInvalidFileId($useShowFile) {
 		$baseFolder = $this->getMockBuilder('\OCP\Files\Folder')->getMock();
 		$this->rootFolder->expects($this->once())
-			->method('get')
+			->method('getUserFolder')
-			->with('testuser1/files/')
+			->with('testuser1')
 			->will($this->returnValue($baseFolder));
 
 		$baseFolder->expects($this->at(0))
@@ -373,21 +348,17 @@ class ViewControllerTest extends TestCase {
 			->with(123)
 			->will($this->returnValue([]));
 
-		if ($useShowFile) {
+		$this->urlGenerator->expects($this->once())
-			$this->setExpectedException('OCP\Files\NotFoundException');
+			->method('linkToRoute')
-			$this->viewController->showFile(123);
+			->with('files.view.index', ['fileNotFound' => true])
-		} else {
+			->willReturn('redirect.url');
-			$response = $this->viewController->index('MyDir', 'MyView', '123');
+
-			$this->assertInstanceOf('OCP\AppFramework\Http\TemplateResponse', $response);
+		$response = $this->viewController->index('MyDir', 'MyView', '123');
-			$params = $response->getParams();
+		$this->assertInstanceOf('OCP\AppFramework\Http\RedirectResponse', $response);
-			$this->assertEquals(1, $params['fileNotFound']);
+		$this->assertEquals('redirect.url', $response->getRedirectURL());
-		}
 	}
 
-	/**
+	public function testShowFileRouteWithTrashedFile() {
-	 * @dataProvider showFileMethodProvider
-	 */
-	public function testShowFileRouteWithTrashedFile($useShowFile) {
 		$this->appManager->expects($this->once())
 			->method('isEnabledForUser')
 			->with('files_trashbin')
@@ -402,8 +373,8 @@ class ViewControllerTest extends TestCase {
 		$baseFolderTrash = $this->getMockBuilder('\OCP\Files\Folder')->getMock();
 
 		$this->rootFolder->expects($this->at(0))
-			->method('get')
+			->method('getUserFolder')
-			->with('testuser1/files/')
+			->with('testuser1')
 			->will($this->returnValue($baseFolderFiles));
 		$this->rootFolder->expects($this->at(1))
 			->method('get')
@@ -439,10 +410,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->index('/whatever', '', '123'));
-			$this->assertEquals($expected, $this->viewController->showFile(123));
-		} else {
-			$this->assertEquals($expected, $this->viewController->index('/whatever', '', '123'));
-		}
 	}
 }

core/routes.php

@@ -119,9 +119,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