Modify

Opened 16 years ago

Closed 11 years ago

#3864 closed defect (fixed)

Unchecked permissions/restrictions for list of available tag realms

Reported by: anonymous Owned by: Steffen Hoffmann
Priority: normal Component: TagsPlugin
Severity: major Keywords: permission realm
Cc: Ryan J Ollos, Michael Renzmann, Jun Omae 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 16 years ago.
sample screenshot
FilterTags.png (7.7 KB) - added by Ryan J Ollos 12 years ago.

Download all attachments as: .zip

Change History (19)

Changed 16 years ago by anonymous

Attachment: tags_permissions.jpg added

sample screenshot

comment:1 Changed 15 years ago by Ryan J Ollos

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

comment:2 Changed 15 years ago by Adrian Fritz

Owner: changed from Alec Thomas to Michael Renzmann
Severity: normalmajor

comment:3 in reply to:  description Changed 13 years ago by Steffen Hoffmann

Keywords: permission realm added
Owner: changed from Michael Renzmann to Steffen Hoffmann
Summary: Unchecked Permissions/RestrictionsUnchecked permissions/restrictions for list of available tag realms
Type: defectenhancement

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 Changed 13 years ago by Steffen Hoffmann

(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 ; Changed 13 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 13 years ago by Steffen Hoffmann

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 Changed 13 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 13 years ago by Steffen Hoffmann

Cc: Ryan J Ollos Michael Renzmann Jun Omae 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 13 years ago by Steffen Hoffmann

(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 13 years ago by Steffen Hoffmann

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 13 years ago by Steffen Hoffmann

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 13 years ago by Steffen Hoffmann

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

comment:13 Changed 12 years ago by Steffen Hoffmann

(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 12 years ago by Steffen Hoffmann

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

Changed 12 years ago by Ryan J Ollos

Attachment: FilterTags.png added

comment:15 Changed 12 years ago by Ryan J Ollos

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 11 years ago by Steffen Hoffmann

Type: enhancementdefect

comment:17 Changed 11 years ago by Steffen Hoffmann

Resolution: fixed
Status: newclosed

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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Steffen Hoffmann.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


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

 
Note: See TracTickets for help on using tickets.