Edgewall Software
Modify

Opened 11 years ago

Closed 11 years ago

#7655 closed defect (fixed)

When setting trac permissions, these should also be enforced by the search system

Reported by: anonymous Owned by: Remy Blank
Priority: normal Milestone: 0.11.2
Component: search system Version: 0.11-stable
Severity: normal Keywords:
Cc: Branch:
Release Notes:
API Changes:

Description (last modified by Christian Boos)

If you for example disable ticket views for the anonymous user, then that user should also be not able to use the quick link search for arbitrary ticket numbers. The search request should be filtered so that directed searches against arbitrary ticket numbers must yield a zero result set in case of the user having no TICKET_VIEW permission

As of now, the queried for ticket will be displayed in the search result list, regardless of whether the user has the appropriate TiCKET_VIEW permission or not.

This seems to be an issue with all existing trac releases out there.

Attachments (3)

7655-permission-checks-r7595.patch (8.2 KB ) - added by Remy Blank 11 years ago.
Patch against 0.11-stable adding permission checks to all relevant IWikiSyntaxProviders
7655-permission-checks-2-r7595.patch (14.2 KB ) - added by Remy Blank 11 years ago.
Updated patch fixing most test cases
7655-permission-checks-3-r7595.patch (14.3 KB ) - added by Remy Blank 11 years ago.
Updated patch

Download all attachments as: .zip

Change History (15)

comment:1 by carsten.klein@…, 11 years ago

the initial ticket was created by me, forgot to set the email address

comment:2 by carsten.klein@…, 11 years ago

the same also applies to either search queries manually entered via the search form or via entering the url parameters into the address bar of the user agent, e.g.

?q=trac+link&ticket=on

PS: the same also applies to changesets as well, or searching for repository paths etc.

comment:3 by Christian Boos, 11 years ago

Description: modified (diff)
Milestone: not applicable0.11.3
Owner: set to Christian Boos
Type: enhancementdefect
Version: 0.11-stable

In the case of tickets:

  • trac/ticket/api.py

    diff -r 5359af881de9 trac/ticket/api.py
    a b  
    318318                num = r.a
    319319                ticket = formatter.resource('ticket', num)
    320320                from trac.ticket.model import Ticket
    321                 if Ticket.id_is_valid(num):
     321                if Ticket.id_is_valid(num) and \
     322                        'TICKET_VIEW' in formatter.perm(ticket):
    322323                    # TODO: watch #6436 and when done, attempt to retrieve
    323324                    #       ticket directly (try: Ticket(self.env, num) ...)
    324325                    cursor = formatter.db.cursor()

Similar checks should be done in other WikiSyntaxProviders.

by Remy Blank, 11 years ago

Patch against 0.11-stable adding permission checks to all relevant IWikiSyntaxProviders

comment:4 by Remy Blank, 11 years ago

Owner: changed from Christian Boos to Remy Blank

The patch above adds similar permission checks to all relevant IWikiSyntaxProviders. A few comments:

  • Permission checks are not strictly correct for resources in the source realm. To be correct, they would have to check whether the resource is a directory or a file, and use BROWSER_VIEW or FILE_VIEW accordingly. Currently, the resources are assumed to be files. I'm not sure this is worth fixing.
  • The patch also removes the href= attribute and adds the missing CSS class to links that are known not to exist (tickets, milestones, changesets). That way, they are not clickable anymore, but are still rendered in grey. This is cleaner IMO, as the links would have led to an error message anyway.

by Remy Blank, 11 years ago

Updated patch fixing most test cases

comment:5 by Remy Blank, 11 years ago

The changes to the "broken" link representation obviously broke many test cases. This updated patch fixes all except two:

  • There is a test case for a link like milestone:?action=new which I suppose should point to the "new milestone" page. If links to non-existing resources are not clickable anymore, this functionality disappears. Should we keep this, or is it an obscure, undocumented feature that can be dropped?
  • Another test case has a link like diff:// which is now rendered as "missing". What is this link supposed to mean, and should its current meaning be kept?

As usual, feedback and testing welcome.

in reply to:  4 ; comment:6 by Christian Boos, 11 years ago

Replying to rblank:

  • The patch also removes the href= attribute and adds the missing CSS class to links that are known not to exist (tickets, milestones, changesets). That way, they are not clickable anymore, but are still rendered in grey. This is cleaner IMO, as the links would have led to an error message anyway.

A few days ago, I made a patch for creating milestones by name. I find that useful when you create "sub-milestones" in a milestone description page. Could we keep the href for non-existing milestones?

