OSU-Net/cyder — pull request #670 by zeeman (2014-04-02 19:47:36+00:00)
Title: First stab at a permissions system refactor

This branch is my first attempt at refactoring the current implementation of permissions handling in Cyder. I know that this is supposed to be low priority, but in light of some recent happenings, I would like to solicit feedback on the idea of a permissions system refactor based on the code I have written so far.

Because of the complex nature of this refactor, I only did enough to get one model (DynamicInterface) and its corresponding views working. That allows this PR to serve as a proof of concept and a proposal for a new permissions system, but minimizes the amount of work required to update it as Cyder changes.

I don't expect this to be accepted quickly, since it's complex and something we'll want to consider carefully, especially as Cyder's permissions model changes. However, with the problems revealed in #653, I also believe that this is an opportune time to consider refactoring the entire permissions model.

### TL;DR
[`_has_perm`](https://github.com/OSU-Net/cyder/blob/e0749342f7f0bdcb786a19eff365511d723967f8/cyder/core/cyuser/backends.py#L55) is bad and I want to do something about it. I want to add a `.has_perm` method to every Model that defines permissions in the Model itself, instead of being hidden away in `cyder/core/cyuser/backends.py`. In order to reduce the effort involved in the refactor, I have reimplemented _has_perm as an adapter to the given object's built-in interface and written a function that can be used to generate `.has_perm` methods for classes that have simple permissions (i.e. all of them except for Token).

### Status Quo
Currently, the functions handling permissions checking are stored in `cyder.core.cyuser.backends`. The problems with this current system are numerous, there are a few that make it especially bad.

First, for some reason, `_has_perm` has its own logic for converting a string to a Model, despite the existence of `get_klasses`. As far as I can tell, this is because `cy_view` will infer what Model it's displaying based on part of the URL, but it passes along the string from the URL, rather than the inferred Model (which it discovers using `get_klasses`). At any rate, having multiple ways of inferring Model information from strings is bad and is just likely to create more issues if that logic ever has to change, so this code will help to further centralize that. This is exacerbated by all the bizarre string processing that `_has_perm` does to discover the correct string name.

Second, even after `_has_perm` knows what Model it's working with, it doesn't actually use it. This makes no sense to me, because the whole point of object-oriented programming is to encapsulate all of the rules and logic around a given type of data in one unit of code. It makes no sense to me to have a function that takes a Model and answers a question about that Model by associating it with information stored elsewhere when the class can contain that information itself.

Third, `_has_perm` is currently not well documented and is pretty well-hidden. While everyone who works on Cyder learns about it eventually, it is still a large source of confusion that I think needs to be eliminated in order to improve code quality, maintainability, and readability.

### The Solution
My solution is intended to handle all of these issues and to improve the logic of permissions handling overall.

First and most importantly, it creates a Model method called `.has_perm(user, ctnr, action)` (ultimately I hope to make `ctnr` an optional argument so you can check against all a user's containers at once) which can be called on both a class (for Model-level permissions) and instance (for row-level permissions) to determine if an action is permitted on that Model. This is a lot more logical to me than the current situation, because it lets objects define their own permissions instead of keeping them hidden. This way, if you want to know who can access a dynamic interface, you can just look at its model instead of searching for `has_dynamic_registration_perm` deep in `cyuser.backends`.

In order to minimize the amount of refactoring needed outside of models, I have also rewritten `cyuser.backends._has_perm` to be a wrapper around the given Model or instance's `.has_perm` method. This means everything that currently depends on `_has_perm` doesn't need to be rewritten, but new code will no longer need to rely on it. My hope is that old code will be refactored over time to use the new system so we can eventually get rid of `_has_perm` altogether, but obviously that's a ways off.

To speed up the time to write `.has_perm` methods, I've also written a function `custom_has_perm`, which allows users to define permissions by specifying a function for each user level which accepts the action to be taken and returns True if it is allowed for that level or False if it is not.

The new method also provides a clear advantage in terms of speed. My very limited testing shows the average speed of views using my `_has_perm` is .04s faster than when using the current `_has_perm`, with over 34,000 fewer function calls on average. This could use some more testing, but it looks like an appreciable performance boost.