Re: [PATCH] Fix for issue 2753 "SVNListParentPath feature doesn't work when svn authz is used."

Kamesh Jayachandran <kamesh_at_collab.net> writes:

> [issue2753] Fix issue 2753.

Let me see if I understand: The issue is that when SVNListParentPath
and AuthzSVNAccessFile are configured then GET requests for the parent
path get passed through the authz stuff. This is a bug because the
authz file doesn't control parent path.

Your patch recognises this request and avoids doing the authz check.

> Relax requests aimed at the repo Parent path from authz control.
>
> * subversion/mod_authz_svn/mod_authz_svn.c
> (req_check_access): When canonicalized 'uri' and 'root_path' are same
> allow the request.
> ]]]
>
> If there are no objections will commit this in next couple of days.
>
> Thanks
> With regards
> Kamesh Jayachandran
>
> Index: subversion/mod_authz_svn/mod_authz_svn.c
> ===================================================================
> --- subversion/mod_authz_svn/mod_authz_svn.c (revision 931820)
> +++ subversion/mod_authz_svn/mod_authz_svn.c (working copy)
> @@ -210,6 +210,8 @@
> svn_authz_t *access_conf = NULL;
> svn_error_t *svn_err;
> char errbuf[256];
> + const char *canonicalized_uri;
> + const char *canonicalized_root_path;
> const char *username_to_authorize = get_username_to_authorize(r, conf);
>
> switch (r->method_number)
> @@ -249,6 +251,15 @@
> break;
> }
>
> + canonicalized_uri = svn_uri_canonicalize(r->uri, r->pool);
> + canonicalized_root_path = svn_uri_canonicalize(conf->base_path, r->pool);

Can conf->base_path be canonicalised once in
create_authz_svn_dir_config rather than for every request?

> + if (strcmp(canonicalized_uri, canonicalized_root_path) == 0)
> + {
> + /*Do no access control when root_path(as configured in <Location>) and
> + given uri are same.*/
> + return OK;
> + }

What happens if SVNParentPath is not being used? Is base_path is the
root of the repository? Does this disable authz on the root of that
repository? Perhaps you should be checking dav_svn__get_list_parentpath?

I think this check would make more sense in access_checker rather than
req_check_access.

The code needs a comment to say why no access control is neccessary in
this case.

> +
> dav_err = dav_svn_split_uri(r,
> r->uri,
> conf->base_path,

-- 
Philip