Re: [PATCH] Make mod_authz_svn to apply the authz checks against upper/lowercased usernames

On Fri, 07 Mar 2008, Kamesh Jayachandran wrote:

> People using mod_auth_sspi(windows domain authentication apache module)
> face the 'Authorization Failed' error while
> accessing SVN with mixed case usernames which does not match in *exact
> case* with their authz rules.
>
> http://blog.michaelcheng.idv.hk/ explains it in detail.
>
> mod_auth_sspi has a directive by name 'SSPIUsernameCase' with possible
> values being 'Lower/Upper'.
...
> I feel we should have similar directive in mod_authz_svn, to author the
> *sane* authz rules in these mixed
> case username scenarios.
...

This sounds fairly reasonable to me. One big question I have is how usernames
are then recorded in the history (e.g. uppercase, lowercase, or as entered
by the user?).

> [[[
> Make mod_authz_svn to apply the authz checks against upper/lowercased
> usernames.
>
> * subversion/mod_authz_svn/mod_authz_svn.c
> (): Include 'apr_lib.h' and 'strings.h'.
> (struct authz_svn_config_rec): New member 'usernamecase'.

How about "force_username_case", or something similar that indicates what
we're doing with username case?

> (authz_svn_cmds): Populate 'authz_svn_config_rec.usernamecase'
> from configuration directive 'AuthzUsernameCase'.

Ditto for the name of the directive, and even more so for the help (which
should mention that the case is applied to authz).

> (convert_to_uppercase_string,
> convert_to_lowercase_string,
> get_username_to_authorize): New functions.

Do we really need both of those conversion functions? Seems like one
should be sufficient.

> (req_check_access, subreq_bypass): Apply authz check against
> upper/lowercased usernames.

upper/lower-cased

> ]]]

This patch doesn't match our coding conventions, but seems to comply with
the code in mod_authz_svn...

> Index: subversion/mod_authz_svn/mod_authz_svn.c
> ===================================================================
> --- subversion/mod_authz_svn/mod_authz_svn.c (revision 29765)
> +++ subversion/mod_authz_svn/mod_authz_svn.c (working copy)
> @@ -28,6 +28,8 @@
> #include <ap_config.h>
> #include <ap_provider.h>
> #include <apr_uri.h>
> +#include <apr_lib.h>
> +#include <strings.h>
> #include <mod_dav.h>
>
> #include "mod_dav_svn.h"
> @@ -46,6 +48,7 @@
> int no_auth_when_anon_ok;
> const char *base_path;
> const char *access_file;
> + const char *usernamecase;
> } authz_svn_config_rec;
>
> /*
> @@ -90,6 +93,11 @@
> "Set to 'On' to suppress authentication and authorization "
> "for requests which anonymous users are allowed to perform. "
> "(default is Off.)"),
> + AP_INIT_TAKE1("AuthzUsernameCase", ap_set_string_slot,
> + (void *)APR_OFFSETOF(authz_svn_config_rec, usernamecase),
> + OR_AUTHCFG,
> + "Set to 'Upper' or 'Lower' to convert the username before "
> + "checking for authorization."),

This looks pretty good as-is.

> { NULL }
> };
>
> @@ -135,6 +143,40 @@
> return access_conf;
> }
>
> +/* Converts STR_TO_CONVERT to upper case.*/
> +static void convert_to_uppercase_string(char *str_to_convert)
> +{
> + char *c = str_to_convert;

While a temporary variable isn't strictly necessary here, it certainly doesn't
hurt readability.

> + while (*c) {
> + *c = apr_toupper(*c);
> + ++c;
> + }
> +}
> +
> +/* Converts STR_TO_CONVERT to lower case.*/
> +static void convert_to_lowercase_string(char *str_to_convert)
> +{
> + char *c = str_to_convert;
> + while (*c) {
> + *c = apr_tolower(*c);
> + ++c;
> + }
> +}

Yeah, these are really redundant; only the apr_toupper()/apr_tolower() calls
differ. We definitely want a single function here.

> +
> +static char* get_username_to_authorize(request_rec *r,
> + authz_svn_config_rec *conf)
> +{
> + char *username_to_authorize = r->user;
> + if (conf->usernamecase) {
> + username_to_authorize = apr_pstrdup(r->pool, r->user);
> + if (strcasecmp(conf->usernamecase, "upper") == 0)
> + convert_to_uppercase_string(username_to_authorize);
> + else
> + convert_to_lowercase_string(username_to_authorize);

Ya know, I'd probably just inline the code here.

> + }

If we don't change r->user, I think the history will take the username as
specified by the end-user. This means that the history will be potentially
subject to several capitalizations of the same name.

> + return username_to_authorize;
> +}
> +
> /* 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
> @@ -162,6 +204,7 @@
> svn_authz_t *access_conf = NULL;
> svn_error_t *svn_err;
> char errbuf[256];
> + const char *username_to_authorize = get_username_to_authorize(r, conf);

canonicalized_username? Dunno, username_to_authorize is a pretty good name too
(nice and explicit).

>
> switch (r->method_number) {
> /* All methods requiring read access to all subtrees of r->uri */
> @@ -307,7 +350,8 @@
> || (!repos_path && (authz_svn_type & svn_authz_write)))
> {
> svn_err = svn_repos_authz_check_access(access_conf, repos_name,
> - repos_path, r->user,
> + repos_path,
> + username_to_authorize,
> authz_svn_type,
> &authz_access_granted,
> r->pool);
> @@ -353,7 +397,7 @@
> svn_err = svn_repos_authz_check_access(access_conf,
> dest_repos_name,
> dest_repos_path,
> - r->user,
> + username_to_authorize,
> svn_authz_write
> |svn_authz_recursive,
> &authz_access_granted,
> @@ -437,9 +481,11 @@
> authz_svn_config_rec *conf = NULL;
> svn_boolean_t authz_access_granted = FALSE;
> char errbuf[256];
> + const char *username_to_authorize;
>
> conf = ap_get_module_config(r->per_dir_config,
> &authz_svn_module);
> + username_to_authorize = get_username_to_authorize(r, conf);
>
> /* If configured properly, this should never be true, but just in case. */
> if (!conf->anonymous || !conf->access_file) {
> @@ -457,7 +503,8 @@
> */
> if (repos_path) {
> svn_err = svn_repos_authz_check_access(access_conf, repos_name,
> - repos_path, r->user,
> + repos_path,
> + username_to_authorize,
> svn_authz_none|svn_authz_read,
> &authz_access_granted,
> r->pool);