Skip to content

[SecurityBundle] Changed encoder configuration example to bcrypt #20301

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

Closed

Conversation

jeremyFreeAgent
Copy link
Contributor

Q A
Branch? "master"
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Simple change in the configuration example to help developers to not use sha512 as encoder when using config:dump-reference.

@@ -386,9 +386,9 @@ private function addEncodersSection(ArrayNodeDefinition $rootNode)
->children()
->arrayNode('encoders')
->example(array(
'Acme\DemoBundle\Entity\User1' => 'sha512',
'Acme\DemoBundle\Entity\User1' => 'bcrypt',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also change the deprecated 'Acme\DemoBundle\Entity\User1' example by 'AppBundle\Entity\User1' ? Thanks!

'Acme\DemoBundle\Entity\User2' => array(
'algorithm' => 'sha512',
'algorithm' => 'bcrypt',
'encode_as_base64' => 'true',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to remove encode_as_base64 and iterations ... but we could add cost

@jeremyFreeAgent
Copy link
Contributor Author

I made the changes.

@jeremyFreeAgent
Copy link
Contributor Author

status: needs review

Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Very nice improvements! Thanks @jeremyFreeAgent

@jeremyFreeAgent
Copy link
Contributor Author

What about changing the target branch to 2.7?

@HeahDude
Copy link
Contributor

Can be done on merge, it's up to you :)

@xabbuh
Copy link
Member

xabbuh commented Oct 30, 2016

👍

@fabpot
Copy link
Member

fabpot commented Oct 30, 2016

Thank you @jeremyFreeAgent.

fabpot added a commit that referenced this pull request Oct 30, 2016
…o bcrypt (jeremyFreeAgent)

This PR was submitted for the master branch but it was merged into the 2.7 branch instead (closes #20301).

Discussion
----------

[SecurityBundle] Changed encoder configuration example to bcrypt

| Q             | A
| ------------- | ---
| Branch?       | "master"
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Simple change in the configuration example to help developers to not use `sha512` as encoder when using `config:dump-reference`.

Commits
-------

a55058f [SecurityBundle] Changed encoder configuration example to bcrypt
@fabpot fabpot closed this Oct 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants