[ChangePassword] Code review applied.
authorJonathan Bueno <jonathan.bueno@openbravo.com>
Fri, 12 Feb 2016 11:41:37 +0100
changeset 28802 25d249e69d3f
parent 28801 60fde1b7c191
child 28803 d5385407e873
[ChangePassword] Code review applied.

Solved camel case in Days To Password Expiration column.
Labels in Login.html are now translatable.
Last Password Update column is now updated in case user didn't have a password and is set for the first time.
Messages are now set correctly from the beginning and are not overwritten.
Refactor code, now check for expiration date is made in the method, exception is thrown in the method also.
AuthenticationExpirationPasswordException now log a single line.
src-db/database/model/triggers/AD_USER_EXPIRYPASS_TRG.xml
src-db/database/sourcedata/AD_COLUMN.xml
src-db/database/sourcedata/AD_ELEMENT.xml
src/org/openbravo/authentication/AuthenticationException.java
src/org/openbravo/authentication/AuthenticationExpirationPasswordException.java
src/org/openbravo/authentication/basic/DefaultAuthenticationManager.java
src/org/openbravo/base/secureApp/LoginHandler.java
src/org/openbravo/erpCommon/security/Login.html
--- a/src-db/database/model/triggers/AD_USER_EXPIRYPASS_TRG.xml	Tue Feb 09 09:45:56 2016 +0100
+++ b/src-db/database/model/triggers/AD_USER_EXPIRYPASS_TRG.xml	Fri Feb 12 11:41:37 2016 +0100
@@ -25,7 +25,7 @@
    IF AD_isTriggerEnabled()='N' THEN RETURN;
    END IF;
 
-   IF (:OLD.password<>:NEW.password) THEN
+   IF (:OLD.password<>:NEW.password OR :OLD.password IS NULL AND :NEW.password IS NOT NULL) THEN
      
      :NEW.lastpasswordupdate := now();
      
--- a/src-db/database/sourcedata/AD_COLUMN.xml	Tue Feb 09 09:45:56 2016 +0100
+++ b/src-db/database/sourcedata/AD_COLUMN.xml	Fri Feb 12 11:41:37 2016 +0100
@@ -238544,10 +238544,10 @@
 <!--06EF3D56611445C4BAC1A2ACE23AD94A-->  <AD_CLIENT_ID><![CDATA[0]]></AD_CLIENT_ID>
 <!--06EF3D56611445C4BAC1A2ACE23AD94A-->  <AD_ORG_ID><![CDATA[0]]></AD_ORG_ID>
 <!--06EF3D56611445C4BAC1A2ACE23AD94A-->  <ISACTIVE><![CDATA[Y]]></ISACTIVE>
-<!--06EF3D56611445C4BAC1A2ACE23AD94A-->  <NAME><![CDATA[Daystopasswordexpiration]]></NAME>
+<!--06EF3D56611445C4BAC1A2ACE23AD94A-->  <NAME><![CDATA[DaysToPasswordExpiration]]></NAME>
 <!--06EF3D56611445C4BAC1A2ACE23AD94A-->  <DESCRIPTION><![CDATA[Define the days that user password must be valid from previous password change]]></DESCRIPTION>
 <!--06EF3D56611445C4BAC1A2ACE23AD94A-->  <HELP><![CDATA[Define the days that user password must be valid from previous password change, if value is set to 0 no update will be required, for value greater than 0, password must be updated when the last password update date of the user plus the days defined here achieves. It will affect to all users depending the client.]]></HELP>
-<!--06EF3D56611445C4BAC1A2ACE23AD94A-->  <COLUMNNAME><![CDATA[Daystopasswordexpiration]]></COLUMNNAME>
+<!--06EF3D56611445C4BAC1A2ACE23AD94A-->  <COLUMNNAME><![CDATA[DaysToPasswordExpiration]]></COLUMNNAME>
 <!--06EF3D56611445C4BAC1A2ACE23AD94A-->  <AD_TABLE_ID><![CDATA[112]]></AD_TABLE_ID>
 <!--06EF3D56611445C4BAC1A2ACE23AD94A-->  <AD_REFERENCE_ID><![CDATA[11]]></AD_REFERENCE_ID>
 <!--06EF3D56611445C4BAC1A2ACE23AD94A-->  <FIELDLENGTH><![CDATA[12]]></FIELDLENGTH>
