[kforge-dev] Access control code: refactoring and simplifying

[Rufus Pollock]

Below are some observations on the access control code and how it could 
be improved. If people are happy with the suggested changes i am happy 
to be responsible for implementing them in the next iteration(s).

Regards,

Rufus

= Access control code: refactoring and simplifying =

A while back I discovered a bug in our authorization system whereby a 
logged in user could not access a project of which he/she was a not a 
member even though visitor could (ticket:??).

After a couple of hours debugging (and adding tests) I tracked this to 
accesscontrol.py. Having worked through that code and corrected the bug 
I think we should try to simplify the access control system. In 
particular I would suggest:

== 1. removing personal grants and bars ==

These are only present to address the issue of allowing users to edit 
their own profiles. This problem could be addressed in just two or three 
lines of code and would allow the domain mode to be simplified as well

Combination of grants, bars and normal RBAC is confusing:
   * what happens if a user has a grant to object A, a bar on object A and
   a role that permits access to object A? (Answer: access allowed)
   * what happens if a user has a grant on object A, a bar on object A?
   (Answer: access denied)
   * etc ...

== 2. Disambiguate meaning of service ==

It is confusing that an example service object in project annakarenina 
is just the service object (and therefore readable) but the example 
plugin in the context of that project represents the subsystem 
application and is *not* readable. We need some way to clearly 
disambiguate a service domain object and the running application 
(subversion repository/moinmoin wiki etc) which that represents both in 
our general discourse and as protection objects in the access control 
system.

== 3. Reduce the repetition in access control code and tests ==

This repetition has grown up partly as a result of the natural accretion 
of code as development has progressed. Having several seperate layers 
each wrapping the one below but makes it hard to work out the logic of 
what is going on.

For example:
   * apache.urlpermission.isAccessAllowed calls
   * accesscontrol.ProjectAccessController.isAuthorized implicitly calls
   * core.accesscontrol.AbstractAccessController.isAuthorized calls
   * accesscontrol.ProjectAccessController.hasAuthorizedRole uses
     * accesscontrol.SystemAccessController.hasAuthorizedRole
     * accesscontrol.ProjectAccessController.isProjectRoleAuthorized

We also have:
     * core.accesscontrol.AbstractAccessController wraps 
core.command.AuthoriseAccess command converting exception into False
     * this is then reused by AccessControllers in accesscontrol.py
     * Next layer of commands in accesscontrol.py then rewraps this true 
or false back into No Exception (OK)/Exception (Not OK)

Do we really need all these separate layers? Probably not. specifically

   1. Why do we need the wrapper commands accesscontrol.py? 
(Is...ActionObjectAuthorised) Why not just use 
accessController.isAuthorized direct?

   2. Why don't we just override isAuthorised rather than 
hasAuthorisedRole. If we need to validate input (which is the generic 
thing done in AbstractAccessController.isAuthorised why not do this in 
__init__?)

   3. Why have logging in base class and in inheriting class saying 
similar stuff?

   4. why use person /object/, protected /object/ but /name/ of action?

== Suggest we change to the following ==

class AuthorizeAccess(Command)

     def __init__(self, person, action, object, context=None)
         # deal with defaulting (e.g. no person then make them visitor ...)

     def execute(self):
         # to be overridden

     def isAuthorized(self):
         # wrapper on execute to return True and False

class AuthorizeSystemAccess(AuthorizeAccess):

     def execute(self):
         # insert exception for person editing themselves
         # get system role and do access

class AuthorizeProjectAccess(AuthorizeAccess):

     def execute(self):
         # do AuthorizeSystemAccess
         # do AuthorizeAccess with person = visitor
         # if self.person != visitor
         #   do AuthorizeAccess with person = self.person