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

move PasswordValidator configuration into _config #53

Conversation

NightJar
Copy link
Contributor

@NightJar NightJar commented Nov 5, 2018

and out of _config.php
i.e. use Injector rather than proceedural boot time code, as
SilverStripe 4 supports this (where 3 did not).
Re-introduce some security level settings that were lost at some point
with no clear reason as to why.

Fixes #52

_config/security.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

Yep I agree with @ScopeyNZ. The tests are good for your PR at least so you know your config is working, but there's no logic changes so no need for tests

_config/security.yml Outdated Show resolved Hide resolved
@robbieaverill
Copy link
Contributor

Also, do you want to put this into the 2.2 release?

@brynwhyman
Copy link
Contributor

@robbieaverill I'm keen for this to be added to the 2.2.0 release.

@NightJar
Copy link
Contributor Author

NightJar commented Nov 6, 2018

Indeed it appears to only be testing config settings, however that isn't the main goal of this minor test suite. The goal is more to catch 'regressions' should someone alter the values, given that the minimums tested here are a requirement for compliance. The tests should still pass if passwords are strengthened with more checks or higher character limits, for example.

The values were previously removed due to duplication. However on inspection I could not find where they were duplicated. I assume framework defaults - however I couldn't find where they were set there either. This is merely an extra layer of assurance.

E.g. the TestNames have no default in the core, and are not configurable. I didn't look too hard at mid-method fallbacks, but it seemed a logical conclusion to add this back in via the use of Injector as seen in the _config/sercurity.yml section of this PR.
To ensure this is set I run the test - not because it's not a config setting, but because it's also an Integration test - the PasswordValidator is always fetched via the way it's created in use (not directly with new or only with Injector via create).

This is my justification for adding the wee test suite.

@robbieaverill
Copy link
Contributor

@NightJar no worries, if you think it's worth keeping it then how about we compromise and you add the test to a @integration group so it can be identified with potentially other similar tests in future?

@NightJar
Copy link
Contributor Author

NightJar commented Nov 6, 2018

Sounds like a plan :)
I've been thinking about how to approach this (integration test business) on a wider scale for a while - this seems like a good start!


Also added a @compliance group, and the justification as above for future reference :)

@NightJar NightJar force-pushed the pulls/2.3/configure-passwords branch 2 times, most recently from 8401fed to efabff6 Compare November 6, 2018 22:58
and out of _config.php
i.e. use Injector rather than proceedural boot time code, as
SilverStripe 4 supports this (where 3 did not).
Re-introduce some security level settings that were lost at some point
with no clear reason as to why.
@NightJar NightJar force-pushed the pulls/2.3/configure-passwords branch from efabff6 to e9baf31 Compare November 7, 2018 02:05
@ScopeyNZ ScopeyNZ merged commit d3e3da3 into silverstripe:master Nov 7, 2018
@ScopeyNZ ScopeyNZ deleted the pulls/2.3/configure-passwords branch November 7, 2018 02:29
@NightJar NightJar removed their assignment Jan 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants