Skip to content

[Security] add PasswordEncoderInterface::needsRehash() #31594

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

Merged
merged 1 commit into from
Jun 4, 2019

Conversation

nicolas-grekas
Copy link
Member

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

Split from #31153, with tests.

Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Could be worth an UPGRADE entry as not implementing them will break in 5.0

@nicolas-grekas nicolas-grekas force-pushed the sec-needs-rehash branch 3 times, most recently from d91d302 to 25c197b Compare May 24, 2019 08:02
@nicolas-grekas nicolas-grekas changed the base branch from master to 4.4 June 2, 2019 20:02
@nicolas-grekas
Copy link
Member Author

friendly ping @symfony/deciders

/**
* {@inheritdoc}
*/
public function needsRehash(string $encoded): bool
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a check for BCrypt passwords too?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's deprecated so I don't think we should improve it further.

Copy link
Contributor

@Tobion Tobion left a comment

Choose a reason for hiding this comment

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

Agree with this. It's best practice to have the hash contain the algo, salt, and options and then determine based on this if a rehash is needed.
But going this way contradics the old way of doing that using a salt parameter and UserPasswordEncoderInterface. So I think we should deprecate

@chalasr
Copy link
Member

chalasr commented Jun 4, 2019

Thank you @nicolas-grekas.

@chalasr chalasr merged commit 50590dc into symfony:4.4 Jun 4, 2019
chalasr pushed a commit that referenced this pull request Jun 4, 2019
… (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[Security] add PasswordEncoderInterface::needsRehash()

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

Split from #31153, with tests.

Commits
-------

50590dc [Security] add PasswordEncoderInterface::needsRehash()
@nicolas-grekas nicolas-grekas deleted the sec-needs-rehash branch June 4, 2019 06:29
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
This was referenced Nov 12, 2019
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