Skip to content

[Form] Add hash_property_path option to PasswordType #46224

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 22, 2022
Merged

[Form] Add hash_property_path option to PasswordType #46224

merged 1 commit into from
Oct 22, 2022

Conversation

Seb33300
Copy link
Contributor

@Seb33300 Seb33300 commented May 1, 2022 •
edited
Loading

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.

@Seb33300 Seb33300 requested review from xabbuh and yceruto as code owners May 1, 2022 13:38
@carsonbot carsonbot added this to the 6.1 milestone May 1, 2022
@Seb33300 Seb33300 mentioned this pull request May 1, 2022
Closed
@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 requested review from wouterj and chalasr as code owners 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

@fabpot fabpot modified the milestones: 6.1, 6.2 May 8, 2022
@Seb33300 Seb33300 force-pushed the password-hash-mapping-extension branch 5 times, most recently from 88e4fd2 to 1f7b15f Compare May 31, 2022 14:18
@javiereguiluz javiereguiluz mentioned this pull request Jun 10, 2022
Merged
@Seb33300 Seb33300 mentioned this pull request Jul 24, 2022
Closed
@javiereguiluz
Copy link
Member

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

yceruto
yceruto requested changes Oct 14, 2022
View reviewed changes
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
View reviewed changes
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
@wouterj
feature #15872 [Form] Document the hash_property_path option (Seb33…
beaa793 
…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
Merged
This was referenced May 7, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wouterj wouterj wouterj requested changes

@fabpot fabpot fabpot approved these changes

@yceruto yceruto yceruto approved these changes

@chalasr chalasr chalasr approved these changes

@xabbuh xabbuh Awaiting requested review from xabbuh xabbuh is a code owner

Assignees
No one assigned
Labels
Feature Form ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Reviewed
Projects
None yet
Milestone
6.2
Development

Successfully merging this pull request may close these issues.

Auto Password Encoder
8 participants
@Seb33300 @carsonbot @javiereguiluz @chalasr @yceruto @fabpot @wouterj @nicolas-grekas