[DX] New service to simplify password encoding #11306

aferrandini · 2014-07-05T08:46:01Z

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 · 2014-07-05T09:03:11Z

src/Symfony/Component/Security/Core/Encoder/PasswordEncoderService.php

+
+        $encoded = $encoder->encodePassword($plainPassword, $user->getSalt());
+
+        return $encoded;


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

Nek- · 2014-07-05T10:05:00Z

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

aferrandini · 2014-07-05T10:17:59Z

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

aferrandini · 2014-07-05T10:28:33Z

@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 · 2014-07-05T10:29:22Z

👍 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 · 2014-07-05T10:30:50Z

src/Symfony/Component/Security/Core/Encoder/PasswordEncoderService.php

+ *
+ * @author Ariel Ferrandini <arielferrandini@gmail.com>
+ */
+class PasswordEncoderService


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

aferrandini · 2014-07-05T10:33:58Z

I forgot to implements the PasswordEncoderServiceInterface.

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

jakzal · 2014-07-05T10:36:08Z

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

How about calling the class UserPasswordEncoder?

aferrandini · 2014-07-05T10:38:16Z

@jakzal 👍 to the class name UserPasswordEncoder

aferrandini · 2014-07-05T10:56:22Z

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

weaverryan · 2014-07-05T12:38:40Z

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 · 2014-07-05T13:21:27Z

@waverryan of course 👍

aferrandini · 2014-07-05T14:49:13Z

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

weaverryan · 2014-07-05T15:08:33Z

@aferrandini you're on fire!

ggam · 2014-07-05T15:17:42Z

@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 · 2014-07-06T11:50:23Z

@ggam updated

fabpot · 2014-07-23T14:58:16Z

👍

fabpot · 2014-07-25T08:49:26Z

Thank you @aferrandini.

…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

…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

jakzal reviewed Jul 5, 2014
View reviewed changes

javiereguiluz added the DX label Jul 5, 2014

New service to simplify password encoding

7bc190a

aferrandini mentioned this pull request Jul 6, 2014

[DX] New service to simplify password encoding symfony/symfony-docs#3995

Merged

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


		$encoded = $encoder->encodePassword($plainPassword, $user->getSalt());

		return $encoded;

Uh oh!

[DX] New service to simplify password encoding #11306

[DX] New service to simplify password encoding #11306

Uh oh!

Conversation

aferrandini commented Jul 5, 2014

Uh oh!

jakzal Jul 5, 2014

Choose a reason for hiding this comment

Uh oh!

Nek- commented Jul 5, 2014

Uh oh!

aferrandini commented Jul 5, 2014

Uh oh!

aferrandini commented Jul 5, 2014

Uh oh!

jakzal commented Jul 5, 2014

Uh oh!

jakzal Jul 5, 2014

Choose a reason for hiding this comment

Uh oh!

aferrandini commented Jul 5, 2014

Uh oh!

jakzal commented Jul 5, 2014

Uh oh!

aferrandini commented Jul 5, 2014

Uh oh!

aferrandini commented Jul 5, 2014

Uh oh!

weaverryan commented Jul 5, 2014

Uh oh!

aferrandini commented Jul 5, 2014

Uh oh!

aferrandini commented Jul 5, 2014

Uh oh!

weaverryan commented Jul 5, 2014

Uh oh!

ggam commented Jul 5, 2014

Uh oh!

aferrandini commented Jul 6, 2014

Uh oh!

fabpot commented Jul 23, 2014

Uh oh!

fabpot commented Jul 25, 2014

Uh oh!

Uh oh!