[DX] New service to simplify password encoding #11306

Merged
merged 1 commit into from Jul 25, 2014

7 participants

@aferrandini
Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #11299
License MIT
Doc PR symfony/symfony-docs#3995

This new service siplifies the way to encode a password. Just get the security.password_encoder service and encode the User password.

$encoded = $this->container->get('security.password_encoder')
    ->encodePassword($user, $plainPassword);

$user->setPassword($encoded);
@jakzal jakzal commented on an outdated diff Jul 5, 2014
...nent/Security/Core/Encoder/PasswordEncoderService.php
+ */
+ public function __construct (EncoderFactoryInterface $encoderFactory)
+ {
+ $this->encoderFactory = $encoderFactory;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function encodePassword (UserInterface $user, $plainPassword)
+ {
+ $encoder = $this->encoderFactory->getEncoder($user);
+
+ $encoded = $encoder->encodePassword($plainPassword, $user->getSalt());
+
+ return $encoded;
@jakzal
Symfony member
jakzal added a note Jul 5, 2014

You could make it more consistent and avoid assignment, just like in the isPasswordValid().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jakzal jakzal commented on an outdated diff Jul 5, 2014
...nent/Security/Core/Encoder/PasswordEncoderService.php
+ private $encoderFactory;
+
+ /**
+ * Constructor
+ *
+ * @param EncoderFactoryInterface $encoderFactory The encoder factory
+ */
+ public function __construct (EncoderFactoryInterface $encoderFactory)
+ {
+ $this->encoderFactory = $encoderFactory;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function encodePassword (UserInterface $user, $plainPassword)
@jakzal
Symfony member
jakzal added a note Jul 5, 2014

Remove the space before ( (also the case in few other places, run php-cs-fixer).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jakzal jakzal commented on an outdated diff Jul 5, 2014
...ity/Core/Tests/Encoder/PasswordEncoderServiceTest.php
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Security\Core\Tests\Encoder;
+
+
+use Symfony\Component\Security\Core\Encoder\PasswordEncoderService;
+
+class PasswordEncoderServiceTest extends \PHPUnit_Framework_TestCase
+{
+ public function testEncodePassword ()
+ {
+ $userMock = $this->getMock('Symfony\Component\Security\Core\User\UserInterface');
+ $userMock->expects($this->once())
@jakzal
Symfony member
jakzal added a note Jul 5, 2014

All the $this->once() should actually be $this->any(). We don't really care if these methods were called, since we're stubbing here (as opposed to mocking).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Nek-

So you don't think that this comment #11299 (comment) is a great idea ? (just not repeat "password")

@aferrandini

@jakzal I've update your comments. Please review it and tell me if I have to change anything else.

@aferrandini

@javiereguiluz we could have both methods, one for validate user password as you mention before and another to validate a password, due to could be a password not stored in the user object.

public function isPasswordValid(UserInterface $user, $encoded, $raw);

public function isUserPasswordValid(UserInterface $user, $raw);
@jakzal
Symfony member

👍 for @javiereguiluz comment, password is redundant.

@Nek- I think security.password_encoder is a name that reflects the serivce's purpose bettern than security.encoder (what kind of an encoder?). Then, the method names (encodePassword, isPasswordValid) are inline with the encoder factory, so probably what most users would expect.

@jakzal jakzal commented on an outdated diff Jul 5, 2014
...nent/Security/Core/Encoder/PasswordEncoderService.php
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Security\Core\Encoder;
+
+
+use Symfony\Component\Security\Core\User\UserInterface;
+
+/**
+ * A generic password encoder
+ *
+ * @author Ariel Ferrandini <arielferrandini@gmail.com>
+ */
+class PasswordEncoderService
@jakzal
Symfony member
jakzal added a note Jul 5, 2014

PasswordEncoder would be better. We don't use the Service suffix (it doesn't bring much value).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jakzal jakzal commented on an outdated diff Jul 5, 2014
...nent/Security/Core/Encoder/PasswordEncoderService.php
+use Symfony\Component\Security\Core\User\UserInterface;
+
+/**
+ * A generic password encoder
+ *
+ * @author Ariel Ferrandini <arielferrandini@gmail.com>
+ */
+class PasswordEncoderService
+{
+ /**
+ * @var EncoderFactoryInterface
+ */
+ private $encoderFactory;
+
+ /**
+ * Constructor
@jakzal
Symfony member
jakzal added a note Jul 5, 2014

Remove this please. It's clear the method is a constructor ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@aferrandini

I forgot to implements the PasswordEncoderServiceInterface.

@jakzal I added the Service prefix in order to prevent collision with PasswordEncoderInterface in the same namespace.

@jakzal
Symfony member

Do we need an interface here? It's a helper class and is unlikely to be overriden.

How about calling the class UserPasswordEncoder?

@javiereguiluz javiereguiluz added the DX label Jul 5, 2014
@aferrandini

@jakzal 👍 to the class name UserPasswordEncoder

@aferrandini aferrandini New service to simplify password encoding
7bc190a
@aferrandini

@jakzal changes made, and also passed the php-cs-fixer hope everything it's ok now

@weaverryan
Symfony member

I like this implementation. @aferrandini we should also get a PR (or at least an issue) on symfony/symfony-docs for the changes (which will shorten things!). Can you create one of those?

@aferrandini

@waverryan of course 👍

@aferrandini

@weaverryan I added a PR to the symfony/symfony-docs repo with documentation.

@weaverryan
Symfony member

@aferrandini you're on fire!

@ggam

@aferrandini docs link point to the current repository (symfony/symfony). I think you should enter the full url https://github.com/symfony/symfony-docs/pull/3995

@aferrandini aferrandini referenced this pull request in symfony/symfony-docs Jul 6, 2014
Merged

[DX] New service to simplify password encoding #3995

@aferrandini

@ggam updated

@fabpot
Symfony member

👍

@fabpot
Symfony member

Thank you @aferrandini.

@fabpot fabpot merged commit 7bc190a into symfony:master Jul 25, 2014

2 checks passed

Details continuous-integration/travis-ci The Travis CI build passed
Details default Success: Travis, fabbot
@fabpot fabpot added a commit that referenced this pull request Jul 25, 2014
@fabpot fabpot feature #11306 [DX] New service to simplify password encoding (aferra…
…ndini)

This PR was merged into the 2.6-dev branch.

Discussion
----------

[DX] New service to simplify password encoding

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #11299
| License       | MIT
| Doc PR | symfony/symfony-docs#3995

This new service siplifies the way to encode a password. Just get the `security.password_encoder` service and encode the `User` password.

```php
$encoded = $this->container->get('security.password_encoder')
    ->encodePassword($user, $plainPassword);

$user->setPassword($encoded);
```

Commits
-------

7bc190a New service to simplify password encoding
eb48706
@weaverryan weaverryan added a commit to symfony/symfony-docs that referenced this pull request Sep 1, 2014
@weaverryan weaverryan feature #3995 [DX] New service to simplify password encoding (aferran…
…dini)

This PR was merged into the master branch.

Discussion
----------

[DX] New service to simplify password encoding

| Q             | A
| ------------- | ---
| Doc fix?      | no
| New docs?     | [#11306](symfony/symfony#11306)
| Applies to    | 2.6

Add documentation for symfony/symfony PR [#11306](symfony/symfony#11306)

New service to simplify password encoding.

Commits
-------

785827f New service to simplify password encoding
9de96e6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment