ghudson@tigris.org writes:
> --- trunk/subversion/include/svn_repos.h (original)
> +++ trunk/subversion/include/svn_repos.h Sat Jul 23 15:38:52 2005
> @@ -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);
> +
These need "@since New in 1.3" tags.
Also, instead of describing the implementation of
svn_repos_authz_read(), maybe just say "If FILE is not a valid authz
rule file, then return SVN_ERR_AUTHZ_INVALID_CONFIG, and the effect on
*AUTHZP is undefined" or something like that.
> /**
> - * 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,
This needs a "@since" tag too, though it was added in r15242 not
r15400.
(I'll add the @since tags, no need to do anything about that.)
> Modified: trunk/subversion/libsvn_repos/authz.c
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_repos/authz.c?rev=15400&p1=trunk/subversion/libsvn_repos/authz.c&p2=trunk/subversion/libsvn_repos/authz.c&r1=15399&r2=15400
> ==============================================================================
> --- trunk/subversion/libsvn_repos/authz.c (original)
> +++ trunk/subversion/libsvn_repos/authz.c Sat Jul 23 15:38:52 2005
> @@ -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,6 +42,21 @@
> svn_boolean_t access;
> };
>
> +struct authz_validate_baton {
> + apr_pool_t *pool;
> + svn_config_t *config;
> + svn_error_t *err;
> +};
We typically document internal structures too. What is the structure
for overall, and what does each field mean?
> +
> +/* 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;
> +};
It's no big deal, but the first sentence of that comment just
duplicates what the public documentation says. If the comment
consisted of just the second sentence alone, then it would be
immediately obvious that you are merely making an implementation
clarification.
>
> /* Determine whether the required access is granted given what authz
> @@ -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);
> +
The doc string for svn_config_get() doesn't say the last parameter
('default_value') can be NULL. svn_config_get() may pass it on to
expand_option_value(), which also doesn't say whether its
corresponding parameter ('opt_value') can be NULL. From looking at
the implementation, I think NULL is in fact safe for 'opt_value' in
expand_option_value(), but it's subtle -- take a look.
Anyway, if we're going to depend on the behavior, then it should be
documented all the way down.
> 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);
> -}
Heh, too bad the diff worked out this way: it *looks* like you renamed
authz_group_contains_user_internal() to authz_group_contains_user(),
and removed one parameter. But of course, your description in the log
message:
> (authz_group_contains_user_internal): Remove function.
> (authz_group_contains_user): Remove useless protection against
> invalid configurations.
is accurate. Too bad authz_group_contains_user() isn't documented,
but that was true before you got here.
> -
> /* 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)
This doc string doesn't say what each parameter is for (specifically,
the reader can't immediately tell what checked_groups is for, nor what
contents it ought to have, although I'm guessing it's for recursive
calls and should start out empty :-) ).
This isn't really Dave's fault -- there are a lot of missing or
inadequate (in the sense of not saying what each parameter does) doc
strings in this file. However, let's try not to make the problem
bigger.
Internal functions generally use ALL_CAPS for parameter names, instead
of using Doxygen markup (yeah, we know C is case-sensitive, but this
is a good way to make the parameters stand out in non-processed doc
strings, and anyway we shouldn't ever have variable names that differ
only by case). So one might rewrite this as:
/* Check for errors in GROUP's definition in CFG. The errors
* detected are references to non-existent groups and circular
* dependencies between groups. If errors are present, return
* SVN_ERR_AUTHZ_INVALID_CONFIG. Use POOL for temporary
* allocations only.
*
* CHECKED_GROUPS should be empty (it is used for recursive calls).
*/
Now a reader knows exactly how to call it, and exactly how it behaves.
> +{
> + const char *value;
> + apr_array_header_t *list;
> + int i;
> +
> + svn_config_get (cfg, &value, "groups", group, NULL);
Same comments about passing NULL to svn_config_get().
> + /* 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;
> +}
We have a general problem (well, problemlet) in Subversion that when
iteration is done via recursion, our usual loop subpool idiom doesn't
work. I don't have anything constructive to suggest, though:
recursion is obviously the right strategy here, and the acrobatics
necessary to get bounded subpool usage wouldn't be worth it, given the
size of most config files.
> +
> +
> +/* 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)
Same comments about doc string.
> +{
> + 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);
Any reason to wrap that line there?
(And, my usual comment about 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)
Same comments about doc string.
> +{
> + 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)
Same comments about doc string.
> +{
> + 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;
> +}
There now exists svn_config_enumerate_sections2(), the older
function is deprecated.
In most places, we use "_p" instead of just "p" as the suffix for
return-by-reference pointers. As we discussed in IRC, you just
happened to see some exceptional code (e.g., "configp") that led you
to omit the underscore. No big deal, just noting it for next time.
I don't think you meant "aborting on error", by the way. Returning an
error is not the same as aborting.
> +
> +
> +
> +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;
> }
The way svn_path_split() works, splitting "/" results in
current_path ==> "/"
base_name ==> "/"
Since the call to authz_get_path_access() is used as the conditional
to the 'while' loop, you could do away with base_name entirely (just
pass NULL) and use current_path for the return check, no?
> - } 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;
> }
>
> Modified: trunk/subversion/mod_authz_svn/mod_authz_svn.c
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/mod_authz_svn/mod_authz_svn.c?rev=15400&p1=trunk/subversion/mod_authz_svn/mod_authz_svn.c&p2=trunk/subversion/mod_authz_svn/mod_authz_svn.c&r1=15399&r2=15400
> ==============================================================================
> --- trunk/subversion/mod_authz_svn/mod_authz_svn.c (original)
> +++ trunk/subversion/mod_authz_svn/mod_authz_svn.c Sat Jul 23 15:38:52 2005
> @@ -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);
Nice job keeping the space-before-paren style in each file. (And
sorry that that distracting burden exists at all.)
> if (svn_err) {
> ap_log_rerror(APLOG_MARK, APLOG_ERR,
> /* If it is an error code that APR can make sense
>
> Modified: trunk/subversion/tests/libsvn_repos/repos-test.c
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/tests/libsvn_repos/repos-test.c?rev=15400&p1=trunk/subversion/tests/libsvn_repos/repos-test.c&p2=trunk/subversion/tests/libsvn_repos/repos-test.c&r1=15399&r2=15400
> ==============================================================================
> --- trunk/subversion/tests/libsvn_repos/repos-test.c (original)
> +++ trunk/subversion/tests/libsvn_repos/repos-test.c Sat Jul 23 15:38:52 2005
> @@ -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)
> +{
This doesn't return an "svn_config_t handle", it returns an error :-).
Anyway, it's an svn_authz_t, not an svn_config_t. But easiest to just
say "Set *AUTHZP to a representation of...".
> + 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_cfg;
> + 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");
> -
> - /* 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");
> + /* Load the test authz rules. */
> + SVN_ERR (authz_get_handle(&authz_cfg, contents, subpool));
>
> /* 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_cfg, "greek",
> test_set[i].path,
> test_set[i].user,
> test_set[i].required,
> @@ -1187,6 +1203,49 @@
> test_set[i].user : "-");
> }
> }
> +
> +
> + /* 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_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 =
> + "[greek:/A]"
> + APR_EOL_STR
> + "@senate = r"
> + APR_EOL_STR;
> +
> + /* Check that references to undefined groups are reported. */
> + err = authz_get_handle (&authz_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);
>
> /* That's a wrap! */
> svn_pool_destroy (subpool);
Nice job upgrading the tests like that!
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Jul 26 20:49:15 2005