Patterns Tutorial Series (part 1): RBAC Domain Model

[lastcraft]

Hi...

Looking at the older part of this thread, I see I'm questioning some of the decisions that were made early. I understand how that might be annoying.

With 200 posts (at this one in fact!) some stuff will need reiteration and highlighting. In fact it is only by peer review that we will end up with anything good. Not sure I would want to go over the requirements again with every new person that signs up though (hint to lurkers) .

Here's an example from the thread's deep dark past,

$authoriser = &new Authoriser($this->_configuration);
$edit = &$authorisor->createEdit(); 

My point is this: To create the Edit object which is in sync with the Authoriser, you have to keep $authoriser alive.

The original idea was to pass the configuration into the edit (later replaced by directly passing the data accessor). Because the edit set up is taken from the Authoriser they are guaranteed to be in sync, or talking to the same database at least. If the Authoriser is destroyed (this was what I was thinking) then the edit should still have a reference to everything it needs.

There are other ways to set up the edit object, but it would mean that both this and the Authoriser need a global access point for shared data (which is pretty much constant).

If you want to set the configuration with define() statements for now then that is fine for the moment. My feeling is that the storage class is the way to go, but I don't want to predujice the design right now. Something like this then...?

define('DATABASE_ENGINE', 'Postgres');
define('DATABASE_NAME', 'rbac_authorisation');
define('TABLE_PREFIX', 'rbac_');
... 

These can be set up in the test runner script (developer dependent) rather than the test case script (shared source code). I am not a fan of doing things this way, but it is common enough in the PHP world that it is easy to understand.

yours, Marcus

[dagfinn]

If the Authoriser is destroyed (this was what I was thinking) then the edit should still have a reference to everything it needs.

In that case the edit has the scope issue. You have to keep it alive from the time it's created until the time it's used. If the context differs enough between those two points, it will have be global. The two points are the same as before, so it's exactly the same problem except that another variable is having it.

Unless, that is, either the Edit or the Authoriser is used in both places, but I don't see that as likely either.

So my question still stands: how does the factory trick help avoid globals? You may have something up your sleeve, but I still can't see it.

On the other hand, I'm not sure it's important. The deeper question is, will the two points even occur in the same application? The way I see it, a lot of applications may be using the Authoriser, only one or a few administrative applications will be using the Edit. The administrative applications will need both (unless they're restricted by some other mechanism). But do we know that the storage use to authorize the administrative user will be the same as the storage the administrative user is editing? Will they be the same for simplicity or different for security? Is the synchronization really that relevant?

Another thing that occurs to me when I say that is that if you have several applications using the same RBAC data, the permissions probably will have to be application-specific. That effectively annihilates my own bright idea that the permissions can be defined by a set of constants.

[lastcraft]

Hi...

In that case the edit has the scope issue. You have to keep it alive from the time it's created until the time it's used. If the context differs enough between those two points, it will have be global. The two points are the same as before, so it's exactly the same problem except that another variable is having it.

OK, I now see that I have been jumping ahead in my minds eye without realising. What could happen with the factory connection is that we would push the linkage up to the application level again. It would be up to the app. developer to use either a subclass, a mutual factory, a registry or configuration. Say a subclass...

class MyAuthoriser extends Authoriser {
    function MyAuthoriser() {
        $this->Authoriser(What goes here?);
    }
} 

However they could add the factory method themselves at that point. They could also do something else entirely, so I have inadvertently designed ahead .

But do we know that the storage use to authorize the administrative user will be the same as the storage the administrative user is editing?

They must use the same database and login of course. To be frank I don't know what goes into the constructor of the AuthorisationEdit or the Authoriser. If it's all wrapped up in a class passed into both then it won't much matter anyway.

Is the synchronization really that relevant?

Now you mention it...no . This means exposing another class name for the editing.

That effectively annihilates my own bright idea that the permissions can be defined by a set of constants.

...or any global means. This kind of information (multiple apps) is the app. domain. This means that the DB set up must go into the constructor from the app.

If you write the test case, I'll implement.

yours, Marcus

[dagfinn]

...or any global means. This kind of information (multiple apps) is the app. domain. This means that the DB set up must go into the constructor from the app.

I think we need to separate two issues here. One is the storage configuration issue, the other is the issue of how to represent the permissions. I'm at all not convinced about your strategy of passing the DB setup into the constructor, but I have a feeling it can safely be ignored for the moment; it shouldn't be hard to change later.

The more important issue is how to represent the permissions. How it seems to me now is that the admin app must know about the permissions needed by the other apps. Which probably means we have to go back to creating them explicitly and storing them in the database (or other persistent storage). I need to think more about that.

[lastcraft]

Hi...

I'm at all not convinced about your strategy of passing the DB setup into the constructor,

More of a DAO.

I need to think more about that.

Let's do an iteration and then think about it.

yours, Marcus

[sike]

i am one of the lurkers and i must admit
that this thread started with interesting ideas,
but after a while it just got stuck.

it seems to me that you all don't share the same
coding styles / strategies for solving problems and thus
block each other. i mean >200 posts and you are near to
nothing (except some nice discussions).

so why don't we start with a very simple approach
implementing the tests and refactor the whole thing step
by step (aka test driven design). this will make the first steps
alot easier and hopefully the different approaches of you all will
end up in a nicely usable set of well tested classes.

Sike

ps. the craftsmen series from the link above are fun to read

[dagfinn]

Let's do an iteration and then think about it.

An iteration with what kind of representation for permissions?

[josheli]

another lurker here. what's interesting is that you would think this type framework/application/library/domain model/whatever it is would be well-implemented in the web application space. indeed, one would think that about lots of things: data-abstraction, authorization, general webapp frameworks... yet, they are all continually reinvented and no clear solutions ever win the day. this phenomenon seems to be most prevalent in PHP. Python, Java, .Net, etc. seem to have more fairly well-established (at least de facto) standards and modules/ways of doing things.

anyway, you guys need a project manager, a benevolent dictator, a Guido/Linus-type or you'll never get anywhere.

[Widow Maker]

Off Topic:

I'm not a lurker, I'm just browsing

[lastcraft]

Hi...

An iteration with what kind of representation for permissions?

The one we have now.

yours, Marcus

[lastcraft]

Hi...

it seems to me that you all don't share the same
coding styles / strategies for solving problems and thus
block each other. i mean >200 posts and you are near to
nothing (except some nice discussions).

I must admit I can only laugh sometimes . It does mirror what happens face to face, although face to face discussions happen at a much faster rate. I have been rather shocked just how different people's approaches are. I would have expected much more agreement than we are getting even now. The earlier part, where we couldn't even agree on requirements at all (still difficult) was really frustrating, but then the two big time suckers are usually requirements and bugs.

I think the discussion has deepened of late. I'm optimistic.

so why don't we start with a very simple approach
implementing the tests and refactor the whole thing step
by step

Because we can't even agree on the test case . I feel we are getting somewhere, just rather slowly.

ps. the craftsmen series from the link above are fun to read

The Agile Software Development book is a real classic as well.

yours, Marcus

[dagfinn]

it seems to me that you all don't share the same
coding styles / strategies for solving problems and thus
block each other. i mean >200 posts and you are near to
nothing (except some nice discussions).

Your observation that we don't all share the same coding styles and strategies for solving problems is absolutely correct. And we seem to block each other, but what I think we're doing is learning from each other. The differences impede progress on the outside, but accelerate progress on the inside. That's my experience, anyway. What I've learned from this so far is extraordinary.

... yet, they are all continually reinvented and no clear solutions ever win the day. this phenomenon seems to be most prevalent in PHP. Python, Java, .Net, etc. seem to have more fairly well-established (at least de facto) standards and modules/ways of doing things.

Maybe this lack of de facto standards comes from not doing what we've been doing in this thread. Maybe everybody rushes ahead with their own half-baked idea and don't take the time to have it challenged by others or to consider different alternative designs.

[dagfinn]

The one we have now.

We don't have one. But if I ignore that, too, and look at your test code again, the only substantial change I would make is to use completely separate classes for administration ("Edit") and authorization. So setup() and tearDown() would use one class (I might call it something like RbacAdmin), and the test cases could still use the Authoriser.

The permissions are still an issue in my opinion, but if you think you know how to solve it, you're welcome to try. Your test class seems to imply that the permissions themselves are not stored in the database, but for the reasons I've mentioned, I doubt that's viable.

Lesser issues: I would replace the word Principal, you forgot to put in an addRole(), and the drop methods aren't actually being tested. (They're only being used to clean up.)

[lastcraft]

Hi...

Your test class seems to imply that the permissions themselves are not stored in the database, but for the reasons I've mentioned, I doubt that's viable.

They are stored in a database, but they are recovered as a simple object in a one shot query, at least that was my idea. Actually it doesn't affect the interface in any case so I don't see any problem using it as is.

Lesser issues: I would replace the word Principal, you forgot to put in an addRole(), and the drop methods aren't actually being tested. (They're only being used to clean up.)

Go for it! Rewrite the test case and I'll do a crap implementation and then a first try at refactoring. Also don't put the drop test stuff in yet and lets maybe have the role creation automatic for a bit until we hit trouble. I am still feeling ambitious .

I think the drop and add can be tested in unit tests (the one we have now is more of a simple acceptance test). Unit tests could over constrain the design as they are normally written along with the code. As we cannot realistically pair (or group) over a news thread, better if we just swap roles from customer to developer and back.

yours, Marcus

[dagfinn]

They are stored in a database, but they are recovered as a simple object in a one shot query, at least that was my idea. Actually it doesn't affect the interface in any case so I don't see any problem using it as is.

Seems pretty obvious when you say it. Too simple for me to understand, I guess.

Go for it!

Just a minute. Here it is:

class RoleBasedPermissionsTest extends UnitTestCase {
    function RoleBasedPermissionsTest() {
        $this->UnitTestCase();
    }
    function setUp() {
        $admin = &new RbacAdmin(); 
        $admin->addUserAccount('fred'); //changed Principal to UserAccount
        $admin->addRole('pleb'); // added this; obviously necessary
        $admin->assign('fred', 'pleb');
        $admin->permit('pleb', 'do_stuff');
    }
    function tearDown() {
        $admin = &new RbacAdmin(); 
        $admin->dropUserAccount('fred');
        $admin->dropRole('pleb');
    }
    function testNonUserHasNothingAllowed() {
        $authoriser = &new Authoriser();
        $permissions = &$authoriser->getPermissions('nobody');
        $this->assertFalse($permissions->can('do_stuff'));
    }
    function testLegitimateUserHasActionAllowed() {
        $authoriser = &new Authoriser();
        $permissions = &$authoriser->getPermissions('fred');
        $this->assertTrue($permissions->can('do_stuff'));
    }
    function testUserCannotDoNonAction() {
        $authoriser = &new Authoriser();
        $permissions = &$authoriser->getPermissions('fred');
        $this->assertFalse($permissions->can('do_unknown'));
    }
} 

[seratonin]

Seems pretty obvious when you say it. Too simple for me to understand, I guess.


Just a minute. Here it is:

class RoleBasedPermissionsTest extends UnitTestCase {
    function RoleBasedPermissionsTest() {
        $this->UnitTestCase();
    }
    function setUp() {
        $admin = &new RbacAdmin(); 
        $admin->addUserAccount('fred'); //changed Principal to UserAccount
        $admin->addRole('pleb'); // added this; obviously necessary
        $admin->assign('fred', 'pleb');
        $admin->permit('pleb', 'do_stuff');
    }
    function tearDown() {
        $admin = &new RbacAdmin(); 
        $admin->dropUserAccount('fred');
        $admin->dropRole('pleb');
    }
    function testNonUserHasNothingAllowed() {
        $authoriser = &new Authoriser();
        $permissions = &$authoriser->getPermissions('nobody');
        $this->assertFalse($permissions->can('do_stuff'));
    }
    function testLegitimateUserHasActionAllowed() {
        $authoriser = &new Authoriser();
        $permissions = &$authoriser->getPermissions('fred');
        $this->assertTrue($permissions->can('do_stuff'));
    }
    function testUserCannotDoNonAction() {
        $authoriser = &new Authoriser();
        $permissions = &$authoriser->getPermissions('fred');
        $this->assertFalse($permissions->can('do_unknown'));
    }
} 

