3
\$\begingroup\$

I've written a simple permissions system that I need to be rather fast, yet useable by other developers who primarily use Java. Is a bitwise permissions system with properties a good compromise. Any suggested speed improvements would be greatly appreciated.

class AppliancePerms(object):
    """Permissions for the Appliance class."""
    _cmd_right = 2 ** 0
    _rest_right = 2 ** 1
    _ssh_right = 2 ** 2
    _num_of_rights = 3

    _all_rights = _cmd_right + _rest_right + _ssh_right

    def __init__(self, perm_digits=None):
        # default to all rights
        if perm_digits is None:
            self._perms = AppliancePerms._all_rights
        else:
            self._perms = perm_digits

    @property
    def permissions(self):
        # zfill to show the total number of permissions even when left most right isn't True
        return bin(self._perms)[2:].zfill(AppliancePerms._num_of_rights)

    @permissions.setter
    def permissions(self, perm_digits):
        """
        Set all permissions at once with a number.
        Suggested syntax is AppliancePermOInst.permissions(int('111', 2))
        """
        self._perms = perm_digits

    @permissions.deleter
    def permissions(self):
        self._perms = 0

    @property
    def rest(self):
        return self._perms & AppliancePerms._rest_right

    @rest.setter
    def rest(self, boolean):
        if boolean:
            self._perms |= AppliancePerms._rest_right
        else:
            del self.rest

    @rest.deleter
    def rest(self):
        if self._perms & AppliancePerms._rest_right:
            self._perms -= AppliancePerms._rest_right

    @property
    def ssh(self):
        return self._perms & AppliancePerms._ssh_right

    @ssh.setter
    def ssh(self, boolean):
        if boolean:
            self._perms |= AppliancePerms._ssh_right
        else:
            del self.ssh

    @ssh.deleter
    def ssh(self):
        if self._perms & AppliancePerms._ssh_right:
            self.perms -= AppliancePerms._ssh_right

    @property
    def cmd(self):
        return self._perms & AppliancePerms._cmd_right

    @cmd.setter
    def cmd(self, boolean):
        if boolean:
            self._perms |= AppliancePerms._cmd_right
        else:
            del self.cmd

    @cmd.deleter
    def cmd(self):
        if self._perms & AppliancePerms._cmd_right:
            self._perms -= AppliancePerms._cmd_right
\$\endgroup\$
2
  • 1
    \$\begingroup\$ You say "useable by non-python programmers", are they end-users, developers that will utilise your library or are they developers that will maintain the code? \$\endgroup\$
    – Peilonrayz
    Oct 6, 2016 at 8:44
  • 2
    \$\begingroup\$ Since this is supposed to be usable by non-python programmers, an example of how to use this would help. \$\endgroup\$
    – Graipher
    Oct 6, 2016 at 9:07

2 Answers 2

2
\$\begingroup\$

I don't like how static and WET your class is. You don't really need all the _rights, and you should probably remove the repetition in all your properties, except permissions.
I'd make a function to build properties for you. Take the cmd property:

@property
def cmd(self):
    return self._perms & AppliancePerms._cmd_right

@cmd.setter
def cmd(self, boolean):
    if boolean:
        self._perms |= AppliancePerms._cmd_right
    else:
        del self.cmd

@cmd.deleter
def cmd(self):
    if self._perms & AppliancePerms._cmd_right:
        self._perms -= AppliancePerms._cmd_right

This is almost identical to the other two functions, the only difference is AppliancePerms._cmd_right. And so if we were to want to write a function to build this then we'd start by replacing that, with a say variable bit. Since you're using @property, rather than property I'm not sure how the del fn_name would work, and so I'd change it to use the property function. This means we can get:

def build_permission_property(bit):
    def get(self):
        return self._perms & bit

    def set(self, boolean):
        if boolean:
            self._perms |= bit
        else:
            delete(self)

    def delete(self):
        if self._perms & bit:
            self._perms -= bit

    return property(get, set, delete)

