[Beaker-devel] Moving business logic out of Beaker's view layers

[Nick Coghlan]

Currently, we have a lot of business logic in our view layers, including
various permissions checks. This is problematic, since it can lead to
permission errors in different UIs, and also means we need to do
systematic testing of permissions errors on each view, which makes the
integration tests slow.

There's a better way to handle this kind of thing: moving the
permissions checks to lower level model code and systematically testing
them there, and using structured exceptions to communicate that
information to the view layer in an abstract way.

As an example of how that can work:

    class UserPermissionsError(Exception):
        """User does not have appropriate permissions"""
        def __init__(self, user, action):
            self.user = user
            self.action = action
            msg = "%s not permitted for %s"
            super(UserPermissionsError).__init__(msg)

    class Target(object):

        # assorted other code

        def can_change_name(self, user):
            return user == self.owner # For example

        def set_name(self, name, user):
            if not self.can_change_name(user):
                action = "Changing name of %r" % self
                raise UserPermissionsError(user, action)
            # Potentially also log the change in an audit table
            self.name = name

In the views, the "can_change_name" query API would be used to control
permissions based display of various UI elements, while the actual
modification operations can rely on the data model layer to do the
permission checks, rather than having to duplicate that logic in every view.

Beaker 0.13 and 0.14 have started us partway down this road (Jobs have
several can_* methods now), but I'd like us to start moving towards
checked update methods at the data model layer, too. This also ties in
with the "ActivityMixin" that was added in 0.13, which moves activity
tracking out of the view layer and into the model layer.

I see two critical benefits to this approach:

1. From a security perspective, it makes it less likely that permissions
will differ between views, since all views should be using common
permission checking and activity tracking code when working with a given
part of the model.
2. From a testing perspective, having these operations in the model
layer means we can do our systematic testing at that level, rather than
going through the view layer. Our view testing (which is the really slow
part of the tests) can then be simplified to ensure the presence/absence
of widgets for various permissions (the "can_*" calls), as well as
correct handling of UserPermissionsError (on actual operations). This
frees up the integration tests from needing to check all of the
different reasons *why* someone may not have permission to do something,
and instead lets us treat it as a binary "permissions or not" test case.

Cheers,
Nick.

P.S. to actually handle this in a more comprehensive way, you don't
expose the raw data model layer to the views at all - you force them to
go through an intermediate layer that enforces the permission checks.
That's not really practical for Beaker's current state though - we first
need to move the business logic out of the views and down to the data
model layer before we can even think of separating the "application
model" (which cares about permissions and audit logs) from the "data
storage model" (which doesn't).

-- 
Nick Coghlan
Red Hat Infrastructure Engineering & Development, Brisbane

Testing Solutions Team Lead
Beaker Development Lead (http://beaker-project.org/)