From 2ab2211f81f4ecb3928a56506ddd7712731a1036 Mon Sep 17 00:00:00 2001 From: Carlos Ruiz Date: Wed, 16 Dec 2020 02:08:11 +0100 Subject: [PATCH] IDEMPIERE-4268 Web Services : Read miss cross-tenant check (#468) Extracting from @hengsin pull request 465 Fixes cross tenant readings in MUser MUserDefWin and MWorkflow --- .../src/org/compiere/model/MUser.java | 25 ++++---- .../src/org/compiere/model/MUserDefWin.java | 36 ++++++------ .../src/org/compiere/wf/MWorkflow.java | 57 +++++++++++-------- 3 files changed, 65 insertions(+), 53 deletions(-) diff --git a/org.adempiere.base/src/org/compiere/model/MUser.java b/org.adempiere.base/src/org/compiere/model/MUser.java index 69acefb8bb..21e331d931 100644 --- a/org.adempiere.base/src/org/compiere/model/MUser.java +++ b/org.adempiere.base/src/org/compiere/model/MUser.java @@ -383,6 +383,8 @@ public class MUser extends X_AD_User implements ImmutablePOSupport private MRole[] m_roles = null; /** Roles of User with Org */ private int m_rolesAD_Org_ID = -1; + /** AD_Client_ID for m_roles above */ + private int m_rolesAD_Client_ID = -1; /** Is Administrator */ private Boolean m_isAdministrator = null; /** User Access Rights */ @@ -787,7 +789,7 @@ public class MUser extends X_AD_User implements ImmutablePOSupport */ public MRole[] getRoles (int AD_Org_ID) { - if (m_roles != null && m_rolesAD_Org_ID == AD_Org_ID) + if (m_roles != null && m_rolesAD_Org_ID == AD_Org_ID && m_rolesAD_Client_ID == Env.getAD_Client_ID(Env.getCtx())) return m_roles; ArrayList list = new ArrayList(); @@ -796,7 +798,7 @@ public class MUser extends X_AD_User implements ImmutablePOSupport // are found but also roles which delegate org access to the user level where // this user has access to the org in question String sql = "SELECT * FROM AD_Role r " - + "WHERE r.IsActive='Y'" + + "WHERE r.IsActive='Y' AND r.AD_Client_ID IN (0, ?) " + " AND EXISTS (SELECT * FROM AD_User_Roles ur" + " WHERE r.AD_Role_ID=ur.AD_Role_ID AND ur.IsActive='Y' AND ur.AD_User_ID=?) " + " AND ( ( r.isaccessallorgs = 'Y' ) OR " @@ -817,18 +819,14 @@ public class MUser extends X_AD_User implements ImmutablePOSupport try { pstmt = DB.prepareStatement (sql, get_TrxName()); - pstmt.setInt (1, getAD_User_ID()); - pstmt.setInt (2, AD_Org_ID); - pstmt.setInt (3, getAD_User_ID()); - pstmt.setInt (4, AD_Org_ID); + pstmt.setInt (1, Env.getAD_Client_ID(Env.getCtx())); + pstmt.setInt (2, getAD_User_ID()); + pstmt.setInt (3, AD_Org_ID); + pstmt.setInt (4, getAD_User_ID()); + pstmt.setInt (5, AD_Org_ID); rs = pstmt.executeQuery (); - try { - PO.setCrossTenantSafe(); - while (rs.next ()) - list.add (new MRole(Env.getCtx(), rs, get_TrxName())); - } finally { - PO.clearCrossTenantSafe(); - } + while (rs.next ()) + list.add (new MRole(Env.getCtx(), rs, get_TrxName())); } catch (Exception e) { @@ -844,6 +842,7 @@ public class MUser extends X_AD_User implements ImmutablePOSupport list.stream().forEach(e -> e.markImmutable()); m_rolesAD_Org_ID = AD_Org_ID; + m_rolesAD_Client_ID = Env.getAD_Client_ID(Env.getCtx()); m_roles = new MRole[list.size()]; list.toArray (m_roles); return m_roles; diff --git a/org.adempiere.base/src/org/compiere/model/MUserDefWin.java b/org.adempiere.base/src/org/compiere/model/MUserDefWin.java index e24f3295d7..c6a177b5aa 100644 --- a/org.adempiere.base/src/org/compiere/model/MUserDefWin.java +++ b/org.adempiere.base/src/org/compiere/model/MUserDefWin.java @@ -16,7 +16,9 @@ package org.compiere.model; import java.sql.ResultSet; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Properties; import org.compiere.util.Env; @@ -35,8 +37,7 @@ public class MUserDefWin extends X_AD_UserDef_Win implements ImmutablePOSupport * */ private static final long serialVersionUID = -7542708120229671875L; - private volatile static List m_fullList = null; - private static final Object m_fullListLock = new Object(); + private static final Map> m_fullMap = new HashMap>(); /** * Standard constructor. @@ -104,26 +105,25 @@ public class MUserDefWin extends X_AD_UserDef_Win implements ImmutablePOSupport */ private static MUserDefWin[] getAll (Properties ctx, int window_ID ) { - synchronized (m_fullListLock) { - if (m_fullList == null) { - try { - PO.setCrossTenantSafe(); - m_fullList = new Query(ctx, MUserDefWin.Table_Name, null, null) - .setOnlyActiveRecords(true) - .list(); - } finally { - PO.clearCrossTenantSafe(); - } + List fullList = null; + synchronized (m_fullMap) { + fullList = m_fullMap.get(Env.getAD_Client_ID(ctx)); + if (fullList == null) { + fullList = new Query(ctx, MUserDefWin.Table_Name, null, null) + .setOnlyActiveRecords(true) + .setClient_ID() + .list(); + m_fullMap.put(Env.getAD_Client_ID(ctx), fullList); } } - if (m_fullList.size() == 0) { + if (fullList.size() == 0) { return null; } List list = new ArrayList(); - for (MUserDefWin udw : m_fullList) { + for (MUserDefWin udw : fullList) { if (udw.getAD_Window_ID() == window_ID && udw.getAD_Client_ID() == Env.getAD_Client_ID(ctx) && (udw.getAD_Language() == null || udw.getAD_Language().equals(Env.getAD_Language(ctx))) @@ -240,13 +240,17 @@ public class MUserDefWin extends X_AD_UserDef_Win implements ImmutablePOSupport @Override protected boolean beforeSave(boolean newRecord) { - m_fullList = null; + synchronized (m_fullMap) { + m_fullMap.remove(getAD_Client_ID()); + } return true; } @Override protected boolean beforeDelete() { - m_fullList = null; + synchronized (m_fullMap) { + m_fullMap.remove(getAD_Client_ID()); + } return true; } diff --git a/org.adempiere.base/src/org/compiere/wf/MWorkflow.java b/org.adempiere.base/src/org/compiere/wf/MWorkflow.java index afed015f5d..f7b88cf3a9 100644 --- a/org.adempiere.base/src/org/compiere/wf/MWorkflow.java +++ b/org.adempiere.base/src/org/compiere/wf/MWorkflow.java @@ -23,7 +23,9 @@ import java.sql.Timestamp; import java.util.ArrayList; import java.util.Arrays; import java.util.Calendar; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Properties; import java.util.logging.Level; import java.util.stream.Collectors; @@ -125,46 +127,43 @@ public class MWorkflow extends X_AD_Workflow implements ImmutablePOSupport , String trxName //Bug 1568766 Trx should be kept all along the road ) { - String key = "C" + AD_Client_ID + "T" + AD_Table_ID; // Reload - if (s_cacheDocValue.isReset()) + Map cachedMap = s_cacheDocValue.get(AD_Client_ID); + if (cachedMap == null) { - final String whereClause = "WorkflowType=? AND IsValid=?"; + final String whereClause = "WorkflowType=? AND IsValid=? AND AD_Client_ID=?"; List workflows; - try { - PO.setCrossTenantSafe(); - workflows = new Query(ctx, Table_Name, whereClause, trxName) - .setParameters(new Object[]{WORKFLOWTYPE_DocumentValue, true}) - .setOnlyActiveRecords(true) - .setOrderBy("AD_Client_ID, AD_Table_ID") - .list(); - } finally { - PO.clearCrossTenantSafe(); - } + workflows = new Query(ctx, Table_Name, whereClause, trxName) + .setParameters(new Object[]{WORKFLOWTYPE_DocumentValue, true, Env.getAD_Client_ID(ctx)}) + .setOnlyActiveRecords(true) + .setOrderBy("AD_Table_ID") + .list(); + cachedMap = new HashMap(); + s_cacheDocValue.put(AD_Client_ID, cachedMap); ArrayList list = new ArrayList(); - String oldKey = ""; - String newKey = null; + int previousTableId = -1; + int currentTableId = -1; for (MWorkflow wf : workflows) { - newKey = "C" + wf.getAD_Client_ID() + "T" + wf.getAD_Table_ID(); - if (!newKey.equals(oldKey) && list.size() > 0) + currentTableId = wf.getAD_Table_ID(); + if (currentTableId != previousTableId && list.size() > 0) { - s_cacheDocValue.put (oldKey, list.stream().map(e -> {return new MWorkflow(Env.getCtx(), e);}).toArray(MWorkflow[]::new)); + cachedMap.put (previousTableId, list.stream().map(e -> {return new MWorkflow(Env.getCtx(), e);}).toArray(MWorkflow[]::new)); list = new ArrayList(); } - oldKey = newKey; + previousTableId = currentTableId; list.add(wf); } // Last one if (list.size() > 0) { - s_cacheDocValue.put (oldKey, list.stream().map(e -> {return new MWorkflow(Env.getCtx(), e);}).toArray(MWorkflow[]::new)); + cachedMap.put (previousTableId, list.stream().map(e -> {return new MWorkflow(Env.getCtx(), e);}).toArray(MWorkflow[]::new)); } - if (s_log.isLoggable(Level.CONFIG)) s_log.config("#" + s_cacheDocValue.size()); + if (s_log.isLoggable(Level.CONFIG)) s_log.config("#" + cachedMap.size()); } // Look for Entry - MWorkflow[] retValue = (MWorkflow[])s_cacheDocValue.get(key); + MWorkflow[] retValue = (MWorkflow[])cachedMap.get(AD_Table_ID); //hengsin: this is not threadsafe /* //set trxName to all workflow instance @@ -180,9 +179,19 @@ public class MWorkflow extends X_AD_Workflow implements ImmutablePOSupport /** Single Cache */ - private static ImmutablePOCache s_cache = new ImmutablePOCache(Table_Name, Table_Name+"|Language_Workflow", 20); + private static ImmutablePOCache s_cache = new ImmutablePOCache(Table_Name, Table_Name, 20); /** Document Value Cache */ - private static CCache s_cacheDocValue = new CCache (Table_Name, Table_Name+"|AD_Client_Table", 5); + private static final CCache> s_cacheDocValue = new CCache<> (Table_Name, Table_Name+"|DocumentValue", 5) { + /** + * generated serial id + */ + private static final long serialVersionUID = 2548097748351277269L; + + @Override + public int reset(int recordId) { + return reset(); + } + }; /** Static Logger */ private static CLogger s_log = CLogger.getCLogger (MWorkflow.class);