[ChangePassword] Fixed code after code review.
authorJonathan Bueno <jonathan.bueno@openbravo.com>
Thu, 28 Jan 2016 10:58:07 +0100
changeset 28667 99dee8bbab26
parent 28666 d1a02b344ff9
child 28668 8289713e0e69
[ChangePassword] Fixed code after code review.

Username is not cleaned in case of invalid user/password.
Added check in LoginHandler to avoid security problem when changing password.
Deleted unused params in getUpdatePasswordDate method.
Login param changed to username in getUpdatePasswordDate method.
Deleted UserLock delay because has been already done (getUpdatePasswordDate and updatePassword methods).
Updated javadoc in getUpdatePasswordDate method.
Corrections in OBCriteria, using eq instead of like, using property of User PROPERTY_USERNAME.
Refactor in Dates in getUpdatePasswordDate method.
Deleted comments.
Now gets the pasword value from password field instead of user field (Both have the same content when updating the password).
Removed strange vars names like strUser and strPass.
setAdminMode and restorePreviousMode moved to updatePassword method.
Replaced vars.getLanguage() with language var where possible.
Updated javadoc in updatePassword method.
getUpdatePasswordDate method moved to DefaultAuthenticationManager and changed visibility to private.
updatePassword method moved to LoginHandler and changed visibility to private.
AuthenticationExpiryPasswordException is now a subclass of authentication exception.
Strings compared with === in js(Login.html).
Using single quotes '' for js strings.
src/org/openbravo/authentication/AuthenticationExpiryPasswordException.java
src/org/openbravo/authentication/basic/DefaultAuthenticationManager.java
src/org/openbravo/base/secureApp/LoginHandler.java
src/org/openbravo/base/secureApp/LoginUtils.java
src/org/openbravo/erpCommon/security/Login.html
--- a/src/org/openbravo/authentication/AuthenticationExpiryPasswordException.java	Fri Jan 22 12:22:19 2016 +0100
+++ b/src/org/openbravo/authentication/AuthenticationExpiryPasswordException.java	Thu Jan 28 10:58:07 2016 +0100
@@ -12,7 +12,6 @@
 
 package org.openbravo.authentication;
 
-import org.openbravo.base.exception.OBException;
 import org.openbravo.erpCommon.utility.OBError;
 
 /**
@@ -20,7 +19,7 @@
  * last update password date for user plus validity days defined for client has been reached
  * 
  */
