From e13db65f686b97dfe152b49de40b907dee6f8dd7 Mon Sep 17 00:00:00 2001 From: Marcin Kurczewski Date: Tue, 2 Sep 2014 09:09:07 +0200 Subject: [PATCH] Paid off technical debt regarding validation --- src/Controllers/AuthController.php | 5 - src/Services/AuthService.php | 9 ++ src/Services/EmailService.php | 9 -- src/Services/PasswordService.php | 19 ---- src/Services/UserService.php | 28 ++---- src/Validator.php | 78 +++++++++++++++ tests/Services/AuthServiceTest.php | 3 + tests/Services/EmailServiceTest.php | 28 ------ tests/Services/UserServiceTest.php | 21 +--- tests/ValidatorTest.php | 149 ++++++++++++++++++++++++++++ 10 files changed, 248 insertions(+), 101 deletions(-) create mode 100644 src/Validator.php delete mode 100644 tests/Services/EmailServiceTest.php create mode 100644 tests/ValidatorTest.php diff --git a/src/Controllers/AuthController.php b/src/Controllers/AuthController.php index a381401b..9ad45e1c 100644 --- a/src/Controllers/AuthController.php +++ b/src/Controllers/AuthController.php @@ -30,15 +30,10 @@ final class AuthController extends AbstractController { if (isset($this->inputReader->userName) and isset($this->inputReader->password)) { - $this->userService->validateUserName($this->inputReader->userName); - $this->passwordService->validatePassword($this->inputReader->password); - $this->authService->loginFromCredentials($this->inputReader->userName, $this->inputReader->password); } elseif (isset($this->inputReader->token)) { - if (!$this->inputReader->token) - throw new \DomainException('Authentication token cannot be empty.'); $this->authService->loginFromToken($this->inputReader->token); } else diff --git a/src/Services/AuthService.php b/src/Services/AuthService.php index 25c17378..6ee03495 100644 --- a/src/Services/AuthService.php +++ b/src/Services/AuthService.php @@ -6,18 +6,22 @@ final class AuthService private $loggedInUser = null; private $loginToken = null; + private $validator; private $passwordService; private $timeService; private $userDao; private $tokenDao; public function __construct( + \Szurubooru\Validator $validator, \Szurubooru\Services\PasswordService $passwordService, \Szurubooru\Services\TimeService $timeService, \Szurubooru\Dao\TokenDao $tokenDao, \Szurubooru\Dao\UserDao $userDao) { $this->loggedInUser = $this->getAnonymousUser(); + + $this->validator = $validator; $this->passwordService = $passwordService; $this->timeService = $timeService; $this->tokenDao = $tokenDao; @@ -41,6 +45,9 @@ final class AuthService public function loginFromCredentials($userName, $password) { + $this->validator->validateUserName($userName); + $this->validator->validatePassword($password); + $user = $this->userDao->getByName($userName); if (!$user) throw new \InvalidArgumentException('User not found.'); @@ -56,6 +63,8 @@ final class AuthService public function loginFromToken($loginTokenName) { + $this->validator->validateToken($loginTokenName); + $loginToken = $this->tokenDao->getByName($loginTokenName); if (!$loginToken) throw new \Exception('Invalid login token.'); diff --git a/src/Services/EmailService.php b/src/Services/EmailService.php index cd5dbccd..f6a5a488 100644 --- a/src/Services/EmailService.php +++ b/src/Services/EmailService.php @@ -3,13 +3,4 @@ namespace Szurubooru\Services; class EmailService { - //todo: refactor this to generic validation - public function validateEmail($email) - { - if (!$email) - return; - - if (!preg_match('/^[^@]+@[^@]+\.\w+$/', $email)) - throw new \DomainException('Specified e-mail appears to be invalid.'); - } } diff --git a/src/Services/PasswordService.php b/src/Services/PasswordService.php index e80ead7a..fdbf6b7e 100644 --- a/src/Services/PasswordService.php +++ b/src/Services/PasswordService.php @@ -10,25 +10,6 @@ class PasswordService $this->config = $config; } - //todo: refactor this to generic validation - public function validatePassword($password) - { - if (!$password) - throw new \DomainException('Password cannot be empty.'); - - $minPasswordLength = intval($this->config->security->minPasswordLength); - if (strlen($password) < $minPasswordLength) - throw new \DomainException('Password must have at least ' . $minPasswordLength . ' character(s).'); - - if (preg_match('/[^\x20-\x7f]/', $password)) - { - throw new \DomainException( - 'Password should contain only characters from ASCII range to avoid potential problems with encoding.'); - } - - return true; - } - public function getHash($password) { return hash('sha256', $this->config->security->secret . '/' . $password); diff --git a/src/Services/UserService.php b/src/Services/UserService.php index 043a001e..2bb4a76e 100644 --- a/src/Services/UserService.php +++ b/src/Services/UserService.php @@ -3,20 +3,20 @@ namespace Szurubooru\Services; class UserService { - private $config; + private $validator; private $userDao; private $passwordService; private $emailService; private $timeService; public function __construct( - \Szurubooru\Config $config, + \Szurubooru\Validator $validator, \Szurubooru\Dao\UserDao $userDao, \Szurubooru\Services\PasswordService $passwordService, \Szurubooru\Services\EmailService $emailService, \Szurubooru\Services\TimeService $timeService) { - $this->config = $config; + $this->validator = $validator; $this->userDao = $userDao; $this->passwordService = $passwordService; $this->emailService = $emailService; @@ -25,9 +25,9 @@ class UserService public function register(\Szurubooru\FormData\RegistrationFormData $formData) { - $this->validateUserName($formData->name); - $this->passwordService->validatePassword($formData->password); - $this->emailService->validateEmail($formData->email); + $this->validator->validateUserName($formData->name); + $this->validator->validatePassword($formData->password); + $this->validator->validateEmail($formData->email); if ($this->userDao->getByName($formData->name)) throw new \DomainException('User with this name already exists.'); @@ -48,20 +48,4 @@ class UserService return $this->userDao->save($user); } - - //todo: refactor this to generic validation - public function validateUserName(&$userName) - { - $userName = trim($userName); - - if (!$userName) - throw new \DomainException('User name cannot be empty.'); - - $minUserNameLength = intval($this->config->users->minUserNameLength); - $maxUserNameLength = intval($this->config->users->maxUserNameLength); - if (strlen($userName) < $minUserNameLength) - throw new \DomainException('User name must have at least ' . $minUserNameLength . ' character(s).'); - if (strlen($userName) > $maxUserNameLength) - throw new \DomainException('User name must have at most ' . $maxUserNameLength . ' character(s).'); - } } diff --git a/src/Validator.php b/src/Validator.php new file mode 100644 index 00000000..8d202867 --- /dev/null +++ b/src/Validator.php @@ -0,0 +1,78 @@ +config = $config; + } + + public function validateNonEmpty($subject, $subjectName = 'Object') + { + if (!$subject) + throw new \DomainException($subjectName . ' cannot be empty.'); + } + + public function validateLength($subject, $minLength, $maxLength, $subjectName = 'Object') + { + $this->validateMinLength($subject, $minLength, $subjectName); + $this->validateMaxLength($subject, $maxLength, $subjectName); + } + + public function validateMinLength($subject, $minLength, $subjectName = 'Object') + { + if (strlen($subject) < $minLength) + throw new \DomainException($subjectName . ' must have at least ' . $minLength . ' character(s).'); + } + + public function validateMaxLength($subject, $maxLength, $subjectName = 'Object') + { + if (strlen($subject) > $maxLength) + throw new \DomainException($subjectName . ' must have at most ' . $maxLength . ' character(s).'); + } + + public function validateUserName(&$userName) + { + $userName = trim($userName); + + $minUserNameLength = intval($this->config->users->minUserNameLength); + $maxUserNameLength = intval($this->config->users->maxUserNameLength); + $this->validateNonEmpty($userName, 'User name'); + $this->validateLength($userName, $minUserNameLength, $maxUserNameLength, 'User name'); + + if (preg_match('/[^a-zA-Z_-]/', $userName)) + { + throw new \DomainException('User name may contain only characters, numbers, underscore (_) and dash (-).'); + } + } + + public function validateEmail($email) + { + if (!$email) + return; + + if (!preg_match('/^[^@]+@[^@]+\.\w+$/', $email)) + throw new \DomainException('Specified e-mail appears to be invalid.'); + } + + public function validatePassword($password) + { + $minPasswordLength = intval($this->config->security->minPasswordLength); + $this->validateNonEmpty($password, 'Password'); + $this->validateMinLength($password, $minPasswordLength, 'Password'); + + if (preg_match('/[^\x20-\x7f]/', $password)) + { + throw new \DomainException( + 'Password may contain only characters from ASCII range to avoid potential problems with encoding.'); + } + } + + public function validateToken($token) + { + $this->validateNonEmpty($token, 'Token'); + } +} diff --git a/tests/Services/AuthServiceTest.php b/tests/Services/AuthServiceTest.php index 56526e61..c3da4094 100644 --- a/tests/Services/AuthServiceTest.php +++ b/tests/Services/AuthServiceTest.php @@ -3,6 +3,7 @@ namespace Szurubooru\Tests\Services; class AuthServiceTest extends \Szurubooru\Tests\AbstractTestCase { + private $validatorMock; private $passwordServiceMock; private $timeServiceMock; private $tokenDaoMock; @@ -10,6 +11,7 @@ class AuthServiceTest extends \Szurubooru\Tests\AbstractTestCase public function setUp() { + $this->validatorMock = $this->mock(\Szurubooru\Validator::class); $this->passwordServiceMock = $this->mock(\Szurubooru\Services\PasswordService::class); $this->timeServiceMock = $this->mock(\Szurubooru\Services\TimeService::class); $this->tokenDaoMock = $this->mock(\Szurubooru\Dao\TokenDao::class); @@ -91,6 +93,7 @@ class AuthServiceTest extends \Szurubooru\Tests\AbstractTestCase private function getAuthService() { return new \Szurubooru\Services\AuthService( + $this->validatorMock, $this->passwordServiceMock, $this->timeServiceMock, $this->tokenDaoMock, diff --git a/tests/Services/EmailServiceTest.php b/tests/Services/EmailServiceTest.php deleted file mode 100644 index 2ba5c067..00000000 --- a/tests/Services/EmailServiceTest.php +++ /dev/null @@ -1,28 +0,0 @@ -setExpectedException(\DomainException::class); - $emailService->validateEmail('ghost'); - } - - public function testEmailWithoutDotInDomain() - { - $emailService = new \Szurubooru\Services\EmailService(); - - $this->setExpectedException(\DomainException::class); - $emailService->validateEmail('ghost@cemetery'); - } - - public function testValidEmail() - { - $emailService = new \Szurubooru\Services\EmailService(); - - $this->assertNull($emailService->validateEmail('ghost@cemetery.consulting')); - } -} diff --git a/tests/Services/UserServiceTest.php b/tests/Services/UserServiceTest.php index cf5190ee..025a36a3 100644 --- a/tests/Services/UserServiceTest.php +++ b/tests/Services/UserServiceTest.php @@ -3,7 +3,7 @@ namespace Szurubooru\Tests\Services; final class UserServiceTest extends \Szurubooru\Tests\AbstractTestCase { - private $configMock; + private $validatorMock; private $userDaoMock; private $passwordServiceMock; private $emailServiceMock; @@ -11,7 +11,7 @@ final class UserServiceTest extends \Szurubooru\Tests\AbstractTestCase public function setUp() { - $this->configMock = new \Szurubooru\Config; + $this->validatorMock = $this->mock(\Szurubooru\Validator::class); $this->userDaoMock = $this->mock(\Szurubooru\Dao\UserDao::class); $this->passwordServiceMock = $this->mock(\Szurubooru\Services\PasswordService::class); $this->emailServiceMock = $this->mock(\Szurubooru\Services\EmailService::class); @@ -25,11 +25,6 @@ final class UserServiceTest extends \Szurubooru\Tests\AbstractTestCase $formData->password = 'password'; $formData->email = 'email'; - //todo: this shouldn't be needed. refactor validation - $this->configMock->users = new \StdClass; - $this->configMock->users->minUserNameLength = 0; - $this->configMock->users->maxUserNameLength = 50; - $this->passwordServiceMock->method('getHash')->willReturn('hash'); $this->timeServiceMock->method('getCurrentTime')->willReturn('now'); $this->userDaoMock->method('hasAnyUsers')->willReturn(true); @@ -52,11 +47,6 @@ final class UserServiceTest extends \Szurubooru\Tests\AbstractTestCase $formData->password = 'password'; $formData->email = 'email'; - //todo: this shouldn't be needed. refactor validation - $this->configMock->users = new \StdClass; - $this->configMock->users->minUserNameLength = 0; - $this->configMock->users->maxUserNameLength = 50; - $this->userDaoMock->method('hasAnyUsers')->willReturn(false); $this->userDaoMock->method('save')->will($this->returnArgument(0)); @@ -73,11 +63,6 @@ final class UserServiceTest extends \Szurubooru\Tests\AbstractTestCase $formData->password = 'password'; $formData->email = 'email'; - //todo: this shouldn't be needed. refactor validation - $this->configMock->users = new \StdClass; - $this->configMock->users->minUserNameLength = 0; - $this->configMock->users->maxUserNameLength = 50; - $this->userDaoMock->method('hasAnyUsers')->willReturn(true); $this->userDaoMock->method('getByName')->willReturn(new \Szurubooru\Entities\User()); $this->userDaoMock->method('save')->will($this->returnArgument(0)); @@ -91,7 +76,7 @@ final class UserServiceTest extends \Szurubooru\Tests\AbstractTestCase private function getUserService() { return new \Szurubooru\Services\UserService( - $this->configMock, + $this->validatorMock, $this->userDaoMock, $this->passwordServiceMock, $this->emailServiceMock, diff --git a/tests/ValidatorTest.php b/tests/ValidatorTest.php new file mode 100644 index 00000000..b6ff79d7 --- /dev/null +++ b/tests/ValidatorTest.php @@ -0,0 +1,149 @@ +config = new \Szurubooru\Config(); + } + + public function testMinLengthName() + { + $validator = $this->getValidator(); + $this->setExpectedException(\Exception::class, 'Object must have at least 50 character(s)'); + $validator->validateMinLength('too short', 50); + } + + public function testMaxLengthName() + { + $validator = $this->getValidator(); + $this->setExpectedException(\Exception::class, 'Object must have at most 1 character(s)'); + $validator->validateMaxLength('too long', 1); + } + + public function testValidLengthName() + { + $validator = $this->getValidator(); + $this->assertNull($validator->validateLength('fitting', 1, 50)); + $this->assertNull($validator->validateMaxLength('fitting', 50)); + $this->assertNull($validator->validateMinLength('fitting', 1)); + } + + public function testEmptyUserName() + { + $this->config->users = new \StdClass; + $this->config->users->minUserNameLength = 0; + $this->config->users->maxUserNameLength = 1; + $this->setExpectedException(\Exception::class, 'User name cannot be empty'); + $userName = ''; + $validator = $this->getValidator(); + $validator->validateUserName($userName); + } + + public function testTooShortUserName() + { + $this->config->users = new \StdClass; + $this->config->users->minUserNameLength = 30; + $this->config->users->maxUserNameLength = 50; + $this->setExpectedException(\Exception::class, 'User name must have at least 30 character(s)'); + $userName = 'godzilla'; + $validator = $this->getValidator(); + $validator->validateUserName($userName); + } + + public function testTooLongUserName() + { + $this->config->users = new \StdClass; + $this->config->users->minUserNameLength = 30; + $this->config->users->maxUserNameLength = 50; + $this->setExpectedException(\Exception::class, 'User name must have at most 50 character(s)'); + $userName = 'godzilla' . str_repeat('a', 50); + $validator = $this->getValidator(); + $validator->validateUserName($userName); + } + + public function testUserNameWithSpaces() + { + $this->config->users = new \StdClass; + $this->config->users->minUserNameLength = 0; + $this->config->users->maxUserNameLength = 100; + $userName = ' godzilla '; + $validator = $this->getValidator(); + $validator->validateUserName($userName); + $this->assertEquals('godzilla', $userName); + } + + public function testUserNameWithInvalidCharacters() + { + $this->config->users = new \StdClass; + $this->config->users->minUserNameLength = 0; + $this->config->users->maxUserNameLength = 100; + $userName = '..:xXx:godzilla:xXx:..'; + $this->setExpectedException(\Exception::class, 'User name may contain only'); + $validator = $this->getValidator(); + $validator->validateUserName($userName); + } + + public function testEmailWithoutAt() + { + $validator = $this->getValidator(); + $this->setExpectedException(\DomainException::class); + $validator->validateEmail('ghost'); + } + + public function testEmailWithoutDotInDomain() + { + $validator = $this->getValidator(); + $this->setExpectedException(\DomainException::class); + $validator->validateEmail('ghost@cemetery'); + } + + public function testValidEmail() + { + $validator = $this->getValidator(); + $this->assertNull($validator->validateEmail('ghost@cemetery.consulting')); + } + + public function testEmptyPassword() + { + $this->config->security = new \StdClass; + $this->config->security->minPasswordLength = 0; + $this->setExpectedException(\Exception::class, 'Password cannot be empty'); + $validator = $this->getValidator(); + $validator->validatePassword(''); + } + + public function testTooShortPassword() + { + $this->config->security = new \StdClass; + $this->config->security->minPasswordLength = 10000; + $this->setExpectedException(\Exception::class, 'Password must have at least 10000 character(s)'); + $validator = $this->getValidator(); + $validator->validatePassword('password123'); + } + + public function testNonAsciiPassword() + { + $this->config->security = new \StdClass; + $this->config->security->minPasswordLength = 0; + $this->setExpectedException(\Exception::class, 'Password may contain only'); + $validator = $this->getValidator(); + $validator->validatePassword('良いパスワード'); + } + + public function testValidPassword() + { + $this->config->security = new \StdClass; + $this->config->security->minPasswordLength = 0; + $validator = $this->getValidator(); + $this->assertNull($validator->validatePassword('password')); + } + + private function getValidator() + { + return new \Szurubooru\Validator($this->config); + } +}