No nits to pick, just wanted to say that as a reviewer I found this
change exceptionally comprehensible. Thanks for the nice scalpel
work, David.
-Karl
danderson@tigris.org writes:
> Author: danderson
> Date: Sun Sep 4 21:45:39 2005
> New Revision: 16048
>
> Modified:
> trunk/subversion/include/svn_repos.h
> trunk/subversion/libsvn_repos/commit.c
> trunk/subversion/mod_authz_svn/mod_authz_svn.c
> trunk/subversion/svnserve/serve.c
>
> Log:
> Fix issue #2388 (fix started in r15985). The issue was that users
> with no write access whatsoever to a repository could commit empty
> revisions and/or leave active transactions lying around.
>
> * subversion/include/svn_repos.h
> (svn_repos_authz_callback_t): Update docstring to document the case
> of global access lookups. Correct references to non-existent
> required_access parameter.
>
> * subversion/libsvn_repos/commit.c
> (svn_repos_get_commit_editor3): Do a global access lookup before
> authorizing the creation of a commit editor.
>
> * subversion/svnserve/serve.c
> (authz_check_access): Change tests to let NULL paths through to the
> authz layer.
>
> * subversion/mod_authz_svn/mod_authz_svn.c
> (req_check_access): Perform global access lookups if a request maps
> to writing on a NULL path. Update comment to explain.
>
>
> Modified: trunk/subversion/include/svn_repos.h
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/include/svn_repos.h?rev=16048&p1=trunk/subversion/include/svn_repos.h&p2=trunk/subversion/include/svn_repos.h&r1=16047&r2=16048
> ==============================================================================
> --- trunk/subversion/include/svn_repos.h (original)
> +++ trunk/subversion/include/svn_repos.h Sun Sep 4 21:45:39 2005
> @@ -97,12 +97,18 @@
> /** Callback type for checking authorization on paths produced by
> * the repository commit editor.
> *
> - * Set @a *allowed to TRUE to indicate that the @a required_access on
> + * Set @a *allowed to TRUE to indicate that the @a required access on
> * @a path in @a root is authorized, or set it to FALSE to indicate
> * unauthorized (presumable according to state stored in @a baton).
> *
> + * If @a path is NULL, the callback should perform a global authz
> + * lookup for the @a required access. That is, the lookup should
> + * check if the @a required access is granted for at least one path of
> + * the repository, and set @a *allowed to TRUE if so. @a root may
> + * also be NULL if @a path is NULL.
> + *
> * This callback is very similar to svn_repos_authz_func_t, with the
> - * exception of the addition of the @a required_access parameter.
> + * exception of the addition of the @a required parameter.
> * This is due to historical reasons: when authz was first implemented
> * for svn_repos_dir_delta(), it seemed there would need only checks
> * for read and write operations, hence the svn_repos_authz_func_t
>
> Modified: trunk/subversion/libsvn_repos/commit.c
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_repos/commit.c?rev=16048&p1=trunk/subversion/libsvn_repos/commit.c&p2=trunk/subversion/libsvn_repos/commit.c&r1=16047&r2=16048
> ==============================================================================
> --- trunk/subversion/libsvn_repos/commit.c (original)
> +++ trunk/subversion/libsvn_repos/commit.c Sun Sep 4 21:45:39 2005
> @@ -777,9 +777,26 @@
> void *authz_baton,
> apr_pool_t *pool)
> {
> - svn_delta_editor_t *e = svn_delta_default_editor (pool);
> + svn_delta_editor_t *e;
> apr_pool_t *subpool = svn_pool_create (pool);
> - struct edit_baton *eb = apr_pcalloc (subpool, sizeof (*eb));
> + struct edit_baton *eb;
> +
> + /* Do a global authz access lookup. Users with no write access
> + whatsoever to the repository don't get a commit editor. */
> + if (authz_callback)
> + {
> + svn_boolean_t allowed;
> +
> + authz_callback (svn_authz_write, &allowed, NULL, NULL,
> + authz_baton, pool);
> + if (!allowed)
> + return svn_error_create(SVN_ERR_AUTHZ_UNWRITABLE, NULL,
> + "Not authorized to open a commit editor.");
> + }
> +
> + /* Allocate the structures. */
> + e = svn_delta_default_editor (pool);
> + eb = apr_pcalloc (subpool, sizeof (*eb));
>
> /* Set up the editor. */
> e->open_root = open_root;
>
> 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=16048&p1=trunk/subversion/mod_authz_svn/mod_authz_svn.c&p2=trunk/subversion/mod_authz_svn/mod_authz_svn.c&r1=16047&r2=16048
> ==============================================================================
> --- trunk/subversion/mod_authz_svn/mod_authz_svn.c (original)
> +++ trunk/subversion/mod_authz_svn/mod_authz_svn.c Sun Sep 4 21:45:39 2005
> @@ -257,11 +257,21 @@
> * the DAV RA method: some requests have no repos_path, but apache
> * still triggers an authz lookup for the URI.
> *
> + * However, if repos_path == NULL and the request requires write
> + * access, then perform a global authz lookup. The request is
> + * denied if the user commiting isn't granted any access anywhere
> + * in the repository. This is to avoid operations that involve no
> + * paths (commiting an empty revision, leaving a dangling
> + * transaction in the FS) being granted by default, letting
> + * unauthenticated users write some changes to the repository.
> + * This was issue #2388.
> + *
> * XXX: For now, requesting access to the entire repository always
> * XXX: succeeds, until we come up with a good way of figuring
> * XXX: this out.
> */
> - if (repos_path)
> + if (repos_path
> + || (!repos_path && (authz_svn_type & svn_authz_write)))
> {
> svn_err = svn_repos_authz_check_access(access_conf, repos_name,
> repos_path, r->user,
>
> Modified: trunk/subversion/svnserve/serve.c
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/svnserve/serve.c?rev=16048&p1=trunk/subversion/svnserve/serve.c&p2=trunk/subversion/svnserve/serve.c&r1=16047&r2=16048
> ==============================================================================
> --- trunk/subversion/svnserve/serve.c (original)
> +++ trunk/subversion/svnserve/serve.c Sun Sep 4 21:45:39 2005
> @@ -118,7 +118,7 @@
> as the default policy when authz is performed on a path with no
> rules. In the latter case, the default is to deny access, and is
> set by svn_repos_authz_check_access. */
> - if (!b->authzdb || !path)
> + if (!b->authzdb)
> {
> *allowed = TRUE;
> return SVN_NO_ERROR;
> @@ -130,7 +130,7 @@
> absolute path. Passing such a malformed path to the authz
> routines throws them into an infinite loop and makes them miss
> ACLs. */
> - if (*path != '/')
> + if (path && *path != '/')
> path = svn_path_join("/", path, pool);
>
> return svn_repos_authz_check_access(b->authzdb, b->authz_repos_name,
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: svn-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: svn-help@subversion.tigris.org
>
--
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Sep 20 03:51:01 2005