SVN checkin comments float up past svn:authz barrier.

[Dave Abrahams <dave@…>]

Our SVN repo contains private client areas. With Trac and SVN logins using the same password file, it's awesome that the Trac browser won't show the private areas to those people who don't have the privileges to look at them. However, unfortunately, when browsing a public parent directory of a private area, one can always see the last checkin comment in its subtree, even if the changes were entirely in a subtree in which one doesn't have read privileges.

[Emmanuel Blot]

(please stop assigning new ticket to 0.11)

[anonymous]

revoking logview priviledge from the anonymous user is a workaround, but comes with the drawback that anonymous can't logview anything

this is *really* serious, after 18months nothing happend?

[ms@…]

poor-man's fix for this problem

[Remy Blank]

I can reproduce this. Even worse, the revision log exhibits the same symptom. I'll attach a patch shortly.

[Remy Blank]

Patch against 0.11-stable hiding changeset info for which the user doesn't have view permission

[Remy Blank]

The patch above hides the changeset date, author and message in the browser and revision log views when the user doesn't have permission to view the changeset (e.g. due to authz).

I'm pushing into unknown territory here, so a review (and some user testing) would be most welcome.

[Remy Blank]

I'd like this to be fixed in 0.11.3. Testing still welcome.

[Remy Blank]

Cleaner approach to hiding forbidden changeset data

[Remy Blank]

My first patch was the wrong approach. The second one should be better:

• The changeset date, author and message are hidden in the browser view when the user doesn't have permission to view the changeset.

• Forbidden changesets are removed entirely from the log (instead of just hiding the metadata).

Christian, I'd appreciate if you could have a look at this patch and let me know what you think.

[Christian Boos]

Yes, it looks fine, but I'd like to test it.

[Remy Blank]

I don't feel confident pushing this shortly before 0.11.3. I'd rather apply the fix right after 0.11.3 is out, so that it has at least a chance to get some serious testing.

[Remy Blank]

Updated patch fixing the "Next changeset" link.

[Remy Blank]

The updated patch above fixes the "Next changeset" link, which would previously link to unauthorized changesets and lead to a permission error. The "Previous changeset" was working well.

As authorization is quite critical, I'd appreciate if I could get a review of the patch before I apply it.

[Remy Blank]

Updated patch for current 0.11-stable head.

[Remy Blank]

The patch has been applied to 0.11-stable in [8184].

[osimons]

I've got one empty changeset in my repos - see #5097. It contains the log message and meta data, but no record of paths affected by change.

Following [8184] I'm no longer able to resync this repos. The CachedRepository.sync() method turns off all authz information, and is not prepared for SubversionRepository.get_changeset() evaluating rev == 0 or any(cset.get_changes()) to false and throwing NoSuchChangeset(rev).

[Remy Blank]

I didn't know you could have empty changesets. get_changeset() should obviously not throw NoSuchChangeset in that case. Thanks for testing!

[Remy Blank]

This issue is even trickier than I thought. I have backed out [8184] in [8198], and I'll post a better fix here, as a patch.

[Remy Blank]

Even another approach, with its own issues

[Remy Blank]

The patch above is a third variant that fixes this issue, as well as the new one reported in #8282. A few explanations:

• SubversionChangeset.get_changes() and CachedChangeset.get_changes() now raise a ChangesetDenied exception if a changeset is not empty, but no changes are authorized. This is used in RealSubversionAuthorizer.has_permission_for_changeset() to determine if a changeset can be accessed or not.

• The big change in SubversionRepository.sync() actually only moves the deactivation of the authorizer higher up in the function. Nothing else is changed there.

But it's still not complete. I get a "No changeset 0 in the repository" error when trying to view revisions 1 and 2 in my repository, which so far I could trace back to Node.get_previous() raising the exception.

[Remy Blank]

Improved patch handling revisions 1 and 2 correctly

[Remy Blank]

The patch above handles revisions 1 and 2 correctly. AFAICT, it should now be complete and fix this issue and #8282.

Simon, would it be possible for you to test this patch with your repository containing empty revisions?

[osimons]

Replying to rblank:

Simon, would it be possible for you to test this patch with your repository containing empty revisions?

Supert Remy, patch makes resync work as it should and it also displays the empty changeset without permission error. Haven't tested much else, but just noticed that when viewing my empty changeset 29 and hitting 'Next Changeset' I get 30. When viewing 30, 'Previous Changeset' will take me to 28 and not 29.

[Christian Boos]

I should probably have joined the discussion on [8184] earlier, but as this things turns out to be more complex than the initial patch, I'm a bit worried of the direction it takes now.

The authz stuff is still a remnant of the 0.10 way to implement TracFineGrainedPermissions, which closely coupled the SubversionRepository and the SubversionAuthorizer. The plan was to transform the SubversionAuthorizer into a real PermissionPolicy and move the permission checks at the generic versioncontrol level. But we missed the opportunity to do so for 0.11, and now the old code still sits there, with its bugs biting us back ;-)

Perhaps a way to implement that "late" in the 0.11 cycle would be to just leave the authz parameter and property in the Repository, but not use it. So we wouldn't need a new ChangesetDenied exception and we could do the checks at a higher level, using the normal fine grained permission mechanism?

[Remy Blank]

Replying to osimons:

just noticed that when viewing my empty changeset 29 and hitting 'Next Changeset' I get 30. When viewing 30, 'Previous Changeset' will take me to 28 and not 29.

That's weird, but not completely surprising. There's an asymmetry in the code between going back and going forward in the history. I will see if I can fix that, too (if we keep the code, see below).

Replying to cboos:

this things turns out to be more complex than the initial patch, I'm a bit worried of the direction it takes now.

Not quite. The number of changes between the last and second last patch is about the same. But yes, the code is quite complex and sometimes convoluted, so it does show a few surprises.

So we wouldn't need a new ChangesetDenied exception and we could do the checks at a higher level, using the normal fine grained permission mechanism?

Well, the ChangesetDenied exception is actually only a marker that allows differentiating between an empty changeset and a non-empty changeset where all changes are denied, in Changeset.get_changes(). It should never be visible to the user.

About migrating to the fine-grained permission mechanism, this is one of the areas in Trac I'm not yet familiar with, so I'm not sure how this would be done. But my feeling is that at least part of the last patch will be needed anyway, namely the missing permission checks in CachedRepository._next_prev_rev(), SubversionRepository._history() and get_changes() in web_ui/util.py. Or would all permission checks be removed from the repositories, and be moved to client code?

I'd be ok with dropping the fixes I propose here and going for the fine-grained permission mechanism (providing it solves this issue, obviously), but I would leave the implementation to someone else, as I would hate to mess up a security-sensitive area like this.

[Christian Boos]

Replying to rblank:

… my feeling is that at least part of the last patch will be needed anyway, namely the missing permission checks in CachedRepository._next_prev_rev(), SubversionRepository._history() and get_changes() in web_ui/util.py. Or would all permission checks be removed from the repositories, and be moved to client code?

I'd be ok with dropping the fixes I propose here and going for the fine-grained permission mechanism (providing it solves this issue, obviously), but I would leave the implementation to someone else, as I would hate to mess up a security-sensitive area like this.

Will be done in MultiRepos somewhat "soon".

[Remy Blank]

This is fixed in [9082].