This works as you'd want, but I'd change the delete to use a bit mask. Since it'll be a mask we can then use &=, and remove the branch. To make the mask we need it actually really easy. It's just ~bit and since Python's int's are 'infinite' it'll work on all numbers. I'd also change get to return 1 or 0 as it stands it can return 0, 1, 2, 4, .... Changing these can result in:

def build_permission_property(bit_index):
    bit = 1 << bit_index
    def get(self):
        return (self._perms & bit) >> bit_index

    def set(self, boolean):
        if boolean:
            self._perms |= bit
        else:
            delete(self)

    def delete(self):
        self._perms &= ~bit

    return property(get, set, delete)

And to use it is quite simple. We just set the class' variable to the property. I'd keep _num_of_rights, and would change the permissions getter to use str.format And so could result in:

class AppliancePerms(object):
    def __init__(self, perm_digits=None):
        # Default to all permissions
        if perm_digits is None:
            perm_digits = (1 << type(self)._num_of_rights) - 1
        self._perms = perm_digits

    cmd = build_permission_property(0)
    rest = build_permission_property(1)
    ssh = build_permission_property(2)
    _num_of_rights = 3

    @property
    def permissions(self):
        return '{:0>{}b}'.format(self._perms, type(self)._num_of_rights)

    @permissions.setter
    def permissions(self, perm_digits):
        self._perms = perm_digits

    @permissions.deleter
    def permissions(self):
        self._perms = 0

This is quite nice, all you need to do is manually set the properties, and _num_of_rights to the amount. But it's still too WET for me. And so I'd use the more advanced Python feature of metaclasses. This may not be good for developers that don't know Python too well. I'd base this metaclass off one of martineau's answers on SO.

You can program this in two classes, but I'd prefer three as then if you need more permission classes it's easy to implement. These classes are:

  • Permission metaclass.
    This will take a list from the class, and build the properties.

  • Base permissions class.
    This will inherit the metaclass in a Python 2 and Python 3 friendly way. (the cross-Python metaclass inheritance is odd.) It will also define the permissions property, and default _perms to zero, so subclasses don't have to.

  • Subclass.
    This will be a domain specific class that provides the list of properties. It will also define custom functionality, such as defaulting to all permissions.

This could become:

def build_permission_property(bit_index):
    bit = 1 << bit_index
    def get(self):
        return (self._perms & bit) >> bit_index

    def set(self, boolean):
        if boolean:
            self._perms |= bit
        else:
            delete(self)

    def delete(self):
        self._perms &= ~bit

    return property(get, set, delete)


class PermissionsMetaClass(type):
    def __new__(meta, classname, bases, class_dict):
        dict_ = class_dict.copy()
        dict_.pop('__module__', None)
        dict_.pop('__qualname__', None)
        for i, perm in enumerate(dict_.get('_permissions', [])):
            dict_[perm] = build_permission_property(i)
        return type.__new__(meta, classname, bases, dict_)


class Permissions(PermissionsMetaClass("Permissions", (object,), {})):
    _perms = 0
    @property
    def permissions(self):
        return '{:0>{}b}'.format(self._perms, len(type(self)._permissions))

    @permissions.setter
    def permissions(self, perm_digits):
        self._perms = perm_digits

    @permissions.deleter
    def permissions(self):
        self._perms = 0


class AppliancePerms(Permissions):
    _permissions = ['cmd', 'rest', 'ssh']
    def __init__(self, perm_digits=None):
        # Default to all permissions
        if perm_digits is None:
            perm_digits = (1 << len(type(self)._permissions)) - 1
        self._perms = perm_digits

I've provided both with and without a metaclass, as metaclasses are a bit harder to understand. And so it's up to you which design you'd rather use. I mostly just recommend adding the function build_permission_property.

\$\endgroup\$
1
\$\begingroup\$

Interface usage

