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

[Validator] Make API endpoint for NotCompromisedPasswordValidator configurable #31060

Conversation

@xelan
Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

xelan commented Apr 10, 2019

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

@xelan xelan force-pushed the xelan:feature/configurable-not-compromised-password-validator-api-endpoint branch from 098821b to 0627a80 Apr 10, 2019

xelan added some commits Apr 10, 2019

@xelan xelan force-pushed the xelan:feature/configurable-not-compromised-password-validator-api-endpoint branch from 0627a80 to c372b16 Apr 10, 2019

@xelan

This comment has been minimized.

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
Copy link
Member

nicolas-grekas left a comment

thanks

Show resolved Hide resolved src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php Outdated
->addDefaultsIfNotSet()
->info('not-compromised password validator configuration')
->children()
->booleanNode('enabled')

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 10, 2019

Member

can be replaced with canBeEnabled?

This comment has been minimized.

Copy link
@xelan

xelan Apr 10, 2019

Author Contributor

IMHO enabled is better, as the validator is enabled and ready to be used by default. Also @stof suggested this naming.

This comment has been minimized.

Copy link
@ro0NL

ro0NL Apr 11, 2019

Contributor

so canBeDisabled then? These methods add the enabled child node ...

Show resolved Hide resolved ...fony/Component/Validator/Constraints/NotCompromisedPasswordValidator.php Outdated
Show resolved Hide resolved src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php Outdated

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

@javiereguiluz javiereguiluz removed the BC Break label Apr 11, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Apr 11, 2019

What do you all think of GenuinePasswordValidator instead of NotCompromisedPasswordValidator?

@xelan

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

xelan commented Apr 12, 2019

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

This comment has been minimized.

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

This comment has been minimized.

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)

@xelan xelan force-pushed the xelan:feature/configurable-not-compromised-password-validator-api-endpoint branch from 9e07654 to dea39d8 Apr 15, 2019

@xelan

This comment has been minimized.

Copy link
Contributor Author

xelan commented Apr 15, 2019

Adapted, thanks for the feedback 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.