From a1f18241166f335b89a6cf3d002efd3d825fde19 Mon Sep 17 00:00:00 2001 From: Kyle Fazzari Date: Tue, 20 Feb 2018 22:44:37 -0800 Subject: [PATCH 1/3] theming: handle not being in the serverroot Currently, the theming app assumes it's in the serverroot. However, with Nextcloud's flexibility regarding configurable app paths, this is not a safe assumption to make. If it happens to be an incorrect assumption, the theming app fails to work. Instead of relying on the serverroot, just use the path from the AppManager and utilize relative paths for assets from there. Fix #8462 Signed-off-by: Kyle Fazzari --- .../lib/Controller/ThemingController.php | 18 ++++++++-- .../Controller/ThemingControllerTest.php | 34 +++++++++++++++++++ 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/apps/theming/lib/Controller/ThemingController.php b/apps/theming/lib/Controller/ThemingController.php index 4e9ce1b646e47..f41b9062a117e 100644 --- a/apps/theming/lib/Controller/ThemingController.php +++ b/apps/theming/lib/Controller/ThemingController.php @@ -51,6 +51,7 @@ use OCA\Theming\Util; use OCP\ITempManager; use OCP\IURLGenerator; +use OCP\App\IAppManager; /** * Class ThemingController @@ -78,6 +79,8 @@ class ThemingController extends Controller { private $scssCacher; /** @var IURLGenerator */ private $urlGenerator; + /** @var IAppManager */ + private $appManager; /** * ThemingController constructor. @@ -93,6 +96,7 @@ class ThemingController extends Controller { * @param IAppData $appData * @param SCSSCacher $scssCacher * @param IURLGenerator $urlGenerator + * @param IAppManager $appManager */ public function __construct( $appName, @@ -105,7 +109,8 @@ public function __construct( ITempManager $tempManager, IAppData $appData, SCSSCacher $scssCacher, - IURLGenerator $urlGenerator + IURLGenerator $urlGenerator, + IAppManager $appManager = NULL ) { parent::__construct($appName, $request); @@ -118,6 +123,12 @@ public function __construct( $this->appData = $appData; $this->scssCacher = $scssCacher; $this->urlGenerator = $urlGenerator; + + if (!is_null($appManager)) { + $this->appManager = $appManager; + } else { + $this->appManager = \OC::$server->getAppManager(); + } } /** @@ -409,12 +420,13 @@ public function getLoginBackground() { * @return FileDisplayResponse|NotFoundResponse */ public function getStylesheet() { - $appPath = substr(\OC::$server->getAppManager()->getAppPath('theming'), strlen(\OC::$SERVERROOT) + 1); + $appPath = $this->appManager->getAppPath('theming'); + /* SCSSCacher is required here * We cannot rely on automatic caching done by \OC_Util::addStyle, * since we need to add the cacheBuster value to the url */ - $cssCached = $this->scssCacher->process(\OC::$SERVERROOT, $appPath . '/css/theming.scss', 'theming'); + $cssCached = $this->scssCacher->process($appPath, 'css/theming.scss', 'theming'); if(!$cssCached) { return new NotFoundResponse(); } diff --git a/apps/theming/tests/Controller/ThemingControllerTest.php b/apps/theming/tests/Controller/ThemingControllerTest.php index 54b842bc4e88c..9ab8d7a06e912 100644 --- a/apps/theming/tests/Controller/ThemingControllerTest.php +++ b/apps/theming/tests/Controller/ThemingControllerTest.php @@ -829,6 +829,40 @@ public function testGetStylesheetFails() { $this->assertEquals($response, $actual); } + public function testGetStylesheetOutsideServerroot() { + $this->themingController = new ThemingController( + 'theming', + $this->request, + $this->config, + $this->themingDefaults, + $this->util, + $this->timeFactory, + $this->l10n, + $this->tempManager, + $this->appData, + $this->scssCacher, + $this->urlGenerator, + $this->appManager + ); + $this->appManager->expects($this->once())->method('getAppPath')->with('theming')->willReturn('/outside/serverroot/theming'); + $file = $this->createMock(ISimpleFile::class); + $file->expects($this->any())->method('getName')->willReturn('theming.css'); + $file->expects($this->any())->method('getContent')->willReturn('compiled'); + $this->scssCacher->expects($this->once())->method('process')->with('/outside/serverroot/theming', 'css/theming.scss', 'theming')->willReturn(true); + $this->scssCacher->expects($this->once())->method('getCachedCSS')->willReturn($file); + + $response = new Http\FileDisplayResponse($file, Http::STATUS_OK, ['Content-Type' => 'text/css']); + $response->cacheFor(86400); + $expires = new \DateTime(); + $expires->setTimestamp($this->timeFactory->getTime()); + $expires->add(new \DateInterval('PT24H')); + $response->addHeader('Expires', $expires->format(\DateTime::RFC1123)); + $response->addHeader('Pragma', 'cache'); + + $actual = $this->themingController->getStylesheet(); + $this->assertEquals($response, $actual); + } + public function testGetJavascript() { $this->themingDefaults ->expects($this->at(0)) From 25c89a6feb80ded179adf995eff8b0539673d287 Mon Sep 17 00:00:00 2001 From: Kyle Fazzari Date: Sat, 24 Feb 2018 08:16:42 -0800 Subject: [PATCH 2/3] Add mock to setup Signed-off-by: Kyle Fazzari --- .../Controller/ThemingControllerTest.php | 20 ++++--------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/apps/theming/tests/Controller/ThemingControllerTest.php b/apps/theming/tests/Controller/ThemingControllerTest.php index 9ab8d7a06e912..debc1b71e4740 100644 --- a/apps/theming/tests/Controller/ThemingControllerTest.php +++ b/apps/theming/tests/Controller/ThemingControllerTest.php @@ -106,7 +106,8 @@ public function setUp() { $this->tempManager, $this->appData, $this->scssCacher, - $this->urlGenerator + $this->urlGenerator, + $this->appManager ); return parent::setUp(); @@ -798,7 +799,7 @@ public function testGetLoginBackground() { public function testGetStylesheet() { - + $this->appManager->expects($this->once())->method('getAppPath')->with('theming')->willReturn(\OC::$SERVERROOT . '/theming'); $file = $this->createMock(ISimpleFile::class); $file->expects($this->any())->method('getName')->willReturn('theming.css'); $file->expects($this->any())->method('getContent')->willReturn('compiled'); @@ -818,6 +819,7 @@ public function testGetStylesheet() { } public function testGetStylesheetFails() { + $this->appManager->expects($this->once())->method('getAppPath')->with('theming')->willReturn(\OC::$SERVERROOT . '/theming'); $file = $this->createMock(ISimpleFile::class); $file->expects($this->any())->method('getName')->willReturn('theming.css'); $file->expects($this->any())->method('getContent')->willReturn('compiled'); @@ -830,20 +832,6 @@ public function testGetStylesheetFails() { } public function testGetStylesheetOutsideServerroot() { - $this->themingController = new ThemingController( - 'theming', - $this->request, - $this->config, - $this->themingDefaults, - $this->util, - $this->timeFactory, - $this->l10n, - $this->tempManager, - $this->appData, - $this->scssCacher, - $this->urlGenerator, - $this->appManager - ); $this->appManager->expects($this->once())->method('getAppPath')->with('theming')->willReturn('/outside/serverroot/theming'); $file = $this->createMock(ISimpleFile::class); $file->expects($this->any())->method('getName')->willReturn('theming.css'); From 8dcbea124ea91dc2a838ac16c0dc5e68dd384ab8 Mon Sep 17 00:00:00 2001 From: Kyle Fazzari Date: Sun, 25 Feb 2018 07:08:59 -0800 Subject: [PATCH 3/3] Remove null check Signed-off-by: Kyle Fazzari --- apps/theming/lib/Controller/ThemingController.php | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/apps/theming/lib/Controller/ThemingController.php b/apps/theming/lib/Controller/ThemingController.php index f41b9062a117e..e7865b90cf048 100644 --- a/apps/theming/lib/Controller/ThemingController.php +++ b/apps/theming/lib/Controller/ThemingController.php @@ -110,7 +110,7 @@ public function __construct( IAppData $appData, SCSSCacher $scssCacher, IURLGenerator $urlGenerator, - IAppManager $appManager = NULL + IAppManager $appManager ) { parent::__construct($appName, $request); @@ -123,12 +123,7 @@ public function __construct( $this->appData = $appData; $this->scssCacher = $scssCacher; $this->urlGenerator = $urlGenerator; - - if (!is_null($appManager)) { - $this->appManager = $appManager; - } else { - $this->appManager = \OC::$server->getAppManager(); - } + $this->appManager = $appManager; } /**