From 2601e46ec4a07a7256f75875449a945be7e528c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Fromme?= Date: Wed, 19 Aug 2026 12:30:42 +0200 Subject: [PATCH] fix: never persist a half-built user on first login --- src/Logger/DatabaseHandler.php | 40 ++++--- src/Security/BpnAuthenticator.php | 15 +-- tests/Logger/DatabaseHandlerTest.php | 153 ++++++++++++++++++++++++ tests/Security/BpnAuthenticatorTest.php | 36 +++++- 4 files changed, 218 insertions(+), 26 deletions(-) create mode 100644 tests/Logger/DatabaseHandlerTest.php diff --git a/src/Logger/DatabaseHandler.php b/src/Logger/DatabaseHandler.php index 367b023..e74d557 100644 --- a/src/Logger/DatabaseHandler.php +++ b/src/Logger/DatabaseHandler.php @@ -4,8 +4,8 @@ declare(strict_types=1); namespace App\Logger; -use App\Entity\LogEntry; -use Doctrine\ORM\EntityManagerInterface; +use Doctrine\DBAL\Connection; +use Doctrine\DBAL\Types\Types; use Monolog\Handler\AbstractProcessingHandler; use Monolog\Level; use Monolog\LogRecord; @@ -13,30 +13,40 @@ use Monolog\LogRecord; /** * Monolog handler that persists log entries to the database. * - * Writes log records to the LogEntry entity, replacing message placeholders + * Writes log records to the log_entry table, replacing message placeholders * with context values. Includes channel, context, and extra data for auditing. * Only processes INFO level and above to avoid storing debug messages. + * + * The write goes through the DBAL connection rather than the EntityManager on purpose. A log record + * can be emitted at any point, including halfway through somebody else's unit of work, and an + * EntityManager flush would write out whatever that unit of work has pending so far — which is how + * a half-built User once reached the database with a NULL password. Staying out of the UnitOfWork + * also keeps logging alive after a failed flush has closed the EntityManager, which is exactly when + * there is something worth logging. + * + * Reads still go through the LogEntry entity and its repository. */ class DatabaseHandler extends AbstractProcessingHandler { - public function __construct(private readonly EntityManagerInterface $entityManager) + public function __construct(private readonly Connection $connection) { parent::__construct(Level::Info); } protected function write(LogRecord $record): void { - $message = $this->replacePlaceHolder($record); - - $logEntry = new LogEntry($record->channel, $message); - $logEntry - ->setContext($record->context) - ->setExtra($record->extra) - ->setErrorCode($record->extra['error_code'] ?? null) - ; - - $this->entityManager->persist($logEntry); - $this->entityManager->flush(); + $this->connection->insert('log_entry', [ + 'channel' => $record->channel, + 'message' => $this->replacePlaceHolder($record), + 'context' => $record->context, + 'extra' => $record->extra, + 'error_code' => $record->extra['error_code'] ?? null, + 'created_at' => new \DateTimeImmutable('now'), + ], [ + 'context' => Types::JSON, + 'extra' => Types::JSON, + 'created_at' => Types::DATETIME_IMMUTABLE, + ]); } private function replacePlaceHolder(LogRecord $record): string diff --git a/src/Security/BpnAuthenticator.php b/src/Security/BpnAuthenticator.php index 63d4245..36ec5fb 100644 --- a/src/Security/BpnAuthenticator.php +++ b/src/Security/BpnAuthenticator.php @@ -106,13 +106,7 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent $userRepository = $this->entityManager->getRepository(User::class); - if (null === $user = $userRepository->findOneBy(['email' => $email])) { - $user = new User($email); - - $this->entityManager->persist($user); - } - - $this->syncFromCrm($user, $crmAttributes); + $user = $userRepository->findOneBy(['email' => $email]) ?? new User($email); $user ->setPassword($encryptedPassword) @@ -124,6 +118,13 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent ->setProfileComplete($this->completenessChecker->isComplete($personalData)) ; + $this->syncFromCrm($user, $crmAttributes); + + // Registered only once it is fully populated: syncFromCrm() logs on a channel that writes + // to the database, and an account already managed at that point would be flushed + // half-built — which is how a NULL password used to reach the user table. A no-op for an + // account that came from the repository. + $this->entityManager->persist($user); $this->entityManager->flush(); return $user; diff --git a/tests/Logger/DatabaseHandlerTest.php b/tests/Logger/DatabaseHandlerTest.php new file mode 100644 index 0000000..d97800c --- /dev/null +++ b/tests/Logger/DatabaseHandlerTest.php @@ -0,0 +1,153 @@ +getParameters(); + + foreach ($parameters as $parameter) { + $type = $parameter->getType(); + + self::assertInstanceOf(\ReflectionNamedType::class, $type); + self::assertNotSame(ObjectManager::class, $type->getName()); + self::assertFalse( + is_a($type->getName(), ObjectManager::class, true), + 'the handler must not be able to flush anybody else’s entities', + ); + } + } + + public function testTheWrittenColumnsMatchTheLogEntryMapping(): void + { + $handler = new DatabaseHandler($this->connection($table, $data, $types)); + + $handler->handle($this->record('auth', 'Login', ['email' => 'teamer@example.org'])); + + $metadata = $this->logEntryMetadata(); + + self::assertSame($metadata->getTableName(), $table); + self::assertSame([], array_diff(array_keys($data), $metadata->getColumnNames())); + + // Everything the table requires has to be supplied: no default fills these in, and the id + // is the only column the database generates itself. + $required = array_diff($metadata->getColumnNames(), ['id']); + foreach ($required as $column) { + self::assertArrayHasKey($column, $data); + } + + self::assertSame([], array_diff(array_keys($types), array_keys($data))); + } + + public function testTheRecordIsWrittenAsIs(): void + { + $handler = new DatabaseHandler($this->connection($table, $data, $types)); + + $handler->handle($this->record('bpn', 'Request failed', ['request_id' => 'r-1'], ['error_code' => '853'])); + + self::assertSame('bpn', $data['channel']); + self::assertSame('Request failed', $data['message']); + self::assertSame(['request_id' => 'r-1'], $data['context']); + self::assertSame(['error_code' => '853'], $data['extra']); + self::assertSame('853', $data['error_code']); + self::assertInstanceOf(\DateTimeImmutable::class, $data['created_at']); + } + + public function testPlaceholdersAreReplacedFromTheContext(): void + { + $handler = new DatabaseHandler($this->connection($table, $data, $types)); + + $handler->handle($this->record('core', 'Booking {id} confirmed', ['id' => '4711'])); + + self::assertSame('Booking 4711 confirmed', $data['message']); + } + + public function testAnErrorCodeIsOptional(): void + { + $handler = new DatabaseHandler($this->connection($table, $data, $types)); + + $handler->handle($this->record('core', 'Login')); + + self::assertNull($data['error_code']); + } + + public function testDebugRecordsAreNotWritten(): void + { + $connection = $this->createMock(Connection::class); + $connection->expects(self::never())->method('insert'); + + $handler = new DatabaseHandler($connection); + + $handler->handle($this->record('core', 'Noise', level: Level::Debug)); + } + + /** + * @param array|null $data + * @param array|null $types + */ + private function connection(?string &$table, ?array &$data, ?array &$types): Connection + { + $connection = $this->createMock(Connection::class); + $connection + ->method('insert') + ->willReturnCallback( + static function (string $insertTable, array $insertData, array $insertTypes) use (&$table, &$data, &$types): int { + $table = $insertTable; + $data = $insertData; + $types = $insertTypes; + + return 1; + } + ) + ; + + return $connection; + } + + /** + * @param array $context + * @param array $extra + */ + private function record(string $channel, string $message, array $context = [], array $extra = [], Level $level = Level::Info): LogRecord + { + return new LogRecord(new \DateTimeImmutable(), $channel, $level, $message, $context, $extra); + } + + /** + * @return ClassMetadata + */ + private function logEntryMetadata(): ClassMetadata + { + /** @var ClassMetadata $metadata */ + $metadata = new ClassMetadata(LogEntry::class, new UnderscoreNamingStrategy(CASE_LOWER, true)); + $metadata->initializeReflection(new RuntimeReflectionService()); + + (new AttributeDriver([dirname(__DIR__, 2).'/src/Entity']))->loadMetadataForClass(LogEntry::class, $metadata); + + return $metadata; + } +} diff --git a/tests/Security/BpnAuthenticatorTest.php b/tests/Security/BpnAuthenticatorTest.php index 961c938..4ce20c7 100644 --- a/tests/Security/BpnAuthenticatorTest.php +++ b/tests/Security/BpnAuthenticatorTest.php @@ -56,7 +56,7 @@ class BpnAuthenticatorTest extends TestCase $user = $this->loadUser($authenticator); - self::assertNull($persisted, 'an existing account must not be persisted again'); + self::assertSame($existing, $persisted, 'a login must not create a second account'); self::assertSame( ['ROLE_USER', Role::TEAMER, Role::pending(Role::GROUPS_ADMIN)], $user->getRoles(), @@ -159,6 +159,27 @@ class BpnAuthenticatorTest extends TestCase self::assertSame(['ROLE_USER', Role::CUSTOMER], $this->loadUser($authenticator)->getRoles()); } + public function testANewAccountIsOnlyRegisteredOnceItCarriesItsPassword(): void + { + $persisted = null; + $persistedPassword = null; + + // A degraded payload is what makes syncFromCrm() log, and that log write reaches the + // database. An account registered before the profile is complete would be written out + // half-built, and the user table rejects it: password is NOT NULL. + $authenticator = $this->authenticator( + $this->crmAttributes([], [], selectionGroups: []), + null, + $persisted, + $persistedPassword, + ); + + $this->loadUser($authenticator); + + self::assertInstanceOf(User::class, $persisted); + self::assertSame('encrypted', $persistedPassword); + } + /** * @param string[] $roles * @param string[] $hotelCodes @@ -175,8 +196,12 @@ class BpnAuthenticatorTest extends TestCase return $attributes; } - private function authenticator(CrmAttributes $crmAttributes, ?User $existing, ?User &$persisted): BpnAuthenticator - { + private function authenticator( + CrmAttributes $crmAttributes, + ?User $existing, + ?User &$persisted, + ?string &$persistedPassword = null, + ): BpnAuthenticator { $personalData = new PersonalData(); $personalData->personId = 42; $personalData->addressId = 4711; @@ -192,8 +217,11 @@ class BpnAuthenticatorTest extends TestCase $entityManager->method('getRepository')->willReturn($repository); $entityManager ->method('persist') - ->willReturnCallback(static function (object $entity) use (&$persisted): void { + ->willReturnCallback(static function (object $entity) use (&$persisted, &$persistedPassword): void { $persisted = $entity; + // Snapshot rather than a reference: what matters is what the account looked like + // at the moment it was registered, not what it grew into afterwards. + $persistedPassword = $entity instanceof User ? $entity->getPassword() : null; }) ;