Skip to content
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

[Form] Add hash_property_path option to PasswordType #46224

Merged
merged 1 commit into from
Oct 22, 2022

Conversation

Seb33300
Copy link
Contributor

@Seb33300 Seb33300 commented May 1, 2022

Q A
Branch? 6.2
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #29066
License MIT
Doc PR symfony/symfony-docs#15872

Same as #42883 but using a Form Extension and rebased to 6.1 & tests.

This PR adds a new hash_mapping option to PasswordType.
The hash_mapping option can be set with a property path where we want to set the hashed password.
The hash_mapping option can only be used on unmapped fields to minimize plain password leak.

@carsonbot carsonbot added this to the 6.1 milestone May 1, 2022
@Seb33300 Seb33300 force-pushed the password-hash-mapping-extension branch 5 times, most recently from 2724b72 to 785c113 Compare May 1, 2022 16:03
@Seb33300 Seb33300 force-pushed the password-hash-mapping-extension branch 5 times, most recently from a8304ce to a8c9dbb Compare May 2, 2022 09:15
@carsonbot
Copy link

Hey!

I think @michaelKaefer has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@javiereguiluz
Copy link
Member

@wouterj or @chalasr what do you think of this proposal? Thanks!

Copy link
Member

@yceruto yceruto left a comment

Choose a reason for hiding this comment

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

This approach seems better than the previous one. Thanks for the second try!

@Seb33300 Seb33300 force-pushed the password-hash-mapping-extension branch from ef8b110 to ffe9830 Compare October 17, 2022 12:01
@Seb33300 Seb33300 force-pushed the password-hash-mapping-extension branch from 5afae70 to 533142b Compare October 18, 2022 14:14
@nicolas-grekas nicolas-grekas added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 19, 2022
yceruto
yceruto previously approved these changes Oct 20, 2022
Copy link
Member

@yceruto yceruto left a comment

Choose a reason for hiding this comment

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

LGTM.

@yceruto
Copy link
Member

yceruto commented Oct 20, 2022

Some tests failures seem to be related https://github.com/symfony/symfony/actions/runs/3274200868/jobs/5387417225#step:7:2208

can you check?

@yceruto yceruto dismissed their stale review October 20, 2022 11:37

Tests failing

@Seb33300
Copy link
Contributor Author

Fixed in e765a9b

All green now 🚀
Thanks for your review 🙏🏻

@chalasr chalasr force-pushed the password-hash-mapping-extension branch from 88f6c8b to 9c06ab2 Compare October 20, 2022 18:52
@Seb33300 Seb33300 force-pushed the password-hash-mapping-extension branch from 9c06ab2 to 56c1282 Compare October 21, 2022 02:43
@Seb33300 Seb33300 force-pushed the password-hash-mapping-extension branch from 56c1282 to 7065dfe Compare October 21, 2022 03:46
@Seb33300
Copy link
Contributor Author

@chalasr I force pushed to change !$parentForm || !($user = $parentForm->getData()) into !($user = $parentForm?->getData()) and fix static analysis related to undefined $user

@fabpot
Copy link
Member

fabpot commented Oct 22, 2022

Thank you @Seb33300.

@fabpot fabpot merged commit cf073c4 into symfony:6.2 Oct 22, 2022
wouterj added a commit to symfony/symfony-docs that referenced this pull request Oct 23, 2022
…300)

This PR was squashed before being merged into the 6.2 branch.

Discussion
----------

[Form] Document the `hash_property_path` option

Documentation for symfony/symfony#46224

Commits
-------

228d73e [Form] Document the `hash_property_path` option
@Seb33300 Seb33300 deleted the password-hash-mapping-extension branch October 24, 2022 05:46
@fabpot fabpot mentioned this pull request Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Form ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto Password Encoder
8 participants