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.
This commit is contained in:
Marcin Kurczewski 2014-09-14 16:44:57 +02:00
parent cf0312ce43
commit 3e1aaebf89
8 changed files with 35 additions and 5 deletions

View file

@ -34,7 +34,7 @@ class UserViewProxy extends AbstractViewProxy
$result->emailUnconfirmed = $user->getEmailUnconfirmed(); $result->emailUnconfirmed = $user->getEmailUnconfirmed();
} }
$result->confirmed = !$user->getEmailUnconfirmed(); $result->confirmed = $user->isAccountConfirmed();
} }
return $result; return $result;
} }

View file

@ -23,6 +23,7 @@ final class User extends Entity
protected $lastLoginTime; protected $lastLoginTime;
protected $avatarStyle; protected $avatarStyle;
protected $browsingSettings; protected $browsingSettings;
protected $accountConfirmed = false;
public function getName() public function getName()
{ {
@ -54,6 +55,16 @@ final class User extends Entity
$this->emailUnconfirmed = $emailUnconfirmed; $this->emailUnconfirmed = $emailUnconfirmed;
} }
public function isAccountConfirmed()
{
return $this->accountConfirmed;
}
public function setAccountConfirmed($accountConfirmed)
{
$this->accountConfirmed = boolval($accountConfirmed);
}
public function getPasswordHash() public function getPasswordHash()
{ {
return $this->passwordHash; return $this->passwordHash;

View file

@ -100,7 +100,7 @@ class AuthService
private function doFinalChecksOnUser($user) private function doFinalChecksOnUser($user)
{ {
if (!$user->getEmail() and $this->config->security->needEmailActivationToRegister) if (!$user->isAccountConfirmed() and $this->config->security->needEmailActivationToRegister)
throw new \DomainException('User didn\'t confirm mail yet.'); throw new \DomainException('User didn\'t confirm account yet.');
} }
} }

View file

