"Browse Source" button is not shown for anonymous users when AuthzSourcePolicy is used

[edgewall.p3u@…]

After upgrading from 0.11 to 0.12 and implementing the new ​AuthzSourcePolicy I found that anonymous users won't see the "Browse Source" button despite of having access to the source tree. For authenticated users this works fine.

See also ​discussion on trac-dev group

[Remy Blank]

I can reproduce the issue here. This happens when anonymous has access to a sub-directory of a repository, but not to the root. But that's because there's actually nothing to show. We should probably allow access to the parent directories of any items for which the user has permission.

[Remy Blank]

Updated source permission policy.

[Remy Blank]

9542-source-perms-r9987.patch​ fixes a few things with the source permission policy:

• Grant access to all parent directories of an allowed resource (this is the actual fix for this issue).
• When permission is neither granted nor revoked, the permission policy now returns None, so that the permission check continues with the following policies.
• When a repository cannot be shown (e.g. due to missing bindings), also apply the permission checks, and don't display the error if the user doesn't have access to the repository.
• While I was at it, I added support for the $anonymous and $authenticated tokens (#8289).

With this patch, the global BROWSER_VIEW, CHANGESET_VIEW and FILE_VIEW permission should be removed, and only LOG_VIEW should be granted globally.

And actually, I'm not sure about that last point. Shouldn't LOG_VIEW also be treated the same as FILE_VIEW and BROWSER_VIEW? The only locations where it is used is in the log view, where the permission check is coarse, and in svn_prop.py, where the resource checked against is the path.

[Remy Blank]

Also handle LOG_VIEW. Applies on top of 9542-source-perms-r9987.patch​.

[Remy Blank]

Yes, I'm pretty sure we should also handle LOG_VIEW in the same way. 9542-handle-log-r9987.patch​ enables that.

Could you please test if both patches fix the issue for you? You should be able to disable BROWSER_VIEW, CHANGESET_VIEW, FILE_VIEW and LOG_VIEW, as documented.

[edgewall.p3u@…]

rblank,

unfortunately I can not apply those patches as they don't seem to be against the 0.12 release version but some other dev-version. Could you provided patched versions of those files so I can test them?

[Remy Blank]

Patches applied in [10006]. More fixes coming shortly in #9566.

[HeX <edgewall.p3u@…>]

Rblank,

Since I couldn't test it I have to rely on the documentation. In [10006] you write:

• Allow access to all parent directories of allowed paths.

Does this mean that anonymous users would be able to see the other projects in the root folder even those they don't have access to? This is something I most definetly don't want to happen. In our usage case we have often project folders in the SVN root that should be future features but should stay hidden from anonymous users. Perhaps there is a better way to show the browser button to anonymous on the one hand but hiding the read protected root subfolders from view on the other.

If however this is actually already the case after [10006] then just ignore my comment here ;)

[Remy Blank]

Replying to HeX <edgewall.p3u@…>:

Does this mean that anonymous users would be able to see the other projects in the root folder even those they don't have access to?

No, it will allow anonymous users to list e.g. /projects, but the listing will only include entries for which anonymous has access, so if anonymous has access to /projects/public but not to /projects/private, the listing of /projects will only show public.

[HeX <edgewall.p3u@…>]

OK that's exactly like I would have liked it to be. Thanks again.

[HeX <edgewall.p3u@…>]

Sorry, but [10006] still doesn't fix the problem with the "Browse Source" button not shown for anonymous users. I'm now running Trac 0.12.1dev-r10006 where this should be solved :(

[HeX <edgewall.p3u@…>]

Replying to rblank:

No, it will allow anonymous users to list e.g. /projects, but the listing will only include entries for which anonymous has access, so if anonymous has access to /projects/public but not to /projects/private, the listing of /projects will only show public.

I just like to mention that your fix actually makes the listing if project folders superior to svn's own listing. If SVN would behave like the Trac Browser does now I would be able to give:

[/]
* = r

and wouldn't need to worry about private project folder names being visible (even if not accessible).

So thanks again for that enhancement :-) (only the Browser button issue left though ;-) )

[Remy Blank]

Replying to HeX <edgewall.p3u@…>:

Sorry, but [10006] still doesn't fix the problem with the "Browse Source" button not shown for anonymous users. I'm now running Trac 0.12.1dev-r10006 where this should be solved :(

I will need more information to fix this, because it works fine here. Would it be possible to attach your authz file here, or send it to me by private e-mail (remy.blank - at - pobox.com)?

[Remy Blank]

Thanks for the file. The reason why the "Browse Source" tab is hidden is the following entry:

[projects:/]
* =

With this rule, you explicitly prevent everyone (including anonymous) who isn't listed explicitly in the rest of the section from listing the root of the "projects" repository.

AFAICT, this rule is there to set a sane default for the whole repository (deny by default), and that makes sense. But it highlights a weakness of the authz format: a rule on a directory serves as both the list permission of the directory, and as a default to be inherited by sub-directories and files. So if you want to allow listing a directory, but restrict sub-directories, you have to set rules on every sub-directory:

[projects:/]
* = r

[projects:/dir1]
* = 

[projects:/dir2]
* = 

Now, I have to admit that I have implemented AuthzSourcePolicy from what I understood the authz system in Subversion should do, and haven't studied the Subversion code itself in detail to make our functionality absolutely equivalent. And from comment:11, our implementation may even be more useful. But I realize that being "not strictly equivalent" may be surprising, if not downright bad.

I need to have a serious look at the implementation in Subversion, and try an equivalent implementation. My guess is that it's going to have the serious drawback of not allowing to browse e.g. the root of a repository, even if I have access to sub-directories, therefore leaving a serious usability issue (it's the original issue of this ticket, actually).

Maybe we should treat BROWSER_VIEW and FILE_VIEW differently? For example, if a directory has an explicit "deny" rule, but a sub-directory or file is allowed, then allow BROWSER_VIEW anyway? That may be difficult to implement, at first sight.

Opinions?

[HeX <edgewall.p3u@…>]

Remy,

unfortunately the way subversion currently handles fine graines authentication a setup like:

[projects:/]
* = r

[projects:/dir1]
* = 

[projects:/dir2]
* = 

does not work since subversion will list dir1 and dir2 even when one can not enter them. So as I said the way you implemented is in my opinion far better than what subversion currently does. But I see how a divergence in the interpretation of svn_authz configuration between Trac ans Subversion might confuse users.

Another aspect which I just recall is that with the setting of:

[projects:/]
* =

in 0.11 it was not possible for anonymous users to follow autogenerated changeset links since they were always linking to the root (which had * = configured). Now with your patched behaviour r10006 this is no longer an issue and should stay that way.

So to summarise, from the usability view the way r10006 handles access rights is much more useful than it was before and I would keep this behaviour. The thing to fix (and which is also the topic of this ticket) would be to activate the "Browse Source" button if the relevant. But if there is any way to simply make Trac check for the browser.href variable and if the user has read-permission there then this could be used as a basis to show the "Browse Source" button.

_{BTW, I think part of ​http://trac.edgewall.org/wiki/TracNavigation should be merged into the ​http://trac.edgewall.org/wiki/TracIni documentation. Currently information on section definition [mainnav] is missing from there altogether.}

[Remy Blank]

Replying to HeX <edgewall.p3u@…>:

The thing to fix (and which is also the topic of this ticket) would be to activate the "Browse Source" button if the relevant.

Unfortunately, that won't work. If you have this entry:

[projects:/]
* =

and we somehow enable the "Browse Source" button, it will only lead to an empty page, as you explicitly disallow listing that directory.

As long as you need such an entry for Subversion not to list disallowed sub-directories, I'm afraid there's not much we can do to improve the situation.

I need fresh ideas…

[HeX <edgewall.p3u@…>]

Replying to rblank:

Unfortunately, that won't work. If you have this entry:

[projects:/]
* =

and we somehow enable the "Browse Source" button, it will only lead to an empty page, as you explicitly disallow listing that directory.

Actually this would be fine since I can configure (and have currently done so):

[mainnav]
browser.href = /browser/PublicProject

That way when the "Browse Source" button is shown it will point to the public sub dir rather than the prohibited root.

What do you think?

[Remy Blank]

Replying to HeX <edgewall.p3u@…>:

Actually this would be fine since I can configure (and have currently done so):

[mainnav]
browser.href = /browser/PublicProject

I wondered if you would do that, as you mentioned browser.href in comment:14. Fair enough, I could make the condition for showing the "Browse Source" button that both of the following are fulfilled:

• There is at least one repository defined.
• The user has a coarse BROWSER_VIEW permission, meaning that she has access to at least one sub-directory of at least one repository.

[HeX <edgewall.p3u@…>]

Replying to rblank:

I wondered if you would do that, as you mentioned browser.href in comment:14. Fair enough, I could make the condition for showing the "Browse Source" button that both of the following are fulfilled:

• There is at least one repository defined.
• The user has a coarse BROWSER_VIEW permission, meaning that she has access to at least one sub-directory of at least one repository.

Sounds good to me. I'm looking forward to test this.

/HeX

[Remy Blank]

More lax permissions for displaying the "Browse Source" tab.

[Remy Blank]

9542-lax-browser-r10010.patch​ should do the trick.

[HeX <edgewall.p3u@…>]

Sorry but the patch will actually remove BROWSER_VIEW, CHANGESET_VIEW, and LOG_VIEW from the list of available permission options. Hence non of those can be assigned to anonymous any longer.

[Remy Blank]

Replying to HeX <edgewall.p3u@…>:

Sorry but the patch will actually remove BROWSER_VIEW, CHANGESET_VIEW, and LOG_VIEW from the list of available permission options. Hence non of those can be assigned to anonymous any longer.

This must be unrelated. The patch only changes the condition under which the "Browse Source" tab is displayed.

And you shouldn't assign any of these permissions to anonymous globally. If anonymous has at least one entry in the authz file, they will be given automatically.

[HeX <edgewall.p3u@…>]

OK my bad (of course ;). I applied the patch with one indentation space too many. Therefore browser.py was not loaded at all. So I can confirm the patch works as you said and once applied would fix this ticket.

And you shouldn't assign any of these permissions to anonymous globally.

Yeah I got confused by comment:17 , second bullet point which of course is only meant for setups without finedgrained access control.

I'm happy that this seems to be finally resolved (once applied). Thanks, Hex.

[Remy Blank]

Patch applied in [10014].

[osimons]

No, this is not quite correct. I find that the navitem is nicely hidden when I'm browsing other modules, but if I (as someone without any permissions to source) then make a request to /browser the request is handled without error and 'Browse Source' appears in the menu again for this request. A nice page without other content than 'View Changes…' button and Help text+link.

The debug log says No policy allowed simon performing BROWSER_VIEW on <Resource u'repository, source:/'> so in that case the request handler really should raise a PermissionError and present an regular error page.

Just raising an error does not seems to make it all right though, as the source navigation item also displays when rendering Trac Error pages… Like when I make a request to a non-existing handler (404).

[Remy Blank]

I'm not sure I understand what you suggest. Is it correct that you would like to have the following:

• When the "Browse Source" button is hidden, the repository index page should generate a 404.
• The "Browse Source" is currently shown on error pages (I hadn't noticed that), and you would like it to disappear there, too.

The first item shouldn't be difficult, as we can use the same condition as for the navitem to generate a PermissionError for the repository index.

I'm not sure what causes the second issue, and I'll have a look.

[Remy Blank]

With [10036], a user who hasn't BROWSER_VIEW permission gets a permission error in the source browser. Also, a ResourceNotFound error is displayed for any path that is not in one of the defined repositories, or for the repository index if no repositories are defined.

I couldn't reproduce the issue with the error pages: if I don't have BROWSER_VIEW permission, the "Browse Source" tab is always hidden, even on error pages.

[Remy Blank]

Ok, now the situation is clear. If an authz file is used across several Trac instances, the modules of other instances carry over for the coarse permission checks. The following patch should fix the issue (but the unit tests don't pass yet).

trac/versioncontrol/svn_authz.py

@@ -216 +216 @@
             self.log.info('Parsing authz file: %s' % self.authz_file)
             try:
                 self._authz = parse(read_file(self.authz_file))
+                rm = RepositoryManager(self.env)
+                modules = set(repos.reponame
+                              for repos in rm.get_real_repositories())
+                modules.add('')
                 self._users = set(user for module in self._authz.itervalues()
+                                  if module in modules
                                   for path in module.itervalues()
                                   for user, result in path.iteritems()
                                   if result)

[Remy Blank]

Oh, stupid error. Here's the correct patch, with unit tests passing:

trac/versioncontrol/svn_authz.py

@@ -216 +216 @@
             self.log.info('Parsing authz file: %s' % self.authz_file)
             try:
                 self._authz = parse(read_file(self.authz_file))
-                self._users = set(user for module in self._authz.itervalues()
-                                  for path in module.itervalues()
+                rm = RepositoryManager(self.env)
+                modules = set(repos.reponame
+                              for repos in rm.get_real_repositories())
+                modules.add('')
+                self._users = set(user
+                                  for module, paths in self._authz.iteritems()
+                                  if module in modules
+                                  for path in paths.itervalues()
                                   for user, result in path.iteritems()
                                   if result)
             except Exception, e:

trac/versioncontrol/tests/svn_authz.py

@@ -201 +201 @@
         rm = RepositoryManager(self.env)
         
         class TestRepositoryManager(rm.__class__):
+            def get_real_repositories(self):
+                return set([Mock(reponame='module')])
+
             def get_repository(self, reponame):
                 if reponame == 'scoped':
                     def get_changeset(rev):
@@ -260 +263 @@
 [/somepath]
 joe = r
 denied =
-[/otherpath]
+[module:/otherpath]
 jane = r
 $anonymous = r
+[inactive:/not-in-this-instance]
+unknown = r
 """)
         self.assertPathPerm(None, 'unknown')
         self.assertRevPerm(None, 'unknown')

[Remy Blank]

Patch confirmed, applied in [10037].

[Remy Blank]

Fixed coarse permission checks when [trac] authz_module_name is defined in [10038].

[osimons]

Works as expected. Thanks a lot!