Skip to content

Commit

Permalink
Merge pull request #45277 from nextcloud/techdebt/noid/log-condition-v2
Browse files Browse the repository at this point in the history 
feat(log): Allow to combine log.conditions to only log (app&user)
  • Loading branch information
@nickvergessen
nickvergessen authored Jul 30, 2024
2 parents 21f558b + 710a69b commit 8760c8e
Show file tree
Hide file tree
Showing 5 changed files with 172 additions and 25 deletions.
12 changes: 12 additions & 0 deletions config/config.sample.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1057,13 +1057,25 @@
* this condition is met
* - ``apps``: if the log message is invoked by one of the specified apps,
* this condition is met
* - ``matches``: if all the conditions inside a group match,
* this condition is met. This allows to log only entries to an app
* by a few users.
*
* Defaults to an empty array.
*/
'log.condition' => [
'shared_secret' => '57b58edb6637fe3059b3595cf9c41b9',
'users' => ['sample-user'],
'apps' => ['files'],
'matches' => [
[
'shared_secret' => '57b58edb6637fe3059b3595cf9c41b9',
'users' => ['sample-user'],
'apps' => ['files'],
'loglevel' => 1,
'message' => 'contains substring'
],
],
],

/**
Expand Down
2 changes: 1 addition & 1 deletion lib/private/Diagnostics/EventLogger.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public function isLoggingActivated(): bool {
return true;
}

$isDebugLevel = $this->internalLogger->getLogLevel([]) === Log::DEBUG;
$isDebugLevel = $this->internalLogger->getLogLevel([], '') === Log::DEBUG;
return $systemValue && $isDebugLevel;
}

Expand Down
92 changes: 68 additions & 24 deletions lib/private/Log.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use OC\Log\ExceptionSerializer;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\ILogger;
use OCP\IRequest;
use OCP\IUserSession;
use OCP\Log\BeforeMessageLoggedEvent;
use OCP\Log\IDataLogger;
Expand Down Expand Up @@ -153,7 +154,7 @@ public function debug(string $message, array $context = []): void {
* @param array $context
*/
public function log(int $level, string $message, array $context = []): void {
$minLevel = $this->getLogLevel($context);
$minLevel = $this->getLogLevel($context, $message);
if ($level < $minLevel
&& (($this->crashReporters?->hasReporters() ?? false) === false)
&& (($this->eventDispatcher?->hasListeners(BeforeMessageLoggedEvent::class) ?? false) === false)) {
Expand Down Expand Up @@ -194,9 +195,25 @@ public function log(int $level, string $message, array $context = []): void {
}
}

public function getLogLevel($context): int {
public function getLogLevel(array $context, string $message): int {
/**
* @psalm-var array{
* shared_secret?: string,
* users?: string[],
* apps?: string[],
* matches?: array<array-key, array{
* shared_secret?: string,
* users?: string[],
* apps?: string[],
* message?: string,
* loglevel: 0|1|2|3|4,
* }>
* } $logCondition
*/
$logCondition = $this->config->getValue('log.condition', []);

$userId = false;

/**
* check for a special log condition - this enables an increased log on
* a per request/user base
Expand All @@ -206,21 +223,8 @@ public function getLogLevel($context): int {
$this->logConditionSatisfied = false;
if (!empty($logCondition)) {
// check for secret token in the request
if (isset($logCondition['shared_secret'])) {
$request = \OC::$server->getRequest();

if ($request->getMethod() === 'PUT' &&
!str_contains($request->getHeader('Content-Type'), 'application/x-www-form-urlencoded') &&
!str_contains($request->getHeader('Content-Type'), 'application/json')) {
$logSecretRequest = '';
} else {
$logSecretRequest = $request->getParam('log_secret', '');
}

// if token is found in the request change set the log condition to satisfied
if ($request && hash_equals($logCondition['shared_secret'], $logSecretRequest)) {
$this->logConditionSatisfied = true;
}
if (isset($logCondition['shared_secret']) && $this->checkLogSecret($logCondition['shared_secret'])) {
$this->logConditionSatisfied = true;
}

// check for user
Expand All @@ -233,6 +237,8 @@ public function getLogLevel($context): int {
} elseif (in_array($user->getUID(), $logCondition['users'], true)) {
// if the user matches set the log condition to satisfied
$this->logConditionSatisfied = true;
} else {
$userId = $user->getUID();
}
}
}
Expand All @@ -243,6 +249,11 @@ public function getLogLevel($context): int {
return ILogger::DEBUG;
}

if ($userId === false && isset($logCondition['matches'])) {
$user = \OCP\Server::get(IUserSession::class)->getUser();
$userId = $user === null ? false : $user->getUID();
}

if (isset($context['app'])) {
/**
* check log condition based on the context of each log message
Expand All @@ -253,16 +264,49 @@ public function getLogLevel($context): int {
}
}

$configLogLevel = $this->config->getValue('loglevel', ILogger::WARN);
if (is_numeric($configLogLevel)) {
return min((int)$configLogLevel, ILogger::FATAL);
if (!isset($logCondition['matches'])) {
$configLogLevel = $this->config->getValue('loglevel', ILogger::WARN);
if (is_numeric($configLogLevel)) {
return min((int)$configLogLevel, ILogger::FATAL);
}

// Invalid configuration, warn the user and fall back to default level of WARN
error_log('Nextcloud configuration: "loglevel" is not a valid integer');
return ILogger::WARN;
}

foreach ($logCondition['matches'] as $option) {
if (
(!isset($option['shared_secret']) || $this->checkLogSecret($option['shared_secret']))
&& (!isset($option['users']) || in_array($userId, $option['users'], true))
&& (!isset($option['apps']) || (isset($context['app']) && in_array($context['app'], $option['apps'], true)))
&& (!isset($option['message']) || str_contains($message, $option['message']))
) {
if (!isset($option['apps']) && !isset($option['loglevel']) && !isset($option['message'])) {
/* Only user and/or secret are listed as conditions, we can cache the result for the rest of the request */
$this->logConditionSatisfied = true;
return ILogger::DEBUG;
}
return $option['loglevel'] ?? ILogger::DEBUG;
}
}

