From 3e1aaebf89e8371dafa88fb6c1b7692827b795f7 Mon Sep 17 00:00:00 2001 From: Marcin Kurczewski Date: Sun, 14 Sep 2014 16:44:57 +0200 Subject: [PATCH] Fixed account activation for first user Until now, AuthService used to check for empty e-mail in order to tell whether an account is activated. This was wrong for following scenario: 1. User doesn't enter any e-mail. 2. Because he is about to become the first user to register, he will become an administrator. 3. Administrators don't need to confirm their e-mail address. Activation e-mail is not sent, code for e-mail activation is run instead. 4. The user succeeds to create an e-mail-less administrator account. 5. The user fails to login due to unconfirmed e-mail. 6. The code that activates an e-mail just moves unconfirmed e-mail to primary e-mail. That was the bug, there's no e-mail to confirm. Things got (hopefully) simpler now, since I added separate column for indicating whether account is activated. --- src/Controllers/ViewProxies/UserViewProxy.php | 2 +- src/Entities/User.php | 11 +++++++++++ src/Services/AuthService.php | 4 ++-- src/Services/UserService.php | 1 + src/Upgrades/Upgrade02.php | 11 +++++++++++ src/di.php | 1 + tests/Services/AuthServiceTest.php | 4 ++-- tests/Services/UserServiceTest.php | 6 ++++++ 8 files changed, 35 insertions(+), 5 deletions(-) create mode 100644 src/Upgrades/Upgrade02.php diff --git a/src/Controllers/ViewProxies/UserViewProxy.php b/src/Controllers/ViewProxies/UserViewProxy.php index f74cd376..d4c992c2 100644 --- a/src/Controllers/ViewProxies/UserViewProxy.php +++ b/src/Controllers/ViewProxies/UserViewProxy.php @@ -34,7 +34,7 @@ class UserViewProxy extends AbstractViewProxy $result->emailUnconfirmed = $user->getEmailUnconfirmed(); } - $result->confirmed = !$user->getEmailUnconfirmed(); + $result->confirmed = $user->isAccountConfirmed(); } return $result; } diff --git a/src/Entities/User.php b/src/Entities/User.php index da8196d2..f486d9c6 100644 --- a/src/Entities/User.php +++ b/src/Entities/User.php @@ -23,6 +23,7 @@ final class User extends Entity protected $lastLoginTime; protected $avatarStyle; protected $browsingSettings; + protected $accountConfirmed = false; public function getName() { @@ -54,6 +55,16 @@ final class User extends Entity $this->emailUnconfirmed = $emailUnconfirmed; } + public function isAccountConfirmed() + { + return $this->accountConfirmed; + } + + public function setAccountConfirmed($accountConfirmed) + { + $this->accountConfirmed = boolval($accountConfirmed); + } + public function getPasswordHash() { return $this->passwordHash; diff --git a/src/Services/AuthService.php b/src/Services/AuthService.php index 9776ef48..2d21ffd6 100644 --- a/src/Services/AuthService.php +++ b/src/Services/AuthService.php @@ -100,7 +100,7 @@ class AuthService private function doFinalChecksOnUser($user) { - if (!$user->getEmail() and $this->config->security->needEmailActivationToRegister) - throw new \DomainException('User didn\'t confirm mail yet.'); + if (!$user->isAccountConfirmed() and $this->config->security->needEmailActivationToRegister) + throw new \DomainException('User didn\'t confirm account yet.'); } } diff --git a/src/Services/UserService.php b/src/Services/UserService.php index ef5c0b85..894ccd22 100644 --- a/src/Services/UserService.php +++ b/src/Services/UserService.php @@ -253,6 +253,7 @@ class UserService //whoever confirms e-mail first, wins. $this->assertNoUserWithThisEmail($user, $user->getEmailUnconfirmed()); + $user->setAccountConfirmed(true); if ($user->getEmailUnconfirmed()) { $user->setEmail($user->getEmailUnconfirmed()); diff --git a/src/Upgrades/Upgrade02.php b/src/Upgrades/Upgrade02.php new file mode 100644 index 00000000..12f45123 --- /dev/null +++ b/src/Upgrades/Upgrade02.php @@ -0,0 +1,11 @@ +getPDO()->exec(' + ALTER TABLE "users" ADD COLUMN accountConfirmed BOOLEAN NOT NULL DEFAULT FALSE'); + } +} diff --git a/src/di.php b/src/di.php index 5d552718..efdbc012 100644 --- a/src/di.php +++ b/src/di.php @@ -10,6 +10,7 @@ return [ 'upgrades' => DI\factory(function (DI\container $container) { return [ $container->get(\Szurubooru\Upgrades\Upgrade01::class), + $container->get(\Szurubooru\Upgrades\Upgrade02::class), ]; }), diff --git a/tests/Services/AuthServiceTest.php b/tests/Services/AuthServiceTest.php index 1e825124..7654bdd6 100644 --- a/tests/Services/AuthServiceTest.php +++ b/tests/Services/AuthServiceTest.php @@ -75,7 +75,7 @@ class AuthServiceTest extends \Szurubooru\Tests\AbstractTestCase $testUser->setPasswordHash('hash'); $this->userServiceMock->expects($this->once())->method('getByNameOrEmail')->willReturn($testUser); - $this->setExpectedException(\Exception::class, 'User didn\'t confirm mail yet'); + $this->setExpectedException(\Exception::class, 'User didn\'t confirm account yet'); $authService = $this->getAuthService(); $formData = new \Szurubooru\FormData\LoginFormData(); $formData->userNameOrEmail = 'dummy'; @@ -147,7 +147,7 @@ class AuthServiceTest extends \Szurubooru\Tests\AbstractTestCase $testToken->setAdditionalData($testUser->getId()); $testToken->setPurpose(\Szurubooru\Entities\Token::PURPOSE_LOGIN); - $this->setExpectedException(\Exception::class, 'User didn\'t confirm mail yet'); + $this->setExpectedException(\Exception::class, 'User didn\'t confirm account yet'); $authService = $this->getAuthService(); $authService->loginFromToken($testToken); diff --git a/tests/Services/UserServiceTest.php b/tests/Services/UserServiceTest.php index d44b0180..a8dd2e10 100644 --- a/tests/Services/UserServiceTest.php +++ b/tests/Services/UserServiceTest.php @@ -106,6 +106,7 @@ final class UserServiceTest extends \Szurubooru\Tests\AbstractTestCase $this->assertEquals('hash', $savedUser->getPasswordHash()); $this->assertEquals(\Szurubooru\Entities\User::ACCESS_RANK_REGULAR_USER, $savedUser->getAccessRank()); $this->assertEquals('now', $savedUser->getRegistrationTime()); + $this->assertTrue($savedUser->isAccountConfirmed()); } public function testValidRegistrationWithMailActivation() @@ -136,6 +137,7 @@ final class UserServiceTest extends \Szurubooru\Tests\AbstractTestCase $this->assertEquals('hash', $savedUser->getPasswordHash()); $this->assertEquals(\Szurubooru\Entities\User::ACCESS_RANK_REGULAR_USER, $savedUser->getAccessRank()); $this->assertEquals('now', $savedUser->getRegistrationTime()); + $this->assertFalse($savedUser->isAccountConfirmed()); } public function testAccessRankOfFirstUser() @@ -220,6 +222,7 @@ final class UserServiceTest extends \Szurubooru\Tests\AbstractTestCase $savedUser = $userService->updateUser($testUser, $formData); $this->assertEquals('hikari@geofront.gov', $savedUser->getEmail()); $this->assertNull($savedUser->getEmailUnconfirmed()); + $this->assertTrue($savedUser->isAccountConfirmed()); } public function testUpdatingEmailWithConfirmation() @@ -237,6 +240,7 @@ final class UserServiceTest extends \Szurubooru\Tests\AbstractTestCase $savedUser = $userService->updateUser($testUser, $formData); $this->assertNull($savedUser->getEmail()); $this->assertEquals('hikari@geofront.gov', $savedUser->getEmailUnconfirmed()); + $this->assertFalse($savedUser->isAccountConfirmed()); } public function testUpdatingEmailWithConfirmationToExisting() @@ -261,6 +265,7 @@ final class UserServiceTest extends \Szurubooru\Tests\AbstractTestCase { $testUser = new \Szurubooru\Entities\User('yep, still me'); $testUser->setEmail('hikari@geofront.gov'); + $testUser->setAccountConfirmed(true); $testUser->setEmailUnconfirmed('coolcat32@sakura.ne.jp'); $formData = new \Szurubooru\FormData\UserEditFormData; @@ -275,6 +280,7 @@ final class UserServiceTest extends \Szurubooru\Tests\AbstractTestCase $savedUser = $userService->updateUser($testUser, $formData); $this->assertEquals('hikari@geofront.gov', $savedUser->getEmail()); $this->assertNull($savedUser->getEmailUnconfirmed()); + $this->assertTrue($savedUser->isAccountConfirmed()); } private function getUserService()