allow IPermissionRequestor to extend existing meta-permissions

[Stephen Compall <stephen.compall@…>]

Sometimes you might want to introduce a new kind of ticket permission, and add it to TICKET_ADMIN without having to wholly redefine it. A good illustration comes from a test IPermissionRequestor I have defined:

def get_permission_actions(self):
    return ['TEST_CREATE', 'TEST_DELETE', 'TEST_MODIFY',
            ('TEST_CREATE', []),
            ('TEST_ADMIN', ['TEST_CREATE', 'TEST_DELETE']),
            ('TEST_ADMIN', ['TEST_MODIFY'])]

Except with the imagination that TEST_CREATE actually has something, and the TEST_ADMIN variants are in separate IPermissionRequestors.

[Stephen Compall <stephen.compall@…>]

patch with unit testing

[Stephen Compall <stephen.compall@…>]

The attached patch documents the extension to get_permission_actions's capabilities, tweaks get_actions and expand_actions, mostly rewrites TicketSystem.get_user_permissions, and a test support code change (shown in description) that causes test_meta_permissions to fail if the remainder of the patch is not present.

[anonymous]

Apologies, I forgot to remove the --lines from the diff. Here is the corrected example:

def get_permission_actions(self):
    return ['TEST_CREATE', 'TEST_DELETE', 'TEST_MODIFY',
            ('TEST_CREATE', []),
            ('TEST_ADMIN', ['TEST_CREATE', 'TEST_DELETE']),
            ('TEST_ADMIN', ['TEST_MODIFY'])]

[Remy Blank]

Interesting.

[Felix Schwarz]

what's necessary to get this patch in? I'd like to see this fixed. Is it just a code review that's missing?

[Remy Blank]

A gentle push, as you just did :)

[Remy Blank]

Updated patch, refactors parts of PermissionSystem.

[Remy Blank]

The initial patch didn't apply cleanly anymore, so I had to dig deeper into PermissionSystem. It turns out that it could be cleaned up quite a bit (mostly by removing duplication).

8036-perm-extension-r10316.patch​ implements the requested functionality and passes all tests. As the permission subsystem is quite critical, it would be great if I could get some review and testing, especially from someone using authz_policy​.

[Remy Blank]

Updated patch for current trunk.

[Remy Blank]

Replying to fschwarz:

what's necessary to get this patch in? I'd like to see this fixed. Is it just a code review that's missing?

Yes, the code review is all that's missing (hint, hint :)

[Remy Blank]

Patch applied in [10417].

[Christian Boos]

Sorry for the lack of review, I looked at it and found the change was looking good, but I didn't test nor tried to go deeper. One suggestion I should have made is that having both get_actions() and _get_actions() methods is not optimal, especially given the latter is also documented which suggests it should also be part of the public API. Maybe rename it to get_actions_dict()?

[Remy Blank]

Replying to cboos:

One suggestion I should have made is that having both get_actions() and _get_actions() methods is not optimal, especially given the latter is also documented which suggests it should also be part of the public API. Maybe rename it to get_actions_dict()?

Yes, I wasn't happy with that name either, but I'm notoriously bad at finding good names :) get_actions_dict() sounds good, changed in [10418].