Index: subversion/include/svn_error_codes.h =================================================================== --- subversion/include/svn_error_codes.h (revision 15244) +++ subversion/include/svn_error_codes.h (working copy) @@ -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 */ Index: subversion/tests/libsvn_repos/repos-test.c =================================================================== --- subversion/tests/libsvn_repos/repos-test.c (revision 15244) +++ subversion/tests/libsvn_repos/repos-test.c (working copy) @@ -1031,6 +1031,46 @@ } +/* 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_config_t **cfg, 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_config_read (cfg, 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 +1078,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_error_t *err; svn_boolean_t access_granted; apr_pool_t *subpool = svn_pool_create (pool); int i; @@ -1080,15 +1117,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 dependancy in groups + * and a reference to an undefined group, then perform authz lookups + * that will run into these two problems. Check that errors are + * indeed reported by the authz layer. */ - /* The authz rules for the test. */ + /* The authz rules for the phase 1 tests. */ contents = "[greek:/A]" APR_EOL_STR @@ -1129,32 +1172,9 @@ 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, - 0, 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(&cfg, 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"); - - SVN_ERR_W (svn_config_read (&cfg, authz_file_path, TRUE, subpool), - "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"); - /* Loop over the test array and test each case. */ for (i = 0; test_set[i].path != NULL; i++) { @@ -1183,6 +1203,51 @@ } } + + /* The authz rules for the phase 2 tests. */ + 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 + APR_EOL_STR + "[greek:/iota]" + APR_EOL_STR + "@senate = rw" + APR_EOL_STR; + + /* Load the test authz rules. */ + SVN_ERR (authz_get_handle(&cfg, contents, subpool)); + + /* Check that group cycles are reported. */ + err = svn_repos_authz_check_access(cfg, "greek", "/A", "plato", + svn_authz_read, &access_granted, + 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); + + /* Check that references to undefined groups are reported. */ + err = svn_repos_authz_check_access(cfg, "greek", "/iota", "plato", + svn_authz_read, &access_granted, + 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; Index: subversion/libsvn_repos/authz.c =================================================================== --- subversion/libsvn_repos/authz.c (revision 15244) +++ subversion/libsvn_repos/authz.c (working copy) @@ -29,6 +29,7 @@ struct authz_baton { apr_pool_t *pool; svn_config_t *config; + svn_error_t *error; const char *user; svn_repos_authz_access_t allow; @@ -88,10 +89,11 @@ } -static svn_boolean_t +static svn_error_t * authz_group_contains_user_internal (svn_config_t *cfg, const char *group, const char *user, + svn_boolean_t *is_in_group, apr_hash_t *checked_groups, apr_pool_t *pool) { @@ -99,7 +101,16 @@ apr_array_header_t *list; int i; - svn_config_get (cfg, &value, "groups", group, ""); + 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++) @@ -109,38 +120,51 @@ /* 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? */ + /* A circular dependancy 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)) - continue; + return svn_error_createf(SVN_ERR_AUTHZ_INVALID_CONFIG, + NULL, + "Circular dependancy 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. */ - if (authz_group_contains_user_internal (cfg, &group_user[1], - user, checked_groups, - pool)) - return TRUE; + SVN_ERR (authz_group_contains_user_internal (cfg, + &group_user[1], + user, + is_in_group, + checked_groups, + pool)); + if (*is_in_group) + return SVN_NO_ERROR; } /* If the user matches, stop. */ else if (strcmp (user, group_user) == 0) - return TRUE; + { + *is_in_group = TRUE; + return SVN_NO_ERROR; + } } - return FALSE; + *is_in_group = FALSE; + return SVN_NO_ERROR; } -static svn_boolean_t +static svn_error_t * authz_group_contains_user(svn_config_t *cfg, const char *group, - const char *user, apr_pool_t *pool) + const char *user, svn_boolean_t *is_in_group, + apr_pool_t *pool) { return authz_group_contains_user_internal (cfg, group, user, + is_in_group, apr_hash_make (pool), pool); } @@ -165,8 +189,19 @@ /* Group rule and user not in group. Stop. */ if (*name == '@') { - if (!authz_group_contains_user (b->config, &name[1], - b->user, b->pool)) + svn_boolean_t in_group; + svn_error_t *group_err; + + group_err = authz_group_contains_user (b->config, &name[1], + b->user, &in_group, + b->pool); + if (group_err) + { + /* Order our caller to stop parsing rules. */ + b->error = group_err; + return FALSE; + } + if (!in_group) return TRUE; } /* User rule for wrong user. Stop. */ @@ -211,6 +246,10 @@ svn_config_enumerate (b->config, section_name, authz_parse_line, b); + /* Errors abort any further processing. */ + if (b->error) + return FALSE; + /* Has the section explicitely determined an access? */ conclusive = authz_access_is_determined (b->allow, b->deny, b->required_access); @@ -231,15 +270,16 @@ * 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 and access_determined to inform the caller of + * the outcome of the lookup. If an error is returned, the value of + * these two variables is undefined. */ -static svn_boolean_t +static svn_error_t * authz_get_path_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, + svn_boolean_t *access_determined, apr_pool_t *pool) { const char *qualified_path; @@ -248,27 +288,37 @@ baton.pool = pool; baton.config = cfg; baton.user = user; + baton.error = SVN_NO_ERROR; /* Try to locate a repository-specific block first. */ qualified_path = apr_pstrcat (pool, repos_name, ":", path, NULL); svn_config_enumerate (cfg, qualified_path, authz_parse_line, &baton); + if (baton.error) + return baton.error; *access_granted = authz_access_is_granted (baton.allow, baton.deny, required_access); /* If the first test has determined access, stop now. */ - if (authz_access_is_determined (baton.allow, baton.deny, - required_access)) - return TRUE; + *access_determined = authz_access_is_determined (baton.allow, + baton.deny, + required_access); + if (*access_determined) + return SVN_NO_ERROR; /* No repository specific rule, try pan-repository rules. */ svn_config_enumerate (cfg, path, authz_parse_line, &baton); + if (baton.error) + return baton.error; *access_granted = authz_access_is_granted (baton.allow, baton.deny, required_access); - return authz_access_is_determined (baton.allow, baton.deny, - required_access); + *access_determined = authz_access_is_determined (baton.allow, + baton.deny, + required_access); + + return SVN_NO_ERROR; } @@ -279,9 +329,11 @@ * 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 in access_granted. If + * an error is thrown during the authz lookup, access_granted is left + * untouched. */ -static void +static svn_error_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, @@ -293,6 +345,7 @@ baton.pool = pool; baton.config = cfg; baton.user = user; + baton.error = SVN_NO_ERROR; baton.required_access = required_access; baton.repos_path = path; baton.qualified_repos_path = apr_pstrcat (pool, repos_name, @@ -301,8 +354,11 @@ baton.access = TRUE; svn_config_enumerate_sections (cfg, authz_parse_section, &baton); + if (baton.error) + return baton.error; *access_granted = baton.access; + return SVN_NO_ERROR; } @@ -321,10 +377,12 @@ /* Determine the granted access for the requested path. */ do { - access_determined = authz_get_path_access (cfg, repos_name, - current_path, user, - required_access, - access_granted, pool); + SVN_ERR(authz_get_path_access (cfg, repos_name, + current_path, user, + required_access, + access_granted, + &access_determined, + pool)); if (!access_determined) { @@ -345,11 +403,10 @@ /* 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)) + SVN_ERR (authz_get_tree_access (cfg, repos_name, path, user, + required_access, access_granted, + pool)); return SVN_NO_ERROR; }