SitePoint Sponsor

User Tag List

Page 9 of 11 FirstFirst ... 567891011 LastLast
Results 201 to 225 of 267
  1. #201
    ********* Victim lastcraft's Avatar
    Join Date
    Apr 2003
    Location
    London
    Posts
    2,423
    Mentioned
    2 Post(s)
    Tagged
    0 Thread(s)
    Hi...

    Quote Originally Posted by dagfinn
    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) .

    Quote Originally Posted by dagfinn
    Here's an example from the thread's deep dark past,
    PHP Code:
    $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...?
    PHP Code:
    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
    Marcus Baker
    Testing: SimpleTest, Cgreen, Fakemail
    Other: Phemto dependency injector
    Books: PHP in Action, 97 things

  2. #202
    SitePoint Guru dagfinn's Avatar
    Join Date
    Jan 2004
    Location
    Oslo, Norway
    Posts
    894
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by lastcraft
    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.
    Dagfinn Reiersøl
    PHP in Action / Blog / Twitter
    "Making the impossible possible, the possible easy,
    and the easy elegant"
    -- Moshe Feldenkrais

  3. #203
    ********* Victim lastcraft's Avatar
    Join Date
    Apr 2003
    Location
    London
    Posts
    2,423
    Mentioned
    2 Post(s)
    Tagged
    0 Thread(s)
    Hi...

    Quote Originally Posted by dagfinn
    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...
    PHP Code:
    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 .

    Quote Originally Posted by dagfinn
    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.

    Quote Originally Posted by dagfinn
    Is the synchronization really that relevant?
    Now you mention it...no . This means exposing another class name for the editing.

    Quote Originally Posted by dagfinn
    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
    Marcus Baker
    Testing: SimpleTest, Cgreen, Fakemail
    Other: Phemto dependency injector
    Books: PHP in Action, 97 things

  4. #204
    SitePoint Guru dagfinn's Avatar
    Join Date
    Jan 2004
    Location
    Oslo, Norway
    Posts
    894
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by lastcraft
    ...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.
    Dagfinn Reiersøl
    PHP in Action / Blog / Twitter
    "Making the impossible possible, the possible easy,
    and the easy elegant"
    -- Moshe Feldenkrais

  5. #205
    ********* Victim lastcraft's Avatar
    Join Date
    Apr 2003
    Location
    London
    Posts
    2,423
    Mentioned
    2 Post(s)
    Tagged
    0 Thread(s)
    Hi...

    Quote Originally Posted by dagfinn
    I'm at all not convinced about your strategy of passing the DB setup into the constructor,
    More of a DAO.

    Quote Originally Posted by dagfinn
    I need to think more about that.
    Let's do an iteration and then think about it.

    yours, Marcus
    Marcus Baker
    Testing: SimpleTest, Cgreen, Fakemail
    Other: Phemto dependency injector
    Books: PHP in Action, 97 things

  6. #206
    SitePoint Zealot sike's Avatar
    Join Date
    Oct 2002
    Posts
    174
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    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

  7. #207
    SitePoint Guru dagfinn's Avatar
    Join Date
    Jan 2004
    Location
    Oslo, Norway
    Posts
    894
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by lastcraft
    Let's do an iteration and then think about it.
    An iteration with what kind of representation for permissions?
    Dagfinn Reiersøl
    PHP in Action / Blog / Twitter
    "Making the impossible possible, the possible easy,
    and the easy elegant"
    -- Moshe Feldenkrais

  8. #208
    SitePoint Zealot
    Join Date
    Dec 2003
    Location
    with my kids
    Posts
    116
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    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.

  9. #209
    Non-Member
    Join Date
    Jan 2004
    Location
    Planet Earth
    Posts
    1,764
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Off Topic:

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

  10. #210
    ********* Victim lastcraft's Avatar
    Join Date
    Apr 2003
    Location
    London
    Posts
    2,423
    Mentioned
    2 Post(s)
    Tagged
    0 Thread(s)
    Hi...

    Quote Originally Posted by dagfinn
    An iteration with what kind of representation for permissions?
    The one we have now.

    yours, Marcus
    Marcus Baker
    Testing: SimpleTest, Cgreen, Fakemail
    Other: Phemto dependency injector
    Books: PHP in Action, 97 things

  11. #211
    ********* Victim lastcraft's Avatar
    Join Date
    Apr 2003
    Location
    London
    Posts
    2,423
    Mentioned
    2 Post(s)
    Tagged
    0 Thread(s)
    Hi...

    Quote Originally Posted by sike
    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.

    Quote Originally Posted by sike
    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.

    Quote Originally Posted by sike
    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
    Marcus Baker
    Testing: SimpleTest, Cgreen, Fakemail
    Other: Phemto dependency injector
    Books: PHP in Action, 97 things

  12. #212
    SitePoint Guru dagfinn's Avatar
    Join Date
    Jan 2004
    Location
    Oslo, Norway
    Posts
    894
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by sike
    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.
    Quote Originally Posted by josheli
    ... 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 Reiersøl
    PHP in Action / Blog / Twitter
    "Making the impossible possible, the possible easy,
    and the easy elegant"
    -- Moshe Feldenkrais

  13. #213
    SitePoint Guru dagfinn's Avatar
    Join Date
    Jan 2004
    Location
    Oslo, Norway
    Posts
    894
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by lastcraft
    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.)
    Dagfinn Reiersøl
    PHP in Action / Blog / Twitter
    "Making the impossible possible, the possible easy,
    and the easy elegant"
    -- Moshe Feldenkrais

  14. #214
    ********* Victim lastcraft's Avatar
    Join Date
    Apr 2003
    Location
    London
    Posts
    2,423
    Mentioned
    2 Post(s)
    Tagged
    0 Thread(s)
    Hi...

    Quote Originally Posted by dagfinn
    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.

    Quote Originally Posted by dagfinn
    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
    Marcus Baker
    Testing: SimpleTest, Cgreen, Fakemail
    Other: Phemto dependency injector
    Books: PHP in Action, 97 things

  15. #215
    SitePoint Guru dagfinn's Avatar
    Join Date
    Jan 2004
    Location
    Oslo, Norway
    Posts
    894
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by lastcraft
    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.

    Quote Originally Posted by lastcraft
    Go for it!
    Just a minute. Here it is:

    PHP Code:
    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'));
        }

    Dagfinn Reiersøl
    PHP in Action / Blog / Twitter
    "Making the impossible possible, the possible easy,
    and the easy elegant"
    -- Moshe Feldenkrais

  16. #216
    SitePoint Evangelist
    Join Date
    Dec 2003
    Location
    Arizona
    Posts
    411
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by dagfinn
    Seems pretty obvious when you say it. Too simple for me to understand, I guess.


    Just a minute. Here it is:

    PHP Code:
    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...

  17. #217
    ********* Victim lastcraft's Avatar
    Join Date
    Apr 2003
    Location
    London
    Posts
    2,423
    Mentioned
    2 Post(s)
    Tagged
    0 Thread(s)
    Hi...

    Quote Originally Posted by dagfinn
    Here it is:
    Here is the first minimal cut. Refactor to taste. First the test case (slightly modified) in acceptance_tests.php...
    PHP Code:
    <?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 Code:
    <?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 Code:
    <?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 Code:
    <?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 Code:
    <?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 Code:
    <?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
    Marcus Baker
    Testing: SimpleTest, Cgreen, Fakemail
    Other: Phemto dependency injector
    Books: PHP in Action, 97 things

  18. #218
    SitePoint Guru dagfinn's Avatar
    Join Date
    Jan 2004
    Location
    Oslo, Norway
    Posts
    894
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by lastcraft
    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.
    Dagfinn Reiersøl
    PHP in Action / Blog / Twitter
    "Making the impossible possible, the possible easy,
    and the easy elegant"
    -- Moshe Feldenkrais

  19. #219
    ********* Victim lastcraft's Avatar
    Join Date
    Apr 2003
    Location
    London
    Posts
    2,423
    Mentioned
    2 Post(s)
    Tagged
    0 Thread(s)
    Hi...

    Quote Originally Posted by dagfinn
    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
    Marcus Baker
    Testing: SimpleTest, Cgreen, Fakemail
    Other: Phemto dependency injector
    Books: PHP in Action, 97 things

  20. #220
    Non-Member
    Join Date
    Jan 2004
    Location
    Planet Earth
    Posts
    1,764
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Or we could somehow integrate the behavior directly into our Domain Model.
    No

    Maybe you could explain your reasoning behind this motive ?

  21. #221
    SitePoint Guru dagfinn's Avatar
    Join Date
    Jan 2004
    Location
    Oslo, Norway
    Posts
    894
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by lastcraft
    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.
    PHP Code:
    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.
    Dagfinn Reiersøl
    PHP in Action / Blog / Twitter
    "Making the impossible possible, the possible easy,
    and the easy elegant"
    -- Moshe Feldenkrais

  22. #222
    ********* Victim lastcraft's Avatar
    Join Date
    Apr 2003
    Location
    London
    Posts
    2,423
    Mentioned
    2 Post(s)
    Tagged
    0 Thread(s)
    Hi...

    Quote Originally Posted by dagfinn
    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.

    Quote Originally Posted by dagfinn
    PHP Code:
        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.

    Quote Originally Posted by dagfinn
    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.

    Quote Originally Posted by dagfinn
    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...
    PHP Code:
    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...
    PHP Code:
    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
    Marcus Baker
    Testing: SimpleTest, Cgreen, Fakemail
    Other: Phemto dependency injector
    Books: PHP in Action, 97 things

  23. #223
    SitePoint Guru dagfinn's Avatar
    Join Date
    Jan 2004
    Location
    Oslo, Norway
    Posts
    894
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    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.

    Quote Originally Posted by lastcraft
    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.
    Quote Originally Posted by lastcraft
    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.
    Quote Originally Posted by lastcraft
    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?
    Dagfinn Reiersøl
    PHP in Action / Blog / Twitter
    "Making the impossible possible, the possible easy,
    and the easy elegant"
    -- Moshe Feldenkrais

  24. #224
    ********* Victim lastcraft's Avatar
    Join Date
    Apr 2003
    Location
    London
    Posts
    2,423
    Mentioned
    2 Post(s)
    Tagged
    0 Thread(s)
    Hi...

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

    Quote Originally Posted by dagfinn
    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.

    Quote Originally Posted by dagfinn
    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.

    Quote Originally Posted by dagfinn
    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.

    Quote Originally Posted by dagfinn
    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.

    Quote Originally Posted by dagfinn
    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
    Marcus Baker
    Testing: SimpleTest, Cgreen, Fakemail
    Other: Phemto dependency injector
    Books: PHP in Action, 97 things

  25. #225
    SitePoint Evangelist
    Join Date
    Dec 2003
    Location
    Arizona
    Posts
    411
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by lastcraft
    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


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •