username

Author: tchapi

src/Controller/Api/ApiController.php

@@ -7,6 +7,7 @@
 use App\Entity\CalendarSubscription;
 use App\Entity\Principal;
 use App\Entity\User;
+use App\Services\Utils;
 use Doctrine\Persistence\ManagerRegistry;
 use Sabre\DAV\Sharing\Plugin as SharingPlugin;
 use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
@@ -26,7 +27,7 @@ class ApiController extends AbstractController
      */
     private function validateUsername(string $username): bool
     {
-        return !empty($username) && is_string($username) && !preg_match('/[^a-zA-Z0-9_.@-]/', $username);
+        return Utils::isValidUsername($username);
     }
 
     /**

src/Entity/User.php

@@ -18,8 +18,18 @@ class User
     #[ORM\Column(type: 'integer')]
     private $id;
 
+    /**
+     * A username ends up in the principal URI (`principals/<username>`), so it must not carry
+     * anything that would change that path's structure. Letters, digits and `_ . @ + ' -` are allowed:
+     * the punctuation is what shows up in mail-derived login names. Enforced when a user is created; existing
+     * accounts are left alone so that an odd username created before this rule stays editable.
+     */
+    public const USERNAME_PATTERN = '/^[a-zA-Z0-9_.@+\'-]+$/';
+
     #[ORM\Column(type: 'string', length: 255, unique: true)]
     #[Assert\NotBlank]
+    #[Assert\Length(max: 255, groups: ['creation'])]
+    #[Assert\Regex(pattern: self::USERNAME_PATTERN, message: 'form.username.invalid', groups: ['creation'])]
     private $username;
 
     #[ORM\Column(name: 'digesta1', type: 'string', length: 255)]

src/Form/UserType.php

@@ -11,6 +11,7 @@
 use Symfony\Component\Form\Extension\Core\Type\SubmitType;
 use Symfony\Component\Form\Extension\Core\Type\TextType;
 use Symfony\Component\Form\FormBuilderInterface;
+use Symfony\Component\Form\FormInterface;
 use Symfony\Component\OptionsResolver\OptionsResolver;
 
 class UserType extends AbstractType
@@ -55,6 +56,11 @@ public function configureOptions(OptionsResolver $resolver): void
         $resolver->setDefaults([
             'new' => false,
             'data_class' => User::class,
+            // The username rule only applies to new accounts: the field is disabled when editing,
+            // and an account created before the rule (or by LDAP/IMAP) must stay editable.
+            'validation_groups' => static fn (FormInterface $form): array => $form->getConfig()->getOption('new')
+                ? ['Default', 'creation']
+                : ['Default'],
         ]);
     }
 }

src/Services/AbstractAuth.php

@@ -12,6 +12,13 @@
  * and an empty password means an *unauthenticated bind* for LDAP servers, which Active
  * Directory (and OpenLDAP with `allow bind_anon_cred`) answers with success, i.e. it
  * would log the caller in as any user.
+ *
+ * It also rejects usernames that would not survive being put in a principal URI. sabre
+ * derives the principal from the login name (`principals/<username>`), so a name containing
+ * a slash would address a different, possibly existing, node: `alice/calendar-proxy-write`
+ * is exactly the URI Davis uses for alice's delegation proxy. Only structural characters are
+ * refused here, not the stricter set required when creating an account, so that an unusual
+ * but working username keeps authenticating.
  */
 abstract class AbstractAuth extends AbstractBasic
 {
@@ -25,6 +32,13 @@ final protected function validateUserPass($username, $password): bool
             return false;
         }
 
+        // Anything that would change the shape of `principals/<username>`:
+        //   [/\\]            a forward or back slash, which would add a path segment
+        //   [\x00-\x20\x7f]  any control character, plus space (0x20) and DEL (0x7f)
+        if (1 === preg_match('~[/\\\\]|[\\x00-\\x20\\x7f]~', $username)) {
+            return false;
+        }
+
         return $this->checkCredentials($username, $password);
     }
 

src/Services/IMAPAuth.php

@@ -130,15 +130,17 @@ protected function imapOpen(string $username, string $password): bool
             $user = $this->doctrine->getRepository(User::class)->findOneBy(['username' => $username]);
 
             if (!$user) {
-                // We only have a username, so we use it for displayname and email
-                $this->utils->createPasswordlessUserWithDefaultObjects($username, $username, $username);
-
-                $em = $this->doctrine->getManager();
-
                 try {
-                    $em->flush();
-                } catch (\Exception $e) {
-                    error_log('IMAP Error (flush): '.$e->getMessage());
+                    // We only have a username, so we use it for displayname and email
+                    $this->utils->createPasswordlessUserWithDefaultObjects($username, $username, $username);
+                    $this->doctrine->getManager()->flush();
+                } catch (\Throwable $e) {
+                    // Letting the login through without a principal would leave the account
+                    // authenticated but unusable: no calendar home, so clients fall back to the
+                    // server root and every write is refused.
+                    error_log('IMAP Error (could not create the user "'.$username.'"): '.$e->getMessage());
+
+                    return false;
                 }
             }
         }

src/Services/LDAPAuth.php

@@ -82,6 +82,39 @@ public function __construct(ManagerRegistry $doctrine, Utils $utils, string $LDA
         $this->utils = $utils;
     }
 
+    /**
+     * Builds the bind DN for a username by filling the placeholders of LDAP_DN_PATTERN.
+     *
+     * Every substituted value is escaped for a DN context: without that, a username such as
+     * `someone,ou=admins` would not be a value inside the DN but extra structure, changing
+     * which entry we bind against.
+     */
+    protected function buildDn(string $username): string
+    {
+        $escape = static fn (string $value): string => ldap_escape($value, '', LDAP_ESCAPE_DN);
+
+        // Extract user and domain from username (in the form user@domain.org)
+        $user_parts = explode('@', $username, 2);
+
+        $ldap_user = $user_parts[0];
+        $ldap_domain = $user_parts[1] ?? '';
+
+        // Replace common placeholders
+        $dn = str_replace(
+            ['%u', '%U', '%d'],
+            [$escape($username), $escape($ldap_user), $escape($ldap_domain)],
+            $this->LDAPDnPattern
+        );
+
+        // Replace domain parts
+        $domain_split = array_reverse(explode('.', $ldap_domain));
+        for ($i = 1; $i <= count($domain_split) and $i <= 9; ++$i) {
+            $dn = str_replace('%'.$i, $escape($domain_split[$i - 1]), $dn);
+        }
+
+        return $dn;
+    }
+
     /**
      * Connects to an LDAP server and tries to authenticate.
      *
@@ -140,25 +173,7 @@ protected function ldapOpen($username, $password)
             return false;
         }
 
-        // Extract user and domain from username (in the form user@domain.org)
-        $user_parts = explode('@', $username, 2);
-
-        $ldap_user = $user_parts[0];
-
-        if (count($user_parts) > 1) {
-            $ldap_domain = $user_parts[1];
-        } else {
-            $ldap_domain = '';
-        }
-
-        // Replace common placeholders
-        $dn = str_replace(['%u', '%U', '%d'], [$username, $ldap_user, $ldap_domain], $this->LDAPDnPattern);
-
-        // Replace domain parts
-        $domain_split = array_reverse(explode('.', $ldap_domain));
-        for ($i = 1; $i <= count($domain_split) and $i <= 9; ++$i) {
-            $dn = str_replace('%'.$i, $domain_split[$i - 1], $dn);
-        }
+        $dn = $this->buildDn($username);
 
         $success = false;
         try {
@@ -200,14 +215,16 @@ protected function ldapOpen($username, $password)
                     }
                 }
 
-                $this->utils->createPasswordlessUserWithDefaultObjects($username, $displayName, $email);
-
-                $em = $this->doctrine->getManager();
-
                 try {
-                    $em->flush();
-                } catch (\Exception $e) {
-                    error_log('LDAP Error (flush): '.$e->getMessage());
+                    $this->utils->createPasswordlessUserWithDefaultObjects($username, $displayName, $email);
+                    $this->doctrine->getManager()->flush();
+                } catch (\Throwable $e) {
+                    // Letting the login through without a principal would leave the account
+                    // authenticated but unusable: no calendar home, so clients fall back to the
+                    // server root and every write is refused.
+                    error_log('LDAP Error (could not create the user "'.$username.'"): '.$e->getMessage());
+
+                    $success = false;
                 }
             }
         }

src/Services/Utils.php

@@ -49,8 +49,20 @@ public function hashPassword(string $username, string $password): string
         return md5($username.':'.$this->authRealm.':'.$password);
     }
 
+    /**
+     * A username is only acceptable if it can be used verbatim in a principal URI.
+     */
+    public static function isValidUsername(?string $username): bool
+    {
+        return null !== $username && '' !== $username && 1 === preg_match(User::USERNAME_PATTERN, $username);
+    }
+
     public function createPasswordlessUserWithDefaultObjects(string $username, string $displayName, string $email)
     {
+        if (!self::isValidUsername($username)) {
+            throw new \InvalidArgumentException(sprintf('Refusing to create the user "%s": a username may only contain letters, digits and the characters _ . @ + \' -', $username));
+        }
+
         $user = new User();
         $user->setUsername($username);
 

tests/Functional/Controllers/UserControllerTest.php

@@ -228,4 +228,29 @@ public function testDelegateRemoveThroughAnotherUsersProxyIs404(): void
         $this->postAdmin($client, '/users/delegates/'.$userId.'/remove/'.$foreignProxy->getId().'/'.$delegate->getId());
         $this->assertResponseStatusCodeSame(404);
     }
