fine-grained permissions are not applied to directories when browsing sources

[esizikov@…]

When trying to develop a custom plugin which uses the shining new fine-grained permissions available in the svn trunk, I've stacked with directory listing not obey the developed plugin.

The problem is that fine-grained permissions are not applied to directories when browsing sources, but only to file nodes.

I've made a proof-of-concept patch for the required functionality, which acts like the existing one for files.

[Christian Boos]

Thanks for creating the ticket and starting work in this direction.

I'm not completely decided if this should go in milestone:0.11.1 or milestone:0.12 yet, but doing the conservative choice for now.

[esizikov@…]

Fixes: apply permission check to all nodes, not only folders as in the first version; catch PermissionError exception to allow other exceptions raising.

[Christian Boos]

Note that in the MultiRepos branch, there are some fine grained permission checks in the ChangesetModule.

Having changeset and source resources be child resources of a repository resource would allow to easily set permissions at the level of a whole repository (for example in the AuthzPolicyPlugin). The current "composite id" would make this difficult.

[esizikov@…]

The problem is that there is no way to deny access (in the Browse Source view) per a directory inside VCS with current code. I mean there's no call to AuthzPolicy plugins during the directory structure rendering.

Please see the provided patch​ - it is very short and easy to understand what I'm talking about.

[Christian Boos]

Replying to esizikov@…:

The problem is that there is no way to deny access (in the Browse Source view) per a directory inside VCS with current code. I mean there's no call to AuthzPolicy plugins during the directory structure rendering.

Sure, this needs to be fixed and is going to be a priority for 0.12, hopefully. See also SvnAuthz.

[Remy Blank]

Current state of AuthzPermissionPolicy.

[Remy Blank]

Christian, this is the current state of the AuthzPermissionPolicy, based on the patch you sent me. All tests pass, and I have checked the permissions through quite a few code paths. A few comments:

• The authz parser has been rewritten completely. It is now much simpler, and also more correct. Indeed, using the ConfigParser was not a good idea, as it doesn't preserve the order of entries, and the authz file is sensitive to the order within a section (see "​path-based authorization").

• The checks for source: resources are in AuthzPermissionPolicy, those for changeset: resources in ChangesetPermissionPolicy. The reason for doing so is that the latter is generic, and could be used with other source: policies as well. We could also make ChangesetPermissionPolicy a simple function, to be called by AuthzPermissionPolicy and possibly others.

• All unit tests have been fixed, and new ones have been added for AuthzPermissionPolicy.

• The permissions in the browser, changeset viewer and revision log have been largely fixed, but I'm sure I have missed a few locations.

• Metadata for denied changesets is not displayed anymore in the browser, changeset viewer and revision log (#5640).

• The changelog RSS and text formats now also have permission checks, where they were previously missing.

This ended up being a pretty large change, and a few things are still not clear to me (e.g. how to handle FILE_VIEW and CHANGESET_VIEW permission checks with no resource). So a thorough review would be highly appreciated.

[Remy Blank]

Added many more permission checks.

[Remy Blank]

The patch above should be reasonably complete. There is only a single permission policy left (AuthzSourcePolicy). Fine-grained permission checks have been added to all versioncontrol.web_ui modules, and existing coarse checks have been converted when appropriate.

One thing I'd like to change, though, is the handling of aliases. Currently, it is possible to specify different permissions for a real repository and for aliases to that repository. I don't think this makes sense, as one wants to protect the content of a repository, regardless of its name. So instead of using reponame for the repository resource, we should actually use repos.reponame.

This would also allow a simplification that I have already started in versioncontrol.api, where I attach a .resource attribute to Repository, Node and Changeset. Permission checks can then take a much simpler form of e.g. node.can_view(perm). The drawback is that the change requires breaking the signature of __init__() on Node and Changeset (to pass a reference to the repository). I think that's reasonable, considering that we already break it on Repository.

So if you agree with the above, I will make a last pass through the code and change the permission checks to the simpler for involving the .resource attributes.

(The patch has become huge, and unfortunately there are only very few unit tests involving permissions, so testing must be done manually. Maybe we should address that at some point.)

[Christian Boos]

Replying to rblank:

The patch above should be reasonably complete. There is only a single permission policy left (AuthzSourcePolicy). Fine-grained permission checks have been added to all versioncontrol.web_ui modules, and existing coarse checks have been converted when appropriate.

Great work Remy! We're really close now.

One thing I'd like to change, though, is the handling of aliases. Currently, it is possible to specify different permissions for a real repository and for aliases to that repository. I don't think this makes sense, as one wants to protect the content of a repository, regardless of its name. So instead of using reponame for the repository resource, we should actually use repos.reponame.

Agreed.

This would also allow a simplification that I have already started in versioncontrol.api, where I attach a .resource attribute to Repository, Node and Changeset.

You still have some places (e.g. _render_zip) where you pass repos_resource and repos, is that still needed? repos should then be enough as we can use repos.resource.

Permission checks can then take a much simpler form of e.g. node.can_view(perm). The drawback is that the change requires breaking the signature of __init__() on Node and Changeset (to pass a reference to the repository). I think that's reasonable, considering that we already break it on Repository.

That's OK, yes.

So if you agree with the above, I will make a last pass through the code and change the permission checks to the simpler for involving the .resource attributes.

Perfect.

(The patch has become huge, and unfortunately there are only very few unit tests involving permissions, so testing must be done manually. Maybe we should address that at some point.)

About huge patches: it would be nice to have an index over the files, much like we have in the changeset view, or collapsible .entry items, so that we can see all the files at a glance and pick the one we're interested in.

For the tests, let's try to get our testing task force involved ;-) (CC'ing them)

[Remy Blank]

Replying to cboos:

You still have some places (e.g. _render_zip) where you pass repos_resource and repos, is that still needed? repos should then be enough as we can use repos.resource.

No, that won't be needed anymore. The current state uses reponame for permission checks (i.e. the wrong name), and the changes in api.py were only to show what I meant. Now that I know you're ok with the change, I'll re-check all permission checks and use repos.resource.

Ok, a final pass tonight, and I should be ready for a final review.

[Christian Boos]

Replying to rblank:

a final pass tonight, and I should be ready for a final review.

No, no need, commit it as soon as you're done.

[Remy Blank]

Use resources associated with model objects for permission checks.

[Remy Blank]

The patch above now uses resources associated with Repository, Node and Changeset objects for permission checks where available.

However, as discussed on IRC, this results in a confusing situation for repository aliases, where two different Resource objects are used, one for permission checks, the other for the rendering context. One possible solution would be to make aliases more like "pointers", and always render with the context of the real repository (and generate redirects for URLs accessing alises, e.g. /changeset/123/alias → /changeset/123/repo).

[Christian Boos]

(last patch still applies cleanly on r9079)

Actually the redirect would be very nice to have in the migration scenario (from single repo to multirepo) where old TracLinks to the default repository would lead to URLs containing the new name in clear.

[Remy Blank]

Last patch re-re-reviewed, tested on Linux (SQLite), OS X (MySQL, PostgreSQL) and Windows 7 (SQLite), and finally applied in [9082]. Let the bug reports flow! :)

So this only leaves the question of the aliases. The more I think about it, the more I prefer the "pointer" approach, where only the real repositories actually exist:

• Aliases are only used to find the repository, but the real repository name is used for the rendering context and permission checks.

• When accessing a URL for an alias, generate a redirect to the corresponding URL for the real repository.

The advantages are: only one Resource object must be handled, and we don't have to keep track of reponame separately from repos. I'll try it out and report back with a patch.

In the meantime, how about starting to plan the merge? :)

[Christian Boos]

Replying to rblank:

In the meantime, how about starting to plan the merge? :)

Yep, let's do that there: MultipleRepositorySupport#Merge.

[Remy Blank]

Make aliases pointers to repositories.

[Remy Blank]

The patch above makes aliases pointers to real repositories, so e.g. a link like [123/repo-alias] will lead to /changeset/123/repo. Moreover, accesses to alias resources are redirected to the real resources, e.g. /browser/repo-alias/file is redirected to /browser/repo/file.

I need to review the whole thing one more time. In the meantime, feedback is very welcome.

[Christian Boos]

Most of the changes look like nice cleanups.

Two things though:

• when I saw the drastic simplification of the timeline repository filter code, this reminded me of the various use cases I tested, among them using aliases as a way to completely hide a "real" repo. That obviously won't work anymore. Neither is it really needed anymore because, as you said on #IRC, one could instead rename the real repository and have aliases for supporting the old TracLinks. This is easy to support now as we have surrogate ids for repositories, but I think we should also have a convenient way to do the rename (OK, we have it in the admin panel, but a trac-admin <env> repository rename oldname newname command would be nice as well)
• do we really need the req.redirect, or is it just cosmetic, to have URLs consistent with the displayed repository name?

[Remy Blank]

Replying to cboos:

I think we should also have a convenient way to do the rename (OK, we have it in the admin panel, but a trac-admin <env> repository rename oldname newname command would be nice as well)

You can already do trac-admin $ENV repository set <repos> name <new_name>, i.e. the name is just another attribute. I plan to add support for a repository description, as I just noticed that you had already planned to have it displayed in the repository index.

do we really need the req.redirect, or is it just cosmetic, to have URLs consistent with the displayed repository name?

It follows the idea that a resource should have a single URL to represent it, but I agree that it's largely cosmetic. Maybe I should change the redirects to permanent ones, as that's really the main use case for aliases. Or would you prefer that I remove the redirects?

[Christian Boos]

Replying to rblank:

Replying to cboos:

I think we should also have a convenient way to do the rename (OK, we have it in the admin panel, but a trac-admin <env> repository rename oldname newname command would be nice as well)

You can already do trac-admin $ENV repository set <repos> name <new_name>, i.e. the name is just another attribute.

Oh, of course.

I plan to add support for a repository description, as I just noticed that you had already planned to have it displayed in the repository index.

Nice! The .description attribute should be already working when set from the [repositories] in the .ini file, but I think we should add a wiki_to_html() on it ;-)

do we really need the req.redirect, or is it just cosmetic, to have URLs consistent with the displayed repository name?

It follows the idea that a resource should have a single URL to represent it, but I agree that it's largely cosmetic. Maybe I should change the redirects to permanent ones, as that's really the main use case for aliases. Or would you prefer that I remove the redirects?

No, I just asked to be sure to understand, I think it's OK to keep them. Maybe we should do a permanent redirect, but that's only permanent until the next alias change ;-)

I've tested the patch a bit more, there's at least one thing broken by the redirect, and it is when '' is an alias. In this case, going to /browse will take you to the target repository, therefore you have no chance to see the Repository index ;-)

A simple:

if reponame and reponame != repos.reponame:  # Redirect alias except for ''

instead of:

if reponame != repos.reponame:  # Redirect alias

in browser.py at line 913 seems to be enough to fix that issue.

The repository filters for the timeline seem to work fine as well, except for the case where '' is the name of a real repository and there's no alias for it …

[Christian Boos]

if reponame and reponame != repos.reponame:  # Redirect alias except for ''

And this also avoid the 'NoneType' object has no attribute 'reponame' error in case reponame == '' and repos == None at that place, though that error comes back later.

This happens when no default repository is defined and there's no alias.

Full fix:

trac/versioncontrol/web_ui/browser.py

@@ -346 +346 @@
             raise ResourceNotFound(_("No repository '%(repo)s' found",
                                    repo=reponame))
 
-        if reponame != repos.reponame:  # Redirect alias
+        if reponame and reponame != repos.reponame:  # Redirect alias
             qs = req.query_string
             req.redirect(req.href.browser(repos.reponame or None, path)
                          + (qs and '?' + qs or ''))
@@ -369 +369 @@
             context = context(repos.resource.child('source', path,
                                                    version=node.created_rev))
 
+        reponame = repos and repos.reponame or None
+
         # Prepare template data
-        path_links = get_path_links(req.href, repos.reponame, path, rev,
+        path_links = get_path_links(req.href, reponame, path, rev,
                                     order, desc)
 
         repo_data = dir_data = file_data = None
@@ -389 +391 @@
                     'browser', context, node.get_properties())
             quickjump_data = list(repos.get_quickjump_entries(rev))
 
-        reponame = repos.reponame or None
         data = {
             'context': context, 'reponame': reponame,
             'repos': repos,

[Remy Blank]

Last patch (including fixes in comment:21) committed in [9109].

[Remy Blank]

And finally, added handling of repository descriptions in [9110].