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

ogizanagi · 2015-03-24T03:04:26Z

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

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

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

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:

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:

#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:

security:encode-password password --user-class="AppBundle\Model\User"

Bonus:

The new command documentation. Thanks to @javiereguiluz.
Full interactivity output

ogizanagi · 2015-03-26T01:56:52Z

@javiereguiluz : May I ask you what you think, at least about the new command output (according to command style guide) ?
Is this relevant enough given the #14017 PR and what we debated in #13988 ?

Ping @inanimatt :)

I'm pretty sure my tests aren't relevant, but I'm not used to. I even deleted the encoders specific static tests as I didn't know how to test them since the salt option no longer exists.
Any clue is valuable to me :)
I also tried to comply last @hhamon reviews on #14017

javiereguiluz · 2015-03-26T06:31:18Z

@ogizanagi honestly I think we should change everything. The output is very different from what we propose for Symfony commands. Sadly, I also find the text contents displayed by the command very verbose :(

ogizanagi · 2015-03-26T07:30:47Z

@javiereguiluz : Actually, I think you're right. I believe the command was meant to be used by symfony's newcomers for didactic purposes. Which explains the initial full interactivity and verbosity.
Even getting rid of useless questions, I only made the output heavier by adding those notes and salt generation confirmation... The help message itself is a big piece...
Maybe it should only be a very simple utility.

Should we keep, remove, or tweak:

In interactive mode:
- salt generation notice
- salt generation confirmation
any mode:
- command introduction
- bcrypt exception and notice
In the help:
- security.encoders example
- salt generation explanations
- non interactive description

javiereguiluz · 2015-03-26T07:34:02Z

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

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


We could chnage this description by something like: The raw password to encode

ogizanagi · 2015-03-26T11:43:49Z

Changes have been made. Thanks @javiereguiluz for your precious corrections & samples for the command description and @jrobeson & @inanimatt for having reminded what was said about the salt option before.

Should we at least make the generateSalt method protected in order to let users extend the command easily ? (Eventually give the encoder instance as first argument, if they have to handle multiple encoders in their application). I'm not really happy with this idea, but...

ogizanagi · 2015-03-30T18:18:18Z

@javiereguiluz : I didn't expect changing the output that much within this PR, only the behavior, but do you think the following command outputs are more compliant with your console style guide ? (I used the recently merged #14057 , but changes haven't been pushed for now)

I'll be glad to change the note about salt generation when using the interactive way if it doesn't please you, but I still think the user should be warn somehow (or explain the confirmation question context).

No interaction:

No password:

Bcrypt without empty salt option

Bcryp with empty-salt option

Interaction & other encoder

javiereguiluz · 2015-03-30T18:25:31Z

@ogizanagi wow! I love it! Now that the new console style guide is merged, I was planning to start updating the existing commands, but I'm glad that you already started with this command. My only suggestion: there is no need to add ✔ when the result is successful because we already provide the [OK] prefix and the color background.

ogizanagi · 2015-03-30T19:39:58Z

Changes pushed, then ! I will update the PR description and screenshots.

mattattui · 2015-03-30T20:21:05Z

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


-        $output->writeln("\n <comment>Encoders are configured by user type in the security.yml file.</comment>");
+            $output->note('The command will take care about generating a salt for you. Be aware that some encoders advise to let them generate their own salt. If you\'re using one of those encoders, please answer \'no\' to the question below. '.PHP_EOL.'Provide the \'empty-salt\' option in order to let the encoder handle the generation itself.');


Very minor point: "The command will generate a salt for you" would be more simple and more correct (otherwise "take care of") 😄

Thank you. I updated this right now. :)

mattattui · 2015-03-30T20:23:23Z

Nice work, @ogizanagi, it's very much improved. And thanks again (and sorry) to @saro0h for the initial work on this. We got there in the end :)

ogizanagi · 2015-03-30T20:33:54Z

Yes, I feel bad for having changed this much @saro0h's original work 😥
I hope it'll be for the best.

ogizanagi · 2015-04-02T00:22:21Z

What should be done about Travis failures in 56604789 and 56604790 ?
Sorry, I don't fully understand the impact of the deps env variable low and high values and the

Fatal error: Class 'Symfony\Component\Console\Style\SymfonyStyle' not found in /home/travis/build/symfony/symfony/src/Symfony/Bundle/SecurityBundle/Command/UserPasswordEncoderCommand.php on line 87

error that Travis reported.
If it relies on lowest components, this build doesn't matter for our case, right ?
But then, what's going on with high ? 3.0 ? Don't care since SymfonyStyle <- OutputStyle implements OutputInterface but not the 3.0 version of this interface for now ?

ogizanagi · 2015-04-04T09:48:18Z

ping @fabpot . I think this PR is ready for merging. Let me know if there is anything else to do (squash ?).

javiereguiluz · 2015-04-04T09:53:48Z

👍 indeed the code looks ready to be merged ... but what should we do with the Travis issues?

ogizanagi · 2015-04-05T16:34:00Z

@javiereguiluz : at least for 3.0 branch and deps=high env var, we'll need this PR (#14220) one day or another.
For deps=low, AFAIU, I think we can safely ignore this build.

nicolas-grekas · 2015-04-05T16:38:24Z

In some cases deps=high can be ignored. But deps=low should never ever be red before merging anything.
Well, at this very moment, the upstream is red so you have no choice I know.

…ommand usage - Remove the salt option. Always generate a salt. - Provide a short version by setting the default user-class to Symfony\Component\Security\Core\User\User. - Add an empty-salt option in order to let encoders generate their built-in salts (BCryptPasswordEncoder), or for custom encoders requiring no salt.

…out salt generation when using the interactive command

…enerate its built-in salt

…etected without empty-salt option

…with console style guide

fabpot · 2015-04-07T10:27:31Z

Thank you @ogizanagi.

…s methods (ogizanagi) This PR was merged into the 3.0-dev branch. Discussion ---------- [3.0][Console][OutputStyle] Implements verbosity levels methods | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A - Travis build for #14032 | License | MIT Implements the verbosity level methods from `OutputInterface` for the new `OutputStyle` abstract class in 3.0 branch. Fixes Travis builds with `deps=high` env var for #14032 PR and future console style updates. ========= Related to: #14032, #14057, #14138, #14132, #14172, ... Commits ------- a30f507 [Console] [OutputStyle] Implements verbosity methods

nicolas-grekas · 2015-04-07T11:10:18Z

@ogizanagi tests are failing on 2.7 after the merge, can you please provide a fixing patch asap?
See https://travis-ci.org/symfony/symfony/builds/57462093

ogizanagi · 2015-04-07T12:06:48Z

@nicolas-grekas : Sure. But I think the error comes from this commit: e9c7912

javiereguiluz reviewed Mar 26, 2015
View reviewed changes

mattattui reviewed Mar 30, 2015
View reviewed changes

xabbuh mentioned this pull request Mar 30, 2015

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

Closed

javiereguiluz mentioned this pull request Mar 31, 2015

Update the styles of all the built-in command consoles #14138

Closed

60 tasks

ogizanagi mentioned this pull request Apr 5, 2015

[3.0][Console][OutputStyle] Implements verbosity levels methods #14220

Merged

ogizanagi added 6 commits April 6, 2015 21:22

[SecurityBundle] UserPasswordEncoderCommand: ask user confirmation ab…

df4640d

…out salt generation when using the interactive command

[SecurityBundle] UserPasswordEncoderCommand: Let the bcrypt encoder g…

289de2f

…enerate its built-in salt

[SecurityBundle] UserPasswordEncoderCommand: Add a note when bcrypt d…

2a6abf7

…etected without empty-salt option

[SecurityBundle] UserPasswordEncoderCommand: Tweak output, complying …

b9374cc

…with console style guide

[SecurityBundle] updates symfony/console min version

42b17dc

fabpot closed this in 90a7fa0 Apr 7, 2015

ogizanagi mentioned this pull request Apr 7, 2015

Fixed the line length of the new Symfony Styles #14136

Closed

ogizanagi deleted the ticket_13988 branch April 11, 2015 09:04


		$output->writeln("\n <comment>Encoders are configured by user type in the security.yml file.</comment>");
		$output->note('The command will take care about generating a salt for you. Be aware that some encoders advise to let them generate their own salt. If you\'re using one of those encoders, please answer \'no\' to the question below. '.PHP_EOL.'Provide the \'empty-salt\' option in order to let the encoder handle the generation itself.');

Uh oh!

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

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

Uh oh!

Conversation

ogizanagi commented Mar 24, 2015

Salt option & salt generation

Generated salt

New empty-salt option

bcrypt encoder

Default user-class

Arguments order / make arguments options

Bonus:

Uh oh!

ogizanagi commented Mar 26, 2015

Uh oh!

javiereguiluz commented Mar 26, 2015

Uh oh!

ogizanagi commented Mar 26, 2015

Uh oh!

javiereguiluz Mar 26, 2015

Choose a reason for hiding this comment

Uh oh!

ogizanagi commented Mar 26, 2015

Uh oh!

ogizanagi commented Mar 30, 2015

No interaction:

No password:

Bcrypt without empty salt option

Bcryp with empty-salt option

Interaction & other encoder

Uh oh!

javiereguiluz commented Mar 30, 2015

Uh oh!

ogizanagi commented Mar 30, 2015

Uh oh!

mattattui Mar 30, 2015

Choose a reason for hiding this comment

Uh oh!

ogizanagi Mar 30, 2015

Choose a reason for hiding this comment

Uh oh!

mattattui commented Mar 30, 2015

Uh oh!

ogizanagi commented Mar 30, 2015

Uh oh!

ogizanagi commented Apr 2, 2015

Uh oh!

ogizanagi commented Apr 4, 2015

Uh oh!

javiereguiluz commented Apr 4, 2015

Uh oh!

ogizanagi commented Apr 5, 2015

Uh oh!

nicolas-grekas commented Apr 5, 2015

Uh oh!

fabpot commented Apr 7, 2015

Uh oh!

nicolas-grekas commented Apr 7, 2015

Uh oh!

ogizanagi commented Apr 7, 2015

Uh oh!

Uh oh!