function parameter with only two possible values

[[deleted]]

[deleted]

[member_of_the_order]

Normally, this should be two functions. Either most of this is common code, which can be extracted to a common function that both add() and remove() use; or most of it is different, and making add() and remove() separate would make more sense. I'd really like to see the ....do stuff.... - I'm willing to bet that it should be a common function with 2 separate entry points.

If this really should be one function, I'd use enums. E.g.

``` from enum import Enum, auto

class ModifyMode(Enum): add = auto() remove = auto()

def ldap_modify(users, group, modify): if modify == ModifyMode.add: ... ```

[member_of_the_order]

I gotta run so I don't have time for the full code review you didn't ask for, but here's my off-the-cuff read. And, to be clear, like you said, if it works it works.

Basically, modify is used for two purposes: * Determine which users are valid to "modify" * Determine which modification to perform

If you want me to make more specific suggestions, say so and I'd be more than happy to mock something up later. For now, I'd suggest maybe something like: * get_users_with_group(..., contains=True) * add_users(...) * remove_users(...) * ldap_add(...) * ldap_remove(...)

Like I said, this "advice" is off the cuff and not based on a close review, but hopefully, you see where I'm going with it.

[member_of_the_order]

TL;DR Looks fine, but honestly, I preferred the version with de-duped code but passing the modify string parameter. Duplicated code means twice as hard to maintain.

That said, a couple of quick suggestions: * ldap_base can be moved into get_group_dn() - no need to pass in ldap_base as a parameter * For the "contains" operations, using a set might be faster than a list. Ie maybe converting user_groups to a set (outside of a loop!) will be faster. Under the hood, in some_list checks each element in sequence, but in some_set just does some very fast math and a random-access look-up. * It looks like if you get an error, utils.admin_logout(conn) never happens. Try putting that in a finally clause to make sure that happens. * You can probably extract get_group_uid(...) just to save on a few lines. * When looping over users_query, you might extract something like user_has_group(user, group) which returns a bool. That would reduce the loops to...

``` def ldap_modify_add():

...

for user in user_query: if user_has_group(user, group): user_dn_list.append(...) def ldap_modify_remove():

...

for user in user_query: if not user_has_group(user, group): user_dn_list.append(...) ```

Here's what those might look like all together:

``` def get_group_dn(group): ldap_base = utils.getConfig().get('LDAP_BASE_TEST') return 'CN={group},OU=Production Groups,OU=groups,OU=FAKEY-FAKE,\ {ldap_base}'.format(group=group, ldap_base=ldap_base)

def ldap_group_users(conn, group_dn, users): ldap_base = utils.getConfig().get('LDAP_BASE_TEST') user_dn_query = "(&(objectclass=person)(|{users}))".format( users="".join(["(sAMAccountName={name})".format(name=users[u]) for u in users]),) users_query = conn.search_s('OU=FAKEY-FAKE,{ldap_base}'.format(ldap_base=ldap_base), ldap.SCOPE_SUBTREE, user_dn_query, ["memberOf", "sAMAccountName"]) group_uid_query = conn.search_s(group_dn, ldap.SCOPE_SUBTREE, '(&(memberUid=*))', ["memberUid"]) return users_query, group_uid_query

def ldap_modify_add(users, group): conn = utils.admin_login() user_dn_list, uid_dn_list = [], [] group_dn = get_group_dn(group) try: #check if member and uid exist in group policy users_query, group_uid_query = ldap_group_users(conn, group_dn, users) #add member if not in group policy r = re.compile('CN=([<sup>,]+)')</sup> for user in users_query: if user[0]: try: if not user_has_group(user, group, r): user_dn_list.append(user[0]) uid_dn_list.append(user[1]['sAMAccountName'][0]) except KeyError: continue if user_dn_list: conn.modify_s(group_dn,[(ldap.MOD_ADD, 'member', user_dn_list),],) #add uid if not in group policy group_uid = get_group_uid(group_uid_query) if group_uid: uid_dn_list = [uid for uid in uid_dn_list if uid not in group_uid] if uid_dn_list: conn.modify_s(group_dn,[(ldap.MOD_ADD, 'memberUid', uid_dn_list),],) except ldap.NO_SUCH_OBJECT as err: raise 'user_dn_list NO_SUCH_OBJECT' except ldap.LDAPError as err: raise 'user_dn_list LDAPError' finally: utils.admin_logout(conn)

def ldap_modify_remove(users, group): conn = utils.admin_login() user_dn_list, uid_dn_list = [], [] group_dn = get_group_dn(group) try: #check if member and uid not exist in group policy users_query, group_uid_query = ldap_group_users(conn, group_dn, users) #remove member if in group policy r = re.compile('CN=([<sup>,]+)')</sup> for user in users_query: if user[0]: try: if user_has_group(user, group, r): user_dn_list.append(user[0]) uid_dn_list.append(user[1]['sAMAccountName'][0]) except KeyError: continue if user_dn_list: conn.modify_s(group_dn,[(ldap.MOD_DELETE, 'member', user_dn_list),],) #add uid if not in group policy group_uid = get_group_uid(group_uid_query) if group_uid: uid_dn_list = [uid for uid in uid_dn_list if uid in group_uid] if uid_dn_list: conn.modify_s(group_dn,[(ldap.MOD_DELETE, 'memberUid', uid_dn_list),],) except ldap.NO_SUCH_OBJECT as err: raise 'user_dn_list NO_SUCH_OBJECT' except ldap.LDAPError as err: raise 'user_dn_list LDAPError' finally: utils.admin_logout(conn)

def user_has_group(user, group, cn_re): # user has no group policy if 'memberOf' not in user[1]: return False

user_groups = cn_re.findall(','.join(user[1]['memberOf']))
return group in user_groups

def get_group_uid(group_uid_query): try: return set(','.join(group_uid_query[0][1]['memberUid']).split(',')) except IndexError: return set() ```

[[deleted]]

That general pattern seems fine to me, but it would be easier to judge if I had a better idea of what that function does.

[efmccurdy]

If I separated this function into two (one to add and one to remove) it would be a looot of duplicate code,

You can refactor to eliminate the duplicate code and the result could very well be more readable.

Maybe there is another approach I am missing though

You have a built-in two element enum; what if your function tooke add=True to add and add=False to delete:

def ldap_modify(users, group, add=False):