[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Error checking in authz

From: David Anderson <david.anderson_at_calixo.net>
Date: 2005-07-23 18:40:21 CEST

Greg Hudson wrote:
> *cfgp should be *authzp.
> cfg should have been changed to authz.


> As far as I can tell, you still have the cyclic dependency from the
> first case in these rules, making this a fragile test.

Oops. Well spotted. In my defense, the tests still succeed when the
right thing is tested consistently :-).

> Why didn't you leave this returning a boolean? You seem to have left
> in some of the internal structure for returning errors from
> svn_repos_authz_check_access(), which is unnecessary gunk in the
> current implementation since it never actually returns an error.
> Perhaps you reasoned that the internals of authz.c should leave the
> door open to returning errors in the future, in which case I do not
> agree. While I think the API should leave the door open to that
> possibility, any good implementation will not return errors; thus,
> the internal logic does not need to contain gunk to that end.

Okay. You are right, I'd left error throwing in the internal
implementation in prevision of possible future error throwing, which
is useless for the time being.

> I advise reverting the function signature here, reverting the
> addition of the err field to the baton, reverting the call site in
> authz_parse_line(), reverting the signature of authz_get_tree_access
> and the changes to that function, reverting the signature of
> authz_get_path_access and the changes to that function, and reverting
> the call sites to the two get_*_access functions in
> svn_repos_authz_check_access().

Done. As the error field is still necessary to the validation
functions, I've split authz_baton into authz_lookup_baton and

> The rest of the patch (the validation checking) looks great. One
> more round and I think we should be good.

Hopefully you're right. Thanks for reviewing.

- Dave.

Add a function which opens and verifies that an authz configuration
file does not contain any logic errors prior to any lookups.

* subversion/include/svn_error_codes.h: New error. Update copyright

* subversion/include/svn_repos.h: Remove useless include.
   (svn_authz_t): New opaque data type.
   (svn_repos_authz_read): New public API.
   (svn_repos_authz_check_access): Fix docstring typo. Use a
     svn_authz_t* instead of a svn_config_t*.

* subversion/libsvn_repos/authz.c
     Use svn_authz_t instead of svn_config_t for handles to authz
   (authz_baton): Rename.
   (authz_lookup_baton): Renamed from authz_baton. All references
   (authz_validate_baton): New baton for use in validation routines.
   (authz_group_contains_user_internal): Remove function.
   (authz_group_contains_user): Remove useless protection against
     invalid configurations.
   (authz_get_path_access): Rewrite docstring.
   (authz_get_tree_access): Return the access status directly.
   (authz_group_walk, authz_validate_rule,
    authz_validate_group, authz_validate_section):
     New internal functions.
   (svn_repos_authz_read): New function.
   (svn_repos_authz_check_access): Change calls to altered internal

* subversion/tests/libsvn_repos/repos-test.c
     Use svn_authz_t instead of svn_config_t for handles to authz
   (authz_get_handle): New function. Factor out the converting a char*
     authz file contents to a parsed svn_authz_t* from the authz test
   (authz): Add a second batch of tests to ensure that invalid authz
     configurations are caught during the authz loading.

* subversion/mod_authz_svn/mod_authz_svn.c
     Use svn_authz_t instead of svn_config_t for handles to authz
   (req_check_access): replace call to svn_config_read with a call to

To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Received on Sat Jul 23 18:41:27 2005

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.