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)
Change History (19)
Changed 16 years ago by
Attachment: | tags_permissions.jpg added |
---|
comment:1 Changed 15 years ago by
This is a big concern of mine. Thank you for describing and reporting it so well.
comment:2 Changed 15 years ago by
Owner: | changed from Alec Thomas to Michael Renzmann |
---|---|
Severity: | normal → major |
comment:3 Changed 13 years ago by
Keywords: | permission realm added |
---|---|
Owner: | changed from Michael Renzmann to Steffen Hoffmann |
Summary: | Unchecked Permissions/Restrictions → Unchecked permissions/restrictions for list of available tag realms |
Type: | defect → 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: 5 Changed 13 years ago by
(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 follow-up: 6 Changed 13 years ago by
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
onDefaultTagProvider
and change permission checking into calls toTicketTagProvider.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 Changed 13 years ago by
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: 10 Changed 13 years ago by
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
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
(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 Changed 13 years ago by
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 aComponent
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
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
(In [10800]) TagsPlugin: Add brackets missing from last changeset [10799], refs #3864.
comment:13 Changed 12 years ago by
(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
(In [12165]) TagsPlugin: Test for proper registration of standard ITagProvider
implementations, refs #3864.
Changed 12 years ago by
Attachment: | FilterTags.png added |
---|
comment:15 Changed 12 years ago by
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
Type: | enhancement → defect |
---|
sample screenshot