From 5fbf184134f34633bc150b2e0210c4a97ec285a9 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Tue, 24 Apr 2018 22:14:00 +0200 Subject: [PATCH 1/9] destaticfy Log classes Signed-off-by: Arthur Schiwon --- .../Sabre/ExceptionLoggerPluginTest.php | 2 +- lib/composer/composer/autoload_classmap.php | 3 + lib/composer/composer/autoload_static.php | 3 + lib/private/Log.php | 46 ++---- lib/private/Log/Errorlog.php | 11 +- lib/private/Log/File.php | 52 +++--- lib/private/Log/IFileBased.php | 30 ++++ lib/private/Log/IWritable.php | 28 ++++ lib/private/Log/LogFactory.php | 66 ++++++++ lib/private/Log/Syslog.php | 17 +- lib/private/Server.php | 5 +- settings/Controller/LogSettingsController.php | 17 +- tests/lib/Log/FileTest.php | 11 +- tests/lib/Log/LogFactoryTest.php | 149 ++++++++++++++++++ tests/lib/LoggerTest.php | 33 +--- 15 files changed, 356 insertions(+), 117 deletions(-) create mode 100644 lib/private/Log/IFileBased.php create mode 100644 lib/private/Log/IWritable.php create mode 100644 lib/private/Log/LogFactory.php create mode 100644 tests/lib/Log/LogFactoryTest.php diff --git a/apps/dav/tests/unit/Connector/Sabre/ExceptionLoggerPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/ExceptionLoggerPluginTest.php index c1d48a7ce5da3..2f5d4ac4b77b4 100644 --- a/apps/dav/tests/unit/Connector/Sabre/ExceptionLoggerPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/ExceptionLoggerPluginTest.php @@ -69,7 +69,7 @@ private function init() { }); $this->server = new Server(); - $this->logger = new TestLogger(Log\File::class, $config); + $this->logger = new TestLogger(new Log\File(\OC::$SERVERROOT.'/data/nextcloud.log'), $config); $this->plugin = new PluginToTest('unit-test', $this->logger); $this->plugin->initialize($this->server); } diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 19cb583cc932a..f706fea2cbe59 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -750,6 +750,9 @@ 'OC\\Log\\Errorlog' => $baseDir . '/lib/private/Log/Errorlog.php', 'OC\\Log\\ExceptionSerializer' => $baseDir . '/lib/private/Log/ExceptionSerializer.php', 'OC\\Log\\File' => $baseDir . '/lib/private/Log/File.php', + 'OC\\Log\\IFileBased' => $baseDir . '/lib/private/Log/IFileBased.php', + 'OC\\Log\\IWritable' => $baseDir . '/lib/private/Log/IWritable.php', + 'OC\\Log\\LogFactory' => $baseDir . '/lib/private/Log/LogFactory.php', 'OC\\Log\\Rotate' => $baseDir . '/lib/private/Log/Rotate.php', 'OC\\Log\\Syslog' => $baseDir . '/lib/private/Log/Syslog.php', 'OC\\Mail\\Attachment' => $baseDir . '/lib/private/Mail/Attachment.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index a913b0498b96b..b879dc5a214f9 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -780,6 +780,9 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\Log\\Errorlog' => __DIR__ . '/../../..' . '/lib/private/Log/Errorlog.php', 'OC\\Log\\ExceptionSerializer' => __DIR__ . '/../../..' . '/lib/private/Log/ExceptionSerializer.php', 'OC\\Log\\File' => __DIR__ . '/../../..' . '/lib/private/Log/File.php', + 'OC\\Log\\IFileBased' => __DIR__ . '/../../..' . '/lib/private/Log/IFileBased.php', + 'OC\\Log\\IWritable' => __DIR__ . '/../../..' . '/lib/private/Log/IWritable.php', + 'OC\\Log\\LogFactory' => __DIR__ . '/../../..' . '/lib/private/Log/LogFactory.php', 'OC\\Log\\Rotate' => __DIR__ . '/../../..' . '/lib/private/Log/Rotate.php', 'OC\\Log\\Syslog' => __DIR__ . '/../../..' . '/lib/private/Log/Syslog.php', 'OC\\Mail\\Attachment' => __DIR__ . '/../../..' . '/lib/private/Mail/Attachment.php', diff --git a/lib/private/Log.php b/lib/private/Log.php index ffe8c665c6fb9..c6f676db5886e 100644 --- a/lib/private/Log.php +++ b/lib/private/Log.php @@ -38,7 +38,8 @@ use InterfaSys\LogNormalizer\Normalizer; use OC\Log\ExceptionSerializer; -use OC\Log\File; +use OC\Log\IFileBased; +use OC\Log\IWritable; use OCP\ILogger; use OCP\Support\CrashReport\IRegistry; use OCP\Util; @@ -54,7 +55,7 @@ */ class Log implements ILogger { - /** @var string */ + /** @var IWritable */ private $logger; /** @var SystemConfig */ @@ -70,27 +71,19 @@ class Log implements ILogger { private $crashReporters; /** - * @param string $logger The logger that should be used + * @param IWritable $logger The logger that should be used * @param SystemConfig $config the system config object * @param Normalizer|null $normalizer * @param IRegistry|null $registry */ - public function __construct($logger = null, SystemConfig $config = null, $normalizer = null, IRegistry $registry = null) { + public function __construct(IWritable $logger, SystemConfig $config = null, $normalizer = null, IRegistry $registry = null) { // FIXME: Add this for backwards compatibility, should be fixed at some point probably if ($config === null) { $config = \OC::$server->getSystemConfig(); } $this->config = $config; - - // FIXME: Add this for backwards compatibility, should be fixed at some point probably - if ($logger === null) { - $logType = $this->config->getValue('log_type', 'file'); - $this->logger = static::getLogClass($logType); - call_user_func([$this->logger, 'init']); - } else { - $this->logger = $logger; - } + $this->logger = $logger; if ($normalizer === null) { $this->normalizer = new Normalizer(); } else { @@ -302,7 +295,7 @@ public function logException(\Throwable $exception, array $context = []) { array_walk($context, [$this->normalizer, 'format']); if ($level >= $minLevel) { - if ($this->logger !== File::class) { + if (!$this->logger instanceof IFileBased) { $data = json_encode($data, JSON_PARTIAL_OUTPUT_ON_ERROR); } $this->writeLog($app, $data, $level); @@ -320,28 +313,13 @@ public function logException(\Throwable $exception, array $context = []) { * @param int $level */ protected function writeLog(string $app, $entry, int $level) { - call_user_func([$this->logger, 'write'], $app, $entry, $level); + $this->logger->write($app, $entry, $level); } - /** - * @param string $logType - * @return string - * @internal - */ - public static function getLogClass(string $logType): string { - switch (strtolower($logType)) { - case 'errorlog': - return \OC\Log\Errorlog::class; - case 'syslog': - return \OC\Log\Syslog::class; - case 'file': - return \OC\Log\File::class; - - // Backwards compatibility for old and fallback for unknown log types - case 'owncloud': - case 'nextcloud': - default: - return \OC\Log\File::class; + public function getLogPath():string { + if($this->logger instanceof IFileBased) { + return $this->logger->getLogFilePath(); } + throw new \RuntimeException('Log implementation has no path'); } } diff --git a/lib/private/Log/Errorlog.php b/lib/private/Log/Errorlog.php index 37498c36aba9a..1302a31fe5c99 100644 --- a/lib/private/Log/Errorlog.php +++ b/lib/private/Log/Errorlog.php @@ -25,14 +25,7 @@ namespace OC\Log; -class Errorlog { - - - /** - * Init class data - */ - public static function init() { - } +class Errorlog implements IWritable { /** * write a message in the log @@ -40,7 +33,7 @@ public static function init() { * @param string $message * @param int $level */ - public static function write($app, $message, $level) { + public function write($app, $message, $level) { error_log('[owncloud]['.$app.']['.$level.'] '.$message); } } diff --git a/lib/private/Log/File.php b/lib/private/Log/File.php index 755c4729c7a61..639e4de8ac7aa 100644 --- a/lib/private/Log/File.php +++ b/lib/private/Log/File.php @@ -45,28 +45,21 @@ * Log is saved at data/nextcloud.log (on default) */ -class File { - static protected $logFile; +class File implements IWritable, IFileBased { + /** @var string */ + protected $logFile; - /** - * Init class data - */ - public static function init() { - $systemConfig = \OC::$server->getSystemConfig(); - $defaultLogFile = $systemConfig->getValue("datadirectory", \OC::$SERVERROOT.'/data').'/nextcloud.log'; - self::$logFile = $systemConfig->getValue("logfile", $defaultLogFile); - - /** - * Fall back to default log file if specified logfile does not exist - * and can not be created. - */ - if (!file_exists(self::$logFile)) { - if(!is_writable(dirname(self::$logFile))) { - self::$logFile = $defaultLogFile; - } else { - if(!touch(self::$logFile)) { - self::$logFile = $defaultLogFile; - } + public function __construct(string $path, string $fallbackPath = '') { + $this->logFile = $path; + if (!file_exists($this->logFile)) { + if( + ( + !is_writable(dirname($this->logFile)) + || !touch($this->logFile) + ) + && $fallbackPath !== '' + ) { + $this->logFile = $fallbackPath; } } } @@ -77,7 +70,7 @@ public static function init() { * @param string|array $message * @param int $level */ - public static function write($app, $message, $level) { + public function write($app, $message, $level) { $config = \OC::$server->getSystemConfig(); // default to ISO8601 @@ -137,9 +130,9 @@ public static function write($app, $message, $level) { } } $entry = json_encode($entry, JSON_PARTIAL_OUTPUT_ON_ERROR); - $handle = @fopen(self::$logFile, 'a'); - if ((fileperms(self::$logFile) & 0777) != 0640) { - @chmod(self::$logFile, 0640); + $handle = @fopen($this->logFile, 'a'); + if ((fileperms($this->logFile) & 0777) != 0640) { + @chmod($this->logFile, 0640); } if ($handle) { fwrite($handle, $entry."\n"); @@ -159,11 +152,10 @@ public static function write($app, $message, $level) { * @param int $offset * @return array */ - public static function getEntries($limit=50, $offset=0) { - self::init(); + public function getEntries($limit=50, $offset=0) { $minLevel = \OC::$server->getSystemConfig()->getValue("loglevel", ILogger::WARN); $entries = array(); - $handle = @fopen(self::$logFile, 'rb'); + $handle = @fopen($this->logFile, 'rb'); if ($handle) { fseek($handle, 0, SEEK_END); $pos = ftell($handle); @@ -205,7 +197,7 @@ public static function getEntries($limit=50, $offset=0) { /** * @return string */ - public static function getLogFilePath() { - return self::$logFile; + public function getLogFilePath() { + return $this->logFile; } } diff --git a/lib/private/Log/IFileBased.php b/lib/private/Log/IFileBased.php new file mode 100644 index 0000000000000..ae083f670e281 --- /dev/null +++ b/lib/private/Log/IFileBased.php @@ -0,0 +1,30 @@ + + * + * @author Arthur Schiwon + * + * @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 . + * + */ + +namespace OC\Log; + +interface IFileBased { + public function getLogFilePath(); + + public function getEntries($limit=50, $offset=0); +} diff --git a/lib/private/Log/IWritable.php b/lib/private/Log/IWritable.php new file mode 100644 index 0000000000000..ff9bb4e71a823 --- /dev/null +++ b/lib/private/Log/IWritable.php @@ -0,0 +1,28 @@ + + * + * @author Arthur Schiwon + * + * @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 . + * + */ + +namespace OC\Log; + +interface IWritable { + public function write($app, $message, $level); +} diff --git a/lib/private/Log/LogFactory.php b/lib/private/Log/LogFactory.php new file mode 100644 index 0000000000000..13049083feed4 --- /dev/null +++ b/lib/private/Log/LogFactory.php @@ -0,0 +1,66 @@ + + * + * @author Arthur Schiwon + * + * @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 . + * + */ + +namespace OC\Log; + +use OCP\IServerContainer; + +class LogFactory { + /** @var IServerContainer */ + private $c; + + public function __construct(IServerContainer $c) { + $this->c = $c; + } + + /** + * @param $type + * @return \OC\Log\Errorlog|File|\stdClass + * @throws \OCP\AppFramework\QueryException + */ + public function get($type) { + switch (strtolower($type)) { + case 'errorlog': + return new Errorlog(); + case 'syslog': + return $this->c->resolve(Syslog::class); + case 'file': + return $this->buildLogFile(); + + // Backwards compatibility for old and fallback for unknown log types + case 'owncloud': + case 'nextcloud': + default: + return $this->buildLogFile(); + } + } + + protected function buildLogFile() { + $config = $this->c->getConfig(); + $defaultLogFile = $config->getSystemValue('datadirectory', \OC::$SERVERROOT.'/data').'/nextcloud.log'; + $logFile = $config->getSystemValue('logfile', $defaultLogFile); + $fallback = $defaultLogFile !== $logFile ? $defaultLogFile : ''; + + return new File($logFile, $fallback); + } +} diff --git a/lib/private/Log/Syslog.php b/lib/private/Log/Syslog.php index 7b3d931ef31ba..be8ecdebb2e77 100644 --- a/lib/private/Log/Syslog.php +++ b/lib/private/Log/Syslog.php @@ -26,22 +26,19 @@ namespace OC\Log; use OCP\ILogger; +use OCP\IConfig; -class Syslog { - static protected $levels = array( +class Syslog implements IWritable { + static protected $levels = [ ILogger::DEBUG => LOG_DEBUG, ILogger::INFO => LOG_INFO, ILogger::WARN => LOG_WARNING, ILogger::ERROR => LOG_ERR, ILogger::FATAL => LOG_CRIT, - ); + ]; - /** - * Init class data - */ - public static function init() { - openlog(\OC::$server->getSystemConfig()->getValue("syslog_tag", "ownCloud"), LOG_PID | LOG_CONS, LOG_USER); - // Close at shutdown + public function __construct(IConfig $config) { + openlog($config->getSystemValue('syslog_tag', 'ownCloud'), LOG_PID | LOG_CONS, LOG_USER); register_shutdown_function('closelog'); } @@ -51,7 +48,7 @@ public static function init() { * @param string $message * @param int $level */ - public static function write($app, $message, $level) { + public function write($app, $message, $level) { $syslog_level = self::$levels[$level]; syslog($syslog_level, '{'.$app.'} '.$message); } diff --git a/lib/private/Server.php b/lib/private/Server.php index 3786486c2b240..a4608bf7a8f0b 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -87,6 +87,7 @@ use OC\Lock\MemcacheLockingProvider; use OC\Lock\NoopLockingProvider; use OC\Lockdown\LockdownManager; +use OC\Log\LogFactory; use OC\Mail\Mailer; use OC\Memcache\ArrayCache; use OC\Memcache\Factory; @@ -546,8 +547,8 @@ public function __construct($webRoot, \OC\Config $config) { $this->registerService(\OCP\ILogger::class, function (Server $c) { $logType = $c->query('AllConfig')->getSystemValue('log_type', 'file'); - $logger = Log::getLogClass($logType); - call_user_func(array($logger, 'init')); + $factory = new LogFactory($c); + $logger = $factory->get($logType); $config = $this->getSystemConfig(); $registry = $c->query(\OCP\Support\CrashReport\IRegistry::class); diff --git a/settings/Controller/LogSettingsController.php b/settings/Controller/LogSettingsController.php index 6405ff9ec733b..3b2479b602ac8 100644 --- a/settings/Controller/LogSettingsController.php +++ b/settings/Controller/LogSettingsController.php @@ -26,8 +26,11 @@ namespace OC\Settings\Controller; +use OC\Log; use OCP\AppFramework\Controller; use OCP\AppFramework\Http\StreamResponse; +use OCP\ILogger; +use OCP\IRequest; /** * Class LogSettingsController @@ -35,6 +38,15 @@ * @package OC\Settings\Controller */ class LogSettingsController extends Controller { + + /** @var Log */ + private $log; + + public function __construct(string $appName, IRequest $request, ILogger $logger) { + parent::__construct($appName, $request); + $this->log = $logger; + } + /** * download logfile * @@ -43,7 +55,10 @@ class LogSettingsController extends Controller { * @return StreamResponse */ public function download() { - $resp = new StreamResponse(\OC\Log\File::getLogFilePath()); + if(!$this->log instanceof Log) { + throw new \UnexpectedValueException('Log file not available'); + } + $resp = new StreamResponse($this->log->getLogPath()); $resp->addHeader('Content-Type', 'application/octet-stream'); $resp->addHeader('Content-Disposition', 'attachment; filename="nextcloud.log"'); return $resp; diff --git a/tests/lib/Log/FileTest.php b/tests/lib/Log/FileTest.php index 53b5c62e81af0..33f0ee8dd1981 100644 --- a/tests/lib/Log/FileTest.php +++ b/tests/lib/Log/FileTest.php @@ -31,6 +31,9 @@ class FileTest extends TestCase private $restore_logfile; private $restore_logdateformat; + /** @var File */ + protected $logFile; + protected function setUp() { parent::setUp(); $config = \OC::$server->getConfig(); @@ -38,7 +41,7 @@ protected function setUp() { $this->restore_logdateformat = $config->getSystemValue('logdateformat'); $config->setSystemValue("logfile", $config->getSystemValue('datadirectory') . "/logtest"); - File::init(); + $this->logFile = new File($config->getSystemValue('datadirectory') . '/logtest'); } protected function tearDown() { $config = \OC::$server->getConfig(); @@ -51,8 +54,8 @@ protected function tearDown() { $config->getSystemValue("logdateformat", $this->restore_logdateformat); } else { $config->deleteSystemValue("logdateformat"); - } - File::init(); + } + $this->logFile = new File($this->restore_logfile); parent::tearDown(); } @@ -63,7 +66,7 @@ public function testMicrosecondsLogTimestamp() { # set format & write log line $config->setSystemValue('logdateformat', 'u'); - File::write('test', 'message', ILogger::ERROR); + $this->logFile->write('test', 'message', ILogger::ERROR); # read log line $handle = @fopen($config->getSystemValue('logfile'), 'r'); diff --git a/tests/lib/Log/LogFactoryTest.php b/tests/lib/Log/LogFactoryTest.php new file mode 100644 index 0000000000000..8c1b7f08c74a7 --- /dev/null +++ b/tests/lib/Log/LogFactoryTest.php @@ -0,0 +1,149 @@ + + * + * @author Arthur Schiwon + * + * @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 . + * + */ + +namespace Test\Log; +use OC\Log\Errorlog; +use OC\Log\File; +use OC\Log\LogFactory; +use OC\Log\Syslog; +use OCP\IConfig; +use OCP\IServerContainer; +use Test\TestCase; + +/** + * Class LogFactoryTest + * + * @package Test\Log + */ +class LogFactoryTest extends TestCase { + /** @var IServerContainer|\PHPUnit_Framework_MockObject_MockObject */ + protected $c; + + /** @var LogFactory */ + protected $factory; + + protected function setUp() { + parent::setUp(); + + $this->c = $this->createMock(IServerContainer::class); + + $this->factory = new LogFactory($this->c); + } + + public function fileTypeProvider(): array { + return [ + [ + 'file' + ], + [ + 'nextcloud' + ], + [ + 'owncloud' + ], + [ + 'krzxkyr_default' + ] + ]; + } + + /** + * @param string $type + * @dataProvider fileTypeProvider + * @throws \OCP\AppFramework\QueryException + */ + public function testFile(string $type) { + $datadir = \OC::$SERVERROOT.'/data'; + $defaultLog = $datadir . '/nextcloud.log'; + + $config = $this->createMock(IConfig::class); + $config->expects($this->exactly(2)) + ->method('getSystemValue') + ->withConsecutive(['datadirectory', $datadir], ['logfile', $defaultLog]) + ->willReturnOnConsecutiveCalls($datadir, $defaultLog); + + $this->c->expects($this->any()) + ->method('getConfig') + ->willReturn($config); + + $log = $this->factory->get($type); + $this->assertInstanceOf(File::class, $log); + } + + public function logFilePathProvider():array { + return [ + [ + '/dev/null', + '/dev/null' + ], + [ + '/xdev/youshallfallback', + \OC::$SERVERROOT.'/data/nextcloud.log' + ] + ]; + } + + /** + * @dataProvider logFilePathProvider + * @throws \OCP\AppFramework\QueryException + */ + public function testFileCustomPath($path, $expected) { + $datadir = \OC::$SERVERROOT.'/data'; + $defaultLog = $datadir . '/nextcloud.log'; + + $config = $this->createMock(IConfig::class); + $config->expects($this->exactly(2)) + ->method('getSystemValue') + ->withConsecutive(['datadirectory', $datadir], ['logfile', $defaultLog]) + ->willReturnOnConsecutiveCalls($datadir, $path); + + $this->c->expects($this->any()) + ->method('getConfig') + ->willReturn($config); + + $log = $this->factory->get('file'); + $this->assertInstanceOf(File::class, $log); + $this->assertSame($expected, $log->getLogFilePath()); + } + + /** + * @throws \OCP\AppFramework\QueryException + */ + public function testErrorLog() { + $log = $this->factory->get('errorlog'); + $this->assertInstanceOf(Errorlog::class, $log); + } + + /** + * @throws \OCP\AppFramework\QueryException + */ + public function testSystemLog() { + $this->c->expects($this->once()) + ->method('resolve') + ->with(Syslog::class) + ->willReturn($this->createMock(Syslog::class)); + + $log = $this->factory->get('syslog'); + $this->assertInstanceOf(Syslog::class, $log); + } +} diff --git a/tests/lib/LoggerTest.php b/tests/lib/LoggerTest.php index 6b9c9af86390a..3d0847d6c8a17 100644 --- a/tests/lib/LoggerTest.php +++ b/tests/lib/LoggerTest.php @@ -11,7 +11,7 @@ use OC\Log; use OCP\ILogger; -class LoggerTest extends TestCase { +class LoggerTest extends TestCase implements Log\IWritable { /** @var \OC\SystemConfig|\PHPUnit_Framework_MockObject_MockObject */ private $config; @@ -23,15 +23,15 @@ class LoggerTest extends TestCase { private $logger; /** @var array */ - static private $logs = array(); + private $logs = []; protected function setUp() { parent::setUp(); - self::$logs = array(); + $this->logs = []; $this->config = $this->createMock(\OC\SystemConfig::class); $this->registry = $this->createMock(\OCP\Support\CrashReport\IRegistry::class); - $this->logger = new Log('Test\LoggerTest', $this->config, null, $this->registry); + $this->logger = new Log($this, $this->config, null, $this->registry); } public function testInterpolation() { @@ -63,11 +63,11 @@ public function testAppCondition() { } private function getLogs() { - return self::$logs; + return $this->logs; } - public static function write($app, $message, $level) { - self::$logs[]= "$level $message"; + public function write($app, $message, $level) { + $this->logs[]= "$level $message"; } public function userAndPasswordData() { @@ -202,23 +202,4 @@ public function testDetectclosure($user, $password) { $this->assertContains('*** sensitive parameters replaced ***', $logLine); } } - - public function dataGetLogClass() { - return [ - ['file', \OC\Log\File::class], - ['errorlog', \OC\Log\Errorlog::class], - ['syslog', \OC\Log\Syslog::class], - - ['owncloud', \OC\Log\File::class], - ['nextcloud', \OC\Log\File::class], - ['foobar', \OC\Log\File::class], - ]; - } - - /** - * @dataProvider dataGetLogClass - */ - public function testGetLogClass($type, $class) { - $this->assertEquals($class, Log::getLogClass($type)); - } } From cfc3ab0119d23a4c8ccccefb13d2271b0c0675ae Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 25 Apr 2018 02:27:43 +0200 Subject: [PATCH 2/9] offer API to create own File log. admin_audit makes use of it Signed-off-by: Arthur Schiwon --- apps/admin_audit/lib/AppInfo/Application.php | 96 +++++++++++-------- .../Sabre/ExceptionLoggerPluginTest.php | 2 +- lib/composer/composer/autoload_classmap.php | 3 +- lib/composer/composer/autoload_static.php | 3 +- lib/private/Log.php | 19 ++-- lib/private/Log/Errorlog.php | 6 +- lib/private/Log/File.php | 23 +++-- lib/private/Log/LogFactory.php | 23 +++-- lib/private/Log/Syslog.php | 5 +- lib/private/Server.php | 16 +++- lib/public/IServerContainer.php | 9 ++ lib/public/Log/ILogFactory.php | 48 ++++++++++ .../IWritable.php => public/Log/IWriter.php} | 15 ++- tests/lib/Log/FileTest.php | 4 +- tests/lib/LoggerTest.php | 9 +- 15 files changed, 198 insertions(+), 83 deletions(-) create mode 100644 lib/public/Log/ILogFactory.php rename lib/{private/Log/IWritable.php => public/Log/IWriter.php} (81%) diff --git a/apps/admin_audit/lib/AppInfo/Application.php b/apps/admin_audit/lib/AppInfo/Application.php index df39e3eb11178..f0387e8605799 100644 --- a/apps/admin_audit/lib/AppInfo/Application.php +++ b/apps/admin_audit/lib/AppInfo/Application.php @@ -53,8 +53,24 @@ class Application extends App { + /** @var ILogger */ + protected $logger; + public function __construct() { parent::__construct('admin_audit'); + $this->initLogger(); + } + + public function initLogger() { + $c = $this->getContainer()->getServer(); + + $logFile = $c->getConfig()->getAppValue('admin_audit', 'logfile', null); + if($logFile === null) { + $this->logger = $c->getLogger(); + return; + } + $this->logger = $c->getLogFactory()->getCustomLogger($logFile); + } public function register() { @@ -65,26 +81,24 @@ public function register() { * Register hooks in order to log them */ protected function registerHooks() { - $logger = $this->getContainer()->getServer()->getLogger(); - - $this->userManagementHooks($logger); - $this->groupHooks($logger); - $this->authHooks($logger); + $this->userManagementHooks(); + $this->groupHooks(); + $this->authHooks(); - $this->consoleHooks($logger); - $this->appHooks($logger); + $this->consoleHooks(); + $this->appHooks(); - $this->sharingHooks($logger); + $this->sharingHooks(); - $this->fileHooks($logger); - $this->trashbinHooks($logger); - $this->versionsHooks($logger); + $this->fileHooks(); + $this->trashbinHooks(); + $this->versionsHooks(); - $this->securityHooks($logger); + $this->securityHooks(); } - protected function userManagementHooks(ILogger $logger) { - $userActions = new UserManagement($logger); + protected function userManagementHooks() { + $userActions = new UserManagement($this->logger); Util::connectHook('OC_User', 'post_createUser', $userActions, 'create'); Util::connectHook('OC_User', 'post_deleteUser', $userActions, 'delete'); @@ -97,8 +111,8 @@ protected function userManagementHooks(ILogger $logger) { $userSession->listen('\OC\User', 'postUnassignedUserId', [$userActions, 'unassign']); } - protected function groupHooks(ILogger $logger) { - $groupActions = new GroupManagement($logger); + protected function groupHooks() { + $groupActions = new GroupManagement($this->logger); /** @var IGroupManager|Manager $groupManager */ $groupManager = $this->getContainer()->getServer()->getGroupManager(); @@ -108,8 +122,8 @@ protected function groupHooks(ILogger $logger) { $groupManager->listen('\OC\Group', 'postCreate', [$groupActions, 'createGroup']); } - protected function sharingHooks(ILogger $logger) { - $shareActions = new Sharing($logger); + protected function sharingHooks() { + $shareActions = new Sharing($this->logger); Util::connectHook(Share::class, 'post_shared', $shareActions, 'shared'); Util::connectHook(Share::class, 'post_unshare', $shareActions, 'unshare'); @@ -119,42 +133,42 @@ protected function sharingHooks(ILogger $logger) { Util::connectHook(Share::class, 'share_link_access', $shareActions, 'shareAccessed'); } - protected function authHooks(ILogger $logger) { - $authActions = new Auth($logger); + protected function authHooks() { + $authActions = new Auth($this->logger); Util::connectHook('OC_User', 'pre_login', $authActions, 'loginAttempt'); Util::connectHook('OC_User', 'post_login', $authActions, 'loginSuccessful'); Util::connectHook('OC_User', 'logout', $authActions, 'logout'); } - protected function appHooks(ILogger $logger) { + protected function appHooks() { $eventDispatcher = $this->getContainer()->getServer()->getEventDispatcher(); - $eventDispatcher->addListener(ManagerEvent::EVENT_APP_ENABLE, function(ManagerEvent $event) use ($logger) { - $appActions = new AppManagement($logger); + $eventDispatcher->addListener(ManagerEvent::EVENT_APP_ENABLE, function(ManagerEvent $event) { + $appActions = new AppManagement($this->logger); $appActions->enableApp($event->getAppID()); }); - $eventDispatcher->addListener(ManagerEvent::EVENT_APP_ENABLE_FOR_GROUPS, function(ManagerEvent $event) use ($logger) { - $appActions = new AppManagement($logger); + $eventDispatcher->addListener(ManagerEvent::EVENT_APP_ENABLE_FOR_GROUPS, function(ManagerEvent $event) { + $appActions = new AppManagement($this->logger); $appActions->enableAppForGroups($event->getAppID(), $event->getGroups()); }); - $eventDispatcher->addListener(ManagerEvent::EVENT_APP_DISABLE, function(ManagerEvent $event) use ($logger) { - $appActions = new AppManagement($logger); + $eventDispatcher->addListener(ManagerEvent::EVENT_APP_DISABLE, function(ManagerEvent $event) { + $appActions = new AppManagement($this->logger); $appActions->disableApp($event->getAppID()); }); } - protected function consoleHooks(ILogger $logger) { + protected function consoleHooks() { $eventDispatcher = $this->getContainer()->getServer()->getEventDispatcher(); - $eventDispatcher->addListener(ConsoleEvent::EVENT_RUN, function(ConsoleEvent $event) use ($logger) { - $appActions = new Console($logger); + $eventDispatcher->addListener(ConsoleEvent::EVENT_RUN, function(ConsoleEvent $event) { + $appActions = new Console($this->logger); $appActions->runCommand($event->getArguments()); }); } - protected function fileHooks(ILogger $logger) { - $fileActions = new Files($logger); + protected function fileHooks() { + $fileActions = new Files($this->logger); $eventDispatcher = $this->getContainer()->getServer()->getEventDispatcher(); $eventDispatcher->addListener( IPreview::EVENT, @@ -215,26 +229,26 @@ function(GenericEvent $event) use ($fileActions) { ); } - protected function versionsHooks(ILogger $logger) { - $versionsActions = new Versions($logger); + protected function versionsHooks() { + $versionsActions = new Versions($this->logger); Util::connectHook('\OCP\Versions', 'rollback', $versionsActions, 'rollback'); Util::connectHook('\OCP\Versions', 'delete',$versionsActions, 'delete'); } - protected function trashbinHooks(ILogger $logger) { - $trashActions = new Trashbin($logger); + protected function trashbinHooks() { + $trashActions = new Trashbin($this->logger); Util::connectHook('\OCP\Trashbin', 'preDelete', $trashActions, 'delete'); Util::connectHook('\OCA\Files_Trashbin\Trashbin', 'post_restore', $trashActions, 'restore'); } - protected function securityHooks(ILogger $logger) { + protected function securityHooks() { $eventDispatcher = $this->getContainer()->getServer()->getEventDispatcher(); - $eventDispatcher->addListener(IProvider::EVENT_SUCCESS, function(GenericEvent $event) use ($logger) { - $security = new Security($logger); + $eventDispatcher->addListener(IProvider::EVENT_SUCCESS, function(GenericEvent $event) { + $security = new Security($this->logger); $security->twofactorSuccess($event->getSubject(), $event->getArguments()); }); - $eventDispatcher->addListener(IProvider::EVENT_FAILED, function(GenericEvent $event) use ($logger) { - $security = new Security($logger); + $eventDispatcher->addListener(IProvider::EVENT_FAILED, function(GenericEvent $event) { + $security = new Security($this->logger); $security->twofactorFailed($event->getSubject(), $event->getArguments()); }); } diff --git a/apps/dav/tests/unit/Connector/Sabre/ExceptionLoggerPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/ExceptionLoggerPluginTest.php index 2f5d4ac4b77b4..1de9333207f7d 100644 --- a/apps/dav/tests/unit/Connector/Sabre/ExceptionLoggerPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/ExceptionLoggerPluginTest.php @@ -69,7 +69,7 @@ private function init() { }); $this->server = new Server(); - $this->logger = new TestLogger(new Log\File(\OC::$SERVERROOT.'/data/nextcloud.log'), $config); + $this->logger = new TestLogger(new Log\File(\OC::$SERVERROOT.'/data/nextcloud.log', '', $config), $config); $this->plugin = new PluginToTest('unit-test', $this->logger); $this->plugin->initialize($this->server); } diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index f706fea2cbe59..2c079a2c2959a 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -239,6 +239,8 @@ 'OCP\\Lock\\ILockingProvider' => $baseDir . '/lib/public/Lock/ILockingProvider.php', 'OCP\\Lock\\LockedException' => $baseDir . '/lib/public/Lock/LockedException.php', 'OCP\\Lockdown\\ILockdownManager' => $baseDir . '/lib/public/Lockdown/ILockdownManager.php', + 'OCP\\Log\\ILogFactory' => $baseDir . '/lib/public/Log/ILogFactory.php', + 'OCP\\Log\\IWriter' => $baseDir . '/lib/public/Log/IWriter.php', 'OCP\\Mail\\IAttachment' => $baseDir . '/lib/public/Mail/IAttachment.php', 'OCP\\Mail\\IEMailTemplate' => $baseDir . '/lib/public/Mail/IEMailTemplate.php', 'OCP\\Mail\\IMailer' => $baseDir . '/lib/public/Mail/IMailer.php', @@ -751,7 +753,6 @@ 'OC\\Log\\ExceptionSerializer' => $baseDir . '/lib/private/Log/ExceptionSerializer.php', 'OC\\Log\\File' => $baseDir . '/lib/private/Log/File.php', 'OC\\Log\\IFileBased' => $baseDir . '/lib/private/Log/IFileBased.php', - 'OC\\Log\\IWritable' => $baseDir . '/lib/private/Log/IWritable.php', 'OC\\Log\\LogFactory' => $baseDir . '/lib/private/Log/LogFactory.php', 'OC\\Log\\Rotate' => $baseDir . '/lib/private/Log/Rotate.php', 'OC\\Log\\Syslog' => $baseDir . '/lib/private/Log/Syslog.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index b879dc5a214f9..f76c626a0693f 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -269,6 +269,8 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OCP\\Lock\\ILockingProvider' => __DIR__ . '/../../..' . '/lib/public/Lock/ILockingProvider.php', 'OCP\\Lock\\LockedException' => __DIR__ . '/../../..' . '/lib/public/Lock/LockedException.php', 'OCP\\Lockdown\\ILockdownManager' => __DIR__ . '/../../..' . '/lib/public/Lockdown/ILockdownManager.php', + 'OCP\\Log\\ILogFactory' => __DIR__ . '/../../..' . '/lib/public/Log/ILogFactory.php', + 'OCP\\Log\\IWriter' => __DIR__ . '/../../..' . '/lib/public/Log/IWriter.php', 'OCP\\Mail\\IAttachment' => __DIR__ . '/../../..' . '/lib/public/Mail/IAttachment.php', 'OCP\\Mail\\IEMailTemplate' => __DIR__ . '/../../..' . '/lib/public/Mail/IEMailTemplate.php', 'OCP\\Mail\\IMailer' => __DIR__ . '/../../..' . '/lib/public/Mail/IMailer.php', @@ -781,7 +783,6 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\Log\\ExceptionSerializer' => __DIR__ . '/../../..' . '/lib/private/Log/ExceptionSerializer.php', 'OC\\Log\\File' => __DIR__ . '/../../..' . '/lib/private/Log/File.php', 'OC\\Log\\IFileBased' => __DIR__ . '/../../..' . '/lib/private/Log/IFileBased.php', - 'OC\\Log\\IWritable' => __DIR__ . '/../../..' . '/lib/private/Log/IWritable.php', 'OC\\Log\\LogFactory' => __DIR__ . '/../../..' . '/lib/private/Log/LogFactory.php', 'OC\\Log\\Rotate' => __DIR__ . '/../../..' . '/lib/private/Log/Rotate.php', 'OC\\Log\\Syslog' => __DIR__ . '/../../..' . '/lib/private/Log/Syslog.php', diff --git a/lib/private/Log.php b/lib/private/Log.php index c6f676db5886e..313201460a264 100644 --- a/lib/private/Log.php +++ b/lib/private/Log.php @@ -39,7 +39,8 @@ use OC\Log\ExceptionSerializer; use OC\Log\IFileBased; -use OC\Log\IWritable; +use OCP\IConfig; +use OCP\Log\IWriter; use OCP\ILogger; use OCP\Support\CrashReport\IRegistry; use OCP\Util; @@ -55,10 +56,10 @@ */ class Log implements ILogger { - /** @var IWritable */ + /** @var IWriter */ private $logger; - /** @var SystemConfig */ + /** @var IConfig */ private $config; /** @var boolean|null cache the result of the log condition check for the request */ @@ -71,15 +72,15 @@ class Log implements ILogger { private $crashReporters; /** - * @param IWritable $logger The logger that should be used - * @param SystemConfig $config the system config object + * @param IWriter $logger The logger that should be used + * @param IConfig $config the system config object * @param Normalizer|null $normalizer * @param IRegistry|null $registry */ - public function __construct(IWritable $logger, SystemConfig $config = null, $normalizer = null, IRegistry $registry = null) { + public function __construct(IWriter $logger, IConfig $config = null, $normalizer = null, IRegistry $registry = null) { // FIXME: Add this for backwards compatibility, should be fixed at some point probably if ($config === null) { - $config = \OC::$server->getSystemConfig(); + $config = \OC::$server->getConfig(); } $this->config = $config; @@ -257,7 +258,7 @@ private function getLogLevel($context) { } if (isset($context['app'])) { - $logCondition = $this->config->getValue('log.condition', []); + $logCondition = $this->config->getSystemValue('log.condition', []); $app = $context['app']; /** @@ -271,7 +272,7 @@ private function getLogLevel($context) { } } - return min($this->config->getValue('loglevel', ILogger::WARN), ILogger::FATAL); + return min($this->config->getSystemValue('loglevel', ILogger::WARN), ILogger::FATAL); } /** diff --git a/lib/private/Log/Errorlog.php b/lib/private/Log/Errorlog.php index 1302a31fe5c99..9dc8b2cc49c24 100644 --- a/lib/private/Log/Errorlog.php +++ b/lib/private/Log/Errorlog.php @@ -25,7 +25,9 @@ namespace OC\Log; -class Errorlog implements IWritable { +use OCP\Log\IWriter; + +class Errorlog implements IWriter { /** * write a message in the log @@ -33,7 +35,7 @@ class Errorlog implements IWritable { * @param string $message * @param int $level */ - public function write($app, $message, $level) { + public function write(string $app, $message, int $level) { error_log('[owncloud]['.$app.']['.$level.'] '.$message); } } diff --git a/lib/private/Log/File.php b/lib/private/Log/File.php index 639e4de8ac7aa..6e95de229cde2 100644 --- a/lib/private/Log/File.php +++ b/lib/private/Log/File.php @@ -36,6 +36,8 @@ */ namespace OC\Log; +use OCP\IConfig; +use OCP\Log\IWriter; use OCP\ILogger; @@ -45,11 +47,13 @@ * Log is saved at data/nextcloud.log (on default) */ -class File implements IWritable, IFileBased { +class File implements IWriter, IFileBased { /** @var string */ protected $logFile; + /** @var IConfig */ + private $config; - public function __construct(string $path, string $fallbackPath = '') { + public function __construct(string $path, string $fallbackPath = '', IConfig $config) { $this->logFile = $path; if (!file_exists($this->logFile)) { if( @@ -62,6 +66,7 @@ public function __construct(string $path, string $fallbackPath = '') { $this->logFile = $fallbackPath; } } + $this->config = $config; } /** @@ -70,12 +75,10 @@ public function __construct(string $path, string $fallbackPath = '') { * @param string|array $message * @param int $level */ - public function write($app, $message, $level) { - $config = \OC::$server->getSystemConfig(); - + public function write(string $app, $message, int $level) { // default to ISO8601 - $format = $config->getValue('logdateformat', \DateTime::ATOM); - $logTimeZone = $config->getValue('logtimezone', 'UTC'); + $format = $this->config->getSystemValue('logdateformat', \DateTime::ATOM); + $logTimeZone = $this->config->getSystemValue('logtimezone', 'UTC'); try { $timezone = new \DateTimeZone($logTimeZone); } catch (\Exception $e) { @@ -95,7 +98,7 @@ public function write($app, $message, $level) { $time = $time->format($format); $url = ($request->getRequestUri() !== '') ? $request->getRequestUri() : '--'; $method = is_string($request->getMethod()) ? $request->getMethod() : '--'; - if($config->getValue('installed', false)) { + if($this->config->getSystemValue('installed', false)) { $user = \OC_User::getUser() ? \OC_User::getUser() : '--'; } else { $user = '--'; @@ -104,7 +107,7 @@ public function write($app, $message, $level) { if ($userAgent === '') { $userAgent = '--'; } - $version = $config->getValue('version', ''); + $version = $this->config->getSystemValue('version', ''); $entry = compact( 'reqId', 'level', @@ -153,7 +156,7 @@ public function write($app, $message, $level) { * @return array */ public function getEntries($limit=50, $offset=0) { - $minLevel = \OC::$server->getSystemConfig()->getValue("loglevel", ILogger::WARN); + $minLevel = $this->config->getSystemValue("loglevel", ILogger::WARN); $entries = array(); $handle = @fopen($this->logFile, 'rb'); if ($handle) { diff --git a/lib/private/Log/LogFactory.php b/lib/private/Log/LogFactory.php index 13049083feed4..c1a4d00ceaa03 100644 --- a/lib/private/Log/LogFactory.php +++ b/lib/private/Log/LogFactory.php @@ -23,9 +23,13 @@ namespace OC\Log; +use OC\Log; +use OCP\ILogger; use OCP\IServerContainer; +use OCP\Log\ILogFactory; +use OCP\Log\IWriter; -class LogFactory { +class LogFactory implements ILogFactory { /** @var IServerContainer */ private $c; @@ -38,7 +42,7 @@ public function __construct(IServerContainer $c) { * @return \OC\Log\Errorlog|File|\stdClass * @throws \OCP\AppFramework\QueryException */ - public function get($type) { + public function get(string $type):IWriter { switch (strtolower($type)) { case 'errorlog': return new Errorlog(); @@ -51,16 +55,23 @@ public function get($type) { case 'owncloud': case 'nextcloud': default: - return $this->buildLogFile(); + return $this->buildLogFile(); } } - protected function buildLogFile() { + public function getCustomLogger(string $path):ILogger { + $log = $this->buildLogFile($path); + return new Log($log, $this->c->getConfig()); + } + + protected function buildLogFile(string $logFile = ''):File { $config = $this->c->getConfig(); $defaultLogFile = $config->getSystemValue('datadirectory', \OC::$SERVERROOT.'/data').'/nextcloud.log'; - $logFile = $config->getSystemValue('logfile', $defaultLogFile); + if($logFile === '') { + $logFile = $config->getSystemValue('logfile', $defaultLogFile); + } $fallback = $defaultLogFile !== $logFile ? $defaultLogFile : ''; - return new File($logFile, $fallback); + return new File($logFile, $fallback, $config); } } diff --git a/lib/private/Log/Syslog.php b/lib/private/Log/Syslog.php index be8ecdebb2e77..7bd7467f5c477 100644 --- a/lib/private/Log/Syslog.php +++ b/lib/private/Log/Syslog.php @@ -27,8 +27,9 @@ use OCP\ILogger; use OCP\IConfig; +use OCP\Log\IWriter; -class Syslog implements IWritable { +class Syslog implements IWriter { static protected $levels = [ ILogger::DEBUG => LOG_DEBUG, ILogger::INFO => LOG_INFO, @@ -48,7 +49,7 @@ public function __construct(IConfig $config) { * @param string $message * @param int $level */ - public function write($app, $message, $level) { + public function write(string $app, $message, int $level) { $syslog_level = self::$levels[$level]; syslog($syslog_level, '{'.$app.'} '.$message); } diff --git a/lib/private/Server.php b/lib/private/Server.php index a4608bf7a8f0b..05772555a03c6 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -135,6 +135,7 @@ use OCP\Contacts\ContactsMenu\IActionFactory; use OCP\IUser; use OCP\Lock\ILockingProvider; +use OCP\Log\ILogFactory; use OCP\Remote\Api\IApiFactory; use OCP\Remote\IInstanceFactory; use OCP\RichObjectStrings\IValidator; @@ -549,13 +550,18 @@ public function __construct($webRoot, \OC\Config $config) { $logType = $c->query('AllConfig')->getSystemValue('log_type', 'file'); $factory = new LogFactory($c); $logger = $factory->get($logType); - $config = $this->getSystemConfig(); + $config = $this->getConfig(); $registry = $c->query(\OCP\Support\CrashReport\IRegistry::class); return new Log($logger, $config, null, $registry); }); $this->registerAlias('Logger', \OCP\ILogger::class); + $this->registerService(ILogFactory::class, function (Server $c) { + return new LogFactory($c); + }); + $this->registerAlias('LogFactory', ILogFactory::class); + $this->registerService(\OCP\BackgroundJob\IJobList::class, function (Server $c) { $config = $c->getConfig(); return new \OC\BackgroundJob\JobList( @@ -1529,6 +1535,14 @@ public function getLogger() { return $this->query('Logger'); } + /** + * @return ILogFactory + * @throws \OCP\AppFramework\QueryException + */ + public function getLogFactory() { + return $this->query('LogFactory'); + } + /** * Returns a router for generating and matching urls * diff --git a/lib/public/IServerContainer.php b/lib/public/IServerContainer.php index 19c9578ee2321..c38aaf9f2cb27 100644 --- a/lib/public/IServerContainer.php +++ b/lib/public/IServerContainer.php @@ -44,6 +44,7 @@ // use OCP namespace for all classes that are considered public. // This means that they should be used by apps instead of the internal ownCloud classes namespace OCP; +use OCP\Log\ILogFactory; use OCP\Security\IContentSecurityPolicyManager; use Symfony\Component\EventDispatcher\EventDispatcherInterface; @@ -314,6 +315,14 @@ public function getJobList(); */ public function getLogger(); + /** + * returns a log factory instance + * + * @return ILogFactory + * @since 14.0.0 + */ + public function getLogFactory(); + /** * Returns a router for generating and matching urls * diff --git a/lib/public/Log/ILogFactory.php b/lib/public/Log/ILogFactory.php new file mode 100644 index 0000000000000..d8e1ab4ee760c --- /dev/null +++ b/lib/public/Log/ILogFactory.php @@ -0,0 +1,48 @@ + + * + * @author Arthur Schiwon + * + * @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 . + * + */ + +namespace OCP\Log; + +use OCP\ILogger; + +/** + * Interface ILogFactory + * + * @package OCP\Log + * @since 14.0.0 + */ +interface ILogFactory { + /** + * @param string $type - one of: file, errorlog, syslog + * @return IWriter + * @since 14.0.0 + */ + public function get(string $type): IWriter; + + /** + * @param string $path + * @return ILogger + * @since 14.0.0 + */ + public function getCustomLogger(string $path): ILogger; +} diff --git a/lib/private/Log/IWritable.php b/lib/public/Log/IWriter.php similarity index 81% rename from lib/private/Log/IWritable.php rename to lib/public/Log/IWriter.php index ff9bb4e71a823..c9b906bf4a38a 100644 --- a/lib/private/Log/IWritable.php +++ b/lib/public/Log/IWriter.php @@ -21,8 +21,17 @@ * */ -namespace OC\Log; +namespace OCP\Log; -interface IWritable { - public function write($app, $message, $level); +/** + * Interface IWriter + * + * @package OCP\Log + * @since 14.0.0 + */ +interface IWriter { + /** + * @since 14.0.0 + */ + public function write(string $app, $message, int $level); } diff --git a/tests/lib/Log/FileTest.php b/tests/lib/Log/FileTest.php index 33f0ee8dd1981..0c50c6c08ba5d 100644 --- a/tests/lib/Log/FileTest.php +++ b/tests/lib/Log/FileTest.php @@ -41,7 +41,7 @@ protected function setUp() { $this->restore_logdateformat = $config->getSystemValue('logdateformat'); $config->setSystemValue("logfile", $config->getSystemValue('datadirectory') . "/logtest"); - $this->logFile = new File($config->getSystemValue('datadirectory') . '/logtest'); + $this->logFile = new File($config->getSystemValue('datadirectory') . '/logtest', '', $config); } protected function tearDown() { $config = \OC::$server->getConfig(); @@ -55,7 +55,7 @@ protected function tearDown() { } else { $config->deleteSystemValue("logdateformat"); } - $this->logFile = new File($this->restore_logfile); + $this->logFile = new File($this->restore_logfile, '', $config); parent::tearDown(); } diff --git a/tests/lib/LoggerTest.php b/tests/lib/LoggerTest.php index 3d0847d6c8a17..9f226a395b676 100644 --- a/tests/lib/LoggerTest.php +++ b/tests/lib/LoggerTest.php @@ -10,8 +10,9 @@ use OC\Log; use OCP\ILogger; +use OCP\Log\IWriter; -class LoggerTest extends TestCase implements Log\IWritable { +class LoggerTest extends TestCase implements IWriter { /** @var \OC\SystemConfig|\PHPUnit_Framework_MockObject_MockObject */ private $config; @@ -29,7 +30,7 @@ protected function setUp() { parent::setUp(); $this->logs = []; - $this->config = $this->createMock(\OC\SystemConfig::class); + $this->config = $this->createMock(\OCP\IConfig::class); $this->registry = $this->createMock(\OCP\Support\CrashReport\IRegistry::class); $this->logger = new Log($this, $this->config, null, $this->registry); } @@ -44,7 +45,7 @@ public function testInterpolation() { public function testAppCondition() { $this->config->expects($this->any()) - ->method('getValue') + ->method('getSystemValue') ->will(($this->returnValueMap([ ['loglevel', ILogger::WARN, ILogger::WARN], ['log.condition', [], ['apps' => ['files']]] @@ -66,7 +67,7 @@ private function getLogs() { return $this->logs; } - public function write($app, $message, $level) { + public function write(string $app, $message, int $level) { $this->logs[]= "$level $message"; } From 69592408c4ef2b891c2d73f704611d2eabe5813a Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 25 Apr 2018 12:26:03 +0200 Subject: [PATCH 3/9] move IFileBased to public namespace, logreader needs it Signed-off-by: Arthur Schiwon --- lib/composer/composer/autoload_classmap.php | 2 +- lib/composer/composer/autoload_static.php | 2 +- lib/private/Log.php | 2 +- lib/private/Log/File.php | 1 + lib/{private => public}/Log/IFileBased.php | 2 +- 5 files changed, 5 insertions(+), 4 deletions(-) rename lib/{private => public}/Log/IFileBased.php (98%) diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 2c079a2c2959a..8bb8497eaa80f 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -239,6 +239,7 @@ 'OCP\\Lock\\ILockingProvider' => $baseDir . '/lib/public/Lock/ILockingProvider.php', 'OCP\\Lock\\LockedException' => $baseDir . '/lib/public/Lock/LockedException.php', 'OCP\\Lockdown\\ILockdownManager' => $baseDir . '/lib/public/Lockdown/ILockdownManager.php', + 'OCP\\Log\\IFileBased' => $baseDir . '/lib/public/Log/IFileBased.php', 'OCP\\Log\\ILogFactory' => $baseDir . '/lib/public/Log/ILogFactory.php', 'OCP\\Log\\IWriter' => $baseDir . '/lib/public/Log/IWriter.php', 'OCP\\Mail\\IAttachment' => $baseDir . '/lib/public/Mail/IAttachment.php', @@ -752,7 +753,6 @@ 'OC\\Log\\Errorlog' => $baseDir . '/lib/private/Log/Errorlog.php', 'OC\\Log\\ExceptionSerializer' => $baseDir . '/lib/private/Log/ExceptionSerializer.php', 'OC\\Log\\File' => $baseDir . '/lib/private/Log/File.php', - 'OC\\Log\\IFileBased' => $baseDir . '/lib/private/Log/IFileBased.php', 'OC\\Log\\LogFactory' => $baseDir . '/lib/private/Log/LogFactory.php', 'OC\\Log\\Rotate' => $baseDir . '/lib/private/Log/Rotate.php', 'OC\\Log\\Syslog' => $baseDir . '/lib/private/Log/Syslog.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index f76c626a0693f..7faa6df822896 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -269,6 +269,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OCP\\Lock\\ILockingProvider' => __DIR__ . '/../../..' . '/lib/public/Lock/ILockingProvider.php', 'OCP\\Lock\\LockedException' => __DIR__ . '/../../..' . '/lib/public/Lock/LockedException.php', 'OCP\\Lockdown\\ILockdownManager' => __DIR__ . '/../../..' . '/lib/public/Lockdown/ILockdownManager.php', + 'OCP\\Log\\IFileBased' => __DIR__ . '/../../..' . '/lib/public/Log/IFileBased.php', 'OCP\\Log\\ILogFactory' => __DIR__ . '/../../..' . '/lib/public/Log/ILogFactory.php', 'OCP\\Log\\IWriter' => __DIR__ . '/../../..' . '/lib/public/Log/IWriter.php', 'OCP\\Mail\\IAttachment' => __DIR__ . '/../../..' . '/lib/public/Mail/IAttachment.php', @@ -782,7 +783,6 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\Log\\Errorlog' => __DIR__ . '/../../..' . '/lib/private/Log/Errorlog.php', 'OC\\Log\\ExceptionSerializer' => __DIR__ . '/../../..' . '/lib/private/Log/ExceptionSerializer.php', 'OC\\Log\\File' => __DIR__ . '/../../..' . '/lib/private/Log/File.php', - 'OC\\Log\\IFileBased' => __DIR__ . '/../../..' . '/lib/private/Log/IFileBased.php', 'OC\\Log\\LogFactory' => __DIR__ . '/../../..' . '/lib/private/Log/LogFactory.php', 'OC\\Log\\Rotate' => __DIR__ . '/../../..' . '/lib/private/Log/Rotate.php', 'OC\\Log\\Syslog' => __DIR__ . '/../../..' . '/lib/private/Log/Syslog.php', diff --git a/lib/private/Log.php b/lib/private/Log.php index 313201460a264..099cc56776b85 100644 --- a/lib/private/Log.php +++ b/lib/private/Log.php @@ -38,7 +38,7 @@ use InterfaSys\LogNormalizer\Normalizer; use OC\Log\ExceptionSerializer; -use OC\Log\IFileBased; +use OCP\Log\IFileBased; use OCP\IConfig; use OCP\Log\IWriter; use OCP\ILogger; diff --git a/lib/private/Log/File.php b/lib/private/Log/File.php index 6e95de229cde2..56121d21d9ffc 100644 --- a/lib/private/Log/File.php +++ b/lib/private/Log/File.php @@ -37,6 +37,7 @@ namespace OC\Log; use OCP\IConfig; +use OCP\Log\IFileBased; use OCP\Log\IWriter; use OCP\ILogger; diff --git a/lib/private/Log/IFileBased.php b/lib/public/Log/IFileBased.php similarity index 98% rename from lib/private/Log/IFileBased.php rename to lib/public/Log/IFileBased.php index ae083f670e281..e4befbfd63223 100644 --- a/lib/private/Log/IFileBased.php +++ b/lib/public/Log/IFileBased.php @@ -21,7 +21,7 @@ * */ -namespace OC\Log; +namespace OCP\Log; interface IFileBased { public function getLogFilePath(); From 0e6a3175162a1312c1035c4a2630a26f8c26d561 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 25 Apr 2018 14:57:08 +0200 Subject: [PATCH 4/9] revert Log's dependency to SystemConfig to work during Installation Signed-off-by: Arthur Schiwon --- lib/private/Log.php | 13 ++++++------- lib/private/Log/LogFactory.php | 9 ++++++++- lib/private/Server.php | 3 +-- tests/lib/LoggerTest.php | 4 ++-- 4 files changed, 17 insertions(+), 12 deletions(-) diff --git a/lib/private/Log.php b/lib/private/Log.php index 099cc56776b85..69705c49e87ba 100644 --- a/lib/private/Log.php +++ b/lib/private/Log.php @@ -39,7 +39,6 @@ use OC\Log\ExceptionSerializer; use OCP\Log\IFileBased; -use OCP\IConfig; use OCP\Log\IWriter; use OCP\ILogger; use OCP\Support\CrashReport\IRegistry; @@ -59,7 +58,7 @@ class Log implements ILogger { /** @var IWriter */ private $logger; - /** @var IConfig */ + /** @var SystemConfig */ private $config; /** @var boolean|null cache the result of the log condition check for the request */ @@ -73,14 +72,14 @@ class Log implements ILogger { /** * @param IWriter $logger The logger that should be used - * @param IConfig $config the system config object + * @param SystemConfig $config the system config object * @param Normalizer|null $normalizer * @param IRegistry|null $registry */ - public function __construct(IWriter $logger, IConfig $config = null, $normalizer = null, IRegistry $registry = null) { + public function __construct(IWriter $logger, SystemConfig $config = null, $normalizer = null, IRegistry $registry = null) { // FIXME: Add this for backwards compatibility, should be fixed at some point probably if ($config === null) { - $config = \OC::$server->getConfig(); + $config = \OC::$server->getSystemConfig(); } $this->config = $config; @@ -258,7 +257,7 @@ private function getLogLevel($context) { } if (isset($context['app'])) { - $logCondition = $this->config->getSystemValue('log.condition', []); + $logCondition = $this->config->getValue('log.condition', []); $app = $context['app']; /** @@ -272,7 +271,7 @@ private function getLogLevel($context) { } } - return min($this->config->getSystemValue('loglevel', ILogger::WARN), ILogger::FATAL); + return min($this->config->getValue('loglevel', ILogger::WARN), ILogger::FATAL); } /** diff --git a/lib/private/Log/LogFactory.php b/lib/private/Log/LogFactory.php index c1a4d00ceaa03..63f08a3232036 100644 --- a/lib/private/Log/LogFactory.php +++ b/lib/private/Log/LogFactory.php @@ -23,6 +23,7 @@ namespace OC\Log; +use OC\AllConfig; use OC\Log; use OCP\ILogger; use OCP\IServerContainer; @@ -60,8 +61,14 @@ public function get(string $type):IWriter { } public function getCustomLogger(string $path):ILogger { + $systemConfig = null; + $iconfig = $this->c->getConfig(); + if($iconfig instanceof AllConfig) { + // Log is bound to SystemConfig, but fetches it from \OC::$server if not supplied + $systemConfig = $iconfig->getSystemConfig(); + } $log = $this->buildLogFile($path); - return new Log($log, $this->c->getConfig()); + return new Log($log, $systemConfig); } protected function buildLogFile(string $logFile = ''):File { diff --git a/lib/private/Server.php b/lib/private/Server.php index 05772555a03c6..c744ba37ded95 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -550,10 +550,9 @@ public function __construct($webRoot, \OC\Config $config) { $logType = $c->query('AllConfig')->getSystemValue('log_type', 'file'); $factory = new LogFactory($c); $logger = $factory->get($logType); - $config = $this->getConfig(); $registry = $c->query(\OCP\Support\CrashReport\IRegistry::class); - return new Log($logger, $config, null, $registry); + return new Log($logger, $this->getSystemConfig(), null, $registry); }); $this->registerAlias('Logger', \OCP\ILogger::class); diff --git a/tests/lib/LoggerTest.php b/tests/lib/LoggerTest.php index 9f226a395b676..83cb87b873318 100644 --- a/tests/lib/LoggerTest.php +++ b/tests/lib/LoggerTest.php @@ -30,7 +30,7 @@ protected function setUp() { parent::setUp(); $this->logs = []; - $this->config = $this->createMock(\OCP\IConfig::class); + $this->config = $this->createMock(\OC\SystemConfig::class); $this->registry = $this->createMock(\OCP\Support\CrashReport\IRegistry::class); $this->logger = new Log($this, $this->config, null, $this->registry); } @@ -45,7 +45,7 @@ public function testInterpolation() { public function testAppCondition() { $this->config->expects($this->any()) - ->method('getSystemValue') + ->method('getValue') ->will(($this->returnValueMap([ ['loglevel', ILogger::WARN, ILogger::WARN], ['log.condition', [], ['apps' => ['files']]] From a21a5bc4ec77c3098985a6561d0f7a00bedb9250 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 25 Apr 2018 15:33:57 +0200 Subject: [PATCH 5/9] improve Syslog a little Signed-off-by: Arthur Schiwon --- lib/private/Log/Syslog.php | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/private/Log/Syslog.php b/lib/private/Log/Syslog.php index 7bd7467f5c477..90a20026f0ec0 100644 --- a/lib/private/Log/Syslog.php +++ b/lib/private/Log/Syslog.php @@ -30,7 +30,7 @@ use OCP\Log\IWriter; class Syslog implements IWriter { - static protected $levels = [ + protected $levels = [ ILogger::DEBUG => LOG_DEBUG, ILogger::INFO => LOG_INFO, ILogger::WARN => LOG_WARNING, @@ -40,7 +40,10 @@ class Syslog implements IWriter { public function __construct(IConfig $config) { openlog($config->getSystemValue('syslog_tag', 'ownCloud'), LOG_PID | LOG_CONS, LOG_USER); - register_shutdown_function('closelog'); + } + + public function __destruct() { + closelog(); } /** @@ -50,7 +53,7 @@ public function __construct(IConfig $config) { * @param int $level */ public function write(string $app, $message, int $level) { - $syslog_level = self::$levels[$level]; + $syslog_level = $this->levels[$level]; syslog($syslog_level, '{'.$app.'} '.$message); } } From 57517882827a3b917da5e99ab2b2b2f81565a2f6 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 25 Apr 2018 16:01:17 +0200 Subject: [PATCH 6/9] add missing php doc and type hints Signed-off-by: Arthur Schiwon --- lib/private/Log/File.php | 4 ++-- lib/public/Log/IFileBased.php | 17 +++++++++++++++-- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/lib/private/Log/File.php b/lib/private/Log/File.php index 56121d21d9ffc..86f57f78fdae0 100644 --- a/lib/private/Log/File.php +++ b/lib/private/Log/File.php @@ -156,7 +156,7 @@ public function write(string $app, $message, int $level) { * @param int $offset * @return array */ - public function getEntries($limit=50, $offset=0) { + public function getEntries(int $limit=50, int $offset=0):array { $minLevel = $this->config->getSystemValue("loglevel", ILogger::WARN); $entries = array(); $handle = @fopen($this->logFile, 'rb'); @@ -201,7 +201,7 @@ public function getEntries($limit=50, $offset=0) { /** * @return string */ - public function getLogFilePath() { + public function getLogFilePath():string { return $this->logFile; } } diff --git a/lib/public/Log/IFileBased.php b/lib/public/Log/IFileBased.php index e4befbfd63223..c0eef47297578 100644 --- a/lib/public/Log/IFileBased.php +++ b/lib/public/Log/IFileBased.php @@ -23,8 +23,21 @@ namespace OCP\Log; +/** + * Interface IFileBased + * + * @package OCP\Log + * + * @since 14.0.0 + */ interface IFileBased { - public function getLogFilePath(); + /** + * @since 14.0.0 + */ + public function getLogFilePath():string; - public function getEntries($limit=50, $offset=0); + /** + * @since 14.0.0 + */ + public function getEntries(int $limit=50, int $offset=0): array; } From ab7a4b869397a5bc267a9f10b42654b35bbe1af6 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 25 Apr 2018 16:25:14 +0200 Subject: [PATCH 7/9] fix dav test Signed-off-by: Arthur Schiwon --- .../tests/unit/Connector/Sabre/ExceptionLoggerPluginTest.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apps/dav/tests/unit/Connector/Sabre/ExceptionLoggerPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/ExceptionLoggerPluginTest.php index 1de9333207f7d..7bab41ec4aea4 100644 --- a/apps/dav/tests/unit/Connector/Sabre/ExceptionLoggerPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/ExceptionLoggerPluginTest.php @@ -28,6 +28,7 @@ use OCA\DAV\Connector\Sabre\Exception\InvalidPath; use OCA\DAV\Connector\Sabre\ExceptionLoggerPlugin as PluginToTest; use OC\Log; +use OCP\IConfig; use PHPUnit_Framework_MockObject_MockObject; use Sabre\DAV\Exception\NotFound; use Sabre\DAV\Exception\ServiceUnavailable; @@ -69,7 +70,7 @@ private function init() { }); $this->server = new Server(); - $this->logger = new TestLogger(new Log\File(\OC::$SERVERROOT.'/data/nextcloud.log', '', $config), $config); + $this->logger = new TestLogger(new Log\File(\OC::$SERVERROOT.'/data/nextcloud.log', '', $this->createMock(IConfig::class)), $config); $this->plugin = new PluginToTest('unit-test', $this->logger); $this->plugin->initialize($this->server); } From b841a477c6b96a13ea061df3fc622c3067514bf2 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 25 Apr 2018 17:42:14 +0200 Subject: [PATCH 8/9] log to $datadir/audit.log by default and add rotation Signed-off-by: Arthur Schiwon --- apps/admin_audit/appinfo/info.xml | 3 + .../composer/composer/autoload_classmap.php | 1 + .../composer/composer/autoload_static.php | 1 + apps/admin_audit/lib/AppInfo/Application.php | 4 +- .../admin_audit/lib/BackgroundJobs/Rotate.php | 52 ++++++++++++++ lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + lib/private/Log/Rotate.php | 26 +++---- lib/public/Log/RotationTrait.php | 71 +++++++++++++++++++ settings/Controller/LogSettingsController.php | 2 +- 10 files changed, 144 insertions(+), 18 deletions(-) create mode 100644 apps/admin_audit/lib/BackgroundJobs/Rotate.php create mode 100644 lib/public/Log/RotationTrait.php diff --git a/apps/admin_audit/appinfo/info.xml b/apps/admin_audit/appinfo/info.xml index 054ed9580f2fc..3006551b409bb 100644 --- a/apps/admin_audit/appinfo/info.xml +++ b/apps/admin_audit/appinfo/info.xml @@ -17,4 +17,7 @@ + + OCA\AdminAudit\BackgroundJobs\Rotate + diff --git a/apps/admin_audit/composer/composer/autoload_classmap.php b/apps/admin_audit/composer/composer/autoload_classmap.php index c08200c7c2031..95ddaac7452ea 100644 --- a/apps/admin_audit/composer/composer/autoload_classmap.php +++ b/apps/admin_audit/composer/composer/autoload_classmap.php @@ -18,4 +18,5 @@ 'OCA\\AdminAudit\\Actions\\UserManagement' => $baseDir . '/../lib/Actions/UserManagement.php', 'OCA\\AdminAudit\\Actions\\Versions' => $baseDir . '/../lib/Actions/Versions.php', 'OCA\\AdminAudit\\AppInfo\\Application' => $baseDir . '/../lib/AppInfo/Application.php', + 'OCA\\AdminAudit\\BackgroundJobs\\Rotate' => $baseDir . '/../lib/BackgroundJobs/Rotate.php', ); diff --git a/apps/admin_audit/composer/composer/autoload_static.php b/apps/admin_audit/composer/composer/autoload_static.php index ef088bd22d944..1c01a35ceb2e1 100644 --- a/apps/admin_audit/composer/composer/autoload_static.php +++ b/apps/admin_audit/composer/composer/autoload_static.php @@ -33,6 +33,7 @@ class ComposerStaticInitAdminAudit 'OCA\\AdminAudit\\Actions\\UserManagement' => __DIR__ . '/..' . '/../lib/Actions/UserManagement.php', 'OCA\\AdminAudit\\Actions\\Versions' => __DIR__ . '/..' . '/../lib/Actions/Versions.php', 'OCA\\AdminAudit\\AppInfo\\Application' => __DIR__ . '/..' . '/../lib/AppInfo/Application.php', + 'OCA\\AdminAudit\\BackgroundJobs\\Rotate' => __DIR__ . '/..' . '/../lib/BackgroundJobs/Rotate.php', ); public static function getInitializer(ClassLoader $loader) diff --git a/apps/admin_audit/lib/AppInfo/Application.php b/apps/admin_audit/lib/AppInfo/Application.php index f0387e8605799..77b76885a20e7 100644 --- a/apps/admin_audit/lib/AppInfo/Application.php +++ b/apps/admin_audit/lib/AppInfo/Application.php @@ -63,8 +63,10 @@ public function __construct() { public function initLogger() { $c = $this->getContainer()->getServer(); + $config = $c->getConfig(); - $logFile = $c->getConfig()->getAppValue('admin_audit', 'logfile', null); + $default = $config->getSystemValue('datadirectory', \OC::$SERVERROOT . '/data') . '/audit.log'; + $logFile = $config->getAppValue('admin_audit', 'logfile', $default); if($logFile === null) { $this->logger = $c->getLogger(); return; diff --git a/apps/admin_audit/lib/BackgroundJobs/Rotate.php b/apps/admin_audit/lib/BackgroundJobs/Rotate.php new file mode 100644 index 0000000000000..421ee65d643a5 --- /dev/null +++ b/apps/admin_audit/lib/BackgroundJobs/Rotate.php @@ -0,0 +1,52 @@ + + * + * @author Arthur Schiwon + * + * @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 . + * + */ + +namespace OCA\AdminAudit\BackgroundJobs; + +use OC\BackgroundJob\TimedJob; +use OCP\Log\RotationTrait; + +class Rotate extends TimedJob { + use RotationTrait; + + public function __construct() { + $this->setInterval(60*60*3); + } + + protected function run($argument) { + $config = \OC::$server->getConfig(); + $default = $config->getSystemValue('datadirectory', \OC::$SERVERROOT . '/data') . '/audit.log'; + $this->filePath = $config->getAppValue('admin_audit', 'logfile', $default); + + if($this->filePath === '') { + // default log file, nothing to do + return; + } + + $this->maxSize = $config->getSystemValue('log_rotate_size', 100 * 1024 * 1024); + + if($this->shouldRotateBySize()) { + $this->rotate(); + } + } +} diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 8bb8497eaa80f..6f8c4131546ae 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -242,6 +242,7 @@ 'OCP\\Log\\IFileBased' => $baseDir . '/lib/public/Log/IFileBased.php', 'OCP\\Log\\ILogFactory' => $baseDir . '/lib/public/Log/ILogFactory.php', 'OCP\\Log\\IWriter' => $baseDir . '/lib/public/Log/IWriter.php', + 'OCP\\Log\\RotationTrait' => $baseDir . '/lib/public/Log/RotationTrait.php', 'OCP\\Mail\\IAttachment' => $baseDir . '/lib/public/Mail/IAttachment.php', 'OCP\\Mail\\IEMailTemplate' => $baseDir . '/lib/public/Mail/IEMailTemplate.php', 'OCP\\Mail\\IMailer' => $baseDir . '/lib/public/Mail/IMailer.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 7faa6df822896..92726f7789d0a 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -272,6 +272,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OCP\\Log\\IFileBased' => __DIR__ . '/../../..' . '/lib/public/Log/IFileBased.php', 'OCP\\Log\\ILogFactory' => __DIR__ . '/../../..' . '/lib/public/Log/ILogFactory.php', 'OCP\\Log\\IWriter' => __DIR__ . '/../../..' . '/lib/public/Log/IWriter.php', + 'OCP\\Log\\RotationTrait' => __DIR__ . '/../../..' . '/lib/public/Log/RotationTrait.php', 'OCP\\Mail\\IAttachment' => __DIR__ . '/../../..' . '/lib/public/Mail/IAttachment.php', 'OCP\\Mail\\IEMailTemplate' => __DIR__ . '/../../..' . '/lib/public/Mail/IEMailTemplate.php', 'OCP\\Mail\\IMailer' => __DIR__ . '/../../..' . '/lib/public/Mail/IMailer.php', diff --git a/lib/private/Log/Rotate.php b/lib/private/Log/Rotate.php index 48b96c98a8d3e..cc41c804ad3f0 100644 --- a/lib/private/Log/Rotate.php +++ b/lib/private/Log/Rotate.php @@ -24,7 +24,7 @@ */ namespace OC\Log; -use OCP\ILogger; +use OCP\Log\RotationTrait; /** * This rotates the current logfile to a new name, this way the total log usage @@ -33,23 +33,17 @@ * location and manage that with your own tools. */ class Rotate extends \OC\BackgroundJob\Job { - private $max_log_size; + use RotationTrait; + public function run($dummy) { $systemConfig = \OC::$server->getSystemConfig(); - $logFile = $systemConfig->getValue('logfile', $systemConfig->getValue('datadirectory', \OC::$SERVERROOT . '/data') . '/nextcloud.log'); - $this->max_log_size = \OC::$server->getConfig()->getSystemValue('log_rotate_size', 100 * 1024 * 1024); - if ($this->max_log_size) { - $filesize = @filesize($logFile); - if ($filesize >= $this->max_log_size) { - $this->rotate($logFile); - } - } - } + $this->filePath = $systemConfig->getValue('logfile', $systemConfig->getValue('datadirectory', \OC::$SERVERROOT . '/data') . '/nextcloud.log'); - protected function rotate($logfile) { - $rotatedLogfile = $logfile.'.1'; - rename($logfile, $rotatedLogfile); - $msg = 'Log file "'.$logfile.'" was over '.$this->max_log_size.' bytes, moved to "'.$rotatedLogfile.'"'; - \OCP\Util::writeLog(Rotate::class, $msg, ILogger::WARN); + $this->maxSize = \OC::$server->getConfig()->getSystemValue('log_rotate_size', 100 * 1024 * 1024); + if($this->shouldRotateBySize()) { + $rotatedFile = $this->rotate(); + $msg = 'Log file "'.$this->filePath.'" was over '.$this->maxSize.' bytes, moved to "'.$rotatedFile.'"'; + \OC::$server->getLogger()->warning($msg, ['app' => Rotate::class]); + } } } diff --git a/lib/public/Log/RotationTrait.php b/lib/public/Log/RotationTrait.php new file mode 100644 index 0000000000000..df42bfeff1ff3 --- /dev/null +++ b/lib/public/Log/RotationTrait.php @@ -0,0 +1,71 @@ + + * + * @author Arthur Schiwon + * + * @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 . + * + */ + +namespace OCP\Log; + +/** + * Trait RotationTrait + * + * @package OCP\Log + * + * @since 14.0.0 + */ +trait RotationTrait { + + /** + * @var string + * @since 14.0.0 + */ + protected $filePath; + + /** + * @var int + * @since 14.0.0 + */ + protected $maxSize; + + /** + * @return string the resulting new filepath + * @since 14.0.0 + */ + protected function rotate():string { + $rotatedFile = $this->filePath.'.1'; + rename($this->filePath, $rotatedFile); + return $rotatedFile; + } + + /** + * @return bool + * @since 14.0.0 + */ + protected function shouldRotateBySize():bool { + if ((int)$this->maxSize > 0) { + $filesize = @filesize($this->filePath); + if ($filesize >= (int)$this->maxSize) { + return true; + } + } + return false; + } + +} diff --git a/settings/Controller/LogSettingsController.php b/settings/Controller/LogSettingsController.php index 3b2479b602ac8..ef195edce637c 100644 --- a/settings/Controller/LogSettingsController.php +++ b/settings/Controller/LogSettingsController.php @@ -39,7 +39,7 @@ */ class LogSettingsController extends Controller { - /** @var Log */ + /** @var ILogger */ private $log; public function __construct(string $appName, IRequest $request, ILogger $logger) { From aff5fe68b31c3663be2a114666650d2f8723a22b Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 26 Apr 2018 23:54:11 +0200 Subject: [PATCH 9/9] use SystemConfig, less dependencies, and not publicly needed Signed-off-by: Arthur Schiwon --- .../Sabre/ExceptionLoggerPluginTest.php | 3 +-- lib/private/Log/File.php | 17 ++++++------- lib/private/Log/LogFactory.php | 24 +++++++----------- lib/private/Server.php | 7 +++--- tests/lib/Log/FileTest.php | 20 +++++++-------- tests/lib/Log/LogFactoryTest.php | 25 ++++++++----------- 6 files changed, 41 insertions(+), 55 deletions(-) diff --git a/apps/dav/tests/unit/Connector/Sabre/ExceptionLoggerPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/ExceptionLoggerPluginTest.php index 7bab41ec4aea4..1de9333207f7d 100644 --- a/apps/dav/tests/unit/Connector/Sabre/ExceptionLoggerPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/ExceptionLoggerPluginTest.php @@ -28,7 +28,6 @@ use OCA\DAV\Connector\Sabre\Exception\InvalidPath; use OCA\DAV\Connector\Sabre\ExceptionLoggerPlugin as PluginToTest; use OC\Log; -use OCP\IConfig; use PHPUnit_Framework_MockObject_MockObject; use Sabre\DAV\Exception\NotFound; use Sabre\DAV\Exception\ServiceUnavailable; @@ -70,7 +69,7 @@ private function init() { }); $this->server = new Server(); - $this->logger = new TestLogger(new Log\File(\OC::$SERVERROOT.'/data/nextcloud.log', '', $this->createMock(IConfig::class)), $config); + $this->logger = new TestLogger(new Log\File(\OC::$SERVERROOT.'/data/nextcloud.log', '', $config), $config); $this->plugin = new PluginToTest('unit-test', $this->logger); $this->plugin->initialize($this->server); } diff --git a/lib/private/Log/File.php b/lib/private/Log/File.php index 86f57f78fdae0..597cb54e4025c 100644 --- a/lib/private/Log/File.php +++ b/lib/private/Log/File.php @@ -36,10 +36,9 @@ */ namespace OC\Log; -use OCP\IConfig; +use OC\SystemConfig; use OCP\Log\IFileBased; use OCP\Log\IWriter; - use OCP\ILogger; /** @@ -51,10 +50,10 @@ class File implements IWriter, IFileBased { /** @var string */ protected $logFile; - /** @var IConfig */ + /** @var SystemConfig */ private $config; - public function __construct(string $path, string $fallbackPath = '', IConfig $config) { + public function __construct(string $path, string $fallbackPath = '', SystemConfig $config) { $this->logFile = $path; if (!file_exists($this->logFile)) { if( @@ -78,8 +77,8 @@ public function __construct(string $path, string $fallbackPath = '', IConfig $co */ public function write(string $app, $message, int $level) { // default to ISO8601 - $format = $this->config->getSystemValue('logdateformat', \DateTime::ATOM); - $logTimeZone = $this->config->getSystemValue('logtimezone', 'UTC'); + $format = $this->config->getValue('logdateformat', \DateTime::ATOM); + $logTimeZone = $this->config->getValue('logtimezone', 'UTC'); try { $timezone = new \DateTimeZone($logTimeZone); } catch (\Exception $e) { @@ -99,7 +98,7 @@ public function write(string $app, $message, int $level) { $time = $time->format($format); $url = ($request->getRequestUri() !== '') ? $request->getRequestUri() : '--'; $method = is_string($request->getMethod()) ? $request->getMethod() : '--'; - if($this->config->getSystemValue('installed', false)) { + if($this->config->getValue('installed', false)) { $user = \OC_User::getUser() ? \OC_User::getUser() : '--'; } else { $user = '--'; @@ -108,7 +107,7 @@ public function write(string $app, $message, int $level) { if ($userAgent === '') { $userAgent = '--'; } - $version = $this->config->getSystemValue('version', ''); + $version = $this->config->getValue('version', ''); $entry = compact( 'reqId', 'level', @@ -157,7 +156,7 @@ public function write(string $app, $message, int $level) { * @return array */ public function getEntries(int $limit=50, int $offset=0):array { - $minLevel = $this->config->getSystemValue("loglevel", ILogger::WARN); + $minLevel = $this->config->getValue("loglevel", ILogger::WARN); $entries = array(); $handle = @fopen($this->logFile, 'rb'); if ($handle) { diff --git a/lib/private/Log/LogFactory.php b/lib/private/Log/LogFactory.php index 63f08a3232036..9b9d12abfa8c1 100644 --- a/lib/private/Log/LogFactory.php +++ b/lib/private/Log/LogFactory.php @@ -23,8 +23,8 @@ namespace OC\Log; -use OC\AllConfig; use OC\Log; +use OC\SystemConfig; use OCP\ILogger; use OCP\IServerContainer; use OCP\Log\ILogFactory; @@ -33,14 +33,15 @@ class LogFactory implements ILogFactory { /** @var IServerContainer */ private $c; + /** @var SystemConfig */ + private $systemConfig; - public function __construct(IServerContainer $c) { + public function __construct(IServerContainer $c, SystemConfig $systemConfig) { $this->c = $c; + $this->systemConfig = $systemConfig; } /** - * @param $type - * @return \OC\Log\Errorlog|File|\stdClass * @throws \OCP\AppFramework\QueryException */ public function get(string $type):IWriter { @@ -61,24 +62,17 @@ public function get(string $type):IWriter { } public function getCustomLogger(string $path):ILogger { - $systemConfig = null; - $iconfig = $this->c->getConfig(); - if($iconfig instanceof AllConfig) { - // Log is bound to SystemConfig, but fetches it from \OC::$server if not supplied - $systemConfig = $iconfig->getSystemConfig(); - } $log = $this->buildLogFile($path); - return new Log($log, $systemConfig); + return new Log($log, $this->systemConfig); } protected function buildLogFile(string $logFile = ''):File { - $config = $this->c->getConfig(); - $defaultLogFile = $config->getSystemValue('datadirectory', \OC::$SERVERROOT.'/data').'/nextcloud.log'; + $defaultLogFile = $this->systemConfig->getValue('datadirectory', \OC::$SERVERROOT.'/data').'/nextcloud.log'; if($logFile === '') { - $logFile = $config->getSystemValue('logfile', $defaultLogFile); + $logFile = $this->systemConfig->getValue('logfile', $defaultLogFile); } $fallback = $defaultLogFile !== $logFile ? $defaultLogFile : ''; - return new File($logFile, $fallback, $config); + return new File($logFile, $fallback, $this->systemConfig); } } diff --git a/lib/private/Server.php b/lib/private/Server.php index c744ba37ded95..a879c65bb9bf0 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -548,7 +548,7 @@ public function __construct($webRoot, \OC\Config $config) { $this->registerService(\OCP\ILogger::class, function (Server $c) { $logType = $c->query('AllConfig')->getSystemValue('log_type', 'file'); - $factory = new LogFactory($c); + $factory = new LogFactory($c, $this->getSystemConfig()); $logger = $factory->get($logType); $registry = $c->query(\OCP\Support\CrashReport\IRegistry::class); @@ -557,9 +557,8 @@ public function __construct($webRoot, \OC\Config $config) { $this->registerAlias('Logger', \OCP\ILogger::class); $this->registerService(ILogFactory::class, function (Server $c) { - return new LogFactory($c); + return new LogFactory($c, $this->getSystemConfig()); }); - $this->registerAlias('LogFactory', ILogFactory::class); $this->registerService(\OCP\BackgroundJob\IJobList::class, function (Server $c) { $config = $c->getConfig(); @@ -1539,7 +1538,7 @@ public function getLogger() { * @throws \OCP\AppFramework\QueryException */ public function getLogFactory() { - return $this->query('LogFactory'); + return $this->query(ILogFactory::class); } /** diff --git a/tests/lib/Log/FileTest.php b/tests/lib/Log/FileTest.php index 0c50c6c08ba5d..d5e550a7e8db4 100644 --- a/tests/lib/Log/FileTest.php +++ b/tests/lib/Log/FileTest.php @@ -36,24 +36,24 @@ class FileTest extends TestCase protected function setUp() { parent::setUp(); - $config = \OC::$server->getConfig(); - $this->restore_logfile = $config->getSystemValue("logfile"); - $this->restore_logdateformat = $config->getSystemValue('logdateformat'); + $config = \OC::$server->getSystemConfig(); + $this->restore_logfile = $config->getValue("logfile"); + $this->restore_logdateformat = $config->getValue('logdateformat'); - $config->setSystemValue("logfile", $config->getSystemValue('datadirectory') . "/logtest"); - $this->logFile = new File($config->getSystemValue('datadirectory') . '/logtest', '', $config); + $config->setValue("logfile", $config->getValue('datadirectory') . "/logtest.log"); + $this->logFile = new File($config->getValue('datadirectory') . '/logtest.log', '', $config); } protected function tearDown() { - $config = \OC::$server->getConfig(); + $config = \OC::$server->getSystemConfig(); if (isset($this->restore_logfile)) { - $config->getSystemValue("logfile", $this->restore_logfile); + $config->getValue("logfile", $this->restore_logfile); } else { - $config->deleteSystemValue("logfile"); + $config->deleteValue("logfile"); } if (isset($this->restore_logdateformat)) { - $config->getSystemValue("logdateformat", $this->restore_logdateformat); + $config->getValue("logdateformat", $this->restore_logdateformat); } else { - $config->deleteSystemValue("logdateformat"); + $config->deleteValue("logdateformat"); } $this->logFile = new File($this->restore_logfile, '', $config); parent::tearDown(); diff --git a/tests/lib/Log/LogFactoryTest.php b/tests/lib/Log/LogFactoryTest.php index 8c1b7f08c74a7..08139f54df493 100644 --- a/tests/lib/Log/LogFactoryTest.php +++ b/tests/lib/Log/LogFactoryTest.php @@ -26,6 +26,7 @@ use OC\Log\File; use OC\Log\LogFactory; use OC\Log\Syslog; +use OC\SystemConfig; use OCP\IConfig; use OCP\IServerContainer; use Test\TestCase; @@ -42,12 +43,16 @@ class LogFactoryTest extends TestCase { /** @var LogFactory */ protected $factory; + /** @var SystemConfig|\PHPUnit_Framework_MockObject_MockObject */ + protected $systemConfig; + protected function setUp() { parent::setUp(); $this->c = $this->createMock(IServerContainer::class); + $this->systemConfig = $this->createMock(SystemConfig::class); - $this->factory = new LogFactory($this->c); + $this->factory = new LogFactory($this->c, $this->systemConfig); } public function fileTypeProvider(): array { @@ -76,16 +81,11 @@ public function testFile(string $type) { $datadir = \OC::$SERVERROOT.'/data'; $defaultLog = $datadir . '/nextcloud.log'; - $config = $this->createMock(IConfig::class); - $config->expects($this->exactly(2)) - ->method('getSystemValue') + $this->systemConfig->expects($this->exactly(2)) + ->method('getValue') ->withConsecutive(['datadirectory', $datadir], ['logfile', $defaultLog]) ->willReturnOnConsecutiveCalls($datadir, $defaultLog); - $this->c->expects($this->any()) - ->method('getConfig') - ->willReturn($config); - $log = $this->factory->get($type); $this->assertInstanceOf(File::class, $log); } @@ -111,16 +111,11 @@ public function testFileCustomPath($path, $expected) { $datadir = \OC::$SERVERROOT.'/data'; $defaultLog = $datadir . '/nextcloud.log'; - $config = $this->createMock(IConfig::class); - $config->expects($this->exactly(2)) - ->method('getSystemValue') + $this->systemConfig->expects($this->exactly(2)) + ->method('getValue') ->withConsecutive(['datadirectory', $datadir], ['logfile', $defaultLog]) ->willReturnOnConsecutiveCalls($datadir, $path); - $this->c->expects($this->any()) - ->method('getConfig') - ->willReturn($config); - $log = $this->factory->get('file'); $this->assertInstanceOf(File::class, $log); $this->assertSame($expected, $log->getLogFilePath());