-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
{J,CS}SResourceLocator: account for symlinks in app path #7061
Merged
MorrisJobke
merged 4 commits into
nextcloud:master
from
kyrofa:bugfix/6028/app_path_realpath
Nov 14, 2017
Merged
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,150 @@ | ||
<?php | ||
/** | ||
* @copyright Copyright (c) 2017 Kyle Fazzari <kyrofa@ubuntu.com> | ||
* | ||
* @author Kyle Fazzari <kyrofa@ubuntu.com> | ||
* | ||
* @license GNU AGPL version 3 or any later version | ||
* | ||
* This program is free software: you can redistribute it and/or modify | ||
* it under the terms of the GNU Affero General Public License as | ||
* published by the Free Software Foundation, either version 3 of the | ||
* License, or (at your option) any later version. | ||
* | ||
* This program is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
* GNU Affero General Public License for more details. | ||
* | ||
* You should have received a copy of the GNU Affero General Public License | ||
* along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
* | ||
*/ | ||
|
||
namespace Test\Template; | ||
|
||
use OC\Files\AppData\Factory; | ||
use OCP\ILogger; | ||
use OCP\IURLGenerator; | ||
use OCP\IConfig; | ||
use OCA\Theming\ThemingDefaults; | ||
use OCP\ICache; | ||
use OC\Template\SCSSCacher; | ||
use OC\Template\CSSResourceLocator; | ||
|
||
class CSSResourceLocatorTest extends \Test\TestCase { | ||
/** @var IAppData|\PHPUnit_Framework_MockObject_MockObject */ | ||
protected $appData; | ||
/** @var IURLGenerator|\PHPUnit_Framework_MockObject_MockObject */ | ||
protected $urlGenerator; | ||
/** @var SystemConfig|\PHPUnit_Framework_MockObject_MockObject */ | ||
protected $config; | ||
/** @var ThemingDefaults|\PHPUnit_Framework_MockObject_MockObject */ | ||
protected $themingDefaults; | ||
/** @var ICache|\PHPUnit_Framework_MockObject_MockObject */ | ||
protected $depsCache; | ||
/** @var ILogger|\PHPUnit_Framework_MockObject_MockObject */ | ||
protected $logger; | ||
|
||
protected function setUp() { | ||
parent::setUp(); | ||
|
||
$this->logger = $this->createMock(ILogger::class); | ||
$this->appData = $this->createMock(IAppData::class); | ||
$this->urlGenerator = $this->createMock(IURLGenerator::class); | ||
$this->config = $this->createMock(IConfig::class); | ||
$this->depsCache = $this->createMock(ICache::class); | ||
$this->themingDefaults = $this->createMock(ThemingDefaults::class); | ||
} | ||
|
||
private function cssResourceLocator() { | ||
/** @var Factory|\PHPUnit_Framework_MockObject_MockObject $factory */ | ||
$factory = $this->createMock(Factory::class); | ||
$factory->method('get')->with('css')->willReturn($this->appData); | ||
$scssCacher = new SCSSCacher( | ||
$this->logger, | ||
$factory, | ||
$this->urlGenerator, | ||
$this->config, | ||
$this->themingDefaults, | ||
\OC::$SERVERROOT, | ||
$this->depsCache | ||
); | ||
return new CSSResourceLocator( | ||
$this->logger, | ||
'theme', | ||
array('core'=>'map'), | ||
array('3rd'=>'party'), | ||
$scssCacher | ||
); | ||
} | ||
|
||
private function rrmdir($directory) { | ||
$files = array_diff(scandir($directory), array('.','..')); | ||
foreach ($files as $file) { | ||
if (is_dir($directory . '/' . $file)) { | ||
$this->rrmdir($directory . '/' . $file); | ||
} else { | ||
unlink($directory . '/' . $file); | ||
} | ||
} | ||
return rmdir($directory); | ||
} | ||
|
||
private function randomString() { | ||
return sha1(uniqid(mt_rand(), true)); | ||
} | ||
|
||
public function testConstructor() { | ||
$locator = $this->cssResourceLocator(); | ||
$this->assertAttributeEquals('theme', 'theme', $locator); | ||
$this->assertAttributeEquals('core', 'serverroot', $locator); | ||
$this->assertAttributeEquals(array('core'=>'map','3rd'=>'party'), 'mapping', $locator); | ||
$this->assertAttributeEquals('3rd', 'thirdpartyroot', $locator); | ||
$this->assertAttributeEquals('map', 'webroot', $locator); | ||
$this->assertAttributeEquals(array(), 'resources', $locator); | ||
} | ||
|
||
public function testFindWithAppPathSymlink() { | ||
// First create new apps path, and a symlink to it | ||
$apps_dirname = $this->randomString(); | ||
$new_apps_path = sys_get_temp_dir() . '/' . $apps_dirname; | ||
$new_apps_path_symlink = $new_apps_path . '_link'; | ||
mkdir($new_apps_path); | ||
symlink($apps_dirname, $new_apps_path_symlink); | ||
|
||
// Create an app within that path | ||
mkdir($new_apps_path . '/' . 'test-css-app'); | ||
|
||
// Use the symlink as the app path | ||
\OC::$APPSROOTS[] = [ | ||
'path' => $new_apps_path_symlink, | ||
'url' => '/css-apps-test', | ||
'writable' => false, | ||
]; | ||
|
||
$locator = $this->cssResourceLocator(); | ||
$locator->find(array('test-css-app/test-file')); | ||
|
||
$resources = $locator->getResources(); | ||
$this->assertCount(1, $resources); | ||
$resource = $resources[0]; | ||
$this->assertCount(3, $resource); | ||
$root = $resource[0]; | ||
$webRoot = $resource[1]; | ||
$file = $resource[2]; | ||
|
||
$expectedRoot = $new_apps_path . '/test-css-app'; | ||
$expectedWebRoot = \OC::$WEBROOT . '/css-apps-test/test-css-app'; | ||
$expectedFile = 'test-file.css'; | ||
|
||
$this->assertEquals($expectedRoot, $root, | ||
'Ensure the app path symlink is resolved into the real path'); | ||
$this->assertEquals($expectedWebRoot, $webRoot); | ||
$this->assertEquals($expectedFile, $file); | ||
|
||
array_pop(\OC::$APPSROOTS); | ||
unlink($new_apps_path_symlink); | ||
$this->rrmdir($new_apps_path); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,142 @@ | ||
<?php | ||
/** | ||
* @copyright Copyright (c) 2017 Kyle Fazzari <kyrofa@ubuntu.com> | ||
* | ||
* @author Kyle Fazzari <kyrofa@ubuntu.com> | ||
* | ||
* @license GNU AGPL version 3 or any later version | ||
* | ||
* This program is free software: you can redistribute it and/or modify | ||
* it under the terms of the GNU Affero General Public License as | ||
* published by the Free Software Foundation, either version 3 of the | ||
* License, or (at your option) any later version. | ||
* | ||
* This program is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
* GNU Affero General Public License for more details. | ||
* | ||
* You should have received a copy of the GNU Affero General Public License | ||
* along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
* | ||
*/ | ||
|
||
namespace Test\Template; | ||
|
||
use OC\Template\JSCombiner; | ||
use OCP\Files\IAppData; | ||
use OCP\IURLGenerator; | ||
use OCP\ICache; | ||
use OC\SystemConfig; | ||
use OCP\ILogger; | ||
use OC\Template\JSResourceLocator; | ||
|
||
class JSResourceLocatorTest extends \Test\TestCase { | ||
/** @var IAppData|\PHPUnit_Framework_MockObject_MockObject */ | ||
protected $appData; | ||
/** @var IURLGenerator|\PHPUnit_Framework_MockObject_MockObject */ | ||
protected $urlGenerator; | ||
/** @var SystemConfig|\PHPUnit_Framework_MockObject_MockObject */ | ||
protected $config; | ||
/** @var ICache|\PHPUnit_Framework_MockObject_MockObject */ | ||
protected $depsCache; | ||
/** @var ILogger|\PHPUnit_Framework_MockObject_MockObject */ | ||
protected $logger; | ||
|
||
protected function setUp() { | ||
parent::setUp(); | ||
|
||
$this->appData = $this->createMock(IAppData::class); | ||
$this->urlGenerator = $this->createMock(IURLGenerator::class); | ||
$this->config = $this->createMock(SystemConfig::class); | ||
$this->depsCache = $this->createMock(ICache::class); | ||
$this->logger = $this->createMock(ILogger::class); | ||
} | ||
|
||
private function jsResourceLocator() { | ||
$jsCombiner = new JSCombiner( | ||
$this->appData, | ||
$this->urlGenerator, | ||
$this->depsCache, | ||
$this->config, | ||
$this->logger | ||
); | ||
return new JSResourceLocator( | ||
$this->logger, | ||
'theme', | ||
array('core'=>'map'), | ||
array('3rd'=>'party'), | ||
$jsCombiner | ||
); | ||
} | ||
|
||
private function rrmdir($directory) { | ||
$files = array_diff(scandir($directory), array('.','..')); | ||
foreach ($files as $file) { | ||
if (is_dir($directory . '/' . $file)) { | ||
$this->rrmdir($directory . '/' . $file); | ||
} else { | ||
unlink($directory . '/' . $file); | ||
} | ||
} | ||
return rmdir($directory); | ||
} | ||
|
||
private function randomString() { | ||
return sha1(uniqid(mt_rand(), true)); | ||
} | ||
|
||
|
||
public function testConstructor() { | ||
$locator = $this->jsResourceLocator(); | ||
$this->assertAttributeEquals('theme', 'theme', $locator); | ||
$this->assertAttributeEquals('core', 'serverroot', $locator); | ||
$this->assertAttributeEquals(array('core'=>'map','3rd'=>'party'), 'mapping', $locator); | ||
$this->assertAttributeEquals('3rd', 'thirdpartyroot', $locator); | ||
$this->assertAttributeEquals('map', 'webroot', $locator); | ||
$this->assertAttributeEquals(array(), 'resources', $locator); | ||
} | ||
|
||
public function testFindWithAppPathSymlink() { | ||
// First create new apps path, and a symlink to it | ||
$apps_dirname = $this->randomString(); | ||
$new_apps_path = sys_get_temp_dir() . '/' . $apps_dirname; | ||
$new_apps_path_symlink = $new_apps_path . '_link'; | ||
mkdir($new_apps_path); | ||
symlink($apps_dirname, $new_apps_path_symlink); | ||
|
||
// Create an app within that path | ||
mkdir($new_apps_path . '/' . 'test-js-app'); | ||
|
||
// Use the symlink as the app path | ||
\OC::$APPSROOTS[] = [ | ||
'path' => $new_apps_path_symlink, | ||
'url' => '/js-apps-test', | ||
'writable' => false, | ||
]; | ||
|
||
$locator = $this->jsResourceLocator(); | ||
$locator->find(array('test-js-app/test-file')); | ||
|
||
$resources = $locator->getResources(); | ||
$this->assertCount(1, $resources); | ||
$resource = $resources[0]; | ||
$this->assertCount(3, $resource); | ||
$root = $resource[0]; | ||
$webRoot = $resource[1]; | ||
$file = $resource[2]; | ||
|
||
$expectedRoot = $new_apps_path . '/test-js-app'; | ||
$expectedWebRoot = \OC::$WEBROOT . '/js-apps-test/test-js-app'; | ||
$expectedFile = 'test-file.js'; | ||
|
||
$this->assertEquals($expectedRoot, $root, | ||
'Ensure the app path symlink is resolved into the real path'); | ||
$this->assertEquals($expectedWebRoot, $webRoot); | ||
$this->assertEquals($expectedFile, $file); | ||
|
||
array_pop(\OC::$APPSROOTS); | ||
unlink($new_apps_path_symlink); | ||
$this->rrmdir($new_apps_path); | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we also do this after the check if the app_path is empty like it is done in in the CSSResourceLocator? Otherwise $real_app_path still might be the working directory for empty app paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the diff is hiding that from you, expand is a little below this. It's done here. I believe it's done after looking for l10n because those should be able to not exist without raising errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although this will cause weird behavior with using the cwd for l10n, huh. I refactored it just a tad to account for that, and pushed it up. It means we need to check twice, but I don't see a straight-forward way around that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you just move the check got app_path and app_url in front of the realpath call? That way we don't append any js sources at cwd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but not without changing the
l10n
behavior. I don't claim to be an expert in the code, of course, but right now it seems that if al10n
file doesn't exist, the error is ignored. If we move the check above that logic, we start raising an error instead. Are we wanting that change?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok, makes sense to keep it like this i guess. 👍