that looks like a pretty solid test case to me...

[lastcraft]

Hi...

Here it is:

Here is the first minimal cut. Refactor to taste. First the test case (slightly modified) in acceptance_tests.php...

<?php
require_once('../authoriser.php');
require_once('../administrator.php');
require_once('../schema_builder.php');
require_once('DB.php');

class RoleBasedPermissionsTest extends UnitTestCase { 
    function RoleBasedPermissionsTest() { 
        $this->UnitTestCase();
        $this->_connection = &DB::connect(RBAC_CONNECTION);
        if (DB::isError($this->_connection)) {
            trigger_error($this->_connection->message . $this->_connection->userinfo);
        }
        $builder = &new SchemaBuilder($this->_connection);
        $builder->dropSchema();
        $builder->createSchema();
    } 
    function setUp() { 
        $admin = &new RbacAdministrator($this->_connection); 
        //$admin->addRole('pleb');
        $admin->permit('pleb', 'do_stuff');
        $admin->assign('fred', 'pleb'); 
    } 
    function tearDown() { 
        $admin = &new RbacAdministrator($this->_connection); 
        $admin->dropAccess('fred'); 
        $admin->dropRole('pleb'); 
    } 
    function testNonUserHasNothingAllowed() { 
        $authoriser = &new Authoriser($this->_connection); 
        $permissions = &$authoriser->getPermissions('nobody'); 
        $this->assertFalse($permissions->can('do_stuff')); 
    } 
    function testLegitimateUserHasActionAllowed() { 
        $authoriser = &new Authoriser($this->_connection); 
        $permissions = &$authoriser->getPermissions('fred'); 
        $this->assertTrue($permissions->can('do_stuff')); 
    } 
    function testUserCannotDoNonAction() { 
        $authoriser = &new Authoriser($this->_connection); 
        $permissions = &$authoriser->getPermissions('fred'); 
        $this->assertFalse($permissions->can('do_unknown')); 
    } 
} 
?>

And for completeness the runner that goes with it. I used the same Mysql user to create the schema, and so the permissions have to be pretty liberal. Really it should be a separate user for this. I called it all_tests.php myself, but it doesn't matter. You will have to set the Mysql string anyway...

<?php
define('SIMPLE_TEST', '/var/www/html/simpletest/');
require_once(SIMPLE_TEST . 'unit_tester.php');
require_once(SIMPLE_TEST . 'reporter.php');

define('RBAC_CONNECTION', 'mysql://me:secret@uno/sitepoint');
   
$test = &new GroupTest('Sitepoint advPHP RBAC');
$test->addTestFile('acceptance_tests.php');
$test->run(new HtmlReporter());
?>

authoriser.php...

<?php
require_once(dirname(__FILE__) . '/permissions.php');
require_once('DB.php');

class Authoriser {
    var $_connection;
    
    function Authoriser(&$connection) {
        $this->_connection = &$connection;
    }
    
    function &getPermissions($access) {
        $result = &$this->_query(
                "select action from assignments, permissions " .
                "where assignments.role = permissions.role " .
                "and access = '$access'");
        $all = array();
        while ($row = &$result->fetchRow(DB_FETCHMODE_ASSOC)) {
            $all[] = $row['action'];
        }
        return new Permissions($all);
    }
    
    function &_query($sql) {
        $result = &$this->_connection->query($sql);
        if (DB::isError($result)) {
            trigger_error($result->message . ', ' . $result->userinfo);
        }
        return $result;
    }
}
?>

administrator.php...

<?php
require_once('DB.php');

class RbacAdministrator {
    var $_connection;
    
    function RbacAdministrator(&$connection) {
        $this->_connection = &$connection;
    }
    
    function permit($role, $action) {
        $this->_query(
                "replace into permissions (role, action) " .
                "values ('$role', '$action')");
    }
    
    function assign($access, $role) {
        $this->_query(
                "replace into assignments (role, access) " .
                "values ('$role', '$access')");
    }
    
    function dropAccess($access) {
        $this->_query("delete from assignments where access='$access'");
    }
    
