Best practice for permission checking - a resume on recent Trac plugin tickets

10 views

Skip to first unread message

Steffen Hoffmann

unread,

Feb 14, 2011, 3:43:28 PM2/14/11

to trac...@googlegroups.com

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello,

this time I'm bringing a recent discussion about permission checking to
your attention. I'm further more asking for advice on how to handle this
in the future.

Summary:
Initially there where two reports related reports against
PrivateTicketsPlugin [1][2] and TracHoursPlugin [3][4] about
incompatible permission check in TracHoursPlugin that caused an error
only if PrivateTicketsPlugin was activated too. This had been meant to
be resolved by two successive changesets [5][6] in TracHoursPlugin. But
later the same issue surfaced again for the combination of
PrivateTicketsPlugin with AnnouncerPlugin [7][8]. This time there was
some more discussion about what's actually going on inside.

Thanks to Odd Simon Simonsen and Ryan J. Ollos it became clear that
recurring issues sprang off from usage of
PermissionSystem.check_permission() [9] without `perm` argument. While
default policies (DefaultPermissionPolicy, LegacyAttachmentPolicy) don't
seem to have a problem with perm=None, PrivateTicketsPolicy does.

Proper permission checks should be fine-grained and always query
permissions per resource for such realms like tickets and wiki. But this
is not obvious when reading current doc-strings in perm.py . So should
we propose a documentation improvement, or is there another more
appropriate action? After all docs will not prevent more variations of
that issue, while a less tolerant API might do.

Trying to introduce fine-grained permission checks to AnnouncerPlugin
now I'm facing another challenge:
How do I detect the resource identifier of an arbitrary resource?

I'd be glad to be corrected, if I've overlooked something, but there
seems no way to be sure today. While initially it seems common to have
resource.name (i.e. resource = ticket) this is not true for wiki pages:
resource.id, so in the absence of a common naming convention it seem
impossible to code a permission query for arbitrary resources of realm
xyz. But this is vital to do such things like generic change
announcements that are extensible to resources provided by other
plugins, like the screenshots [10] or tags [11].

Any comments and suggestions are welcome.

Sincerely,

Steffen Hoffmann
(hasienda)

iEYEARECAAYFAk1Zk+4ACgkQ31DJeiZFuHd8gACfeXcwAIvPpRgkc0AleKCH3csl
/EMAn13Rh3PcNWGYi3oZRKKnLK4SgctW
=88Mq
-----END PGP SIGNATURE-----

Christian Boos

unread,

Feb 14, 2011, 4:02:38 PM2/14/11

to trac...@googlegroups.com

On 2/14/2011 9:43 PM, Steffen Hoffmann wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hello,
>
> this time I'm bringing a recent discussion about permission checking to
> your attention. I'm further more asking for advice on how to handle this
> in the future.
>
> Summary:
> Initially there where two reports related reports against
> PrivateTicketsPlugin [1][2] and TracHoursPlugin [3][4] about
> incompatible permission check in TracHoursPlugin that caused an error
> only if PrivateTicketsPlugin was activated too. This had been meant to
> be resolved by two successive changesets [5][6] in TracHoursPlugin. But
> later the same issue surfaced again for the combination of
> PrivateTicketsPlugin with AnnouncerPlugin [7][8]. This time there was
> some more discussion about what's actually going on inside.
>
>
> Thanks to Odd Simon Simonsen and Ryan J. Ollos it became clear that
> recurring issues sprang off from usage of
> PermissionSystem.check_permission() [9] without `perm` argument. While
> default policies (DefaultPermissionPolicy, LegacyAttachmentPolicy) don't
> seem to have a problem with perm=None, PrivateTicketsPolicy does.

perm=None means we're checking for permission actions as a whole.
Known to have delicate semantics (see the whole discussion of #6211
which lead to the current documentation in perm.py; I just notice that
#6211 is still re-opened, another indication that we're not 100% clear
on this issue).
And frankly, the TICKET_VIEW permissions and al. are something of the
past, that we were not able to ditch yet. I'd happily move to short name
actions ('view', 'modify', etc.) on mandatory resources anytime. See
also
http://trac.edgewall.org/wiki/TracDev/Proposals/EvenFinerGrainedPermissions
for "recent" thoughts on future directions for the permission system. If
it's not already written there, I should add that some regex-based
authzpolicy permission system could probably help for speeding up the
thing considerably.

> Proper permission checks should be fine-grained and always query
> permissions per resource for such realms like tickets and wiki. But this
> is not obvious when reading current doc-strings in perm.py . So should
> we propose a documentation improvement, or is there another more
> appropriate action? After all docs will not prevent more variations of
> that issue, while a less tolerant API might do.

According to the current semantic, resource==None is a way to express
whole realm checks; i.e. if you are granted WIKI_VIEW for resource ==
None, that means you *may* have access to more specific wiki resources.
That would actually be the same as 'view' for Resource('wiki') in a
system with short action names and mandatory resources.

> Trying to introduce fine-grained permission checks to AnnouncerPlugin
> now I'm facing another challenge:
> How do I detect the resource identifier of an arbitrary resource?

I don't follow you here. Arbitrary resources are just Resource, right?
Which are built using Resource(realm, id).
So the id of an arbitrary resource r is r.id. For being truly general,
you have to take the eventual .parent resources (and .parent chain
actually), see the tracopt/perm/authz_policy.py module for an example.

> I'd be glad to be corrected, if I've overlooked something, but there
> seems no way to be sure today. While initially it seems common to have
> resource.name (i.e. resource = ticket)

That would be ... surprising, to say the least, knowing that we have
__slots__ = ('realm', 'id', 'version', 'parent') for Resource instances...

Hope this helps,

-- Christian

Reply all

Reply to author

Forward

0 new messages