Change permission representation

Now permissions are checked in different order. Permissions without a
check method (which are cheap to check) are checked first. Only if no
permission is found do we check permissions with check methods.

Author: schlatterbeck

roundup/admin.py

@@ -1920,7 +1920,7 @@ def do_security(self, args):
         roles.sort()
         for _rolename, role in roles:
             sys.stdout.write(_('Role "%(name)s":\n') % role.__dict__)
-            for permission in role.permissions:
+            for permission in role.permission_list():
                 d = permission.__dict__
                 if permission.klass:
                     if permission.properties:

roundup/security.py

@@ -184,12 +184,73 @@ class Role:
     def __init__(self, name='', description='', permissions=None):
         self.name = name.lower()
         self.description = description
+        # This is a dict of permission names each containing a dict of
+        # *class names*, with a special entry for non-class permissions
+        # where the key is None. In each class dict we have a dictionary
+        # with the values True and False for permission with and without
+        # a check method. These dicts each contain a list of permissions.
         if permissions is None:
-            permissions = []
-        self.permissions = permissions
+            self._permissions = {}
+        elif isinstance(permissions, list):
+            self._permissions = {}
+            for p in permissions:
+                self.addPermission(p)
+        else:
+            raise ValueError("Invalid permissions for Role: %s" % permissions)
 
     def __repr__(self):
-        return '<Role 0x%x %r,%r>' % (id(self), self.name, self.permissions)
+        pl = self.permission_list()
+        return '<Role 0x%x %r,%r>' % (id(self), self.name, pl)
+
+    def addPermission (self, *permissions):
+        for p in permissions:
+            pn = p.name
+            self._permissions.setdefault(pn, {})
+            cn = p.klass
+            if p.klass not in self._permissions[pn]:
+                self._permissions[pn][cn] = dict (((False, []), (True, [])))
+            self._permissions[pn][cn][bool(p.check)].append(p)
+
+    def hasPermission (self, db, perm, uid, classname, property, itemid, chk):
+        # if itemid is given a classname must, too, checked in caller
+        assert not itemid or classname
+        perms = self._permissions
+        if perm not in perms:
+            return False
+        # If we have a classname we also need to check permission with
+        # an empty classname (e.g. 'admin' has access on everything)
+        if classname is not None and None in perms[perm]:
+            for p in perms[perm][None][chk]:
+                # permission match?
+                if p.test(db, perm, classname, property, uid, itemid):
+                    return True
+        if classname not in perms[perm]:
+            return False
+        for p in perms[perm][classname][chk]:
+            # permission match?
+            if p.test(db, perm, classname, property, uid, itemid):
+                return True
+
+    def permission_list (self):
+        """ Used for reporting in admin tool """
+        l = []
+        for p in self._permissions:
+            for c in self._permissions[p]:
+                for cond in (False, True):
+                    l.extend (self._permissions[p][c][cond])
+        l.sort (key = lambda x: (x.klass or '', x.name))
+        return l
+
+    def searchable (self, classname, propname):
+        for perm in 'View', 'Search':
+            # Only permissions without a check method
+            if perm not in self._permissions:
+                continue
+            if classname not in self._permissions[perm]:
+                continue
+            for p in self._permissions[perm][classname][False]:
+                if p.searchable(classname, propname):
+                    return True
 
 
 class Security:
@@ -199,7 +260,7 @@ def __init__(self, db):
         '''
         self.db = weakref.proxy(db)       # use a weak ref to avoid circularity
 
-        # permssions are mapped by name to a list of Permissions by class
+        # Permissions are mapped by name to a list of Permissions by class
         self.permission = {}
 
         # roles are mapped by name to the Role
@@ -278,28 +339,30 @@ def hasPermission(self, permission, userid, classname=None,
            Permission.test() method.
 
         '''
+        if classname == 'status':
+            import pdb; pdb.set_trace()
         if itemid and classname is None:
             raise ValueError('classname must accompany itemid')
-        for rolename in self.db.user.get_roles(userid):
-            if not rolename or (rolename not in self.role):
-                continue
-            # for each of the user's Roles, check the permissions
-            for perm in self.role[rolename].permissions:
-                # permission match?
-                if perm.test(self.db, permission, classname, property,
-                             userid, itemid):
-                    return 1
-        return 0
+        # for each of the user's Roles, check the permissions
+        # Note that checks with a check method are typically a lot more
+        # expensive than the ones without. So we check the ones without
+        # a check method first
+        for has_check in False, True:
+            for rolename in self.db.user.get_roles(userid):
+                if not rolename or (rolename not in self.role):
+                    continue
+                r = self.role[rolename]
+                v = r.hasPermission(self.db, permission, userid, classname,
+                                    property, itemid, has_check)
+                if v:
+                    return v
+        return False
 
     def roleHasSearchPermission(self, classname, property, *rolenames):
         """ For each of the given roles, check the permissions.
             Property can be a transitive property.
         """
         perms = []
-        # pre-compute permissions
-        for rn in rolenames:
-            for perm in self.role[rn].permissions:
-                perms.append(perm)
         # Note: break from inner loop means "found"
         #       break from outer loop means "not found"
         cn = classname
@@ -319,8 +382,8 @@ def roleHasSearchPermission(self, classname, property, *rolenames):
                 prop = cls.getprops()[propname]
             except KeyError:
                 break
-            for perm in perms:
-                if perm.searchable(cn, propname):
+            for rn in rolenames:
+                if self.role[rn].searchable(cn, propname):
                     break
             else:
                 break
@@ -334,8 +397,8 @@ def roleHasSearchPermission(self, classname, property, *rolenames):
                     return 0
                 props = dict.fromkeys(('id', cls.labelprop(), cls.orderprop()))
                 for p in props.keys():
-                    for perm in perms:
-                        if perm.searchable(prop.classname, p):
+                    for rn in rolenames:
+                        if self.role[rn].searchable(prop.classname, p):
                             break
                     else:
                         return 0
@@ -409,7 +472,7 @@ def addPermissionToRole(self, rolename, permission, classname=None,
             permission = self.getPermission(permission, classname,
                                             properties, check, props_only)
         role = self.role[rolename.lower()]
-        role.permissions.append(permission)
+        role.addPermission(permission)
 
     # Convenience methods for removing non-allowed properties from a
     # filterspec or sort/group list

test/db_test_base.py

@@ -3324,14 +3324,14 @@ def stdoutwrite(s):
                           'Role "admin":\n',
                           ' User may create everything (Create)\n',
                           ' User may edit everything (Edit)\n',
+                          ' User may use the email interface (Email Access)\n',
+                          ' User may access the rest interface (Rest Access)\n',
                           ' User may restore everything (Restore)\n',
                           ' User may retire everything (Retire)\n',
                           ' User may view everything (View)\n',
                           ' User may access the web interface (Web Access)\n',
-                          ' User may access the rest interface (Rest Access)\n',
-                          ' User may access the xmlrpc interface (Xmlrpc Access)\n',
                           ' User may manipulate user Roles through the web (Web Roles)\n',
-                          ' User may use the email interface (Email Access)\n',
+                          ' User may access the xmlrpc interface (Xmlrpc Access)\n',
                           'Role "anonymous":\n', 'Role "user":\n',
                           ' User is allowed to access msg (View for "msg" only)\n',
                           ' Prevent users from seeing roles (View for "user": [\'username\', \'supervisor\', \'assignable\'] only)\n']

test/test_admin.py

@@ -1420,33 +1420,33 @@ def testSecurityListOne(self):
             ret = self.admin.main()
 
             result = """Role "user":
- User may access the web interface (Web Access)
  User may use the email interface (Email Access)
  User may access the rest interface (Rest Access)
+ User may access the web interface (Web Access)
  User may access the xmlrpc interface (Xmlrpc Access)
- User is allowed to access issue (View for "issue" only)
- User is allowed to edit issue (Edit for "issue" only)
- User is allowed to create issue (Create for "issue" only)
- User is allowed to access file (View for "file" only)
- User is allowed to edit file (Edit for "file" only)
  User is allowed to create file (Create for "file" only)
- User is allowed to access msg (View for "msg" only)
- User is allowed to edit msg (Edit for "msg" only)
- User is allowed to create msg (Create for "msg" only)
- User is allowed to access keyword (View for "keyword" only)
- User is allowed to edit keyword (Edit for "keyword" only)
+ User is allowed to edit file (Edit for "file" only)
+ User is allowed to access file (View for "file" only)
+ User is allowed to create issue (Create for "issue" only)
+ User is allowed to edit issue (Edit for "issue" only)
+ User is allowed to access issue (View for "issue" only)
  User is allowed to create keyword (Create for "keyword" only)
+ User is allowed to edit keyword (Edit for "keyword" only)
+ User is allowed to access keyword (View for "keyword" only)
+ User is allowed to create msg (Create for "msg" only)
+ User is allowed to edit msg (Edit for "msg" only)
+ User is allowed to access msg (View for "msg" only)
  User is allowed to access priority (View for "priority" only)
+ User is allowed to create queries (Create for "query" only)
+ User is allowed to edit their queries (Edit for "query" only)
+ User is allowed to restore their queries (Restore for "query" only)
+ User is allowed to retire their queries (Retire for "query" only)
+  (Search for "query" only)
+ User is allowed to view their own and public queries (View for "query" only)
  User is allowed to access status (View for "status" only)
+ User is allowed to edit their own user details (Edit for "user": ('username', 'password', 'address', 'realname', 'phone', 'organisation', 'alternate_addresses', 'queries', 'timezone') only)
   (View for "user": ('id', 'organisation', 'phone', 'realname', 'timezone', 'username') only)
  User is allowed to view their own user details (View for "user" only)
- User is allowed to edit their own user details (Edit for "user": ('username', 'password', 'address', 'realname', 'phone', 'organisation', 'alternate_addresses', 'queries', 'timezone') only)
- User is allowed to view their own and public queries (View for "query" only)
-  (Search for "query" only)
- User is allowed to edit their queries (Edit for "query" only)
- User is allowed to retire their queries (Retire for "query" only)
- User is allowed to restore their queries (Restore for "query" only)
- User is allowed to create queries (Create for "query" only)
 """
         print(out.getvalue())
 
@@ -1496,52 +1496,52 @@ def testSecurityListAll(self):
 Role "admin":
  User may create everything (Create)
  User may edit everything (Edit)
+ User may use the email interface (Email Access)
+ User may access the rest interface (Rest Access)
  User may restore everything (Restore)
  User may retire everything (Retire)
  User may view everything (View)
  User may access the web interface (Web Access)
- User may access the rest interface (Rest Access)
- User may access the xmlrpc interface (Xmlrpc Access)
  User may manipulate user Roles through the web (Web Roles)
- User may use the email interface (Email Access)
+ User may access the xmlrpc interface (Xmlrpc Access)
 Role "anonymous":
  User may access the web interface (Web Access)
- User is allowed to register new user (Register for "user" only)
- User is allowed to access issue (View for "issue" only)
  User is allowed to access file (View for "file" only)
- User is allowed to access msg (View for "msg" only)
+ User is allowed to access issue (View for "issue" only)
  User is allowed to access keyword (View for "keyword" only)
+ User is allowed to access msg (View for "msg" only)
  User is allowed to access priority (View for "priority" only)
  User is allowed to access status (View for "status" only)
+ User is allowed to register new user (Register for "user" only)
   (Search for "user" only)
 Role "user":
- User may access the web interface (Web Access)
  User may use the email interface (Email Access)
  User may access the rest interface (Rest Access)
+ User may access the web interface (Web Access)
  User may access the xmlrpc interface (Xmlrpc Access)
- User is allowed to access issue (View for "issue" only)
- User is allowed to edit issue (Edit for "issue" only)
- User is allowed to create issue (Create for "issue" only)
- User is allowed to access file (View for "file" only)
- User is allowed to edit file (Edit for "file" only)
  User is allowed to create file (Create for "file" only)
- User is allowed to access msg (View for "msg" only)
- User is allowed to edit msg (Edit for "msg" only)
- User is allowed to create msg (Create for "msg" only)
- User is allowed to access keyword (View for "keyword" only)
- User is allowed to edit keyword (Edit for "keyword" only)
+ User is allowed to edit file (Edit for "file" only)
+ User is allowed to access file (View for "file" only)
+ User is allowed to create issue (Create for "issue" only)
+ User is allowed to edit issue (Edit for "issue" only)
+ User is allowed to access issue (View for "issue" only)
  User is allowed to create keyword (Create for "keyword" only)
+ User is allowed to edit keyword (Edit for "keyword" only)
+ User is allowed to access keyword (View for "keyword" only)
+ User is allowed to create msg (Create for "msg" only)
+ User is allowed to edit msg (Edit for "msg" only)
+ User is allowed to access msg (View for "msg" only)
  User is allowed to access priority (View for "priority" only)
+ User is allowed to create queries (Create for "query" only)
+ User is allowed to edit their queries (Edit for "query" only)
+ User is allowed to restore their queries (Restore for "query" only)
+ User is allowed to retire their queries (Retire for "query" only)
+  (Search for "query" only)
+ User is allowed to view their own and public queries (View for "query" only)
  User is allowed to access status (View for "status" only)
