Fixed the ordering argument for the UserPasswordEncoderCommand by using options #14017

saro0h · 2015-03-22T16:32:53Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#13988
License	MIT
Doc PR	~

aitboudad · 2015-03-22T17:23:29Z

src/Symfony/Bundle/SecurityBundle/Command/UserPasswordEncoderCommand.php

@@ -85,14 +88,16 @@ protected function execute(InputInterface $input, OutputInterface $output)
            $password = $helper->ask($input, $output, $passwordQuestion);
        }

-        if (!$salt) {
+        if (!$salt && !$input->getOption('no-interaction')) {


I think you can use isInteractive here ?

if (!$salt) { if ($input->isInteractive()) { $saltQuestion = $this->createSaltQuestion($input, $output); $salt = $helper->ask($input, $output, $saltQuestion); } else { $salt = $this->generateSalt($output); } }

stof · 2015-03-22T17:44:33Z

src/Symfony/Bundle/SecurityBundle/Command/UserPasswordEncoderCommand.php

@@ -222,4 +227,14 @@ private function writeResult(OutputInterface $output)
            '',
        ));
    }
+
+    private function generateSalt(OutputInterface $output)


this needs to be public (but marked as @internal) to be compatible with PHP 5.3 as you call it from a closure.

ogizanagi · 2015-03-22T18:43:24Z

src/Symfony/Bundle/SecurityBundle/Command/UserPasswordEncoderCommand.php

        }

        $output->writeln("\n <comment>Encoders are configured by user type in the security.yml file.</comment>");

