Merge patch 16002

nova/auth/fakeldap.py

@@ -18,128 +18,207 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 """
-  Fake LDAP server for test harnesses.
+Fake LDAP server for test harnesses.
+
+This class does very little error checking, and knows nothing about ldap
+class definitions. It implements the minimum emulation of the python ldap
+library to work with nova.
 """
 
-import logging
+import json
 
 from nova import datastore
 
-SCOPE_SUBTREE  = 1
+
+SCOPE_SUBTREE  = 2
 MOD_ADD = 0
 MOD_DELETE = 1
 
-SUBS = {
-    'groupOfNames': ['novaProject']
-}
-
 
 class NO_SUCH_OBJECT(Exception):
     pass
 
 
+class OBJECT_CLASS_VIOLATION(Exception):
+    pass
+
+
 def initialize(uri):
-    return FakeLDAP(uri)
+    return FakeLDAP()
+
+
+def _match_query(query, attrs):
+    """Match an ldap query to an attribute dictionary.
+
+    &, |, and ! are supported in the query. No syntax checking is performed,
+    so malformed querys will not work correctly.
+
+    """
+    # cut off the parentheses
+    inner = query[1:-1]
+    if inner.startswith('&'):
+        # cut off the &
+        l, r = _paren_groups(inner[1:])
+        return _match_query(l, attrs) and _match_query(r, attrs)
+    if inner.startswith('|'):
+        # cut off the |
+        l, r = _paren_groups(inner[1:])
+        return _match_query(l, attrs) or _match_query(r, attrs)
+    if inner.startswith('!'):
+        # cut off the ! and the nested parentheses
+        return not _match_query(query[2:-1], attrs)
+
+    (k, sep, v) = inner.partition('=')
+    return _match(k, v, attrs)
+
+
+def _paren_groups(source):
+    """Split a string into parenthesized groups."""
+    count = 0
+    start = 0
+    result = []
+    for pos in xrange(len(source)):
+        if source[pos] == '(':
+            if count == 0:
+                start = pos
+            count += 1
+        if source[pos] == ')':
+            count -= 1
+            if count == 0:
+                result.append(source[start:pos+1])
+    return result
+
+
+def _match(k, v, attrs):
+    """Match a given key and value against an attribute list."""
+    if k not in attrs:
+        return False
+    if k != "objectclass":
+        return v in attrs[k]
+    # it is an objectclass check, so check subclasses
+    values = _subs(v)
+    for value in values:
+        if value in attrs[k]:
+            return True
+    return False
+
+
+def _subs(value):
+    """Returns a list of subclass strings.
+
+    The strings represent the ldap objectclass plus any subclasses that
+    inherit from it. Fakeldap doesn't know about the ldap object structure,
+    so subclasses need to be defined manually in the dictionary below.
+
+    """
+    subs = {'groupOfNames': ['novaProject']}
+    if value in subs:
+        return [value] + subs[value]
+    return [value]
+
+
+def _from_json(encoded):
+    """Convert attribute values from json representation.
+
+    Args:
+    encoded -- a json encoded string
+
+    Returns a list of strings
+
+    """
+    return [str(x) for x in json.loads(encoded)]
+
+
+def _to_json(unencoded):
+    """Convert attribute values into json representation.
+
+    Args:
+    unencoded -- an unencoded string or list of strings.  If it
+        is a single string, it will be converted into a list.
+
+    Returns a json string
+
+    """
+    return json.dumps(list(unencoded))
 
 
 class FakeLDAP(object):
-    def __init__(self, _uri):
+    #TODO(vish): refactor this class to use a wrapper instead of accessing
-        self.keeper = datastore.Keeper('fakeldap')
+    #            redis directly
-        if self.keeper['objects'] is None:
-            self.keeper['objects'] = {}
 
     def simple_bind_s(self, dn, password):
+        """This method is ignored, but provided for compatibility."""
         pass
 
     def unbind_s(self):
+        """This method is ignored, but provided for compatibility."""
         pass
 
-    def _paren_groups(self, source):
+    def add_s(self, dn, attr):
-        count = 0
+        """Add an object with the specified attributes at dn."""
-        start = 0
+        key = "%s%s" % (self.__redis_prefix, dn)
-        result = []
-        for pos in xrange(len(source)):
-            if source[pos] == '(':
-                if count == 0:
-                    start = pos
-                count += 1
-            if source[pos] == ')':
-                count -= 1
-                if count == 0:
-                    result.append(source[start:pos+1])
 
-    def _match_query(self, query, attrs):
+        value_dict = dict([(k, _to_json(v)) for k, v in attr])
-        inner = query[1:-1]
+        datastore.Redis.instance().hmset(key, value_dict)
-        if inner.startswith('&'):
-            l, r = self._paren_groups(inner[1:])
-            return self._match_query(l, attrs) and self._match_query(r, attrs)
-        if inner.startswith('|'):
-            l, r = self._paren_groups(inner[1:])
-            return self._match_query(l, attrs) or self._match_query(r, attrs)
-        if inner.startswith('!'):
-            return not self._match_query(query[2:-1], attrs)
 
-        (k, sep, v) = inner.partition('=')
+    def delete_s(self, dn):
-        return self._match(k, v, attrs)
+        """Remove the ldap object at specified dn."""
+        datastore.Redis.instance().delete("%s%s" % (self.__redis_prefix, dn))
 
-    def _subs(self, v):
+    def modify_s(self, dn, attrs):
-        if v in SUBS:
+        """Modify the object at dn using the attribute list.
-            return [v] + SUBS[v]
-        return [v]
 
-    def _match(self, k, v, attrs):
+        Args:
-        if attrs.has_key(k):
+        dn -- a dn
-            for v in self._subs(v):
+        attrs -- a list of tuples in the following form:
-                if (v in attrs[k]):
+            ([MOD_ADD | MOD_DELETE], attribute, value)
-                    return True
+
-        return False
+        """
+        redis = datastore.Redis.instance()
+        key = "%s%s" % (self.__redis_prefix, dn)
+
+        for cmd, k, v in attrs:
+            values = _from_json(redis.hget(key, k))
+            if cmd == MOD_ADD:
+                values.append(v)
+            else:
+                values.remove(v)
+            values = redis.hset(key, k, _to_json(values))
 
     def search_s(self, dn, scope, query=None, fields=None):
-        #logging.debug("searching for %s" % dn)
+        """Search for all matching objects under dn using the query.
-        filtered = {}
+
-        d = self.keeper['objects'] or {}
+        Args:
-        for cn, attrs in d.iteritems():
+        dn -- dn to search under
-            if cn[-len(dn):] == dn:
+        scope -- only SCOPE_SUBTREE is supported
-                filtered[cn] = attrs
+        query -- query to filter objects by
-        objects = filtered
+        fields -- fields to return. Returns all fields if not specified
-        if query:
+
-            objects = {}
+        """
-            for cn, attrs in filtered.iteritems():
+        if scope != SCOPE_SUBTREE:
-                if self._match_query(query, attrs):
+            raise NotImplementedError(str(scope))
-                    objects[cn] = attrs
+        redis = datastore.Redis.instance()
-        if objects == {}:
+        keys = redis.keys("%s*%s" % (self.__redis_prefix, dn))
+        objects = []
+        for key in keys:
+            # get the attributes from redis
+            attrs = redis.hgetall(key)
+            # turn the values from redis into lists
+            attrs = dict([(k, _from_json(v))
+                          for k, v in attrs.iteritems()])
+            # filter the objects by query
+            if not query or _match_query(query, attrs):
+                # filter the attributes by fields
+                attrs = dict([(k, v) for k, v in attrs.iteritems()
+                              if not fields or k in fields])
+                objects.append((key[len(self.__redis_prefix):], attrs))
+        if objects == []:
             raise NO_SUCH_OBJECT()
-        return objects.items()
+        return objects
-
-    def add_s(self, cn, attr):
-        #logging.debug("adding %s" % cn)
-        stored = {}
-        for k, v in attr:
-            if type(v) is list:
-                stored[k] = v
-            else:
-                stored[k] = [v]
-        d = self.keeper['objects']
-        d[cn] = stored
-        self.keeper['objects'] = d
-
-    def delete_s(self, cn):
-        logging.debug("deleting %s" % cn)
-        d = self.keeper['objects']
-        del d[cn]
-        self.keeper['objects'] = d
-
-    def modify_s(self, cn, attr):
-        logging.debug("modifying %s" % cn)
-        d = self.keeper['objects']
-        for cmd, k, v in attr:
-            logging.debug("command %s" % cmd)
-            if cmd == MOD_ADD:
-                d[cn][k].append(v)
-            else:
-                d[cn][k].remove(v)
-        self.keeper['objects'] = d
 
 
+    @property
+    def __redis_prefix(self):
+        return 'ldap:'
 
 

nova/auth/users.py

@@ -52,15 +52,21 @@ from nova import objectstore # for flags
 
 FLAGS = flags.FLAGS
 
-flags.DEFINE_string('ldap_url', 'ldap://localhost', 'Point this at your ldap server')
+flags.DEFINE_string('ldap_url', 'ldap://localhost',
+                    'Point this at your ldap server')
 flags.DEFINE_string('ldap_password',  'changeme', 'LDAP password')
-flags.DEFINE_string('user_dn', 'cn=Manager,dc=example,dc=com', 'DN of admin user')
+flags.DEFINE_string('user_dn', 'cn=Manager,dc=example,dc=com',
+                    'DN of admin user')
 flags.DEFINE_string('user_unit', 'Users', 'OID for Users')
-flags.DEFINE_string('user_ldap_subtree', 'ou=Users,dc=example,dc=com', 'OU for Users')
+flags.DEFINE_string('user_ldap_subtree', 'ou=Users,dc=example,dc=com',
-flags.DEFINE_string('project_ldap_subtree', 'ou=Groups,dc=example,dc=com', 'OU for Projects')
+                    'OU for Users')
-flags.DEFINE_string('role_ldap_subtree', 'ou=Groups,dc=example,dc=com', 'OU for Roles')
+flags.DEFINE_string('project_ldap_subtree', 'ou=Groups,dc=example,dc=com',
+                    'OU for Projects')
+flags.DEFINE_string('role_ldap_subtree', 'ou=Groups,dc=example,dc=com',
+                    'OU for Roles')
 
