Skip to content

Commit 44cd20f

Browse files
thorstenthorsten
committed
fix: avoid duplicate email registration
1 parent 909fe6f commit 44cd20f

File tree

6 files changed

+267
-5
lines changed

6 files changed

+267
-5
lines changed

‎phpmyfaq/src/phpMyFAQ/Helper/RegistrationHelper.php‎

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
use phpMyFAQ\Strings;
2424
use phpMyFAQ\Translation;
2525
use phpMyFAQ\User;
26+
use phpMyFAQ\User\UserData;
2627
use phpMyFAQ\Utils;
2728
use Symfony\Component\Mailer\Exception\TransportExceptionInterface;
2829

@@ -42,17 +43,31 @@ public function __construct(Configuration $configuration)
4243
}
4344

4445
/**
45-
* Creates a new user account, sends mail and returns success or
46+
* Creates a new user account and saves the user data.
47+
* If user generation was successful, account activation is sent via an email
4648
* error message as an array.
4749
* The password will be automatically generated and sent by email
48-
* as soon if admin switch user to "active"
50+
* as soon if admin switches user to "active"
4951
*
5052
* @throws Exception|TransportExceptionInterface
5153
*/
5254
public function createUser(string $userName, string $fullName, string $email, bool $isVisible): array
5355
{
5456
$user = new User($this->configuration);
5557

58+
// Check if email already exists in the userdata table (even if the username is different)
59+
if (!empty($email)) {
60+
if (!$user->userdata instanceof UserData) {
61+
$user->userdata = new UserData($this->configuration);
62+
}
63+
if ($user->userdata->emailExists($email)) {
64+
return [
65+
'registered' => false,
66+
'error' => User::ERROR_USER_EMAIL_NOT_UNIQUE
67+
];
68+
}
69+
}
70+
5671
if (!$user->createUser($userName, '')) {
5772
return [
5873
'registered' => false,
@@ -95,14 +110,14 @@ public function createUser(string $userName, string $fullName, string $email, bo
95110
}
96111

97112
/**
98-
* Returns true, if the hostname of the given email address is allowed.
113+
* Returns true if the hostname of the given email address is allowed.
99114
* otherwise false.
100115
*/
101116
public function isDomainAllowed(string $email): bool
102117
{
103118
$whitelistedDomains = $this->configuration->get('security.domainWhiteListForRegistrations');
104119

105-
if (Strings::strlen($whitelistedDomains) === 0) {
120+
if ($whitelistedDomains === null || Strings::strlen(trim((string) $whitelistedDomains)) === 0) {
106121
return true;
107122
}
108123

‎phpmyfaq/src/phpMyFAQ/User.php‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,8 @@ class User
6767

6868
final public const ERROR_USER_LOGIN_NOT_UNIQUE = 'The Login name already exists.';
6969

70+
final public const ERROR_USER_EMAIL_NOT_UNIQUE = 'The email address already exists.';
71+
7072
final public const ERROR_USER_LOGIN_INVALID = 'The chosen login is invalid. A valid login has at least four ' .
7173
'characters. Only letters, numbers and underscore _ are allowed. The first letter must be a letter. ';
7274

@@ -384,14 +386,24 @@ public function createUser(string $login, string $pass = '', string $domain = ''
384386
throw new Exception(self::ERROR_USER_LOGIN_NOT_UNIQUE);
385387
}
386388

389+
// If $login is an email address, check if it already exists in userdata table
390+
if ($this->isEmailAddress($login)) {
391+
if (!$this->userdata instanceof UserData) {
392+
$this->userdata = new UserData($this->configuration);
393+
}
394+
if ($this->userdata->emailExists($login)) {
395+
throw new Exception(self::ERROR_USER_EMAIL_NOT_UNIQUE);
396+
}
397+
}
398+
387399
// set user-ID
388400
if (0 === $userId) {
389401
$this->userId = $this->configuration->getDb()->nextId(Database::getTablePrefix() . 'faquser', 'user_id');
390402
} else {
391403
$this->userId = $userId;
392404
}
393405

394-
// create user entry
406+
// create a user entry
395407
$insert = sprintf(
396408
"INSERT INTO %sfaquser (user_id, login, session_timestamp, member_since) VALUES (%d, '%s', %d, '%s')",
397409
Database::getTablePrefix(),
@@ -1033,4 +1045,14 @@ public function getWebAuthnKeys(): string
10331045

10341046
return '';
10351047
}
1048+
1049+
/**
1050+
* Checks if a string is a valid email address.
1051+
*
1052+
* @param string $string String to check
1053+
*/
1054+
private function isEmailAddress(string $string): bool
1055+
{
1056+
return filter_var($string, FILTER_VALIDATE_EMAIL) !== false;
1057+
}
10361058
}

‎phpmyfaq/src/phpMyFAQ/User/UserData.php‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,4 +287,26 @@ public function delete(int $userId): bool
287287

288288
return true;
289289
}
290+
291+
/**
292+
* Checks if an email address already exists in the user data table.
293+
* Returns true if the email exists, false otherwise.
294+
*
295+
* @param string $email Email address to check
296+
*/
297+
public function emailExists(string $email): bool
298+
{
299+
if (empty($email)) {
300+
return false;
301+
}
302+
303+
$select = sprintf(
304+
"SELECT user_id FROM %sfaquserdata WHERE email = '%s'",
305+
Database::getTablePrefix(),
306+
$this->configuration->getDb()->escape($email)
307+
);
308+
309+
$res = $this->configuration->getDb()->query($select);
310+
return $this->configuration->getDb()->numRows($res) > 0;
311+
}
290312
}

‎tests/phpMyFAQ/Helper/RegistrationHelperTest.php‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
<?php
2+
3+
namespace phpMyFAQ\Helper;
4+
5+
use phpMyFAQ\Configuration;
6+
use PHPUnit\Framework\MockObject\MockObject;
7+
use PHPUnit\Framework\TestCase;
8+
9+
class RegistrationHelperTest extends TestCase
10+
{
11+
private Configuration|MockObject $configuration;
12+
private RegistrationHelper $registrationHelper;
13+
14+
protected function setUp(): void
15+
{
16+
$this->configuration = $this->createMock(Configuration::class);
17+
$this->registrationHelper = new RegistrationHelper($this->configuration);
18+
}
19+
20+
public function testIsDomainAllowedReturnsTrueWhenNoWhitelist(): void
21+
{
22+
$this->configuration->method('get')
23+
->with('security.domainWhiteListForRegistrations')
24+
->willReturn('');
25+
26+
$result = $this->registrationHelper->isDomainAllowed('test@example.com');
27+
$this->assertTrue($result);
28+
}
29+
30+
public function testIsDomainAllowedReturnsTrueWhenDomainInWhitelist(): void
31+
{
32+
$this->configuration->method('get')
33+
->with('security.domainWhiteListForRegistrations')
34+
->willReturn('example.com, test.org');
35+
36+
$result = $this->registrationHelper->isDomainAllowed('test@example.com');
37+
$this->assertTrue($result);
38+
}
39+
40+
public function testIsDomainAllowedReturnsFalseWhenDomainNotInWhitelist(): void
41+
{
42+
$this->configuration->method('get')
43+
->with('security.domainWhiteListForRegistrations')
44+
->willReturn('alloweddomain.com, anotherdomain.org');
45+
46+
$result = $this->registrationHelper->isDomainAllowed('test@forbiddendomain.com');
47+
$this->assertFalse($result);
48+
}
49+
50+
public function testIsDomainAllowedHandlesNullConfiguration(): void
51+
{
52+
$this->configuration->method('get')
53+
->with('security.domainWhiteListForRegistrations')
54+
->willReturn(null);
55+
56+
$result = $this->registrationHelper->isDomainAllowed('test@example.com');
57+
$this->assertTrue($result);
58+
}
59+
60+
public function testIsDomainAllowedHandlesEmptyDomainList(): void
61+
{
62+
$this->configuration->method('get')
63+
->with('security.domainWhiteListForRegistrations')
64+
->willReturn(' ');
65+
66+
$result = $this->registrationHelper->isDomainAllowed('test@example.com');
67+
$this->assertTrue($result);
68+
}
69+
}

‎tests/phpMyFAQ/User/UserDataTest.php‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,4 +92,30 @@ public function testDelete(): void
9292
$result = $this->userData->delete(1);
9393
$this->assertTrue($result);
9494
}
95+
96+
public function testEmailExistsReturnsTrueWhenEmailExists(): void
97+
{
98+
$this->database->method('escape')->willReturn('test@example.com');
99+
$this->database->method('query')->willReturn(true);
100+
$this->database->method('numRows')->willReturn(1);
101+
102+
$result = $this->userData->emailExists('test@example.com');
103+
$this->assertTrue($result);
104+
}
105+
106+
public function testEmailExistsReturnsFalseWhenEmailDoesNotExist(): void
107+
{
108+
$this->database->method('escape')->willReturn('nonexistent@example.com');
109+
$this->database->method('query')->willReturn(true);
110+
$this->database->method('numRows')->willReturn(0);
111+
112+
$result = $this->userData->emailExists('nonexistent@example.com');
113+
$this->assertFalse($result);
114+
}
115+
116+
public function testEmailExistsReturnsFalseForEmptyEmail(): void
117+
{
118+
$result = $this->userData->emailExists('');
119+
$this->assertFalse($result);
120+
}
95121
}