-public class AuthenticationExpiryPasswordException extends OBException {
+public class AuthenticationExpiryPasswordException extends AuthenticationException {
   private static final long serialVersionUID = 1L;
   private OBError error;
 
--- a/src/org/openbravo/authentication/basic/DefaultAuthenticationManager.java	Fri Jan 22 12:22:19 2016 +0100
+++ b/src/org/openbravo/authentication/basic/DefaultAuthenticationManager.java	Thu Jan 28 10:58:07 2016 +0100
@@ -13,6 +13,7 @@
 package org.openbravo.authentication.basic;
 
 import java.io.IOException;
+import java.util.Calendar;
 import java.util.Date;
 
 import javax.servlet.ServletException;
@@ -22,6 +23,7 @@
 
 import org.apache.commons.lang.StringUtils;
 import org.apache.log4j.Logger;
+import org.hibernate.criterion.Restrictions;
 import org.openbravo.authentication.AuthenticationException;
 import org.openbravo.authentication.AuthenticationExpiryPasswordException;
 import org.openbravo.authentication.AuthenticationManager;
@@ -29,8 +31,11 @@
 import org.openbravo.base.secureApp.LoginUtils;
 import org.openbravo.base.secureApp.VariablesHistory;
 import org.openbravo.base.secureApp.VariablesSecureApp;
+import org.openbravo.dal.service.OBCriteria;
+import org.openbravo.dal.service.OBDal;
 import org.openbravo.erpCommon.utility.OBError;
 import org.openbravo.erpCommon.utility.Utility;
+import org.openbravo.model.ad.access.User;
 
 /**
  * 
@@ -69,57 +74,54 @@
     }
 
     VariablesHistory variables = new VariablesHistory(request);
-    final String strUser;
-    final String strPass;
+    final String user;
+    final String pass;
     // Begins code related to login process
     if (resetPassword) {
-      strUser = vars.getStringParameter("loggedUser");
+      user = vars.getStringParameter("loggedUser");
     } else {
-      strUser = vars.getStringParameter("user");
+      user = vars.getStringParameter("user");
     }
-    strPass = vars.getStringParameter("password");
-    username = strUser;
-    if (StringUtils.isEmpty(strUser)) {
+    pass = vars.getStringParameter("password");
+    username = user;
+    if (StringUtils.isEmpty(user)) {
       // redirects to the menu or the menu with the target
       setTargetInfoInVariables(request, variables);
       return null; // just give up, return null
     }
 
-    final String userId = LoginUtils.getValidUserId(conn, strUser, strPass);
-    final String sessionId = createDBSession(request, strUser, userId);
+    final String userId = LoginUtils.getValidUserId(conn, user, pass);
+    final String sessionId = createDBSession(request, user, userId);
 
     if (userId == null) {
 
       OBError errorMsg = new OBError();
       errorMsg.setType("Error");
 
-      if (LoginUtils.checkUserPassword(conn, strUser, strPass) == null) {
-        log4j.debug("Failed user/password. Username: " + strUser + " - Session ID:" + sessionId);
+      if (LoginUtils.checkUserPassword(conn, user, pass) == null) {
+        log4j.debug("Failed user/password. Username: " + user + " - Session ID:" + sessionId);
         errorMsg.setTitle("IDENTIFICATION_FAILURE_TITLE");
         errorMsg.setMessage("IDENTIFICATION_FAILURE_MSG");
       } else {
-        log4j.debug(strUser + " is locked cannot activate session ID " + sessionId);
+        log4j.debug(user + " is locked cannot activate session ID " + sessionId);
         errorMsg.setTitle("LOCKED_USER_TITLE");
         errorMsg.setMessage("LOCKED_USER_MSG");
       }
 
-      // throw error message will be caught by LoginHandler
       throw new AuthenticationException("IDENTIFICATION_FAILURE_TITLE", errorMsg);
     }
-    // Check if password valid date is reached
-    Date lastUpdatePasswordDate = LoginUtils.getUpdatePasswordDate(conn, strUser, strPass);
+    Date lastUpdatePasswordDate = getUpdatePasswordDate(user);
 
     if (lastUpdatePasswordDate != null) {
 
-      // Checks if password
       Date today = new Date();
       if (lastUpdatePasswordDate.compareTo(today) <= 0) {
-        log4j.debug("Failed user/password. Username: " + strUser + " - Session ID:" + sessionId);
         OBError errorMsg = new OBError();
         errorMsg.setType("Error");
         errorMsg.setTitle("IDENTIFICATION_FAILURE_TITLE");
         errorMsg.setMessage("IDENTIFICATION_FAILURE_MSG");
         throw new AuthenticationExpiryPasswordException("IDENTIFICATION_FAILURE_TITLE", errorMsg);
+
       }
     }
 
@@ -168,4 +170,34 @@
       response.sendRedirect(HttpBaseUtils.getLocalAddress(request));
     }
   }
+
+  /**
+   * Returns the expiry password date from login and unHashedPassword parameters
+   * 
+   * @param username
+   *          the username
+   * @return the expiry password date or null in case validity days are not applicable
+   */
+  private static Date getUpdatePasswordDate(String username) {
+
+    Date total;
+
+    final OBCriteria<User> obc = OBDal.getInstance().createCriteria(User.class);
+    obc.setFilterOnReadableClients(false);
+    obc.setFilterOnReadableOrganization(false);
+    obc.add(Restrictions.eq(User.PROPERTY_USERNAME, username));
+    final User userOB = (User) obc.uniqueResult();
+    Date lastUpdateDate = userOB.getLastupdatepassworddate();
+    Long validityDays = userOB.getClient().getDaystopasswordexpiration();
+    if (validityDays != null && validityDays > 0) {
+      Calendar expirationDate = Calendar.getInstance();
+      expirationDate.setTimeInMillis(lastUpdateDate.getTime());
+      expirationDate.add(Calendar.DATE, validityDays.intValue());
+      total = expirationDate.getTime();
+      return total;
+    } else {
+      return null;
+    }
+
+  }
 }
--- a/src/org/openbravo/base/secureApp/LoginHandler.java	Fri Jan 22 12:22:19 2016 +0100
+++ b/src/org/openbravo/base/secureApp/LoginHandler.java	Thu Jan 28 10:58:07 2016 +0100
@@ -1,5 +1,6 @@
 /*
  ************************************************************************************
+
  * Copyright (C) 2001-2015 Openbravo S.L.U.
  * Licensed under the Apache Software License version 2.0
  * You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
@@ -21,11 +22,14 @@
 import org.apache.commons.lang.StringUtils;
 import org.codehaus.jettison.json.JSONException;
 import org.codehaus.jettison.json.JSONObject;
+import org.hibernate.criterion.Restrictions;
 import org.openbravo.authentication.AuthenticationException;
 import org.openbravo.authentication.AuthenticationExpiryPasswordException;
 import org.openbravo.authentication.AuthenticationManager;
 import org.openbravo.base.HttpBaseServlet;
+import org.openbravo.base.exception.OBException;
 import org.openbravo.dal.core.OBContext;
+import org.openbravo.dal.service.OBCriteria;
 import org.openbravo.dal.service.OBDal;
 import org.openbravo.erpCommon.obps.ActivationKey;
 import org.openbravo.erpCommon.obps.ActivationKey.LicenseRestriction;
@@ -39,6 +43,7 @@
 import org.openbravo.model.ad.module.Module;
 import org.openbravo.model.ad.system.Client;
 import org.openbravo.model.ad.system.SystemInformation;
+import org.openbravo.utils.FormatUtilities;
 import org.openbravo.xmlEngine.XmlDocument;
 
 /**
@@ -71,17 +76,16 @@
     vars.setSessionObject("#loggingIn", "Y");
     final Boolean resetPassword = Boolean.parseBoolean(vars.getStringParameter("resetPassword"));
     Boolean sameOldPassword = false;
-    final String strUser;
-    final String strPass;
+    final String user;
+    final String password;
     if (resetPassword) {
-      strPass = vars.getStringParameter("user");
-      strUser = vars.getStringParameter("loggedUser");
-      OBContext.setAdminMode();
-      sameOldPassword = LoginUtils.updatePassword(strUser, strPass);
-
-      OBContext.restorePreviousMode();
+      password = vars.getStringParameter("password");
+      user = vars.getStringParameter("loggedUser");
+      if (!vars.getSessionValue("#AD_Session_ID").equalsIgnoreCase("")) {
+        sameOldPassword = updatePassword(user, password);
+      }
     } else {
-      strUser = vars.getStringParameter("user");
+      user = vars.getStringParameter("user");
     }
 
     // When redirect parameter is true, instead of returning a json object with the login result and
@@ -96,19 +100,20 @@
 
       String language = systemClient.getLanguage().getLanguage();
 
-      if (strUser.equals("") && !OBVersion.getInstance().is30()) {
+      if (user.equals("") && !OBVersion.getInstance().is30()) {
         res.sendRedirect(res.encodeRedirectURL(strDireccion + "/security/Login_F1.html"));
       } else {
         try {
           if (sameOldPassword) {
             OBError errorMsg = new OBError();
-            String msg = Utility.messageBD(myPool, "CPUpdatePassword", vars.getLanguage());
-            String title = Utility.messageBD(myPool, "CPDifferentPassword", vars.getLanguage());
+            String msg = Utility.messageBD(myPool, "CPUpdatePassword", language);
+            String title = Utility.messageBD(myPool, "CPDifferentPassword", language);
             errorMsg.setType("Error");
             errorMsg.setTitle(title);
             errorMsg.setMessage(msg);
+
             throw new AuthenticationExpiryPasswordException(Utility.messageBD(myPool,
-                "CPSamePasswordThanOld", vars.getLanguage()), errorMsg);
+                "CPSamePasswordThanOld", language), errorMsg);
           }
           AuthenticationManager authManager = AuthenticationManager.getAuthenticationManager(this);
 
@@ -118,8 +123,26 @@
           if (StringUtils.isEmpty(strUserAuth)) {
             throw new AuthenticationException("Message");// FIXME
           }
-          checkLicenseAndGo(res, vars, strUserAuth, strUser, sessionId, doRedirect);
+          checkLicenseAndGo(res, vars, strUserAuth, user, sessionId, doRedirect);
 
+        } catch (AuthenticationExpiryPasswordException aepe) {
+
+          final OBError errorMsg = aepe.getOBError();
+          if (errorMsg != null) {
+            vars.removeSessionValue("#LoginErrorMsg");
+
+            if (errorMsg.getMessage().equalsIgnoreCase("Write a new password")) {
+              String msg = Utility.messageBD(myPool, "CPUpdatePassword", language);
+              String title = Utility.messageBD(myPool, "CPDifferentPassword", language);
+              goToUpdatePassword(res, vars, msg, title, "Error", "../security/Login_FS.html",
+                  doRedirect);
+            } else {
+              String msg = Utility.messageBD(myPool, "CPUpdatePassword", language);
+              String title = Utility.messageBD(myPool, "CPExpiryPassword", language);
+              goToUpdatePassword(res, vars, msg, title, "Error", "../security/Login_FS.html",
+                  doRedirect);
+            }
+          }
         } catch (AuthenticationException e) {
 
           final OBError errorMsg = e.getOBError();
@@ -136,24 +159,6 @@
           } else {
             throw new ServletException("Error"); // FIXME
           }
-        } catch (AuthenticationExpiryPasswordException aepe) {
-
-          final OBError errorMsg = aepe.getOBError();
-          if (errorMsg != null) {
-            vars.removeSessionValue("#LoginErrorMsg");
-
-            if (errorMsg.getMessage().equalsIgnoreCase("Write a new password")) {
-              String msg = Utility.messageBD(myPool, "CPUpdatePassword", vars.getLanguage());
-              String title = Utility.messageBD(myPool, "CPDifferentPassword", vars.getLanguage());
-              goToUpdatePassword(res, vars, msg, title, "Error", "../security/Login_FS.html",
-                  doRedirect);
-            } else {
-              String msg = Utility.messageBD(myPool, "CPUpdatePassword", vars.getLanguage());
-              String title = Utility.messageBD(myPool, "CPExpiryPassword", vars.getLanguage());
-              goToUpdatePassword(res, vars, msg, title, "Error", "../security/Login_FS.html",
-                  doRedirect);
-            }
-          }
         }
       }
     } finally {
@@ -525,4 +530,41 @@
     return "User-login control Servlet";
   } // end of getServletInfo() method
 
+  /**
+   * Returns a boolean indicating if password has been changed for user. The password must be
+   * different from previous one, otherwise method will return false
+   * 
+   * 
+   * 
+   * @param username
+   *          the username
+   * @param unHashedPassword
+   *          the password, the unhashed password as it is entered by the user.
+   * @return false in case that password has changed succesfully in database, true in case password
+   *         were the same than the old one
+   */
+  private static Boolean updatePassword(String username, String unHashedPassword) {
+    try {
+      OBContext.setAdminMode();
+      final OBCriteria<User> obc = OBDal.getInstance().createCriteria(User.class);
+      obc.add(Restrictions.eq(User.PROPERTY_USERNAME, username));
+      obc.setFilterOnReadableClients(false);
+      final User userOB = (User) obc.uniqueResult();
+      String oldPassword = userOB.getPassword();
+      String newPassword = FormatUtilities.sha1Base64(unHashedPassword);
+      if (oldPassword.equals(newPassword)) {
+        return true;
+      } else {
+        userOB.setPassword(newPassword);
+        OBDal.getInstance().save(userOB);
+        OBDal.getInstance().flush();
+        OBDal.getInstance().commitAndClose();
+        return false;
+      }
+    } catch (final Exception e) {
+      throw new OBException(e);
+    } finally {
+      OBContext.restorePreviousMode();
+    }
+  }
 }
