=== trunk/subversion/mod_authz_svn/mod_authz_svn.c ================================================================== --- trunk/subversion/mod_authz_svn/mod_authz_svn.c (revision 117) +++ trunk/subversion/mod_authz_svn/mod_authz_svn.c (revision 118) @@ -227,8 +227,9 @@ 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 === trunk/subversion/include/svn_repos.h ================================================================== --- trunk/subversion/include/svn_repos.h (revision 117) +++ trunk/subversion/include/svn_repos.h (revision 118) @@ -1689,6 +1689,19 @@ svn_authz_recursive = 4 } svn_repos_authz_access_t; +/** Read authz configuration data from @a file (a file or registry + * path) into @a *cfgp, 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_config_t **cfgp, 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 === trunk/subversion/tests/libsvn_repos/repos-test.c ================================================================== --- trunk/subversion/tests/libsvn_repos/repos-test.c (revision 117) +++ trunk/subversion/tests/libsvn_repos/repos-test.c (revision 118) @@ -1059,7 +1059,7 @@ 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), + SVN_ERR_W (svn_repos_authz_read (cfg, authz_file_path, TRUE, pool), "Opening test authz file"); /* Close the temporary descriptor, which'll delete the file. */ @@ -1127,9 +1127,9 @@ * 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. + * 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 phase 1 tests. */ @@ -1205,7 +1205,8 @@ } - /* The authz rules for the phase 2 tests. */ + /* The authz rules for the phase 2 tests, first case (cyclic + dependancy). */ contents = "[groups]" APR_EOL_STR @@ -1217,31 +1218,35 @@ "[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); + /* Load the test authz rules and check that group cycles are + reported. */ + err = authz_get_handle(&cfg, 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 = + "[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 + "@senate = r" + APR_EOL_STR; /* Check that references to undefined groups are reported. */ - err = svn_repos_authz_check_access(cfg, "greek", "/iota", "plato", - svn_authz_read, &access_granted, - subpool); + err = authz_get_handle(&cfg, 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 " === trunk/subversion/libsvn_repos/authz.c ================================================================== --- trunk/subversion/libsvn_repos/authz.c (revision 117) +++ trunk/subversion/libsvn_repos/authz.c (revision 118) @@ -360,7 +360,106 @@ +/* Callback to check wether 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_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 (!value) + { + b->error = 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 wether 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_baton *b = baton; + svn_boolean_t is_in_group; + + /* Ask wether the defined group contains the empty user, which will + walk the entire group definition, detecting potential cyclic + dependancies as it goes. */ + b->error = authz_group_contains_user (b->config, name, "", + &is_in_group, b->pool); + if (b->error) + 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_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->error) + return FALSE; + + return TRUE; +} + + + svn_error_t * +svn_repos_authz_read (svn_config_t **cfgp, const char *file, + svn_boolean_t must_exist, apr_pool_t *pool) +{ + struct authz_baton baton = { 0 }; + + baton.pool = pool; + baton.error = SVN_NO_ERROR; + + /* Load the rule file. */ + SVN_ERR (svn_config_read (cfgp, file, must_exist, pool)); + baton.config = *cfgp; + + /* Step through the entire rule file, aborting on error. */ + svn_config_enumerate_sections(*cfgp, authz_validate_section, + &baton); + SVN_ERR (baton.error); + + return SVN_NO_ERROR; +} + + + +svn_error_t * svn_repos_authz_check_access (svn_config_t *cfg, const char *repos_name, const char *path, const char *user, svn_repos_authz_access_t required_access,