Have I Been Pwned Integration (Password security enhancements) #832

[amosfolz]

This will need more work but I feel is at a place to open up for some feedback. The proposed feature adds two additional config values under site.password_security.

'password_security' => [
             'enforce_no_compromised'   => '5', // Set to false to turn off this feature. Otherwise, provide a numeric string, which sets the maximum number
                                       // of times that is "acceptable" for a password to have appeared in breaches. The recommended and most secure
                                       // option is '0' - meaning only passwords that are not on the list of compromised passwords will be allowed.

             'enforce_update_passwords' => true // Settings this to true will check user's passwords against list of known compromised passwords at each log on.
]

(I am open to suggestions on the naming convention for these).

[codecov[bot]]

Codecov Report

Merging #974 (a5432d6) into develop (c92bf0b) will decrease coverage by 0.12%. The diff coverage is 58.06%.

@@              Coverage Diff              @@
##             develop     #974      +/-   ##
=============================================
- Coverage      67.76%   67.64%   -0.13%     
- Complexity      1997     2021      +24     
=============================================
  Files            169      171       +2     
  Lines           6995     7086      +91     
=============================================
+ Hits            4740     4793      +53     
- Misses          2255     2293      +38     

Impacted Files	Coverage Δ	Complexity Δ
.../src/Database/Migrations/v430/UpdateUsersTable.php	100.00% <ø> (ø)	3.00 <0.00> (ø)
...nkles/account/src/Controller/AccountController.php	95.59% <25.92%> (-4.41%)	92.00 <2.00> (+6.00)	:arrow_down:
.../account/src/ServicesProvider/ServicesProvider.php	77.86% <44.44%> (-1.96%)	11.00 <0.00> (+1.00)	:arrow_down:
...kles/account/src/Authenticate/PasswordSecurity.php	70.21% <70.21%> (ø)	14.00 <14.00> (?)
...grations/v430/AddPasswordResetColumnUsersTable.php	100.00% <100.00%> (ø)	3.00 <3.00> (?)
...account/src/Repository/PasswordResetRepository.php	100.00% <100.00%> (ø)	1.00 <0.00> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c92bf0b...a5432d6. Read the comment docs.

[amosfolz]

The controller is really long... While having private function in the controller is not wrong, I think it would make more sense to put both checkPassword and checkHash in a separate (static) class? Would make easier to use those methods elsewhere as well as writing proper tests.

I thought they would probably need to be moved. I will need some guidance on where to move them to...

[Silic0nS0ldier]

Looks like there are some changes on master that aren't on develop. Probably want to sync it up. Would make the diff clearer.

[Silic0nS0ldier]

I've added some comments (but neglected to use the code review function, good job me). Mainly some concerns around return types and potential for script errors.

[amosfolz]

As discussed in #993, The config should be site => password => security

This is updated.

[amosfolz]

This is ready for review.

I have changed the process. If a password is found on the list of compromised passwords flag_password_reset_required is set for that user. This flag can easily be set using additional methods in the future and should allow for customization. For example, if we wanted to add capability for admin to force a password reset for a user.

A check was added to the login process. If a user has flag_password_reset_required they will be denied login until their password is reset. (They are directed to a similar page as forgot-password so they can enter their email and begin the password reset)

[lcharette]

For example, if we wanted to add capability for admin to force a password reset for a user.

༼ つ ◕_◕ ༽つ

[Silic0nS0ldier]

The experience around the (root) admin account being on a compromised passwords list could use some work. Namely, the admin is not forced to perform a password reset to login despite the recorded user activity stating that they have to. Probably not great that the account with the most access isn't covered by these additional protections.

[amosfolz]

The experience around the (root) admin account being on a compromised passwords list could use some work. Namely, the admin is not forced to perform a password reset to login despite the recorded user activity stating that they have to. Probably not great that the account with the most access isn't covered by these additional protections.

Good catch! This is because the if that checks if flag_password_reset_required is set was using an instance of currentUser from prior to that field being set. (so it wasn't enforcing the change until the next login.) This should be fixed now.

[amosfolz]

I believe this is ready for another review.

[lcharette]

FYI, I tagged this to 4.4.x as I think it will require more testing from both @Silic0nS0ldier and I.

[Silic0nS0ldier]

As can be seen, I'm trying to resurrect this PR nearly 2 years on. Its been synced with master and had some issues resolved (mainly PHP crashes relating to recently completed deprecations and type violations).

[lcharette]

Closing this, as it need to be updated for UF5. It should be implement as it's own sprinkle anyway.