From c503ecd54495167f97b6602e5b41c1cf95467395 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Mon, 24 Nov 2014 16:24:26 +0100 Subject: [PATCH 1/3] Introduce app info xml parser including basic unit test - necessary for #10777 --- lib/private/app.php | 57 +-------------------- lib/private/app/infoparser.php | 92 ++++++++++++++++++++++++++++++++++ lib/private/helper.php | 5 +- lib/private/urlgenerator.php | 12 ++++- lib/public/iurlgenerator.php | 8 ++- tests/data/app/valid-info.xml | 22 ++++++++ tests/lib/app/infoparser.php | 57 +++++++++++++++++++++ 7 files changed, 193 insertions(+), 60 deletions(-) create mode 100644 lib/private/app/infoparser.php create mode 100644 tests/data/app/valid-info.xml create mode 100644 tests/lib/app/infoparser.php diff --git a/lib/private/app.php b/lib/private/app.php index bc9ca0351e..8e36d43bfb 100644 --- a/lib/private/app.php +++ b/lib/private/app.php @@ -635,63 +635,10 @@ class OC_App { } $file = self::getAppPath($appId) . '/appinfo/info.xml'; } - $data = array(); - if (!file_exists($file)) { - return null; - } - $content = @file_get_contents($file); - if (!$content) { - return null; - } - $xml = new SimpleXMLElement($content); - $data['info'] = array(); - $data['remote'] = array(); - $data['public'] = array(); - foreach ($xml->children() as $child) { - /** - * @var $child SimpleXMLElement - */ - if ($child->getName() == 'remote') { - foreach ($child->children() as $remote) { - /** - * @var $remote SimpleXMLElement - */ - $data['remote'][$remote->getName()] = (string)$remote; - } - } elseif ($child->getName() == 'public') { - foreach ($child->children() as $public) { - /** - * @var $public SimpleXMLElement - */ - $data['public'][$public->getName()] = (string)$public; - } - } elseif ($child->getName() == 'types') { - $data['types'] = array(); - foreach ($child->children() as $type) { - /** - * @var $type SimpleXMLElement - */ - $data['types'][] = $type->getName(); - } - } elseif ($child->getName() == 'description') { - $xml = (string)$child->asXML(); - $data[$child->getName()] = substr($xml, 13, -14); //script tags - } elseif ($child->getName() == 'documentation') { - foreach ($child as $subChild) { - $url = (string) $subChild; - // If it is not an absolute URL we assume it is a key - // i.e. admin-ldap will get converted to go.php?to=admin-ldap - if(!\OC::$server->getHTTPHelper()->isHTTPURL($url)) { - $url = OC_Helper::linkToDocs($url); - } + $parser = new \OC\App\InfoParser(\OC::$server->getHTTPHelper(), \OC::$server->getURLGenerator()); + $data = $parser->parse($file); - $data["documentation"][$subChild->getName()] = $url; - } - } else { - $data[$child->getName()] = (string)$child; - } - } self::$appInfo[$appId] = $data; return $data; diff --git a/lib/private/app/infoparser.php b/lib/private/app/infoparser.php new file mode 100644 index 0000000000..908fcd8263 --- /dev/null +++ b/lib/private/app/infoparser.php @@ -0,0 +1,92 @@ +httpHelper = $httpHelper; + $this->urlGenerator = $urlGenerator; + } + + /** + * @param string $file + * @return null|string + */ + public function parse($file) { + if (!file_exists($file)) { + return null; + } + + $content = @file_get_contents($file); + if (!$content) { + return null; + } + $xml = new SimpleXMLElement($content); + $data['info'] = array(); + $data['remote'] = array(); + $data['public'] = array(); + foreach ($xml->children() as $child) { + /** + * @var $child SimpleXMLElement + */ + if ($child->getName() == 'remote') { + foreach ($child->children() as $remote) { + /** + * @var $remote SimpleXMLElement + */ + $data['remote'][$remote->getName()] = (string)$remote; + } + } elseif ($child->getName() == 'public') { + foreach ($child->children() as $public) { + /** + * @var $public SimpleXMLElement + */ + $data['public'][$public->getName()] = (string)$public; + } + } elseif ($child->getName() == 'types') { + $data['types'] = array(); + foreach ($child->children() as $type) { + /** + * @var $type SimpleXMLElement + */ + $data['types'][] = $type->getName(); + } + } elseif ($child->getName() == 'description') { + $xml = (string)$child->asXML(); + $data[$child->getName()] = substr($xml, 13, -14); //script tags + } elseif ($child->getName() == 'documentation') { + foreach ($child as $subChild) { + $url = (string)$subChild; + + // If it is not an absolute URL we assume it is a key + // i.e. admin-ldap will get converted to go.php?to=admin-ldap + if (!$this->httpHelper->isHTTPURL($url)) { + $url = $this->urlGenerator->linkToDocs($url); + } + + $data["documentation"][$subChild->getName()] = $url; + } + } else { + $data[$child->getName()] = (string)$child; + } + } + + return $data; + } +} diff --git a/lib/private/helper.php b/lib/private/helper.php index be448b8ff9..d43eefcdc5 100644 --- a/lib/private/helper.php +++ b/lib/private/helper.php @@ -58,12 +58,11 @@ class OC_Helper { } /** - * @param $key + * @param string $key * @return string url to the online documentation */ public static function linkToDocs($key) { - $theme = new OC_Defaults(); - return $theme->buildDocLinkToKey($key); + return OC::$server->getURLGenerator()->linkToDocs($key); } /** diff --git a/lib/private/urlgenerator.php b/lib/private/urlgenerator.php index e50e9eed6a..d263d25aee 100644 --- a/lib/private/urlgenerator.php +++ b/lib/private/urlgenerator.php @@ -8,6 +8,7 @@ */ namespace OC; +use OC_Defaults; use OCP\IURLGenerator; use RuntimeException; @@ -156,7 +157,7 @@ class URLGenerator implements IURLGenerator { /** * Makes an URL absolute - * @param string $url the url in the owncloud host + * @param string $url the url in the ownCloud host * @return string the absolute version of the url */ public function getAbsoluteURL($url) { @@ -173,4 +174,13 @@ class URLGenerator implements IURLGenerator { return \OC_Request::serverProtocol() . '://' . \OC_Request::serverHost(). $webRoot . $separator . $url; } + + /** + * @param string $key + * @return string url to the online documentation + */ + public function linkToDocs($key) { + $theme = new OC_Defaults(); + return $theme->buildDocLinkToKey($key); + } } diff --git a/lib/public/iurlgenerator.php b/lib/public/iurlgenerator.php index dbbd8a3bb6..fa817c10ea 100644 --- a/lib/public/iurlgenerator.php +++ b/lib/public/iurlgenerator.php @@ -69,8 +69,14 @@ interface IURLGenerator { /** * Makes an URL absolute - * @param string $url the url in the owncloud host + * @param string $url the url in the ownCloud host * @return string the absolute version of the url */ public function getAbsoluteURL($url); + + /** + * @param string $key + * @return string url to the online documentation + */ + public function linkToDocs($key); } diff --git a/tests/data/app/valid-info.xml b/tests/data/app/valid-info.xml new file mode 100644 index 0000000000..6fcef693be --- /dev/null +++ b/tests/data/app/valid-info.xml @@ -0,0 +1,22 @@ + + + files_encryption + Server-side Encryption + + This application encrypts all files accessed by ownCloud at rest, wherever they are stored. As an example, with this application enabled, external cloud based Amazon S3 storage will be encrypted, protecting this data on storage outside of the control of the Admin. When this application is enabled for the first time, all files are encrypted as users log in and are prompted for their password. The recommended recovery key option enables recovery of files in case the key is lost. + Note that this app encrypts all files that are touched by ownCloud, so external storage providers and applications such as SharePoint will see new files encrypted when they are accessed. Encryption is based on AES 128 or 256 bit keys. More information is available in the Encryption documentation + + AGPL + Sam Tuke, Bjoern Schiessle, Florin Peter + 4 + true + + user-encryption + admin-encryption + + false + + + + 166047 + diff --git a/tests/lib/app/infoparser.php b/tests/lib/app/infoparser.php new file mode 100644 index 0000000000..d1b2313e88 --- /dev/null +++ b/tests/lib/app/infoparser.php @@ -0,0 +1,57 @@ +getMockBuilder('\OC\HTTPHelper') + ->disableOriginalConstructor() + ->getMock(); + + $httpHelper->expects($this->any()) + ->method('isHTTPURL') + ->will($this->returnCallback(function ($url) { + return stripos($url, 'https://') === 0 || stripos($url, 'http://') === 0; + })); + + $urlGenerator = $this->getMockBuilder('\OCP\IURLGenerator') + ->disableOriginalConstructor() + ->getMock(); + + //linkToDocs + $httpHelper->expects($this->any()) + ->method('linkToDocs') + ->will($this->returnCallback(function ($url) { + return $url; + })); + + $this->parser = new \OC\App\InfoParser($httpHelper, $urlGenerator); + } + + public function testParsingValidXml() { + $data = $this->parser->parse(OC::$SERVERROOT.'/tests/data/app/valid-info.xml'); + + $expectedKeys = array( + 'id', 'info', 'remote', 'public', 'name', 'description', 'licence', 'author', 'requiremin', 'shipped', + 'documentation', 'rememberlogin', 'types', 'ocsid' + ); + foreach($expectedKeys as $expectedKey) { + $this->assertArrayHasKey($expectedKey, $data, "ExpectedKey($expectedKey) was missing."); + } + } +} From d4f107d4dd11d11c52f419d2f33abc5dc4a93573 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Mon, 24 Nov 2014 17:26:07 +0100 Subject: [PATCH 2/3] simplify xml parser code --- lib/private/app/infoparser.php | 77 ++++++++++++------------------- tests/data/app/expected-info.json | 19 ++++++++ tests/lib/app/infoparser.php | 25 ++++------ 3 files changed, 56 insertions(+), 65 deletions(-) create mode 100644 tests/data/app/expected-info.json diff --git a/lib/private/app/infoparser.php b/lib/private/app/infoparser.php index 908fcd8263..f7c3f8213a 100644 --- a/lib/private/app/infoparser.php +++ b/lib/private/app/infoparser.php @@ -33,60 +33,41 @@ class InfoParser { return null; } - $content = @file_get_contents($file); - if (!$content) { + $xml = simplexml_load_file($file); + $array = json_decode(json_encode((array)$xml), TRUE); + if (is_null($array)) { return null; } - $xml = new SimpleXMLElement($content); - $data['info'] = array(); - $data['remote'] = array(); - $data['public'] = array(); - foreach ($xml->children() as $child) { - /** - * @var $child SimpleXMLElement - */ - if ($child->getName() == 'remote') { - foreach ($child->children() as $remote) { - /** - * @var $remote SimpleXMLElement - */ - $data['remote'][$remote->getName()] = (string)$remote; - } - } elseif ($child->getName() == 'public') { - foreach ($child->children() as $public) { - /** - * @var $public SimpleXMLElement - */ - $data['public'][$public->getName()] = (string)$public; - } - } elseif ($child->getName() == 'types') { - $data['types'] = array(); - foreach ($child->children() as $type) { - /** - * @var $type SimpleXMLElement - */ - $data['types'][] = $type->getName(); - } - } elseif ($child->getName() == 'description') { - $xml = (string)$child->asXML(); - $data[$child->getName()] = substr($xml, 13, -14); //script tags - } elseif ($child->getName() == 'documentation') { - foreach ($child as $subChild) { - $url = (string)$subChild; + if (!array_key_exists('info', $array)) { + $array['info'] = array(); + } + if (!array_key_exists('remote', $array)) { + $array['remote'] = array(); + } + if (!array_key_exists('public', $array)) { + $array['public'] = array(); + } - // If it is not an absolute URL we assume it is a key - // i.e. admin-ldap will get converted to go.php?to=admin-ldap - if (!$this->httpHelper->isHTTPURL($url)) { - $url = $this->urlGenerator->linkToDocs($url); - } - - $data["documentation"][$subChild->getName()] = $url; + if (array_key_exists('documentation', $array)) { + foreach ($array['documentation'] as $key => $url) { + // If it is not an absolute URL we assume it is a key + // i.e. admin-ldap will get converted to go.php?to=admin-ldap + if (!$this->httpHelper->isHTTPURL($url)) { + $url = $this->urlGenerator->linkToDocs($url); } - } else { - $data[$child->getName()] = (string)$child; + + $array["documentation"][$key] = $url; + + } + } + if (array_key_exists('types', $array)) { + foreach ($array['types'] as $type => $v) { + unset($array['types'][$type]); + $array['types'][] = $type; + } } - return $data; + return $array; } } diff --git a/tests/data/app/expected-info.json b/tests/data/app/expected-info.json new file mode 100644 index 0000000000..c67d6d657d --- /dev/null +++ b/tests/data/app/expected-info.json @@ -0,0 +1,19 @@ +{ + "info": [], + "remote": [], + "public": [], + "id": "files_encryption", + "name": "Server-side Encryption", + "description": "\n\tThis application encrypts all files accessed by ownCloud at rest, wherever they are stored. As an example, with this application enabled, external cloud based Amazon S3 storage will be encrypted, protecting this data on storage outside of the control of the Admin. When this application is enabled for the first time, all files are encrypted as users log in and are prompted for their password. The recommended recovery key option enables recovery of files in case the key is lost. \n\tNote that this app encrypts all files that are touched by ownCloud, so external storage providers and applications such as SharePoint will see new files encrypted when they are accessed. Encryption is based on AES 128 or 256 bit keys. More information is available in the Encryption documentation \n\t", + "licence": "AGPL", + "author": "Sam Tuke, Bjoern Schiessle, Florin Peter", + "requiremin": "4", + "shipped": "true", + "documentation": { + "user": "https://docs.example.com/server/go.php?to=user-encryption", + "admin": "https://docs.example.com/server/go.php?to=admin-encryption" + }, + "rememberlogin": "false", + "types": ["filesystem"], + "ocsid": "166047" +} diff --git a/tests/lib/app/infoparser.php b/tests/lib/app/infoparser.php index d1b2313e88..e416202a30 100644 --- a/tests/lib/app/infoparser.php +++ b/tests/lib/app/infoparser.php @@ -19,39 +19,30 @@ class InfoParser extends \PHPUnit_Framework_TestCase { private $parser; public function setUp() { + $config = $this->getMockBuilder('\OC\AllConfig') + ->disableOriginalConstructor()->getMock(); $httpHelper = $this->getMockBuilder('\OC\HTTPHelper') - ->disableOriginalConstructor() + ->setConstructorArgs(array($config)) + ->setMethods(array('getHeaders')) ->getMock(); - - $httpHelper->expects($this->any()) - ->method('isHTTPURL') - ->will($this->returnCallback(function ($url) { - return stripos($url, 'https://') === 0 || stripos($url, 'http://') === 0; - })); - $urlGenerator = $this->getMockBuilder('\OCP\IURLGenerator') ->disableOriginalConstructor() ->getMock(); //linkToDocs - $httpHelper->expects($this->any()) + $urlGenerator->expects($this->any()) ->method('linkToDocs') ->will($this->returnCallback(function ($url) { - return $url; + return "https://docs.example.com/server/go.php?to=$url"; })); $this->parser = new \OC\App\InfoParser($httpHelper, $urlGenerator); } public function testParsingValidXml() { + $expectedData = json_decode(file_get_contents(OC::$SERVERROOT.'/tests/data/app/expected-info.json'), true); $data = $this->parser->parse(OC::$SERVERROOT.'/tests/data/app/valid-info.xml'); - $expectedKeys = array( - 'id', 'info', 'remote', 'public', 'name', 'description', 'licence', 'author', 'requiremin', 'shipped', - 'documentation', 'rememberlogin', 'types', 'ocsid' - ); - foreach($expectedKeys as $expectedKey) { - $this->assertArrayHasKey($expectedKey, $data, "ExpectedKey($expectedKey) was missing."); - } + $this->assertEquals($expectedData, $data); } } From 5ce34fbaf69538ad3338beebdfb015e94f8b6a3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Tue, 25 Nov 2014 10:22:06 +0100 Subject: [PATCH 3/3] handle invalid xml file --- lib/private/app/infoparser.php | 25 ++++++++++++++++++------- tests/data/app/invalid-info.xml | 22 ++++++++++++++++++++++ tests/lib/app/infoparser.php | 5 +++++ 3 files changed, 45 insertions(+), 7 deletions(-) create mode 100644 tests/data/app/invalid-info.xml diff --git a/lib/private/app/infoparser.php b/lib/private/app/infoparser.php index f7c3f8213a..b4bdbea5c0 100644 --- a/lib/private/app/infoparser.php +++ b/lib/private/app/infoparser.php @@ -11,9 +11,17 @@ namespace OC\App; use OCP\IURLGenerator; -use SimpleXMLElement; class InfoParser { + /** + * @var \OC\HTTPHelper + */ + private $httpHelper; + + /** + * @var IURLGenerator + */ + private $urlGenerator; /** * @param \OC\HTTPHelper $httpHelper @@ -25,15 +33,20 @@ class InfoParser { } /** - * @param string $file - * @return null|string + * @param string $file the xml file to be loaded + * @return null|array where null is an indicator for an error */ public function parse($file) { if (!file_exists($file)) { return null; } - $xml = simplexml_load_file($file); + $loadEntities = libxml_disable_entity_loader(false); + $xml = @simplexml_load_file($file); + libxml_disable_entity_loader($loadEntities); + if ($xml == false) { + return null; + } $array = json_decode(json_encode((array)$xml), TRUE); if (is_null($array)) { return null; @@ -56,15 +69,13 @@ class InfoParser { $url = $this->urlGenerator->linkToDocs($url); } - $array["documentation"][$key] = $url; - + $array['documentation'][$key] = $url; } } if (array_key_exists('types', $array)) { foreach ($array['types'] as $type => $v) { unset($array['types'][$type]); $array['types'][] = $type; - } } diff --git a/tests/data/app/invalid-info.xml b/tests/data/app/invalid-info.xml new file mode 100644 index 0000000000..3947f5420c --- /dev/null +++ b/tests/data/app/invalid-info.xml @@ -0,0 +1,22 @@ + +files_encryption + Server-side Encryption + + This application encrypts all files accessed by ownCloud at rest, wherever they are stored. As an example, with this application enabled, external cloud based Amazon S3 storage will be encrypted, protecting this data on storage outside of the control of the Admin. When this application is enabled for the first time, all files are encrypted as users log in and are prompted for their password. The recommended recovery key option enables recovery of files in case the key is lost. + Note that this app encrypts all files that are touched by ownCloud, so external storage providers and applications such as SharePoint will see new files encrypted when they are accessed. Encryption is based on AES 128 or 256 bit keys. More information is available in the Encryption documentation + + AGPL + Sam Tuke, Bjoern Schiessle, Florin Peter + 4 + true + + user-encryption + admin-encryption + + false + + + + 166047 + diff --git a/tests/lib/app/infoparser.php b/tests/lib/app/infoparser.php index e416202a30..277e1582e4 100644 --- a/tests/lib/app/infoparser.php +++ b/tests/lib/app/infoparser.php @@ -45,4 +45,9 @@ class InfoParser extends \PHPUnit_Framework_TestCase { $this->assertEquals($expectedData, $data); } + + public function testParsingInvalidXml() { + $data = $this->parser->parse(OC::$SERVERROOT.'/tests/data/app/invalid-info.xml'); + $this->assertNull($data); + } }