Activity-based permission checking

Question

I threw together this Python function to check if a User has a certain Permission.

First of all, Permissions have this kind of format: category1.category2.some.task

and can also include wildcards like this: app.view.*

All the permissions a User has are stored in a list, so for example:

["app.cat1.*", "app.cat2.do.something", "app.cat2.cat3.*"]

You can then use the function below to check if a User has a certain Permission in his list:

def check_permission(needed_permission, permissions):
 #normalise needed permissions ending with a "." dot, convert filter to list again!
 needed_permissions_list = list(filter(None, needed_permission.split(".")))
 #loop over the permissions the user has, check if each matches the needed one until we find one and return True
 for permission in permissions:
 #permission is the permission the user already has and we are working on
 #permission_list is the list representation of the permission mentioned above
 permission_list = permission.split(".")
 #if the needed permission is exactly the same as the one we are working on, return True. E.g. needed: app.*, in list: app.*
 if permission_list == needed_permissions_list:
 return True
 #needed permission nearly matches, not the same because of wildcard. Return True
 if permission_list[-1] == "*" and (permission_list[:-1] == needed_permissions_list):
 return True
 #zip the to lists together to iterate
 zipped = zip(permission_list, needed_permissions_list)
 #iterate over the two lists simultaneousley
 for item, item2 in zipped:
 #whoa, an item doesn't match, time for further checks
 if item != item2:
 if item == "*":
 #it's not matching because it's a wildcard, so return True
 return True
 else:
 #it's not a wildcard, so it's definetely not the permission we are looking for
 break 
 return True
 #nothing matched, return False. Permission we're looking for not in the list
 return False
permissions = ["app.cat1.*", "app.cat2.do.something", "app.cat2.cat3.*"]
assert check_permission("app.cat2.do.something", permissions) == True
assert check_permission("app.cat2.do.something.special", permissions) == False
assert check_permission("app.cat1.some.task", permissions) == True
assert check_permission("app.cat2.cat4", permissions) == False
assert check_permission("app.cat2.cat3.do.something.special", permissions) == True
assert check_permission("something.completely.different", permissions) == False

Answer 1

This is completely unnecessarily complicated:

needed_permissions_list = list(filter(None, needed_permission.split(".")))

This is equivalent:

needed_permissions_list = needed_permission.split(".")

The return True at the end of the if item != item2 is unreachable, so you can delete that line.

item2 is not a good name. needed_item would be better, so you don't mix up which is which. It's important to know which is which, because the needed item must not contain *.

This is unnecessary:

 if permission_list[-1] == "*" and (permission_list[:-1] == needed_permissions_list):
 return True

This treats a special case, that the rest of the method automatically handles. You can safely remove this part.

This is a shorter and simpler implementation:

def check_permission(needed_permission, permissions):
 needed_permissions_list = needed_permission.split(".")
 for permission in permissions:
 permission_list = permission.split(".")
 if permission_list == needed_permissions_list:
 return True
 zipped = zip(permission_list, needed_permissions_list)
 for item, needed_item in zipped:
 if item != needed_item:
 if item == "*":
 return True
 else:
 break
 return False

The assertions are not written in a Pythonic way: you should avoid comparisons with True and False. This is the Pythonic way:

assert check_permission("app.cat2.do.something", permissions)
assert not check_permission("app.cat2.do.something.special", permissions)
assert check_permission("app.cat1.some.task", permissions)
assert not check_permission("app.cat2.cat4", permissions)
assert check_permission("app.cat2.cat3.do.something.special", permissions)
assert not check_permission("something.completely.different", permissions)

Your spec is not very clear. For example you didn't mention if the pattern a.*.c is valid. Probably not valid, because your code doesn't handle the case (for example a.b.x will yield true). Effectively, a pattern like a.*.c is handled as if it was a.*. It's good to specify this clearly.

I don't really like this special treatment:

 if permission_list == needed_permissions_list:
 return True

Because this check has an overlap with the logic in the for loop: if the two lists are not identical, the loop will repeat some of the checks. It would be better to get rid of this overlap, see this alternative implementation:

def check_permission2(needed_permission, permissions):
 needed_permissions_list = needed_permission.split(".")
 for permission in permissions:
 permission_list = permission.split(".")
 zipped = zip(permission_list, needed_permissions_list)
 for item, needed_item in zipped:
 if item != needed_item:
 if item == "*":
 return True
 else:
 break
 else:
 return len(permission_list) == len(needed_permissions_list)
 return False

Finally, another alternative implementation that also passes your assertions and somewhat simpler, taking a different approach without splitting to segments:

def check_permission(needed_permission, permissions):
 for permission in permissions:
 index_of_star = permission.find('*')
 if index_of_star == -1:
 if needed_permission == permission:
 return True
 else:
 normalized = permission[:index_of_star]
 if needed_permission.startswith(normalized):
 return True
 return False

Answer 2

needed_permissions_list = list(filter(None, needed_permission.split(".")))

can be

needed_permissions_list = [p for p in needed_permission.split('.') if p]

• Stefan: Thank you for your answer, however regarding the part "I think will only work if you can assume that the permissions list is sorted." I don't think you got how the code works :) The code is iterating over all permission and then splits each up in a list based on the "."
• Reinderien: Ah, you're right.