Given an AppliancePerms instance app_perm, you have app_perm.rest, app_perm.ssh and app_perm.cmd accept and return integers while app_perm.permissions accept an integer but return a string. This is, at best, confusing.

Furthermore, reading at the variable names, the intended usage for setting rest, ssh and cmd is to use app_perm.ssh = True or app_perm.cmd = False whereas getting the value app_perm.ssh will return 2 or 0.

You should improve the consistency here:

  • return booleans when accessing individual fields;
  • return integers when accessing the whole permissions.

Proper formatting

On the other hand, I understand that you would like to have an pretty rendering of the selected bits. One way of doing it, is returning a subclass of int that will show itself as a bitfield:

class BitField(int):
    def __repr__(self):
        return '{:0{}b}'.format(self, 3)

You’ll note that I simplified the formating from integer to string. str.format is very versatile and you should favor it instead of manually manipulating string. Usage of this class being:

>>> five = BitField(5)
>>> print(five)
5
>>> str(five)
'5'
>>> repr(five)
'101'
>>> five
101

In order to be trully versatile, we should be able to parametrize the building of this bitfield by its length. Unfortunately, integers are immutable and we can modify them in their __init__ method. So we need to use __new__ instead:

class BitField(int):
    def __new__(cls, value, length):
        the_integer = super().__new__(cls, value)
        the_integer._field_length = length
        return the_integer

    def __repr__(self):
        return '{:0{}b}'.format(self, self._field_length)

Parsing provided permissions

In the same vein, using app_perms.permissions = int('101', 2) is unfriendly. So you should parse it in your @permissions.setter. Getting a bitfield and setting it back should obviously work, but you could support being assigned a string or even an integer reprensenting a bitfield so '101', 101 or BitField(5) could work the same.

Proposed improvements

class BitField(int):
    def __new__(cls, value, length):
        the_integer = super().__new__(cls, value)
        the_integer._field_length = length
        return the_integer

    def __repr__(self):
        return '{:0{}b}'.format(self, self._field_length)


class AppliancePerms(object):
    """Permissions for the Appliance class."""
    _cmd_right = 2 ** 0
    _rest_right = 2 ** 1
    _ssh_right = 2 ** 2
    _num_of_rights = 3

    _all_rights = _cmd_right + _rest_right + _ssh_right

    def __init__(self, perm_digits=None):
        # default to all rights
        if perm_digits is None:
            self._perms = AppliancePerms._all_rights
        else:
            self._perms = perm_digits

    @property
    def permissions(self):
        return BitField(self._perms, AppliancePerms._num_of_rights)

    @permissions.setter
    def permissions(self, perm_digits):
        self._perms = int(repr(perm_digits), 2)

    @permissions.deleter
    def permissions(self):
        self._perms = 0

    @property
    def rest(self):
        return bool(self._perms & AppliancePerms._rest_right)

    @rest.setter
    def rest(self, boolean):
        if boolean:
            self._perms |= AppliancePerms._rest_right
        else:
            del self.rest

    @rest.deleter
    def rest(self):
        if self._perms & AppliancePerms._rest_right:
            self._perms -= AppliancePerms._rest_right

    @property
    def ssh(self):
        return bool(self._perms & AppliancePerms._ssh_right)

    @ssh.setter
    def ssh(self, boolean):
        if boolean:
            self._perms |= AppliancePerms._ssh_right
        else:
            del self.ssh

    @ssh.deleter
    def ssh(self):
        if self._perms & AppliancePerms._ssh_right:
            self.perms -= AppliancePerms._ssh_right

    @property
    def cmd(self):
        return bool(self._perms & AppliancePerms._cmd_right)

    @cmd.setter
    def cmd(self, boolean):
        if boolean:
            self._perms |= AppliancePerms._cmd_right
        else:
            del self.cmd

    @cmd.deleter
    def cmd(self):
        if self._perms & AppliancePerms._cmd_right:
            self._perms -= AppliancePerms._cmd_right
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.