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.

lib/python/AccessControl/PermissionRole.py

@@ -13,8 +13,8 @@
 __doc__='''Objects that implement Permission-based roles.
 
 
-$Id: PermissionRole.py,v 1.17 2002/10/01 14:09:46 gvanrossum Exp $'''
-__version__='$Revision: 1.17 $'[11:-2]
+$Id: PermissionRole.py,v 1.18 2003/06/10 15:39:04 shane Exp $'''
+__version__='$Revision: 1.18 $'[11:-2]
 
 _use_python_impl = 0
 import os
@@ -64,7 +64,7 @@ if _use_python_impl:
         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()

lib/python/AccessControl/ZopeGuards.py

@@ -11,7 +11,7 @@
 #
 ##############################################################################
 
-__version__='$Revision: 1.15 $'[11:-2]
+__version__='$Revision: 1.16 $'[11:-2]
 
 from RestrictedPython.Guards import safe_builtins, _full_read_guard, \
      full_write_guard
@@ -41,6 +41,11 @@ except ImportError:
 
 
     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 @@ except ImportError:
             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

lib/python/AccessControl/ZopeSecurityPolicy.py

@@ -13,8 +13,8 @@
 __doc__='''Define Zope\'s default security policy
 
 
-$Id: ZopeSecurityPolicy.py,v 1.22 2003/06/09 16:26:39 shane Exp $'''
-__version__='$Revision: 1.22 $'[11:-2]
+$Id: ZopeSecurityPolicy.py,v 1.23 2003/06/10 15:39:04 shane Exp $'''
+__version__='$Revision: 1.23 $'[11:-2]
 
 
 _use_python_impl = 0
@@ -81,19 +81,13 @@ if _use_python_impl:
                      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 @@ if _use_python_impl:
                 # 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 @@ if _use_python_impl:
                             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 _use_python_impl:
                     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 _use_python_impl:
                         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 @@ if _use_python_impl:
                     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

lib/python/AccessControl/cAccessControl.c

@@ -36,7 +36,7 @@
   USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
   DAMAGE.
 
-  $Id: cAccessControl.c,v 1.19 2003/01/14 15:03:05 shane Exp $
+  $Id: cAccessControl.c,v 1.20 2003/06/10 15:39:04 shane Exp $
 
   If you have questions regarding this software,
   contact:
@@ -733,7 +733,7 @@ static void unauthErr(PyObject *name, PyObject *value) {
 */
 
 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 @@ static PyObject *ZopeSecurityPolicy_validate(PyObject *self, PyObject *args) {
         /* 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 @@ static PyObject *ZopeSecurityPolicy_validate(PyObject *self, PyObject *args) {
 	/*| # 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 @@ static PyObject *ZopeSecurityPolicy_validate(PyObject *self, PyObject *args) {
 			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 @@ static PyObject *ZopeSecurityPolicy_validate(PyObject *self, PyObject *args) {
 		**| # 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 @@ static PyObject *ZopeSecurityPolicy_validate(PyObject *self, PyObject *args) {
 		**|        "__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 @@ static PyObject *ZopeSecurityPolicy_validate(PyObject *self, PyObject *args) {
 		}
 
 		/*| 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 @@ static PyObject *ZopeSecurityPolicy_validate(PyObject *self, PyObject *args) {
 	**|    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 @@ static PyObject *ZopeSecurityPolicy_validate(PyObject *self, PyObject *args) {
                     {
                       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 @@ static PyObject *ZopeSecurityPolicy_validate(PyObject *self, PyObject *args) {
 	**|          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 @@ static PyObject *ZopeSecurityPolicy_validate(PyObject *self, PyObject *args) {
                     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 @@ static PyObject *ZopeSecurityPolicy_validate(PyObject *self, PyObject *args) {
           }
         } /* 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 @@ guarded_getattr(PyObject *inst, PyObject *name, PyObject *default_,
       /*
         # 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);
         }
 
       /*
@@ -2150,7 +2078,7 @@ void initcAccessControl(void) {
 
 	module = Py_InitModule3("cAccessControl",
 		cAccessControl_methods,
-		"$Id: cAccessControl.c,v 1.19 2003/01/14 15:03:05 shane Exp $\n");
+		"$Id: cAccessControl.c,v 1.20 2003/06/10 15:39:04 shane Exp $\n");
 
 	aq_init(); /* For Python <= 2.1.1, aq_init() should be after
                       Py_InitModule(). */

lib/python/AccessControl/tests/testZopeSecurityPolicy.py

@@ -13,8 +13,8 @@
 """Tests of ZopeSecurityPolicy
 """
 
-__rcs_id__='$Id: testZopeSecurityPolicy.py,v 1.5 2002/08/14 21:28:08 mj Exp $'
-__version__='$Revision: 1.5 $'[11:-2]
+__rcs_id__='$Id: testZopeSecurityPolicy.py,v 1.6 2003/06/10 15:39:04 shane Exp $'
+__version__='$Revision: 1.6 $'[11:-2]
 
 import os, sys, unittest
 
@@ -139,7 +139,7 @@ class ZopeSecurityPolicyTests (unittest.TestCase):
         res = self.policy.validate(ob, ob, attrname, getattr(ob, attrname),
                                    self.context)
         if not res:
-            assert 0, 'Policy quietly denied %s' % attrname
+            self.fail('Policy quietly denied %s' % attrname)
 
     def assertPolicyDenies(self, ob, attrname):
         try:
@@ -150,10 +150,10 @@ class ZopeSecurityPolicyTests (unittest.TestCase):
             pass
         else:
             if res:
-                assert 0, 'Policy quietly allowed %s' % attrname
+                self.fail('Policy quietly allowed %s' % attrname)
             else:
-                assert 0, ('Policy denied %s, but did not '
-                           'throw an exception.' % attrname)
+                self.fail('Policy denied %s, but did not '
+                          'throw an exception.' % attrname)
 
     def testUserAccess(self):
         item = self.item
@@ -212,18 +212,23 @@ class ZopeSecurityPolicyTests (unittest.TestCase):
         r_item = self.a.r_item
         context = self.context
         v = self.policy.checkPermission('View', r_item, context)
-        assert not v, '_View_Permission should deny access to user'
+        self.assert_(not v, '_View_Permission should deny access to user')
         o_context = SecurityContext(self.uf.getUserById('theowner'))
         v = self.policy.checkPermission('View', r_item, o_context)
-        assert v, '_View_Permission should grant access to theowner'
+        self.assert_(v, '_View_Permission should grant access to theowner')
 
     def testAqNames(self):
         policy = self.policy
-        assert not policy.validate('', '', 'aq_self', '', None)
-        assert not policy.validate('', '', 'aq_base', '', None)
-        assert policy.validate('', '', 'aq_parent', '', None)
-        assert policy.validate('', '', 'aq_explicit', '', None)
-        assert policy.validate('', '', 'aq_inner', '', None)
+        names = {
+            'aq_self': 0, 'aq_base': 0,
+            'aq_parent': 1, 'aq_explicit': 1, 'aq_inner': 1
+            }
+        for name, allowed in names.items():
+            if not allowed:
+                self.assertRaises(Unauthorized, policy.validate,
+                                  '', '', name, '', None)
+            else:
+                policy.validate('', '', name, '', None)
 
     if 0:
         # This test purposely generates a log entry.
@@ -242,7 +247,7 @@ class ZopeSecurityPolicyTests (unittest.TestCase):
             except TypeError:
                 pass
             else:
-                assert 0, 'Policy accepted bad __roles__'
+                self.fail('Policy accepted bad __roles__')
 
 
 def test_suite():