refactor(admin_audit): improve type safety and clarity in Action logging

apps/admin_audit/lib/Actions/Action.php

@@ -2,65 +2,98 @@
 
 declare(strict_types=1);
 /**
- * SPDX-FileCopyrightText: 2016 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-FileCopyrightText: 2016-2026 Nextcloud GmbH and Nextcloud contributors
  * SPDX-License-Identifier: AGPL-3.0-or-later
  */
 namespace OCA\AdminAudit\Actions;
 
 use OCA\AdminAudit\IAuditLogger;
 
+/**
+ * Base class for audit logging actions
+ *
+ * Provides structured logging for admin audit events with type-safe parameter handling
+ */
 class Action {
-
 	public function __construct(
-		private IAuditLogger $logger,
+		private readonly IAuditLogger $logger,
 	) {
 	}
 
 	/**
-	 * Log a single action with a log level of info
+	 * Log a single action with a log level of info.
 	 *
-	 * @param string $text
-	 * @param array $params
-	 * @param array $elements
-	 * @param bool $obfuscateParameters
+	 * Example usage:
+	 *   $this->log(
+	 *       'User "%s" added to group "%s"',
+	 *       ['user' => 'alice', 'group' => 'admins'],
+	 *       ['user', 'group']
+	 *   );
+	 *
+	 * @param string $messageTemplate Format string for vsprintf (e.g., "User %s deleted file %s")
+	 * @param array $data Associative array of data values
+	 * @param array $requiredKeys Array of keys that must exist in $data; order must match format placeholders
+	 * @param bool $excludeSensitiveData If true, omit parameter details from error logs
 	 */
-	public function log(string $text,
-		array $params,
-		array $elements,
-		bool $obfuscateParameters = false): void {
-		foreach ($elements as $element) {
-			if (!isset($params[$element])) {
-				if ($obfuscateParameters) {
-					$this->logger->critical(
-						'$params["' . $element . '"] was missing.',
-						['app' => 'admin_audit']
-					);
-				} else {
-					$this->logger->critical(
-						'$params["' . $element . '"] was missing. Transferred value: {params}',
-						['app' => 'admin_audit', 'params' => $params]
-					);
-				}
-				return;
+	public function log(
+		string $messageTemplate,
+		array $data,
+		array $requiredKeys,
+		bool $excludeSensitiveData = false
+	): void {
+		$missingKeys = [];
+		foreach ($requiredKeys as $key) {
+			if (!isset($data[$key])) {
+				$missingKeys[] = $key;
 			}
 		}
 
-		$replaceArray = [];
-		foreach ($elements as $element) {
-			if ($params[$element] instanceof \DateTime) {
-				$params[$element] = $params[$element]->format('Y-m-d H:i:s');
+		if (!empty($missingKeys)) {
+			$context = ['app' => 'admin_audit', 'missing_keys' => $missingKeys];
+
+			if (!$excludeSensitiveData) {
+				$context['provided_keys'] = array_keys($data);
 			}
-			$replaceArray[] = $params[$element];
+
+			$this->logger->critical(
+				'Required audit parameters missing: {missing_keys}',
+				$context
+			);
+			return;
 		}
 
-		$this->logger->info(
-			vsprintf(
-				$text,
-				$replaceArray
-			),
-			[
-				'app' => 'admin_audit'
-			]
-		);
+		$replacementValues = [];
+		foreach ($requiredKeys as $key) {
+			$value = $data[$key];
+
+			// Handle different types safely
+			if ($value instanceof \DateTime) {
+				$replacementValues[] = $value->format('Y-m-d H:i:s');
+			} elseif (is_bool($value)) {
+				$replacementValues[] = $value ? 'true' : 'false';
+			} elseif (is_scalar($value)) {
+				$replacementValues[] = (string)$value;
+			} elseif ($value === null) {
+				$replacementValues[] = 'null';
+			} else {
+				$replacementValues[] = json_encode($value, JSON_UNESCAPED_SLASHES) ?: gettype($value);
+			}
+		}
+
+		try {
+			$message = vsprintf($messageTemplate, $replacementValues);
+			$this->logger->info($message, ['app' => 'admin_audit']);
+		} catch (\ValueError $e) {
+			// vsprintf throws ValueError in PHP 8+ when format/argument mismatch occurs
+			$this->logger->critical(
+				'Audit log format string mismatch: {error}',
+				[
+					'app' => 'admin_audit',
+					'error' => $e->getMessage(),
+					'format' => $messageTemplate,
+					'element_count' => count($requiredKeys)
+				]
+			);
+		}
 	}
 }

apps/admin_audit/tests/Actions/ActionTest.php

@@ -0,0 +1,113 @@
+<?php
+// apps/admin_audit/tests/Actions/ActionTest.php
+
+declare(strict_types=1);
+/**
+ * SPDX-FileCopyrightText: 2016-2026 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+namespace OCA\AdminAudit\Tests\Actions;
+
+use OCA\AdminAudit\Actions\Action;
+use OCA\AdminAudit\IAuditLogger;
+use PHPUnit\Framework\MockObject\MockObject;
+use Test\TestCase;
+
+class ActionTest extends TestCase {
+    private IAuditLogger&MockObject $logger;
+    private Action $action;
+
+    protected function setUp(): void {
+        parent::setUp();
+        $this->logger = $this->createMock(IAuditLogger:: class);
+        $this->action = new Action($this->logger);
+    }
+
+    public function testLogWithBooleanTrue(): void {
+        $this->logger->expects($this->once())
+            ->method('info')
+            ->with('Setting enabled: true', ['app' => 'admin_audit']);
+
+        $this->action->log(
+            'Setting enabled: %s',
+            ['enabled' => true],
+            ['enabled']
+        );
+    }
+
+    public function testLogWithBooleanFalse(): void {
+        $this->logger->expects($this->once())
+            ->method('info')
+            ->with('Setting enabled: false', ['app' => 'admin_audit']);
+
+        $this->action->log(
+            'Setting enabled: %s',
+            ['enabled' => false],
+            ['enabled']
+        );
+    }
+
+    public function testLogWithNull(): void {
+        $this->logger->expects($this->once())
+            ->method('info')
+            ->with('Value is: null', ['app' => 'admin_audit']);
+
+        $this->action->log(
+            'Value is: %s',
+            ['value' => null],
+            ['value']
+        );
+    }
+
+    public function testLogWithMissingKey(): void {
+        $this->logger->expects($this->once())
+            ->method('critical')
+            ->with(
+                'Required audit parameters missing: {missing_keys}',
+                $this->callback(function ($context) {
+                    return $context['app'] === 'admin_audit'
+                        && $context['missing_keys'] === ['missing_key']
+                        && isset($context['provided_keys']);
+                })
+            );
+
+        $this->action->log(
+            'Value: %s',
+            ['other_key' => 'value'],
+            ['missing_key']
+        );
+    }
+
+    public function testLogWithDateTimeValue(): void {
+        $date = new \DateTime('2026-01-02 15:30:45');
+
+        $this->logger->expects($this->once())
+            ->method('info')
+            ->with('Date: 2026-01-02 15:30:45', ['app' => 'admin_audit']);
+
+        $this->action->log(
+            'Date: %s',
+            ['date' => $date],
+            ['date']
+        );
+    }
+
+    public function testLogWithFormatMismatch(): void {
+        $this->logger->expects($this->once())
+            ->method('critical')
+            ->with(
+                'Audit log format string mismatch: {error}',
+                $this->callback(function ($context) {
+                    return $context['app'] === 'admin_audit'
+                        && isset($context['error'])
+                        && $context['format'] === 'Too many:  %s %s %s';
+                })
+            );
+
+        $this->action->log(
+            'Too many: %s %s %s',
+            ['one' => '1', 'two' => '2'],
+            ['one', 'two']  // Only 2 values for 3 placeholders
+        );
+    }
+}