diff options
author | Olivier Paroz <github@oparoz.com> | 2015-09-08 15:22:18 +0300 |
---|---|---|
committer | Olivier Paroz <github@oparoz.com> | 2015-09-17 23:53:36 +0300 |
commit | ec6a904c8df91a614c435d48568970edc1c3c2a8 (patch) | |
tree | ea47ee52f829ccc16d45547d53e3ab6248941da4 | |
parent | 4ea24d9e510d60fd483bd6643d7f0b1ade52e704 (diff) |
Don't send the error messages via GET/POST
-rw-r--r-- | appinfo/application.php | 7 | ||||
-rw-r--r-- | controller/filesapicontroller.php | 15 | ||||
-rw-r--r-- | controller/filescontroller.php | 10 | ||||
-rw-r--r-- | controller/filespubliccontroller.php | 1 | ||||
-rw-r--r-- | controller/httperror.php | 12 | ||||
-rw-r--r-- | controller/pagecontroller.php | 29 | ||||
-rw-r--r-- | middleware/checkmiddleware.php | 13 | ||||
-rw-r--r-- | middleware/envcheckmiddleware.php | 4 | ||||
-rw-r--r-- | middleware/sharingcheckmiddleware.php | 4 | ||||
-rw-r--r-- | tests/unit/controller/FilesApiControllerTest.php | 1 | ||||
-rw-r--r-- | tests/unit/controller/FilesControllerTest.php | 26 | ||||
-rw-r--r-- | tests/unit/controller/FilesPublicControllerTest.php | 2 | ||||
-rw-r--r-- | tests/unit/controller/PageControllerTest.php | 27 | ||||
-rw-r--r-- | tests/unit/middleware/EnvCheckMiddlewareTest.php | 25 | ||||
-rw-r--r-- | tests/unit/middleware/SharingCheckMiddlewareTest.php | 7 |
15 files changed, 132 insertions, 51 deletions
diff --git a/appinfo/application.php b/appinfo/application.php index 436204d9..1cbe2719 100644 --- a/appinfo/application.php +++ b/appinfo/application.php @@ -78,7 +78,8 @@ class Application extends App { $c->query('Request'), $c->query('Environment'), $c->query('OCP\IURLGenerator'), - $c->query('OCP\IConfig') + $c->query('OCP\IConfig'), + $c->query('Session') ); } ); @@ -125,6 +126,7 @@ class Application extends App { $c->query('ConfigService'), $c->query('SearchMediaService'), $c->query('DownloadService'), + $c->query('Session'), $c->query('Logger') ); } @@ -139,6 +141,7 @@ class Application extends App { $c->query('ConfigService'), $c->query('SearchMediaService'), $c->query('DownloadService'), + $c->query('Session'), $c->query('Logger') ); } @@ -153,6 +156,7 @@ class Application extends App { $c->query('ConfigService'), $c->query('SearchMediaService'), $c->query('DownloadService'), + $c->query('Session'), $c->query('Logger') ); } @@ -374,6 +378,7 @@ class Application extends App { $c->query('OCP\IConfig'), $c->query('OCP\AppFramework\Utility\IControllerMethodReflector'), $c->query('OCP\IURLGenerator'), + $c->query('Session'), $c->query('Logger') ); } diff --git a/controller/filesapicontroller.php b/controller/filesapicontroller.php index 551017d5..a59e7029 100644 --- a/controller/filesapicontroller.php +++ b/controller/filesapicontroller.php @@ -14,6 +14,7 @@ namespace OCA\Gallery\Controller; use OCP\IRequest; use OCP\IURLGenerator; +use OCP\ISession; use OCP\ILogger; use OCP\AppFramework\ApiController; @@ -38,6 +39,8 @@ class FilesApiController extends ApiController { /** @var IURLGenerator */ private $urlGenerator; + /** @var ISession */ + private $session; /** * Constructor @@ -49,6 +52,7 @@ class FilesApiController extends ApiController { * @param ConfigService $configService * @param SearchMediaService $searchMediaService * @param DownloadService $downloadService + * @param ISession $session * @param ILogger $logger */ public function __construct( @@ -59,6 +63,7 @@ class FilesApiController extends ApiController { ConfigService $configService, SearchMediaService $searchMediaService, DownloadService $downloadService, + ISession $session, ILogger $logger ) { parent::__construct($appName, $request); @@ -68,6 +73,7 @@ class FilesApiController extends ApiController { $this->configService = $configService; $this->searchMediaService = $searchMediaService; $this->downloadService = $downloadService; + $this->session = $session; $this->logger = $logger; } @@ -101,9 +107,14 @@ class FilesApiController extends ApiController { * @NoAdminRequired * @NoCSRFRequired * @CORS + * @UseSession * * Sends the file matching the fileId * + * In case of error we send an HTML error page + * We need to keep the session open in order to be able to send the error message to the error + * page + * * @param int $fileId the ID of the file we want to download * @param string|null $filename * @@ -113,7 +124,9 @@ class FilesApiController extends ApiController { try { $download = $this->getDownload($fileId, $filename); } catch (ServiceException $exception) { - return $this->htmlError($this->urlGenerator, $this->appName, $exception); + return $this->htmlError( + $this->session, $this->urlGenerator, $this->appName, $exception + ); } return new ImageResponse($download); diff --git a/controller/filescontroller.php b/controller/filescontroller.php index 32a50d4b..1370bb95 100644 --- a/controller/filescontroller.php +++ b/controller/filescontroller.php @@ -14,6 +14,7 @@ namespace OCA\Gallery\Controller; use OCP\IRequest; use OCP\IURLGenerator; +use OCP\ISession; use OCP\ILogger; use OCP\AppFramework\Controller; @@ -38,6 +39,8 @@ class FilesController extends Controller { /** @var IURLGenerator */ private $urlGenerator; + /** @var ISession */ + private $session; /** * Constructor @@ -49,6 +52,7 @@ class FilesController extends Controller { * @param ConfigService $configService * @param SearchMediaService $searchMediaService * @param DownloadService $downloadService + * @param ISession $session * @param ILogger $logger */ public function __construct( @@ -59,6 +63,7 @@ class FilesController extends Controller { ConfigService $configService, SearchMediaService $searchMediaService, DownloadService $downloadService, + ISession $session, ILogger $logger ) { parent::__construct($appName, $request); @@ -68,6 +73,7 @@ class FilesController extends Controller { $this->configService = $configService; $this->searchMediaService = $searchMediaService; $this->downloadService = $downloadService; + $this->session = $session; $this->logger = $logger; } @@ -114,7 +120,9 @@ class FilesController extends Controller { try { $download = $this->getDownload($fileId, $filename); } catch (ServiceException $exception) { - return $this->htmlError($this->urlGenerator, $this->appName, $exception); + return $this->htmlError( + $this->session, $this->urlGenerator, $this->appName, $exception + ); } return new ImageResponse($download); diff --git a/controller/filespubliccontroller.php b/controller/filespubliccontroller.php index 90c06a1a..32380b48 100644 --- a/controller/filespubliccontroller.php +++ b/controller/filespubliccontroller.php @@ -41,6 +41,7 @@ class FilesPublicController extends FilesController { /** * @PublicPage * @NoCSRFRequired + * @UseSession * * Sends the file matching the fileId * diff --git a/controller/httperror.php b/controller/httperror.php index 51c1d34b..b80deb92 100644 --- a/controller/httperror.php +++ b/controller/httperror.php @@ -17,6 +17,7 @@ namespace OCA\Gallery\Controller; use Exception; use OCP\IURLGenerator; +use OCP\ISession; use OCP\AppFramework\Http; use OCP\AppFramework\Http\JSONResponse; @@ -53,22 +54,19 @@ trait HttpError { } /** + * @param ISession $session * @param IURLGenerator $urlGenerator * @param string $appName * @param \Exception $exception * * @return RedirectResponse */ - public function htmlError($urlGenerator, $appName, Exception $exception) { + public function htmlError($session, $urlGenerator, $appName, Exception $exception) { $message = $exception->getMessage(); $code = $this->getHttpStatusCode($exception); - + $session->set('galleryErrorMessage', $message); $url = $urlGenerator->linkToRoute( - $appName . '.page.error_page', - [ - 'message' => $message, - 'code' => $code - ] + $appName . '.page.error_page', ['code' => $code] ); return new RedirectResponse($url); diff --git a/controller/pagecontroller.php b/controller/pagecontroller.php index 83172d5e..ca9e074b 100644 --- a/controller/pagecontroller.php +++ b/controller/pagecontroller.php @@ -17,6 +17,7 @@ namespace OCA\Gallery\Controller; use OCP\IURLGenerator; use OCP\IRequest; use OCP\IConfig; +use OCP\ISession; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; @@ -25,7 +26,6 @@ use OCP\AppFramework\Http\RedirectResponse; use OCA\Gallery\Environment\Environment; use OCA\Gallery\Http\ImageResponse; -use OCA\Gallery\Service\DownloadService; /** * Generates templates for the landing page from within ownCloud, the public @@ -35,18 +35,14 @@ use OCA\Gallery\Service\DownloadService; */ class PageController extends Controller { - /** - * @var Environment - */ + /** @var Environment */ private $environment; - /** - * @var IURLGenerator - */ + /** @var IURLGenerator */ private $urlGenerator; - /** - * @var IConfig - */ + /** @var IConfig */ private $appConfig; + /** @var ISession */ + private $session; /** * Constructor @@ -56,19 +52,22 @@ class PageController extends Controller { * @param Environment $environment * @param IURLGenerator $urlGenerator * @param IConfig $appConfig + * @param ISession $session */ public function __construct( $appName, IRequest $request, Environment $environment, IURLGenerator $urlGenerator, - IConfig $appConfig + IConfig $appConfig, + ISession $session ) { parent::__construct($appName, $request); $this->environment = $environment; $this->urlGenerator = $urlGenerator; $this->appConfig = $appConfig; + $this->session = $session; } /** @@ -117,7 +116,7 @@ class PageController extends Controller { $url = $this->urlGenerator->linkToRoute( $this->appName . '.files_public.download', [ - 'token' => $token, + 'token' => $token, 'fileId' => $node->getId(), 'filename' => $filename ] @@ -131,16 +130,18 @@ class PageController extends Controller { * @PublicPage * @NoCSRFRequired * @Guest + * @UseSession * * Generates an error page based on the error code * - * @param string $message * @param int $code * * @return TemplateResponse */ - public function errorPage($message, $code) { + public function errorPage($code) { $appName = $this->appName; + $message = $this->session->get('galleryErrorMessage'); + $this->session->remove('galleryErrorMessage'); $params = [ 'appName' => $appName, 'message' => $message, diff --git a/middleware/checkmiddleware.php b/middleware/checkmiddleware.php index 6bfeec57..1f5a8184 100644 --- a/middleware/checkmiddleware.php +++ b/middleware/checkmiddleware.php @@ -16,6 +16,7 @@ namespace OCA\Gallery\Middleware; use OCP\IURLGenerator; use OCP\IRequest; +use OCP\ISession; use OCP\ILogger; use OCP\AppFramework\Http\JSONResponse; @@ -37,6 +38,8 @@ abstract class CheckMiddleware extends Middleware { protected $request; /** @var IURLGenerator */ private $urlGenerator; + /** @var ISession */ + protected $session; /** @var ILogger */ protected $logger; @@ -46,17 +49,20 @@ abstract class CheckMiddleware extends Middleware { * @param string $appName * @param IRequest $request * @param IURLGenerator $urlGenerator + * @param ISession $session * @param ILogger $logger */ public function __construct( $appName, IRequest $request, IURLGenerator $urlGenerator, + ISession $session, ILogger $logger ) { $this->appName = $appName; $this->request = $request; $this->urlGenerator = $urlGenerator; + $this->session = $session; $this->logger = $logger; } @@ -148,12 +154,9 @@ abstract class CheckMiddleware extends Middleware { * @return RedirectResponse */ private function redirectToErrorPage($message, $code) { + $this->session->set('galleryErrorMessage', $message); $url = $this->urlGenerator->linkToRoute( - $this->appName . '.page.error_page', - [ - 'message' => $message, - 'code' => $code - ] + $this->appName . '.page.error_page', ['code' => $code] ); return new RedirectResponse($url); diff --git a/middleware/envcheckmiddleware.php b/middleware/envcheckmiddleware.php index 8364e525..f39e9fef 100644 --- a/middleware/envcheckmiddleware.php +++ b/middleware/envcheckmiddleware.php @@ -40,8 +40,6 @@ class EnvCheckMiddleware extends CheckMiddleware { /** @var IHasher */ private $hasher; - /** @var ISession */ - private $session; /** @var Environment */ private $environment; /** @var IControllerMethodReflector */ @@ -73,11 +71,11 @@ class EnvCheckMiddleware extends CheckMiddleware { $appName, $request, $urlGenerator, + $session, $logger ); $this->hasher = $hasher; - $this->session = $session; $this->environment = $environment; $this->reflector = $reflector; } diff --git a/middleware/sharingcheckmiddleware.php b/middleware/sharingcheckmiddleware.php index 3e809e7d..d04e5cf1 100644 --- a/middleware/sharingcheckmiddleware.php +++ b/middleware/sharingcheckmiddleware.php @@ -18,6 +18,7 @@ use OCP\IConfig; use OCP\IRequest; use OCP\ILogger; use OCP\IURLGenerator; +use OCP\ISession; use OCP\AppFramework\Http; use OCP\AppFramework\Utility\IControllerMethodReflector; @@ -42,6 +43,7 @@ class SharingCheckMiddleware extends CheckMiddleware { * @param IConfig $appConfig * @param IControllerMethodReflector $reflector * @param IURLGenerator $urlGenerator + * @param ISession $session * @param ILogger $logger */ public function __construct( @@ -50,12 +52,14 @@ class SharingCheckMiddleware extends CheckMiddleware { IConfig $appConfig, IControllerMethodReflector $reflector, IURLGenerator $urlGenerator, + ISession $session, ILogger $logger ) { parent::__construct( $appName, $request, $urlGenerator, + $session, $logger ); diff --git a/tests/unit/controller/FilesApiControllerTest.php b/tests/unit/controller/FilesApiControllerTest.php index e53d179e..3d837b79 100644 --- a/tests/unit/controller/FilesApiControllerTest.php +++ b/tests/unit/controller/FilesApiControllerTest.php @@ -31,6 +31,7 @@ class FilesApiControllerTest extends FilesControllerTest { $this->configService, $this->searchMediaService, $this->downloadService, + $this->session, $this->logger ); } diff --git a/tests/unit/controller/FilesControllerTest.php b/tests/unit/controller/FilesControllerTest.php index 853487a6..65134059 100644 --- a/tests/unit/controller/FilesControllerTest.php +++ b/tests/unit/controller/FilesControllerTest.php @@ -15,6 +15,7 @@ namespace OCA\Gallery\Controller; use OCA\Gallery\Service\ServiceException; use OCP\IRequest; use OCP\IURLGenerator; +use OCP\ISession; use OCP\ILogger; use OCP\AppFramework\IAppContainer; @@ -57,6 +58,10 @@ class FilesControllerTest extends \Test\GalleryUnitTest { protected $searchMediaService; /** @var DownloadService */ protected $downloadService; + /** @var ISession */ + protected $session; + /** @var ISession */ + protected $sessionValue = null; /** @var ILogger */ protected $logger; @@ -90,6 +95,9 @@ class FilesControllerTest extends \Test\GalleryUnitTest { $this->downloadService = $this->getMockBuilder('\OCA\Gallery\Service\DownloadService') ->disableOriginalConstructor() ->getMock(); + $this->session = $this->getMockBuilder('\OCP\ISession') + ->disableOriginalConstructor() + ->getMock(); $this->logger = $this->getMockBuilder('\OCP\ILogger') ->disableOriginalConstructor() ->getMock(); @@ -101,6 +109,7 @@ class FilesControllerTest extends \Test\GalleryUnitTest { $this->configService, $this->searchMediaService, $this->downloadService, + $this->session, $this->logger ); } @@ -131,6 +140,7 @@ class FilesControllerTest extends \Test\GalleryUnitTest { $redirect = new RedirectResponse( $this->urlGenerator->linkToRoute($this->appName . '.page.error_page') ); + $this->mockSessionSet('galleryErrorMessage', $exception->getMessage()); $response = $this->controller->download($fileId, $filename); @@ -356,4 +366,20 @@ class FilesControllerTest extends \Test\GalleryUnitTest { ) ->willReturn($answer); } + + /** + * Needs to be called at least once by testDownloadWithWrongId() or the tests will fail + * + * @param $key + * @param $value + */ + private function mockSessionSet($key, $value) { + $this->session->expects($this->once()) + ->method('set') + ->with( + $key, + $value + ); + } + } diff --git a/tests/unit/controller/FilesPublicControllerTest.php b/tests/unit/controller/FilesPublicControllerTest.php index 1253018b..400f355e 100644 --- a/tests/unit/controller/FilesPublicControllerTest.php +++ b/tests/unit/controller/FilesPublicControllerTest.php @@ -13,6 +13,7 @@ namespace OCA\Gallery\Controller; require_once __DIR__ . '/FilesControllerTest.php'; + /** * Class FilesPublicControllerTest * @@ -30,6 +31,7 @@ class FilesPublicControllerTest extends FilesControllerTest { $this->configService, $this->searchMediaService, $this->downloadService, + $this->session, $this->logger ); } diff --git a/tests/unit/controller/PageControllerTest.php b/tests/unit/controller/PageControllerTest.php index cf71d729..0189dfff 100644 --- a/tests/unit/controller/PageControllerTest.php +++ b/tests/unit/controller/PageControllerTest.php @@ -12,17 +12,16 @@ namespace OCA\Gallery\Controller; - use OCP\IConfig; use OCP\IRequest; use OCP\IURLGenerator; +use OCP\ISession; use OCP\AppFramework\Http; use OCP\AppFramework\Http\TemplateResponse; use OCP\AppFramework\Http\RedirectResponse; use OCA\Gallery\Environment\Environment; -use OCA\Gallery\Service\DownloadService; /** * @package OCA\Gallery\Controller @@ -39,6 +38,8 @@ class PageControllerTest extends \Test\TestCase { private $urlGenerator; /** @var IConfig */ private $appConfig; + /** @var ISession */ + protected $session; /** @var PageController */ protected $controller; @@ -60,12 +61,16 @@ class PageControllerTest extends \Test\TestCase { $this->appConfig = $this->getMockBuilder('\OCP\IConfig') ->disableOriginalConstructor() ->getMock(); + $this->session = $this->getMockBuilder('\OCP\ISession') + ->disableOriginalConstructor() + ->getMock(); $this->controller = new PageController( $this->appName, $this->request, $this->environment, $this->urlGenerator, - $this->appConfig + $this->appConfig, + $this->session ); } @@ -167,8 +172,9 @@ class PageControllerTest extends \Test\TestCase { ]; $template = new TemplateResponse($this->appName, 'index', $params, 'guest'); $template->setStatus($code); + $this->mockSessionGet('galleryErrorMessage', $message); - $response = $this->controller->errorPage($message, $code); + $response = $this->controller->errorPage($code); $this->assertEquals($params, $response->getParams()); $this->assertEquals('index', $response->getTemplateName()); @@ -233,4 +239,17 @@ class PageControllerTest extends \Test\TestCase { ->willReturn($url); } + /** + * Needs to be called at least once by testDownloadWithWrongId() or the tests will fail + * + * @param $key + * @param $value + */ + private function mockSessionGet($key, $value) { + $this->session->expects($this->once()) + ->method('get') + ->with($key) + ->willReturn($value); + } + } diff --git a/tests/unit/middleware/EnvCheckMiddlewareTest.php b/tests/unit/middleware/EnvCheckMiddlewareTest.php index cb57481c..7f37ac65 100644 --- a/tests/unit/middleware/EnvCheckMiddlewareTest.php +++ b/tests/unit/middleware/EnvCheckMiddlewareTest.php @@ -44,9 +44,9 @@ class EnvCheckMiddlewareTest extends \Codeception\TestCase\Test { private $appName = 'gallery'; /** @var IRequest */ private $request; - /** @var IHasher * */ + /** @var IHasher */ private $hasher; - /** @var ISession * */ + /** @var ISession */ private $session; /** @var Environment */ private $environment; @@ -165,7 +165,9 @@ class EnvCheckMiddlewareTest extends \Codeception\TestCase\Test { public function testBeforeControllerWithPublicNotationAndToken() { $this->reflector->reflect(__CLASS__, __FUNCTION__); - $this->mockGetTokenAndPasswordParams($this->sharedFolderToken, $this->passwordForFolderShare); + $this->mockGetTokenAndPasswordParams( + $this->sharedFolderToken, $this->passwordForFolderShare + ); $linkItem = Share::getShareByToken($this->sharedFolderToken, false); $this->mockHasherVerify($this->passwordForFolderShare, $linkItem['share_with'], true); @@ -461,7 +463,7 @@ class EnvCheckMiddlewareTest extends \Codeception\TestCase\Test { $code = Http::STATUS_NOT_FOUND; $exception = new CheckException($message, $code); - $template = $this->mockHtml404Response($message, $code); + $template = $this->mockHtml404Response($code); $response = $this->middleware->afterException($this->controller, 'authenticate', $exception); @@ -542,10 +544,10 @@ class EnvCheckMiddlewareTest extends \Codeception\TestCase\Test { return new TemplateResponse($this->appName, 'authenticate', [], 'guest'); } - private function mockHtml404Response($message, $code) { + private function mockHtml404Response($code) { $this->mockAcceptHeader('html'); $redirectUrl = 'http://newroute.com'; - $this->mockUrlToErrorPage($message, $code, $redirectUrl); + $this->mockUrlToErrorPage($code, $redirectUrl); return new RedirectResponse($redirectUrl); } @@ -584,20 +586,13 @@ class EnvCheckMiddlewareTest extends \Codeception\TestCase\Test { /** * Mocks IURLGenerator->linkToRoute() * - * @param string $message * @param int $code * @param string $url */ - private function mockUrlToErrorPage($message, $code, $url) { + private function mockUrlToErrorPage($code, $url) { $this->urlGenerator->expects($this->once()) ->method('linkToRoute') - ->with( - $this->appName . '.page.error_page', - [ - 'message' => $message, - 'code' => $code - ] - ) + ->with($this->appName . '.page.error_page', ['code' => $code]) ->willReturn($url); } diff --git a/tests/unit/middleware/SharingCheckMiddlewareTest.php b/tests/unit/middleware/SharingCheckMiddlewareTest.php index ff3b31b3..a798fe6b 100644 --- a/tests/unit/middleware/SharingCheckMiddlewareTest.php +++ b/tests/unit/middleware/SharingCheckMiddlewareTest.php @@ -21,6 +21,7 @@ use OCP\IConfig; use OCP\IRequest; use OCP\ILogger; use OCP\IURLGenerator; +use OCP\ISession; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; @@ -44,6 +45,8 @@ class SharingCheckMiddlewareTest extends \Test\TestCase { protected $reflector; /** @var IURLGenerator */ private $urlGenerator; + /** @var ISession */ + private $session; /** @var ILogger */ protected $logger; /** @var Controller */ @@ -68,6 +71,9 @@ class SharingCheckMiddlewareTest extends \Test\TestCase { $this->urlGenerator = $this->getMockBuilder('\OCP\IURLGenerator') ->disableOriginalConstructor() ->getMock(); + $this->session = $this->getMockBuilder('\OCP\ISession') + ->disableOriginalConstructor() + ->getMock(); $this->logger = $this->getMockBuilder('\OCP\ILogger') ->disableOriginalConstructor() ->getMock(); @@ -81,6 +87,7 @@ class SharingCheckMiddlewareTest extends \Test\TestCase { $this->config, $this->reflector, $this->urlGenerator, + $this->session, $this->logger ); } |