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]