IDEMPIERE-358 Login- how to make unique and safe / Implement same approach for fitnesse and webservices login

org.adempiere.base/src/org/compiere/util/Login.java

@@ -171,6 +171,7 @@ public class Login
 	 * @param force ignore pwd
 	 * @return  Array of Role KeyNamePair or null if error
 	 * The error (NoDatabase, UserPwdError, DBLogin) is saved in the log
+	 * @deprecated
 	 */
 	protected KeyNamePair[] getRoles (CConnection cc,
 		String app_user, String app_pwd, boolean force)
@@ -206,6 +207,7 @@ public class Login
 	 *  @param app_user Principal
 	 *  @return role array or null if in error.
 	 *  The error (NoDatabase, UserPwdError, DBLogin) is saved in the log
+	 *  @deprecated use public KeyNamePair[] getRoles(String app_user, KeyNamePair client)
 	 */
 	public KeyNamePair[] getRoles (Principal app_user)
 	{
@@ -225,6 +227,7 @@ public class Login
 	 *  @param app_pwd password
 	 *  @return role array or null if in error.
 	 *  The error (NoDatabase, UserPwdError, DBLogin) is saved in the log
+	 *  @deprecated use public KeyNamePair[] getRoles(String app_user, KeyNamePair client)
 	 */
 	public KeyNamePair[] getRoles (String app_user, String app_pwd)
 	{
@@ -238,6 +241,7 @@ public class Login
 	 *  @param force ignore pwd
 	 *  @return role array or null if in error.
 	 *  The error (NoDatabase, UserPwdError, DBLogin) is saved in the log
+	 *  @deprecated use public KeyNamePair[] getRoles(String app_user, KeyNamePair client)
 	 */
 	private KeyNamePair[] getRoles (String app_user, String app_pwd, boolean force)
 	{
@@ -296,6 +300,8 @@ public class Login
 					"         AND c.IsActive='Y') AND " +
 					" AD_User.IsActive='Y'";
 
+			// deprecate this method - it cannot cope with same user found on multiple clients
+			// use public KeyNamePair[] getRoles(String app_user, KeyNamePair client) approach instead
 			MUser user = MTable.get(m_ctx, MUser.Table_ID).createQuery( where, null).setParameters(app_user).firstOnly();   // throws error if username collision occurs
 
 			// always do calculation to confuse timing based attacks
@@ -397,7 +403,13 @@ public class Login
 				do	//	read all roles
 				{
 					MUser user = new MUser(m_ctx, rs.getInt(1), null);
-					if (user.getPassword() != null && user.getPassword().equals(app_pwd)) { 
+					boolean valid = false;
+					if (hash_password) {
+						valid = user.authenticateHash(app_pwd);
+					} else {
+						valid = user.getPassword() != null && user.getPassword().equals(app_pwd);
+					}
+					if (valid) { 
 						int AD_Role_ID = rs.getInt(2);
 						if (AD_Role_ID == 0)
 							Env.setContext(m_ctx, "#SysAdmin", "Y");

org.idempiere.fitnesse.fixture/src/org/idempiere/fitnesse/fixture/Login.java

@@ -27,6 +27,7 @@
 package org.idempiere.fitnesse.fixture;
 
 import org.compiere.model.MSession;
+import org.compiere.model.MUser;
 import org.compiere.util.Env;
 import org.compiere.util.KeyNamePair;
 
@@ -140,7 +141,29 @@ public class Login extends TableFixture {
 			return null; // already logged with same data
 		
 		org.compiere.util.Login login = new org.compiere.util.Login(m_ads!=null ? m_ads.getCtx() : null);
-		KeyNamePair[] roles = login.getRoles(m_user, m_password);
+		
+		KeyNamePair[] clients = login.getClients(m_user, m_password);
+		boolean okclient = false;
+		KeyNamePair selectedClient = null;
+		for (KeyNamePair client : clients) {
+			if (client.getKey() == m_client_id) {
+				okclient = true;
+				selectedClient = client;
+				break;
+			}
+		}
+		if (!okclient)
+			return "Error logging in - client not allowed for this user";
+
+       	Env.setContext(m_ads.getCtx(), "#AD_Client_ID", (String) selectedClient.getID());
+    	MUser user = MUser.get (m_ads.getCtx(), m_user);
+    	if (user != null) {
+    		Env.setContext(m_ads.getCtx(), "#AD_User_ID", user.getAD_User_ID() );
+    		Env.setContext(m_ads.getCtx(), "#AD_User_Name", user.getName() );
+    		Env.setContext(m_ads.getCtx(), "#SalesRep_ID", user.getAD_User_ID() );
+    	}
+		
+		KeyNamePair[] roles = login.getRoles(m_user, selectedClient);
 		if (roles != null)
 		{
 			boolean okrole = false;
@@ -153,19 +176,6 @@ public class Login extends TableFixture {
 			if (!okrole)
 				return "Error logging in - role not allowed for this user";
 
-			KeyNamePair[] clients = login.getClients( new KeyNamePair(m_role_id, "" ) );
-			boolean okclient = false;
-			for (KeyNamePair client : clients) {
-				if (client.getKey() == m_client_id) {
-					okclient = true;
-					break;
-				}
-			}
-			if (!okclient)
-				return "Error logging in - client not allowed for this role";
-
-			m_ads.getCtx().setProperty("#AD_Client_ID", Integer.toString(m_client_id));
-			
 			KeyNamePair[] orgs  = login.getOrgs( new KeyNamePair(m_role_id, "" ));
 
 			if (orgs == null)

org.idempiere.webservices/WEB-INF/src/org/idempiere/webservices/AbstractService.java

@@ -33,6 +33,7 @@ import org.adempiere.base.ServiceQuery;
 import org.adempiere.base.equinox.EquinoxExtensionLocator;
 import org.adempiere.exceptions.AdempiereException;
 import org.compiere.model.Lookup;
+import org.compiere.model.MUser;
 import org.compiere.model.MWebService;
 import org.compiere.model.MWebServiceType;
 import org.compiere.model.PO;
@@ -89,7 +90,29 @@ public class AbstractService {
 			return ret;
 		
 		Login login = new Login(m_cs.getCtx());
-		KeyNamePair[] roles = login.getRoles(loginRequest.getUser(), loginRequest.getPass());
+		KeyNamePair[] clients = login.getClients(loginRequest.getUser(), loginRequest.getPass());
+		boolean okclient = false;
+		KeyNamePair selectedClient = null;
+		for (KeyNamePair client : clients) {
+			if (client.getKey() == loginRequest.getClientID()) {
+				okclient = true;
+				selectedClient = client;
+				break;
+			}
+		}
+		if (!okclient)
+			return "Error logging in - client not allowed for this user";
+
+		m_cs.getCtx().setProperty("#AD_Client_ID", "" + loginRequest.getClientID());
+       	Env.setContext(m_cs.getCtx(), "#AD_Client_ID", (String) selectedClient.getID());
+    	MUser user = MUser.get (m_cs.getCtx(), loginRequest.getUser());
+    	if (user != null) {
+    		Env.setContext(m_cs.getCtx(), "#AD_User_ID", user.getAD_User_ID() );
+    		Env.setContext(m_cs.getCtx(), "#AD_User_Name", user.getName() );
+    		Env.setContext(m_cs.getCtx(), "#SalesRep_ID", user.getAD_User_ID() );
+    	}
+
+		KeyNamePair[] roles = login.getRoles(loginRequest.getUser(), selectedClient);
 		if (roles != null) {
 			boolean okrole = false;
 			for (KeyNamePair role : roles) {
@@ -101,19 +124,6 @@ public class AbstractService {
 			if (!okrole)
 				return "Error logging in - role not allowed for this user";
 
-			KeyNamePair[] clients = login.getClients(new KeyNamePair(loginRequest.getRoleID(), ""));
-			boolean okclient = false;
-			for (KeyNamePair client : clients) {
-				if (client.getKey() == loginRequest.getClientID()) {
-					okclient = true;
-					break;
-				}
-			}
-			if (!okclient)
-				return "Error logging in - client not allowed for this role";
-
-			m_cs.getCtx().setProperty("#AD_Client_ID", "" + loginRequest.getClientID());
-
 			KeyNamePair[] orgs = login.getOrgs(new KeyNamePair(loginRequest.getRoleID(), ""));
 
 			if (orgs == null)
@@ -152,8 +162,6 @@ public class AbstractService {
 			if (!m_cs.login(AD_User_ID, loginRequest.getRoleID(), loginRequest.getClientID(), loginRequest.getOrgID(), loginRequest.getWarehouseID(), loginRequest.getLang()))
 				return "Error logging in";
 			
-			
-			
 		} else {
 			return "Error logging in - no roles or user/pwd invalid for user " + loginRequest.getUser();
 		}