Edgewall Software
Modify

Opened 10 years ago

Closed 8 years ago

Last modified 4 years ago

#7116 closed enhancement (fixed)

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

Reported by: esizikov@… Owned by: Christian Boos
Priority: high Milestone: 0.12-multirepos
Component: version control Version: devel
Severity: major Keywords: permissions, authzpolicy, svnauthz, multirepos, authzsourcepolicy
Cc: retracile, Tim Hatch, leho@…
Release Notes:
API Changes:

Description

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.

Attachments (6)

trac-browser-fine-grained-security-for-directories.patch (2.1 KB ) - added by esizikov@… 10 years ago.
trac-browser-fine-grained-security-for-directories.2.patch (2.5 KB ) - added by esizikov@… 10 years ago.
Fixes: apply permission check to all nodes, not only folders as in the first version; catch PermissionError exception to allow other exceptions raising.
7116-authz-policy-r9049.patch (69.6 KB ) - added by Remy Blank 8 years ago.
Current state of AuthzPermissionPolicy.
7116-authz-policy-r9054.patch (94.4 KB ) - added by Remy Blank 8 years ago.
Added many more permission checks.
7716-authz-policy-2-r9054.patch (96.6 KB ) - added by Remy Blank 8 years ago.
Use resources associated with model objects for permission checks.
7116-pointer-aliases-r9085.patch (47.3 KB ) - added by Remy Blank 8 years ago.
Make aliases pointers to repositories.

Download all attachments as: .zip

Change History (30)

comment:1 Changed 10 years ago by Christian Boos

Component: generalversion control
Milestone: 0.110.12
Owner: changed from Jonas Borgström to Christian Boos
Priority: normalhigh
Severity: normalmajor

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.

Changed 10 years ago by esizikov@…

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

comment:2 Changed 8 years ago by Christian Boos

Keywords: svnauthz multirepos added
Milestone: 0.130.12

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.

comment:3 Changed 8 years ago by Christian Boos

Keywords: authzpolicy added; authz removed

comment:4 Changed 8 years ago by 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.

comment:5 in reply to:  4 Changed 8 years ago by 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.

comment:6 Changed 8 years ago by Christian Boos

Milestone: 0.120.12-multirepos

Changed 8 years ago by Remy Blank

Current state of AuthzPermissionPolicy.

comment:7 Changed 8 years ago by 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.

Changed 8 years ago by Remy Blank

Added many more permission checks.

comment:8 Changed 8 years ago by 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.)

comment:9 in reply to:  8 ; Changed 8 years ago by Christian Boos

Cc: retracile Tim Hatch added

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)

comment:10 in reply to:  9 ; Changed 8 years ago by 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.

comment:11 in reply to:  10 Changed 8 years ago by 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.

Changed 8 years ago by Remy Blank

Use resources associated with model objects for permission checks.

comment:12 Changed 8 years ago by 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).

comment:13 Changed 8 years ago by 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.

comment:14 Changed 8 years ago by 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? :)

comment:15 Changed 8 years ago by lkraav <leho@…>

Cc: leho@… added

comment:16 in reply to:  14 Changed 8 years ago by Christian Boos

Replying to rblank:

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

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

Changed 8 years ago by Remy Blank

Make aliases pointers to repositories.

comment:17 Changed 8 years ago by 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.

comment:18 in reply to:  17 ; Changed 8 years ago by 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?

comment:19 in reply to:  18 ; Changed 8 years ago by 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?

comment:20 in reply to:  19 ; Changed 8 years ago by 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 …

comment:21 in reply to:  20 Changed 8 years ago by 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

    diff --git a/trac/versioncontrol/web_ui/browser.py b/trac/versioncontrol/web_ui/browser.py
    a b class BrowserModule(Component):  
    346346            raise ResourceNotFound(_("No repository '%(repo)s' found",
    347347                                   repo=reponame))
    348348
    349         if reponame != repos.reponame:  # Redirect alias
     349        if reponame and reponame != repos.reponame:  # Redirect alias
    350350            qs = req.query_string
    351351            req.redirect(req.href.browser(repos.reponame or None, path)
    352352                         + (qs and '?' + qs or ''))
    class BrowserModule(Component):  
    369369            context = context(repos.resource.child('source', path,
    370370                                                   version=node.created_rev))
    371371
     372        reponame = repos and repos.reponame or None
     373
    372374        # Prepare template data
    373         path_links = get_path_links(req.href, repos.reponame, path, rev,
     375        path_links = get_path_links(req.href, reponame, path, rev,
    374376                                    order, desc)
    375377
    376378        repo_data = dir_data = file_data = None
    class BrowserModule(Component):  
    389391                    'browser', context, node.get_properties())
    390392            quickjump_data = list(repos.get_quickjump_entries(rev))
    391393
    392         reponame = repos.reponame or None
    393394        data = {
    394395            'context': context, 'reponame': reponame,
    395396            'repos': repos,

comment:22 Changed 8 years ago by Remy Blank

Resolution: fixed
Status: newclosed

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

comment:23 Changed 8 years ago by Remy Blank

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

comment:24 Changed 4 years ago by Ryan J Ollos

Keywords: authzsourcepolicy added

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted.
to The owner will be changed from Christian Boos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .

 
Note: See TracTickets for help on using tickets.