[Zope-Checkins] CVS: Zope/lib/python/AccessControl - PermissionRole.py:1.18 ZopeGuards.py:1.16 ZopeSecurityPolicy.py:1.23 cAccessControl.c:1.20

Shane Hathaway shane@zope.com
Tue, 10 Jun 2003 11:39:35 -0400


Update of /cvs-repository/Zope/lib/python/AccessControl
In directory cvs.zope.org:/tmp/cvs-serv31796

Modified Files:
	PermissionRole.py ZopeGuards.py ZopeSecurityPolicy.py 
	cAccessControl.c 
Log Message:
Merged shane-security-policy-branch.

The Zope security policy now raises Unauthorized for all denied
access.  This is designed to make it easier to diagnose problems in
security settings, since the Unauthorized error will propagate to
something that can display or log the error.

All 2000+ of the Zope unit tests pass with this change, but I suspect
there may be code that expects the security policy to return 0 instead
of raising Unauthorized.  If any severe issues surface, we can revert
this change.



=== Zope/lib/python/AccessControl/PermissionRole.py 1.17 => 1.18 ===
--- Zope/lib/python/AccessControl/PermissionRole.py:1.17	Tue Oct  1 10:09:46 2002
+++ Zope/lib/python/AccessControl/PermissionRole.py	Tue Jun 10 11:39:04 2003
@@ -64,7 +64,7 @@
         def __init__(self, name, default=('Manager',)):
             self.__name__=name
             self._p='_'+string.translate(name,name_trans)+"_Permission"
-            self._d=default
+            self._d = self.__roles__ = default
 
         def __of__(self, parent, getattr=getattr):
             r=imPermissionRole()


=== Zope/lib/python/AccessControl/ZopeGuards.py 1.15 => 1.16 ===
--- Zope/lib/python/AccessControl/ZopeGuards.py:1.15	Tue Jan 14 10:03:03 2003
+++ Zope/lib/python/AccessControl/ZopeGuards.py	Tue Jun 10 11:39:04 2003
@@ -41,6 +41,11 @@
 
 
     def guarded_getattr(inst, name, default=_marker):
+        """Retrieves an attribute, checking security in the process.
+
+        Raises Unauthorized if the attribute is found but the user is
+        not allowed to access the attribute.
+        """
         if name[:1] != '_':
             # Try to get the attribute normally so that unusual
             # exceptions are caught early.
@@ -55,12 +60,7 @@
             validate = getSecurityManager().validate
             # Filter out the objects we can't access.
             if hasattr(inst, 'aq_acquire'):
-                try:
-                    return inst.aq_acquire(name, aq_validate, validate)
-                except AttributeError:
-                    # A denial of access was converted into an
-                    # AttributeError.  Convert it back.
-                    raise Unauthorized, name
+                return inst.aq_acquire(name, aq_validate, validate)
             # Or just try to get the attribute directly.
             if validate(inst, inst, name, v):
                 return v


=== Zope/lib/python/AccessControl/ZopeSecurityPolicy.py 1.22 => 1.23 ===
--- Zope/lib/python/AccessControl/ZopeSecurityPolicy.py:1.22	Mon Jun  9 12:26:39 2003
+++ Zope/lib/python/AccessControl/ZopeSecurityPolicy.py	Tue Jun 10 11:39:04 2003
@@ -81,19 +81,13 @@
                      Containers=SimpleObjectPolicies.Containers,
                      valid_aq_=('aq_parent','aq_inner', 'aq_explicit')):
 
+            # Note: accessed is not used.
 
             ############################################################
             # Provide special rules for the acquisition attributes
             if type(name) is StringType:
                 if name.startswith('aq_') and name not in valid_aq_:
-                    return 0
-
-            containerbase = aq_base(container)
-            accessedbase = aq_base(accessed)
-            if accessedbase is accessed:
-                # accessed is not a wrapper, so assume that the
-                # value could not have been acquired.
-                accessedbase = container
+                    raise Unauthorized(name, value)
 
             ############################################################
             # If roles weren't passed in, we'll try to get them from the object
@@ -111,26 +105,22 @@
                 # of roles passed in. Presumably, the value is some simple
                 # object like a string or a list.  We'll try to get roles
                 # from its container.
-                if container is None: return 0 # Bail if no container
+                if container is None:
+                    # Either container or a list of roles is required
+                    # for ZopeSecurityPolicy to know whether access is
+                    # allowable.
+                    raise Unauthorized(name, value)
 
                 roles=getattr(container, '__roles__', _noroles)
                 if roles is _noroles:
-                    # Try to acquire __roles__.  If it can't be
-                    # acquired, the value is unprotected.  Deny access
-                    # to acquired unprotected values even if they are
-                    # in a simple container.
-                    if containerbase is container:
-                        # Container is not wrapped.
-                        roles=_noroles
-                        if containerbase is not accessedbase:
-                            return 0
-                    else:
-                        # Try to acquire roles
-                        try: roles = container.aq_acquire('__roles__')
+                    # Try to acquire __roles__.  If __roles__ can't be
+                    # acquired, the value is unprotected and roles is
+                    # left set to _noroles.
+                    if aq_base(container) is not container:
+                        try:
+                            roles = container.aq_acquire('__roles__')
                         except AttributeError:
-                            roles=_noroles
-                            if containerbase is not accessedbase:
-                                return 0
+                            pass
 
                 # We need to make sure that we are allowed to
                 # get unprotected attributes from the container. We are
@@ -151,10 +141,7 @@
                             p=p(name, value)
 
                 if not p:
-                    if (containerbase is accessedbase):
-                        raise Unauthorized(name, value)
-                    else:
-                        return 0
+                    raise Unauthorized(name, value)
 
                 if roles is _noroles: return 1
 
@@ -183,9 +170,7 @@
                     if (owner is not None) and not owner.allowed(value, roles):
                         # We don't want someone to acquire if they can't
                         # get an unacquired!
-                        if accessedbase is containerbase:
-                            raise Unauthorized(name, value)
-                        return 0
+                        raise Unauthorized(name, value)
 
                 # Proxy roles, which are a lot safer now.
                 proxy_roles=getattr(eo, '_proxy_roles', None)
@@ -194,10 +179,7 @@
                         if r in roles: return 1
 
                     # Proxy roles actually limit access!
-                    if accessedbase is containerbase:
-                        raise Unauthorized(name, value)
-
-                    return 0
+                    raise Unauthorized(name, value)
 
 
             try:
@@ -205,11 +187,8 @@
                     return 1
             except AttributeError: pass
 
-            # We don't want someone to acquire if they can't get an unacquired!
-            if accessedbase is containerbase:
-                raise Unauthorized(name, value)
+            raise Unauthorized(name, value)
 
-            return 0
 
         def checkPermission(self, permission, object, context):
             # XXX proxy roles and executable owner are not checked


=== Zope/lib/python/AccessControl/cAccessControl.c 1.19 => 1.20 ===
--- Zope/lib/python/AccessControl/cAccessControl.c:1.19	Tue Jan 14 10:03:05 2003
+++ Zope/lib/python/AccessControl/cAccessControl.c	Tue Jun 10 11:39:04 2003
@@ -733,7 +733,7 @@
 */
 
 static PyObject *ZopeSecurityPolicy_validate(PyObject *self, PyObject *args) {
-	PyObject *accessed = NULL;
+	PyObject *accessed = NULL;  /* Note: accessed is not used. */
 	PyObject *container = NULL;
 	PyObject *name = NULL;
 	PyObject *value = NULL;
@@ -742,8 +742,6 @@
         /* Import from SimpleObject Policy._noroles */
         /* Note that _noroles means missing roles, spelled with a NULL in C.
            Jim. */
-	PyObject *containerbase = NULL;
-	PyObject *accessedbase = NULL;
 	PyObject *p = NULL;
 	PyObject *rval = NULL;
 	PyObject *stack = NULL;
@@ -762,7 +760,7 @@
 	/*| # Provide special rules for acquisition attributes
 	**| if type(name) is StringType:
 	**|     if name[:3] == 'aq_' and name not in valid_aq_:
-	**|	   return 0
+	**|	   raise Unauthorized(name, value)
 	*/ 
 
 	if (PyString_Check(name)) {		/* XXX what about unicode? */
@@ -771,28 +769,15 @@
 			if (strcmp(sname,"aq_parent")   != 0 &&
                             strcmp(sname,"aq_inner") != 0 &&
                             strcmp(sname,"aq_explicit") != 0) {
-				/* Access control violation, return 0 */
-				return PyInt_FromLong(0);
+				/* Access control violation */
+				unauthErr(name, value);
+				return NULL;  /* roles is not owned yet */
 			}
 		}
 	}
 
 	Py_XINCREF(roles);	/* Convert the borrowed ref to a real one */
 