+
+    public function testUserCreationRejectsAUsernameThatBreaksThePrincipalUri(): void
+    {
+        $user = new AdminUser('admin', 'test');
+
+        $client = static::createClient();
+        $client->loginUser($user);
+
+        $crawler = $client->request('GET', '/users/new');
+        $form = $crawler->selectButton('user_save')->form();
+
+        $client->submit($form, [
+            'user[username]' => 'bad/user',
+            'user[displayName]' => 'Bad User',
+            'user[email]' => 'bad@example.org',
+            'user[password][first]' => 'secret',
+            'user[password][second]' => 'secret',
+        ]);
+
+        // The form is re-rendered rather than redirecting, and nothing is created
+        $this->assertResponseIsSuccessful();
+        $this->assertNull(
+            static::getContainer()->get('doctrine.orm.entity_manager')->getRepository(User::class)->findOneByUsername('bad/user')
+        );
+    }
 }

tests/Functional/Service/AuthBackendTest.php

@@ -103,4 +103,31 @@ public function testBasicAuthStillAcceptsValidCredentials(): void
         [$ok] = self::check($backend, 'test_user:wrong');
         $this->assertFalse($ok);
     }
+
+    /**
+     * A username becomes the principal URI (`principals/<username>`), so one containing a
+     * slash would address a different node — `alice/calendar-proxy-write` is exactly the URI
+     * Davis uses for alice's delegation proxy.
+     */
+    public function testUsernamesThatWouldBreakThePrincipalUriAreRejected(): void
+    {
+        foreach (['alice/calendar-proxy-write', 'alice\\bob', 'alice bob', "alice\tbob", "alice\nbob"] as $username) {
+            $backend = self::acceptAllBackend();
+
+            [$ok] = self::check($backend, $username.':password');
+
+            $this->assertFalse($ok, sprintf('%s must not authenticate', var_export($username, true)));
+            $this->assertSame([], $backend->seen, 'The backend must not even be consulted');
+        }
+    }
+
+    public function testAnUnusualButStructurallySoundUsernameStillAuthenticates(): void
+    {
+        $backend = self::acceptAllBackend();
+
+        [$ok, $principal] = self::check($backend, 'first.last+tag@example.org:password');
+
+        $this->assertTrue($ok);
+        $this->assertSame('principals/first.last+tag@example.org', $principal);
+    }
 }

tests/Functional/Service/LDAPAuthTest.php

@@ -0,0 +1,77 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Services;
+
+use App\Services\LDAPAuth;
+use App\Services\Utils;
+use Doctrine\Persistence\ManagerRegistry;
+use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
+
+class LDAPAuthTest extends KernelTestCase
+{
+    private function buildDn(string $pattern, string $username): string
+    {
+        self::bootKernel();
+        $container = static::getContainer();
+
+        $backend = new LDAPAuth(
+            $container->get(ManagerRegistry::class),
+            $container->get(Utils::class),
+            'ldap://127.0.0.1',
+            $pattern,
+            'mail',
+            false,
+            'try'
+        );
+
+        // buildDn is protected; no setAccessible() needed since PHP 8.1
+        return (new \ReflectionMethod($backend, 'buildDn'))->invoke($backend, $username);
+    }
+
+    public function testPlaceholdersAreFilledIn(): void
+    {
+        $this->assertSame(
+            'uid=alice,ou=users,dc=example,dc=com',
+            $this->buildDn('uid=%u,ou=users,dc=example,dc=com', 'alice')
+        );
+    }
+
+    public function testUserAndDomainPartsAreSplitOnTheAtSign(): void
+    {
+        $this->assertSame(
+            'uid=alice,dc=example.org',
+            $this->buildDn('uid=%U,dc=%d', 'alice@example.org')
+        );
+    }
+
+    public function testDomainComponentsAreAvailableInReverseOrder(): void
+    {
+        $this->assertSame(
+            'uid=alice,dc=example,dc=org',
+            $this->buildDn('uid=%U,dc=%2,dc=%1', 'alice@example.org')
+        );
+    }
+
+    /**
+     * Regression test: the username was interpolated into the DN pattern verbatim, so a name
+     * carrying DN syntax added structure to the DN instead of being a value inside it.
+     */
+    public function testAUsernameCannotInjectDnStructure(): void
+    {
+        $evil = 'alice,ou=admins';
+
+        $dn = $this->buildDn('uid=%u,ou=users,dc=example,dc=com', $evil);
+
+        $this->assertSame('uid='.ldap_escape($evil, '', LDAP_ESCAPE_DN).',ou=users,dc=example,dc=com', $dn);
+        $this->assertStringNotContainsString('uid=alice,ou=admins,', $dn, 'The comma must not stay structural');
+    }
+
+    public function testTheDomainPartIsEscapedToo(): void
+    {
+        $dn = $this->buildDn('uid=%U,dc=%d', 'alice@example.org,ou=admins');
+
+        $this->assertStringNotContainsString('dc=example.org,ou=admins', $dn);
+    }
+}