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

Add User lockout and user unlock options #2897

Merged
merged 16 commits into from Jan 17, 2017

Conversation

JimMackin
Copy link
Contributor

Description

Provides admin settings for automatically locking users after a configurable amount of failed logins. Also provides admin users the ability to unlock locked users.
Also provides an admin setting for automatically unlocking users after a configurable amount of time.

Motivation and Context

Currently there is no limit to the number of login attempts that a user can make, this could in theory allow brute force attacks on the login screen. This PR allows an automated way of preventing this.

How To Test This

Setting the new maxfailedlogins config setting and deliberately failing to login, say, 3 times will prevent future logins until the user is unlocked on the user screen by an admin.
Setting the automaticunlocktime config setting however will unlock users x minutes after they are locked.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Final checklist

  • My code follows the code style of this project found here.
  • My change requires a change to the documentation.
  • I have read the How to Contribute guidelines.

@JimMackin
Copy link
Contributor Author

One thing to raise is that currently config settings are only saved if they already exist due to how the configurator works. This means that new installs will have the default config settings but upgrades may not be able to save the settings. We may need to add to the upgrade script to add these missing config settings.

Copy link
Member

@mattlorimer mattlorimer left a comment

Choose a reason for hiding this comment

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

Generally works well, a minor issue and few tweaks needed to be PSR 2 compliant

return;
}
$this->bean->setPreference('user_locked_out', false);
$this->bean->setPreference('user_locked_out_time', '');
Copy link
Member

Choose a reason for hiding this comment

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

needs to also reset the loginfailed preference

@@ -122,6 +122,11 @@ function preDisplay() {
}
}
}
if(is_admin($current_user)){
Copy link
Member

Choose a reason for hiding this comment

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

unnecessary should just be part of the above if statement which checks for the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The two checks above also check for if the user is editing their own record, this shouldn't matter since if the user is locked they wont be viewing their own record but seems safer to keep this check separate?

Copy link
Member

Choose a reason for hiding this comment

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

yes but your inside that elseif check, so that is right I am talking about the if statement on line 111

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, you're right. Fixed.

@@ -83,6 +83,21 @@ public function SugarAuthenticate(){
self::__construct();
}

private function isUserLockedOut(User $user){
Copy link
Member

Choose a reason for hiding this comment

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

Docblocks Comment missing

@@ -250,8 +250,41 @@
{/if}
</table>



<table id="userResetPassId" name="userResetPassName" width="100%" border="0" cellspacing="1" cellpadding="0" class="edit view">
Copy link
Member

Choose a reason for hiding this comment

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

shows even when LDAP or SAML are selected, should be hidden, unless this support them??

@@ -1270,5 +1270,9 @@
'LBL_AUTO_UNLOCK_TIME_UNITS' => 'minutes',
'ERR_MAX_FAILED_LOGINS' => 'Please specify a valid value for the number of max failed logins',
'ERR_AUTOMATIC_UNLOCK_TIME' => 'Please specify a valid value for the automatic unlock time',
'LBL_ENABLE_MAX_FAILED_LOGINS' => 'Lock users after a number of failed logins',
'LBL_ENABLE_MAX_FAILED_LOGINS_HELP' => '',
Copy link
Contributor

Choose a reason for hiding this comment

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

Empty language string!

@mattlorimer mattlorimer changed the base branch from hotfix-7.6.x to feature/user_lockout January 17, 2017 12:37
@mattlorimer mattlorimer merged commit 2f24cc1 into salesagility:feature/user_lockout Jan 17, 2017
@horus68
Copy link
Contributor

horus68 commented Nov 14, 2018

@Dillon-Brown I see this is merged in the feature branch. But what SuiteCRM release is this planned for?
Please consider also the related PR: #2953

@SinergiaCRM
Copy link
Contributor

Hi @mattlorimer,

We wanted to develop a solution similar to this one, but it looks there has been some validation work done here.

What is the status of this PR? Is it merging to master soon?

Although we haven't tested it yet, we could try to bring it up.

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.

None yet

4 participants