Skip to content

Commit

Permalink
When using permalinks don't error out if file id can't be found
Browse files Browse the repository at this point in the history 
Fixes #952

* Use only the index route (since it went to showFile anyways)
* Fix tests
* Use getUserFolder to force init of users mounts
  • Loading branch information
@rullzer
rullzer committed Aug 23, 2016
1 parent 18f694b commit 8124436
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 78 deletions.
15 changes: 6 additions & 9 deletions apps/files/lib/Controller/ViewController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -67,7 +67,7 @@ class ViewController extends Controller {
protected $userSession;
/** @var IAppManager */
protected $appManager;
/** @var \OCP\Files\Folder */
/** @var IRootFolder */
protected $rootFolder;

/**
Expand All @@ -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,
Expand All @@ -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;
Expand Down Expand Up @@ -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 = [];

Expand Down
97 changes: 30 additions & 67 deletions apps/files/tests/Controller/ViewControllerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -39,7 +39,6 @@
use OCP\IConfig;
use OCP\IUserSession;
use Symfony\Component\EventDispatcher\EventDispatcherInterface;
use OCP\Files\Folder;
use OCP\App\IAppManager;

/**
Expand All @@ -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() {
Expand All @@ -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',
Expand Down Expand Up @@ -305,27 +304,17 @@ 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'));

$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))
Expand All @@ -344,27 +333,20 @@ 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'));

$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');
Expand All @@ -391,43 +373,28 @@ 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))
->method('getById')
->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')
Expand All @@ -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')
Expand Down Expand Up @@ -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'));
}
}
4 changes: 2 additions & 2 deletions core/routes.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 8124436

Please sign in to comment.