--- a/src-db/database/sourcedata/AD_ELEMENT.xml	Tue Feb 09 09:45:56 2016 +0100
+++ b/src-db/database/sourcedata/AD_ELEMENT.xml	Fri Feb 12 11:41:37 2016 +0100
@@ -21005,7 +21005,7 @@
 <!--02F1D370B8B34FB79D151811C705B200-->  <AD_CLIENT_ID><![CDATA[0]]></AD_CLIENT_ID>
 <!--02F1D370B8B34FB79D151811C705B200-->  <AD_ORG_ID><![CDATA[0]]></AD_ORG_ID>
 <!--02F1D370B8B34FB79D151811C705B200-->  <ISACTIVE><![CDATA[Y]]></ISACTIVE>
-<!--02F1D370B8B34FB79D151811C705B200-->  <COLUMNNAME><![CDATA[Daystopasswordexpiration]]></COLUMNNAME>
+<!--02F1D370B8B34FB79D151811C705B200-->  <COLUMNNAME><![CDATA[DaysToPasswordExpiration]]></COLUMNNAME>
 <!--02F1D370B8B34FB79D151811C705B200-->  <NAME><![CDATA[Days To Password Expiration]]></NAME>
 <!--02F1D370B8B34FB79D151811C705B200-->  <PRINTNAME><![CDATA[Days To Password Expiration]]></PRINTNAME>
 <!--02F1D370B8B34FB79D151811C705B200-->  <DESCRIPTION><![CDATA[Define the days that user password must be valid from previous password change]]></DESCRIPTION>
--- a/src/org/openbravo/authentication/AuthenticationException.java	Tue Feb 09 09:45:56 2016 +0100
+++ b/src/org/openbravo/authentication/AuthenticationException.java	Fri Feb 12 11:41:37 2016 +0100
@@ -29,6 +29,11 @@
     this.error = null;
   }
 
