svn commit: r769963 - in /ofbiz/trunk: applications/securityext/servicedef/ applications/securityext/src/org/ofbiz/securityext/test/ applications/securityext/testdef/da/ applications/securityext/testdef/data/ framework/security/src/org/ofbiz/security/a...

[jaz-3]

Author: jaz
Date: Wed Apr 29 22:08:07 2009
New Revision: 769963

URL: http://svn.apache.org/viewvc?rev=769963&view=rev
Log:
Added check to prevent recursive permission lookup (when implementing dynamic access logic using services) - JIRA OFBIZ-2381

Modified:
    ofbiz/trunk/applications/securityext/servicedef/services.xml
    ofbiz/trunk/applications/securityext/src/org/ofbiz/securityext/test/AuthorizationTests.java
    ofbiz/trunk/applications/securityext/testdef/da/DynamicAccessTest.xml
    ofbiz/trunk/applications/securityext/testdef/data/SecurityTestData.xml
    ofbiz/trunk/framework/security/src/org/ofbiz/security/authz/AbtractAuthorization.java

Modified: ofbiz/trunk/applications/securityext/servicedef/services.xml
URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/securityext/servicedef/services.xml?rev=769963&r1=769962&r2=769963&view=diff
==============================================================================
--- ofbiz/trunk/applications/securityext/servicedef/services.xml (original)
+++ ofbiz/trunk/applications/securityext/servicedef/services.xml Wed Apr 29 22:08:07 2009
@@ -138,4 +138,8 @@
                 location="component://securityext/testdef/da/DynamicAccessTest.xml" invoke="testDa">
         <implements service="dynamicAccessInterface"/>
     </service>
+    <service name="dynamicAccessRecursiveTest" engine="simple" auth="false"
+                location="component://securityext/testdef/da/DynamicAccessTest.xml" invoke="testDaRecursion">
+        <implements service="dynamicAccessInterface"/>
+    </service>
 </services>

Modified: ofbiz/trunk/applications/securityext/src/org/ofbiz/securityext/test/AuthorizationTests.java
URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/securityext/src/org/ofbiz/securityext/test/AuthorizationTests.java?rev=769963&r1=769962&r2=769963&view=diff
==============================================================================
--- ofbiz/trunk/applications/securityext/src/org/ofbiz/securityext/test/AuthorizationTests.java (original)
+++ ofbiz/trunk/applications/securityext/src/org/ofbiz/securityext/test/AuthorizationTests.java Wed Apr 29 22:08:07 2009
@@ -60,4 +60,9 @@
         Debug.logInfo("Running testAutoGrantCleanup()", module);
         assertFalse("User was auto-granted an unexpected permission", security.hasPermission("user", "test:autogranted", null, true));
     }
+    
+    public void testDynamicAccessRecursion() throws Exception {
+        Debug.logInfo("Running testDynamicAccessRecursion()", module);
+        assertFalse("User was granted an unexpected permission", security.hasPermission("user", "test:recursion", null, true));
+    }
 }

Modified: ofbiz/trunk/applications/securityext/testdef/da/DynamicAccessTest.xml
URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/securityext/testdef/da/DynamicAccessTest.xml?rev=769963&r1=769962&r2=769963&view=diff
==============================================================================
--- ofbiz/trunk/applications/securityext/testdef/da/DynamicAccessTest.xml (original)
+++ ofbiz/trunk/applications/securityext/testdef/da/DynamicAccessTest.xml Wed Apr 29 22:08:07 2009
@@ -32,4 +32,12 @@
         </if-compare>
         <field-to-result field="permissionGranted"/>
     </simple-method>
+    
+    <simple-method method-name="testDaRecursion" short-description="Test recusion" login-required="false">
+        <set field="permissionGranted" value="false" type="Boolean"/>
+        <if-has-permission permission="test:recursion">
+            <set field="permissionGranted" value="true" type="Boolean"/>
+        </if-has-permission>
+        <field-to-result field="permissionGranted"/>
+    </simple-method>
 </simple-methods>        
\ No newline at end of file