--- a/src/org/openbravo/base/secureApp/LoginUtils.java	Fri Jan 22 12:22:19 2016 +0100
+++ b/src/org/openbravo/base/secureApp/LoginUtils.java	Thu Jan 28 10:58:07 2016 +0100
@@ -12,12 +12,9 @@
 package org.openbravo.base.secureApp;
 
 import java.io.File;
-import java.util.Calendar;
-import java.util.Date;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.concurrent.TimeUnit;
 
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
@@ -25,12 +22,10 @@
 import javax.xml.parsers.DocumentBuilderFactory;
 
 import org.apache.log4j.Logger;
-import org.hibernate.criterion.Restrictions;
 import org.openbravo.base.HttpBaseUtils;
 import org.openbravo.base.exception.OBException;
 import org.openbravo.base.exception.OBSecurityException;
 import org.openbravo.dal.core.OBContext;
-import org.openbravo.dal.service.OBCriteria;
 import org.openbravo.dal.service.OBDal;
 import org.openbravo.dal.service.OBQuery;
 import org.openbravo.database.ConnectionProvider;
@@ -39,7 +34,6 @@
 import org.openbravo.erpCommon.utility.DimensionDisplayUtility;
 import org.openbravo.erpCommon.utility.Utility;
 import org.openbravo.model.ad.access.RoleOrganization;
