Unchecked permissions/restrictions for list of available tag realms

sample screenshot

This is a big concern of mine. Thank you for describing and reporting it so well.

Replying to anonymous:

Sequence:

Done similarly here to reproduce.

Result: as soon as being logged in as the new user & viewing the tags-page, it is even possible to filter "hidden tags", such as eg tickets

Yes, you see the checkbox, but following your example a TagsQuery won't yield any result for the ticket realm, if TICKET_VIEW has not been granted to the active user, right? This is what I see here.

additional info: please take care of possible restrictions

And the message here is ...?

I do care, but (not) hiding the checkbox, if the user has no view-permission for the whole realm in question, is merely an enhancement. No information is disclosed apart from the fact, that a ticket system might be in use for this instance. Are you really that worried for just that?

I think this is largely a misunderstanding. Anyway I'll try to fix what I see as an improvement at least for the 'ticket' and 'wiki' realm. Please note, that there's no sane way to auto-detect the same for arbitrary realms without some guessing AFAIK.

(In [10789]) TagsPlugin: Hide tag realms the user has no view permission for from '/tags' page, refs #3864.

This required to rebase TicketTagProvider on DefaultTagProvider and change permission checking into calls to TicketTagProvider.check_permission().

Good I checked this, since TICKET_MODIFY isn't required to change keywords but only TICKET_CHGPROP is. Some cosmetic changes to slightly shorten code are included as well.

Replying to hasienda:

(In [10789]) TagsPlugin: Hide tag realms the user has no view permission for from '/tags' page, refs #3864. This required to rebase TicketTagProvider on DefaultTagProvider and change permission checking into calls to TicketTagProvider.check_permission().

The way this is implemented obviously breaks external providers, seeing you now require them to implement check_permission():

AttributeError: 'FullBlogTagSystem' object has no attribute 'check_permission'

If something is required, it should be part of the API.

Replying to osimons:

The way this is implemented obviously breaks external providers, seeing you now require them to implement check_permission(): [...] If something is required, it should be part of the API.

Sorry, I've overlooked that detail. Because it's next to impossible to deal with arbitrary permissions without the mapping provided by check_permission, it is beyond both, my knowledge and my imagination, why this method is part of DefaultTagProvider class, adapted to both default implementations ('ticket' and 'wiki'), but not part of the ITagProvider interface.

I see the API change/extension as inevitable to steer clear out of this situation. Or can you imaging a backwards-compatible way? Of course I could catch the AttributeError as I did for other API changes, but hiding checkboxes wouldn't work for all plugins, that don't implement check_permission, what leads to slightly inconsistent UI behavior, possibly even confusing users according to the complaint of this ticket.

So you feel this (added backwards-compatibility) should be done anyway, don't you?

I suppose the interface could be easily extended to provide what you need? The API could request the permission map (get_permission_map()) or you could add a check_resource_permission() that delegates this to the providers. Using get_permission_map() may be a useful idea, providing view and modify keys - optionally supporting value False if a feature is permanently disabled. Like if a provider don't support modify, it should use 'modify': False in the map. A related question may what should happen if a provider does not have any tags - should 'blog' be shown as a filter when there is nothing to be found? I don't know, I'll leave details to you :-)

Anyway, regardless of API you can test for hasattr to see if it exists, and if not just stick to non-optimal defaults. Like showing 'blog' even though user don't have the necessary permissions. This would then get fixed once external code gets updated to support the adjusted interface. Provide a graceful fallback.

BTW, Using check_permission() as name is not such a hot idea, seeing the general permission policy interface uses this method name for its API. A plugin should be able to implement both a security policy and a tags provider. Seeing how a Component can implement any desired interface, any API method name must really be unique across all Trac and plugin extension points.

Thanks for the instant feedback.

I agree with you or at least have no objections against any of these statements. I don't think, that such general changes should be done without peer review. So I'll prepare the necessary changes as patches for review.

Your advice and opinions are always appreciated. Thanks for your time and attention.

(In [10799]) TagsPlugin: Fix check based on method not required by current ITagProvider interface, refs #3864.

This is an immediate step to get interim functionality with implementatons of ITagProvider interface provided by other Trac plugins.

Replying to osimons:

Anyway, regardless of API you can test for hasattr to see if it exists, and if not just stick to non-optimal defaults. [...] Provide a graceful fallback.

Done.

BTW, Using check_permission() as name is not such a hot idea, seeing the general permission policy interface uses this method name for its API. [...] Seeing how a Component can implement any desired interface, any API method name must really be unique across all Trac and plugin extension points.

Thanks for pointing that out. This will be one of the next steps.

Checked once again today: Code doesn't even compile because I forgot two brackets. Meant to have tested this, but obviously not the code, that I committed. Must have been too late, sorry.

(In [10800]) TagsPlugin: Add brackets missing from last changeset [10799], refs #3864.

(In [12164]) TagsPlugin: Prevent provider duplication for TicketTagProvider, refs #3864.

This must have haunted TagsPlugin applications on Trac-0.11 since [10789], but without a bug report only attempts to add more unit tests revealed this flaw.

Turned out that in Trac-0.11 its possible to register a Trac Component twice for the same ExtensionPoint interface by class inheritance, where trac.core.implements is explicitely written in both, class and base class. My bad to change TicketTagProvider to inherit from DefaultTagProvider without actually checking for side-effects, especially against any Trac revision before Trac-0.13dev at that time.

(In [12165]) TagsPlugin: Test for proper registration of standard ITagProvider implementations, refs #3864.

I had wondered what the effect of [10789] would be, but after our discussion on IRC this evening and reproducing myself to gain understanding, I see one effect is very simple:

Mostly just interested to gain a better understanding for my own purposes, and to keep an eye out for similar issues in other plugins.