+  public AuthenticationException(String msg, Boolean isLogExceptionNeeded) {
+    super(msg, isLogExceptionNeeded);
+    this.error = null;
+  }
+
   public AuthenticationException(String msg, Throwable cause) {
     super(msg, cause);
     this.error = null;
--- a/src/org/openbravo/authentication/AuthenticationExpirationPasswordException.java	Tue Feb 09 09:45:56 2016 +0100
+++ b/src/org/openbravo/authentication/AuthenticationExpirationPasswordException.java	Fri Feb 12 11:41:37 2016 +0100
@@ -22,6 +22,7 @@
 public class AuthenticationExpirationPasswordException extends AuthenticationException {
   private static final long serialVersionUID = 1L;
   private OBError error;
+  private boolean passwordExpiration;
 
   public AuthenticationExpirationPasswordException(String msg) {
     super(msg);
@@ -33,12 +34,20 @@
     this.error = null;
   }
 
-  public AuthenticationExpirationPasswordException(String msg, OBError error) {
-    super(msg);
+  public AuthenticationExpirationPasswordException(String msg, OBError error,
+      boolean passwordExpiration) {
+    super(msg, false);
     this.error = error;
+    this.passwordExpiration = passwordExpiration;
+    this.getLogger().error(error.getTitle());
+
   }
 
   public OBError getOBError() {
     return error;
   }
+
+  public boolean isPasswordExpiration() {
+    return passwordExpiration;
+  }
 }
\ No newline at end of file
--- a/src/org/openbravo/authentication/basic/DefaultAuthenticationManager.java	Tue Feb 09 09:45:56 2016 +0100
+++ b/src/org/openbravo/authentication/basic/DefaultAuthenticationManager.java	Fri Feb 12 11:41:37 2016 +0100
@@ -33,6 +33,7 @@
 import org.openbravo.base.secureApp.VariablesSecureApp;
 import org.openbravo.dal.service.OBCriteria;
 import org.openbravo.dal.service.OBDal;
+import org.openbravo.database.ConnectionProvider;
 import org.openbravo.erpCommon.utility.OBError;
 import org.openbravo.erpCommon.utility.Utility;
 import org.openbravo.model.ad.access.User;
@@ -111,6 +112,8 @@
       throw new AuthenticationException("IDENTIFICATION_FAILURE_TITLE", errorMsg);
     }
 
+    getUpdatePasswordDate(userId, variables.getLanguage(), this.conn);
+
     // Using the Servlet API instead of vars.setSessionValue to avoid breaking code
     // vars.setSessionValue always transform the key to upper-case
     request.getSession(true).setAttribute("#Authenticated_user", userId);
@@ -118,17 +121,6 @@
     vars.setSessionValue("#AD_SESSION_ID", sessionId);
     vars.setSessionValue("#LogginIn", "Y");
 
-    Date lastUpdatePassword = getUpdatePasswordDate(userId);
-    Date today = new Date();
-
-    if ((lastUpdatePassword != null) && (lastUpdatePassword.compareTo(today) <= 0)) {
-      OBError errorMsg = new OBError();
-      errorMsg.setType("Error");
-      errorMsg.setTitle("IDENTIFICATION_FAILURE_TITLE");
-      errorMsg.setMessage("IDENTIFICATION_FAILURE_MSG");
-      throw new AuthenticationExpirationPasswordException("IDENTIFICATION_FAILURE_TITLE", errorMsg);
-    }
-
     if (!StringUtils.isEmpty(strAjax) && StringUtils.isEmpty(userId)) {
       bdErrorAjax(response, "Error", "",
           Utility.messageBD(this.conn, "NotLogged", variables.getLanguage()));
@@ -171,13 +163,17 @@
   /**
    * Returns the expiration password date from login and unHashedPassword parameters
    * 
+   * @param conn
+   * 
+   * @param string
+   * 
    * @param username
    *          the username
    * @return the expiration password date or null in case validity days are not applicable
    */
-  private static Date getUpdatePasswordDate(String id) {
+  private static void getUpdatePasswordDate(String id, String language, ConnectionProvider conn) {
 
-    Date total;
+    Date total = null;
 
     final OBCriteria<User> obc = OBDal.getInstance().createCriteria(User.class);
     obc.setFilterOnReadableClients(false);
@@ -185,15 +181,21 @@
     obc.add(Restrictions.eq(User.PROPERTY_ID, id));
     final User userOB = (User) obc.uniqueResult();
     Date lastUpdatePassword = userOB.getLastPasswordUpdate();
-    Long validityDays = userOB.getClient().getDaystopasswordexpiration();
+    Long validityDays = userOB.getClient().getDaysToPasswordExpiration();
     if (validityDays != null && validityDays > 0) {
       Calendar expirationDate = Calendar.getInstance();
       expirationDate.setTimeInMillis(lastUpdatePassword.getTime());
       expirationDate.add(Calendar.DATE, validityDays.intValue());
       total = expirationDate.getTime();
-      return total;
-    } else {
-      return null;
+    }
+
+    Date today = new Date();
+    if ((total != null) && (total.compareTo(today) <= 0)) {
+      OBError errorMsg = new OBError();
+      errorMsg.setType("Error");
+      errorMsg.setTitle(Utility.messageBD(conn, "CPExpirationPassword", language));
+      errorMsg.setMessage(Utility.messageBD(conn, "CPUpdatePassword", language));
+      throw new AuthenticationExpirationPasswordException(errorMsg.getTitle(), errorMsg, true);
     }
 
   }
--- a/src/org/openbravo/base/secureApp/LoginHandler.java	Tue Feb 09 09:45:56 2016 +0100
+++ b/src/org/openbravo/base/secureApp/LoginHandler.java	Fri Feb 12 11:41:37 2016 +0100
@@ -27,10 +27,10 @@
 import org.openbravo.authentication.AuthenticationExpirationPasswordException;
 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.database.ConnectionProvider;
 import org.openbravo.erpCommon.obps.ActivationKey;
 import org.openbravo.erpCommon.obps.ActivationKey.LicenseRestriction;
 import org.openbravo.erpCommon.security.Login;
@@ -74,29 +74,13 @@
     req.getSession().removeAttribute("#Authenticated_user");
     vars.removeSessionValue("#AD_Role_ID");
     vars.setSessionObject("#loggingIn", "Y");
-    final Boolean resetPassword = Boolean.parseBoolean(vars.getStringParameter("resetPassword"));
-    Boolean sameOldPassword = false;
     final String user;
     final String password;
     Client systemClient = OBDal.getInstance().get(Client.class, "0");
     String language = systemClient.getLanguage().getLanguage();
-    if (resetPassword) {
-      password = vars.getStringParameter("password");
+    boolean isPasswordResetFlow = Boolean.parseBoolean(vars.getStringParameter("resetPassword"));
+    if (isPasswordResetFlow) {
       user = vars.getStringParameter("loggedUser");
-      if (!vars.getSessionValue("#AD_Session_ID").equalsIgnoreCase("")) {
-        sameOldPassword = updatePassword(user, password);
-        if (sameOldPassword) {
-          OBError errorMsg = new OBError();
-          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 AuthenticationExpirationPasswordException(Utility.messageBD(myPool,
-              "CPSamePasswordThanOld", language), errorMsg);
-        }
-      }
     } else {
       user = vars.getStringParameter("user");
     }
@@ -113,6 +97,11 @@
         res.sendRedirect(res.encodeRedirectURL(strDireccion + "/security/Login_F1.html"));
       } else {
         try {
+          if (isPasswordResetFlow) {
+            password = vars.getStringParameter("password");
+            updatePassword(user, password, myPool, language);
+          }
+
           AuthenticationManager authManager = AuthenticationManager.getAuthenticationManager(this);
 
           final String strUserAuth = authManager.authenticate(req, res);
@@ -125,22 +114,10 @@
 
         } catch (AuthenticationExpirationPasswordException aepe) {
 
-          final OBError errorMsg = aepe.getOBError();
-          if (errorMsg != null) {
-            vars.removeSessionValue("#LoginErrorMsg");
+          vars.removeSessionValue("#LoginErrorMsg");
+          goToUpdatePassword(res, vars, aepe.getOBError().getMessage(), aepe.getOBError()
+              .getTitle(), "Error", "../security/Login_FS.html", doRedirect);
 
-            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, "CPExpirationPassword", language);
-              goToUpdatePassword(res, vars, msg, title, "Error", "../security/Login_FS.html",
-                  doRedirect);
-            }
-          }
         } catch (AuthenticationException e) {
 
           final OBError errorMsg = e.getOBError();
@@ -538,10 +515,14 @@
    *          the username
    * @param unHashedPassword
    *          the password, the unhashed password as it is entered by the user.
+   * @param myPool
+   * @param language
    * @return false in case that password has changed succesfully in database, true in case password
    *         were the same than the old one
+   * @throws ServletException
    */
-  private static Boolean updatePassword(String username, String unHashedPassword) {
+  private static void updatePassword(String username, String unHashedPassword,
+      ConnectionProvider myPool, String language) throws ServletException {
     try {
       OBContext.setAdminMode();
       final OBCriteria<User> obc = OBDal.getInstance().createCriteria(User.class);
@@ -551,16 +532,17 @@
       String oldPassword = userOB.getPassword();
       String newPassword = FormatUtilities.sha1Base64(unHashedPassword);
       if (oldPassword.equals(newPassword)) {
-        return true;
+        OBError errorMsg = new OBError();
+        errorMsg.setType("Error");
+        errorMsg.setTitle(Utility.messageBD(myPool, "CPDifferentPassword", language));
+        errorMsg.setMessage(Utility.messageBD(myPool, "CPSamePasswordThanOld", language));
+        throw new AuthenticationExpirationPasswordException(errorMsg.getMessage(), errorMsg, false);
       } 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/erpCommon/security/Login.html	Tue Feb 09 09:45:56 2016 +0100
+++ b/src/org/openbravo/erpCommon/security/Login.html	Fri Feb 12 11:41:37 2016 +0100
@@ -238,8 +238,10 @@
       document.getElementById('resetPassword').value=result.resetPassword;
       document.getElementById('user').value = '';
       document.getElementById('user').type = 'password';
-      document.getElementById('userlabel').innerHTML = 'New Password';
-      document.getElementById('passwordlabel').innerHTML = 'Confirm Password';
+      document.getElementById('userlabel').style.display = 'none';
+      document.getElementById('passwordlabel').style.display = 'none';
+      document.getElementById('newpasswordlabel').style.display = '';
+      document.getElementById('confirmpasswordlabel').style.display = '';
     }
     if (shouldContinue) {
       try {
@@ -470,12 +472,18 @@
               <dt>
                 <label for="user" class="LabelText Login_LabelText" id="userlabel" >User Name</label>
               </dt>
+              <dt>
+                <label for="user" class="LabelText Login_LabelText" id="newpasswordlabel" style="display:none;">New Password</label>
+              </dt>
               <dd>
                 <input class="dojoValidateValid Login_TextBox" type="text" maxlength="60" name="user" id="user" />
               </dd>
               <dt>
                 <label for="password" class="LabelText Login_LabelText" id="passwordlabel" >Password</label>
               </dt>
+              <dt>
+                <label for="password" class="LabelText Login_LabelText" id="confirmpasswordlabel" style="display:none;">Confirm Password</label>
+              </dt>
               <dd>
                 <input class="dojoValidateValid Login_TextBox" type="password" maxlength="40" name="password" id="password" />
               </dd>