Error handling and tests if file was not found

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

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\NotFoundResponse;
-use OCP\AppFramework\Http\DataDisplayResponse;
 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']);
+		if ($iconFile !== false) {
-		$response->cacheFor(86400);
+			$response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/svg+xml']);
-		$expires = new \DateTime();
+			$response->cacheFor(86400);
-		$expires->setTimestamp($this->timeFactory->getTime());
+			$expires = new \DateTime();
-		$expires->add(new \DateInterval('PT24H'));
+			$expires->setTimestamp($this->timeFactory->getTime());
-		$response->addHeader('Expires', $expires->format(\DateTime::RFC2822));
+			$expires->add(new \DateInterval('PT24H'));
-		$response->addHeader('Pragma', 'cache');
+			$response->addHeader('Expires', $expires->format(\DateTime::RFC2822));
-		return $response;
+			$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']);
+			if ($iconFile !== false) {
-			$response->cacheFor(86400);
+				$response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/x-icon']);
-			$expires = new \DateTime();
+				$response->cacheFor(86400);
-			$expires->setTimestamp($this->timeFactory->getTime());
+				$expires = new \DateTime();
-			$expires->add(new \DateInterval('PT24H'));
+				$expires->setTimestamp($this->timeFactory->getTime());
-			$response->addHeader('Expires', $expires->format(\DateTime::RFC2822));
+				$expires->add(new \DateInterval('PT24H'));
-			$response->addHeader('Pragma', 'cache');
+				$response->addHeader('Expires', $expires->format(\DateTime::RFC2822));
-		} else {
+				$response->addHeader('Pragma', 'cache');
-			$response = new Response();
+				return $response;
-			$response->setStatus(Http::STATUS_NOT_FOUND);
+			}
-			$response->cacheFor(0);
-			$response->setLastModified(new \DateTime('now', new \DateTimeZone('GMT')));
 		}
-		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']);
+			if ($iconFile !== false) {
-			$response->cacheFor(86400);
+				$response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/png']);
-			$expires = new \DateTime();
+				$response->cacheFor(86400);
-			$expires->setTimestamp($this->timeFactory->getTime());
+				$expires = new \DateTime();
-			$expires->add(new \DateInterval('PT24H'));
+				$expires->setTimestamp($this->timeFactory->getTime());
-			$response->addHeader('Expires', $expires->format(\DateTime::RFC2822));
+				$expires->add(new \DateInterval('PT24H'));
-			$response->addHeader('Pragma', 'cache');
+				$response->addHeader('Expires', $expires->format(\DateTime::RFC2822));
-		} else {
+				$response->addHeader('Pragma', 'cache');
-			$response = new Response();
+				return $response;
-			$response->setStatus(Http::STATUS_NOT_FOUND);
+			}
-			$response->cacheFor(0);
-			$response->setLastModified(new \DateTime('now', new \DateTimeZone('GMT')));
 		}
-		return $response;
+		return new NotFoundResponse();
 	}
 }

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());
+		if ($svg !== false) {
-		$svg = $this->util->colorizeSvg($svg, $color);
+			$color = $this->util->elementColor($this->themingDefaults->getMailHeaderColor());
-		return $svg;
+			$svg = $this->util->colorizeSvg($svg, $color);
+			return $svg;
+		} else {
+			return false;
+		}
 	}
 
 }

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();
+		$this->assertInstanceOf(NotFoundResponse::class, $this->iconController->getTouchIcon());
-		$expected->setStatus(Http::STATUS_NOT_FOUND);
-		$expected->cacheFor(0);
-		$expected->setLastModified(new \DateTime('now', new \DateTimeZone('GMT')));
-		$this->assertEquals($expected, $this->iconController->getTouchIcon());
 	}
 
 }

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'));
+	}
 }