[Zope-Checkins] SVN: Zope/branches/tseaver-collector_1774/lib/python/AccessControl/ Incorporate Jim's feedback:

[Tres Seaver]

Log message for revision 40437:
  Incorporate Jim's feedback:
  
   - Sync with Python version.  Notably, move check of user's roles *below*
     the test of ownership and proxy roles, which makes the error handling
     *much* cleaner.
  
   - In the case that we have a stack, but the top *doesn't* have proxy roles,
     fall back to the user roles.
  
   - In the case that we have a stack, and the top *does* have proxy roles,
     skip checking the user roles altogether (proxy rolse replace user roles).
  
  

Changed:
  U   Zope/branches/tseaver-collector_1774/lib/python/AccessControl/ImplPython.py
  U   Zope/branches/tseaver-collector_1774/lib/python/AccessControl/cAccessControl.c

-=-
Modified: Zope/branches/tseaver-collector_1774/lib/python/AccessControl/ImplPython.py
===================================================================
--- Zope/branches/tseaver-collector_1774/lib/python/AccessControl/ImplPython.py	2005-11-30 23:35:15 UTC (rev 40436)
+++ Zope/branches/tseaver-collector_1774/lib/python/AccessControl/ImplPython.py	2005-11-30 23:52:43 UTC (rev 40437)
@@ -490,6 +490,7 @@
                             # object is higher up than the owner, 
                             # deny access
                             return 0
+
                 for r in proxy_roles:
                     if r in roles:
                         return 1

Modified: Zope/branches/tseaver-collector_1774/lib/python/AccessControl/cAccessControl.c
===================================================================
--- Zope/branches/tseaver-collector_1774/lib/python/AccessControl/cAccessControl.c	2005-11-30 23:35:15 UTC (rev 40436)
+++ Zope/branches/tseaver-collector_1774/lib/python/AccessControl/cAccessControl.c	2005-11-30 23:52:43 UTC (rev 40437)
@@ -1295,7 +1295,7 @@
                                                     PyObject *args) {
 
     /* return value */
-    PyObject *rval = NULL;
+    PyObject *result = NULL;
 
     /* arguments, not increfe'd */
     PyObject *permission = NULL;
@@ -1346,17 +1346,6 @@
           roles = role_list;
     }
 
-    /*| result = context.user.allowed(object, roles)
-    */
-
-    user = PyObject_GetAttr(context, user_str);
-    if (user != NULL) {
-        ASSIGN(user, PyObject_GetAttr(user, allowed_str));
-        if (user != NULL) {
-            rval = callfunction2(user, object, roles);
-        }
-    }
-
   
     /*| # Check executable security
     **| stack = context.stack
@@ -1391,7 +1380,7 @@
                 if (owner == NULL) goto cP_done;
  
                 if (! PyObject_IsTrue(owner)) {
-                   rval = 0;
+                   result = PyInt_FromLong(0);
                    goto cP_done;
                 }
             }
@@ -1405,7 +1394,6 @@
     **|        # should not be able to use proxy roles to access items 
     **|        # above their subfolder!
     **|        owner = eo.getWrappedOwner()
-    **|                        
     **|        if owner is not None:
     **|            if object is not aq_base(object):
     **|                if not owner._check_context(object):
@@ -1416,19 +1404,18 @@
     **|        for r in proxy_roles:
     **|            if r in roles:
     **|                return 1
-    **|
-    **|    return result
+    **|         return 0
     */
         proxy_roles = PyObject_GetAttr(eo, _proxy_roles_str);
  
         if (proxy_roles == NULL) {
             PyErr_Clear();
-            goto cP_done;
+            goto cP_check_user;
         }
 
         if (PyObject_IsTrue(proxy_roles)) {
             method = PyObject_GetAttr(eo, getWrappedOwner_str);
-            if (method == NULL) goto cP_done;
+            if (method == NULL)  goto cP_done;
  
             wrappedowner = PyObject_CallObject(method, NULL);
             if (wrappedowner == NULL) goto cP_done;
@@ -1441,9 +1428,12 @@
                     incontext = callmethod1(wrappedowner,
                                             _check_context_str, object);
                     if (incontext == NULL) goto cP_done;
+ 
+                    if ( ! PyObject_IsTrue(incontext)) {
+                        ASSIGN(result, PyInt_FromLong(0));
+                        goto cP_done;
+                    }
                 }
- 
-                if ( ! PyObject_IsTrue(incontext)) goto cP_done;
             }
         }
  
@@ -1459,7 +1449,7 @@
         } else {
             PyObject *proxy_role;
             length = PySequence_Size(proxy_roles);
-            if (length < 0) contains = -1;
+            if (length < 0) goto cP_done;
             for (iRole=0; contains == 0 && iRole < length; iRole++) {
                 proxy_role = PySequence_GetItem(proxy_roles, iRole);
                 if (proxy_role == NULL) goto cP_done;
@@ -1469,10 +1459,27 @@
             }
         }
  
-        if (contains > 0)
-            rval = PyInt_FromLong(contains);
+        /* If we have proxy roles, and they don't match, then lose
+         * (don't even check user roles at that point)
+         */
+        if (contains < 0) contains = 0;
+        ASSIGN(result, PyInt_FromLong(contains));
+        goto cP_done;
     } /* End of stack check */
 
+cP_check_user:
+
+    /*| return context.user.allowed(object, roles)
+    */
+    user = PyObject_GetAttr(context, user_str);
+    if (user != NULL) {
+        ASSIGN(user, PyObject_GetAttr(user, allowed_str));
+        if (user != NULL) {
+            result = callfunction2(user, object, roles);
+            if (result == NULL) goto cP_done;
+        }
+    }
+
 cP_done:
     Py_XDECREF(roles);
     Py_XDECREF(user);
@@ -1485,7 +1492,7 @@
     Py_XDECREF(objectbase);
     Py_XDECREF(incontext);
 
-    return rval;
+    return result;
 }
 
 /*