HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Activity-based permission checking

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
activitypermissioncheckingbased

Problem

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

Solution

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

Code Snippets

needed_permissions_list = list(filter(None, needed_permission.split(".")))
needed_permissions_list = needed_permission.split(".")
if permission_list[-1] == "*" and (permission_list[:-1] == needed_permissions_list):
        return True
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
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)

Context

StackExchange Code Review Q#74225, answer score: 3

Revisions (0)

No revisions yet.