+ User is allowed to edit their own user details (Edit for "user": ('username', 'password', 'address', 'realname', 'phone', 'organisation', 'alternate_addresses', 'queries', 'timezone') only)
   (View for "user": ('id', 'organisation', 'phone', 'realname', 'timezone', 'username') only)
  User is allowed to view their own user details (View for "user" only)
- User is allowed to edit their own user details (Edit for "user": ('username', 'password', 'address', 'realname', 'phone', 'organisation', 'alternate_addresses', 'queries', 'timezone') only)
- User is allowed to view their own and public queries (View for "query" only)
-  (Search for "query" only)
- User is allowed to edit their queries (Edit for "query" only)
- User is allowed to retire their queries (Retire for "query" only)
- User is allowed to restore their queries (Restore for "query" only)
- User is allowed to create queries (Create for "query" only)
 """
         print(out.getvalue())
 
@@ -1579,43 +1579,50 @@ def testSecurityInvalidAttribute(self):
 Role "admin":
  User may create everything (Create)
  User may edit everything (Edit)
+ User may use the email interface (Email Access)
+ User may access the rest interface (Rest Access)
  User may restore everything (Restore)
  User may retire everything (Retire)
  User may view everything (View)
  User may access the web interface (Web Access)
- User may access the rest interface (Rest Access)
- User may access the xmlrpc interface (Xmlrpc Access)
  User may manipulate user Roles through the web (Web Roles)
- User may use the email interface (Email Access)
+ User may access the xmlrpc interface (Xmlrpc Access)
 Role "anonymous":
  User may access the web interface (Web Access)
- User is allowed to register new user (Register for "user" only)
- User is allowed to access issue (View for "issue" only)
  User is allowed to access file (View for "file" only)
- User is allowed to access msg (View for "msg" only)
+ User is allowed to access issue (View for "issue" only)
  User is allowed to access keyword (View for "keyword" only)
+ User is allowed to access msg (View for "msg" only)
  User is allowed to access priority (View for "priority" only)
  User is allowed to access status (View for "status" only)
+ User is allowed to register new user (Register for "user" only)
   (Search for "user" only)
 Role "user":
- User may access the web interface (Web Access)
  User may use the email interface (Email Access)
  User may access the rest interface (Rest Access)
+ User may access the web interface (Web Access)
  User may access the xmlrpc interface (Xmlrpc Access)
- User is allowed to access issue (View for "issue" only)
- User is allowed to edit issue (Edit for "issue" only)
- User is allowed to create issue (Create for "issue" only)
- User is allowed to access file (View for "file" only)
- User is allowed to edit file (Edit for "file" only)
  User is allowed to create file (Create for "file" only)
- User is allowed to access msg (View for "msg" only)
- User is allowed to edit msg (Edit for "msg" only)
- User is allowed to create msg (Create for "msg" only)
- User is allowed to access keyword (View for "keyword" only)
- User is allowed to edit keyword (Edit for "keyword" only)
+ User is allowed to edit file (Edit for "file" only)
+ User is allowed to access file (View for "file" only)
+ User is allowed to create issue (Create for "issue" only)
+ User is allowed to edit issue (Edit for "issue" only)
+ User is allowed to access issue (View for "issue" only)
  User is allowed to create keyword (Create for "keyword" only)
+ User is allowed to edit keyword (Edit for "keyword" only)
+ User is allowed to access keyword (View for "keyword" only)
+ User is allowed to create msg (Create for "msg" only)
+ User is allowed to edit msg (Edit for "msg" only)
+ User is allowed to access msg (View for "msg" only)
  User is allowed to access priority (View for "priority" only)
+ User is allowed to create queries (Create for "query" only)
+ User is allowed to edit their queries (Edit for "query" only)
+ User is allowed to restore their queries (Restore for "query" only)
+ User is allowed to retire their queries (Retire for "query" only)
+  (Search for "query" only)
+ User is allowed to view their own and public queries (View for "query" only)
  User is allowed to access status (View for "status" only)
+ User is allowed to edit their own user details (Edit for "user": ('username', 'password', 'address', 'realname', 'phone', 'organisation', 'alternate_addresses', 'queries', 'timezone') only)
   (View for "user": ('id', 'organization', 'phone', 'realname', 'timezone', 'username') only)
 
   **Invalid properties for user: ['organization']