-        if (!$userClass) {
+        if (!$userClass && $input->isInteractive()) {
            $userClassQuestion = $this->createUserClassQuestion($input, $output);


This will never be reached now and can be removed, right ?

I should remove the class by default.

No. The point was that this question isn't even required for using this command.
The fact the symfony's core user class is the default one for the user-class option is good, as, anyway, it was the one provided by default with the autocompleter.
Now that the symfony's core user class is the default one, there is no issue while using the non-interactive command without providing a user-class, and the interactive command get rid of a pretty useless question.

I think the whole condition and the createUserClassQuestion method can be removed.

Sorry if I was unclear.

@ogizanagi is it really a useless question ? Each user class can use a separate encoder.

What I mean by "useless" is what I explained in #13988 :

Since we configure Symfony\Component\Security\Core\User\User in the setAutocompleterValues and set the $value to Symfony\Component\Security\Core\User\User if null, I guess we can configure the default value of the user-class.

Indeed, why not providing the Symfony\Component\Security\Core\User\User as the default value of the user-class option/argument ?
Asking it explicitly to the user is only useful for new symfony's users. Once understood, I think most users will provide it directly through the user-class option, or expect the default Symfony\Component\Security\Core\User\User value.

The only point of this question would be if the user was able to pick one encoder from a choice list. But as we cannot access the configured encoders, this question doesn't make any sense to me. :/

ogizanagi · 2015-03-22T20:44:30Z

src/Symfony/Bundle/SecurityBundle/Command/UserPasswordEncoderCommand.php

-        if (!$userClass) {
-            $userClassQuestion = $this->createUserClassQuestion($input, $output);
-            $userClass = $helper->ask($input, $output, $userClassQuestion);
+        if (!$userClass && $input->isInteractive()) {


Should be if (!$userClass) {

hhamon · 2015-03-23T23:39:10Z

src/Symfony/Bundle/SecurityBundle/Command/UserPasswordEncoderCommand.php

@@ -32,10 +33,10 @@ protected function configure()
    {
        $this
            ->setName('security:encode-password')
-            ->setDescription('Encode a password.')
+            ->setDescription('Encodes a password.')
            ->addArgument('password', InputArgument::OPTIONAL, 'Enter a password')


I would change the description to The raw password to encode.

saro0h · 2015-03-24T07:48:26Z

Changes have been made.

…ng options

saro0h · 2015-03-25T07:56:23Z

Ping @fabpot

xabbuh · 2015-03-30T20:47:02Z

@saro0h Did you close in favor of #14032?

@saro0h

…implify the command usage (ogizanagi) This PR was squashed before being merged into the 2.7 branch (closes #14032). Discussion ---------- [SecurityBundle] UserPasswordEncoderCommand: Improve & simplify the command usage | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #13988 | License | MIT Overlaps #14017 (might replace or follow it) . Sorry if it is considered as a duplicate, but the debate has evolved, and I think the arguments ordering isn't the best nor single way to improve this command usage anymore. Thank you @saro0h for having considered the mentioned issues and spent time on it. # Salt option & salt generation Thanks to @ircmaxell & @inanimatt, we came to the conclusion that the use-cases for the `salt` option/argument are pretty arguable. So I suggest to get rid of it: **a salt will always be generated by the command.** ### Generated salt The generated salt is now in the Table output: `security:encode-password test -n` ![screenshot 2015-03-30 a 21 38 21](https://cloud.githubusercontent.com/assets/2211145/6905081/d96f3ea4-d725-11e4-9b7c-83de8a75f28e.PNG) The "Generated salt" row and the last comment about the salt aren't present if the new `empty-salt` option is provided (see below). # New empty-salt option As some encoders might generate their own built-in salts (like the `BCryptPasswordEncoder`) and some custom encoders could do the same (or not require a salt at all), I suggest a new option: `empty-salt`. This option will not provide any salt to the configured encoder, which will generate its own, then. With the interactive way, the user will always be asked confirmation for the salt generation if the `empty-salt` option isn't set: `security:encode-password password` ![screenshot 2015-03-30 a 21 38 43](https://cloud.githubusercontent.com/assets/2211145/6905072/c53df984-d725-11e4-9a1a-81c3a363b5fe.PNG) ### bcrypt encoder As the `BCryptPasswordEncoder` is shipped with the security component, and listening to @inanimatt valuable comments in #13988, I introduced a second commit (0cdb546) making an exception for the `bcrypt` encoder, and always set the `empty-salt` option with it. We're aware that's not ideal from a OO design perspective, but far better from a DX one. If not desired, I will revert it. Anyway I think #13988 (comment) and other comments about the `PasswordEncoder` API and salt generation should be considered for 3.0. A note is added when bcrypt encoder is detected without the `empty-salt` option: `security:encode-password password "Custom\Class\Bcrypt\User" -n` ![screenshot 2015-03-30 a 19 46 36](https://cloud.githubusercontent.com/assets/2211145/6905066/b53edb52-d725-11e4-87e9-636bf177299a.PNG) # Default user-class The default `user-class` is set to `Symfony\Component\Security\Core\User\User`. I think this makes sense, as in the previous version of the command, the Symfony\Component\Security\Core\User\User was configured in the setAutocompleterValues and set the $value to Symfony\Component\Security\Core\User\User if null. Asking the question to the user with the interactive command would have been useful only if the user was able to pick one encoder from a choice list. # Arguments order / make arguments options When we had both `salt` and `user-class` as arguments, the command looked like: ```sh security:encode-password [password] [user-class] [salt] ``` All arguments were optional, for the interactive command purpose (The command asked the user for missing arguments). But, as they were arguments, we had to provide each of them in the proper order. That meant we couldn't provide a salt without defining the optional user-class. So I suggested using options instead of arguments for both user-class & salt. But as the `salt` option/argument is removed, now I don't feel the need for the `user-class` to be an option. Indeed, the new command short version will look like the following: ```sh #Default user-class: Symfony\Component\Security\Core\User\User security:encode-password password #Another user-class: security:encode-password password "AppBundle\Model\User" ``` Making the user-class an option IMO isn't a necessity anymore, and will only lengthen the command: ```sh security:encode-password password --user-class="AppBundle\Model\User" ``` ## Bonus: - [The new command documentation](https://cloud.githubusercontent.com/assets/2211145/6845201/48a66382-d3b2-11e4-8227-b799215a2783.PNG). Thanks to @javiereguiluz. - [Full interactivity output](https://cloud.githubusercontent.com/assets/2211145/6906381/d7753ce4-d72e-11e4-8547-2ef35c6257e9.PNG) Commits ------- b3f6340 [SecurityBundle] UserPasswordEncoderCommand: Improve & simplify the command usage

saro0h force-pushed the issue-13988 branch 2 times, most recently from e128f1c to 6df6563 Compare March 22, 2015 16:34

aitboudad reviewed Mar 22, 2015
View reviewed changes

aitboudad added the Security label Mar 22, 2015

saro0h force-pushed the issue-13988 branch 2 times, most recently from f71bb61 to b3f21cc Compare March 22, 2015 17:40

stof reviewed Mar 22, 2015
View reviewed changes

saro0h force-pushed the issue-13988 branch from b3f21cc to ae46b16 Compare March 22, 2015 18:17

ogizanagi reviewed Mar 22, 2015
View reviewed changes

saro0h force-pushed the issue-13988 branch 2 times, most recently from c70679d to 86a9c9c Compare March 22, 2015 19:57

ogizanagi reviewed Mar 22, 2015
View reviewed changes

saro0h force-pushed the issue-13988 branch from 86a9c9c to 31cacb6 Compare March 22, 2015 22:36

hhamon reviewed Mar 23, 2015
View reviewed changes

ogizanagi mentioned this pull request Mar 24, 2015

[SecurityBundle] UserPasswordEncoderCommand: Improve & simplify the command usage #14032

Closed

saro0h force-pushed the issue-13988 branch from 31cacb6 to 857654b Compare March 24, 2015 07:47

saro0h force-pushed the issue-13988 branch from 857654b to 212358e Compare March 24, 2015 07:55

Fixed the ordering argument for the UserPasswordEncoderCommand by usi…

caffbfb

…ng options

saro0h force-pushed the issue-13988 branch from 212358e to caffbfb Compare March 24, 2015 12:38

saro0h closed this Mar 30, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fixed the ordering argument for the UserPasswordEncoderCommand by using options #14017

Fixed the ordering argument for the UserPasswordEncoderCommand by using options #14017

Uh oh!

saro0h commented Mar 22, 2015

Uh oh!

aitboudad Mar 22, 2015

Uh oh!

stof Mar 22, 2015

Uh oh!

ogizanagi Mar 22, 2015

Uh oh!

saro0h Mar 22, 2015

Uh oh!

ogizanagi Mar 22, 2015

Uh oh!

stof Mar 22, 2015

Uh oh!

ogizanagi Mar 22, 2015

Uh oh!

ogizanagi Mar 22, 2015

Uh oh!

hhamon Mar 23, 2015

Uh oh!

saro0h commented Mar 24, 2015

Uh oh!

saro0h commented Mar 25, 2015

Uh oh!

xabbuh commented Mar 30, 2015

Uh oh!

Uh oh!

Uh oh!

Fixed the ordering argument for the UserPasswordEncoderCommand by using options #14017

Fixed the ordering argument for the UserPasswordEncoderCommand by using options #14017

Uh oh!

Conversation

saro0h commented Mar 22, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saro0h commented Mar 24, 2015

Uh oh!

saro0h commented Mar 25, 2015

Uh oh!

xabbuh commented Mar 30, 2015

Uh oh!

Uh oh!