IDEMPIERE-2944 Preserve iDempiere session between sucessive webservice calls / integrate peer review from hengsin

AP2-330 Web services loggin. Fix thread safety.

org.idempiere.webservices/WEB-INF/src/com/trekglobal/ws/CompositeServiceImpl.java

@@ -65,10 +65,7 @@ public class CompositeServiceImpl extends AbstractService implements CompositeSe
 	 */
 	@Override
 	public CompositeResponsesDocument compositeOperation(CompositeRequestDocument reqs) {
-		boolean connected = getCompiereService().isConnected();
-		
 		try {
-			if (!connected)
 			getCompiereService().connect();
 			
 			CompositeResponsesDocument ret = CompositeResponsesDocument.Factory.newInstance();
@@ -124,7 +121,6 @@ public class CompositeServiceImpl extends AbstractService implements CompositeSe
 	
 			return ret;
 		} finally {
-			if (!connected)
 			getCompiereService().disconnect();
 		}
 	}

org.idempiere.webservices/WEB-INF/src/org/compiere/model/MWebService.java

@@ -76,7 +76,7 @@ public class MWebService extends X_WS_WebService
 	 *	@param webServiceValue
 	 *	@return Table
 	 */
-	public static MWebService get (Properties ctx, String webServiceValue)
+	public static synchronized MWebService get (Properties ctx, String webServiceValue)
 	{
 		if (webServiceValue == null)
 			return null;

org.idempiere.webservices/WEB-INF/src/org/idempiere/adinterface/CompiereService.java

@@ -79,13 +79,13 @@ public class CompiereService {
 		
 	public final String dateFormatOnlyForCtx =  "yyyy-MM-dd";
 
-	private boolean m_connected;
+	private int m_connectCount;
 
 	/**
 	 * 
 	 * @return AD_Client_ID of current request
 	 */
-    public int getAD_Client_ID() {
+	public synchronized int getAD_Client_ID() {
 		return m_AD_Client_ID;
 	}
 
@@ -93,7 +93,7 @@ public class CompiereService {
      * 
      * @return AD_Org_ID of current request
      */
-	public int getAD_Org_ID() {
+	public synchronized int getAD_Org_ID() {
 		return m_AD_Org_ID;
 	}
 
@@ -101,7 +101,7 @@ public class CompiereService {
 	 * 
 	 * @return context of current request
 	 */
-	public Properties getCtx() {
+	public synchronized Properties getCtx() {
 		return Env.getCtx();
 	}
 
@@ -111,20 +111,16 @@ public class CompiereService {
 	public CompiereService()
 	{		
 		m_loggedin = false;
-		m_connected = false;
+		m_connectCount = 0;
 	}
 	
 	/**
 	 * setup request
 	 */
 	public void connect()
-	{
-		if (!m_connected)
 	{
 		CompiereUtil.initWeb();
 
-			m_connected = true;
-			
 		ServerContext.setCurrentInstance(new Properties());
 		Env.setContext(getCtx(), "#AD_Language", "en_US" );
 		m_language = Language.getLanguage("en_US");
@@ -135,53 +131,39 @@ public class CompiereService {
 		dateFormatJDBC = DisplayType.getDateFormat_JDBC();
 		dateTimeFormatJDBC = DisplayType.getTimestampFormat_Default();
 		timeFormatJDBC = DisplayType.getTimeFormat_Default();
+
+		m_connectCount++;
 	}
+	
+	/**
+	 * Increase connect count
+	 */
+	public synchronized void connectCacheInstance()
+	{
+		m_connectCount++;
 	}
 
 	/**
 	 * cleanup request
 	 */
-	public void disconnect() 
+	public synchronized void disconnect() 
 	{
+		m_connectCount--;
 		// TODO: create a thread that checks expired connected compiereservices and log them out
-		if (isExpired()) {
+		expungeIfExpire();		
-			synchronized (csMap) {
-				//save session in cache
-				String key = getKey(m_AD_Client_ID,
-						m_AD_Org_ID,
-						m_userName,
-						m_AD_Role_ID,
-						m_M_Warehouse_ID,
-						m_locale,
-						m_password,
-						m_IPAddress);
-				if (csMap.containsKey(key)) {				
-					csMap.remove(key.toString());
-					ctxMap.remove(key.toString());
-				}
-			}
-		}		
-	}
-	
-	/**
-	 * @return true if started
-	 */
-	public boolean isConnected() 
-	{
-		return m_connected;
 	}
 
 	/**
 	 * @return Language of current request
 	 */
-	public Language getLanguage() {
+	public synchronized Language getLanguage() {
 		return m_language;
 	}
 
 	/**
 	 * @return true if already logged in
 	 */
-	public boolean isLoggedIn() {
+	public synchronized boolean isLoggedIn() {
 		return m_loggedin;
 	}
 	
@@ -195,7 +177,7 @@ public class CompiereService {
 	 *  @param AD_Org_ID org
 	 *  @param M_Warehouse_ID warehouse
 	 */
-	private String checkLogin (Properties ctx, int AD_User_ID, int AD_Role_ID, int AD_Client_ID, int AD_Org_ID, int M_Warehouse_ID)
+	private synchronized String checkLogin (Properties ctx, int AD_User_ID, int AD_Role_ID, int AD_Client_ID, int AD_Org_ID, int M_Warehouse_ID)
 	{
 		//  Get Login Info
 		String loginInfo = null;
@@ -261,7 +243,7 @@ public class CompiereService {
 	 * @param Lang
 	 * @return true if login is successful
 	 */
-	public boolean login( int AD_User_ID, int AD_Role_ID, int AD_Client_ID, int AD_Org_ID, int M_Warehouse_ID, String Lang ) {
+	public synchronized boolean login( int AD_User_ID, int AD_Role_ID, int AD_Client_ID, int AD_Org_ID, int M_Warehouse_ID, String Lang ) {
 		m_loggedin = false;
 		String loginInfo = checkLogin (getCtx(), AD_User_ID, AD_Role_ID, AD_Client_ID, AD_Org_ID, M_Warehouse_ID );				
 		if (loginInfo == null)
@@ -341,7 +323,7 @@ public class CompiereService {
 	 * 
 	 * @return AD_User_ID of current request
 	 */
-	public int getAD_User_ID() {
+	public synchronized int getAD_User_ID() {
 		return m_AD_User_ID;
 	}
 
@@ -349,7 +331,7 @@ public class CompiereService {
 	 * 
 	 * @return AD_Role_ID of current request
 	 */
-	public int getAD_Role_ID() {
+	public synchronized int getAD_Role_ID() {
 		return m_AD_Role_ID;
 	}
 
@@ -357,7 +339,7 @@ public class CompiereService {
 	 * 
 	 * @return locale code of current request
 	 */
-	public String getLocale() {
+	public synchronized String getLocale() {
 		return m_locale;
 	}
 
@@ -365,7 +347,7 @@ public class CompiereService {
 	 * 
 	 * @return M_Warehouse_ID of current request
 	 */
-	public int getM_Warehouse_ID() {
+	public synchronized int getM_Warehouse_ID() {
 		return m_M_Warehouse_ID;
 	}
 	
@@ -373,43 +355,43 @@ public class CompiereService {
 	 * 
 	 * @return logged in user name of current request
 	 */
-	public String getUserName() {
+	public synchronized String getUserName() {
 		return m_userName;
 	}
 
 	/**
 	 * @return set password
 	 */
-	public void setPassword(String pass) {
+	public synchronized void setPassword(String pass) {
 		m_password = pass;
 	}
 
 	/**
 	 * @return logged in password of current request
 	 */
-	public String getPassword() {
+	public synchronized String getPassword() {
 		return m_password;
 	}
 
 	/**
 	 * @return set expiry minutes
 	 */
-	public void setExpiryMinutes(int expiryMinutes) {
+	public synchronized void setExpiryMinutes(int expiryMinutes) {
 		m_expiryMinutes = expiryMinutes;
 	}
 
 	/**
 	 * @return logged in expiry minutes of current request
 	 */
-	public int getExpiryMinutes() {
+	public synchronized int getExpiryMinutes() {
 		return m_expiryMinutes;
 	}
 
-	public void refreshLastAuthorizationTime() {
+	public synchronized void refreshLastAuthorizationTime() {
 		m_lastAuthorizationTime = System.currentTimeMillis();
 	}
 
-	public void setIPAddress(String remoteAddr) {
+	public synchronized void setIPAddress(String remoteAddr) {
 		m_IPAddress = remoteAddr;
 	}
 
@@ -427,9 +409,7 @@ public class CompiereService {
 			if (csMap.containsKey(key)) {
 				l_cs = csMap.get(key);
 				if (l_cs != null) {
-					if (l_cs.isExpired()) {
+					if (l_cs.expungeIfExpire()) {						
-						csMap.remove(key);
-						ctxMap.remove(key);
 						l_cs = null;
 					} else {
 						Properties cachedCtx = ctxMap.get(key);
@@ -463,13 +443,13 @@ public class CompiereService {
 		return key.toString();
 	}
 	
-	private boolean isExpired() {
+	private synchronized boolean expungeIfExpire() {
 		boolean expired =
 			   (
 				   (getExpiryMinutes() <= 0)
 				|| (m_lastAuthorizationTime + (getExpiryMinutes() * 60000) <= System.currentTimeMillis())
 			   );
-		if (m_connected && expired) 
+		if (m_connectCount==0 && expired) 
 		{
 			synchronized (csMap) {
 				String key = getKey(m_AD_Client_ID,
@@ -480,15 +460,25 @@ public class CompiereService {
 						m_locale,
 						m_password,
 						m_IPAddress);
-				if (ctxMap.containsKey(key)) {
+				if (csMap.containsKey(key)) {
-					Properties cachedCtx = ctxMap.get(key);
+					csMap.remove(key);
-					Env.getCtx().putAll(cachedCtx);
 				}
+				if (ctxMap.containsKey(key)) {
+					Properties cachedCtx = ctxMap.remove(key);
+					Properties currentCtx = ServerContext.getCurrentInstance();
+					try {
+						ServerContext.setCurrentInstance(cachedCtx);
 						if (log.isLoggable(Level.INFO)) log.info("Closing expired/invalid " + this);
 						Env.logout();
+					} finally {
+						if (currentCtx == cachedCtx) {
 							ServerContext.dispose();
+						} else {
+							ServerContext.setCurrentInstance(currentCtx);
+						}
+					}
+				}				
 				m_loggedin = false;
-				m_connected = false;
 			}
 		}
 		return expired;

org.idempiere.webservices/WEB-INF/src/org/idempiere/adinterface/ModelADServiceImpl.java

@@ -168,12 +168,8 @@ public class ModelADServiceImpl extends AbstractService implements ModelADServic
 	 * use the runProcess web service
 	 */
 	public StandardResponseDocument setDocAction(ModelSetDocActionRequestDocument req) {
-		boolean connected = getCompiereService().isConnected();
-		
-		boolean manageTrx = this.manageTrx;
 		Trx trx=null;
 		try {
-			if (!connected)
 			getCompiereService().connect();
 			
 			StandardResponseDocument ret = StandardResponseDocument.Factory.newInstance();
@@ -289,9 +285,6 @@ public class ModelADServiceImpl extends AbstractService implements ModelADServic
 			if (manageTrx && !trx.commit())
 				return rollbackAndSetError(trx, resp, ret, true, "Cannot commit after docAction");
 	
-			if (manageTrx)
-				trx.close();
-	
 			// resp.setError("");
 			resp.setIsError(false);
 	
@@ -305,7 +298,6 @@ public class ModelADServiceImpl extends AbstractService implements ModelADServic
 			if (manageTrx && trx != null)
 				trx.close();
 
-			if (!connected)
 			getCompiereService().disconnect();
 		}
 	}
@@ -390,10 +382,7 @@ public class ModelADServiceImpl extends AbstractService implements ModelADServic
 	
 
 	public RunProcessResponseDocument runProcess(ModelRunProcessRequestDocument req) {
-		boolean connected = getCompiereService().isConnected();
-		
 		try {
-			if (!connected)
 			getCompiereService().connect();
 			
 			RunProcessResponseDocument resbadlogin = RunProcessResponseDocument.Factory.newInstance();
@@ -430,16 +419,12 @@ public class ModelADServiceImpl extends AbstractService implements ModelADServic
 			requestCtx.put(serviceType+"_Summary", response.getRunProcessResponse().getSummary());
 			return response;
 		} finally {
-			if (!connected)
 			getCompiereService().disconnect();
 		}
 	}
 
 	public WindowTabDataDocument getList(ModelGetListRequestDocument req) {
-		boolean connected = getCompiereService().isConnected();
-		
 		try {
-			if (!connected)
 			getCompiereService().connect();
 			
 			WindowTabDataDocument resdoc = WindowTabDataDocument.Factory.newInstance();
@@ -649,19 +634,13 @@ public class ModelADServiceImpl extends AbstractService implements ModelADServic
 	
 			return resdoc;
 		} finally {
-			if (!connected)
 			getCompiereService().disconnect();
 		}
 	} // getList
 
 	public StandardResponseDocument deleteData(ModelCRUDRequestDocument req) {
-		boolean connected = getCompiereService().isConnected();
-		
 		Trx trx = null;
-		boolean manageTrx = this.manageTrx;
-		
 		try {
-			if (!connected)
 			getCompiereService().connect();
 			
 			StandardResponseDocument ret = StandardResponseDocument.Factory.newInstance();
@@ -727,7 +706,6 @@ public class ModelADServiceImpl extends AbstractService implements ModelADServic
 			if (manageTrx && trx != null)
 				trx.close();
 			
-			if (!connected)
 			getCompiereService().disconnect();
 		}
 	}
@@ -740,14 +718,8 @@ public class ModelADServiceImpl extends AbstractService implements ModelADServic
 	}
 
 	public StandardResponseDocument createData(ModelCRUDRequestDocument req) {
-		
-		boolean connected = getCompiereService().isConnected();
-		
 		Trx trx = null;
-		boolean manageTrx = this.manageTrx;
-		
 		try {
-			if (!connected)
 			getCompiereService().connect();
 			
 	    	StandardResponseDocument ret = StandardResponseDocument.Factory.newInstance();
@@ -849,20 +821,14 @@ public class ModelADServiceImpl extends AbstractService implements ModelADServic
 			if (manageTrx && trx != null)
 				trx.close();
 			
-			if (!connected)
 			getCompiereService().disconnect();
 				
 		}
 	} // createData
 
 	public StandardResponseDocument createUpdateData(ModelCRUDRequestDocument req) {
-		boolean connected = getCompiereService().isConnected();
-		
 		Trx trx = null;
-		boolean manageTrx = this.manageTrx;
-		
 		try {
-			if (!connected)
 			getCompiereService().connect();
 			
 			StandardResponseDocument ret = StandardResponseDocument.Factory.newInstance();
@@ -1058,7 +1024,6 @@ public class ModelADServiceImpl extends AbstractService implements ModelADServic
 			if (manageTrx && trx != null)
 				trx.close();
 			
-			if (!connected)
 			getCompiereService().disconnect();
 		}
 	} // createUpdateData
@@ -1234,13 +1199,8 @@ public class ModelADServiceImpl extends AbstractService implements ModelADServic
 	}
 	
 	public StandardResponseDocument updateData(ModelCRUDRequestDocument req){
-		boolean connected = getCompiereService().isConnected();
-		
 		Trx trx = null;
-		boolean manageTrx = this.manageTrx;
-		
 		try {
-			if (!connected)
 			getCompiereService().connect();
 			
 	    	StandardResponseDocument ret = StandardResponseDocument.Factory.newInstance();
@@ -1323,16 +1283,12 @@ public class ModelADServiceImpl extends AbstractService implements ModelADServic
 			if (manageTrx && trx != null)
 				trx.close();
 			
-			if (!connected)
 			getCompiereService().disconnect();
 		}
 	} // updateData
 
 	public WindowTabDataDocument readData(ModelCRUDRequestDocument req) {
-		boolean connected = getCompiereService().isConnected();
-		
 		try {
-			if (!connected)
 			getCompiereService().connect();
 			
 			WindowTabDataDocument ret = WindowTabDataDocument.Factory.newInstance();
@@ -1423,18 +1379,13 @@ public class ModelADServiceImpl extends AbstractService implements ModelADServic
 	
 			return ret;
 		} finally {
-			if (!connected)
 			getCompiereService().disconnect();
 		}
 	}
 
 	public WindowTabDataDocument queryData(ModelCRUDRequestDocument req) {
-		boolean connected = getCompiereService().isConnected();
-		
-		boolean manageTrx = this.manageTrx;
 		Trx trx=null;
 		try {
-			if (!connected)
 			getCompiereService().connect();
 			
 			CompiereService m_cs = getCompiereService();
@@ -1589,7 +1540,6 @@ public class ModelADServiceImpl extends AbstractService implements ModelADServic
 			if (manageTrx && trx != null)
 				trx.close();
 			
-			if (!connected)
 			getCompiereService().disconnect();
 		}
 	}

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

@@ -14,6 +14,8 @@
 package org.idempiere.webservices;
 
 import java.math.BigDecimal;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
 import java.sql.Timestamp;
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -38,8 +40,9 @@ import org.compiere.model.MWebService;
 import org.compiere.model.MWebServiceType;
 import org.compiere.model.PO;
 import org.compiere.model.POInfo;
-import org.compiere.model.Query;
 import org.compiere.model.X_WS_WebServiceMethod;
+import org.compiere.model.X_WS_WebServiceTypeAccess;
+import org.compiere.util.CCache;
 import org.compiere.util.DB;
 import org.compiere.util.Env;
 import org.compiere.util.KeyNamePair;
@@ -64,7 +67,9 @@ import org.idempiere.webservices.fault.IdempiereServiceFault;
  */
 public class AbstractService {
 
-	private static final String ROLE_ACCESS_SQL = "SELECT IsActive FROM WS_WebServiceTypeAccess WHERE AD_Role_ID=? "
+	private static final String ROLE_ACCESS_SQL = "SELECT IsActive FROM WS_WebServiceTypeAccess WHERE AD_Role_ID IN ("
+			+ "SELECT AD_Role_ID FROM AD_Role WHERE AD_Role_ID=? UNION "
+			+ "SELECT Included_Role_ID as AD_Role_ID FROM AD_Role_Included WHERE AD_Role_ID=?) "
 	        + "AND WS_WebServiceType_ID=?";
 	private static final String COMPIERE_SERVICE = "CompiereService";
 	@Resource
@@ -91,6 +96,7 @@ public class AbstractService {
 			if (cachedCs != null) {
 				m_cs = cachedCs;
 				req.setAttribute(COMPIERE_SERVICE, cachedCs);
+				m_cs.connectCacheInstance();
 				return authenticate(webService, method, serviceType, cachedCs); // already logged with same data
 			}
 		}
@@ -201,6 +207,9 @@ public class AbstractService {
 		return authenticate(webService, method, serviceType, m_cs);
 	}
 
+	private static CCache<String,MWebServiceType> s_WebServiceTypeCache	= new CCache<String,MWebServiceType>(MWebServiceType.Table_Name, 10, 60);	//60 minutes
+	private static CCache<String,Boolean> s_RoleAccessCache = new CCache<>(X_WS_WebServiceTypeAccess.Table_Name, 60, 60);
+
 	/**
 	 * Authenticate user for requested service type
 	 * @param webServiceValue
@@ -219,28 +228,59 @@ public class AbstractService {
 		if (m_webservicemethod == null || !m_webservicemethod.isActive())
 			return "Method " + methodValue + " not registered";
 
-		MWebServiceType m_webservicetype = new Query(m_cs.getCtx(), MWebServiceType.Table_Name,
+		MWebServiceType m_webservicetype = null;
-				"AD_Client_ID IN (0,?) AND WS_WebService_ID=? AND WS_WebServiceMethod_ID=? AND Value=?",
+		String key = m_cs.getAD_Client_ID() + "|" + m_webservice.getWS_WebService_ID() + "|" 
-				null)
+				+ m_webservicemethod.getWS_WebServiceMethod_ID() + "|" + serviceTypeValue;
-				.setOnlyActiveRecords(true)
+		synchronized (s_WebServiceTypeCache) {
-				.setParameters(m_cs.getAD_Client_ID(), m_webservice.getWS_WebService_ID(), m_webservicemethod.getWS_WebServiceMethod_ID(), serviceTypeValue)
+			m_webservicetype = s_WebServiceTypeCache.get(key);
-				.setOrderBy("AD_Client_ID DESC") // IDEMPIERE-3394 give precedence to tenant defined if there are system+tenant
+			if (m_webservicetype == null) {
-				.first();
+				final String sql = "SELECT * FROM WS_WebServiceType " + "WHERE AD_Client_ID=? " + "AND WS_WebService_ID=? "
+						+ "AND WS_WebServiceMethod_ID=? " + "AND Value=? " + "AND IsActive='Y'";
+				PreparedStatement pstmt = null;
+				ResultSet rs = null;
+				try {
+					pstmt = DB.prepareStatement(sql, null);
+					pstmt.setInt(1, m_cs.getAD_Client_ID());
+					pstmt.setInt(2, m_webservice.getWS_WebService_ID());
+					pstmt.setInt(3, m_webservicemethod.getWS_WebServiceMethod_ID());
+					pstmt.setString(4, serviceTypeValue);
+					rs = pstmt.executeQuery();
+					if (rs.next()) {
+						m_webservicetype = new MWebServiceType(m_cs.getCtx(), rs, null);
+						s_WebServiceTypeCache.put(key, m_webservicetype);
+					}
+				} catch (Exception e) {
+					throw new IdempiereServiceFault(e.getClass().toString() + " " + e.getMessage() + " sql=" + sql, e.getCause(), new QName(
+							"authenticate"));
+				} finally {
+					DB.close(rs, pstmt);
+					rs = null;
+					pstmt = null;
+				}
+			}
+		}
 
 		if (m_webservicetype == null)
 			return "Service type " + serviceTypeValue + " not configured";
 
 		getHttpServletRequest().setAttribute("MWebServiceType", m_webservicetype);
 		
+		int AD_Role_ID = Env.getAD_Role_ID( m_cs.getCtx());
+		key = AD_Role_ID + "|" + m_webservicetype.get_ID();
+		synchronized (s_RoleAccessCache) {
+			Boolean bAccess = s_RoleAccessCache.get(key);
+			if (bAccess == null) {
 				// Check if role has access on web-service
 		        String hasAccess = DB.getSQLValueStringEx(null, ROLE_ACCESS_SQL,
-                Env.getAD_Role_ID( m_cs.getCtx()),
+		                AD_Role_ID, AD_Role_ID, m_webservicetype.get_ID());
-                m_webservicetype.get_ID());
+		        bAccess = "Y".equals(hasAccess);
-
+		        s_RoleAccessCache.put(key, bAccess);
-        if (!"Y".equals(hasAccess))
+			}
+	        if (!bAccess.booleanValue())
 	        {
 	            return "Web Service Error: Login role does not have access to the service type";
 	        }			
+		}
         
 		String ret=invokeLoginValidator(null, m_cs.getCtx(), m_webservicetype, IWSValidator.TIMING_ON_AUTHORIZATION);
 		if(ret!=null && ret.length()>0)