‎tests/phpMyFAQ/UserTest.php‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
<?php
2+
3+
namespace phpMyFAQ;
4+
5+
use Exception;
6+
use phpMyFAQ\Database\Sqlite3;
7+
use phpMyFAQ\User\UserData;
8+
use PHPUnit\Framework\MockObject\MockObject;
9+
use PHPUnit\Framework\TestCase;
10+
use ReflectionClass;
11+
12+
class UserTest extends TestCase
13+
{
14+
private Configuration|MockObject $configuration;
15+
private Sqlite3|MockObject $database;
16+
private User $user;
17+
private UserData|MockObject $userData;
18+
19+
protected function setUp(): void
20+
{
21+
$this->configuration = $this->createMock(Configuration::class);
22+
$this->database = $this->createMock(Sqlite3::class);
23+
$this->userData = $this->createMock(UserData::class);
24+
25+
$this->configuration->method('getDb')->willReturn($this->database);
26+
$this->configuration->method('get')->willReturnMap([
27+
['security.permLevel', 'basic'],
28+
]);
29+
30+
$this->user = new User($this->configuration);
31+
$this->user->userdata = $this->userData;
32+
}
33+
34+
public function testIsEmailAddressReturnsTrueForValidEmail(): void
35+
{
36+
$reflection = new ReflectionClass($this->user);
37+
$method = $reflection->getMethod('isEmailAddress');
38+
39+
$result = $method->invoke($this->user, 'test@example.com');
40+
$this->assertTrue($result);
41+
}
42+
43+
public function testIsEmailAddressReturnsFalseForInvalidEmail(): void
44+
{
45+
$reflection = new ReflectionClass($this->user);
46+
$method = $reflection->getMethod('isEmailAddress');
47+
48+
$result = $method->invoke($this->user, 'invalid-email');
49+
$this->assertFalse($result);
50+
}
51+
52+
public function testCreateUserThrowsExceptionWhenEmailAlreadyExistsAsLogin(): void
53+
{
54+
$this->expectException(Exception::class);
55+
$this->expectExceptionMessage(User::ERROR_USER_EMAIL_NOT_UNIQUE);
56+
57+
// Mock that login is valid
58+
$this->database->method('escape')->willReturn('test@example.com');
59+
60+
// Mock that getUserByLogin returns false (login doesn't exist as login)
61+
$this->database->method('query')->willReturn(true);
62+
$this->database->method('numRows')->willReturn(0);
63+
64+
// Mock that email exists in userdata
65+
$this->userData->method('emailExists')->willReturn(true);
66+
67+
$this->user->createUser('test@example.com');
68+
}
69+
70+
public function testCreateUserDoesNotCheckEmailWhenLoginIsNotEmail(): void
71+
{
72+
// Mock database operations to simulate login not existing
73+
$this->database->method('escape')->willReturn('username');
74+
$this->database->method('query')->willReturn(true);
75+
$this->database->method('numRows')->willReturn(0);
76+
77+
// emailExists should never be called for non-email logins
78+
$this->userData->expects($this->never())->method('emailExists');
79+
80+
// This will throw an exception because no auth container is set up,
81+
// but that's expected - we just want to verify emailExists wasn't called
82+
try {
83+
$this->user->createUser('username');
84+
} catch (Exception $e) {
85+
// Expected - ignore this exception as we're only testing the email check logic
86+
}
87+
}
88+
89+
public function testCreateUserThrowsExceptionWhenLoginNotUnique(): void
90+
{
91+
$this->expectException(Exception::class);
92+
$this->expectExceptionMessage(User::ERROR_USER_LOGIN_NOT_UNIQUE);
93+
94+
// Mock that login exists
95+
$this->database->method('escape')->willReturn('existinguser');
96+
$this->database->method('query')->willReturn(true);
97+
$this->database->method('numRows')->willReturn(1);
98+
$this->database->method('fetchArray')->willReturn([
99+
'user_id' => 1,
100+
'login' => 'existinguser',
101+
'account_status' => 'active',
102+
'is_superadmin' => false,
103+
'auth_source' => 'local'
104+
]);
105+
106+
$this->user->createUser('existinguser');
107+
}
108+
}

0 commit comments

Comments
 (0)