From 1201435a99b20a3306b25f98b21e66fade35a180 Mon Sep 17 00:00:00 2001 From: blaipr Date: Tue, 18 Aug 2026 23:58:51 +0200 Subject: [PATCH] fix: mail a group's member once, however many groups they are in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `getUserEmailForGroup()` addresses the mail carrying a newly issued temporary master password. A user belongs to the group either by their own `userGroupId` or through `UserToUserGroup`, and the join carries one row per membership they hold — so `User.userGroupId = :id`, true on every one of those rows, returned somebody in the group directly once for each *other* group they were in. `sendByEmailForGroup()` mails what it is handed. A user whose own group is the one being mailed and who belongs to two others came back as two rows for one person, and received the master password twice. The existing repository test checked the bind values and nothing else, so it passed either way. The new one runs against a real database and asserts both directions: the member of several groups is addressed once, and members by either route are still addressed at all — a fix that dropped people would satisfy the first assertion on its own. --- .../Adapter/Out/User/Repositories/User.php | 6 + .../Application/User/GroupRecipientsTest.php | 258 ++++++++++++++++++ 2 files changed, 264 insertions(+) create mode 100644 tests/Integration/Application/User/GroupRecipientsTest.php diff --git a/src/Infrastructure/Adapter/Out/User/Repositories/User.php b/src/Infrastructure/Adapter/Out/User/Repositories/User.php index 6f7683250..12599d979 100644 --- a/src/Infrastructure/Adapter/Out/User/Repositories/User.php +++ b/src/Infrastructure/Adapter/Out/User/Repositories/User.php @@ -515,6 +515,12 @@ public function getUserEmailForGroup(int $groupId): QueryResult { $query = $this->queryFactory ->newSelect() + // A user belongs to the group by their own `userGroupId` or through UserToUserGroup, and + // the join carries one row per membership they hold — so `User.userGroupId = :id`, being + // true on every one of those rows, returned somebody in the group directly once for each + // *other* group they were in. The caller mails what it is handed, so they received the + // temporary master password once per extra membership. + ->distinct() ->cols(UserModel::getColsWithPreffix(UserModel::TABLE)) ->from(UserModel::TABLE) ->innerJoin( diff --git a/tests/Integration/Application/User/GroupRecipientsTest.php b/tests/Integration/Application/User/GroupRecipientsTest.php new file mode 100644 index 000000000..423efcf83 --- /dev/null +++ b/tests/Integration/Application/User/GroupRecipientsTest.php @@ -0,0 +1,258 @@ +. + */ + +namespace SP\Tests\Integration\Application\User; + +use DI\ContainerBuilder; +use PDO; +use PHPUnit\Framework\Attributes\Group; +use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\TestCase; +use Psr\Container\ContainerInterface; +use SP\Application\User\Ports\UserService; +use SP\Domain\Core\Bootstrap\Path; +use SP\Domain\Database\Ports\DbStorageHandler; +use SP\Domain\File\FileSystem; +use SP\Infrastructure\Definitions\CoreDefinitions; +use SP\Infrastructure\Definitions\DomainDefinitions; +use SP\Tests\Support\DatabaseTrait; + +use function SP\Tests\getDbHandler; +use function SP\Tests\getResource; + +/** + * Everybody in a group is one recipient, however many ways they are in it. + * + * `getUserEmailForGroup()` is what addresses the mail carrying a newly issued temporary master + * password. A user belongs to the group either by their own `userGroupId` or through + * `UserToUserGroup`, and the join carries one row per membership they hold — so + * `User.userGroupId = :id`, true on every one of those rows, returned somebody in the group + * directly once for each *other* group they were in. `sendByEmailForGroup()` mails what it is + * handed, so that user received the master password twice, or five times, depending on how many + * groups they happened to belong to. + * + * Against a real database, because the defect is in what the join returns and a mocked repository + * returns whatever the test says. + */ +#[Group('integration')] +final class GroupRecipientsTest extends TestCase +{ + use DatabaseTrait; + + private const TARGET_GROUP = 2; + + private string $root; + private string $configPath; + private PDO $pdo; + + protected function setUp(): void + { + parent::setUp(); + + self::loadFixtures(); + + $this->root = FileSystem::buildPath( + sys_get_temp_dir(), + 'syspass-group-recipients-' . bin2hex(random_bytes(6)) + ); + $this->configPath = FileSystem::buildPath($this->root, 'config'); + + foreach ([$this->configPath, $this->cachePath(), $this->tmpPath(), $this->backupPath()] as $dir) { + if (!mkdir($dir, 0777, true) && !is_dir($dir)) { + self::fail(sprintf('Directory "%s" was not created', $dir)); + } + } + + file_put_contents( + FileSystem::buildPath($this->configPath, 'config.xml'), + getResource('config', 'config.xml') + ); + + $this->pdo = getDbHandler()->getConnection(); + } + + protected function tearDown(): void + { + FileSystem::rmdirRecursive($this->root); + + parent::tearDown(); + } + + /** + * Somebody whose own group is the one being mailed, who is also in others, is addressed once. + */ + #[Test] + public function aMemberOfSeveralGroupsIsOneRecipient(): void + { + $login = 'recip_' . bin2hex(random_bytes(4)); + $userId = $this->givenAUserInTheTargetGroup($login); + + // ...and in two more, which is what used to multiply them. + foreach ($this->twoOtherGroupIds() as $groupId) { + $this->alsoAMemberOf($userId, $groupId); + } + + $logins = $this->recipientLoginsFor(self::TARGET_GROUP); + + self::assertContains($login, $logins, 'setup: the user must be a recipient at all'); + self::assertSame( + 1, + count(array_keys($logins, $login, true)), + 'a recipient is a person, not a membership — the master password is mailed to them once' + ); + } + + /** + * And the list is still everybody, not just the first of them. + */ + #[Test] + public function everyMemberOfTheGroupIsARecipient(): void + { + $first = 'recipa_' . bin2hex(random_bytes(4)); + $second = 'recipb_' . bin2hex(random_bytes(4)); + + $this->givenAUserInTheTargetGroup($first); + $this->alsoAMemberOf($this->givenAUserInGroup($second, $this->twoOtherGroupIds()[0]), self::TARGET_GROUP); + + $logins = $this->recipientLoginsFor(self::TARGET_GROUP); + + self::assertContains($first, $logins, 'a member by their own group'); + self::assertContains($second, $logins, 'a member through UserToUserGroup'); + } + + /** + * @return string[] + */ + private function recipientLoginsFor(int $groupId): array + { + return array_map( + static fn(object $user) => $user->getLogin(), + $this->buildContainer()->get(UserService::class)->getUserEmailForGroup($groupId) + ); + } + + private function givenAUserInTheTargetGroup(string $login): int + { + return $this->givenAUserInGroup($login, self::TARGET_GROUP); + } + + private function givenAUserInGroup(string $login, int $groupId): int + { + $statement = $this->pdo->prepare( + 'INSERT INTO `User` (`name`, `login`, `email`, `userGroupId`, `userProfileId`, `isDisabled`, `pass`, `hashSalt`) + VALUES (:name, :login, :email, :groupId, 1, 0, :pass, :hashSalt)' + ); + + $statement->execute( + [ + 'name' => 'Recipient Test User', + 'login' => $login, + 'email' => $login . '@example.invalid', + 'groupId' => $groupId, + 'pass' => '', + 'hashSalt' => '', + ] + ); + + return (int)$this->pdo->lastInsertId(); + } + + private function alsoAMemberOf(int $userId, int $groupId): void + { + $this->pdo + ->prepare('INSERT INTO `UserToUserGroup` (`userId`, `userGroupId`) VALUES (:userId, :groupId)') + ->execute(['userId' => $userId, 'groupId' => $groupId]); + } + + /** + * Two groups from the fixtures that are not the one being mailed. + * + * @return int[] + */ + private function twoOtherGroupIds(): array + { + $statement = $this->pdo->prepare( + 'SELECT `id` FROM `UserGroup` WHERE `id` <> :target ORDER BY `id` LIMIT 2' + ); + $statement->execute(['target' => self::TARGET_GROUP]); + + $ids = array_map('intval', $statement->fetchAll(PDO::FETCH_COLUMN)); + + self::assertCount(2, $ids, 'the fixtures must provide two other groups to belong to'); + + return $ids; + } + + private function buildContainer(): ContainerInterface + { + $_ENV['CONFIG_PATH'] = $this->configPath; + + try { + $coreDefinitions = CoreDefinitions::getDefinitions(REAL_APP_ROOT, 'cli'); + } finally { + unset($_ENV['CONFIG_PATH']); + } + + $coreDefinitions['paths'] = array_map( + fn(array $path) => match ($path[0]) { + Path::CACHE => [Path::CACHE, $this->cachePath()], + Path::TMP => [Path::TMP, $this->tmpPath()], + Path::BACKUP => [Path::BACKUP, $this->backupPath()], + default => $path, + }, + $coreDefinitions['paths'] + ); + + $moduleDefinitions = FileSystem::require( + FileSystem::buildPath(REAL_APP_ROOT, 'src', 'Infrastructure', 'Adapter', 'In', 'Cli', 'module.php') + ); + + $builder = new ContainerBuilder(); + $builder->addDefinitions( + DomainDefinitions::getDefinitions(), + $coreDefinitions, + $moduleDefinitions, + [DbStorageHandler::class => getDbHandler()] + ); + + return $builder->build(); + } + + private function cachePath(): string + { + return FileSystem::buildPath($this->root, 'cache'); + } + + private function tmpPath(): string + { + return FileSystem::buildPath($this->root, 'tmp'); + } + + private function backupPath(): string + { + return FileSystem::buildPath($this->root, 'backup'); + } +}