From d409fe1c525ba05f342d52a9686ae395a0dac465 Mon Sep 17 00:00:00 2001 From: Julius Haertl Date: Sat, 5 Nov 2016 20:11:49 +0100 Subject: [PATCH] Error handling and tests if file was not found Signed-off-by: Julius Haertl --- .../theming/lib/Controller/IconController.php | 77 +++++++++---------- apps/theming/lib/IconBuilder.php | 25 ++++-- .../tests/Controller/IconControllerTest.php | 9 +-- apps/theming/tests/IconBuilderTest.php | 36 +++++++++ 4 files changed, 96 insertions(+), 51 deletions(-) diff --git a/apps/theming/lib/Controller/IconController.php b/apps/theming/lib/Controller/IconController.php index 519c52f5fa9..4c25d911e5e 100644 --- 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 3819a2be4cb..be3035eb0d8 100644 --- 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 b1742db018f..591c1075492 100644 --- 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 529518b30de..a13d4b9476d 100644 --- 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')); + } } -- 2.39.5