Skip to content

[Validator] Make API endpoint for NotCompromisedPasswordValidator configurable #31060

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
May 6, 2019
Merged

[Validator] Make API endpoint for NotCompromisedPasswordValidator configurable #31060

merged 1 commit into from
May 6, 2019

Conversation

xelan
Copy link
Contributor

@xelan xelan commented Apr 10, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? yes, but acceptable [1]
Deprecations? no [1]
Tests pass? yes
Fixed tickets #30871, #31054
License MIT
Doc PR symfony/symfony-docs#... (TODO)

Makes the API endpoint for the NotCompromisedPasswordValidator configurable. The endpoint includes the placeholder which will be replaced with the first digits of the password hash for k-anonymity.

The endpoint can either be set via constructor injection of the validator if the component is used standalone, or via the framework configuration of symfony/framework-bundle.

[1] As discussed in #31054, the validator is not in a stable release yet, therefore the BC break is considered acceptable. No deprecation / BC layer is necessary.

@xelan
Copy link
Contributor Author

xelan commented Apr 10, 2019

I'll create the Doc PR if the rest is ok.

stof
stof reviewed Apr 10, 2019
View reviewed changes
@xelan
Copy link
Contributor Author

xelan commented Apr 10, 2019

Thank you very much for your fast review and constructive feedback, @stof. I've integrated the changes you requested.

nicolas-grekas
nicolas-grekas reviewed Apr 10, 2019
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

thanks

@nicolas-grekas nicolas-grekas added this to the next milestone Apr 11, 2019
@nicolas-grekas
Copy link
Member

What do you all think of GenuinePasswordValidator instead of NotCompromisedPasswordValidator?

@xelan
Copy link
Contributor Author

xelan commented Apr 11, 2019

Would also be possible, however I think NotCompromisedPasswordValidator is better concerning the responsiblity of the validator. It uses a blacklist of hashes for known compromised passwords.

@xelan
Copy link
Contributor Author

xelan commented Apr 12, 2019 •
edited
Loading

An initial prototype of my HIBP-compatible password compromisation check server, based on a simple program to chunk the source data in buckets and some Apache 2.4 rewrite rules, is available here: https://github.com/xelan/sphynx

Feedback is welcome 😄

@xelan
Copy link
Contributor Author

xelan commented Apr 15, 2019

The requested changes concerning constant naming and documentation are incorporated. I'm still not sure about the section configuration canBeEnabled/canBeDisabled. How would you prefer to have it, @nicolas-grekas and @ro0NL? The advantage of canBeDisabled is that the section is enabled by default and the enabled key is automatically added but without a description (aka info()), right?

@stof
Copy link
Member

stof commented Apr 15, 2019

Well, I think in this case, having the info on the field is important to explain what will be the effect of disabling the validator (it won't prevent you to use the constraint, but will disable the validation)

nicolas-grekas
nicolas-grekas reviewed Apr 15, 2019
View reviewed changes
@xelan
Copy link
Contributor Author

xelan commented Apr 15, 2019

Adapted, thanks for the feedback 😄

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 May 5, 2019
nicolas-grekas
nicolas-grekas approved these changes May 5, 2019
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(with one minor comment)

@fabpot
Copy link
Member

fabpot commented May 6, 2019

Thank you @xelan.

@fabpot fabpot merged commit f6a80c2 into symfony:master May 6, 2019
fabpot added a commit that referenced this pull request May 6, 2019
@fabpot
feature #31060 [Validator] Make API endpoint for NotCompromisedPasswo…
ecfccc6 
…rdValidator configurable (xelan)

This PR was squashed before being merged into the 4.3-dev branch (closes #31060).

Discussion
----------

[Validator] Make API endpoint for NotCompromisedPasswordValidator configurable

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | yes, but acceptable [1]
| Deprecations? | no [1]
| Tests pass?   | yes
| Fixed tickets | #30871, #31054
| License       | MIT
| Doc PR        | symfony/symfony-docs#... (TODO)

Makes the API endpoint for the `NotCompromisedPasswordValidator` configurable. The endpoint includes the placeholder which will be replaced with the first digits of the password hash for k-anonymity.

The endpoint can either be set via constructor injection of the validator if the component is used standalone, or via the framework configuration of symfony/framework-bundle.

[1] As discussed in #31054, the validator is not in a stable release yet, therefore the BC break is considered acceptable. No deprecation / BC layer is necessary.

Commits
-------

f6a80c2 [Validator] Make API endpoint for NotCompromisedPasswordValidator configurable
@javiereguiluz javiereguiluz mentioned this pull request May 6, 2019
Closed
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request May 7, 2019
@javiereguiluz
minor #11527 Updated the configuration reference for NotCompromisedPa…
c20f346 
…ssword constraint (javiereguiluz)

This PR was squashed before being merged into the master branch (closes #11527).

Discussion
----------

Updated the configuration reference for NotCompromisedPassword constraint

Documents the changes made in symfony/symfony#31060

Commits
-------

f5b081e Updated the configuration reference for NotCompromisedPassword constraint
@fabpot fabpot mentioned this pull request May 9, 2019
Merged
@ogizanagi ogizanagi mentioned this pull request May 31, 2019
Merged
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request May 31, 2019
@javiereguiluz
minor #11634 Fix validation.not_compromised_password option default…
ed1a67a 
… (ogizanagi)

This PR was merged into the 4.3 branch.

Discussion
----------

Fix `validation.not_compromised_password` option default

Reflecting symfony/symfony#31060 changes in the docs.

Commits
-------

35925da Fix `validation.not_compromised_password` option default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javiereguiluz javiereguiluz javiereguiluz left review comments

@stof stof stof left review comments

@ro0NL ro0NL ro0NL left review comments

@xabbuh xabbuh xabbuh left review comments

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@maxhelias maxhelias maxhelias approved these changes

Assignees
No one assigned
Labels
Feature Status: Reviewed Validator
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
9 participants
@xelan @nicolas-grekas @stof @fabpot @javiereguiluz @ro0NL @xabbuh @maxhelias @carsonbot