Otherwise the patch looks good (not tested yet).

in reply to:  4 comment:7 by Christian Boos, 11 years ago

Also:

Replying to rblank:

  • Permission checks are not strictly correct for resources in the source realm. To be correct, they would have to check whether the resource is a directory or a file, and use BROWSER_VIEW or FILE_VIEW accordingly. Currently, the resources are assumed to be files. I'm not sure this is worth fixing.

No, I don't think it's worth fixing. The permission names BROWSER_VIEW and FILE_VIEW are coarse grained permission names, and those permission names are usually redundant with the realm information already present in resources when checking fine-grained permissions. Here, it's even worse, it's potentially a bad match as you said.

What would be really needed here is to adopt simpler permission names for fine-grained permission checks (i.e. if 'view' in req.perm('source', path, rev)).

in reply to:  6 ; comment:8 by Remy Blank, 11 years ago

Replying to cboos:

A few days ago, I made a patch for creating milestones by name. I find that useful when you create "sub-milestones" in a milestone description page. Could we keep the href for non-existing milestones?

Sure. How about keeping href= only when the milestone does not exist and the user has MILESTONE_CREATE?

The permission names BROWSER_VIEW and FILE_VIEW are coarse grained permission names, and those permission names are usually redundant with the realm information already present in resources when checking fine-grained permissions. Here, it's even worse, it's potentially a bad match as you said.

Is FILE_VIEW really coarse-grained? I thought constructs like

'FILE_VIEW' in req.perm('source', path, rev)

indicated fine-grained permissions. I'll have to read more about permissions.

Should I drop the potentially wrong checks for FILE_VIEW altogether?

in reply to:  8 comment:9 by Christian Boos, 11 years ago

Replying to rblank:

How about keeping href= only when the milestone does not exist and the user has MILESTONE_CREATE?

Yes, even better (btw, we should probably do that for wiki links as well).

The permission names BROWSER_VIEW and FILE_VIEW are coarse grained permission names, and those permission names are usually redundant with the realm information already present in resources when checking fine-grained permissions. Here, it's even worse, it's potentially a bad match as you said.

Is FILE_VIEW really coarse-grained?

Well, I meant that this name is coming from the days we only had coarse grained permissions, like any other permission name. My point was that usually the only annoying thing with such names is a bit of redundancy (e.g. if WIKI_VIEW in req.perm('wiki', ...)), but in this case there's even possible mismatch (BROWSER_ vs. FILE_ for a source realm which can be either a directory or a file), as you pointed out.

I thought constructs like

'FILE_VIEW' in req.perm('source', path, rev)

indicated fine-grained permissions.

Right, that's a fine grained check and 'FILE_VIEW' in req.perm is a coarse grained one. My take was that we'd better replace that in the future by 'view' in req.perm('source', path, rev) for fine grained checks (because that would be correct regardless of what path is) and by 'view' in req.perm('source') for coarse grained permission if ever needed.

I'll have to read more about permissions.

Should I drop the potentially wrong checks for FILE_VIEW altogether?

Well, in 0.11-stable and trunk we don't use fine grained permission checks in the versioncontrol/web_ui modules anyway. I think I've added the checks in the multirepos branch but didn't do much testing about them. In any case, the adaptation of the RealSubversionAuthorizer to the new security model remains to be done.

by Remy Blank, 11 years ago

Updated patch

comment:10 by Remy Blank, 11 years ago

The update above integrates the considerations of the preceding comments, and should be ready for applying:

  • It solves the initial problem for attachments, tickets, changesets and changlogs, by de-linkifying trac links to these resources if the user doesn't have permission to view them.
  • It also solves the problem for milestones and wiki pages. However, for these realms, trac links to non-existing resources are kept as links to allow for easy creation of a new resource (already implemented for wiki pages, to be done shortly for milestones), provided that the user has *_CREATE permission.
  • It does not solve the problem for other links in the source realm (file browser, file view, diffs), as this would require checking the type of resources (file or directory) before checking permissions, and this would degrade performance. To avoid this, a type-independent VIEW permission should be created, as described in comment:7 and comment:9.

All tests pass, so if there are no objections, I'll merge this patch soon.

comment:11 by Christian Boos, 11 years ago

Milestone: 0.11-retriage0.11.2

Soon as "for 0.11.2"? Fine by me.

comment:12 by Remy Blank, 11 years ago

Resolution: fixed
Status: newclosed

Patch applied to 0.11-stable in [7604] and merged to trunk in [7605].

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Remy Blank to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.