Modified: ofbiz/trunk/applications/securityext/testdef/data/SecurityTestData.xml
URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/securityext/testdef/data/SecurityTestData.xml?rev=769963&r1=769962&r2=769963&view=diff
==============================================================================
--- ofbiz/trunk/applications/securityext/testdef/data/SecurityTestData.xml (original)
+++ ofbiz/trunk/applications/securityext/testdef/data/SecurityTestData.xml Wed Apr 29 22:08:07 2009
@@ -22,6 +22,7 @@
     <SecurityPermission permissionId="test:groovy1" dynamicAccess="component://securityext/testdef/da/DaTest1.groovy"/>
     <SecurityPermission permissionId="test:groovy2" dynamicAccess="org.ofbiz.securityext.test.DaTest2.groovy"/>
     <SecurityPermission permissionId="test:service" dynamicAccess="service:dynamicAccessTestService"/>
+    <SecurityPermission permissionId="test:recursion" dynamicAccess="service:dynamicAccessRecursiveTest"/>
     <SecurityPermission permissionId="test:autogranted" dynamicAccess=""/>    
     <SecurityPermissionAutoGrant permissionId="test:groovy1" grantPermission="test:autogranted"/>              
 </entity-engine-xml>
\ No newline at end of file

Modified: ofbiz/trunk/framework/security/src/org/ofbiz/security/authz/AbtractAuthorization.java
URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/security/src/org/ofbiz/security/authz/AbtractAuthorization.java?rev=769963&r1=769962&r2=769963&view=diff
==============================================================================
--- ofbiz/trunk/framework/security/src/org/ofbiz/security/authz/AbtractAuthorization.java (original)
+++ ofbiz/trunk/framework/security/src/org/ofbiz/security/authz/AbtractAuthorization.java Wed Apr 29 22:08:07 2009
@@ -17,6 +17,7 @@
  * Used to manage Auto-Grant permissions for the current "request"
  */
  private static ThreadLocal<List<String>> autoGrant = new ThreadLocal<List<String>>();
+ private static ThreadLocal<String> origPermission = new ThreadLocal<String>();
  private static ThreadLocal<String> uid = new ThreadLocal<String>();
 
  /**
@@ -69,18 +70,26 @@
 
  // verify the ThreadLocal data; make sure it isn't stale (from a thread pool)
         String threadUid = uid.get();
-        if (!userId.equals(threadUid)) {
+        if (threadUid != null && !userId.equals(threadUid)) {
+            origPermission.remove();
             autoGrant.remove();
             uid.remove();
+            threadUid = null;
         }
-
+        
+        // set the tracking values on thread local
+        if (UtilValidate.isEmpty(threadUid)) {
+            origPermission.set(permission);
+            uid.set(userId);
+        }
+                      
  // split the permission string; so we can walk up the levels
  String[] permSplit = expandedPermission.split(":");
  StringBuffer joined = new StringBuffer();
  int index = 1;
 
  if (permSplit != null && permSplit.length > 1) {
-    if (Debug.verboseOn()) Debug.logVerbose("Security 2.0 schema found -- walking tree : " + expandedPermission, module);
+    if (Debug.infoOn()) Debug.logInfo("Security 2.0 schema found -- walking tree : " + expandedPermission, module);
      // start walking
      for (String perm : permSplit) {
         if (permSplit.length >= index) {
@@ -112,10 +121,15 @@
      }
     
      // finally check dynamic permission (outside the loop)
-     if (hasDynamicPermission(userId, expandedPermission, context)) {
-        // permission granted
-        handleAutoGrantPermissions(userId, expandedPermission, context);
-        return true;
+     String threadPerm = origPermission.get();
+     if (!permission.equals(threadPerm)) {
+         if (hasDynamicPermission(userId, expandedPermission, context)) {
+            // permission granted
+            handleAutoGrantPermissions(userId, expandedPermission, context);
+            return true;
+         }
+     } else {
+        Debug.logWarning("Recursive permission check detected; do not call hasPermission() from a dynamic access implementation!", module);
      }
  } else {
     // legacy mode; only call static permission check; no auto grants
@@ -140,8 +154,7 @@
                     alreadyGranted.add(FlexibleStringExpander.expandString(toGrant, context));
                 }
             }
-            autoGrant.set(granted);
-            uid.set(userId);
+            autoGrant.set(granted);            
         }
  }
 }