Re: [PATCH] introduce AuthzSVNRepoRelativeAccessFile configuration item for mod_authz_svn

Nick Piper <nick.piper_at_logica.com> writes:

> [[[
> Implement AuthzSVNRepoRelativeAccessFile to allow SVNParentPath to use
> a different authz configuration file for each repository.
>
> * subversion/mod_authz_svn/mod_authz_svn.c
> (get_access_conf) Check if AuthzSVNRepoRelativeAccessFile is on, and
> if so, load the conf/authz file from inside the repository being
> accessed rather than one which is statically configured in the
> Apache configuration.
> (subreq_bypass, access_checker, check_user_id, auth_checker)
> Recognise that it's valid not to have a AuthzSVNAccessFile if
> AuthzSVNRepoRelativeAccessFile is used.
> ]]]

Sounds like a reasonable thing to do.

> Index: subversion/mod_authz_svn/mod_authz_svn.c
> ===================================================================
> --- subversion/mod_authz_svn/mod_authz_svn.c (revision 1028364)
> +++ subversion/mod_authz_svn/mod_authz_svn.c (working copy)
> @@ -50,6 +50,7 @@
> int authoritative;
> int anonymous;
> int no_auth_when_anon_ok;
> + int repo_relative_access_file;
> const char *base_path;
> const char *access_file;
> const char *force_username_case;
> @@ -103,6 +104,13 @@
> "Set to 'On' to suppress authentication and authorization "
> "for requests which anonymous users are allowed to perform. "
> "(default is Off.)"),
> + AP_INIT_FLAG("AuthzSVNRepoRelativeAccessFile", ap_set_flag_slot,
> + (void *)APR_OFFSETOF(authz_svn_config_rec,
> + repo_relative_access_file),

The name should be AuthzSVNReposRelativeAccessFile for consistency with
SVNReposName.

It should be an error to specify both AuthzSVNAccessFile and
AuthzSVNReposRelativeAccessFile similar to SVNPath/SVNParentPath.

Perhaps it should be a path rather than a boolean?

> + OR_AUTHCFG,
> + "Set to 'On' to allow the AuthzSVNAccessFile to be relative "
> + "to the repository disk path. "
> + "(default is Off.)"),
> AP_INIT_TAKE1("AuthzForceUsernameCase", ap_set_string_slot,
> (void *)APR_OFFSETOF(authz_svn_config_rec,
> force_username_case),
> @@ -119,18 +127,34 @@
> get_access_conf(request_rec *r, authz_svn_config_rec *conf)
> {
> const char *cache_key = NULL;
> + const char *access_file = NULL;
> + const char *repos_path = NULL;

Those don't need to be initialised.

> void *user_data = NULL;
> svn_authz_t *access_conf = NULL;
> svn_error_t *svn_err;
> + dav_error *dav_err;
> char errbuf[256];
>
> + if (conf->repo_relative_access_file) {
> + dav_err = dav_svn_get_repos_path(r, conf->base_path, &repos_path);
> + if (dav_err) {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, dav_err->desc);
> + return NULL;
> + }
> + access_file = apr_pstrcat(r->pool, repos_path, "/conf/authz", NULL);

Use svn_dirent_join_many.

> + } else {
> + access_file = conf->access_file;
> + }

The whitespace is wrong, it should be

     if ( )
       {
         dav_err =
       }

> +
> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, "Path to authz file is %s", access_file);
> +
> cache_key = apr_pstrcat(r->pool, "mod_authz_svn:",
> - conf->access_file, (char *)NULL);
> + access_file, (char *)NULL);
> apr_pool_userdata_get(&user_data, cache_key, r->connection->pool);
> access_conf = user_data;
> if (access_conf == NULL)
> {
> - svn_err = svn_repos_authz_read(&access_conf, conf->access_file,
> + svn_err = svn_repos_authz_read(&access_conf, access_file,
> TRUE, r->connection->pool);
> if (svn_err)
> {

-- 
Philip