-	/*| containerbase = aq_base(container)
-	**| accessedbase = getattr(accessed, 'aq_base', container)
-	*/
-
-	containerbase = aq_base(container);
-	if (containerbase == NULL) goto err;
-	
-	if (aq_isWrapper(accessed))
-		accessedbase = aq_base(accessed);
-	else {
-		Py_INCREF(container);
-		accessedbase = container;
-	}
-
 	/*| # If roles weren't passed in, we'll try to get them from
 	**| # the object
 	**|
@@ -818,48 +803,33 @@
 		**| # is some simple object like a string or a list.
 		**| # We'll try to get roles from it's container
 		**|
-		**| if container is None: return 0  # Bail if no container
+		**| if container is None: raise Unauthorized(name, value)
 		*/
 
 		if (container == Py_None)  {
-			rval= PyInt_FromLong(0);
+			unauthErr(name, value);
 			goto err;
 		}
 
 		/*| roles = getattr(container, "__roles__", _noroles)
-		**| if roles is _noroles:
-		**|    aq = getattr(container, 'aq_acquire', None)
-		**|    if aq is None:
-		**|       roles = _noroles
-		**|       if containerbase is not accessedbase: return 0
-		**|    else:
-		**|       # Try to acquire roles
-		**|      try: roles = aq('__roles__')
-		**|      except AttributeError:
-		**|	    roles = _noroles
-		**|         if containerbase is not accessedbase: return 0
+                **| if roles is _noroles:
+                **|     if aq_base(container) is not container:
+                **|         try:
+                **|             roles = container.aq_acquire('__roles__')
+                **|         except AttributeError:
+                **|             pass
 		*/
                 roles = PyObject_GetAttr(container, __roles__);
 		if (roles == NULL) {
 			PyErr_Clear();
 
-			if (!aq_isWrapper(container)) {
-				if (containerbase != accessedbase)  {
-					rval = PyInt_FromLong(0);
-					goto err;
-				}
-			} 
-                        else {
+			if (aq_isWrapper(container)) {
 				roles = aq_acquire(container, __roles__);
 				if (roles == NULL) {
                                   if (PyErr_ExceptionMatches(
                                       PyExc_AttributeError))
                                     {
                                         PyErr_Clear();
-				        if (containerbase != accessedbase) {
-						rval = PyInt_FromLong(0);
-						goto err;
-					}
                                     }
                                   else
                                     goto err;
@@ -879,7 +849,6 @@
 		**|        "__allow_access_to_unprotected_subobjects__", None)
 		*/
 
-		/** XXX do we need to incref this stuff?  I dont think so */
 		p = callfunction2(Containers, OBJECT(container->ob_type),
                                   Py_None);
 		if (p == NULL)
@@ -916,21 +885,13 @@
 		}
 
 		/*| if not p:
-		**|    if containerbase is accessedbase:
-		**|	  raise Unauthorized, cleanupName(name, value)
-		**|    else:
-		**|       return 0
+		**|     raise Unauthorized, cleanupName(name, value)
 		*/
                
 		if (p == NULL || ! PyObject_IsTrue(p)) {
-			Py_XDECREF(p);
-			if (containerbase == accessedbase) {
-				unauthErr(name, value);
-				goto err;
-			} else {
-				rval = PyInt_FromLong(0);
-				goto err;
-			}
+                  Py_XDECREF(p);
+                  unauthErr(name, value);
+                  goto err;
 		}
                 else
                   Py_DECREF(p);
@@ -1014,10 +975,8 @@
 	**|    if (owner is not None) and not owner.allowed(value, roles)
 	**| 	  # We don't want someone to acquire if they can't 
 	**|	  # get an unacquired!
-	**|	  if accessedbase is containerbase:
-	**|	     raise Unauthorized, ('You are not authorized to'
-	**|		'access <em>%s</em>.' % cleanupName(name, value))
-	**|       return 0
+	**|       raise Unauthorized, ('You are not authorized to'
+	**|	      'access <em>%s</em>.' % cleanupName(name, value))
 	*/
 
 		eo = PySequence_GetItem(stack, -1);
@@ -1047,10 +1006,7 @@
                     {
                       Py_DECREF(owner);
                       Py_DECREF(eo);
-                      if (accessedbase == containerbase) {
-                        unauthErr(name, value);
-                      } 
-                      else rval = PyInt_FromLong(0);
+                      unauthErr(name, value);
                       goto err;
                     }
 		}
@@ -1065,11 +1021,8 @@
 	**|          if r in roles: return 1
 	**|
 	**|       # proxy roles actually limit access!
-	**|       if accessedbase is containerbase:
-	**|	     raise Unauthorized, ('You are not authorized to access'
-	**|		'<em>%s</em>.' % cleanupName(name, value))
-	**|
-	**|	  return 0
+	**|	  raise Unauthorized, ('You are not authorized to access'
+	**|	      '<em>%s</em>.' % cleanupName(name, value))
 	*/
 		proxy_roles = PyObject_GetAttr(eo, _proxy_roles_str);
                 Py_DECREF(eo);
@@ -1111,12 +1064,9 @@
                     Py_DECREF(proxy_roles);
 
                     if (contains > 0)
-                      rval = PyInt_FromLong(contains);
+                      rval = PyInt_FromLong(1);
                     else if (contains == 0) {
-                      if (accessedbase == containerbase) {
-                        unauthErr(name, value);
-                      }
-                      else rval = PyInt_FromLong(contains);
+                      unauthErr(name, value);
                     }
                     goto err;
                   }