    function dropRole($role) {
        $this->_query("delete from assignments where role='$role'");
        $this->_query("delete from permissions where role='$role'");
    }
    
    function _query($sql) {
        $result = $this->_connection->query($sql);
        if (DB::isError($result)) {
            trigger_error($result->message . ', ' . $result->userinfo);
        }
    }
}
?>

permissions.php

<?php
class Permissions {
    var $_all;
    
    function Permissions($all) {
        $this->_all = $all;
    }
    
    function can($action) {
        return in_array($action, $this->_all);
    }
}
?>

...and finally schema_builder.php...

<?php
require_once('DB.php');

class SchemaBuilder {
    var $_connection;
    
    function SchemaBuilder(&$connection) {
        $this->_connection = &$connection;
    }
    
    function dropSchema() {
        $this->_query('drop table if exists permissions');
        $this->_query('drop table if exists assignments');
    }
    
    function createSchema() {
        $this->_query(
                'create table permissions (' .
                'role char(32),' .
                'action char(32), ' .
                'unique (role, action))');
        $this->_query(
                'create table assignments (' .
                'access char(32), ' .
                'role char(32),' .
                'unique (access, role))');
    }
    
    function _query($sql) {
        $result = $this->_connection->query($sql);
        if (DB::isError($result)) {
            trigger_error($result->message . ', ' . $result->userinfo);
        }
    }
}
?>

I tried to make everything as minimal as possible with the least amount of work on my part. Of course you pay the 10% clutter tax with the PEAR error handling and so the first refactoring is probably to wrap the connection. The table names are hard coded (problem if there is only one database) and some Mysql specific stuff may well have crept in. It's also blatantly unnormalised.

Anyway, plenty of refactoring opportunities here .

yours, Marcus

[dagfinn]

I tried to make everything as minimal as possible with the least amount of work on my part. Of course you pay the 10% clutter tax with the PEAR error handling and so the first refactoring is probably to wrap the connection. The table names are hard coded (problem if there is only one database) and some Mysql specific stuff may well have crept in. It's also blatantly unnormalised.

Anyway, plenty of refactoring opportunities here .

yours, Marcus

Here's my first attempt at analyzing what you've done.

What you've done is interesting, unexpected and very sneaky; I've never seen it before. You haven't represented any of the domain objects in the database, only their relationships. That is as minimal as possible.

What happens then is that every time you add a relationship, you implicitly create two domain objects unless they already exist. If you misspell a name, you've created a new domain object by that name.

Like I said, it's interesting. but it probably won't work well in practice. So if we don't like it, we need to change the test case. I'm going to think about that.

The obvious refactoring candidate is the _query() method. I agree that the best way to handle that would be to make an object to wrap the connection (a Resource Decorator if you want a fancy name for it) and move the method there.

[lastcraft]

Hi...

What happens then is that every time you add a relationship, you implicitly create two domain objects unless they already exist. If you misspell a name, you've created a new domain object by that name.

Your analysis is spot on and I hadn't thought about that way. It is deliberately a long way from perfect, especially on the data side. Ii is a solution to the test case, which test the event of authorsation, not the RBAC model. Once we have some constraints for the admin. the model should become visible and so explicit.

More tests please...

yours, Marcus

[Widow Maker]

Or we could somehow integrate the behavior directly into our Domain Model.

No

Maybe you could explain your reasoning behind this motive ?

[dagfinn]

More tests please...

OK. I made another class for it, since the setups conflict to some extent. On the other hand, there's some duplication between them as well.

class RbacAdministratorTest extends UnitTestCase {
    var $admin;
    function RbacAdministratorTest() {
        $this->UnitTestCase();
        $this->_connection = &DB::connect(RBAC_CONNECTION);
        if (DB::isError($this->_connection)) {
            trigger_error($this->_connection->message . $this->_connection->userinfo);
        }
        $builder = &new SchemaBuilder($this->_connection);
        $builder->dropSchema();
        $builder->createSchema();
    }
    function setUp() {
        $admin = &new RbacAdministrator($this->_connection);
        //This will presumably have to be added to the other test class as well:
        $admin->addAccess('fred'); // I don't like the term, but I won't waste time arguing about it :-)
        $admin->addRole('pleb');
        $admin->addPermission('do_stuff');
    }
    function testCanAssignExisting() {
        $this->assertTrue($admin->assign('fred','pleb'));
    }
    function testCanPermitExisting() {
        $this->assertTrue($admin->permit('pleb','do_stuff'));
    }
    function testCannotAssignNonExistent() {
        $admin = &new RbacAdministrator($this->_connection);
        $this->assertFalse($admin->assign('nobody','pleb'));
        $this->assertFalse($admin->assign('fred','norole'));
    }
    function testCannotPermitNonExistent() {
        $admin = &new RbacAdministrator($this->_connection);
        $this->assertFalse($admin->permit('norole','do_stuff'));
        $this->assertFalse($admin->pleb('pleb','do_unknown'));
    }
    // You could add the drop tests here as well
} 

An important question I've ignored i how exactly to do the error handling. Using exceptions would probably be a good idea in PHP 5. I just used a dumb TRUE/FALSE return code instead.

Actually, these tests don't guarantee that you won't cheat by sending the FALSE return value and doing the operation anyway. I guess I'm naive.

[lastcraft]

Hi...

I made another class for it, since the setups conflict to some extent. On the other hand, there's some duplication between them as well.

Cool. I think most of the duplication is in the connection anyway.

    function setUp() {
        $admin = &new RbacAdministrator($this->_connection);
        $admin->addAccess('fred');
        $admin->addRole('pleb');
        $admin->addPermission('do_stuff');
    } 

I don't like this way of setting things up because you can get orphaned objects in the database. There is a user interface principle called "conformance". Basically if you don't want something to happen, don't make it possible.

The main extra point of the tests you have added sems to be not allowing the creation of a user without a role. I think this is an important point and should be enforced.

An important question I've ignored i how exactly to do the error handling. Using exceptions would probably be a good idea in PHP 5. I just used a dumb TRUE/FALSE return code instead.

I think a boolean return is best, but we need a way of recovering the error as opposed to a connection problem. The solution I am going to propose is pants, but will advance us another iteration. Simply store the last error. This adds unwelcome state to tha administrator and again pushes me in the direction of having an RbacEdit/RbacTransaction/UnitOfWork type of object to handle all of this. Maybe next time.

Actually, these tests don't guarantee that you won't cheat by sending the FALSE return value and doing the operation anyway. I guess I'm naive.

I think your instincts are correct. We need a better way.

We will want to view the state of the system before editing it (I hope). That means we need a way of capturing the state. We could add accessors: getRoles(), getPermissionsForRole(), getAllAccesses(). Or we could pass in a couple of visitors...

class AccessVisitor() {
    function receiveListing($access, $roles) { ... }
}

class RoleVisitor() {
    function receiveListing($role, $permissions) { ... }
} 

The latter fits display code better, but will need mocks to test and is less intuitive to non-OO users. I am not ready for that yet.

Here is a possible refactored test suite...

require_once('../authoriser.php');
require_once('../administrator.php');
require_once('../schema_builder.php');
require_once('../connection.php');

