Edgewall Software
Modify

Opened 12 years ago

Closed 10 years ago

Last modified 8 years ago

#8036 closed enhancement (fixed)

allow IPermissionRequestor to extend existing meta-permissions

Reported by: Stephen Compall <stephen.compall@…> Owned by: Remy Blank
Priority: high Milestone: 1.0
Component: general Version: 0.11.2.1
Severity: normal Keywords: patch permission
Cc: felix.schwarz@…, leho@… Branch:
Release Notes:

Plugins can now extend meta-permissions

API Changes:

perm: Made meta-permissions cumulative, so e.g. TICKET_ADMIN can be extended by plugins. [10417]

Internal Changes:

Description (last modified by Remy Blank)

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.

Attachments (3)

perm-action-alias-extension.diff (4.1 KB ) - added by Stephen Compall <stephen.compall@…> 12 years ago.
patch with unit testing
8036-perm-extension-r10316.patch (8.2 KB ) - added by Remy Blank 10 years ago.
Updated patch, refactors parts of PermissionSystem.
8036-perm-extension-r10380.patch (8.2 KB ) - added by Remy Blank 10 years ago.
Updated patch for current trunk.

Download all attachments as: .zip

Change History (18)

by Stephen Compall <stephen.compall@…>, 12 years ago

patch with unit testing

comment:1 by Stephen Compall <stephen.compall@…>, 12 years ago

Keywords: patch added

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.

comment:2 by anonymous, 12 years ago

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'])]

comment:3 by Christian Boos, 12 years ago

Milestone: 0.13

comment:4 by Christian Boos, 12 years ago

Keywords: permission added

comment:5 by Remy Blank, 10 years ago

Owner: set to Remy Blank

Interesting.

comment:6 by Felix Schwarz, 10 years ago

Cc: felix.schwarz@… added

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

comment:7 by Remy Blank, 10 years ago

Milestone: next-major-0.1X0.13
Priority: normalhigh

A gentle push, as you just did :)

comment:8 by Remy Blank, 10 years ago

Description: modified (diff)

by Remy Blank, 10 years ago

Updated patch, refactors parts of PermissionSystem.

comment:9 by Remy Blank, 10 years ago

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.

by Remy Blank, 10 years ago

Updated patch for current trunk.

in reply to:  6 comment:10 by Remy Blank, 10 years ago

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 :)

comment:11 by Remy Blank, 10 years ago

API Changes: modified (diff)
Resolution: fixed
Status: newclosed

Patch applied in [10417].

comment:12 by Christian Boos, 10 years ago

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()?

in reply to:  12 comment:13 by Remy Blank, 10 years ago

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].

comment:14 by lkraav <leho@…>, 10 years ago

Cc: leho@… added

comment:15 by Christian Boos, 8 years ago

Release Notes: modified (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Remy Blank to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.