@@ -1152,25 +1102,15 @@
           }
         } /* End of authentiction skip for public only access */
 
-	/*| # we don't want someone to acquire if they can't get an
-	**| # unacquired!
-	**| if accessedbase is containerbase:
-	**|   raise Unauthorizied, ("You are not authorized to access"
+	/*| raise Unauthorizied, ("You are not authorized to access"
 	**|	 "<em>%s</em>." % cleanupName(name, value))
-	**| return 0
 	*/
 
-
-        if (accessedbase == containerbase) 
-          unauthErr(name, value);
-        else
-          rval = PyInt_FromLong(0);        
+        unauthErr(name, value);
   err:
 
 	Py_XDECREF(stack);
 	Py_XDECREF(roles);
-	Py_XDECREF(containerbase);
-	Py_XDECREF(accessedbase);
 
 	return rval;
 }
@@ -2011,24 +1951,12 @@
       /*
         # Filter out the objects we can't access.
         if hasattr(inst, 'aq_acquire'):
-            try:
-                return inst.aq_acquire(name, aq_validate, validate)
-            except AttributeError:
-                # A denial of access was converted into an
-                # AttributeError.  Convert it back.
-                raise Unauthorized, name
+            return inst.aq_acquire(name, aq_validate, validate)
        */
       if (aq_isWrapper(inst))
         {
-          t = aq_Acquire(inst, name, aq_validate, validate, 1, NULL, 0);
-          if (t == NULL && PyErr_Occurred() == PyExc_AttributeError)
-            {
-              PyErr_Clear();
-              unauthErr(name, v);
-              goto err;
-            }
           Py_DECREF(v);
-          return t;
+          return aq_Acquire(inst, name, aq_validate, validate, 1, NULL, 0);
         }
 
       /*