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

[2.7][SecurityBundle] Added a command to encode a password #12818

Merged
merged 1 commit into from Mar 19, 2015

Conversation

Projects
None yet
@saro0h
Contributor

saro0h commented Dec 3, 2014

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #11206
License MIT
Doc PR ~
  • Give some love to the UI (need your feedbacks)
  • Write tests
  • Write documentation

The encoder got depend directly from the configuration of the user class in the security.yml.
So the user choose the user type, and, using the method getEncoder($user) of the security.encoder_factory service, I get the right encoder configured.

Here is the output for security:encode-password:
capture d ecran 2015-01-01 a 19 48 07

@saro0h saro0h changed the title from [WIP] Added a command to encode a password to [SecurityBundle][WIP] Added a command to encode a password Dec 3, 2014

$userClassQuestion->setValidator(function ($value) use ($output) {
if ('' === trim($value)) {
$value = 'Symfony\Component\Security\Core\User\User';

This comment has been minimized.

@stloyd

stloyd Dec 3, 2014

Contributor

IMHO this should be an class variable not "hidden" & hardcoded one...

@stloyd

stloyd Dec 3, 2014

Contributor

IMHO this should be an class variable not "hidden" & hardcoded one...

This comment has been minimized.

@saro0h

saro0h Dec 3, 2014

Contributor

What do you mean?

@saro0h

saro0h Dec 3, 2014

Contributor

What do you mean?

This comment has been minimized.

@stloyd

stloyd Dec 3, 2014

Contributor

With your approach even if you extend this class you need to replace whole method (copy&paste) to replace default class value...

@stloyd

stloyd Dec 3, 2014

Contributor

With your approach even if you extend this class you need to replace whole method (copy&paste) to replace default class value...

This comment has been minimized.

@hhamon

hhamon Dec 4, 2014

Contributor

@stloyd I can't understand what you're meaning! This method has been made private because there are really few chances someone wants to override it IMO.

@hhamon

hhamon Dec 4, 2014

Contributor

@stloyd I can't understand what you're meaning! This method has been made private because there are really few chances someone wants to override it IMO.

This comment has been minimized.

@fabpot

fabpot Mar 16, 2015

Member

@saro0h Would it be possible to load the configuration and propose a choice amongst the configure user classes? That would be much more easier for end users (typing a fully-qualified class name without a typo is hard on the CLI -- at least for me.)

@fabpot

fabpot Mar 16, 2015

Member

@saro0h Would it be possible to load the configuration and propose a choice amongst the configure user classes? That would be much more easier for end users (typing a fully-qualified class name without a typo is hard on the CLI -- at least for me.)

@fabpot fabpot added the Security label Dec 7, 2014

@saro0h saro0h changed the title from [SecurityBundle][WIP] Added a command to encode a password to [SecurityBundle Added a command to encode a password Dec 17, 2014

@saro0h

This comment has been minimized.

Show comment
Hide comment
@saro0h

saro0h Dec 17, 2014

Contributor

Ping @javiereguiluz : Is that what you expected?

Contributor

saro0h commented Dec 17, 2014

Ping @javiereguiluz : Is that what you expected?

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Dec 17, 2014

Member

@saro0h sorry for the late response. This is indeed what I expected, but I think we can simplify the user experience of the command. I find it confusing having to read so many text and it's also confusing to me the order of the questions.

The ida of this command is to provide a very simple and lightweight utility to encode plain text passwords using the appropriate encoding. The way passwords are encoded makes it almost "impossible" to encode the password by hand. This is necessary when you want to encode passwords for the "in_memory" provider and when you want to do checks of encoded passwords in the database.

This is how I imagined this command could work:

 Symfony Password Encoder Utility
 ================================

 > Select the encoder for which you want to encode the password:
   o Symfony\Component\Security\Core\User\User
   o ...

 > Type in your password (it won't be shown):
   ***********

 > (Optional) Type in the random salt (<Enter>):
   <Enter>

 [OK] The encoded password is:
 238ae5152rd77b787f78a7e7c723909fcd09090ea09908122332f23c32d23e43456a

In addition, if you only define one encoder, the command shouldn't ask you to pick one.

Member

javiereguiluz commented Dec 17, 2014

@saro0h sorry for the late response. This is indeed what I expected, but I think we can simplify the user experience of the command. I find it confusing having to read so many text and it's also confusing to me the order of the questions.

The ida of this command is to provide a very simple and lightweight utility to encode plain text passwords using the appropriate encoding. The way passwords are encoded makes it almost "impossible" to encode the password by hand. This is necessary when you want to encode passwords for the "in_memory" provider and when you want to do checks of encoded passwords in the database.

This is how I imagined this command could work:

 Symfony Password Encoder Utility
 ================================

 > Select the encoder for which you want to encode the password:
   o Symfony\Component\Security\Core\User\User
   o ...

 > Type in your password (it won't be shown):
   ***********

 > (Optional) Type in the random salt (<Enter>):
   <Enter>

 [OK] The encoded password is:
 238ae5152rd77b787f78a7e7c723909fcd09090ea09908122332f23c32d23e43456a

In addition, if you only define one encoder, the command shouldn't ask you to pick one.

@saro0h saro0h changed the title from [SecurityBundle Added a command to encode a password to [SecurityBundle] Added a command to encode a password Dec 17, 2014

@saro0h

This comment has been minimized.

Show comment
Hide comment
@saro0h

saro0h Dec 21, 2014

Contributor

@javiereguiluz Unfortunatly, I can't access the security.encoders parameter in the container… The user has to type the user class.

Contributor

saro0h commented Dec 21, 2014

@javiereguiluz Unfortunatly, I can't access the security.encoders parameter in the container… The user has to type the user class.

@saro0h

This comment has been minimized.

Show comment
Hide comment
@saro0h

saro0h Dec 21, 2014

Contributor

ping @fabpot : to close this issue #11206

Contributor

saro0h commented Dec 21, 2014

ping @fabpot : to close this issue #11206

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Dec 23, 2014

Contributor

I like the idea, but why should we do a totaly "Symfony-coupled" implementation instead of console application ? This can be useful for a lot of PHP projects (and developpers).

Maybe can you check the minimal dependencies to provide this kind of stuff ? I you don't, I will :)

Contributor

mickaelandrieu commented Dec 23, 2014

I like the idea, but why should we do a totaly "Symfony-coupled" implementation instead of console application ? This can be useful for a lot of PHP projects (and developpers).

Maybe can you check the minimal dependencies to provide this kind of stuff ? I you don't, I will :)

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Dec 25, 2014

Member

@saro0h : I provided a similar command (but much less worked) in a separate bundle, and I faced the same issue about the security.encoders, which I cannot access from the container.
I didn't go further, as my own needs where limited, but can't you simply use the Yaml Parser in order to retrieve them from the security.yml ? (And provide an option to specify the location of the yaml file where they are described, if not the default location)

Member

ogizanagi commented Dec 25, 2014

@saro0h : I provided a similar command (but much less worked) in a separate bundle, and I faced the same issue about the security.encoders, which I cannot access from the container.
I didn't go further, as my own needs where limited, but can't you simply use the Yaml Parser in order to retrieve them from the security.yml ? (And provide an option to specify the location of the yaml file where they are described, if not the default location)

@hhamon

This comment has been minimized.

Show comment
Hide comment
@hhamon

hhamon Dec 25, 2014

Contributor

@mickaelandrieu which PHP Open-Source projects? Maybe Silex eventually. I don't think decoupling this command really worths it for now. It mostly serves Symfony users first. Maybe if there is a real need to get it extracted somewhere else, we will be able to do it.

@ogizanagi instead of parsing the YAML files, the easiest way is to rely on a global configuration parameter in the container. Remember the console is executed at "runtime" whereas configuration files like security.yml are processed and validated at "compile" (very first request) time. So, you're doing the same thing twice without validating the file if you do it in your command. Also, relying on the container allows to rely on an environment. Console commands are "environment aware" thanks to the global --env option.

Contributor

hhamon commented Dec 25, 2014

@mickaelandrieu which PHP Open-Source projects? Maybe Silex eventually. I don't think decoupling this command really worths it for now. It mostly serves Symfony users first. Maybe if there is a real need to get it extracted somewhere else, we will be able to do it.

@ogizanagi instead of parsing the YAML files, the easiest way is to rely on a global configuration parameter in the container. Remember the console is executed at "runtime" whereas configuration files like security.yml are processed and validated at "compile" (very first request) time. So, you're doing the same thing twice without validating the file if you do it in your command. Also, relying on the container allows to rely on an environment. Console commands are "environment aware" thanks to the global --env option.

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Dec 25, 2014

Member

@hhamon : Indeed, I completely ignored the environments in my suggestion. This was a bad idea...

instead of parsing the YAML files, the easiest way is to rely on a global configuration parameter in the container.

Should the SecurityExtension expose the $config['encoders'] as a global parameter in the container, then ? (Or should the EncoderFactory provide access to the currently private $encoders property through a dedicated method ?)

Member

ogizanagi commented Dec 25, 2014

@hhamon : Indeed, I completely ignored the environments in my suggestion. This was a bad idea...

instead of parsing the YAML files, the easiest way is to rely on a global configuration parameter in the container.

Should the SecurityExtension expose the $config['encoders'] as a global parameter in the container, then ? (Or should the EncoderFactory provide access to the currently private $encoders property through a dedicated method ?)

@saro0h

This comment has been minimized.

Show comment
Hide comment
@saro0h

saro0h Dec 25, 2014

Contributor

Nope it doesn't and I think it's a bad idea to expose it just for this command.

Contributor

saro0h commented Dec 25, 2014

Nope it doesn't and I think it's a bad idea to expose it just for this command.

@saro0h saro0h changed the title from [SecurityBundle] Added a command to encode a password to [2.7][SecurityBundle] Added a command to encode a password Dec 27, 2014

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Jan 1, 2015

Contributor

@hhamon Laravel also, the biggest PHP US framework for now :) But I understand your point: the way the command is used require a class who implements UserInterface. I did a cli who only need to set the encoder method and arguments (like cost for instance) and return the correct encoded password.

I don't understand why we need an User class to encode a password but it's ok for Symfony (framework) projects.

Contributor

mickaelandrieu commented Jan 1, 2015

@hhamon Laravel also, the biggest PHP US framework for now :) But I understand your point: the way the command is used require a class who implements UserInterface. I did a cli who only need to set the encoder method and arguments (like cost for instance) and return the correct encoded password.

I don't understand why we need an User class to encode a password but it's ok for Symfony (framework) projects.

@hhamon

This comment has been minimized.

Show comment
Hide comment
@hhamon

hhamon Jan 1, 2015

Contributor

Do Laravel users use the Security component?

Contributor

hhamon commented Jan 1, 2015

Do Laravel users use the Security component?

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Jan 1, 2015

Contributor

@hhamon yes : https://github.com/laravel/framework/blob/master/composer.json#L36

But I don't know well Laravel, I can't say if they use Symfony encoders to deal with passwords. For bcrypt, they have their own package called Hashing.

Contributor

mickaelandrieu commented Jan 1, 2015

@hhamon yes : https://github.com/laravel/framework/blob/master/composer.json#L36

But I don't know well Laravel, I can't say if they use Symfony encoders to deal with passwords. For bcrypt, they have their own package called Hashing.

According to the response you will give to the question "<question>Provide your configured user class</question>" your
password will be encoded the way it was configured.
- If you answer "<comment>Symfony\Component\Security\Core\User\User</comment>", the password provided will be encoded

This comment has been minimized.

@fabpot

fabpot Feb 5, 2015

Member

we should allow to pass the class name as an argument for a non-interactive usage.

@fabpot

fabpot Feb 5, 2015

Member

we should allow to pass the class name as an argument for a non-interactive usage.

This comment has been minimized.

@saro0h

saro0h Feb 6, 2015

Contributor

Sure, let me do that

@saro0h

saro0h Feb 6, 2015

Contributor

Sure, let me do that

@saro0h

This comment has been minimized.

Show comment
Hide comment
@saro0h

saro0h Feb 8, 2015

Contributor

Changes have been made.

Contributor

saro0h commented Feb 8, 2015

Changes have been made.

@hacfi

This comment has been minimized.

Show comment
Hide comment
@hacfi

hacfi Feb 9, 2015

Contributor

Is it possible to use null as salt? I think that was recommended previously for bcrypt.

Contributor

hacfi commented Feb 9, 2015

Is it possible to use null as salt? I think that was recommended previously for bcrypt.

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Feb 9, 2015

Member

BTW, passing the salt as an option should be allowed for non-interactive usage too, shouldn't it ?

Member

ogizanagi commented Feb 9, 2015

BTW, passing the salt as an option should be allowed for non-interactive usage too, shouldn't it ?

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu
Contributor

mickaelandrieu commented Feb 9, 2015

@ogizanagi 👍

@saro0h

This comment has been minimized.

Show comment
Hide comment
@saro0h

saro0h Feb 9, 2015

Contributor

Done.

Contributor

saro0h commented Feb 9, 2015

Done.

$userClassQuestion->setValidator(function ($value) use ($output) {
if ('' === trim($value)) {
$value = 'Symfony\Component\Security\Core\User\User';
$output->writeln("<info>You did not provide any user class.</info> <comment>The user class used is: Symfony\Component\Security\Core\User\User</comment> \n");

This comment has been minimized.

@jeremyFreeAgent

jeremyFreeAgent Feb 12, 2015

Contributor

What about a default value?

@jeremyFreeAgent

jeremyFreeAgent Feb 12, 2015

Contributor

What about a default value?

This comment has been minimized.

@saro0h

saro0h Feb 12, 2015

Contributor

I can't because, there is no way to access the security.encoders key to get all user classes configured.

@saro0h

saro0h Feb 12, 2015

Contributor

I can't because, there is no way to access the security.encoders key to get all user classes configured.

This comment has been minimized.

@ogizanagi

ogizanagi Feb 12, 2015

Member

I think he suggests to set Symfony\Component\Security\Core\User\User as the default value, no matter what user classes are configured under security.encoders.
I personally did this in my own implementation of this command, as I mainly use it for in_memory users.

@ogizanagi

ogizanagi Feb 12, 2015

Member

I think he suggests to set Symfony\Component\Security\Core\User\User as the default value, no matter what user classes are configured under security.encoders.
I personally did this in my own implementation of this command, as I mainly use it for in_memory users.

This comment has been minimized.

@jeremyFreeAgent

jeremyFreeAgent Feb 12, 2015

Contributor

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

@jeremyFreeAgent

jeremyFreeAgent Feb 12, 2015

Contributor

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

@saro0h

This comment has been minimized.

Show comment
Hide comment
@saro0h

saro0h Mar 15, 2015

Contributor

Ping @fabpot

Contributor

saro0h commented Mar 15, 2015

Ping @fabpot

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Mar 16, 2015

Member

👍 for the feature.

Member

lyrixx commented Mar 16, 2015

👍 for the feature.

@saro0h

This comment has been minimized.

Show comment
Hide comment
@saro0h

saro0h Mar 16, 2015

Contributor

ping @fabpot Done

Contributor

saro0h commented Mar 16, 2015

ping @fabpot Done

@saro0h

This comment has been minimized.

Show comment
Hide comment
@saro0h

saro0h Mar 17, 2015

Contributor

@fabpot Now I remember why I added the package ircmaxell/password-compat => https://travis-ci.org/symfony/symfony/jobs/54716343#L3250

Contributor

saro0h commented Mar 17, 2015

@fabpot Now I remember why I added the package ircmaxell/password-compat => https://travis-ci.org/symfony/symfony/jobs/54716343#L3250

@saro0h

This comment has been minimized.

Show comment
Hide comment
@saro0h

saro0h Mar 18, 2015

Contributor

Ping @fabpot modification done.

Contributor

saro0h commented Mar 18, 2015

Ping @fabpot modification done.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Mar 19, 2015

Member

Thank you @saro0h.

Member

fabpot commented Mar 19, 2015

Thank you @saro0h.

@fabpot fabpot merged commit a7bd0fc into symfony:2.7 Mar 19, 2015

1 of 2 checks passed

fabbot.io Doing some proofreading and checking your coding style.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

fabpot added a commit that referenced this pull request Mar 19, 2015

feature #12818 [2.7][SecurityBundle] Added a command to encode a pass…
…word (saro0h)

This PR was merged into the 2.7 branch.

Discussion
----------

[2.7][SecurityBundle] Added a command to encode a password

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

- [x] Give some love to the UI (need your feedbacks)
- [x] Write tests
- [x] Write documentation

-------------------------------------------------------

The encoder got depend directly from the configuration of the user class in the security.yml.
So the user choose the user type, and, using the method `getEncoder($user)` of the `security.encoder_factory` service, I get the right encoder configured.

*Here is the output for `security:encode-password`*:
![capture d ecran 2015-01-01 a 19 48 07](https://cloud.githubusercontent.com/assets/667519/5593008/3af45686-91ef-11e4-8024-e66a2e000fbe.png)

Commits
-------

a7bd0fc Added a command to encode a password
@saro0h

This comment has been minimized.

Show comment
Hide comment
@saro0h

saro0h Mar 19, 2015

Contributor

\o/

Contributor

saro0h commented Mar 19, 2015

\o/

@inanimatt

This comment has been minimized.

Show comment
Hide comment
@inanimatt

inanimatt Mar 22, 2015

Contributor

Why allow salt to be specified? In fact it's required if you don't want to give command line input? Traditionally people generate weaker salts than desirable and I feel like in a simple convenience method like this it's… well, inconvenient. If you need a specific salt, then perhaps that's sufficiently specialised that you could just do it yourself. As in, the command should hit the common use case and not worry about special cases especially when the cost for everyone else is a potentially weak salt.

If some of the encoders require one to be given instead of generated (I.e not bcrypt) then I'd still recommend changing the argument order: start with the class, then the password, then an optional salt which is generated if not provided. That'd be better DX imo.

Contributor

inanimatt commented on a7bd0fc Mar 22, 2015

Why allow salt to be specified? In fact it's required if you don't want to give command line input? Traditionally people generate weaker salts than desirable and I feel like in a simple convenience method like this it's… well, inconvenient. If you need a specific salt, then perhaps that's sufficiently specialised that you could just do it yourself. As in, the command should hit the common use case and not worry about special cases especially when the cost for everyone else is a potentially weak salt.

If some of the encoders require one to be given instead of generated (I.e not bcrypt) then I'd still recommend changing the argument order: start with the class, then the password, then an optional salt which is generated if not provided. That'd be better DX imo.

This comment has been minimized.

Show comment
Hide comment
@inanimatt

inanimatt Mar 22, 2015

Contributor

Also didn't mean to sound negative, this is a great idea and a useful contribution, thanks!

Contributor

inanimatt replied Mar 22, 2015

Also didn't mean to sound negative, this is a great idea and a useful contribution, thanks!

This comment has been minimized.

Show comment
Hide comment
@sstok

sstok Mar 22, 2015

Contributor
Contributor

sstok replied Mar 22, 2015

fabpot added a commit that referenced this pull request Mar 22, 2015

minor #13985 [SecurityBundle] UserPasswordEncoderCommand: fix help ar…
…guments order. (ogizanagi)

This PR was merged into the 2.7 branch.

Discussion
----------

[SecurityBundle] UserPasswordEncoderCommand: fix help arguments order.

| Q             | A
| ------------- | ---
| Fixed tickets | ✘
| License       | MIT

A very small fix about the newly introduced command to encode a user password (#12818).
The help message states the command looks like:
```
security:encode-password [password] [salt] [user-class]
```

but is the following instead:
```
security:encode-password [password] [user-class] [salt]
```

Commits
-------

0a5b1b9 [SecurityBundle] UserPasswordEncoderCommand: fix help arguments order.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment