=== subversion/include/svn_error_codes.h ================================================================== --- subversion/include/svn_error_codes.h (revision 116) +++ subversion/include/svn_error_codes.h (local) @@ -1,7 +1,7 @@ /** * @copyright * ==================================================================== - * Copyright (c) 2000-2004 CollabNet. All rights reserved. + * Copyright (c) 2000-2005 CollabNet. All rights reserved. * * This software is licensed as described in the file COPYING, which * you should have received as part of this distribution. The terms @@ -784,6 +784,9 @@ SVN_ERR_AUTHZ_CATEGORY_START + 2, "Item is partially readable") + SVN_ERRDEF (SVN_ERR_AUTHZ_INVALID_CONFIG, + SVN_ERR_AUTHZ_CATEGORY_START + 3, + "Invalid authz configuration") /* svndiff errors */ === subversion/include/svn_repos.h ================================================================== --- subversion/include/svn_repos.h (revision 116) +++ subversion/include/svn_repos.h (local) @@ -30,7 +30,6 @@ #include "svn_types.h" #include "svn_error.h" #include "svn_version.h" -#include "svn_config.h" #ifdef __cplusplus @@ -1689,14 +1688,30 @@ svn_authz_recursive = 4 } svn_repos_authz_access_t; +/** A data type which stores the authz information. */ +typedef struct svn_authz_t svn_authz_t; + +/** Read authz configuration data from @a file (a file or registry + * path) into @a *authzp, allocated in @a pool. + * + * If the read file is not a valid authz rule file, then abort loading + * and return an error. + * + * If @a file does not exist, then if @a must_exist, return an error, + * otherwise return an empty @c svn_config_t. + */ +svn_error_t * +svn_repos_authz_read (svn_authz_t **authzp, const char *file, + svn_boolean_t must_exist, apr_pool_t *pool); + /** - * Check wether @a user can access @a path in the repository @a - * repos_name with the @a required_access. @a cfg lists the ACLs to + * Check whether @a user can access @a path in the repository @a + * repos_name with the @a required_access. @a authz lists the ACLs to * check against. Set @a *access_granted to indicate if the requested * access is granted. */ svn_error_t * -svn_repos_authz_check_access (svn_config_t *cfg, const char *repos_name, +svn_repos_authz_check_access (svn_authz_t *authz, const char *repos_name, const char *path, const char *user, svn_repos_authz_access_t required_access, svn_boolean_t *access_granted, === subversion/libsvn_repos/authz.c ================================================================== --- subversion/libsvn_repos/authz.c (revision 116) +++ subversion/libsvn_repos/authz.c (local) @@ -24,9 +24,10 @@ #include "svn_error.h" #include "svn_path.h" #include "svn_repos.h" +#include "svn_config.h" -struct authz_baton { +struct authz_lookup_baton { apr_pool_t *pool; svn_config_t *config; @@ -41,8 +42,23 @@ svn_boolean_t access; }; +struct authz_validate_baton { + apr_pool_t *pool; + svn_config_t *config; + svn_error_t *err; +}; +/* The data type storing the internal representation of an authz + * configuration. Currently this is just a wrapper around a + * svn_config_t. */ +struct svn_authz_t +{ + svn_config_t *cfg; +}; + + + /* Determine whether the required access is granted given what authz * are allowed and denied. Return TRUE if the required access is * granted. @@ -89,17 +105,17 @@ static svn_boolean_t -authz_group_contains_user_internal (svn_config_t *cfg, - const char *group, - const char *user, - apr_hash_t *checked_groups, - apr_pool_t *pool) +authz_group_contains_user (svn_config_t *cfg, + const char *group, + const char *user, + apr_pool_t *pool) { const char *value; apr_array_header_t *list; int i; - svn_config_get (cfg, &value, "groups", group, ""); + svn_config_get (cfg, &value, "groups", group, NULL); + list = svn_cstring_split (value, ",", TRUE, pool); for (i = 0; i < list->nelts; i++) @@ -109,23 +125,11 @@ /* If the 'user' is a subgroup, recurse into it. */ if (*group_user == '@') { - /* Guard against circular dependencies by checking group - name against hash. */ - /* XXX: Should this return an error? */ - if (apr_hash_get (checked_groups, &group_user[1], - APR_HASH_KEY_STRING)) - continue; - - /* Add group to hash of checked groups. */ - apr_hash_set (checked_groups, &group_user[1], - APR_HASH_KEY_STRING, ""); - - /* Recurse on that group. */ - if (authz_group_contains_user_internal (cfg, &group_user[1], - user, checked_groups, - pool)) + if (authz_group_contains_user (cfg, &group_user[1], + user, pool)) return TRUE; } + /* If the user matches, stop. */ else if (strcmp (user, group_user) == 0) return TRUE; @@ -136,24 +140,13 @@ -static svn_boolean_t -authz_group_contains_user(svn_config_t *cfg, const char *group, - const char *user, apr_pool_t *pool) -{ - return authz_group_contains_user_internal (cfg, group, user, - apr_hash_make (pool), - pool); -} - - - /* Callback to parse one line of an authz file and update the * authz_baton accordingly. */ static svn_boolean_t authz_parse_line (const char *name, const char *value, void *baton) { - struct authz_baton *b = baton; + struct authz_lookup_baton *b = baton; /* Work out whether this ACL line applies to the user. */ if (strcmp (name, "*") != 0) @@ -169,6 +162,7 @@ b->user, b->pool)) return TRUE; } + /* User rule for wrong user. Stop. */ else if (strcmp (name, b->user) != 0) return TRUE; @@ -196,7 +190,7 @@ static svn_boolean_t authz_parse_section (const char *section_name, void *baton) { - struct authz_baton *b = baton; + struct authz_lookup_baton *b = baton; svn_boolean_t conclusive; /* Does the section apply to us? */ @@ -231,9 +225,9 @@ * to access a section specific to the given repository before falling * back to pan-repository rules. * - * Return a boolean which tells the caller whether we were able to - * determine the requested access rights. Access status is in - * *access_granted. + * Update *access_granted to inform the caller of the outcome of the + * lookup. Return a boolean indicating whether the access rights were + * successfully determined. */ static svn_boolean_t authz_get_path_access (svn_config_t *cfg, const char *repos_name, @@ -243,7 +237,7 @@ apr_pool_t *pool) { const char *qualified_path; - struct authz_baton baton = { 0 }; + struct authz_lookup_baton baton = { 0 }; baton.pool = pool; baton.config = cfg; @@ -279,16 +273,15 @@ * requested access. * * As soon as one is found, or else when the whole ACL file has been - * searched, return the updated authorization in access_granted. + * searched, return the updated authorization status. */ -static void +static svn_boolean_t authz_get_tree_access (svn_config_t *cfg, const char *repos_name, const char *path, const char *user, svn_repos_authz_access_t required_access, - svn_boolean_t *access_granted, apr_pool_t *pool) { - struct authz_baton baton = { 0 }; + struct authz_lookup_baton baton = { 0 }; baton.pool = pool; baton.config = cfg; @@ -302,13 +295,165 @@ svn_config_enumerate_sections (cfg, authz_parse_section, &baton); - *access_granted = baton.access; + return baton.access; } +/* Examine the entire definition of a group, checking for errors. The + * errors detected are references to non-existent groups and circular + * dependencies between groups. */ +static svn_error_t * +authz_group_walk (svn_config_t *cfg, + const char *group, + apr_hash_t *checked_groups, + apr_pool_t *pool) +{ + const char *value; + apr_array_header_t *list; + int i; + + svn_config_get (cfg, &value, "groups", group, NULL); + /* Having a non-existent group in the ACL configuration might be the + sign of a typo. Refuse to perform authz on uncertain rules. */ + if (!value) + return svn_error_createf (SVN_ERR_AUTHZ_INVALID_CONFIG, NULL, + "An authz rule refers to group '%s', " + "which is undefined", + group); + + list = svn_cstring_split (value, ",", TRUE, pool); + + for (i = 0; i < list->nelts; i++) + { + const char *group_user = APR_ARRAY_IDX(list, i, char *); + + /* If the 'user' is a subgroup, recurse into it. */ + if (*group_user == '@') + { + /* A circular dependency between groups is a Bad Thing. We + don't do authz with invalid ACL files. */ + if (apr_hash_get (checked_groups, &group_user[1], + APR_HASH_KEY_STRING)) + return svn_error_createf (SVN_ERR_AUTHZ_INVALID_CONFIG, + NULL, + "Circular dependency between " + "groups '%s' and '%s'", + &group_user[1], group); + + /* Add group to hash of checked groups. */ + apr_hash_set (checked_groups, &group_user[1], + APR_HASH_KEY_STRING, ""); + + /* Recurse on that group. */ + SVN_ERR (authz_group_walk (cfg, &group_user[1], + checked_groups, pool)); + } + } + + return SVN_NO_ERROR; +} + + + +/* Callback to check whether groups mentioned in the given authz rule + exist. */ +static svn_boolean_t authz_validate_rule (const char *name, + const char *value, + void *baton) +{ + const char *val; + struct authz_validate_baton *b = baton; + + /* If the rule applies to a group, check its existence. */ + if (*name == '@') + { + svn_config_get (b->config, &val, "groups", + &name[1], NULL); + /* Having a non-existent group in the ACL configuration might be + the sign of a typo. Refuse to perform authz on uncertain + rules. */ + if (!val) + { + b->err = svn_error_createf (SVN_ERR_AUTHZ_INVALID_CONFIG, NULL, + "An authz rule refers to group " + "'%s', which is undefined", + name); + return FALSE; + } + } + + return TRUE; +} + + + +/* Callback to check whether groups defined don't cyclicly depend on + each other. */ +static svn_boolean_t authz_validate_group (const char *name, + const char *value, + void *baton) +{ + struct authz_validate_baton *b = baton; + + b->err = authz_group_walk (b->config, name, apr_hash_make (b->pool), + b->pool); + if (b->err) + return FALSE; + + return TRUE; +} + + + +/* Callback to validate a section of the authz ruleset. */ +static svn_boolean_t authz_validate_section (const char *name, + void *baton) +{ + struct authz_validate_baton *b = baton; + + /* If the section is the groups definition, use the group checking + callback. Otherwise, use the rule checking callback. */ + if (strncmp (name, "groups", 6) == 0) + svn_config_enumerate (b->config, name, authz_validate_group, baton); + else + svn_config_enumerate (b->config, name, authz_validate_rule, baton); + + if (b->err) + return FALSE; + + return TRUE; +} + + + svn_error_t * -svn_repos_authz_check_access (svn_config_t *cfg, const char *repos_name, +svn_repos_authz_read (svn_authz_t **authzp, const char *file, + svn_boolean_t must_exist, apr_pool_t *pool) +{ + svn_authz_t *authz = apr_palloc (pool, sizeof(*authz)); + struct authz_validate_baton baton = { 0 }; + + baton.pool = pool; + baton.err = SVN_NO_ERROR; + + /* Load the rule file. */ + SVN_ERR (svn_config_read (&authz->cfg, file, must_exist, pool)); + baton.config = authz->cfg; + + /* Step through the entire rule file, aborting on error. */ + svn_config_enumerate_sections (authz->cfg, authz_validate_section, + &baton); + SVN_ERR (baton.err); + + *authzp = authz; + return SVN_NO_ERROR; +} + + + +svn_error_t * +svn_repos_authz_check_access (svn_authz_t *authz, const char *repos_name, const char *path, const char *user, svn_repos_authz_access_t required_access, svn_boolean_t *access_granted, @@ -316,40 +461,33 @@ { const char *base_name = path; const char *current_path = path; - svn_boolean_t access_determined = FALSE; /* Determine the granted access for the requested path. */ - do + while (!authz_get_path_access (authz->cfg, repos_name, + current_path, user, + required_access, + access_granted, + pool)) { - access_determined = authz_get_path_access (cfg, repos_name, - current_path, user, - required_access, - access_granted, pool); - - if (!access_determined) + /* Stop if the loop hits the repository root with no + results. */ + if (base_name[0] == '/' && base_name[1] == '\0') { - /* Stop if the loop hits the repository root with no - results. */ - if (base_name[0] == '/' && base_name[1] == '\0') - { - /* Deny access by default. */ - *access_granted = FALSE; - return SVN_NO_ERROR; - } - - /* Work back to the parent path. */ - svn_path_split (current_path, ¤t_path, &base_name, pool); + /* Deny access by default. */ + *access_granted = FALSE; + return SVN_NO_ERROR; } - } while (!access_determined); + /* Work back to the parent path. */ + svn_path_split (current_path, ¤t_path, &base_name, pool); + } + /* If the caller requested recursive access, we need to walk through the entire authz config to see whether any child paths are denied to the requested user. */ - if (*access_granted - && (required_access & svn_authz_recursive)) - authz_get_tree_access (cfg, repos_name, path, user, - required_access, access_granted, - pool); + if (*access_granted && (required_access & svn_authz_recursive)) + *access_granted = authz_get_tree_access (authz->cfg, repos_name, path, + user, required_access, pool); return SVN_NO_ERROR; } === subversion/mod_authz_svn/mod_authz_svn.c ================================================================== --- subversion/mod_authz_svn/mod_authz_svn.c (revision 116) +++ subversion/mod_authz_svn/mod_authz_svn.c (local) @@ -104,7 +104,7 @@ dav_error *dav_err; svn_repos_authz_access_t authz_svn_type = svn_authz_none; svn_boolean_t authz_access_granted = FALSE; - svn_config_t *access_conf = NULL; + svn_authz_t *access_conf = NULL; svn_error_t *svn_err; const char *cache_key; void *user_data; @@ -227,8 +227,8 @@ apr_pool_userdata_get(&user_data, cache_key, r->connection->pool); access_conf = user_data; if (access_conf == NULL) { - svn_err = svn_config_read(&access_conf, conf->access_file, TRUE, - r->connection->pool); + 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 === subversion/tests/libsvn_repos/repos-test.c ================================================================== --- subversion/tests/libsvn_repos/repos-test.c (revision 116) +++ subversion/tests/libsvn_repos/repos-test.c (local) @@ -1031,6 +1031,47 @@ } +/* Helper for the authz test. Return a svn_config_t handle containing + a parsed representation of the given authz_contents. */ +static svn_error_t * +authz_get_handle (svn_authz_t **authzp, const char *authz_contents, + apr_pool_t *pool) +{ + char *authz_file_tmpl; + apr_file_t *authz_file; + apr_status_t apr_err; + const char *authz_file_path; + + /* Create a temporary file. */ + authz_file_tmpl = apr_pstrdup (pool, "authz_test_XXXXXX"); + apr_err = apr_file_mktemp (&authz_file, authz_file_tmpl, 0, pool); + if (apr_err != APR_SUCCESS) + return svn_error_wrap_apr (apr_err, "Opening temporary file"); + + /* Write the authz ACLs to the file. */ + apr_err = apr_file_write_full (authz_file, authz_contents, + strlen(authz_contents), NULL); + if (apr_err != APR_SUCCESS) + return svn_error_wrap_apr (apr_err, "Writing test authz file"); + + /* Read the authz configuration back and start testing. */ + apr_err = apr_file_name_get (&authz_file_path, authz_file); + if (apr_err != APR_SUCCESS) + return svn_error_wrap_apr (apr_err, "Getting authz file path"); + + SVN_ERR_W (svn_repos_authz_read (authzp, authz_file_path, TRUE, pool), + "Opening test authz file"); + + /* Close the temporary descriptor, which'll delete the file. */ + apr_err = apr_file_close (authz_file); + if (apr_err != APR_SUCCESS) + return svn_error_wrap_apr (apr_err, "Removing test authz file"); + + return SVN_NO_ERROR; +} + + + /* Test that authz is giving out the right authorizations. */ static svn_error_t * authz (const char **msg, @@ -1038,12 +1079,9 @@ svn_test_opts_t *opts, apr_pool_t *pool) { - char *authz_file_tmpl; - apr_file_t *authz_file; - apr_status_t apr_err; - const char *authz_file_path; const char *contents; - svn_config_t *cfg; + svn_authz_t *authz; + svn_error_t *err; svn_boolean_t access_granted; apr_pool_t *subpool = svn_pool_create (pool); int i; @@ -1080,15 +1118,21 @@ if (msg_only) return SVN_NO_ERROR; - /* The test logic: We dump a test authz file to disk, then load it - * and perform various access tests on it. Each test has a known - * outcome and tests different aspects of authz, such as inheriting - * parent-path authz, pan-repository rules or recursive access. - * 'plato' is our friendly neighborhood user with more access rights - * than other anonymous philosophers. + /* The test logic: + * + * 1. Perform various access tests on a set of authz rules. Each + * test has a known outcome and tests different aspects of authz, + * such as inheriting parent-path authz, pan-repository rules or + * recursive access. 'plato' is our friendly neighborhood user with + * more access rights than other anonymous philosophers. + * + * 2. Load an authz file containing a cyclic dependency in groups + * and another containing a reference to an undefined group. Verify + * that svn_repos_authz_read fails to load both and returns an + * "invalid configuration" error. */ - /* The authz rules for the test. */ + /* The authz rules for the phase 1 tests. */ contents = "[greek:/A]" APR_EOL_STR @@ -1129,41 +1173,13 @@ APR_EOL_STR APR_EOL_STR; - /* Create a temporary file and retrieve its path. */ - authz_file_tmpl = apr_pstrdup(subpool, "authz_test_XXXXXX"); - apr_err = apr_file_mktemp (&authz_file, authz_file_tmpl, - APR_CREATE | APR_READ | APR_WRITE, subpool); - if (apr_err != APR_SUCCESS) - return svn_error_wrap_apr(apr_err, "Opening temporary file"); + /* Load the test authz rules. */ + SVN_ERR (authz_get_handle(&authz, contents, subpool)); - /* Write the authz ACLs to the file. */ - apr_err = apr_file_write_full (authz_file, contents, - strlen(contents), NULL); - if (apr_err != APR_SUCCESS) - return svn_error_wrap_apr(apr_err, "Writing test authz file"); - - /* Read the authz configuration back and start testing. */ - apr_err = apr_file_name_get (&authz_file_path, authz_file); - if (apr_err != APR_SUCCESS) - return svn_error_wrap_apr(apr_err, "Getting authz file path"); - - /* Close the temporary descriptor. */ - apr_err = apr_file_close (authz_file); - if (apr_err != APR_SUCCESS) - return svn_error_wrap_apr(apr_err, "Closing test authz file"); - - SVN_ERR_W (svn_config_read (&cfg, authz_file_path, TRUE, subpool), - "Opening test authz file"); - - /* Delete the file. */ - apr_err = apr_file_remove (authz_file_path, subpool); - if (apr_err != APR_SUCCESS) - return svn_error_wrap_apr(apr_err, "Removing test authz file"); - /* Loop over the test array and test each case. */ for (i = 0; test_set[i].path != NULL; i++) { - SVN_ERR (svn_repos_authz_check_access (cfg, "greek", + SVN_ERR (svn_repos_authz_check_access (authz, "greek", test_set[i].path, test_set[i].user, test_set[i].required, @@ -1188,6 +1204,49 @@ } } + + /* The authz rules for the phase 2 tests, first case (cyclic + dependency). */ + contents = + "[groups]" + APR_EOL_STR + "slaves = cooks,scribes,@gladiators" + APR_EOL_STR + "gladiators = equites,thraces,@slaves" + APR_EOL_STR + APR_EOL_STR + "[greek:/A]" + APR_EOL_STR + "@slaves = r" + APR_EOL_STR; + + /* Load the test authz rules and check that group cycles are + reported. */ + err = authz_get_handle (&authz, contents, subpool); + if (!err || err->apr_err != SVN_ERR_AUTHZ_INVALID_CONFIG) + return svn_error_createf (SVN_ERR_TEST_FAILED, err, + "Got %s error instead of expected " + "SVN_ERR_AUTHZ_INVALID_CONFIG", + err ? "unexpected" : "no"); + svn_error_clear (err); + + /* The authz rules for the phase 2 tests, second case (missing group + definition). */ + contents = + "[greek:/A]" + APR_EOL_STR + "@senate = r" + APR_EOL_STR; + + /* Check that references to undefined groups are reported. */ + err = authz_get_handle (&authz, contents, subpool); + if (!err || err->apr_err != SVN_ERR_AUTHZ_INVALID_CONFIG) + return svn_error_createf (SVN_ERR_TEST_FAILED, err, + "Got %s error instead of expected " + "SVN_ERR_AUTHZ_INVALID_CONFIG", + err ? "unexpected" : "no"); + svn_error_clear (err); + /* That's a wrap! */ svn_pool_destroy (subpool); return SVN_NO_ERROR;