Edgewall Software

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#11650 closed defect (fixed)

Incorrect error message when BROWSER_VIEW permission revoked via TracFineGrainedPermissions — at Version 12

Reported by: Ryan J Ollos Owned by: Jun Omae
Priority: normal Milestone: 1.0.2
Component: version control/browser Version:
Severity: normal Keywords: authzpolicy
Cc: Jun Omae Branch:
Release Notes:

Improve error message when no viewable repositories and accessing repository index. Fixed display path in error message for Subversion and Mercurial repository when a PermissionError is raised accessing the repository root.

API Changes:
Internal Changes:

Description

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

Change History (14)

by Ryan J Ollos, 10 years ago

Attachment: BrowserViewRevoked.png added

by Ryan J Ollos, 10 years ago

Attachment: NoBrowserView.png added

comment:1 by Jun Omae, 10 years ago

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.

comment:2 by Ryan J Ollos, 10 years ago

Milestone: 1.0.2

comment:3 by Ryan J Ollos, 10 years ago

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

comment:4 by Ryan J Ollos, 10 years ago

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.

comment:5 by Jun Omae, 10 years ago

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

comment:6 by Ryan J Ollos, 10 years ago

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

in reply to:  4 ; comment:7 by Ryan J Ollos, 10 years ago

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

    a b  
    588588        self.repo = None
    589589
    590590    def normalize_path(self, path):
    591         """Remove leading "/" (even at root)"""
    592         return path and path.strip('/') or ''
     591        """Remove leading "/", except at root."""
     592        return path and path.strip('/') or '/'
    593593
    594594    def normalize_rev(self, rev):
    595595        """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.

comment:8 by Jun Omae, 10 years ago

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

comment:9 by Jun Omae, 10 years ago

Release Notes: modified (diff)

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

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

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

comment:11 by Ryan J Ollos, 10 years ago

Resolution: fixed
Status: newclosed

comment:12 by Ryan J Ollos, 10 years ago

Owner: set to Jun Omae
Release Notes: modified (diff)
Note: See TracTickets for help on using tickets.