Enhance name normalization and validation with logging and additional tests

Author: arabcoders

src/Commands/Backend/CreateUsersCommand.php

@@ -254,7 +254,10 @@ private function get_backends_users(array $backends, array &$map): array
 
                     // -- this was source of lots of bugs and confusion for users,
                     // -- we decided to normalize the user-names early in the process.
-                    $user['name'] = normalizeName((string)$user['name']);
+                    $user['name'] = normalizeName((string)$user['name'], $this->logger, [
+                        'log_message' => "Normalized '{backend}: {name}' to '{backend}: {new_name}'",
+                        'context' => [ 'backend' => $backedName ],
+                    ]);
 
                     // -- run map actions.
                     $this->map_actions($backedName, $user, $map);
@@ -277,7 +280,7 @@ private function get_backends_users(array $backends, array &$map): array
                     $info['backendName'] = normalizeName(r("{backend}_{user}", [
                         'backend' => $backedName,
                         'user' => $user['name']
-                    ]));
+                    ]),$this->logger);
 
                     if (false === isValidName($info['backendName'])) {
                         $this->logger->error(
@@ -357,7 +360,7 @@ private function create_user(iInput $input, array $users): void
 
         foreach ($users as $user) {
             // -- User subdirectory name.
-            $userName = normalizeName(ag($user, 'name', 'unknown'));
+            $userName = normalizeName(ag($user, 'name', 'unknown'), $this->logger);
 
             if (false === isValidName($userName)) {
                 $this->logger->error(

src/Libs/helpers.php

@@ -730,7 +730,7 @@ function makeBackend(array $backend, string|null $name = null, array $options =
                 backendUrl: new Uri(ag($backend, 'url')),
                 cache: $cache,
                 userContext: $userContext ?? Container::get(UserContext::class),
-                logger: ag($options, iLogger::class, fn() => Container::get(iLogger::class)),
+                logger: ag($options, iLogger::class, fn () => Container::get(iLogger::class)),
                 backendId: ag($backend, 'uuid', null),
                 backendToken: ag($backend, 'token', null),
                 backendUser: ag($backend, 'user', null),
@@ -909,10 +909,6 @@ function getAppVersion(): string
      */
     function isValidName(string $name): bool
     {
-        if (true === ctype_digit($name[0])) {
-            return false;
-        }
-
         return 1 === preg_match('/^[a-z_0-9]+$/', $name);
     }
 }
@@ -949,7 +945,7 @@ function formatDuration(int|float $milliseconds): string
      */
     function array_keys_diff(array $base, array $list, bool $has = true): array
     {
-        return array_filter($base, fn($key) => $has === in_array($key, $list), ARRAY_FILTER_USE_KEY);
+        return array_filter($base, fn ($key) => $has === in_array($key, $list), ARRAY_FILTER_USE_KEY);
     }
 }
 
@@ -1354,7 +1350,7 @@ function getSystemMemoryInfo(string $memFile = '/proc/meminfo'): array
     function parseConfigValue(mixed $value, Closure|null $callback = null): mixed
     {
         if (is_string($value) && preg_match('#%{(.+?)}#s', $value)) {
-            $val = preg_replace_callback('#%{(.+?)}#s', fn($match) => Config::get($match[1], $match[1]), $value);
+            $val = preg_replace_callback('#%{(.+?)}#s', fn ($match) => Config::get($match[1], $match[1]), $value);
             return null !== $callback && null !== $val ? $callback($val) : $val;
         }
 
@@ -1389,7 +1385,7 @@ function checkIgnoreRule(string $guid, UserContext|null $userContext = null): bo
         if (false === in_array('guid_' . $db, $sources)) {
             throw new RuntimeException(r("Invalid db source name '{db}' was given. Expected values are '{dbs}'.", [
                 'db' => $db,
-                'dbs' => implode(', ', array_map(fn($f) => after($f, 'guid_'), $sources)),
+                'dbs' => implode(', ', array_map(fn ($f) => after($f, 'guid_'), $sources)),
             ]));
         }
 
@@ -2112,8 +2108,8 @@ function registerEvents(bool $ignoreCache = false): void
         /** @var array<ScannerItem> $list */
         $list = cacheableItem(
             'event_listeners',
-            fn() => AttributesScanner::scan(Config::get('events.listeners.locations', []))->for(EventListener::class),
-            Config::get('events.listeners.cache', fn() => new DateInterval('PT1H')),
+            fn () => AttributesScanner::scan(Config::get('events.listeners.locations', []))->for(EventListener::class),
+            Config::get('events.listeners.cache', fn () => new DateInterval('PT1H')),
             $ignoreCache
         );
 
@@ -2147,7 +2143,7 @@ function registerEvents(bool $ignoreCache = false): void
      */
     function queueEvent(string $event, array $data = [], array $opts = []): EventInfo
     {
-        $repo = ag($opts, EventsRepository::class, fn() => Container::get(EventsRepository::class));
+        $repo = ag($opts, EventsRepository::class, fn () => Container::get(EventsRepository::class));
         assert($repo instanceof EventsRepository);
 
         $item = null;
@@ -2428,7 +2424,7 @@ function perUserCacheAdapter(string $user): CacheInterface
         }
 
         $ns = getAppVersion();
-        $ns .= isValidName($user) ? '.' . $user : '.' . md5($user);
+        $ns .= isValidName($user) ? ".{$user}" : '.' . md5($user);
 
         try {
             $backend = new RedisAdapter(redis: Container::get(Redis::class), namespace: $ns);
@@ -2731,13 +2727,28 @@ function deletePath(string $path, iLogger|null $logger = null, bool $dryRun = fa
      * Normalize the name to be in [a-z_0-9] format.
      *
      * @param string $name The name to normalize.
+     * @param iLogger |null $logger (Optional) The logger instance.
      *
      * @return string The normalized name.
      */
-    function normalizeName(string $name): string
+    function normalizeName(string $name, iLogger|null $logger = null,array $opts=[]): string
     {
-        $name = strtolower($name);
-        $name = preg_replace('/[^a-z0-9_]/', '_', $name);
-        return trim($name);
+        if (true === ctype_digit($name)) {
+            $newName = 'user_' . $name;
+        } else {
+            $newName = strtolower($name);
+            $newName = preg_replace('/[^a-z0-9_]/', '_', $newName);
+            $newName = trim($newName);
+        }
+
+        if ($newName !== $name && null !== $logger) {
+            $logger->notice(ag($opts, 'log_message', "Normalized '{name}' to '{new_name}'."), [
+                'name' => $name,
+                'new_name' => $newName,
+                ...ag($opts,'context',[])
+            ]);
+        }
+
+        return $newName;
     }
 }

tests/Libs/HelpersTest.php

@@ -678,12 +678,12 @@ public function __toString(): string
 
     public function test_isValidName(): void
     {
-        $this->assertTrue(isValidName('foo'), 'When name is valid, true is returned.');
-        $this->assertTrue(isValidName('foo_bar'), 'When name is valid, true is returned.');
-        $this->assertFalse(isValidName('foo_baR'), 'When name is invalid, false is returned.');
-        $this->assertFalse(isValidName('3oo_bar'), 'When name is invalid, false is returned.');
+        $validNames = ['foo', '123', 'foo_bar', '1foo_bar'];
+        $invalidNames = ['foo bar', 'foo-bar', 'foo/bar', 'foo?bar', 'foo*bar', 'foo_baR', 'FOOBAR'];
 
-        $invalidNames = ['foo bar', 'foo-bar', 'foo/bar', 'foo?bar', 'foo*bar', '1foo', 'foo_baR', 'FOOBAR'];
+        foreach ($validNames as $name) {
+            $this->assertTrue(isValidName($name), "When given name is '{$name}', true should be is returned.");
+        }
 
         foreach ($invalidNames as $name) {
             $this->assertFalse(isValidName($name), "When given name is '{$name}', false should be is returned.");
@@ -1632,4 +1632,36 @@ public function test_commandContext()
         );
         unset($_ENV['IN_CONTAINER']);
     }
+
+    public function test_normalizeName()
+    {
+        $isValid = ['foo','foo_bar','0user','user_123','user_123_foo','user_123_foo_bar'];
+        foreach ($isValid as $name) {
+            $this->assertSame(
+                $name,
+                normalizeName($name),
+                "When valid name '{$name}' is passed, it should return same string."
+            );
+        }
+
+        $this->assertSame(
+            'user_123',
+            normalizeName('123'),
+            'When name is made entirely of numbers, it should prepend user_ to it.'
+        );
+
+        $invalidNames = [
+            'foo bar',
+            'foo-bar',
+            'foo@baR',
+        ];
+
+        foreach ($invalidNames as $name) {
+            $this->assertSame(
+                'foo_bar',
+                normalizeName($name),
+                "When invalid name '{$name}' is passed, it should return same string with underscores."
+            );
+        }
+    }
 }