search

silverstripe / cwp-core

CWP basic compatibility module
BSD 3-Clause "New" or "Revised" License
3 stars 12 forks source link

move PasswordValidator configuration into _config #53

Closed NightJar closed 5 years ago

NightJar commented 5 years ago

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

robbieaverill commented 5 years ago

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

brynwhyman commented 5 years ago

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

NightJar commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 :)