-# mapping with these flags is necessary because we're going to tie in to an existing ldap schema
+# NOTE(vish): mapping with these flags is necessary because we're going
+#             to tie in to an existing ldap schema
 flags.DEFINE_string('ldap_cloudadmin',
     'cn=cloudadmins,ou=Groups,dc=example,dc=com', 'cn for Cloud Admins')
 flags.DEFINE_string('ldap_itsec',
@@ -72,11 +78,15 @@ flags.DEFINE_string('ldap_netadmin',
 flags.DEFINE_string('ldap_developer',
     'cn=developers,ou=Groups,dc=example,dc=com', 'cn for Developers')
 
-# a user with one of these roles will be a superuser and have access to all api commands
+# NOTE(vish): a user with one of these roles will be a superuser and
-flags.DEFINE_list('superuser_roles', ['cloudadmin'], 'roles that ignore rbac checking completely')
+#             have access to all api commands
+flags.DEFINE_list('superuser_roles', ['cloudadmin'],
+                  'roles that ignore rbac checking completely')
 
-# a user with one of these roles will have it for every project, even if he or she is not a member of the project
+# NOTE(vish): a user with one of these roles will have it for every
-flags.DEFINE_list('global_roles', ['cloudadmin', 'itsec'], 'roles that apply to all projects')
+#             project, even if he or she is not a member of the project
+flags.DEFINE_list('global_roles', ['cloudadmin', 'itsec'],
+                  'roles that apply to all projects')
 
 flags.DEFINE_string('credentials_template',
                     utils.abspath('auth/novarc.template'),
@@ -90,15 +100,20 @@ flags.DEFINE_string('credential_cert_file', 'cert.pem',
                     'Filename of certificate in credentials zip')
 flags.DEFINE_string('credential_rc_file', 'novarc',
                     'Filename of rc in credentials zip')
-flags.DEFINE_string('vpn_ip', '127.0.0.1', 'Public IP for the cloudpipe VPN servers')
+flags.DEFINE_string('vpn_ip', '127.0.0.1',
+                    'Public IP for the cloudpipe VPN servers')
 
 
 class AuthBase(object):
     @classmethod
     def safe_id(cls, obj):
-        """this method will return the id of the object if the object is of this class, otherwise
+        """Safe get object id.
-        it will return the original object.  This allows methods to accept objects or
+
-        ids as paramaters"""
+        This method will return the id of the object if the object
+        is of this class, otherwise it will return the original object.
+        This allows methods to accept objects or ids as paramaters.
+
+        """
         if isinstance(obj, cls):
             return obj.id
         else:
@@ -195,7 +210,8 @@ class User(AuthBase):
         return UserManager.instance().get_key_pairs(self.id)
 
     def __repr__(self):
-        return "User('%s', '%s', '%s', '%s', %s)" % (self.id, self.name, self.access, self.secret, self.admin)
+        return "User('%s', '%s', '%s', '%s', %s)" % (
+                self.id, self.name, self.access, self.secret, self.admin)
 
 class KeyPair(AuthBase):
     def __init__(self, id, owner_id, public_key, fingerprint):
@@ -209,7 +225,8 @@ class KeyPair(AuthBase):
         return UserManager.instance().delete_key_pair(self.owner, self.name)
 
     def __repr__(self):
-        return "KeyPair('%s', '%s', '%s', '%s')" % (self.id, self.owner_id, self.public_key, self.fingerprint)
+        return "KeyPair('%s', '%s', '%s', '%s')" % (
+                self.id, self.owner_id, self.public_key, self.fingerprint)
 
 class Group(AuthBase):
     """id and name are currently the same"""
@@ -223,7 +240,8 @@ class Group(AuthBase):
         return User.safe_id(user) in self.member_ids
 
     def __repr__(self):
-        return "Group('%s', '%s', %s)" % (self.id, self.description, self.member_ids)
+        return "Group('%s', '%s', %s)" % (
+                self.id, self.description, self.member_ids)
 
 class Project(Group):
     def __init__(self, id, project_manager_id, description, member_ids):
@@ -298,7 +316,9 @@ class Project(Group):
         return UserManager.instance().generate_x509_cert(user, self)
 
     def __repr__(self):
-        return "Project('%s', '%s', '%s', %s)" % (self.id, self.project_manager_id, self.description, self.member_ids)
+        return "Project('%s', '%s', '%s', %s)" % (
+                self.id, self.project_manager_id,
+                self.description, self.member_ids)
 
 class UserManager(object):
     def __init__(self):
@@ -322,7 +342,9 @@ class UserManager(object):
                 except: pass
         return cls._instance
 
-    def authenticate(self, access, signature, params, verb='GET', server_string='127.0.0.1:8773', path='/', verify_signature=True):
+    def authenticate(self, access, signature, params, verb='GET',
+                     server_string='127.0.0.1:8773', path='/',
+                     verify_signature=True):
         # TODO: Check for valid timestamp
         (access_key, sep, project_name) = access.partition(':')
 
@@ -334,12 +356,16 @@ class UserManager(object):
 
         project = self.get_project(project_name)
         if project == None:
-            raise exception.NotFound('No project called %s could be found' % project_name)
+            raise exception.NotFound('No project called %s could be found' %
+                                     project_name)
         if not user.is_admin() and not project.has_member(user):
-            raise exception.NotFound('User %s is not a member of project %s' % (user.id, project.id))
+            raise exception.NotFound('User %s is not a member of project %s' %
+                                     (user.id, project.id))
         if verify_signature:
-            # hmac can't handle unicode, so encode ensures that secret isn't unicode
+            # NOTE(vish): hmac can't handle unicode, so encode ensures that
-            expected_signature = signer.Signer(user.secret.encode()).generate(params, verb, server_string, path)
+            #             secret isn't unicode
+            expected_signature = signer.Signer(user.secret.encode()).generate(
+                    params, verb, server_string, path)
             logging.debug('user.secret: %s', user.secret)
             logging.debug('expected_signature: %s', expected_signature)
             logging.debug('signature: %s', signature)
@@ -369,17 +395,21 @@ class UserManager(object):
 
     def add_role(self, user, role, project=None):
         with LDAPWrapper() as conn:
-            return conn.add_role(User.safe_id(user), role, Project.safe_id(project))
+            return conn.add_role(User.safe_id(user), role,
+                                 Project.safe_id(project))
 
     def remove_role(self, user, role, project=None):
         with LDAPWrapper() as conn:
-            return conn.remove_role(User.safe_id(user), role, Project.safe_id(project))
+            return conn.remove_role(User.safe_id(user), role,
+                                    Project.safe_id(project))
 
-    def create_project(self, name, manager_user, description=None, member_users=None):
+    def create_project(self, name, manager_user,
+                       description=None, member_users=None):
         if member_users:
             member_users = [User.safe_id(u) for u in member_users]
         with LDAPWrapper() as conn:
-            return conn.create_project(name, User.safe_id(manager_user), description, member_users)
+            return conn.create_project(name, User.safe_id(manager_user),
+                                       description, member_users)
 
     def get_projects(self):
         with LDAPWrapper() as conn:
@@ -392,7 +422,8 @@ class UserManager(object):
 
     def add_to_project(self, user, project):
         with LDAPWrapper() as conn:
-            return conn.add_to_project(User.safe_id(user), Project.safe_id(project))
+            return conn.add_to_project(User.safe_id(user),
+                                       Project.safe_id(project))
 
     def is_project_manager(self, user, project):
         if not isinstance(project, Project):
@@ -408,7 +439,8 @@ class UserManager(object):
 
     def remove_from_project(self, user, project):
         with LDAPWrapper() as conn:
-            return conn.remove_from_project(User.safe_id(user), Project.safe_id(project))
+            return conn.remove_from_project(User.safe_id(user),
+                                            Project.safe_id(project))
 
     def delete_project(self, project):
         with LDAPWrapper() as conn:
@@ -426,7 +458,8 @@ class UserManager(object):
         with LDAPWrapper() as conn:
             return conn.find_users()
 
-    def create_user(self, user, access=None, secret=None, admin=False, create_project=True):
+    def create_user(self, user, access=None, secret=None,
+                    admin=False, create_project=True):
         if access == None: access = str(uuid.uuid4())
         if secret == None: secret = str(uuid.uuid4())
         with LDAPWrapper() as conn:
@@ -503,9 +536,12 @@ class LDAPWrapper(object):
     def connect(self):
         """ connect to ldap as admin user """
         if FLAGS.fake_users:
+            self.NO_SUCH_OBJECT = fakeldap.NO_SUCH_OBJECT
+            self.OBJECT_CLASS_VIOLATION = fakeldap.OBJECT_CLASS_VIOLATION
             self.conn = fakeldap.initialize(FLAGS.ldap_url)
         else:
-            assert(ldap.__name__ != 'fakeldap')
+            self.NO_SUCH_OBJECT = ldap.NO_SUCH_OBJECT
+            self.OBJECT_CLASS_VIOLATION = ldap.OBJECT_CLASS_VIOLATION
             self.conn = ldap.initialize(FLAGS.ldap_url)
         self.conn.simple_bind_s(self.user, self.passwd)
 
@@ -518,7 +554,7 @@ class LDAPWrapper(object):
     def find_dns(self, dn, query=None):
         try:
             res = self.conn.search_s(dn, ldap.SCOPE_SUBTREE, query)
-        except Exception:
+        except self.NO_SUCH_OBJECT:
             return []
         # just return the DNs
         return [dn for dn, attributes in res]
@@ -526,25 +562,29 @@ class LDAPWrapper(object):
     def find_objects(self, dn, query = None):
         try:
             res = self.conn.search_s(dn, ldap.SCOPE_SUBTREE, query)
-        except Exception:
+        except self.NO_SUCH_OBJECT:
             return []
         # just return the attributes
         return [attributes for dn, attributes in res]
 
     def find_users(self):
-        attrs = self.find_objects(FLAGS.user_ldap_subtree, '(objectclass=novaUser)')
+        attrs = self.find_objects(FLAGS.user_ldap_subtree,
+                                  '(objectclass=novaUser)')
         return [self.__to_user(attr) for attr in attrs]
 
     def find_key_pairs(self, uid):
-        attrs = self.find_objects(self.__uid_to_dn(uid), '(objectclass=novaKeyPair)')
+        attrs = self.find_objects(self.__uid_to_dn(uid),
+                                  '(objectclass=novaKeyPair)')
         return [self.__to_key_pair(uid, attr) for attr in attrs]
 
     def find_projects(self):
-        attrs = self.find_objects(FLAGS.project_ldap_subtree, '(objectclass=novaProject)')
+        attrs = self.find_objects(FLAGS.project_ldap_subtree,
+                                  '(objectclass=novaProject)')
         return [self.__to_project(attr) for attr in attrs]
 
     def find_roles(self, tree):
-        attrs = self.find_objects(tree, '(&(objectclass=groupOfNames)(!(objectclass=NovaProject)))')
+        attrs = self.find_objects(tree,
+                '(&(objectclass=groupOfNames)(!(objectclass=novaProject)))')
         return [self.__to_group(attr) for attr in attrs]
 
     def find_group_dns_with_member(self, tree, uid):
@@ -554,7 +594,8 @@ class LDAPWrapper(object):
         return dns
 
     def find_user(self, uid):
-        attr = self.find_object(self.__uid_to_dn(uid), '(objectclass=novaUser)')
+        attr = self.find_object(self.__uid_to_dn(uid),
+                                '(objectclass=novaUser)')
         return self.__to_user(attr)
 
     def find_key_pair(self, uid, key_name):
@@ -611,11 +652,14 @@ class LDAPWrapper(object):
         self.conn.add_s(self.__uid_to_dn(name), attr)
         return self.__to_user(dict(attr))
 
-    def create_project(self, name, manager_uid, description=None, member_uids=None):
+    def create_project(self, name, manager_uid,
+                       description=None, member_uids=None):
         if self.project_exists(name):
-            raise exception.Duplicate("Project can't be created because project %s already exists" % name)
+            raise exception.Duplicate("Project can't be created because "
+                                      "project %s already exists" % name)
         if not self.user_exists(manager_uid):
-            raise exception.NotFound("Project can't be created because manager %s doesn't exist" % manager_uid)
+            raise exception.NotFound("Project can't be created because "
+                                     "manager %s doesn't exist" % manager_uid)
         manager_dn = self.__uid_to_dn(manager_uid)
         # description is a required attribute
         if description is None:
@@ -624,7 +668,8 @@ class LDAPWrapper(object):
         if member_uids != None:
             for member_uid in member_uids:
                 if not self.user_exists(member_uid):
-                    raise exception.NotFound("Project can't be created because user %s doesn't exist" % member_uid)
+                    raise exception.NotFound("Project can't be created "
+                            "because user %s doesn't exist" % member_uid)
                 members.append(self.__uid_to_dn(member_uid))
         # always add the manager as a member because members is required
         if not manager_dn in members:
@@ -655,16 +700,21 @@ class LDAPWrapper(object):
         if project_id == None:
             return FLAGS.__getitem__("ldap_%s" % role).value
         else:
-            return 'cn=%s,cn=%s,%s' % (role, project_id, FLAGS.project_ldap_subtree)
+            return 'cn=%s,cn=%s,%s' % (role,
+                                       project_id,
+                                       FLAGS.project_ldap_subtree)
 
-    def __create_group(self, group_dn, name, uid, description, member_uids = None):
+    def __create_group(self, group_dn, name, uid,
+                       description, member_uids = None):
         if self.group_exists(name):
-            raise exception.Duplicate("Group can't be created because group %s already exists" % name)
+            raise exception.Duplicate("Group can't be created because "
+                                      "group %s already exists" % name)
         members = []
         if member_uids != None:
             for member_uid in member_uids:
                 if not self.user_exists(member_uid):
-                    raise exception.NotFound("Group can't be created because user %s doesn't exist" % member_uid)
+                    raise exception.NotFound("Group can't be created "
+                            "because user %s doesn't exist" % member_uid)
                 members.append(self.__uid_to_dn(member_uid))
         dn = self.__uid_to_dn(uid)
         if not dn in members:
@@ -693,15 +743,12 @@ class LDAPWrapper(object):
 
     def remove_role(self, uid, role, project_id=None):
         role_dn = self.__role_to_dn(role, project_id)
-        try:
+        return self.remove_from_group(uid, role_dn)
-            return self.remove_from_group(uid, role_dn)
-        except Exception, ex:
-            print type(ex), ex
-
 
     def is_in_group(self, uid, group_dn):
         if not self.user_exists(uid):
-            raise exception.NotFound("User %s can't be searched in group becuase the user doesn't exist" % (uid,))
+            raise exception.NotFound("User %s can't be searched in group "
+                    "becuase the user doesn't exist" % (uid,))
         if not self.group_exists(group_dn):
             return False
         res = self.find_object(group_dn,
@@ -710,11 +757,14 @@ class LDAPWrapper(object):
 
     def add_to_group(self, uid, group_dn):
         if not self.user_exists(uid):
-            raise exception.NotFound("User %s can't be added to the group becuase the user doesn't exist" % (uid,))
+            raise exception.NotFound("User %s can't be added to the group "
+                    "becuase the user doesn't exist" % (uid,))
         if not self.group_exists(group_dn):
-            raise exception.NotFound("The group at dn %s doesn't exist" % (group_dn,))
+            raise exception.NotFound("The group at dn %s doesn't exist" %
+                                     (group_dn,))
         if self.is_in_group(uid, group_dn):
-            raise exception.Duplicate("User %s is already a member of the group %s" % (uid, group_dn))
+            raise exception.Duplicate("User %s is already a member of "
+                                      "the group %s" % (uid, group_dn))
         attr = [
             (ldap.MOD_ADD, 'member', self.__uid_to_dn(uid))
         ]
@@ -722,11 +772,14 @@ class LDAPWrapper(object):
 
     def remove_from_group(self, uid, group_dn):
         if not self.group_exists(group_dn):
-            raise exception.NotFound("The group at dn %s doesn't exist" % (group_dn,))
+            raise exception.NotFound("The group at dn %s doesn't exist" %
+                                     (group_dn,))
         if not self.user_exists(uid):
-            raise exception.NotFound("User %s can't be removed from the group because the user doesn't exist" % (uid,))
+            raise exception.NotFound("User %s can't be removed from the "
+                    "group because the user doesn't exist" % (uid,))
         if not self.is_in_group(uid, group_dn):
-            raise exception.NotFound("User %s is not a member of the group" % (uid,))
+            raise exception.NotFound("User %s is not a member of the group" %
+                                     (uid,))
         self._safe_remove_from_group(group_dn, uid)
 
     def _safe_remove_from_group(self, group_dn, uid):
@@ -734,14 +787,15 @@ class LDAPWrapper(object):
         attr = [(ldap.MOD_DELETE, 'member', self.__uid_to_dn(uid))]
         try:
             self.conn.modify_s(group_dn, attr)
-        except ldap.OBJECT_CLASS_VIOLATION:
+        except self.OBJECT_CLASS_VIOLATION:
             logging.debug("Attempted to remove the last member of a group. "
                           "Deleting the group at %s instead." % group_dn )
             self.delete_group(group_dn)
 
     def remove_from_all(self, uid):
         if not self.user_exists(uid):
-            raise exception.NotFound("User %s can't be removed from all because the user doesn't exist" % (uid,))
+            raise exception.NotFound("User %s can't be removed from all "
+                    "because the user doesn't exist" % (uid,))
         dn = self.__uid_to_dn(uid)
         role_dns = self.find_group_dns_with_member(
                 FLAGS.role_ldap_subtree, uid)
@@ -794,9 +848,8 @@ class LDAPWrapper(object):
 
     def delete_roles(self, project_dn):
         roles = self.find_roles(project_dn)
-        if roles != None:
+        for role in roles:
-            for role in roles:
+            self.delete_group('cn=%s,%s' % (role.id, project_dn))
-                self.delete_group('cn=%s,%s' % (role.id, project_dn))
 
     def delete_project(self, name):
         project_dn = 'cn=%s,%s' % (name, FLAGS.project_ldap_subtree)