Backport #4474 email confirmation lifecycle

Bind confirmation tokens to the current email hash with 24h expiry;
consume/revoke atomically; revoke outstanding confirmations on admin
email edits and profile email changes (transactional, with stale-state
guard). Adds EmailConfirmationTest; drops superseded mock tests.
This commit is contained in:
Adam Daley committed 2026-10-23 01:36:44 +01:00
1 parent 9058e45540
commit 3eda3bcd3c
8 files changed
+265 -72

No files matched your search

+4
View File
@@ -402,6 +402,10 @@ class Admin extends \FOSSBilling\Api\AbstractApi
'custom_20' => 'setCustom20',
];
if (isset($data['email']) && $client->getEmail() !== trim((string) $data['email'])) {
$service->revokeEmailConfirmations((int) $client->getId());
}
foreach ($simpleFields as $field => $setter) {
if (array_key_exists($field, $data)) {
$client->{$setter}($data[$field]);
+49 -16
View File
@@ -20,6 +20,7 @@ use Box\Mod\Client\Repository\ClientGroupRepository;
use Box\Mod\Client\Repository\ClientPasswordResetRepository;
use Box\Mod\Client\Repository\ClientRepository;
use Box\Mod\Staff\Entity\Admin;
use FOSSBilling\Doctrine\RowLock;
use FOSSBilling\i18n;
use FOSSBilling\InformationException;
use FOSSBilling\InjectionAwareInterface;
@@ -156,28 +157,60 @@ class Service implements InjectionAwareInterface
public function approveClientEmailByHash($hash): bool
{
$dbal = $this->di['dbal'];
$result = $dbal->fetchAssociative('SELECT id, client_id FROM extension_meta WHERE extension = "mod_client" AND meta_key = "confirm_email" AND meta_value = :hash', ['hash' => $hash]);
if (!$result) {
throw new InformationException('Invalid email confirmation link');
}
$dbal->executeStatement('UPDATE client SET email_approved = 1 WHERE id = :id', ['id' => $result['client_id']]);
$dbal->executeStatement('DELETE FROM extension_meta WHERE id = :id', ['id' => $result['id']]);
return true;
return $dbal->transactional(function () use ($dbal, $hash): bool {
$result = $dbal->fetchAssociative(
'SELECT id, client_id, rel_id FROM extension_meta WHERE extension = :extension AND meta_key = :key AND rel_type = :binding AND meta_value = :hash AND created_at > :cutoff',
['extension' => 'mod_client', 'key' => 'confirm_email', 'binding' => 'email_confirmation_v1', 'hash' => $hash, 'cutoff' => date('Y-m-d H:i:s', time() - 86400)],
);
if (!$result) {
throw new InformationException('Invalid email confirmation link');
}
// Serialize redemption with client updates on every supported database.
// Do not rely on the affected count: MySQL may report zero for a no-op.
$dbal->executeStatement('UPDATE client SET email_approved = email_approved WHERE id = :id', ['id' => $result['client_id']]);
$email = $dbal->fetchOne('SELECT email FROM client WHERE id = :id' . RowLock::suffix($dbal), ['id' => $result['client_id']]);
if (!is_string($email) || !hash_equals(hash('sha256', $email), (string) $result['rel_id'])) {
throw new InformationException('Invalid email confirmation link');
}
// The row may have been consumed or revoked while waiting for the client lock.
if ($dbal->executeStatement('DELETE FROM extension_meta WHERE id = :id AND created_at > :cutoff', ['id' => $result['id'], 'cutoff' => date('Y-m-d H:i:s', time() - 86400)]) !== 1) {
throw new InformationException('Invalid email confirmation link');
}
$dbal->executeStatement('UPDATE client SET email_approved = true WHERE id = :id', ['id' => $result['client_id']]);
$this->revokeEmailConfirmations((int) $result['client_id']);
return true;
});
}
public function revokeEmailConfirmations(int $clientId): void
{
$this->di['dbal']->delete('extension_meta', ['extension' => 'mod_client', 'client_id' => $clientId, 'meta_key' => 'confirm_email']);
}
public function generateEmailConfirmationLink($client_id)
{
// The managed client also reflects a profile email change before its flush.
$client = $this->clientRepository->find((int) $client_id) ?? throw new InformationException('Client not found');
$hash = strtolower((string) $this->di['tools']->generatePassword(50));
$this->di['dbal']->insert('extension_meta', [
'extension' => 'mod_client',
'client_id' => $client_id,
'meta_key' => 'confirm_email',
'meta_value' => $hash,
'created_at' => date('Y-m-d H:i:s'),
'updated_at' => date('Y-m-d H:i:s'),
]);
$dbal = $this->di['dbal'];
$dbal->transactional(function () use ($dbal, $client, $client_id, $hash): void {
$dbal->executeStatement('UPDATE client SET email_approved = email_approved WHERE id = :id', ['id' => $client_id]);
$this->revokeEmailConfirmations((int) $client_id);
$dbal->insert('extension_meta', [
'extension' => 'mod_client',
'client_id' => $client_id,
'meta_key' => 'confirm_email',
'meta_value' => $hash,
'rel_type' => 'email_confirmation_v1',
'rel_id' => hash('sha256', (string) $client->getEmail()),
'created_at' => date('Y-m-d H:i:s'),
'updated_at' => date('Y-m-d H:i:s'),
]);
});
return $this->di['tools']->url('/client/confirm-email/' . $hash);
}
@@ -272,6 +272,7 @@ test('update returns true', function (): void {
$serviceMock = Mockery::mock(Box\Mod\Client\Service::class);
$serviceMock->shouldReceive('emailAlreadyRegistered')->atLeast()->once()->andReturn(false);
$serviceMock->shouldReceive('revokeEmailConfirmations')->once()->with(1);
$serviceMock->shouldReceive('canChangeCurrency')->atLeast()->once()->andReturn(true);
$eventMock = Mockery::mock('\Box_EventManager');
@@ -447,7 +448,7 @@ test('changePassword returns true', function (): void {
$eventMock->shouldReceive('fire')->atLeast()->once();
$passwordMock = Mockery::mock(FOSSBilling\PasswordManager::class);
$passwordMock->shouldReceive('hashIt')->atLeast()->once()->with($data['password']);
$passwordMock->shouldReceive('hashIt')->atLeast()->once()->with($data['password'])->andReturn('new-password-hash');
$profileService = Mockery::mock(Box\Mod\Profile\Service::class);
$profileService->shouldReceive('invalidateSessions')->atLeast()->once();
@@ -462,6 +463,7 @@ test('changePassword returns true', function (): void {
$adminClient->setDi($di);
$di['em']->getRepository(Box\Mod\Client\Entity\ClientPasswordReset::class)->shouldReceive('changePassword')->once()->with(Mockery::type(Box\Mod\Client\Entity\Client::class), 'new-password-hash');
$result = $adminClient->change_password($data);
expect($result)->toBeTrue();
});
@@ -514,7 +514,7 @@ test('updatePassword returns true', function (): void {
$passwordResetRepository = Mockery::mock(Box\Mod\Client\Repository\ClientPasswordResetRepository::class);
$passwordResetRepository->shouldReceive('findOneByHash')->atLeast()->once()->andReturn($passwordReset);
$passwordResetRepository->shouldReceive('changePassword')->atLeast()->once();
$passwordResetRepository->shouldReceive('changePassword')->once()->with(Mockery::type(Box\Mod\Client\Entity\Client::class), 'new-password-hash', $data['hash']);
$client = new Box\Mod\Client\Entity\Client();
$rp = new ReflectionProperty($client, 'id');
@@ -536,7 +536,7 @@ test('updatePassword returns true', function (): void {
$eventMock->shouldReceive('fire')->times(2);
$passwordMock = Mockery::mock(FOSSBilling\PasswordManager::class);
$passwordMock->shouldReceive('hashIt')->atLeast()->once();
$passwordMock->shouldReceive('hashIt')->atLeast()->once()->andReturn('new-password-hash');
$emailServiceMock = Mockery::mock(Box\Mod\Email\Service::class);
$emailServiceMock->shouldReceive('sendTemplate')->atLeast()->once();
@@ -0,0 +1,184 @@
<?php
/**
* Copyright 2022-2026 FOSSBilling
* SPDX-License-Identifier: Apache-2.0.
*
* @copyright FOSSBilling (https://www.fossbilling.org)
* @license http://www.apache.org/licenses/LICENSE-2.0 Apache-2.0
*/
declare(strict_types=1);
use Box\Mod\Client\Entity\Client;
use Box\Mod\Client\Service;
use Doctrine\DBAL\DriverManager;
use FOSSBilling\InformationException;
use FOSSBilling\Tools;
use function Tests\Helpers\container;
use function Tests\Helpers\createEntity;
beforeEach(function (): void {
$this->connection = DriverManager::getConnection(['driver' => 'pdo_sqlite', 'memory' => true]);
$this->connection->executeStatement('CREATE TABLE client (id INTEGER PRIMARY KEY, email TEXT, email_approved BOOLEAN DEFAULT false)');
$this->connection->executeStatement('CREATE TABLE extension_meta (id INTEGER PRIMARY KEY AUTOINCREMENT, client_id INTEGER, extension TEXT, meta_key TEXT, meta_value TEXT, rel_type TEXT, rel_id TEXT, created_at TEXT, updated_at TEXT)');
$this->client = createEntity(Client::class, ['email' => 'owner@example.com']);
$this->connection->insert('client', ['id' => 1, 'email' => $this->client->getEmail(), 'email_approved' => 0]);
$this->di = container();
$this->di['dbal'] = $this->connection;
$this->di['em']->getRepository(Client::class)->shouldReceive('find')->with(1)->andReturn($this->client);
$tools = Mockery::mock(Tools::class);
$tools->shouldReceive('generatePassword')->andReturnUsing(fn (): string => bin2hex(random_bytes(25)));
$tools->shouldReceive('url')->andReturnUsing(fn (string $path): string => $path);
$this->di['tools'] = $tools;
$this->service = new Service();
$this->service->setDi($this->di);
$this->issue = fn (): string => basename($this->service->generateEmailConfirmationLink(1));
});
test('confirmation approves the issued address and cannot be replayed', function (): void {
$hash = ($this->issue)();
expect($this->service->approveClientEmailByHash($hash))->toBeTrue();
expect((bool) $this->connection->fetchOne('SELECT email_approved FROM client WHERE id = 1'))->toBeTrue();
expect(fn () => $this->service->approveClientEmailByHash($hash))->toThrow(InformationException::class, 'Invalid email confirmation link');
});
test('an old confirmation cannot approve a replacement address', function (): void {
$hash = ($this->issue)();
$this->connection->update('client', ['email' => 'victim@example.com'], ['id' => 1]);
expect(fn () => $this->service->approveClientEmailByHash($hash))->toThrow(InformationException::class);
expect((bool) $this->connection->fetchOne('SELECT email_approved FROM client WHERE id = 1'))->toBeFalse();
expect($this->connection->getTransactionNestingLevel())->toBe(0);
});
test('confirmation rejects unknown legacy expired and malformed bindings', function (string $kind): void {
$hash = ($this->issue)();
$changes = match ($kind) {
'unknown' => ['meta_value' => 'another-token'],
'legacy' => ['rel_type' => null, 'rel_id' => null],
'expired' => ['created_at' => date('Y-m-d H:i:s', time() - 86401)],
'malformed' => ['rel_id' => 'invalid-binding'],
};
$this->connection->update('extension_meta', $changes, ['client_id' => 1]);
expect(fn () => $this->service->approveClientEmailByHash($hash))->toThrow(InformationException::class);
expect((bool) $this->connection->fetchOne('SELECT email_approved FROM client WHERE id = 1'))->toBeFalse();
})->with(['unknown', 'legacy', 'expired', 'malformed']);
test('resending replaces older links and retains a valid current link', function (): void {
$old = ($this->issue)();
$new = ($this->issue)();
expect(fn () => $this->service->approveClientEmailByHash($old))->toThrow(InformationException::class);
expect($this->service->approveClientEmailByHash($new))->toBeTrue();
});
test('issuance binds to the managed email before a profile flush', function (): void {
$old = ($this->issue)();
$this->client->setEmail('new@example.com');
$new = ($this->issue)();
expect(fn () => $this->service->approveClientEmailByHash($new))->toThrow(InformationException::class);
$this->connection->update('client', ['email' => $this->client->getEmail()], ['id' => 1]);
expect(fn () => $this->service->approveClientEmailByHash($old))->toThrow(InformationException::class);
expect($this->service->approveClientEmailByHash($new))->toBeTrue();
});
test('revocation prevents restoring an old address from reviving its link', function (): void {
$hash = ($this->issue)();
$this->service->revokeEmailConfirmations(1);
$this->connection->update('client', ['email' => 'new@example.com'], ['id' => 1]);
$this->connection->update('client', ['email' => 'owner@example.com'], ['id' => 1]);
expect(fn () => $this->service->approveClientEmailByHash($hash))->toThrow(InformationException::class);
});
test('approval consumes sibling tokens and leaves other clients and metadata intact', function (): void {
$hash = ($this->issue)();
$row = $this->connection->fetchAssociative('SELECT * FROM extension_meta');
unset($row['id']);
$row['meta_value'] = 'sibling-token';
$this->connection->insert('extension_meta', $row);
$row['client_id'] = 2;
$this->connection->insert('extension_meta', $row);
$row['client_id'] = 1;
$row['meta_key'] = 'unrelated';
$this->connection->insert('extension_meta', $row);
expect($this->service->approveClientEmailByHash($hash))->toBeTrue();
expect(fn () => $this->service->approveClientEmailByHash('sibling-token'))->toThrow(InformationException::class);
expect((int) $this->connection->fetchOne('SELECT COUNT(*) FROM extension_meta'))->toBe(2);
});
test('confirmation does not accept database collation aliases for the bound email', function (): void {
$hash = ($this->issue)();
$this->connection->update('client', ['email' => 'OWNER@example.com'], ['id' => 1]);
expect(fn () => $this->service->approveClientEmailByHash($hash))->toThrow(InformationException::class);
});
test('profile email changes reset a concurrently approved database row despite stale managed state', function (bool $confirmationRequired): void {
$connection = $this->connection;
$connection->executeStatement('DROP TABLE client');
$config = Doctrine\ORM\ORMSetup::createAttributeMetadataConfig([dirname(__DIR__, 2) . '/Entity'], true);
$config->setProxyDir(sys_get_temp_dir());
$config->setProxyNamespace('FOSSBilling\\Tests\\DoctrineProxies');
$em = new Doctrine\ORM\EntityManager($connection, $config);
// The legacy MySQL ENUM column definition cannot be created on SQLite.
$clientMetadata = $em->getClassMetadata(Client::class);
$clientMetadata->fieldMappings['gender']['columnDefinition'] = null;
$clientMetadata->fieldMappings['gender']['type'] = 'string';
// Create only the tables this flow touches. The full metadata set cannot
// be built on SQLite: legacy duplicate index names (e.g. two
// `client_id_idx` indexes) collide once index scoping stops being per-table.
(new Doctrine\ORM\Tools\SchemaTool($em))->createSchema([
$clientMetadata,
$em->getClassMetadata(Box\Mod\Client\Entity\ClientPasswordReset::class),
]);
$client = new Client();
$client->setEmail('owner@example.com');
$client->setEmailApproved(false);
$em->persist($client);
$em->flush();
$di = container();
$di['em'] = $em;
$di['dbal'] = $connection;
$tools = Mockery::mock(Tools::class);
$tools->shouldReceive('generatePassword')->andReturnUsing(fn (): string => bin2hex(random_bytes(25)));
$tools->shouldReceive('url')->andReturnUsing(fn (string $path): string => $path);
$tools->shouldReceive('validateAndSanitizeEmail')->andReturn('victim@example.com');
$di['tools'] = $tools;
$module = Mockery::mock(FOSSBilling\Module::class);
$module->shouldReceive('getConfig')->andReturn(['disable_change_email' => false]);
$di['mod'] = $di->protect(fn () => $module);
$di['mod_config'] = $di->protect(fn () => ['require_email_confirmation' => $confirmationRequired]);
$service = new Service();
$service->setDi($di);
$emailService = Mockery::mock(Box\Mod\Email\Service::class);
$newHash = null;
if ($confirmationRequired) {
$emailService->shouldReceive('sendTemplate')->once()->andReturnUsing(function (array $data) use (&$newHash, $connection): void {
expect($connection->isTransactionActive())->toBeTrue();
$newHash = basename($data['email_confirmation_link']);
});
}
$di['mod_service'] = $di->protect(fn (string $name) => $name === 'client' ? $service : $emailService);
$oldHash = basename($service->generateEmailConfirmationLink($client->getId()));
// A confirmation in another request changed only the database flag.
$connection->executeStatement('UPDATE client SET email_approved = true WHERE id = :id', ['id' => $client->getId()]);
expect($client->getEmailApproved())->toBeFalse();
$profile = new Box\Mod\Profile\Service();
$profile->setDi($di);
expect($profile->updateClient($client, ['email' => 'victim@example.com']))->toBeTrue();
expect($connection->fetchOne('SELECT email FROM client WHERE id = :id', ['id' => $client->getId()]))->toBe('victim@example.com');
expect((bool) $connection->fetchOne('SELECT email_approved FROM client WHERE id = :id', ['id' => $client->getId()]))->toBeFalse();
expect(fn () => $service->approveClientEmailByHash($oldHash))->toThrow(InformationException::class);
if ($confirmationRequired) {
expect($service->approveClientEmailByHash($newHash))->toBeTrue();
}
})->with([false, true]);
test('failed approval rolls back token consumption', function (): void {
$hash = ($this->issue)();
$this->connection->executeStatement("CREATE TRIGGER reject_approval BEFORE UPDATE OF email_approved ON client WHEN NEW.email_approved = true BEGIN SELECT RAISE(ABORT, 'approval failed'); END");
expect(fn () => $this->service->approveClientEmailByHash($hash))->toThrow(Doctrine\DBAL\Exception\DriverException::class);
expect((int) $this->connection->fetchOne('SELECT COUNT(*) FROM extension_meta'))->toBe(1);
expect((bool) $this->connection->fetchOne('SELECT email_approved FROM client WHERE id = 1'))->toBeFalse();
$this->connection->executeStatement('DROP TRIGGER reject_approval');
expect($this->service->approveClientEmailByHash($hash))->toBeTrue();
});
@@ -22,57 +22,6 @@ test('getDi returns dependency injection container', function (): void {
expect($getDi)->toEqual($di);
});
test('approveClientEmailByHash returns true', function (): void {
$service = new Box\Mod\Client\Service();
$dbal = Mockery::mock(Doctrine\DBAL\Connection::class);
$dbal->shouldReceive('fetchAssociative')->atLeast()->once()->andReturn(['client_id' => 2, 'id' => 1]);
$dbal->shouldReceive('executeStatement')->atLeast()->once()->andReturn(1);
$di = container();
$di['dbal'] = $dbal;
$service->setDi($di);
$result = $service->approveClientEmailByHash('');
expect($result)->toBeTrue();
});
test('approveClientEmailByHash throws exception for invalid hash', function (): void {
$service = new Box\Mod\Client\Service();
$di = container();
$service->setDi($di);
$service->approveClientEmailByHash('');
})->throws(FOSSBilling\Exception::class, 'Invalid email confirmation link');
test('generateEmailConfirmationLink returns string', function (): void {
$service = new Box\Mod\Client\Service();
$model = createEntity(Box\Mod\Extension\Entity\ExtensionMeta::class);
$toolsMock = Mockery::mock(FOSSBilling\Tools::class);
$toolsMock->shouldReceive('url')
->atLeast()->once()
->andReturn('fossbilling.org/index.php/client/confirm-email/');
$toolsMock->shouldReceive('generatePassword')
->atLeast()->once()
->andReturn('randomhash123456789012345678901234567890');
$di = container();
$di['tools'] = $toolsMock;
$service->setDi($di);
$clientId = 1;
$result = $service->generateEmailConfirmationLink($clientId);
expect($result)->toBeString();
expect(str_contains((string) $result, '/client/confirm-email/'))->toBeTrue();
});
test('onAfterClientSignUp returns true', function (): void {
$service = new Box\Mod\Client\Service();
$eventParams = [
+10
View File
@@ -121,6 +121,12 @@ class Service implements InjectionAwareInterface
public function updateClient(Client $client, array $data = []): bool
{
return $this->di['dbal']->transactional(fn (): bool => $this->updateClientProfile($client, $data));
}
private function updateClientProfile(Client $client, array $data): bool
{
$clientId = (int) $client->getId();
$event_params = $data;
$event_params['id'] = $client->getId();
$this->di['events_manager']->fire(['event' => 'onBeforeClientProfileUpdate', 'params' => $event_params]);
@@ -145,6 +151,10 @@ class Service implements InjectionAwareInterface
}
if ($client->getEmail() !== $email) {
// Lock the row and reset the stored flag even if the managed entity
// still has its original false value after a concurrent confirmation.
$this->di['dbal']->executeStatement('UPDATE client SET email_approved = false WHERE id = :id', ['id' => $clientId]);
$clientService->revokeEmailConfirmations($clientId);
$client->setEmail($email);
$client->setEmailApproved(false);
+13 -2
View File
@@ -87,7 +87,7 @@ test('changes admin password', function (): void {
$passwordMock = Mockery::mock(FOSSBilling\PasswordManager::class);
$passwordMock->shouldReceive('hashIt')
->with($password);
->with($password)->andReturn('new-hash');
$di = container();
$di['logger'] = new Tests\Helpers\TestLogger();
@@ -96,6 +96,10 @@ test('changes admin password', function (): void {
$model = createEntity(Box\Mod\Staff\Entity\Admin::class);
$repository = Mockery::mock(Box\Mod\Staff\Repository\AdminPasswordResetRepository::class);
$repository->shouldReceive('changePassword')->once()->with($model, 'new-hash');
$di['em']->shouldReceive('getRepository')->with(Box\Mod\Staff\Entity\AdminPasswordReset::class)->andReturn($repository);
$service = new Service();
$service->setDi($di);
@@ -123,7 +127,11 @@ test('updates client', function (): void {
$clientServiceMock->shouldReceive('emailAlreadyRegistered')
->andReturn(false);
$clientServiceMock->shouldReceive('revokeEmailConfirmations')->once()->with(0);
$di = container();
$di['dbal']->shouldReceive('transactional')->once()->andReturnUsing(fn (callable $operation): mixed => $operation());
$di['dbal']->shouldReceive('executeStatement')->once()->with('UPDATE client SET email_approved = false WHERE id = :id', ['id' => 0])->andReturn(1);
$di['logger'] = new Tests\Helpers\TestLogger();
$di['events_manager'] = $emMock;
$di['mod_service'] = $di->protect(fn ($name): Mockery\MockInterface => $clientServiceMock);
@@ -206,6 +214,7 @@ test('throws exception when email change is not allowed', function (): void {
->andReturn(false);
$di = container();
$di['dbal']->shouldReceive('transactional')->once()->andReturnUsing(fn (callable $operation): mixed => $operation());
$di['logger'] = new Tests\Helpers\TestLogger();
$di['events_manager'] = $emMock;
$di['db'] = $dbMock;
@@ -250,6 +259,7 @@ test('throws exception when email already registered', function (): void {
->andReturn(true);
$di = container();
$di['dbal']->shouldReceive('transactional')->once()->andReturnUsing(fn (callable $operation): mixed => $operation());
$di['logger'] = new Tests\Helpers\TestLogger();
$di['events_manager'] = $emMock;
$di['db'] = $dbMock;
@@ -296,7 +306,7 @@ test('changes client password', function (): void {
$passwordMock = Mockery::mock(FOSSBilling\PasswordManager::class);
$passwordMock->shouldReceive('hashIt')
->with($password);
->with($password)->andReturn('new-password-hash');
$di = container();
$di['logger'] = new Tests\Helpers\TestLogger();
@@ -307,6 +317,7 @@ test('changes client password', function (): void {
$service = new Service();
$service->setDi($di);
$di['em']->getRepository(Box\Mod\Client\Entity\ClientPasswordReset::class)->shouldReceive('changePassword')->once()->with($model, 'new-password-hash');
$result = $service->changeClientPassword($model, $password);
expect($result)->toBeTrue();
});