8000 [Security] Added type-hints to password encoders by derrabus · Pull Request #32352 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Security] Added type-hints to password encoders #32352

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 4, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,9 @@ public function needsRehash(string $encoded): bool
/**
* Demerges a merge password and salt string.
*
* @param string $mergedPasswordSalt The merged password and salt string
*
* @return array An array where the first element is the password and the second the salt
*/
protected function demergePasswordAndSalt($mergedPasswordSalt)
protected function demergePasswordAndSalt(string $mergedPasswordSalt)
{
if (empty($mergedPasswordSalt)) {
return ['', ''];
Expand All @@ -56,14 +54,11 @@ protected function demergePasswordAndSalt($mergedPasswordSalt)
/**
* Merges a password and a salt.
*
* @param string $password The password to be used
* @param string|null $salt The salt to be used
*
* @return string a merged password and salt
*
* @throws \InvalidArgumentException
*/
protected function mergePasswordAndSalt($password, $salt)
protected function mergePasswordAndSalt(string $password, ?string $salt)
{
if (empty($salt)) {
return $password;
Expand All @@ -82,24 +77,19 @@ protected function mergePasswordAndSalt($password, $salt)
* This method implements a constant-time algorithm to compare passwords to
* avoid (remote) timing attacks.
*
* @param string $password1 The first password
* @param string $password2 The second password
*
* @return bool true if the two passwords are the same, false otherwise
*/
protected function comparePasswords($password1, $password2)
protected function comparePasswords(string $password1, string $password2)
{
return hash_equals($password1, $password2);
}

/**
* Checks if the password is too long.
*
* @param string $password The password to check
*
* @return bool true if the password is too long, false otherwise
*/
protected function isPasswordTooLong($password)
protected function isPasswordTooLong(string $password)
{
return \strlen($password) > static::MAX_PASSWORD_LENGTH;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public function __construct(string $algorithm = 'sha512', bool $encodeHashAsBase
/**
* {@inheritdoc}
*/
public function encodePassword($raw, $salt)
public function encodePassword(string $raw, ?string $salt)
{
if ($this->isPasswordTooLong($raw)) {
throw new BadCredentialsException('Invalid password.');
Expand All @@ -63,7 +63,7 @@ public function encodePassword($raw, $salt)
/**
* {@inheritdoc}
*/
public function isPasswordValid($encoded, $raw, $salt)
public function isPasswordValid(string $encoded, string $raw, ?string $salt)
{
return !$this->isPasswordTooLong($raw) && $this->comparePasswords($encoded, $this->encodePassword($raw, $salt));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,15 @@ public function __construct(PasswordEncoderInterface $bestEncoder, PasswordEncod
/**
* {@inheritdoc}
*/
public function encodePassword($raw, $salt): string
public function encodePassword(string $raw, ?string $salt): string
{
return $this->bestEncoder->encodePassword($raw, $salt);
}

/**
* {@inheritdoc}
*/
public function isPasswordValid($encoded, $raw, $salt): bool
public function isPasswordValid(string $encoded, string $raw, ?string $salt): bool
{
if ($this->bestEncoder->isPasswordValid($encoded, $raw, $salt)) {
return true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public function __construct(int $opsLimit = null, int $memLimit = null, int $cos
/**
* {@inheritdoc}
*/
public function encodePassword($raw, $salt)
public function encodePassword(string $raw, ?string $salt)
{
if (\strlen($raw) > self::MAX_PASSWORD_LENGTH) {
throw new BadCredentialsException('Invalid password.');
Expand All @@ -78,7 +78,7 @@ public function encodePassword($raw, $salt)
/**
* {@inheritdoc}
*/
public function isPasswordValid($encoded, $raw, $salt): bool
public function isPasswordValid(string $encoded, string $raw, ?string $salt): bool
{
if (72 < \strlen($raw) && 0 === strpos($encoded, '$2')) {
// BCrypt encodes only the first 72 chars
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,12 @@ interface PasswordEncoderInterface
/**
* Encodes the raw password.
*
* @param string $raw The password to encode
* @param string|null $salt The salt
*
* @return string The encoded password
*
* @throws BadCredentialsException If the raw password is invalid, e.g. excessively long
* @throws \InvalidArgumentException If the salt is invalid
*/
public function encodePassword($raw, $salt);
public function encodePassword(string $raw, ?string $salt);

/**
* Checks a raw password against an encoded password.
Expand All @@ -44,7 +41,7 @@ public function encodePassword($raw, $salt);
*
* @throws \InvalidArgumentException If the salt is invalid
*/
public function isPasswordValid($encoded, $raw, $salt);
public function isPasswordValid(string $encoded, string $raw, ?string $salt);

/**
* Checks if an encoded password would benefit from rehashing.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public function __construct(string $algorithm = 'sha512', bool $encodeHashAsBase
*
* @throws \LogicException when the algorithm is not supported
*/
public function encodePassword($raw, $salt)
public function encodePassword(string $raw, ?string $salt)
{
if ($this->isPasswordTooLong($raw)) {
throw new BadCredentialsException('Invalid password.');
Expand All @@ -70,7 +70,7 @@ public function encodePassword($raw, $salt)
/**
* {@inheritdoc}
*/
public function isPasswordValid($encoded, $raw, $salt)
public function isPasswordValid(string $encoded, string $raw, ?string $salt)
{
return !$this->isPasswordTooLong($raw) && $this->comparePasswords($encoded, $this->encodePassword($raw, $salt));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public function __construct(bool $ignorePasswordCase = false)
/**
* {@inheritdoc}
*/
public function encodePassword($raw, $salt)
public function encodePassword(string $raw, ?string $salt)
{
if ($this->isPasswordTooLong($raw)) {
throw new BadCredentialsException('Invalid password.');
Expand All @@ -45,7 +45,7 @@ public function encodePassword($raw, $salt)
/**
* {@inheritdoc}
*/
public function isPasswordValid($encoded, $raw, $salt)
public function isPasswordValid(string $encoded, string $raw, ?string $salt)
{
if ($this->isPasswordTooLong($raw)) {
return false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public static function isSupported(): bool
/**
* {@inheritdoc}
*/
public function encodePassword($raw, $salt): string
public function encodePassword(string $raw, ?string $salt): string
{
if (\strlen($raw) > self::MAX_PASSWORD_LENGTH) {
throw new BadCredentialsException('Invalid password.');
Expand All @@ -74,7 +74,7 @@ public function encodePassword($raw, $salt): string
/**
* {@inheritdoc}
*/
public function isPasswordValid($encoded, $raw, $salt): bool
public function isPasswordValid(string $encoded, string $raw, ?string $salt): bool
{
if (\strlen($raw) > self::MAX_PASSWORD_LENGTH) {
return false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public function __construct(EncoderFactoryInterface $encoderFactory)
/**
* {@inheritdoc}
*/
public function encodePassword(UserInterface $user, $plainPassword)
public function encodePassword(UserInterface $user, string $plainPassword)
{
$encoder = $this->encoderFactory->getEncoder($user);

Expand All @@ -40,7 +40,7 @@ public function encodePassword(UserInterface $user, $plainPassword)
/**
* {@inheritdoc}
*/
public function isPasswordValid(UserInterface $user, $raw)
public function isPasswordValid(UserInterface $user, string $raw)
{
$encoder = $this->encoderFactory->getEncoder($user);

Expand Down
10 changes: 2 additions & 8 deletions
Original file line number Diff line number Diff line change
Expand Up @@ -23,20 +23,14 @@ interface UserPasswordEncoderInterface
/**
* Encodes the plain password.
*
* @param UserInterface $user The user
* @param string $plainPassword The password to encode
*
* @return string The encoded password
*/
public function encodePassword(UserInterface $user, $plainPassword);
public function encodePassword(UserInterface $user, string $plainPassword);

/**
* @param UserInterface $user The user
* @param string $raw A raw password
*
* @return bool true if the password is valid, false otherwise
*/
public function isPasswordValid(UserInterface $user, $raw);
public function isPasswordValid(UserInterface $user, string $raw);

/**
* Checks if an encoded password would benefit from rehashing.
Expand Down
3A0D
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use Symfony\Component\Security\Core\Authentication\Provider\DaoAuthenticationProvider;
use Symfony\Component\Security\Core\Encoder\PlaintextPasswordEncoder;
use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
use Symfony\Component\Security\Core\User\UserInterface;

class DaoAuthenticationProviderTest extends TestCase
{
Expand Down Expand Up @@ -73,7 +74,7 @@ public function testRetrieveUserReturnsUserFromTokenOnReauthentication()
->method('loadUserByUsername')
;

$user = $this->getMockBuilder('Symfony\\Component\\Security\\Core\\User\\UserInterface')->getMock();
$user = new TestUser();
$token = $this->getSupportedToken();
$token->expects($this->once())
->method('getUser')
Expand All @@ -90,7 +91,7 @@ public function testRetrieveUserReturnsUserFromTokenOnReauthentication()

public function testRetrieveUser()
{
$user = $this->getMockBuilder('Symfony\\Component\\Security\\Core\\User\\UserInterface')->getMock();
$user = new TestUser();

$userProvider = $this->getMockBuilder('Symfony\\Component\\Security\\Core\\User\\UserProviderInterface')->getMock();
$userProvider->expects($this->once())
Expand Down Expand Up @@ -127,11 +128,7 @@ public function testCheckAuthenticationWhenCredentialsAreEmpty()
->willReturn('')
;

$method->invoke(
$provider,
$this->getMockBuilder('Symfony\\Component\\Security\\Core\\User\\UserInterface')->getMock(),
$token
);
$method->invoke($provider, new TestUser(), $token);
}

public function testCheckAuthenticationWhenCredentialsAre0()
Expand All @@ -154,11 +151,7 @@ public function testCheckAuthenticationWhenCredentialsAre0()
->willReturn('0')
;

$method->invoke(
$provider,
$this->getMockBuilder('Symfony\\Component\\Security\\Core\\User\\UserInterface')->getMock(),
$token
);
$method->invoke($provider, new TestUser(), $token);
}

/**
Expand All @@ -182,7 +175,7 @@ public function testCheckAuthenticationWhenCredentialsAreNotValid()
->willReturn('foo')
;

$method->invoke($provider, $this->getMockBuilder('Symfony\\Component\\Security\\Core\\User\\UserInterface')->getMock(), $token);
$method->invoke($provider, new TestUser(), $token);
}

/**
Expand Down Expand Up @@ -256,7 +249,7 @@ public function testCheckAuthentication()
->willReturn('foo')
;

$method->invoke($provider, $this->getMockBuilder('Symfony\\Component\\Security\\Core\\User\\UserInterface')->getMock(), $token);
$method->invoke($provider, new TestUser(), $token);
}

protected function getSupportedToken()
Expand Down Expand Up @@ -299,3 +292,30 @@ protected function getProvider($user = null, $userChecker = null, $passwordEncod
return new DaoAuthenticationProvider($userProvider, $userChecker, 'key', $encoderFactory);
}
}

class TestUser implements UserInterface
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 UserInterface wasn't mocked properly in the tests: getPassword() returned null where it shouldn't. This caused problems with the new type-hints, so I switched to a dummy implementation instead of bloating the mocking code.

{
public function getRoles()
{
return [];
}

public function getPassword()
{
return 'secret';
}

public function getSalt()
{
return null;
}

public function getUsername()
{
return 'jane_doe';
}

public function eraseCredentials()
{
}
}
0