Eric Gillespie <epg@pretzelnet.org> writes:
> I think we've waited long enough for people to object :). I'm
> going to commit this change today or tomorrow. I'll fix what
> formatting issues i find, but i'm not the biggest expert on
> Subversion style either ;->.
Here's an updated patch. I changed the option value to
"short_circuit" and corrected the style problems i found.
Compiles without warnings, davautocheck running now.
Josh, all we lack now is a log message. Take a look at
<http://svn.collab.net/repos/svn/trunk/www/hacking.html#log-messages>.
Thanks.
Index: subversion/mod_authz_svn/mod_authz_svn.c
===================================================================
--- subversion/mod_authz_svn/mod_authz_svn.c (revision 24955)
+++ subversion/mod_authz_svn/mod_authz_svn.c (working copy)
@@ -26,6 +26,7 @@
#include <http_protocol.h>
#include <http_log.h>
#include <ap_config.h>
+#include <ap_provider.h>
#include <apr_uri.h>
#include <mod_dav.h>
#if AP_MODULE_MAGIC_AT_LEAST(20060110,0)
@@ -34,6 +35,7 @@
#endif
#include "mod_dav_svn.h"
+#include "mod_authz_svn.h"
#include "svn_path.h"
#include "svn_config.h"
#include "svn_string.h"
@@ -95,6 +97,48 @@
{ NULL }
};
+/*
+ * Get the, possibly cached, svn_authz_t for this request.
+ */
+static svn_authz_t *get_access_conf(request_rec *r,
+ authz_svn_config_rec *conf)
+{
+ const char *cache_key = NULL;
+ void *user_data = NULL;
+ svn_authz_t *access_conf = NULL;
+ svn_error_t *svn_err;
+ char errbuf[256];
+
+ cache_key = apr_pstrcat(r->pool, "mod_authz_svn:",
+ conf->access_file, 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,
+ TRUE, r->connection->pool);
+ if (svn_err) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR,
+ /* If it is an error code that APR can make sense
+ of, then show it, otherwise, pass zero to avoid
+ putting "APR does not understand this error code"
+ in the error log. */
+ ((svn_err->apr_err >= APR_OS_START_USERERR &&
+ svn_err->apr_err < APR_OS_START_CANONERR) ?
+ 0 : svn_err->apr_err),
+ r, "Failed to load the AuthzSVNAccessFile: %s",
+ svn_err_best_message(svn_err,
+ errbuf, sizeof(errbuf)));
+ svn_error_clear(svn_err);
+ access_conf = NULL;
+ } else {
+ /* Cache the open repos for the next request on this connection */
+ apr_pool_userdata_set(access_conf, cache_key,
+ NULL, r->connection->pool);
+ }
+ }
+ return access_conf;
+}
+
/* Check if the current request R is allowed. Upon exit *REPOS_PATH_REF
* will contain the path and repository name that an operation was requested
* on in the form 'name:path'. *DEST_REPOS_PATH_REF will contain the
@@ -121,8 +165,6 @@
svn_boolean_t authz_access_granted = FALSE;
svn_authz_t *access_conf = NULL;
svn_error_t *svn_err;
- const char *cache_key;
- void *user_data;
char errbuf[256];
switch (r->method_number) {
@@ -239,34 +281,12 @@
}
/* Retrieve/cache authorization file */
- cache_key = apr_pstrcat(r->pool, "mod_authz_svn:", conf->access_file, 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,
- TRUE, r->connection->pool);
- if (svn_err) {
- ap_log_rerror(APLOG_MARK, APLOG_ERR,
- /* If it is an error code that APR can make sense
- of, then show it, otherwise, pass zero to avoid
- putting "APR does not understand this error code"
- in the error log. */
- ((svn_err->apr_err >= APR_OS_START_USERERR &&
- svn_err->apr_err < APR_OS_START_CANONERR) ?
- 0 : svn_err->apr_err),
- r, "Failed to load the AuthzSVNAccessFile: %s",
- svn_err_best_message(svn_err,
- errbuf, sizeof(errbuf)));
- svn_error_clear(svn_err);
-
+ access_conf = get_access_conf(r,conf);
+ if(access_conf == NULL)
+ {
return DECLINED;
}
- /* Cache the open repos for the next request on this connection */
- apr_pool_userdata_set(access_conf, cache_key,
- NULL, r->connection->pool);
- }
-
/* Perform authz access control.
*
* First test the special case where repos_path == NULL, and skip
@@ -408,6 +428,70 @@
}
/*
+ * This function is used as a provider to allow mod_dav_svn to bypass the
+ * generation of an apache request when checking GET access from
+ * "mod_dav_svn/authz.c" .
+ */
+static int subreq_bypass(request_rec *r,
+ const char *repos_path,
+ const char *repos_name)
+{
+ svn_error_t *svn_err = NULL;
+ svn_authz_t *access_conf = NULL;
+ authz_svn_config_rec *conf = NULL;
+ svn_boolean_t authz_access_granted = FALSE;
+ char errbuf[256];
+
+ conf = ap_get_module_config(r->per_dir_config,
+ &authz_svn_module);
+
+ /* If configured properly, this should never be true, but just in case. */
+ if (!conf->anonymous || !conf->access_file) {
+ log_access_verdict(APLOG_MARK, r, 0, repos_path, NULL);
+ return HTTP_FORBIDDEN;
+ }
+
+ /* Retrieve authorization file */
+ access_conf = get_access_conf(r,conf);
+ if(access_conf == NULL)
+ return HTTP_FORBIDDEN;
+
+ /* Perform authz access control.
+ * See similarly labeled comment in req_check_access.
+ */
+ if (repos_path) {
+ svn_err = svn_repos_authz_check_access(access_conf, repos_name,
+ repos_path, r->user,
+ svn_authz_none|svn_authz_read,
+ &authz_access_granted,
+ r->pool);
+ if (svn_err) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR,
+ /* If it is an error code that APR can make
+ sense of, then show it, otherwise, pass
+ zero to avoid putting "APR does not
+ understand this error code" in the error
+ log. */
+ ((svn_err->apr_err >= APR_OS_START_USERERR &&
+ svn_err->apr_err < APR_OS_START_CANONERR) ?
+ 0 : svn_err->apr_err),
+ r, "Failed to perform access control: %s",
+ svn_err_best_message(svn_err, errbuf, sizeof(errbuf)));
+ svn_error_clear(svn_err);
+ return HTTP_FORBIDDEN;
+ }
+ if (!authz_access_granted) {
+ log_access_verdict(APLOG_MARK, r, 0, repos_path, NULL);
+ return HTTP_FORBIDDEN;
+ }
+ }
+
+ log_access_verdict(APLOG_MARK, r, 1, repos_path, NULL);
+
+ return OK;
+}
+
+/*
* Hooks
*/
@@ -552,6 +636,11 @@
#if AP_MODULE_MAGIC_AT_LEAST(20060110,0)
ap_hook_optional_fn_retrieve(import_ap_satisfies, NULL, NULL, APR_HOOK_MIDDLE);
#endif
+ ap_register_provider(p,
+ AUTHZ_SVN__SUBREQ_BYPASS_PROV_GRP,
+ AUTHZ_SVN__SUBREQ_BYPASS_PROV_NAME,
+ AUTHZ_SVN__SUBREQ_BYPASS_PROV_VER,
+ (void*)subreq_bypass);
}
module AP_MODULE_DECLARE_DATA authz_svn_module =
Index: subversion/mod_dav_svn/mod_dav_svn.c
===================================================================
--- subversion/mod_dav_svn/mod_dav_svn.c (revision 24955)
+++ subversion/mod_dav_svn/mod_dav_svn.c (working copy)
@@ -33,12 +33,16 @@
#include "mod_dav_svn.h"
#include "dav_svn.h"
+#include "mod_authz_svn.h"
/* This is the default "special uri" used for SVN's special resources
(e.g. working resources, activities) */
#define SVN_DEFAULT_SPECIAL_URI "!svn"
+/* This is the value to be given to SVNPathAuthz to bypass the apache
+ * subreq mechanism and make a call directly to mod_authz_svn. */
+#define PATHAUTHZ_BYPASS_ARG "short_circuit"
/* per-server configuration */
typedef struct {
@@ -55,6 +59,13 @@
CONF_FLAG_OFF
};
+/* An enum used for the per directory configuration path_authz_method. */
+enum path_authz_conf {
+ CONF_PATHAUTHZ_DEFAULT,
+ CONF_PATHAUTHZ_ON,
+ CONF_PATHAUTHZ_OFF,
+ CONF_PATHAUTHZ_BYPASS
+};
/* per-dir configuration */
typedef struct {
@@ -63,7 +74,7 @@
const char *xslt_uri; /* XSL transform URI */
const char *fs_parent_path; /* path to parent of SVN FS'es */
enum conf_flag autoversioning; /* whether autoversioning is active */
- enum conf_flag do_path_authz; /* whether GET subrequests are active */
+ enum path_authz_conf path_authz_method; /* how GET subrequests are handled */
enum conf_flag list_parentpath; /* whether to allow GET of parentpath */
const char *root_dir; /* our top-level directory */
const char *master_uri; /* URI to the master SVN repos */
@@ -77,6 +88,8 @@
extern module AP_MODULE_DECLARE_DATA dav_svn_module;
+/* The authz_svn provider for bypassing path authz. */
+static authz_svn__subreq_bypass_func_t pathauthz_bypass_func = NULL;
static int
init(apr_pool_t *p, apr_pool_t *plog, apr_pool_t *ptemp, server_rec *s)
@@ -162,7 +175,7 @@
newconf->xslt_uri = INHERIT_VALUE(parent, child, xslt_uri);
newconf->fs_parent_path = INHERIT_VALUE(parent, child, fs_parent_path);
newconf->autoversioning = INHERIT_VALUE(parent, child, autoversioning);
- newconf->do_path_authz = INHERIT_VALUE(parent, child, do_path_authz);
+ newconf->path_authz_method = INHERIT_VALUE(parent, child, path_authz_method);
newconf->list_parentpath = INHERIT_VALUE(parent, child, list_parentpath);
/* Prefer our parent's value over our new one - hence the swap. */
newconf->root_dir = INHERIT_VALUE(child, parent, root_dir);
@@ -230,14 +243,23 @@
static const char *
-SVNPathAuthz_cmd(cmd_parms *cmd, void *config, int arg)
+SVNPathAuthz_cmd(cmd_parms *cmd, void *config, const char *arg1)
{
dir_conf_t *conf = config;
- if (arg)
- conf->do_path_authz = CONF_FLAG_ON;
+ if (apr_strnatcasecmp("off", arg1) == 0)
+ conf->path_authz_method = CONF_PATHAUTHZ_OFF;
+ else if (apr_strnatcasecmp(PATHAUTHZ_BYPASS_ARG,arg1) == 0)
+ {
+ conf->path_authz_method = CONF_PATHAUTHZ_BYPASS;
+ if (pathauthz_bypass_func == NULL)
+ pathauthz_bypass_func=ap_lookup_provider(
+ AUTHZ_SVN__SUBREQ_BYPASS_PROV_GRP,
+ AUTHZ_SVN__SUBREQ_BYPASS_PROV_NAME,
+ AUTHZ_SVN__SUBREQ_BYPASS_PROV_VER);
+ }
else
- conf->do_path_authz = CONF_FLAG_OFF;
+ conf->path_authz_method = CONF_PATHAUTHZ_ON;
return NULL;
}
@@ -447,16 +469,33 @@
}
+/* FALSE if path authorization should be skipped.
+ * TRUE if either the bypass or the apache subrequest methods should be used.
+ */
svn_boolean_t
dav_svn__get_pathauthz_flag(request_rec *r)
{
dir_conf_t *conf;
conf = ap_get_module_config(r->per_dir_config, &dav_svn_module);
- return conf->do_path_authz != CONF_FLAG_OFF;
+ return conf->path_authz_method != CONF_PATHAUTHZ_OFF;
}
+/* Function pointer if we should use the bypass directly to mod_authz_svn.
+ * NULL otherwise. */
+authz_svn__subreq_bypass_func_t
+dav_svn__get_pathauthz_bypass(request_rec *r)
+{
+ dir_conf_t *conf;
+ conf = ap_get_module_config(r->per_dir_config, &dav_svn_module);
+
+ if(conf->path_authz_method==CONF_PATHAUTHZ_BYPASS)
+ return pathauthz_bypass_func;
+ return NULL;
+}
+
+
svn_boolean_t
dav_svn__get_list_parentpath_flag(request_rec *r)
{
@@ -630,9 +669,11 @@
ACCESS_CONF|RSRC_CONF, "turn on deltaV autoversioning."),
/* per directory/location */
- AP_INIT_FLAG("SVNPathAuthz", SVNPathAuthz_cmd, NULL,
+ AP_INIT_TAKE1("SVNPathAuthz", SVNPathAuthz_cmd, NULL,
ACCESS_CONF|RSRC_CONF,
- "control path-based authz by enabling/disabling subrequests"),
+ "control path-based authz by enabling subrequests(On,default), "
+ "disabling subrequests(Off), or"
+ "querying mod_authz_svn directly(" PATHAUTHZ_BYPASS_ARG ")"),
/* per directory/location */
AP_INIT_FLAG("SVNListParentPath", SVNListParentPath_cmd, NULL,
Index: subversion/mod_dav_svn/dav_svn.h
===================================================================
--- subversion/mod_dav_svn/dav_svn.h (revision 24955)
+++ subversion/mod_dav_svn/dav_svn.h (working copy)
@@ -31,6 +31,7 @@
#include "svn_repos.h"
#include "svn_path.h"
#include "svn_xml.h"
+#include "mod_authz_svn.h"
#ifdef __cplusplus
extern "C" {
@@ -255,6 +256,11 @@
/* for the repository referred to by this request, are subrequests active? */
svn_boolean_t dav_svn__get_pathauthz_flag(request_rec *r);
+/* for the repository referred to by this request, are subrequests bypassed?
+ * A function pointer if yes, NULL if not.
+ */
+authz_svn__subreq_bypass_func_t dav_svn__get_pathauthz_bypass(request_rec *r);
+
/* for the repository referred to by this request, is a GET of
SVNParentPath allowed? */
svn_boolean_t dav_svn__get_list_parentpath_flag(request_rec *r);
Index: subversion/mod_dav_svn/authz.c
===================================================================
--- subversion/mod_dav_svn/authz.c (revision 24955)
+++ subversion/mod_dav_svn/authz.c (working copy)
@@ -22,6 +22,7 @@
#include "svn_pools.h"
#include "svn_path.h"
+#include "mod_authz_svn.h"
#include "dav_svn.h"
@@ -41,6 +42,7 @@
request_rec *subreq;
enum dav_svn__build_what uri_type;
svn_boolean_t allowed = FALSE;
+ authz_svn__subreq_bypass_func_t allow_read_bypass = NULL;
/* Easy out: if the admin has explicitly set 'SVNPathAuthz Off',
then this whole callback does nothing. */
@@ -49,6 +51,20 @@
return TRUE;
}
+ /* If bypass is specified and authz has exported the provider.
+ Otherwise, we fall through to the full version. This should be
+ safer than allowing or disallowing all accesses if there is a
+ configuration error.
+ XXX: Is this the proper thing to do in this case? */
+ allow_read_bypass = dav_svn__get_pathauthz_bypass(r);
+ if (allow_read_bypass != NULL)
+ {
+ if (allow_read_bypass(r,path, repos->repo_name) == OK)
+ return TRUE;
+ else
+ return FALSE;
+ }
+
/* If no revnum is specified, assume HEAD. */
if (SVN_IS_VALID_REVNUM(rev))
uri_type = DAV_SVN__BUILD_URI_VERSION;
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed May 9 21:05:55 2007