Improved: Refactoring permission model call

(OFBIZ-7113)
With the problem to have a permission service not on the same transaction that the related service [1]

I realized the following improvement to
 * unified call evalPermission
 * move all related field for permission service on ModelService to ModelPermission
 * Remove deprecated code
 * add labelized error message  
 * call as same transaction the permission service by defautl with possibility to overide this rule 
 * add new attributes on permission model: require-new-transaction and return-error-on-failure

Thanks to Jacques Leroux for the review


git-svn-id: https://svn.apache.org/repos/asf/ofbiz/ofbiz-framework/trunk@1866137 13f79535-47bb-0310-9956-ffa450edef68

applications/accounting/minilang/ledger/GeneralLedgerServices.xml

@@ -2482,7 +2482,6 @@ under the License.
         <set-service-fields service-name="createAcctgTrans" map="newAcctgTrans" to-map="createAcctgTransInMap"/>
         <now-timestamp field="nowTimestamp"/>
         <set field="createAcctgTransInMap.transactionDate" from-field="nowTimestamp"/>
-        <set field="createAcctgTransInMap.isPosted" value="N"/>
         <set field="originalAcctgTransId" from-field="parameters.fromAcctgTransId"/>
         <field-to-result field="originalAcctgTransId" result-name="acctgTransId"/>
 

applications/accounting/minilang/payment/PaymentServices.xml

@@ -547,7 +547,7 @@ under the License.
         <iterate list="paymentApplications" entry="paymentApplication">
             <get-related-one relation-name="Invoice" value-field="paymentApplication" to-value-field="updateInvoiceCtx"/>
             <if-compare field="updateInvoiceCtx.statusId" operator="equals" value="INVOICE_PAID">
-                <set-service-fields service-name="updateInvoice" map="updateInvoiceCtx" to-map="invoiceStatusCtx"/>
+                <set-service-fields service-name="setInvoiceStatus" map="updateInvoiceCtx" to-map="invoiceStatusCtx"/>
                 <set field="invoiceStatusCtx.paidDate" type="Timestamp" value=""/>
                 <set field="invoiceStatusCtx.statusId" value="INVOICE_READY"/>
                 <call-service service-name="setInvoiceStatus" in-map-name="invoiceStatusCtx"/>

applications/commonext/servicedef/services.xml

@@ -19,7 +19,7 @@ under the License.
 -->
 
 <services xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
-        xsi:noNamespaceSchemaLocation="http://ofbiz.apache.org/dtds/services.xsd">
+          xsi:noNamespaceSchemaLocation="http://ofbiz.apache.org/dtds/services.xsd">
     <description>Commonext Component Services</description>
     <vendor>OFBiz</vendor>
     <version>1.0</version>

framework/service/config/ServiceErrorUiLabels.xml

@@ -67,7 +67,7 @@
         <value xml:lang="ja">取引先IDが正しくありません</value>
         <value xml:lang="nl">Relatie nummer van deze partij is niet gevonden</value>
         <value xml:lang="ro">Cod Subiect Lipseste</value>
-        <value xml:lang="th">ไม่ได้ใส่รหัสกลุ่มผู้ใช้</value>
+        <value xml:lang="th">aไม่ได้ใส่รหัสกลุ่มผู้ใช้</value>
         <value xml:lang="zh">找不到会员标识</value>
         <value xml:lang="zh-TW">無該團體識別</value>
     </property>
@@ -114,6 +114,26 @@
         <value xml:lang="zh">参数 ${parameterName} 中的标识（ID）值无效：${errorDetails}</value>
         <value xml:lang="zh-TW">參數 ${parameterName} 中的識別(識別)值無效:${errorDetails}</value>
     </property>
+    <property key="ServicePermissionError">
+        <value xml:lang="en">You haven't the permission for the service ${serviceName}, reason : ${failMessage}</value>
+        <value xml:lang="fr">Vous n'avez pas la permission sur le service ${serviceName}, raison : ${failMessage}</value>
+    </property>
+    <property key="ServicePermissionErrorDefinitionProblem">
+        <value xml:lang="en">Problem on permission service definition, please see with your integrator</value>
+        <value xml:lang="fr">Problème dans la définition d'un service de validation de permission, merci d'en informer votre intégrateur</value>
+    </property>
+    <property key="ServicePermissionErrorInvalidPermissionType">
+        <value xml:lang="en">The permission type call by the service isn't valid. Please see with your integrator</value>
+        <value xml:lang="fr">Le type de permission défini sur le service appelé n'est pas conforme, veuiller prendre contact avec votre intégrateur !</value>
+    </property>
+    <property key="ServicePermissionErrorRefused">
+        <value xml:lang="en">Access refused</value>
+        <value xml:lang="fr">Accés refusé</value>
+    </property>
+    <property key="ServicePermissionErrorUserLoginMissing">
+        <value xml:lang="en">User login is missing</value>
+        <value xml:lang="fr">Absence d'utilisateur identifié</value>
+    </property>
     <property key="ServiceTestDeadLockError">
         <value xml:lang="en">Error running deadlock test services: ${errorString}</value>
         <value xml:lang="it">Errore durante il test del servizio deadlock: ${errorString}</value>

framework/service/dtd/services.xsd

@@ -153,12 +153,28 @@ under the License.
                 </xs:restriction>
             </xs:simpleType>
         </xs:attribute>
+        <xs:attribute name="require-new-transaction" default="false">
+            <xs:simpleType>
+                <xs:restriction base="xs:token">
+                    <xs:enumeration value="true"/>
+                    <xs:enumeration value="false"/>
+                </xs:restriction>
+            </xs:simpleType>
+        </xs:attribute>
+        <xs:attribute name="return-error-on-failure" default="true">
+            <xs:annotation><xs:documentation>If set to false, when the permissions failed return the failMessage as error, else continue the service and give the hand to origin service to resolve the problem.</xs:documentation></xs:annotation>
+            <xs:simpleType>
+                <xs:restriction base="xs:token">
+                    <xs:enumeration value="true"/>
+                    <xs:enumeration value="false"/>
+                </xs:restriction>
+            </xs:simpleType>
+        </xs:attribute>
     </xs:attributeGroup>
     <xs:element name="required-permissions">
         <xs:complexType>
             <xs:sequence>
                 <xs:element minOccurs="0" maxOccurs="unbounded" ref="check-permission"/>
-                <xs:element minOccurs="0" maxOccurs="unbounded" ref="check-role-member"/>
                 <xs:element minOccurs="0" maxOccurs="unbounded" ref="permission-service"/>
             </xs:sequence>
             <xs:attributeGroup ref="attlist.required-permissions"/>
@@ -183,19 +199,6 @@ under the License.
         <xs:attribute name="permission" type="xs:string" use="required"/>
         <xs:attribute name="action" type="xs:string"/>
     </xs:attributeGroup>
-    <xs:element name="check-role-member">
-        <xs:annotation>
-            <xs:documentation>
-                    This is deprecated
-            </xs:documentation>
-        </xs:annotation>   
-        <xs:complexType>
-            <xs:attributeGroup ref="attlist.check-role-member"/>
-        </xs:complexType>
-    </xs:element>
-    <xs:attributeGroup name="attlist.check-role-member">
-        <xs:attribute name="role-type" type="xs:string" use="required"/>
-    </xs:attributeGroup>
     <xs:element name="service-security">
         <xs:complexType>
             <xs:attributeGroup ref="attlist.service-security"/>

framework/service/servicedef/services_test_se.xml

@@ -166,4 +166,50 @@ under the License.
     <service name="testXmlRpcClientAdd" engine="java" auth="false" location="org.apache.ofbiz.service.test.XmlRpcTests" invoke="testXmlRpcClientAdd">
         <implements service="testServiceInterface"/>
     </service>
+
+    <!--Test permission-->
+    <service name="testSimplePermission" engine="java" auth="true"
+             location="org.apache.ofbiz.service.test.ServiceEngineTestPermissionServices" invoke="genericTestService">
+        <required-permissions join-type="AND">
+            <check-permission permission="TESTPERM" action="_UPDATE"/>
+        </required-permissions>
+    </service>
+    <service name="testSimpleServicePermission" engine="java" auth="true"
+             location="org.apache.ofbiz.service.test.ServiceEngineTestPermissionServices" invoke="genericTestService">
+        <permission-service service-name="testPermissionPing"/>
+        <attribute name="givePermission" type="String" mode="IN"/>
+    </service>
+    <service name="testSimpleGroupAndPermission" engine="java" auth="true"
+             location="org.apache.ofbiz.service.test.ServiceEngineTestPermissionServices" invoke="genericTestService">
+        <required-permissions join-type="AND">
+            <check-permission permission="TESTPERM" action="_UPDATE"/>
+            <permission-service service-name="testPermissionPing"/>
+        </required-permissions>
+        <attribute name="givePermission" type="String" mode="IN"/>
+    </service>
+    <service name="testSimpleGroupOrPermission" engine="java" auth="true"
+             location="org.apache.ofbiz.service.test.ServiceEngineTestPermissionServices" invoke="genericTestService">
+        <required-permissions join-type="OR">
+            <check-permission permission="TESTPERM" action="_UPDATE"/>
+            <permission-service service-name="testPermissionPing"/>
+        </required-permissions>
+        <attribute name="givePermission" type="String" mode="IN"/>
+    </service>
+    <service name="testServicePermissionWithTransaction" engine="java" auth="true"
+             location="org.apache.ofbiz.service.test.ServiceEngineTestPermissionServices" invoke="genericTestService">
+        <permission-service service-name="testPermissionPing" require-new-transaction="true"/>
+        <attribute name="testingId" type="String" mode="IN"/>
+    </service>
+    <service name="testPermissionPing" engine="java" auth="true"
+             location="org.apache.ofbiz.service.test.ServiceEngineTestPermissionServices" invoke="testPermissionPing">
+        <implements service="permissionInterface"/>
+        <attribute name="givePermission" type="String" mode="IN"/>
+    </service>
+    <service name="testPermissionExistingTesting" engine="java" auth="true"
+             location="org.apache.ofbiz.service.test.ServiceEngineTestPermissionServices" invoke="testPermissionExistingTesting">
+        <implements service="permissionInterface"/>
+        <attribute name="testingId" type="String" mode="IN"/>
+    </service>
+
+
 </services>

