Modify

Opened 6 years ago

Closed 9 months ago

#3864 closed defect (fixed)

Unchecked permissions/restrictions for list of available tag realms

Reported by: anonymous Owned by: hasienda
Priority: normal Component: TagsPlugin
Severity: major Keywords: permission realm
Cc: rjollos, otaku42, jun66j5 Trac Release: 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

additional info: please take care of possible restrictions

Attachments (2)

tags_permissions.jpg (13.2 KB) - added by anonymous 6 years ago.
sample screenshot
FilterTags.png (7.7 KB) - added by rjollos 2 years ago.

Download all attachments as: .zip

Change History (19)

Changed 6 years ago by anonymous

sample screenshot

comment:1 Changed 5 years ago by rjollos

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

comment:2 Changed 5 years ago by AdrianFritz

  • Owner changed from athomas to otaku42
  • Severity changed from normal to major

comment:3 in reply to: ↑ description Changed 3 years ago by hasienda

  • Keywords permission realm added
  • Owner changed from otaku42 to hasienda
  • Summary changed from Unchecked Permissions/Restrictions to Unchecked permissions/restrictions for list of available tag realms
  • Type changed from defect to enhancement

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.

comment:4 follow-up: Changed 3 years ago 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.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 3 years ago by osimons

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.

comment:6 in reply to: ↑ 5 Changed 3 years ago by hasienda

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?

comment:7 follow-up: Changed 3 years ago 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.

comment:8 Changed 3 years ago by hasienda

  • Cc rjollos otaku42 jun66j5 added; anonymous removed

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.

comment:9 Changed 3 years ago 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.

comment:10 in reply to: ↑ 7 Changed 3 years ago by hasienda

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.

comment:11 Changed 3 years ago 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.

comment:12 Changed 3 years ago by hasienda

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

comment:13 Changed 2 years ago 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.

comment:14 Changed 2 years ago by hasienda

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

Changed 2 years ago by rjollos

comment:15 Changed 2 years ago 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.

comment:16 Changed 10 months ago by hasienda

  • Type changed from enhancement to defect

comment:17 Changed 9 months ago by hasienda

  • Resolution set to fixed
  • Status changed from new to closed

In 13815:

TagsPlugin: Completing preparation for v0.7 release.

Availability of that code as stable, tagged release
closes #2429, #3359, #3610, #3624, #3677, #3754, #3864, #3947, #3983, #4078, #4277, #4503, #4799, #5523, #7787, #7857, #8638, #9057, #9058, #9059, #9060, #9061, #9062, #9063, #9149, #9210, #9521, #9630, #9636, #10032, #10416, #10636, #11096, #11147, #11152, #11274, #11302, #11658 and #11659.

Additionally there are some issues and enhancement requests showing progress,
but known to require more work to resolve them satisfactorily, specifically
refs #2804, #4200, #8747 and #9064.

Thanks to all contributors and followers, that enabled and encouraged a good
portion of this development work.

Add Comment

Modify Ticket

Action
as closed The owner will remain hasienda.
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.