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

Fixes #3109 – Maxloginattempts lockout #3115

Merged
merged 2 commits into from Jun 2, 2018
Merged

Conversation

@Shade-
Copy link
Contributor

@Shade- Shade- commented Apr 14, 2018

This PR tries to solve #3109 by adapting the frontend code to the backend one, in which login attempts are saved in the database and a hard lockout is issued after a customizable number of failed attempts.

By doing so, a new column has been added (named loginlockoutexpiry, identical to the backend counterpart) to store lockout expiry dates. As an uid is not available before validating credentials, a soft lockout is set alongside the database one by setting a cookie with the expiry date, similarly to the past. This soft lockout has been extended to display in the modal too by adding 2 new templates (header_welcomeblock_guest_login_modal for the usual login modal and header_welcomeblock_guest_login_modal_lockout for the locked out one).

It worked fine in my tests, but as it vastly changes how the lockout system is designed, it might be worth a lot of independent tests as well.

@Shade- Shade- force-pushed the Shade-:fix-3109 branch from 66fc2d8 to 19ea5a2 Apr 14, 2018
@Ben-MyBB Ben-MyBB added the b:1.8 label Apr 22, 2018
@Shade- Shade- force-pushed the Shade-:fix-3109 branch from 482bd22 to be83739 May 6, 2018
@Eldenroot
Copy link
Contributor

@Eldenroot Eldenroot commented May 15, 2018

Works fine for me, but would be nice to have someone else who can confirm to be 100 % sure.

@euantorano
Copy link
Member

@euantorano euantorano commented May 19, 2018

@Shade- This is showing as being conflicted, any chance you can take a look?

@Shade-
Copy link
Contributor Author

@Shade- Shade- commented May 19, 2018

Sure, will do

@Shade- Shade- force-pushed the Shade-:fix-3109 branch from be83739 to 6f09833 May 20, 2018
Shade- added 2 commits Apr 14, 2018
…orce attacks
@Shade- Shade- removed the s:conflict label May 20, 2018
@Shade-
Copy link
Contributor Author

@Shade- Shade- commented May 20, 2018

Done

@@ -6358,92 +6357,100 @@ function get_inactive_forums()
* @param bool $fatal (Optional) Stop execution if it finds an error with the login. Default is True
* @return bool|int Number of logins when success, false if failed.
*/
function login_attempt_check($fatal = true)
function login_attempt_check($uid = 0, $fatal = true)

This comment has been minimized.

@euantorano

euantorano Jun 2, 2018
Member

I doubt many plugins use this function, but we should make a note that the function signature has changed in the release notes.

@euantorano euantorano merged commit 2697b32 into mybb:feature Jun 2, 2018
@euantorano
Copy link
Member

@euantorano euantorano commented Jun 2, 2018

This seems to work fine for me, but since it's changing a fairly critical code path, it could do with some extra testing on the test board @mybb/sqa

@Shade-
Copy link
Contributor Author

@Shade- Shade- commented Jun 2, 2018

Agreed. Got an exam in some days, and will have some days off to perform some testing afterwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants