[ofbiz-framework] branch trunk updated: Fixed: Refactoring permission model call, alone permission service failed (OFBIZ-11440)

[nmalin]

This is an automated email from the ASF dual-hosted git repository.

nmalin pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git


The following commit(s) were added to refs/heads/trunk by this push:
     new 9a50fa9  Fixed: Refactoring permission model call, alone permission service failed (OFBIZ-11440)
9a50fa9 is described below

commit 9a50fa907b041df6907a15d6742a5527fcbf610d
Author: Nicolas Malin <[hidden email]>
AuthorDate: Wed Mar 4 18:36:13 2020 +0100

    Fixed: Refactoring permission model call, alone permission service failed
    (OFBIZ-11440)
   
    During refactoring (OFBIZ-7113), the service reader used two different methods to read a
    permission service definition if is present alone or in a group. For the alone version a link
    was missing between the service and the permission service to resolve correctly the permission.
   
    To solve this problem I reorganized to use only one method to initialize a service model permission.
   
    Other point, an attribute renaming was missed : action -> permissionMainAction, when service permission call was called
   
    Last, these problems weren't detected before due to missing unit test on failure service permission, now cover.
---
 .../org/apache/ofbiz/service/ModelPermission.java  |  6 ++--
 .../apache/ofbiz/service/ModelServiceReader.java   | 42 ++++++++++++----------
 .../ofbiz/service/test/ServicePermissionTests.java | 10 ++++++
 3 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/framework/service/src/main/java/org/apache/ofbiz/service/ModelPermission.java b/framework/service/src/main/java/org/apache/ofbiz/service/ModelPermission.java