class RoleBasedPermissionsTest extends UnitTestCase { 
    function RoleBasedPermissionsTest() { 
        $this->UnitTestCase();
        $builder = &new SchemaBuilder(new RbacConnection(RBAC_CONNECTION); 
        $builder->dropSchema(); 
        $builder->createSchema(); 
    } 
    function setUp() { 
        $administrator = &new RbacAdministrator(new RbacConnection(RBAC_CONNECTION); 
        $administrator->permit('pleb', 'do_stuff');
        $administrator->assign('fred', 'pleb'); 
    } 
    function tearDown() { 
        $administrator = &new RbacAdministrator(new RbacConnection(RBAC_CONNECTION); 
        $administrator->dropAccess('fred'); 
        $administrator->dropRole('pleb'); 
    } 
    function testNonUserHasNothingAllowed() { 
        $authoriser = &new Authoriser(new RbacConnection(RBAC_CONNECTION); 
        $permissions = &$authoriser->getPermissions('nobody'); 
        $this->assertFalse($permissions->can('do_stuff')); 
    } 
    function testLegitimateUserHasActionAllowed() { 
        $authoriser = &new Authoriser(new RbacConnection(RBAC_CONNECTION); 
        $permissions = &$authoriser->getPermissions('fred'); 
        $this->assertTrue($permissions->can('do_stuff')); 
    } 
    function testUserCannotDoNonAction() { 
        $authoriser = &new Authoriser(new RbacConnection(RBAC_CONNECTION); 
        $permissions = &$authoriser->getPermissions('fred'); 
        $this->assertFalse($permissions->can('do_unknown')); 
    } 
}

class RbacAdministratorTest extends UnitTestCase { 
    var $_administrator;
    
    function RbacAdministratorTest() { 
        $this->UnitTestCase(); 
        $builder = &new SchemaBuilder(new RbacConnection(RBAC_CONNECTION); 
        $builder->dropSchema(); 
        $builder->createSchema(); 
    } 
    function setUp() { 
        $this->_administrator = &new RbacAdministrator(
                new RbacConnection(RBAC_CONNECTION)); 
    } 
    function testCanAssignOnlyToAnExistingRole() { 
        $this->_administrator->permit('pleb', 'do_stuff');
        $this->assertTrue($this->_administrator->assign('fred','pleb'));
        $this->assertIdentical($this->_administrator->getLastError(), false);
        
        $this->_administrator->dropAccess('fred');
        $this->_administrator->dropRole('pleb');
    } 
    function testCannotAssignToNonRole() { 
        $this->assertFalse($this->_administrator->assign('fred','no role'));
        $this->assertEqual(
                $this->_administrator->getLastError(),
                'Role [no role] does not exist');
    }
    function testEmptySystemHasNothing() {
        $this->assertIdentical($this->_administrator->getRoles(), array())
        $this->assertIdentical($this->_administrator->getAccesses(), array())
    }
    function testCanReportSingleRole() {
        $this->_administrator->permit('pleb', '1');
        $this->_administrator->permit('pleb', '2');
        $this->assertIdentical(
                $this->_administrator->getRoles(),
                array('pleb' => array('1', '2')));
        
        $this->_administrator->dropRole('pleb');
    }
    function testCanReportMultipleAccess() {
        $this->_administrator->permit('pleb', 'do_stuff');
        $this->_administrator->assign('1','pleb')
        $this->_administrator->assign('2','pleb')
        $this->assertIdentical(
                $this->_administrator->getAccesses(),
                array('1' => array('pleb'), '2' => array('pleb'))))
        
        $this->_administrator->dropAccess('1');
        $this->_administrator->dropAccess('2');
        $this->_administrator->dropRole('pleb');
    }
} 

Your turn to implement?

yours, Marcus

[dagfinn]

I've been a bit busy, but I finally read all your fine print--I think. It's not entirely clear to me what you're doing.

Hi...
The main extra point of the tests you have added sems to be not allowing the creation of a user without a role.

No. The extra point (and it was the only one) was to not allow the creation of relationships between the objects (user, role, permission) without creating the objects themselves first.

I think a boolean return is best, but we need a way of recovering the error as opposed to a connection problem.

I suggest forget about connection problems. Don't have that cluttering the basic design at this stage. In fact, let's forget about error handling. I just added it for the sake of the test. Let's test the actual result of the operation instead of an error return.

If I were really concerned with error handling at this stage and had to do it in PHP 4, I would write a custom error handler.

We will want to view the state of the system before editing it (I hope). That means we need a way of capturing the state. We could add accessors: getRoles(), getPermissionsForRole(), getAllAccesses().

Yes. The reason I didn't add that was that I was only trying to solve one problem. In place of getRoles(), we will need getRolesForAccess(). Or $access->getRoles().

A couple of questions about your proposed test suite:

testCanAssignOnlyToAnExistingRole(): You're dropping the objects without creating them first. An oversight?

testCannotAssignToNotRole(): Are you testing a non-role, a non-access, or both?

[lastcraft]

Hi...

This post is my 666th, so expect pure evil...

The extra point (and it was the only one) was to not allow the creation of relationships between the objects (user, role, permission) without creating the objects themselves first.

I think needed objects should be created as needed, and orphans removed automatically, otherwise the interface will treble in size. Say you add a permission. Now you need a way to list permissions in the system when they are attached to roles as well as checking a permisison is present when no roles are hooked up to it. Suddenly you have createPermission(), dropPermission(), isPermission(), show<Orphaned|All>Permissions(), permit(), deny(), addRole(), dropRole(), showAllRoles(), showPermissionsByRole() . This is before we add usage into the mix.

By using just the permit() method to create things we prevent meaningless situations occouring in the first place (affordance). As well as stripping a load of simple accessors out of the interface we take away the maintanence burden from the client. After all they just want to set permissions to a user, not micromanage our library.

But there is an exception. I would suspect that the permission structure edits would be done at a different time from assigning users and could get out of sync. Assigning users would probably be done automatically as well. Thus the exception is trying to assign a role to an access key without that role existing. It means an isRole() method somewhere.

In fact, let's forget about error handling. I just added it for the sake of the test. Let's test the actual result of the operation instead of an error return.

Ok, I don't mind smaller steps.

If I were really concerned with error handling at this stage and had to do it in PHP 4, I would write a custom error handler.

I really don't think it is the job of our library to tell the application how to handle errors as it just leads to endless wrapping and extra interfaces. Look at the suffering PEAR:: DB inflicts on us . In lieu of exceptions a simple last error accessor is the least damage we can do, but if a connection error uses another mechanism then there is really only one possible error state. So at this juncture I agree - it is unnecessary.

testCanAssignOnlyToAnExistingRole(): You're dropping the objects without creating them first. An oversight?

No for once. The permit() method has enough information to do everything it needs.

testCannotAssignToNotRole(): Are you testing a non-role, a non-access, or both?

A non-role. The access can be created if it doesn't exist as outlined above. Adding independent access keys doesn't make sense without a role. In fact adding them would be a three step process: Add access (low level), add role (administrator), assign access (low level again). Nothing is gained with that first step.

The reverse is not true though so we have an intersting case coming up. If the administrator adds a role, a bunch of access keys are added and then the admin. drops the role, we don't want to dump the entire user base. For this reason I think that dropRole() should not be allowed if users are attached to only that role. This area needs work, perhaps with some typical use cases.

yours, Marcus

[seratonin]

Hi...

This post is my 666th, so expect pure evil...



I think needed objects should be created as needed, and orphans removed automatically, otherwise the interface will treble in size. Say you add a permission. Now you need a way to list permissions in the system when they are attached to roles as well as checking a permisison is present when no roles are hooked up to it. Suddenly you have createPermission(), dropPermission(), isPermission(), show<Orphaned|All>Permissions(), permit(), deny(), addRole(), dropRole(), showAllRoles(), showPermissionsByRole() . This is before we add usage into the mix.

By using just the permit() method to create things we prevent meaningless situations occouring in the first place (affordance). As well as stripping a load of simple accessors out of the interface we take away the maintanence burden from the client. After all they just want to set permissions to a user, not micromanage our library.

But there is an exception. I would suspect that the permission structure edits would be done at a different time from assigning users and could get out of sync. Assigning users would probably be done automatically as well. Thus the exception is trying to assign a role to an access key without that role existing. It means an isRole() method somewhere.



Ok, I don't mind smaller steps.



I really don't think it is the job of our library to tell the application how to handle errors as it just leads to endless wrapping and extra interfaces. Look at the suffering PEAR:: DB inflicts on us . In lieu of exceptions a simple last error accessor is the least damage we can do, but if a connection error uses another mechanism then there is really only one possible error state. So at this juncture I agree - it is unnecessary.



No for once. The permit() method has enough information to do everything it needs.



A non-role. The access can be created if it doesn't exist as outlined above. Adding independent access keys doesn't make sense without a role. In fact adding them would be a three step process: Add access (low level), add role (administrator), assign access (low level again). Nothing is gained with that first step.

The reverse is not true though so we have an intersting case coming up. If the administrator adds a role, a bunch of access keys are added and then the admin. drops the role, we don't want to dump the entire user base. For this reason I think that dropRole() should not be allowed if users are attached to only that role. This area needs work, perhaps with some typical use cases.

yours, Marcus

I agree that orphans need to be handled transparently.

JT