Incorrect error message when BROWSER_VIEW permission revoked via TracFineGrainedPermissions

[Ryan J Ollos]

I noticed while evaluating the changes in #11646 that the error message is different when the user lack BROWSER_VIEW vs when the permission is revoked using TracFineGrainedPermissions.

When the user lacks BROWSER_VIEW:

When BROWSER_VIEW is revoked through the following fine-grained permissions policy:

[repository:*@*]
* = !BROWSER_VIEW

[Jun Omae]

The repository browser cannot determine whether BROWSER_VIEW is revoked from all repositories or one. Currently, if the policy is used, it behaves as well with no viewable repositories. e.g. repositories are not defined or hidden.

However, "No node /" message doesn't seem to be user-friendly. How about showing "No viewable repositories" in that case? See jomae.git@t11650.

[Ryan J Ollos]

The issue in #11662 was discovered while testing the changes in this ticket. It's not directly related.

[Ryan J Ollos]

The changes look good.

I stumbled on an issue not directly related. When viewing SVN repository repos1 at path /repos1 and not having the required permission, the message is:

BROWSER_VIEW privileges are required to perform this operation on path @48 in repos1. You don't have the required permissions.

When viewing Git repository repos2 at path /repos2, the message is:

BROWSER_VIEW privileges are required to perform this operation on path /@a42350267ec907fbca96b2a2c25db1910e301715 in repos2. You don't have the required permissions.

For the first message, /@48 should be displayed rather than @48. I propose a change in log:rjollos.git:t11650.2.

[Jun Omae]

Lookd good to me. I think MercurialRepository also has the same issue.

[Ryan J Ollos]

Thanks for reviewing. I'll take a look at MercurialPlugin as well.

[Ryan J Ollos]

Replying to rjollos:

For the first message, /@48 should be displayed rather than @48. I propose a change in log:rjollos.git:t11650.2.

Change committed to 1.0-stable in [12947], merged to trunk in [12948].

The issue would be fixed for MercurialPlugin by:

tracext/hg/backend.py

@@ -588 +588 @@
         self.repo = None
 
     def normalize_path(self, path):
-        """Remove leading "/" (even at root)"""
-        return path and path.strip('/') or ''
+        """Remove leading "/", except at root."""
+        return path and path.strip('/') or '/'
 
     def normalize_rev(self, rev):
         """Return the full hash for the specified rev."""

The change would make normalize_path for MercurialPlugin to be the same as normalize_path for Git and Subversion. However, I'm concerned that this might break something since the comment says the behavior is intentional. I haven't found any issues with the change yet though.

[Jun Omae]

I've rebased the proposed changes, jomae.git@t11650.3. I'll commit it tonight.

[Jun Omae]

The jomae.git@t11650.3 has been committed in [13010] and merged to trunk in [13011].

[Christian Boos]

Replying to rjollos:

Replying to rjollos:

For the first message, /@48 should be displayed rather than @48. I propose a change in log:rjollos.git:t11650.2.

Change committed to 1.0-stable in [12947], merged to trunk in [12948].

The issue would be fixed for MercurialPlugin by: […]

The change would make normalize_path for MercurialPlugin to be the same as normalize_path for Git and Subversion. However, I'm concerned that this might break something since the comment says the behavior is intentional. I haven't found any issues with the change yet though.

I think the change is safe, committed as [5003d900/mercurial-plugin].

[Ryan J Ollos]

With a single default repository defined either through the repositories admin page or through trac.ini, No viewable repositories is displayed when navigating to /browser.

[repositories]
.dir = /home/user/Workspace/tracdev/repos/tracdev
.type = sv

If the repository is given a name, then the repository index is shown:

[repositories]
trac.dir = /home/user/Workspace/tracdev/repos/tracdev
trac.type = svn

I haven't yet tested the behavior prior to the changes in this ticket, but I suspect we may have a regression.

[Ryan J Ollos]

I apologize. comment:7 was due to an AuthzPolicy that I didn't realize was active. Everything is working correctly!