@ -253,6 +253,7 @@ class UserService
//whoever confirms e-mail first, wins. //whoever confirms e-mail first, wins.
$this->assertNoUserWithThisEmail($user, $user->getEmailUnconfirmed()); $this->assertNoUserWithThisEmail($user, $user->getEmailUnconfirmed());
$user->setAccountConfirmed(true);
if ($user->getEmailUnconfirmed()) if ($user->getEmailUnconfirmed())
{ {
$user->setEmail($user->getEmailUnconfirmed()); $user->setEmail($user->getEmailUnconfirmed());

View file

@ -0,0 +1,11 @@
<?php
namespace Szurubooru\Upgrades;
class Upgrade02 implements IUpgrade
{
public function run(\Szurubooru\DatabaseConnection $databaseConnection)
{
$databaseConnection->getPDO()->exec('
ALTER TABLE "users" ADD COLUMN accountConfirmed BOOLEAN NOT NULL DEFAULT FALSE');
}
}

View file

@ -10,6 +10,7 @@ return [
'upgrades' => DI\factory(function (DI\container $container) { 'upgrades' => DI\factory(function (DI\container $container) {
return [ return [
$container->get(\Szurubooru\Upgrades\Upgrade01::class), $container->get(\Szurubooru\Upgrades\Upgrade01::class),
$container->get(\Szurubooru\Upgrades\Upgrade02::class),
]; ];
}), }),

View file

@ -75,7 +75,7 @@ class AuthServiceTest extends \Szurubooru\Tests\AbstractTestCase
$testUser->setPasswordHash('hash'); $testUser->setPasswordHash('hash');
$this->userServiceMock->expects($this->once())->method('getByNameOrEmail')->willReturn($testUser); $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(); $authService = $this->getAuthService();
$formData = new \Szurubooru\FormData\LoginFormData(); $formData = new \Szurubooru\FormData\LoginFormData();
$formData->userNameOrEmail = 'dummy'; $formData->userNameOrEmail = 'dummy';
@ -147,7 +147,7 @@ class AuthServiceTest extends \Szurubooru\Tests\AbstractTestCase
$testToken->setAdditionalData($testUser->getId()); $testToken->setAdditionalData($testUser->getId());
$testToken->setPurpose(\Szurubooru\Entities\Token::PURPOSE_LOGIN); $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 = $this->getAuthService();
$authService->loginFromToken($testToken); $authService->loginFromToken($testToken);

View file

@ -106,6 +106,7 @@ final class UserServiceTest extends \Szurubooru\Tests\AbstractTestCase
$this->assertEquals('hash', $savedUser->getPasswordHash()); $this->assertEquals('hash', $savedUser->getPasswordHash());
$this->assertEquals(\Szurubooru\Entities\User::ACCESS_RANK_REGULAR_USER, $savedUser->getAccessRank()); $this->assertEquals(\Szurubooru\Entities\User::ACCESS_RANK_REGULAR_USER, $savedUser->getAccessRank());
$this->assertEquals('now', $savedUser->getRegistrationTime()); $this->assertEquals('now', $savedUser->getRegistrationTime());
$this->assertTrue($savedUser->isAccountConfirmed());
} }
public function testValidRegistrationWithMailActivation() public function testValidRegistrationWithMailActivation()
@ -136,6 +137,7 @@ final class UserServiceTest extends \Szurubooru\Tests\AbstractTestCase
$this->assertEquals('hash', $savedUser->getPasswordHash()); $this->assertEquals('hash', $savedUser->getPasswordHash());
$this->assertEquals(\Szurubooru\Entities\User::ACCESS_RANK_REGULAR_USER, $savedUser->getAccessRank()); $this->assertEquals(\Szurubooru\Entities\User::ACCESS_RANK_REGULAR_USER, $savedUser->getAccessRank());
$this->assertEquals('now', $savedUser->getRegistrationTime()); $this->assertEquals('now', $savedUser->getRegistrationTime());
$this->assertFalse($savedUser->isAccountConfirmed());
} }
public function testAccessRankOfFirstUser() public function testAccessRankOfFirstUser()
@ -220,6 +222,7 @@ final class UserServiceTest extends \Szurubooru\Tests\AbstractTestCase
$savedUser = $userService->updateUser($testUser, $formData); $savedUser = $userService->updateUser($testUser, $formData);
$this->assertEquals('hikari@geofront.gov', $savedUser->getEmail()); $this->assertEquals('hikari@geofront.gov', $savedUser->getEmail());
$this->assertNull($savedUser->getEmailUnconfirmed()); $this->assertNull($savedUser->getEmailUnconfirmed());
$this->assertTrue($savedUser->isAccountConfirmed());
} }
public function testUpdatingEmailWithConfirmation() public function testUpdatingEmailWithConfirmation()
@ -237,6 +240,7 @@ final class UserServiceTest extends \Szurubooru\Tests\AbstractTestCase
$savedUser = $userService->updateUser($testUser, $formData); $savedUser = $userService->updateUser($testUser, $formData);
$this->assertNull($savedUser->getEmail()); $this->assertNull($savedUser->getEmail());
$this->assertEquals('hikari@geofront.gov', $savedUser->getEmailUnconfirmed()); $this->assertEquals('hikari@geofront.gov', $savedUser->getEmailUnconfirmed());
$this->assertFalse($savedUser->isAccountConfirmed());
} }
public function testUpdatingEmailWithConfirmationToExisting() public function testUpdatingEmailWithConfirmationToExisting()
@ -261,6 +265,7 @@ final class UserServiceTest extends \Szurubooru\Tests\AbstractTestCase
{ {
$testUser = new \Szurubooru\Entities\User('yep, still me'); $testUser = new \Szurubooru\Entities\User('yep, still me');
$testUser->setEmail('hikari@geofront.gov'); $testUser->setEmail('hikari@geofront.gov');
$testUser->setAccountConfirmed(true);
$testUser->setEmailUnconfirmed('coolcat32@sakura.ne.jp'); $testUser->setEmailUnconfirmed('coolcat32@sakura.ne.jp');
$formData = new \Szurubooru\FormData\UserEditFormData; $formData = new \Szurubooru\FormData\UserEditFormData;
@ -275,6 +280,7 @@ final class UserServiceTest extends \Szurubooru\Tests\AbstractTestCase
$savedUser = $userService->updateUser($testUser, $formData); $savedUser = $userService->updateUser($testUser, $formData);
$this->assertEquals('hikari@geofront.gov', $savedUser->getEmail()); $this->assertEquals('hikari@geofront.gov', $savedUser->getEmail());
$this->assertNull($savedUser->getEmailUnconfirmed()); $this->assertNull($savedUser->getEmailUnconfirmed());
$this->assertTrue($savedUser->isAccountConfirmed());
} }
private function getUserService() private function getUserService()