details: https://code.openbravo.com/erp/devel/pi/rev/e32f0aa03825
changeset: 34096:e32f0aa03825
user: Asier Lostalé <asier.lostale <at> openbravo.com>
date: Tue May 29 16:18:01 2018 +0200
summary: fixes 38651: problems in user locking implementation
Fixes some problems in user locking:
1. When defining an incremental delay for login, it is only possible to set
ranges of integer seconds. Increment by 1 second on each failed attempt
is too much: it should be possible to define this increment to something
smaller than complete seconds.
2. After a failed login attempt, login response is delayed. While in this
delay, a database connection is kept open. It would be better to return
it to the pool and get another one afterwards.
3. When trying to log in with a non existing user, delay is also correctly
applied. The query count number of failed attempts is checking from the
beginning of the time. It should check if there was any attempt during
the
last one day at most.
4. After a user is locked, subsequent login attempts mark it as locked
again.
diffstat:
src/org/openbravo/base/secureApp/LoginUtils.java | 2 +-
src/org/openbravo/base/secureApp/UserLock.java | 70 ++++++++++++-----------
2 files changed, 38 insertions(+), 34 deletions(-)
diffs (157 lines):
diff -r d2cc1590b1a1 -r e32f0aa03825
src/org/openbravo/base/secureApp/LoginUtils.java
--- a/src/org/openbravo/base/secureApp/LoginUtils.java Tue May 29 00:27:14
2018 +0530
+++ b/src/org/openbravo/base/secureApp/LoginUtils.java Tue May 29 16:18:01
2018 +0200
@@ -79,7 +79,7 @@
public static String getValidUserId(ConnectionProvider connectionProvider,
String login,
String unHashedPassword) {
try {
- // Deley response and check for locked user
+ // Delay response and check for locked user
UserLock lockSettings = new UserLock(login);
lockSettings.delayResponse();
if (lockSettings.isLockedUser()) {
diff -r d2cc1590b1a1 -r e32f0aa03825
src/org/openbravo/base/secureApp/UserLock.java
--- a/src/org/openbravo/base/secureApp/UserLock.java Tue May 29 00:27:14
2018 +0530
+++ b/src/org/openbravo/base/secureApp/UserLock.java Tue May 29 16:18:01
2018 +0200
@@ -11,7 +11,7 @@
* under the License.
* The Original Code is Openbravo ERP.
* The Initial Developer of the Original Code is Openbravo SL
- * All portions are Copyright (C) 2010-2017 Openbravo SL
+ * All portions are Copyright (C) 2010-2018 Openbravo SL
* All Rights Reserved.
* Contributor(s): ______________________________________.
************************************************************************
@@ -19,8 +19,10 @@
package org.openbravo.base.secureApp;
+import java.util.Calendar;
import java.util.Date;
import java.util.Properties;
+import java.util.concurrent.TimeUnit;
import org.apache.log4j.Logger;
import org.hibernate.Query;
@@ -38,12 +40,12 @@
public class UserLock {
private static Logger log4j = Logger.getLogger(UserLock.class);
- private static int delayInc;
+ private static float delayInc;
private static int delayMax;
private static int lockAfterTrials;
private static boolean lockingConfigured;
- private int delay;
+ private float delay;
private String userName;
private int numberOfFails;
@@ -65,7 +67,7 @@
}
try {
- delayInc = Integer.parseInt(propInc);
+ delayInc = Float.parseFloat(propInc);
} catch (NumberFormatException e) {
log4j.error("Could not set login.trial.delay.increment property " +
propInc, e);
delayInc = 0;
@@ -96,7 +98,8 @@
// Count how many times this user has attempted to login without success
long t = System.currentTimeMillis();
- // to improve performance this query is not done as subquery of the main
one, see issue #25466
+ // to improve performance this query is not done as subquery of the main
one,
+ // see issue #25466
StringBuilder hql = new StringBuilder();
hql.append("select max(s1.creationDate)");
hql.append(" from ADSession s1");
@@ -105,6 +108,12 @@
Query q1 = OBDal.getInstance().getSession().createQuery(hql.toString());
q1.setParameter("name", userName);
Date lastFailedAttempt = (Date) q1.list().get(0);
+
+ if (lastFailedAttempt == null) {
+ Calendar yesterday = Calendar.getInstance();
+ yesterday.add(Calendar.DATE, -1);
+ lastFailedAttempt = yesterday.getTime();
+ }
log4j.debug("Time taken to check user lock 1st query " +
(System.currentTimeMillis() - t));
long t1 = System.currentTimeMillis();
@@ -113,16 +122,11 @@
hql.append(" from ADSession s ");
hql.append(" where s.loginStatus='F'");
hql.append(" and s.username = :name");
- if (lastFailedAttempt != null) {
- hql.append(" and s.creationDate > :lastFail");
- } else {
- hql.append(" and s.creationDate > s.creationDate-1");
- }
+ hql.append(" and s.creationDate > :lastFail");
+
Query q = OBDal.getInstance().getSession().createQuery(hql.toString());
q.setParameter("name", userName);
- if (lastFailedAttempt != null) {
- q.setParameter("lastFail", lastFailedAttempt);
- }
+ q.setParameter("lastFail", lastFailedAttempt);
numberOfFails = ((Long) q.list().get(0)).intValue();
log4j.debug("Time taken to check user lock " + (System.currentTimeMillis()
- t)
@@ -166,20 +170,17 @@
boolean lockUser = (lockAfterTrials != 0) && (numberOfFails >=
lockAfterTrials);
log4j.debug("lock: " + lockUser + " -lock after:" + lockAfterTrials + "-
fails:"
+ numberOfFails + " - user:" + user);
- if (lockUser) {
- // Try to lock the user in database
- delay = 0;
- if (user != null) {
- try {
- OBContext.setAdminMode();
+ if (lockUser && user != null && !user.isLocked()) {
+ try {
+ OBContext.setAdminMode();
- user.setLocked(true);
- OBDal.getInstance().flush();
- log4j.warn(userName + " is locked after " + numberOfFails + " failed
logins.");
- return;
- } finally {
- OBContext.restorePreviousMode();
- }
+ // re-attach user to session as it was detached in commit and close
+ user = OBDal.getInstance().get(User.class, user.getId());
+ user.setLocked(true);
+ OBDal.getInstance().flush();
+ log4j.warn(userName + " is locked after " + numberOfFails + " failed
logins.");
+ } finally {
+ OBContext.restorePreviousMode();
}
}
}
@@ -204,13 +205,16 @@
* attempts failed.
*/
public void delayResponse() {
- if (delay > 0) {
- log4j.debug("Delaying response " + delay + " seconds because of the
previous login failed.");
- try {
- Thread.sleep(delay * 1000);
- } catch (InterruptedException e) {
- log4j.error("Error delaying login response", e);
- }
+ if (delay == 0) {
+ return;
+ }
+ // release DB connection while delaying response
+ OBDal.getInstance().commitAndClose();
+ log4j.debug("Delaying response " + delay + " seconds because of the
previous login failed.");
+ try {
+ TimeUnit.MILLISECONDS.sleep((long) (delay * 1_000L));
+ } catch (InterruptedException e) {
+ log4j.error("Error delaying login response", e);
}
}
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Openbravo-commits mailing list
Openbravo-commits@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openbravo-commits
