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

[anonymous]

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.

[carsten.klein@…]

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

[carsten.klein@…]

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.

[Christian Boos]

In the case of tickets:

trac/ticket/api.py

@@ -318 +318 @@
                 num = r.a
                 ticket = formatter.resource('ticket', num)
                 from trac.ticket.model import Ticket
-                if Ticket.id_is_valid(num):
+                if Ticket.id_is_valid(num) and \
+                        'TICKET_VIEW' in formatter.perm(ticket):
                     # TODO: watch #6436 and when done, attempt to retrieve
                     #       ticket directly (try: Ticket(self.env, num) ...)
                     cursor = formatter.db.cursor()

Similar checks should be done in other WikiSyntaxProviders.

[Remy Blank]

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

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

[Remy Blank]

Updated patch fixing most test cases

[Remy Blank]

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.

[Christian Boos]

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

[Christian Boos]

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

[Remy Blank]

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?

[Christian Boos]

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.

[Remy Blank]

Updated patch

[Remy Blank]

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.

[Christian Boos]

Soon as "for 0.11.2"? Fine by me.

[Remy Blank]

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