Error handling and tests if file was not found

Signed-off-by: Julius Haertl <jus@bitgrid.net>

diff --git a/apps/theming/lib/Controller/IconController.php b/apps/theming/lib/Controller/IconController.php
index 519c52f5fa96efe762fc1377d43c8cb6eca6dcb3..4c25d911e5e7ba2343027fb09c71c45bc6196e17 100644 (file)
--- a/apps/theming/lib/Controller/IconController.php
+++ b/apps/theming/lib/Controller/IconController.php
@@ -27,8 +27,7 @@ use OCA\Theming\ImageManager;
 use OCA\Theming\ThemingDefaults;
 use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http;
-use OCP\AppFramework\Http\Response;
-use OCP\AppFramework\Http\DataDisplayResponse;
+use OCP\AppFramework\Http\NotFoundResponse;
 use OCP\AppFramework\Http\FileDisplayResponse;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Files\NotFoundException;
@@ -88,7 +87,7 @@ class IconController extends Controller {
         *
         * @param $app string app name
         * @param $image string image file name (svg required)
-        * @return FileDisplayResponse
+        * @return FileDisplayResponse|NotFoundResponse
         */
        public function getThemedIcon($app, $image) {
                try {
@@ -97,14 +96,18 @@ class IconController extends Controller {
                        $icon = $this->iconBuilder->colorSvg($app, $image);
                        $iconFile = $this->imageManager->setCachedImage("icon-" . $app . '-' . str_replace("/","_",$image), $icon);
                }
-               $response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/svg+xml']);
-               $response->cacheFor(86400);
-               $expires = new \DateTime();
-               $expires->setTimestamp($this->timeFactory->getTime());
-               $expires->add(new \DateInterval('PT24H'));
-               $response->addHeader('Expires', $expires->format(\DateTime::RFC2822));
-               $response->addHeader('Pragma', 'cache');
-               return $response;
+               if ($iconFile !== false) {
+                       $response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/svg+xml']);
+                       $response->cacheFor(86400);
+                       $expires = new \DateTime();
+                       $expires->setTimestamp($this->timeFactory->getTime());
+                       $expires->add(new \DateInterval('PT24H'));
+                       $response->addHeader('Expires', $expires->format(\DateTime::RFC2822));
+                       $response->addHeader('Pragma', 'cache');
+                       return $response;
+               } else {
+                       return new NotFoundResponse();
+               }
        }
 
        /**
@@ -114,7 +117,7 @@ class IconController extends Controller {
         * @NoCSRFRequired
         *
         * @param $app string app name
-        * @return FileDisplayResponse|DataDisplayResponse
+        * @return FileDisplayResponse|NotFoundResponse
         */
        public function getFavicon($app = "core") {
                if ($this->themingDefaults->shouldReplaceIcons()) {
@@ -124,20 +127,18 @@ class IconController extends Controller {
                                $icon = $this->iconBuilder->getFavicon($app);
                                $iconFile = $this->imageManager->setCachedImage('favIcon-' . $app, $icon);
                        }
-                       $response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/x-icon']);
-                       $response->cacheFor(86400);
-                       $expires = new \DateTime();
-                       $expires->setTimestamp($this->timeFactory->getTime());
-                       $expires->add(new \DateInterval('PT24H'));
-                       $response->addHeader('Expires', $expires->format(\DateTime::RFC2822));
-                       $response->addHeader('Pragma', 'cache');
-               } else {
-                       $response = new Response();
-                       $response->setStatus(Http::STATUS_NOT_FOUND);
-                       $response->cacheFor(0);
-                       $response->setLastModified(new \DateTime('now', new \DateTimeZone('GMT')));
+                       if ($iconFile !== false) {
+                               $response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/x-icon']);
+                               $response->cacheFor(86400);
+                               $expires = new \DateTime();
+                               $expires->setTimestamp($this->timeFactory->getTime());
+                               $expires->add(new \DateInterval('PT24H'));
+                               $response->addHeader('Expires', $expires->format(\DateTime::RFC2822));
+                               $response->addHeader('Pragma', 'cache');
+                               return $response;
+                       }
                }
-               return $response;
+               return new NotFoundResponse();
        }
 
        /**
@@ -147,7 +148,7 @@ class IconController extends Controller {
         * @NoCSRFRequired
         *
         * @param $app string app name
-        * @return FileDisplayResponse|DataDisplayResponse
+        * @return FileDisplayResponse|NotFoundResponse
         */
        public function getTouchIcon($app = "core") {
                if ($this->themingDefaults->shouldReplaceIcons()) {
@@ -157,19 +158,17 @@ class IconController extends Controller {
                                $icon = $this->iconBuilder->getTouchIcon($app);
                                $iconFile = $this->imageManager->setCachedImage('touchIcon-' . $app, $icon);
                        }
-                       $response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/png']);
-                       $response->cacheFor(86400);
-                       $expires = new \DateTime();
-                       $expires->setTimestamp($this->timeFactory->getTime());
-                       $expires->add(new \DateInterval('PT24H'));
-                       $response->addHeader('Expires', $expires->format(\DateTime::RFC2822));
-                       $response->addHeader('Pragma', 'cache');
-               } else {
-                       $response = new Response();
-                       $response->setStatus(Http::STATUS_NOT_FOUND);
-                       $response->cacheFor(0);
-                       $response->setLastModified(new \DateTime('now', new \DateTimeZone('GMT')));
+                       if ($iconFile !== false) {
+                               $response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/png']);
+                               $response->cacheFor(86400);
+                               $expires = new \DateTime();
+                               $expires->setTimestamp($this->timeFactory->getTime());
+                               $expires->add(new \DateInterval('PT24H'));
+                               $response->addHeader('Expires', $expires->format(\DateTime::RFC2822));
+                               $response->addHeader('Pragma', 'cache');
+                               return $response;
+                       }
                }
-               return $response;
+               return new NotFoundResponse();
        }
 }

diff --git a/apps/theming/lib/IconBuilder.php b/apps/theming/lib/IconBuilder.php
index 3819a2be4cb8727588e8c36f9e422d83613185d9..be3035eb0d8e52887fc00c24dbe644ce4bd64989 100644 (file)
--- a/apps/theming/lib/IconBuilder.php
+++ b/apps/theming/lib/IconBuilder.php
@@ -48,10 +48,13 @@ class IconBuilder {
 
        /**
         * @param $app string app name
-        * @return string image blob
+        * @return string|false image blob
         */
        public function getFavicon($app) {
                $icon = $this->renderAppIcon($app);
+               if($icon === false) {
+                       return false;
+               }
                $icon->resizeImage(32, 32, Imagick::FILTER_LANCZOS, 1);
                $icon->setImageFormat("png24");
                $data = $icon->getImageBlob();
@@ -61,10 +64,13 @@ class IconBuilder {
 
        /**
         * @param $app string app name
-        * @return string image blob
+        * @return string|false image blob
         */
        public function getTouchIcon($app) {
                $icon = $this->renderAppIcon($app);
+               if($icon === false) {
+                       return false;
+               }
                $icon->setImageFormat("png24");
                $data = $icon->getImageBlob();
                $icon->destroy();
@@ -76,11 +82,14 @@ class IconBuilder {
         * fallback to logo
         *
         * @param $app string app name
-        * @return Imagick
+        * @return Imagick|false
         */
        public function renderAppIcon($app) {
                $appIcon = $this->util->getAppIcon($app);
                $appIconContent = file_get_contents($appIcon);
+               if($appIconContent === false) {
+                       return false;
+               }
 
                $color = $this->themingDefaults->getMailHeaderColor();
                $mime = mime_content_type($appIcon);
@@ -151,9 +160,13 @@ class IconBuilder {
        public function colorSvg($app, $image) {
                $imageFile = $this->util->getAppImage($app, $image);
                $svg = file_get_contents($imageFile);
-               $color = $this->util->elementColor($this->themingDefaults->getMailHeaderColor());
-               $svg = $this->util->colorizeSvg($svg, $color);
-               return $svg;
+               if ($svg !== false) {
+                       $color = $this->util->elementColor($this->themingDefaults->getMailHeaderColor());
+                       $svg = $this->util->colorizeSvg($svg, $color);
+                       return $svg;
+               } else {
+                       return false;
+               }
        }
 
 }

diff --git a/apps/theming/tests/Controller/IconControllerTest.php b/apps/theming/tests/Controller/IconControllerTest.php
index b1742db018f6257e901ba0f06ac50bdd7364d78c..591c107549293cc332cfb887a3afdd7c76f6bdaf 100644 (file)
--- a/apps/theming/tests/Controller/IconControllerTest.php
+++ b/apps/theming/tests/Controller/IconControllerTest.php
@@ -27,6 +27,7 @@ use OC\Files\SimpleFS\SimpleFile;
 use OCA\Theming\ImageManager;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\DataDisplayResponse;
+use OCP\AppFramework\Http\NotFoundResponse;
 use OCP\Files\IRootFolder;
 use OCP\Files\NotFoundException;
 use OCP\IConfig;
@@ -153,7 +154,7 @@ class IconControllerTest extends TestCase {
                $expected->setStatus(Http::STATUS_NOT_FOUND);
                $expected->cacheFor(0);
                $expected->setLastModified(new \DateTime('now', new \DateTimeZone('GMT')));
-               $this->assertEquals($expected, $this->iconController->getFavicon());
+               $this->assertInstanceOf(NotFoundResponse::class, $this->iconController->getFavicon());
        }
 
        public function testGetTouchIconDefault() {
@@ -194,11 +195,7 @@ class IconControllerTest extends TestCase {
                $this->themingDefaults->expects($this->any())
                        ->method('shouldReplaceIcons')
                        ->willReturn(false);
-               $expected = new Http\Response();
-               $expected->setStatus(Http::STATUS_NOT_FOUND);
-               $expected->cacheFor(0);
-               $expected->setLastModified(new \DateTime('now', new \DateTimeZone('GMT')));
-               $this->assertEquals($expected, $this->iconController->getTouchIcon());
+               $this->assertInstanceOf(NotFoundResponse::class, $this->iconController->getTouchIcon());
        }
 
 }

diff --git a/apps/theming/tests/IconBuilderTest.php b/apps/theming/tests/IconBuilderTest.php
index 529518b30de9ebfa83c1fd71921fd459731cecf7..a13d4b9476d5d181661f3811fc5d12c800aadb3d 100644 (file)
--- a/apps/theming/tests/IconBuilderTest.php
+++ b/apps/theming/tests/IconBuilderTest.php
@@ -25,6 +25,7 @@ namespace OCA\Theming\Tests;
 use OCA\Theming\IconBuilder;
 use OCA\Theming\ThemingDefaults;
 use OCA\Theming\Util;
+use OCP\AppFramework\Http\NotFoundResponse;
 use OCP\Files\IRootFolder;
 use OCP\IConfig;
 use Test\TestCase;
@@ -149,4 +150,39 @@ class IconBuilderTest extends TestCase {
                // cloud be something like $expectedIcon->compareImages($icon, Imagick::METRIC_MEANABSOLUTEERROR)[1])
        }
 
+       /**
+        * @expectedException \PHPUnit_Framework_Error_Warning
+        */
+       public function testGetFaviconNotFound() {
+               $util = $this->getMockBuilder(Util::class)->disableOriginalConstructor()->getMock();
+               $iconBuilder = new IconBuilder($this->themingDefaults, $util);
+               $util->expects($this->once())
+                       ->method('getAppIcon')
+                       ->willReturn('notexistingfile');
+               $this->assertFalse($iconBuilder->getFavicon('noapp'));
+       }
+
+       /**
+        * @expectedException \PHPUnit_Framework_Error_Warning
+        */
+       public function testGetTouchIconNotFound() {
+               $util = $this->getMockBuilder(Util::class)->disableOriginalConstructor()->getMock();
+               $iconBuilder = new IconBuilder($this->themingDefaults, $util);
+               $util->expects($this->once())
+                       ->method('getAppIcon')
+                       ->willReturn('notexistingfile');
+               $this->assertFalse($iconBuilder->getTouchIcon('noapp'));
+       }
+
+       /**
+        * @expectedException \PHPUnit_Framework_Error_Warning
+        */
+       public function testColorSvgNotFound() {
+               $util = $this->getMockBuilder(Util::class)->disableOriginalConstructor()->getMock();
+               $iconBuilder = new IconBuilder($this->themingDefaults, $util);
+               $util->expects($this->once())
+                       ->method('getAppImage')
+                       ->willReturn('notexistingfile');
+               $this->assertFalse($iconBuilder->colorSvg('noapp','noimage'));
+       }
 }