// Invalid configuration, warn the user and fall back to default level of WARN
error_log('Nextcloud configuration: "loglevel" is not a valid integer');
return ILogger::WARN;
}

protected function checkLogSecret(string $conditionSecret): bool {
$request = \OCP\Server::get(IRequest::class);

if ($request->getMethod() === 'PUT' &&
!str_contains($request->getHeader('Content-Type'), 'application/x-www-form-urlencoded') &&
!str_contains($request->getHeader('Content-Type'), 'application/json')) {
return hash_equals($conditionSecret, '');
}

// if token is found in the request change set the log condition to satisfied
return hash_equals($conditionSecret, $request->getParam('log_secret', ''));
}

/**
* Logs an exception very detailed
*
Expand All @@ -275,7 +319,7 @@ public function logException(Throwable $exception, array $context = []): void {
$app = $context['app'] ?? 'no app in context';
$level = $context['level'] ?? ILogger::ERROR;

$minLevel = $this->getLogLevel($context);
$minLevel = $this->getLogLevel($context, $context['message'] ?? $exception->getMessage());
if ($level < $minLevel
&& (($this->crashReporters?->hasReporters() ?? false) === false)
&& (($this->eventDispatcher?->hasListeners(BeforeMessageLoggedEvent::class) ?? false) === false)) {
Expand Down Expand Up @@ -320,7 +364,7 @@ public function logData(string $message, array $data, array $context = []): void
$app = $context['app'] ?? 'no app in context';
$level = $context['level'] ?? ILogger::ERROR;

$minLevel = $this->getLogLevel($context);
$minLevel = $this->getLogLevel($context, $message);

array_walk($context, [$this->normalizer, 'format']);

Expand Down
1 change: 1 addition & 0 deletions lib/private/SystemConfig.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ class SystemConfig {
'github.client_secret' => true,
'log.condition' => [
'shared_secret' => true,
'matches' => true,
],
'license-key' => true,
'redis' => [
Expand Down
90 changes: 90 additions & 0 deletions tests/lib/LoggerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
use OC\Log;
use OC\SystemConfig;
use OCP\ILogger;
use OCP\IUser;
use OCP\IUserSession;
use OCP\Log\IWriter;
use OCP\Support\CrashReport\IRegistry;
use PHPUnit\Framework\MockObject\MockObject;
Expand Down Expand Up @@ -73,6 +75,94 @@ public function testAppCondition() {
$this->assertEquals($expected, $this->getLogs());
}

public function dataMatchesCondition(): array {
return [
[
'user0',
[
'apps' => ['app2'],
],
[
'1 Info of app2',
],
],
[
'user2',
[
'users' => ['user1', 'user2'],
'apps' => ['app1'],
],
[
'1 Info of app1',
],
],
[
'user3',
[
'users' => ['user3'],
],
[
'1 Info without app',
'1 Info of app1',
'1 Info of app2',
'0 Debug of app3',
],
],
[
'user4',
[
'users' => ['user4'],
'apps' => ['app3'],
'loglevel' => 0,
],
[
'0 Debug of app3',
],
],
[
'user4',
[
'message' => ' of ',
],
[
'1 Info of app1',
'1 Info of app2',
'0 Debug of app3',
],
],
];
}

/**
* @dataProvider dataMatchesCondition
*/
public function testMatchesCondition(string $userId, array $conditions, array $expectedLogs): void {
$this->config->expects($this->any())
->method('getValue')
->willReturnMap([
['loglevel', ILogger::WARN, ILogger::WARN],
['log.condition', [], ['matches' => [
$conditions,
]]],
]);
$logger = $this->logger;

$user = $this->createMock(IUser::class);
$user->method('getUID')
->willReturn($userId);
$userSession = $this->createMock(IUserSession::class);
$userSession->method('getUser')
->willReturn($user);
$this->overwriteService(IUserSession::class, $userSession);

$logger->info('Info without app');
$logger->info('Info of app1', ['app' => 'app1']);
$logger->info('Info of app2', ['app' => 'app2']);
$logger->debug('Debug of app3', ['app' => 'app3']);

$this->assertEquals($expectedLogs, $this->getLogs());
}

public function testLoggingWithDataArray(): void {
$this->mockDefaultLogLevel();
$writerMock = $this->createMock(IWriter::class);
Expand Down

0 comments on commit 8760c8e

Please sign in to comment.