# Ticket #3864 (new enhancement)

Opened 5 years ago

## Unchecked permissions/restrictions for list of available tag realms

Reported by: Assigned to: anonymous hasienda normal TagsPlugin major permission realm rjollos, otaku42, jun66j5 0.11

### Description

Sequence: - remove the trac-user-group anonymous - remove the trac-user-group authenticated - add new user with wiki_view and tag_view permissions, only

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

## Change History

### 10/06/08 23:08:29 changed by anonymous

sample screenshot

### 08/04/09 02:11:21 changed by rjollos

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

### 03/05/10 02:34:49 changed by AdrianFritz

• owner changed from athomas to otaku42.
• severity changed from normal to major.

### (in reply to: ↑ description ) 10/16/11 21:49:15 changed by hasienda

• keywords set to permission realm.
• owner changed from otaku42 to hasienda.
• type changed from defect to enhancement.
• summary changed from Unchecked Permissions/Restrictions to Unchecked permissions/restrictions for list of available tag realms.

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.

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.

### (follow-up: ↓ 5 ) 10/16/11 23:02:33 changed by 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().

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.

### (in reply to: ↑ 4 ; follow-up: ↓ 6 ) 10/18/11 11:56:19 changed by osimons

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

### (in reply to: ↑ 5 ) 10/19/11 01:29:32 changed by hasienda

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?

### (follow-up: ↓ 10 ) 10/19/11 02:08:50 changed by osimons

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.

### 10/19/11 02:50:15 changed by hasienda

• cc set to rjollos, otaku42, jun66j5.

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.

### 10/19/11 03:19:26 changed by hasienda

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

### (in reply to: ↑ 7 ) 10/19/11 03:23:35 changed by hasienda

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.

### 10/19/11 21:05:37 changed by hasienda

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.

### 10/19/11 21:06:00 changed by hasienda

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

### 10/14/12 14:26:24 changed by hasienda

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

### 10/14/12 14:50:51 changed by hasienda

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

### 10/16/12 00:31:06 changed by rjollos

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.

### Add/Change #3864 (Unchecked permissions/restrictions for list of available tag realms)

Change Properties