index 0336490..b4f070d 100644
--- a/framework/service/src/main/java/org/apache/ofbiz/service/ModelPermission.java
+++ b/framework/service/src/main/java/org/apache/ofbiz/service/ModelPermission.java
@@ -133,8 +133,8 @@ public class ModelPermission implements Serializable {
 
         permission.auth = true;
         Map<String, Object> ctx = permission.makeValid(context, ModelService.IN_PARAM);
-        if (UtilValidate.isNotEmpty(action)) {
-            ctx.put("mainAction", action);
+        if (UtilValidate.isNotEmpty(permissionMainAction)) {
+            ctx.put("mainAction", permissionMainAction);
         }
         if (UtilValidate.isNotEmpty(permissionResourceDesc)) {
             ctx.put("resourceDescription", permissionResourceDesc);
@@ -154,7 +154,7 @@ public class ModelPermission implements Serializable {
             Debug.logError(failMessage + e.getMessage(), module);
             return ServiceUtil.returnError(UtilProperties.getMessage(resource, "ServicePermissionErrorDefinitionProblem", locale));
         }
-        if (Debug.verboseOn()) Debug.logVerbose("Service permision result : hasPermission " + resp.get("hasPermission") + ", failMessage " + failMessage , module);
+        if (Debug.verboseOn()) Debug.logVerbose("Service permission 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);
diff --git a/framework/service/src/main/java/org/apache/ofbiz/service/ModelServiceReader.java b/framework/service/src/main/java/org/apache/ofbiz/service/ModelServiceReader.java
index 9ef2f6a..3b45ecb 100644
--- a/framework/service/src/main/java/org/apache/ofbiz/service/ModelServiceReader.java
+++ b/framework/service/src/main/java/org/apache/ofbiz/service/ModelServiceReader.java
@@ -243,7 +243,7 @@ public class ModelServiceReader implements Serializable {
         // construct the context
         service.contextInfo = new HashMap<>();
         createNotification(serviceElement, service);
-        createPermission(serviceElement, service);
+        createAloneServicePermission(serviceElement, service);
         createPermGroups(serviceElement, service);
         createGroupDefs(serviceElement, service);
         createImplDefs(serviceElement, service);
@@ -305,15 +305,25 @@ public class ModelServiceReader implements Serializable {
         }
     }
 
-    private static void createPermission(Element baseElement, ModelService model) {
+    private static ModelPermission createServicePermission(Element e, ModelService model) {
+        ModelPermission modelPermission = new ModelPermission();
+        modelPermission.permissionType = ModelPermission.PERMISSION_SERVICE;
+        modelPermission.permissionServiceName = e.getAttribute("service-name");
+        modelPermission.permissionMainAction = e.getAttribute("main-action");
+        modelPermission.permissionResourceDesc = e.getAttribute("resource-description");
+        modelPermission.permissionRequireNewTransaction = !"false".equalsIgnoreCase(e.getAttribute("require-new-transaction"));
+        modelPermission.serviceModel = model;
+        return modelPermission;
+    }
+
+    private static void createAloneServicePermission(Element baseElement, ModelService model) {
         Element e = UtilXml.firstChildElement(baseElement, "permission-service");
         if (e != null) {
-            ModelPermission modelPermission = new ModelPermission();
-            modelPermission.permissionServiceName = e.getAttribute("service-name");
-            modelPermission.permissionMainAction = e.getAttribute("main-action");
-            modelPermission.permissionResourceDesc = e.getAttribute("resource-description");
-            modelPermission.permissionRequireNewTransaction = !"false".equalsIgnoreCase(e.getAttribute("require-new-transaction"));
-            model.auth = true; // auth is always required when permissions are set
+            ModelPermission modelPermission = createServicePermission(e, model);
+            model.modelPermission = modelPermission;
+
+            // auth is always required when permissions are set
+            model.auth = true;
         }
     }
 
@@ -343,17 +353,11 @@ public class ModelServiceReader implements Serializable {
 
         // Create the permissions based on permission services
         for (Element element : UtilXml.childElementList(baseElement, "permission-service")) {
-            ModelPermission perm = new ModelPermission();
-            if (baseElement != null) {
-                perm.permissionType = ModelPermission.PERMISSION_SERVICE;
-                perm.permissionServiceName = element.getAttribute("service-name");
-                perm.action = element.getAttribute("main-action");
-                perm.permissionResourceDesc = element.getAttribute("resource-description");
-                perm.permissionRequireNewTransaction = !"false".equalsIgnoreCase(element.getAttribute("require-new-transaction"));
-                perm.auth = true; // auth is always required when permissions are set
-                perm.serviceModel = service;
-                group.permissions.add(perm);
-            }
+            group.permissions.add(createServicePermission(element, service));
+        }
+        if (UtilValidate.isNotEmpty(group.permissions)) {
+            // auth is always required when permissions are set
+            service.auth = true;
         }
     }
 
diff --git a/framework/service/src/main/java/org/apache/ofbiz/service/test/ServicePermissionTests.java b/framework/service/src/main/java/org/apache/ofbiz/service/test/ServicePermissionTests.java
index e400ebd..09d5fa8 100644
--- a/framework/service/src/main/java/org/apache/ofbiz/service/test/ServicePermissionTests.java
+++ b/framework/service/src/main/java/org/apache/ofbiz/service/test/ServicePermissionTests.java
@@ -43,6 +43,16 @@ public class ServicePermissionTests extends OFBizTestCase {
         assertTrue(ServiceUtil.isSuccess(results));
     }
 
+    public void testServicePermissionError() throws Exception {
+        Map<String, Object> testingPermMap = UtilMisc.toMap("userLogin", getUserLogin("permUser1"), "givePermission", "N");
+        try {
+            Map<String, Object> results = dispatcher.runSync("testSimpleServicePermission", testingPermMap);
+            assertFalse("The testGroupPermission don't raise service exception", ServiceUtil.isError(results));
+        } catch (ServiceAuthException e) {
+            assertNotNull(e);
+        }
+    }
+
     public void testGroupPermissionSuccess() throws Exception {
         Map<String, Object> testingPermMap = UtilMisc.toMap("userLogin", getUserLogin("permUser1"), "givePermission", "Y");
         Map<String, Object> results = dispatcher.runSync("testSimpleGroupAndPermission", testingPermMap);