- Sanitize and test RoleManager role handling

CHANGES.rst

@@ -6,6 +6,8 @@ For changes before version 3.0, see ``HISTORY.rst``.
 4.0a6 (unreleased)
 ------------------
 
+- Sanitize and test `RoleManager` role handling
+
 - Drop `Record` dependency, which now does its own security declaration.
 
 

src/AccessControl/rolemanager.py

@@ -386,41 +386,27 @@ def valid_roles(self):
         """Return list of valid roles.
         """
         obj = self
-        valid = {}
+        valid = set()
         x = 0
         while x < 100:
-            if hasattr(obj, '__ac_roles__'):
-                roles = obj.__ac_roles__
-                for role in roles:
-                    if role not in valid:
-                        valid[role] = 1
+            valid.update(getattr(obj, '__ac_roles__', ()))
             if getattr(obj, '__parent__', None) is None:
                 break
             obj = obj.__parent__
             x = x + 1
-        roles = sorted(valid.keys())
-        return tuple(roles)
+        return tuple(sorted(valid))
 
     def validate_roles(self, roles):
         """Return true if all given roles are valid.
         """
-        valid = self.valid_roles()
-        for role in roles:
-            if role not in valid:
-                return 0
-        return 1
+        return set(roles) <= set(self.valid_roles())
 
     security.declareProtected(change_permissions, 'userdefined_roles')
     def userdefined_roles(self):
         """Return list of user-defined roles.
         """
-        roles = list(self.__ac_roles__)
-        for role in classattr(self.__class__, '__ac_roles__'):
-            try:
-                roles.remove(role)
-            except:
-                pass
-        return tuple(roles)
+        default_roles = classattr(self.__class__, '__ac_roles__')
+        return tuple(set(self.__ac_roles__) - set(default_roles))
 
     def possible_permissions(self):
         d = {}

src/AccessControl/tests/testRole.py

@@ -93,3 +93,42 @@ def test_manage_delLocalRoles(self):
         root.context1.manage_delLocalRoles(['user1'])
         roles = root.context1.get_local_roles_for_userid('user1')
         self.assertEqual(roles, ())
+
+    def test_valid_roles(self):
+        from AccessControl.rolemanager import RoleManager
+        root, user = _makeRootAndUser()
+
+        # default case, __ac_roles__ not overridden
+        self.assertEqual(set(root.context1.valid_roles()), 
+                         set(RoleManager.__ac_roles__))
+
+        # forcing our own roles
+        root.context1.__ac_roles__ = ('Role2', 'Role1')
+        roles = root.context1.valid_roles()
+        self.assertEqual(roles, ('Role1', 'Role2'))
+
+    def test_validate_roles(self):
+        from AccessControl.rolemanager import RoleManager
+        root, user = _makeRootAndUser()
+
+        # default case, __ac_roles__ not overridden
+        self.assertTrue(root.context1.validate_roles(RoleManager.__ac_roles__))
+        self.assertFalse(root.context1.validate_roles(('Role1', 'Role2')))
+
+        # forcing our own roles
+        root.context1.__ac_roles__ = ('Role2', 'Role1')
+        self.assertFalse(root.context1.validate_roles(RoleManager.__ac_roles__))
+        self.assertTrue(root.context1.validate_roles(('Role1', 'Role2')))
+
+    def test_userdefined_roles(self):
+        from AccessControl.rolemanager import RoleManager
+        root, user = _makeRootAndUser()
+
+        # default case, __ac_roles__ not overridden
+        self.assertEqual(root.context1.userdefined_roles(), ())
+
+        # forcing our own roles
+        root.context1.__ac_roles__ = ('Role2', 'Role1')
+        self.assertEqual(set(root.context1.userdefined_roles()),
+                         set(('Role1', 'Role2')))
+