framework/service/src/main/java/org/apache/ofbiz/service/ModelPermGroup.java

@@ -19,6 +19,7 @@
 package org.apache.ofbiz.service;
 
 import java.io.Serializable;
+import java.util.ArrayList;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
@@ -38,21 +39,26 @@ public class ModelPermGroup implements Serializable {
 
     public List<ModelPermission> permissions = new LinkedList<>();
     public String joinType;
-
-    public boolean evalPermissions(DispatchContext dctx, Map<String, ? extends Object> context) {
-        if (UtilValidate.isNotEmpty(permissions))  {
+    public Map<String, Object> evalPermissions(DispatchContext dctx, Map<String, ? extends Object> context) {
+        List<String> permissionErrors = new ArrayList<String>();
+        if (UtilValidate.isNotEmpty(permissions)) {
             boolean foundOne = false;
             for (ModelPermission perm: permissions) {
-                if (perm.evalPermission(dctx, context)) {
+                Map<String, Object> permResult = perm.evalPermission(dctx, context);
+                if (ServiceUtil.isSuccess(permResult)) {
                     foundOne = true;
                 } else {
+                    ServiceUtil.addErrors(permissionErrors, null, permResult);
                     if (joinType.equals(PERM_JOIN_AND)) {
-                        return false;
+                        return permResult;
                     }
                 }
             }
-            return foundOne;
+            if (! foundOne) {
+                return ServiceUtil.returnError(permissionErrors);
+            }
         }
-        return true;
+        return ServiceUtil.returnSuccess();
     }
 }
+

framework/service/src/main/java/org/apache/ofbiz/service/ModelPermission.java

@@ -19,9 +19,11 @@
 package org.apache.ofbiz.service;
 
 import java.io.Serializable;
+import java.util.Locale;
 import java.util.Map;
 
 import org.apache.ofbiz.base.util.Debug;
+import org.apache.ofbiz.base.util.UtilProperties;
 import org.apache.ofbiz.base.util.UtilValidate;
 import org.apache.ofbiz.entity.GenericValue;
 import org.apache.ofbiz.security.Security;
@@ -43,28 +45,56 @@ public class ModelPermission implements Serializable {
     public String nameOrRole = null;
     public String action = null;
     public String permissionServiceName = null;
+    public String permissionMainAction = null;
     public String permissionResourceDesc = null;
+    public boolean permissionRequireNewTransaction = false;
+    public boolean permissionReturnErrorOnFailure = true;
     public Boolean auth;
-    public String clazz = null;
 
-    public boolean evalPermission(DispatchContext dctx, Map<String, ? extends Object> context) {
+    public static final String resource = "ServiceErrorUiLabels";
+
+    @Override
+    public String toString() {
+        StringBuilder buf = new StringBuilder();
+        buf.append(serviceModel.name).append("::");
+        buf.append(permissionType).append("::");
+        buf.append(nameOrRole).append("::");
+        buf.append(action).append("::");
+        buf.append(permissionServiceName).append("::");
+        buf.append(permissionMainAction).append("::");
+        buf.append(permissionResourceDesc).append("::");
+        buf.append(permissionRequireNewTransaction).append("::");
+        buf.append(permissionReturnErrorOnFailure).append("::");
+        return buf.toString();
+    }
+
+    public Map<String, Object> evalPermission(DispatchContext dctx, Map<String, ? extends Object> context) {
         GenericValue userLogin = (GenericValue) context.get("userLogin");
+        Locale locale = (Locale) context.get("locale");
         Security security = dctx.getSecurity();
         if (userLogin == null) {
             Debug.logInfo("Secure service requested with no userLogin object", module);
-            return false;
+            return ServiceUtil.returnError(UtilProperties.getMessage(resource, "ServicePermissionErrorUserLoginMissing", locale));
         }
+        boolean hasPermission = false;
+        if (Debug.verboseOn()) Debug.logVerbose(" Permission : Analyse " + this.toString(), module);
         switch (permissionType) {
             case PERMISSION:
-                return evalSimplePermission(security, userLogin);
+                hasPermission = evalSimplePermission(security, userLogin);
+                break;
             case ENTITY_PERMISSION:
-                return evalEntityPermission(security, userLogin);
+                hasPermission = evalEntityPermission(security, userLogin);
+                break;
             case PERMISSION_SERVICE:
                 return evalPermissionService(serviceModel, dctx, context);
             default:
                 Debug.logWarning("Invalid permission type [" + permissionType + "] for permission named : " + nameOrRole + " on service : " + serviceModel.name, module);
-                return false;
+                return ServiceUtil.returnError(UtilProperties.getMessage(resource, "ServicePermissionErrorInvalidPermissionType", locale));
         }
+        if (! hasPermission) {
+            return ServiceUtil.returnError(UtilProperties.getMessage(resource, "ServicePermissionErrorRefused", locale));
+        }
+        return ServiceUtil.returnSuccess();
     }
 
     private boolean evalSimplePermission(Security security, GenericValue userLogin) {
@@ -77,7 +107,7 @@ private boolean evalSimplePermission(Security security, GenericValue userLogin)
 
     private boolean evalEntityPermission(Security security, GenericValue userLogin) {
         if (nameOrRole == null) {
-            Debug.logWarning("Null permission name passed for evaluation", module);
+            Debug.logError("Null permission name passed for evaluation", module);
             return false;
         }
         if (action == null) {
@@ -86,18 +116,21 @@ private boolean evalEntityPermission(Security security, GenericValue userLogin)
         return security.hasEntityPermission(nameOrRole, action, userLogin);
     }
 
-    private boolean evalPermissionService(ModelService origService, DispatchContext dctx, Map<String, ? extends Object> context) {
+    private Map<String, Object> evalPermissionService(ModelService origService, DispatchContext dctx, Map<String, ? extends Object> context) {
+        LocalDispatcher dispatcher = dctx.getDispatcher();
         ModelService permission;
+        Locale locale = (Locale) context.get("locale");
         if (permissionServiceName == null) {
             Debug.logWarning("No ModelService found; no service name specified!", module);
-            return false;
+            return ServiceUtil.returnError(UtilProperties.getMessage(resource, "ServicePermissionErrorDefinitionProblem", locale));
         }
         try {
             permission = dctx.getModelService(permissionServiceName);
         } catch (GenericServiceException e) {
             Debug.logError(e, "Failed to get ModelService: " + e.toString(), module);
-            return false;
+            return ServiceUtil.returnError(UtilProperties.getMessage(resource, "ServicePermissionErrorDefinitionProblem", locale));
         }
+
         permission.auth = true;
         Map<String, Object> ctx = permission.makeValid(context, ModelService.IN_PARAM);
         if (UtilValidate.isNotEmpty(action)) {
@@ -108,20 +141,25 @@ private boolean evalPermissionService(ModelService origService, DispatchContext
         } else if (origService != null) {
             ctx.put("resourceDescription", origService.name);
         }
-        LocalDispatcher dispatcher = dctx.getDispatcher();
         Map<String, Object> resp;
         String failMessage = null;
         try {
-            resp = dispatcher.runSync(permission.name,  ctx, 300, true);
+            if (permissionRequireNewTransaction) {
+                resp = dispatcher.runSync(permission.name, ctx, 300, true);
+            } else {
+                resp = dispatcher.runSync(permission.name, ctx);
+            }
             failMessage = (String) resp.get("failMessage");
         } catch (GenericServiceException e) {
-            Debug.logError(null + e.getMessage(), module);
-            return false;
+            Debug.logError(failMessage + e.getMessage(), module);
+            return ServiceUtil.returnError(UtilProperties.getMessage(resource, "ServicePermissionErrorDefinitionProblem", locale));
         }
-        if (ServiceUtil.isError(resp) || ServiceUtil.isFailure(resp)) {
-            Debug.logError(failMessage, module);
-            return false;
+        if (Debug.verboseOn()) Debug.logVerbose("Service permision result : hasPermission " + resp.get("hasPermission") + ", failMessage " + failMessage , module);
+        if (permissionReturnErrorOnFailure &&
+                (UtilValidate.isNotEmpty(failMessage) || ! ((Boolean) resp.get("hasPermission")).booleanValue())) {
+            if (UtilValidate.isEmpty(failMessage)) failMessage = UtilProperties.getMessage(resource, "ServicePermissionErrorRefused", locale);
+            return ServiceUtil.returnError(failMessage);
         }
-        return (Boolean) resp.get("hasPermission");
+        return resp;
     }
 }