-import org.openbravo.model.ad.access.User;
 import org.openbravo.model.ad.domain.Preference;
 import org.openbravo.model.ad.system.Client;
 import org.openbravo.service.db.DalConnectionProvider;
@@ -96,98 +90,6 @@
   }
 
   /**
-   * Returns the last update password date from login and unHashedPassword parameters
-   * 
-   * 
-   * 
-   * @param connectionProvider
-   *          , see the {@link DalConnectionProvider} for an instance of a ConnectionProvider for
-   *          the DAL.
-   * @param login
-   *          the login
-   * @param unHashedPassword
-   *          the password, the unhashed password as it is entered by the user.
-   * @return the last password update date from the user
-   */
-  public static Date getUpdatePasswordDate(ConnectionProvider connectionProvider, String login,
-      String unHashedPassword) {
-    // Gets the expiry password date
-    try {
-
-      UserLock lockSettings = new UserLock(login);
-      lockSettings.delayResponse();
-      if (lockSettings.isLockedUser()) {
-        return null;
-      }
-      Date total;
-
-      final OBCriteria<User> obc = OBDal.getInstance().createCriteria(User.class);
-      obc.add(Restrictions.like("username", login));
-      obc.setFilterOnReadableClients(false);
-      final List<User> listUser = obc.list();
-      User userOB = listUser.get(0);
-      Date lastUpdateDate = userOB.getLastupdatepassworddate();
-      Long validityDays = userOB.getClient().getDaystopasswordexpiration();
-      if (validityDays != null && validityDays > 0) {
-        Calendar currentDate = Calendar.getInstance();
-        currentDate
-            .setTimeInMillis(lastUpdateDate.getTime() + TimeUnit.DAYS.toMillis(validityDays));
-        total = new Date(currentDate.getTimeInMillis());
-        return total;
-      } else {
-        return null;
-      }
-    } catch (final Exception e) {
-      throw new OBException(e);
-    }
-
-  }
-
-  /**
-   * Returns a boolean indicating if password has been changed for user. The password must be
-   * different from previous one, otherwise method will return false
-   * 
-   * 
-   * 
-   * @param login
-   *          the login
-   * @param unHashedPassword
-   *          the password, the unhashed password as it is entered by the user.
-   * @return true in case that password has changed succesfully, false in other case
-   */
-  public static Boolean updatePassword(String login, String unHashedPassword) {
-    // Set the Updated password date
-    try {
-
-      UserLock lockSettings = new UserLock(login);
-      lockSettings.delayResponse();
-      if (lockSettings.isLockedUser()) {
-        return null;
-      }
-
-      final OBCriteria<User> obc = OBDal.getInstance().createCriteria(User.class);
-      obc.add(Restrictions.like("username", login));
-      obc.setFilterOnReadableClients(false);
-      final List<User> listUser = obc.list();
-      User userOB = listUser.get(0);
-      String oldPassword = userOB.getPassword();
-      String newPassword = FormatUtilities.sha1Base64(unHashedPassword);
-      if (oldPassword.equals(newPassword)) {
-
-        return true;
-      } else {
-        userOB.setPassword(newPassword);
-        OBDal.getInstance().save(userOB);
-        OBDal.getInstance().flush();
-        OBDal.getInstance().commitAndClose();
-        return false;
-      }
-    } catch (final Exception e) {
-      throw new OBException(e);
-    }
-  }
-
-  /**
    * Similar to {@link LoginUtils#getValidUserId(ConnectionProvider, String, String)} but not
    * blocking user accounts.
    * 
--- a/src/org/openbravo/erpCommon/security/Login.html	Fri Jan 22 12:22:19 2016 +0100
+++ b/src/org/openbravo/erpCommon/security/Login.html	Thu Jan 28 10:58:07 2016 +0100
@@ -231,7 +231,6 @@
       shouldContinue = setLoginMessage(result.messageType, result.messageTitle, result.messageText);
       if (!shouldContinue) {
         document.getElementById('password').value = '';
-        document.getElementById('user').value = '';
       }
     }
     if (result.resetPassword && document.getElementById('loggedUser').value===''){
@@ -256,6 +255,7 @@
       submitCommandForm(command, false, null, result.target, target, true);
     }else if (result.resetPassword){
       enableButton('buttonOK');
+      document.getElementById('user').value = '';
       setWindowElementFocus('user', 'id');
     } else {
       enableButton('buttonOK');