From 8cf16ba13708d3887b0323e1220c21a11db14f92 Mon Sep 17 00:00:00 2001 From: Carlos Ruiz Date: Tue, 1 Dec 2020 00:16:22 +0100 Subject: [PATCH] IDEMPIERE-4268 Web Services : Read miss cross-tenant check (#425) Implement cross tenant check on foreign keys for insert and update --- .../src/org/compiere/model/PO.java | 92 ++++++++++++++++++- .../adinterface/ModelADServiceImpl.java | 11 ++- 2 files changed, 101 insertions(+), 2 deletions(-) diff --git a/org.adempiere.base/src/org/compiere/model/PO.java b/org.adempiere.base/src/org/compiere/model/PO.java index 17cd7d4b1e..bf5554d300 100644 --- a/org.adempiere.base/src/org/compiere/model/PO.java +++ b/org.adempiere.base/src/org/compiere/model/PO.java @@ -112,7 +112,7 @@ public abstract class PO /** * */ - private static final long serialVersionUID = -2086165095004944867L; + private static final long serialVersionUID = 1556095090658747371L; public static final String LOCAL_TRX_PREFIX = "POSave"; @@ -1943,6 +1943,8 @@ public abstract class PO /** Cache */ private static CCache trl_cache = new CCache("PO_Trl", 5); + /** Cache for foreign keys */ + private static CCache> fks_cache = new CCache>("FKs", 5); public String get_Translation (String columnName, String AD_Language) { @@ -5000,4 +5002,92 @@ public abstract class PO } } + /** + * Validate Foreign keys for cross tenant + * to be called programmatically before saving in programs that can receive arbitrary values in IDs + * This is an expensive operation in terms of database, use it wisely + * + * TODO: there is huge room for performance improvement, for example: + * - caching the valid values found on foreign tables + * - caching the column ID of the foreign column + * - caching the systemAccess + * + * @return true if all the foreign keys are valid + */ + public boolean validForeignKeys() { + List fks = getForeignColumnIdxs(); + if (fks == null) { + return true; + } + for (ValueNamePair vnp : fks) { + String fkcol = vnp.getID(); + String fktab = vnp.getName(); + int index = get_ColumnIndex(fkcol); + if (is_new() || is_ValueChanged(index)) { + int fkval = get_ValueAsInt(index); + if (fkval > 0) { + MTable ft = MTable.get(getCtx(), fktab); + boolean systemAccess = false; + String accessLevel = ft.getAccessLevel(); + if ( MTable.ACCESSLEVEL_All.equals(accessLevel) + || MTable.ACCESSLEVEL_SystemOnly.equals(accessLevel) + || MTable.ACCESSLEVEL_SystemPlusClient.equals(accessLevel)) { + systemAccess = true; + } + StringBuilder sql = new StringBuilder("SELECT AD_Client_ID FROM ") + .append(fktab) + .append(" WHERE ") + .append(ft.getKeyColumns()[0]) + .append("=?"); + int pocid = DB.getSQLValue(get_TrxName(), sql.toString(), fkval); + if (pocid < 0) { + log.saveError("Error", "Foreign ID " + fkval + " not found in " + fkcol); + return false; + } + if (pocid == 0 && !systemAccess) { + log.saveError("Error", "System ID " + fkval + " cannot be used in " + fkcol); + return false; + } + int curcid = Env.getAD_Client_ID(getCtx()); + if (pocid > 0 && pocid != curcid) { + log.saveError("Error", "Cross tenant ID " + fkval + " not allowed in " + fkcol); + return false; + } + } + } + } + return true; + } + + /** + * Returns a list of indexes for the foreign columns, null if none + * @return array of int indexes + */ + private List getForeignColumnIdxs() { + List retValue; + if (fks_cache.containsKey(get_Table_ID())) { + retValue = fks_cache.get(get_Table_ID()); + return retValue; + } + retValue = new ArrayList(); + int size = get_ColumnCount(); + for (int i = 0; i < size; i++) { + int dt = p_info.getColumnDisplayType(i); + if (dt != DisplayType.ID && DisplayType.isID(dt)) { + MColumn col = MColumn.get(p_info.getColumn(i).AD_Column_ID); + if ("AD_Client_ID".equals(col.getColumnName())) { + // ad_client_id is verified with checkValidClient + continue; + } + String refTable = col.getReferenceTableName(); + retValue.add(new ValueNamePair(col.getColumnName(), refTable)); + } + } + if (retValue.size() == 0) { + retValue = null; + } + fks_cache.put(get_Table_ID(), retValue); + return retValue; + } + } // PO diff --git a/org.idempiere.webservices/WEB-INF/src/org/idempiere/adinterface/ModelADServiceImpl.java b/org.idempiere.webservices/WEB-INF/src/org/idempiere/adinterface/ModelADServiceImpl.java index 158debbdbc..920aea86eb 100644 --- a/org.idempiere.webservices/WEB-INF/src/org/idempiere/adinterface/ModelADServiceImpl.java +++ b/org.idempiere.webservices/WEB-INF/src/org/idempiere/adinterface/ModelADServiceImpl.java @@ -821,7 +821,10 @@ public class ModelADServiceImpl extends AbstractService implements ModelADServic retResp =invokeWSValidator(m_webservicetype, IWSValidator.TIMING_BEFORE_SAVE, po, fields,trx,requestCtx, resp, ret); if (retResp != null) return retResp; - + + if (!po.validForeignKeys()) + return rollbackAndSetError(trx, resp, ret, true, "Cannot save record in " + tableName + ": " + CLogger.retrieveErrorString("no log message")); + if (!po.save()) return rollbackAndSetError(trx, resp, ret, true, "Cannot save record in " + tableName + ": " + CLogger.retrieveErrorString("no log message")); @@ -1023,6 +1026,9 @@ public class ModelADServiceImpl extends AbstractService implements ModelADServic if (retResp != null) return retResp; + if (!po.validForeignKeys()) + return rollbackAndSetError(trx, resp, ret, true, "Cannot save record in " + tableName + ": " + CLogger.retrieveErrorString("no log message")); + if (!po.save()) return rollbackAndSetError(trx, resp, ret, true, "Cannot save record in " + tableName + ": " + CLogger.retrieveErrorString("no log message")); @@ -1295,6 +1301,9 @@ public class ModelADServiceImpl extends AbstractService implements ModelADServic if (retResp != null) return retResp; + if (!po.validForeignKeys()) + return rollbackAndSetError(trx, resp, ret, true, "Cannot save record in " + tableName + ": " + CLogger.retrieveErrorString("no log message")); + if (!po.save()) return rollbackAndSetError(trx, resp, ret, true, "Cannot save record in " + tableName + ": " + CLogger.retrieveErrorString("no log message"));