Implement props_only feature for permissions.

Author: Unknown

CHANGES.txt

@@ -176,6 +176,10 @@ Features:
      ctx['permission'] the name of the permission (e.g. View, Edit)
   At some future date the older 3 argument style check command will
   be deprecated. See ``upgrading.txt`` for details.
+- New property for permissions added to simplify the model. See
+  ``customizing.txt`` and search for props_only and
+    set_props_only_default in the section 'Adding a new Permission'.
+    (John Rouillard)
 
 Fixed:
 

doc/customizing.txt

@@ -1280,7 +1280,7 @@ When adding a new Permission, you will need to:
 1. add it to your tracker's ``schema.py`` so it is created, using
    ``security.addPermission``, for example::
 
-    self.security.addPermission(name="View", klass='frozzle',
+    db.security.addPermission(name="View", klass='frozzle',
         description="User is allowed to access frozzles")
 
    will set up a new "View" permission on the Class "frozzle".
@@ -1290,12 +1290,36 @@ When adding a new Permission, you will need to:
 4. add it to the appropriate xxxPermission methods on in your tracker
    interfaces module
 
-The ``addPermission`` method takes a couple of optional parameters:
+The ``addPermission`` method takes a few optional parameters:
 
 **properties**
   A sequence of property names that are the only properties to apply the
   new Permission to (eg. ``... klass='user', properties=('name',
   'email') ...``)
+**props_only**
+  A boolean value (set to false by default) that is a new feature
+  in roundup 1.6.
+  A permission defined using
+     ``properties=('list', 'of', 'property', 'names')``
+  is used to determine access for things other than just those
+  properties. For example a check for View permission on the issue
+  class or an issue item can use any View permission for the issue
+  class even if that permission has a property list.  This can be
+  confusing and surprising as you would think that a permission
+  including properties would be used only for determining the
+  access permission for those properties.
+
+  Setting ``props_only=True`` will make the permission valid only for
+  those properties.
+
+  If you use a lot of permissions with property checks, it can be
+  difficult to change all of them. Calling the function:
+
+    db.security.set_props_only_default(True)
+
+  at the top of ``schema.py`` will make every permission creation
+  behave as though props_only was set to True. It is expected that the
+  default of True will become the default in a future Roundup release.
 **check**
   A function to be executed which returns boolean determining whether
   the Permission is allowed. If it returns True, the permission is

doc/upgrading.txt

@@ -396,6 +396,25 @@ parameter. This is expected to be the preferred form in the future.
 You do not need to use the ``ctx`` parameter in the function if you do
 not need it.
 
+Changes to property permissions
+-------------------------------
+
+If you create a permission:
+
+    db.security.addPermission(name='View', klass='user',
+       properties=['theme'], check=own_record,
+       description="User is allowed to view their own theme")
+
+that combines checks and properties, the permission also matches a
+permission check for the View permission on the user class.  So this
+also allows the user to see their user record. It is unexpected that
+checking for access without a property would match this permission.
+
+This release adds support for making a permission like above only be
+used during property permission tests. See ``customizing.txt`` and
+search for props_only and set_props_only_default in the section
+'Adding a new Permission'
+
 Improve query editing
 ---------------------
 

roundup/security.py

@@ -6,13 +6,17 @@
 
 from roundup import hyperdb, support
 
+import logging
+logger = logging.getLogger('roundup.security')
+
 class Permission:
     ''' Defines a Permission with the attributes
         - name
         - description
         - klass (optional)
         - properties (optional)
         - check function (optional)
+        - props_only (optional, internal field is limit_perm_to_props_only)
 
         The klass may be unset, indicating that this permission is not
         locked to a particular class. That means there may be multiple
@@ -24,16 +28,49 @@ class Permission:
         If check function is set, permission is granted only when
         the function returns value interpreted as boolean true.
         The function is called with arguments db, userid, itemid.
+
+        When the system checks klass permission rather than the klass
+        property permission (i.e. properties=None and item=None), it
+        will apply any permission that matches on permission name and
+        class. If the permission has a check function, the check
+        function will be run. By making the permission valid only for
+        properties using props_only=True the permission will be
+        skipped. You can set the default value for props_only for all
+        properties by calling:
+
+           db.security.set_props_only_default()
+
+        with a True or False value.
     '''
+
+    limit_perm_to_props_only=False
+
     def __init__(self, name='', description='', klass=None,
-            properties=None, check=None):
+            properties=None, check=None, props_only=None):
         import inspect
         self.name = name
         self.description = description
         self.klass = klass
         self.properties = properties
         self._properties_dict = support.TruthDict(properties)
         self.check = check
+        if properties is not None:
+            # Set to None unless properties are defined.
+            # This means that:
+            # a=Property(name="Edit", klass="issue", check=dummy,
+            #     props_only=True)
+            # b=Property(name="Edit", klass="issue", check=dummy,
+            #     props_only=False)
+            # a == b will be true.
+            if props_only is None:
+                self.limit_perm_to_props_only = \
+                                Permission.limit_perm_to_props_only
+            else:
+                # see note on use of bool() in set_props_only_default()
+                self.limit_perm_to_props_only = bool(props_only)
+        else:
+            self.limit_perm_to_props_only = None
+
 
         if check is None:
             self.check_version = 0
@@ -51,6 +88,17 @@ def __init__(self, name='', description='', klass=None,
                 self.check_version = 2
 
     def test(self, db, permission, classname, property, userid, itemid):
+        ''' Test permissions 5 args:
+            permission - string like Edit, Register etc. Required, no wildcard.
+            classname - string like issue, msg etc. Can be None to match any
+                        class.
+            property - array of strings that are property names. Optional.
+                   if None this is an item or klass access check.
+            userid - number that is id for user.
+            itemid - id for classname. e.g. 3 in issue3. If missing this is
+                 a class access check, otherwies it's a object access check.
+        '''
+
         if permission != self.name:
             return 0
 
@@ -62,13 +110,19 @@ def test(self, db, permission, classname, property, userid, itemid):
         if property is not None and not self._properties_dict[property]:
             return 0
 
+        # is this a props_only permission and permissions are set
+        if property is None and self.properties is not None and \
+                self.limit_perm_to_props_only:
+            return 0
+
         # check code
         if itemid is not None and self.check is not None:
             if self.check_version == 1:
                 if not self.check(db, userid, itemid):
                     return 0
             elif self.check_version == 2:
-                if not self.check(db, userid, itemid, property=property, permission=permission, classname=classname):
+                if not self.check(db, userid, itemid, property=property, \
+                            permission=permission, classname=classname):
                     return 0
 
         # we have a winner
@@ -97,8 +151,9 @@ def searchable(self, classname, property):
 
 
     def __repr__(self):
-        return '<Permission 0x%x %r,%r,%r,%r>'%(id(self), self.name,
-            self.klass, self.properties, self.check)
+        return '<Permission 0x%x %r,%r,%r,%r,%r>'%(id(self), self.name,
+            self.klass, self.properties, self.check,
+            self.limit_perm_to_props_only)
 
     def __cmp__(self, other):
         if self.name != other.name:
@@ -107,10 +162,16 @@ def __cmp__(self, other):
         if self.klass != other.klass: return 1
         if self.properties != other.properties: return 1
         if self.check != other.check: return 1
+        if self.limit_perm_to_props_only != \
+             other.limit_perm_to_props_only: return 1
 
         # match
         return 0
 
+    def __getitem__(self,index):
+        return (self.name, self.klass, self.properties, self.check,
+         self.limit_perm_to_props_only)[index]
+
 class Role:
     ''' Defines a Role with the attributes
         - name
@@ -158,7 +219,7 @@ def __init__(self, db):
         mailgw.initialiseSecurity(self)
 
     def getPermission(self, permission, classname=None, properties=None,
-            check=None):
+            check=None, props_only=None):
         ''' Find the Permission matching the name and for the class, if the
             classname is specified.
 
@@ -175,7 +236,8 @@ def getPermission(self, permission, classname=None, properties=None,
 
         # look through all the permissions of the given name
         tester = Permission(permission, klass=classname, properties=properties,
-            check=check)
+                            check=check,
+                            props_only=props_only)
         for perm in self.permission[permission]:
             if perm == tester:
                 return perm
@@ -186,12 +248,19 @@ def hasPermission(self, permission, userid, classname=None,
             property=None, itemid=None):
         '''Look through all the Roles, and hence Permissions, and
            see if "permission" exists given the constraints of
-           classname, property and itemid.
+           classname, property, itemid, and props_only.
 
-           If classname is specified (and only classname) then the
-           search will match if there is *any* Permission for that
-           classname, even if the Permission has additional
-           constraints.
+           If classname is specified (and only classname) the
+           search will match:
+
+              if there is *any* Permission for that classname, and
+              that Permission was not created with props_only = True
+
+           *NOTE* the Permission will match even if there are
+           additional constraints like a check or properties and
+           props_only is False. This can be unexpected. Using
+           props_only = True or setting the default value to True can
+           help prevent surprises.
 
            If property is specified, the Permission matched must have
            either no properties listed or the property must appear in
@@ -203,6 +272,7 @@ def hasPermission(self, permission, userid, classname=None,
 
            Note that this functionality is actually implemented by the
            Permission.test() method.
+
         '''
         if itemid and classname is None:
             raise ValueError, 'classname must accompany itemid'
@@ -308,8 +378,17 @@ def addRole(self, **propspec):
         self.role[role.name] = role
         return role
 
+    def set_props_only_default(self, props_only=None):
+        if props_only is not None:
+            # NOTE: only valid values are True and False because these
+            # will be compared as part of tuple == tuple and
+            # (3,) == (True,) is False even though 3 is a True value
+            # in a boolean context. So use bool() to coerce value.
+            Permission.limit_perm_to_props_only = \
+                                    bool(props_only)
+
     def addPermissionToRole(self, rolename, permission, classname=None,
-            properties=None, check=None):
+            properties=None, check=None, props_only=None):
         ''' Add the permission to the role's permission list.
 
             'rolename' is the name of the role to add the permission to.
@@ -321,7 +400,7 @@ def addPermissionToRole(self, rolename, permission, classname=None,
         '''
         if not isinstance(permission, Permission):
             permission = self.getPermission(permission, classname,
-                properties, check)
+                properties, check, props_only)
         role = self.role[rolename.lower()]
         role.permissions.append(permission)