Re: [PATCH] Authz support in Svnserve

Since I know you like thoroughness, I'll include nits of all sizes in
this review (some of which may duplicate what Greg Hudson already
noticed). Although I lead off with some silly grammar mistakes,
please note that there are more weighty comments later on.

David Anderson <david.anderson@calixo.net> writes:
> [[[
> Make svnserve enforce path-based access control configuration during the
> processing of client operations.
>
> * notes/authz_policy.txt: Update the list of functions that take authz
> callbacks. Update the list of operations that require manual
> authz lookups.
>
> * subversion/include/svn_config.h
> (SVN_CONFIG_OPTION_AUTHZ_DB): New define.
>
> * subversion/libsvn_repos/repos.c
> (create_conf): update the contents svnserve.conf to document the new
> configuration option for authz. Create a 'authz' configuration
> file with documentation and a few authz setting examples.

Missing word "of"; "a" should be "an".

> * subversion/libsvn_repos/repos.c
> (SVN_REPOS__CONF_AUTHZ): New define.
>
> * subversion/svnserve/serve.c
> (server_baton_t): Add a handle to the authz configuration and the
> repository name for authz lookups.

This sentence would be unambiguous if it read "Add an authz
configuration handle, and the repository name, both for authz lookups"
or something like that. (I parsed it twice and then finally went to
look at the code to make sure I'd understood it right.)

> (authz_check_access, authz_check_access_cb, lookup_access): New
> functions.
> (must_have_access): Add optional authz lookups if a specific path is
> passed. All callers updated.
> (change_rev_prop): Check for write access to the repository and pass
> an authz read callback to svn_repos_fs_change_rev_prop2.

I have to admit, it confused me to see that we're passing a "read
callback" to something called svn_repos_fs_change_rev_prop2 :-). I'll
see if I can figure that out when I get to the code...

Didn't the function 'must_have_write_access' disappear, to be replaced
by 'must_have_access'? If so, mention the disappeared symbol's name,
please. (I'll have more comments about this in the code review.)

> (accept_report, rev_proplist, rev_prop, log_cmd, get_locations,
> get_file_revs): Pass an authz read callback to libsvn_repos
> functions.

Here "read callback" made immediate sense, of course.

> (add_lock_tokens): Use the svn RA utility functions to parse the
> client command. Perform authz lookups during the processing of
> lock tokens.

You only use one utility function, svn_ra_svn_parse_tuple, right?
(I.e., a singular vs plural confusion.)

> (commit): Pass an authz read/write callback to the commit editor.
> (get_file, get_dir, check_path, stat, lock, lock_many, unlock,
> unlock_many, get_lock, get_locks): Enforce authz access control on
> the path(s) touched by the commands.
> (find_repos): Read the authz configuration if present and add the
> necessary information to the server baton.

Okay, log message looks great (very minor nits notwithstanding).

On to the code:

> --- subversion/libsvn_repos/repos.c (/online) (revision 443)
> +++ subversion/libsvn_repos/repos.c (/local/authz-in-svnserve) (revision 443)
> @@ -1392,6 +1392,20 @@
> APR_EOL_STR
> "# password-db = passwd"
> APR_EOL_STR
> + "### The authz-db option controls the location of the authorization"
> + APR_EOL_STR
> + "### rules for path-based access control. Unless you specify a path"
> + APR_EOL_STR

Please do two spaces after a sentence-ending period above (as you do
below).

> + "### starting with a /, the file's location is relative to the conf"
> + APR_EOL_STR
> + "directory. If you don't specify an authz-db, no path-based access"
> + APR_EOL_STR
> + "control is done."
> + APR_EOL_STR
> + "### Uncomment the line below to use the default authorization file."
> + APR_EOL_STR
> + "# authz-db = authz"
> + APR_EOL_STR
> "### This option specifies the authentication realm of the repository."
> APR_EOL_STR
> "### If two repositories have the same authentication realm, they should"
> @@ -1433,7 +1447,53 @@
> _("Creating passwd file"));
> }

That looks good. Using "authz" as the default file name seems
reasonable; the documentation for mod_authz_svn seems to just say
"/path/to/access/file" for that, so there's no great gain to choosing
a compatible file name.

However, perhaps the descriptive text above should mention that the
file format is the same as that used by mod_authz_svn in http://
access, and that in fact the two servers can share the same file.
That's something a reader of the config file would want to know.

> + {
> + static const char * const authz_contents =
> + "### This file is an example authorization file for svnserve."
> + APR_EOL_STR

Same thing here -- since the format is compatible w/ mod_authz_svn,
say so.

> + "### Its format is similar to that of svnserve.conf. As shown below,"
> + APR_EOL_STR
> + "### each section defines authorizations for the path and (optional)"
> + APR_EOL_STR
> + "### repository specified by the section name."
> + APR_EOL_STR
> + "### The authorizations follow. An authorization line can refer to a"
> + APR_EOL_STR
> + "### single user, to a group of users defined in a special [groups]"
> + APR_EOL_STR
> + "### section, or to anyone using the '*' wildcard. Each definition can"
> + APR_EOL_STR
> + "### grant read ('r') access, read-write ('rw') access, or no access"
> + APR_EOL_STR
> + "### ('')."
> + APR_EOL_STR
> + APR_EOL_STR
> + "# [groups]"
> + APR_EOL_STR
> + "# harry_and_sally = harry,sally"
> + APR_EOL_STR
> + APR_EOL_STR
> + "# [/foo/bar]"
> + APR_EOL_STR
> + "# harry = rw"
> + APR_EOL_STR
> + "# * ="
> + APR_EOL_STR
> + APR_EOL_STR
> + "# [repository:/baz/fuz]"
> + APR_EOL_STR
> + "# @harry_and_sally = rw"
> + APR_EOL_STR
> + "# * = r"
> + APR_EOL_STR;
>
> + SVN_ERR_W (svn_io_file_create (svn_path_join (repos->conf_path,
> + SVN_REPOS__CONF_AUTHZ,
> + pool),
> + authz_contents, pool),
> + _("Creating authz file"));
> + }
> +
> return SVN_NO_ERROR;
> }
>
> === subversion/libsvn_repos/repos.h
> ==================================================================
> --- subversion/libsvn_repos/repos.h (/online) (revision 443)
> +++ subversion/libsvn_repos/repos.h (/local/authz-in-svnserve) (revision 443)
> @@ -70,6 +70,7 @@
> /* In the repository conf directory, look for these files. */
> #define SVN_REPOS__CONF_SVNSERVE_CONF "svnserve.conf"
> #define SVN_REPOS__CONF_PASSWD "passwd"
> +#define SVN_REPOS__CONF_AUTHZ "authz"

This isn't really due to your change, but I find both the comment and
the names of these constants a bit misleading. These files are just
what is looked for *by default*, right? Depending on how the admin
configures things for run-time, totally different files might be
looked for. I wish the comment and the names expressed this better.

> /* The Repository object, created by svn_repos_open() and
> svn_repos_create(), allocated in POOL. */
> === subversion/svnserve/serve.c
> ==================================================================
> --- subversion/svnserve/serve.c (/online) (revision 443)
> +++ subversion/svnserve/serve.c (/local/authz-in-svnserve) (revision 443)
> @@ -47,6 +47,8 @@
> svn_fs_t *fs; /* For convenience; same as svn_repos_fs(repos) */
> svn_config_t *cfg; /* Parsed repository svnserve.conf */
> svn_config_t *pwdb; /* Parsed password database */
> + svn_authz_t *authzdb; /* Parsed authz rules */
> + const char *repos_name; /* The name of the repository (for authz) */
> const char *realm; /* Authentication realm */
> const char *repos_url; /* URL to base of repository */
> const char *fs_path; /* Decoded base path inside repository */

Agree w/ Greg's comment that 'authz_repos_name' would be better here.

> @@ -102,6 +104,74 @@
>
> /* --- AUTHENTICATION AND AUTHORIZATION FUNCTIONS --- */
>
> +/* Set ALLOWED to TRUE if PATH is accessible in the REQUIRED mode to
> + the user described in BATON according to the authz rules in BATON.
> + Use POOL for temporary allocations only. If no authz rules are
> + present in BATON, grant access by default to emulate complete
> + absence of authz code. */
> +static svn_error_t *authz_check_access(svn_boolean_t *allowed,
> + const char *path,
> + svn_repos_authz_access_t required,
> + server_baton_t *b,
> + apr_pool_t *pool)

"Set *ALLOWED to TRUE if PATH is..." (that is, don't forget the "*").

> +{
> + /* If authz cannot be performed, grant access. This is NOT the same
> + 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)
> + {
> + *allowed = TRUE;
> + return SVN_NO_ERROR;
> + }
> +
> + /* If the authz request is for the empty path (ie. ""), replace it
> + with the root path. This happens because of stripping done at
> + various levels in svnserve that remove the leading / on an
> + absolute path. Passing such a malformed path to the authz
> + routines throws them into an infinite loop and makes them miss
> + ACLs. */
> + if (*path == '\0')
> + path = "/";

Also like Greg, this feels a result of squishiness elsewhere in the
system to me. At least clarify what you mean by "malformed" -- it
seems like you're saying that the path is malformed w.r.t. authz if it
does not have a leading "/", right?

But more to the point, which authz routines go into an infinite loop
if passed the empty string instead of "/"? Can we fix them?

> + return svn_repos_authz_check_access(b->authzdb, b->repos_name,
> + path, b->user, required,
> + allowed, pool);
> +}
> +
> +/* Set ALLOWED to TRUE if PATH is readable by the user described in
> + * BATON. Use POOL for temporary allocations only. ROOT is not used.
> + *
> + * This is passed as a callback to many functions of svn_repos.h to
> + * check authz read access. */
> +static svn_error_t *authz_check_access_cb(svn_boolean_t *allowed,
> + svn_fs_root_t *root,
> + const char *path,
> + void *baton,
> + apr_pool_t *pool)

Again, "*ALLOWED".

This callback is implementing a predefined type, right? Always say
which type, e.g., "This implements the svn_foo_t interface." If you
do that, the final sentence of your doc string above becomes less
necessary, too.

> +{
> + server_baton_t *sb = baton;
> +
> + return authz_check_access(allowed, path, svn_authz_read, sb, pool);
> +}
> +
> +/* Set ALLOWED to TRUE if the REQUIRED access to PATH is granted,
> + * according to the state in BATON. Use POOL for temporary
> + * allocations only. ROOT is not used.
> + *
> + * This is passed as a callback to the commit editor. */
> +static svn_error_t *authz_commit_cb(svn_repos_authz_access_t required,
> + svn_boolean_t *allowed,
> + svn_fs_root_t *root,
> + const char *path,
> + void *baton,
> + apr_pool_t *pool)

Again "*ALLOWED". Same thing about naming the type it implements.

> +{
> + server_baton_t *sb = baton;
> +
> + return authz_check_access(allowed, path, required, sb, pool);
> +}
> +
> static enum access_type get_access(server_baton_t *b, enum authn_type auth)
> {
> const char *var = (auth == AUTHENTICATED) ? SVN_CONFIG_OPTION_AUTH_ACCESS :
> @@ -268,31 +338,91 @@
> return svn_ra_svn_write_cmd_response(conn, pool, "()c", "");
> }
>
> -/* Ensure that the client has write access. If the client already has
> - write access, just send a trivial auth request. Else, try to authenticate
> - the client. If NEEDS_USERNAME is TRUE, only use auth mechs that will yield
> - a username. Return an error if write access couldn't be achieved. */
> -static svn_error_t *must_have_write_access(svn_ra_svn_conn_t *conn,
> - apr_pool_t *pool, server_baton_t *b,
> - svn_boolean_t needs_username)
> +/* Ensure that the client has the REQUIRED_ACCESS by checking the
> + * blanket access directives and possibly authz directives if PATH is
> + * not NULL.
> + */
> +static svn_boolean_t lookup_access(apr_pool_t *pool,
> + server_baton_t *b,
> + svn_repos_authz_access_t required,
> + const char *path,
> + svn_boolean_t needs_username)

The parameter's name is REQUIRED, not REQUIRED_ACCESS. How is the
NEEDS_USERNAME parameter used? The doc string doesn't mention it, nor
does it mention B, though B's function can be deduced more easily
(it's the source of the "authz directives" referred to earlier,
presumably).

> {
> - if (current_access(b) == WRITE_ACCESS
> + enum access_type req = (required & svn_authz_write) ?
> + WRITE_ACCESS : READ_ACCESS;
> + svn_boolean_t authorized;
> + svn_error_t *err;
> +
> + /* Get authz's opinion on the access. If authz is disabled in the
> + server config, this will just allow access blindly. This will
> + make the function only consider blanket access rules when
> + deciding what to do. */
> + err = authz_check_access(&authorized, path, required, b, pool);

I didn't really understand the last sentence in that comment.

> +
> + /* If an error made lookup fail, deny access. ### TODO: Once
> + logging is implemented, this is a perfect place to log the
> + problem. */
> + if (err)
> + {
> + svn_error_clear(err);
> + return FALSE;
> + }

Ben Collins-Sussman appreciates the reminder :-).

> + /* If the required access is blanket-granted AND granted by
> + authz AND the request complies with NEEDS_USERNAME
> + restrictions, then the lookup has succeeded. */
> + if (current_access(b) >= req
> + && authorized
> && (! needs_username || b->user))
> + return TRUE;
> +
> + return FALSE;
> +}
> +
> +/* Check that the client has the REQUIRED_ACCESS by using
> + * lookup_access. If access is not granted, try to authenticate the
> + * client to get access. Otherwise, just send a trivial auth request
> + * and be done.
> + *
> + */
> +static svn_error_t *must_have_access(svn_ra_svn_conn_t *conn,
> + apr_pool_t *pool,
> + server_baton_t *b,
> + svn_repos_authz_access_t required,
> + const char *path,
> + svn_boolean_t needs_username)

There are a lot of parameters here; planning to document them? :-)
(Documenting by reference to another function is fine, just don't
leave the reader wondering.)

Same thing about REQUIRED vs REQUIRED_ACCESS.

Normally we don't state implementation details in a doc string, we
just state semantics. So unless there's a reason the reader needs to
know that lookup_access() is called, don't mention it.

By the way, why the extra blank line " *\n"? :-)

> +{
> + enum access_type req = (required & svn_authz_write) ?
> + WRITE_ACCESS : READ_ACCESS;
> +
> + /* See whether the user already has the required access. If so,
> + nothing needs to be done. Create the FS access and send a trivial
> + auth request. */
> + if (lookup_access(pool, b, required, path, needs_username))
> {
> SVN_ERR(create_fs_access(b, pool));
> return trivial_auth_request(conn, pool, b);
> }

Two spaces between sentences. (Or, at least, consistency :-) ).

> - /* If we can get write access by authenticating, try that. */
> - if (b->user == NULL && get_access(b, AUTHENTICATED) == WRITE_ACCESS
> + /* If the required blanket access can be obtained by authenticating,
> + try that. Unfortunately, we can't tell until after
> + authentication whether authz will work or not. We force
> + requiring a username here so that the ANONYMOUS mechanism (which
> + obviously won't work) doesn't get sent here. */
> + if (b->user == NULL
> + && get_access(b, AUTHENTICATED) >= req
> && (b->tunnel_user || b->pwdb) && b->protocol_version >= 2)
> - SVN_ERR(auth_request(conn, pool, b, WRITE_ACCESS, needs_username));
> + SVN_ERR(auth_request(conn, pool, b, req, TRUE));

Why is it obvious that ANONYMOUS won't work? Just because it's write
access? If so, that would seem like a site policy decision, not an
assumption for us to hardcode in.

> - if (current_access(b) != WRITE_ACCESS)
> + /* Now that an authentication has been done (if using protocol
> + version 2 that is), get the new take of authz on the
> + request. */
> + if (! lookup_access(pool, b, required, path, needs_username))
> return svn_error_create(SVN_ERR_RA_SVN_CMD_ERR,
> - svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> - "Connection is read-only"), NULL);
> + svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED,
> + NULL, NULL), NULL);
>
> + /* Else, access is granted, and there is much rejoicing. */
> SVN_ERR(create_fs_access(b, pool));
>
> return SVN_NO_ERROR;

My usual dour expression softened, and a hint of a smile crossed my
face. Then I quickly caught myself, took another sip of my vodka and
tonic, and continued the review as if nothing had happened.

> @@ -409,7 +539,7 @@
> SVN_CMD_ERR(svn_repos_begin_report(&report_baton, rev, b->user, b->repos,
> b->fs_path, target, tgt_path, text_deltas,
> recurse, ignore_ancestry, editor,
> - edit_baton, NULL, NULL, pool));
> + edit_baton, authz_check_access_cb, b, pool));
>
> rb.sb = b;
> rb.repos_url = svn_path_uri_decode(b->repos_url, pool);
> @@ -581,9 +711,12 @@
> svn_string_t *value;
>
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "rc?s", &rev, &name, &value));
> - SVN_ERR(must_have_write_access(conn, pool, b, FALSE));
> + SVN_ERR(must_have_write_access(conn, pool, b, svn_authz_write, NULL,
> + FALSE));

Uh...

I thought it's called must_have_access() now? And how does this code
even compile? :-)

> SVN_CMD_ERR(svn_repos_fs_change_rev_prop2(b->repos, rev, b->user,
> - name, value, NULL, NULL, pool));
> + name, value,
> + authz_check_access_cb,
> + b, pool));

Minor style nit: try to put a callback and its baton on the same line,
especially if other pairs are being kept together elsewhere in the
argument list.

> SVN_ERR(svn_ra_svn_write_cmd_response(conn, pool, ""));
> return SVN_NO_ERROR;
> }
> @@ -598,7 +731,8 @@
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "r", &rev));
> SVN_ERR(trivial_auth_request(conn, pool, b));
> SVN_CMD_ERR(svn_repos_fs_revision_proplist(&props, b->repos, rev,
> - NULL, NULL, pool));
> + authz_check_access_cb,
> + b, pool));
> SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "w((!", "success"));
> SVN_ERR(write_proplist(conn, pool, props));
> SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!))"));
> @@ -616,7 +750,7 @@
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "rc", &rev, &name));
> SVN_ERR(trivial_auth_request(conn, pool, b));
> SVN_CMD_ERR(svn_repos_fs_revision_prop(&value, b->repos, rev, name,
> - NULL, NULL, pool));
> + authz_check_access_cb, b, pool));
> SVN_ERR(svn_ra_svn_write_cmd_response(conn, pool, "(?s)", value));
> return SVN_NO_ERROR;
> }
> @@ -632,17 +766,19 @@
> return SVN_NO_ERROR;
> }
>
> -/* Add the LOCK_TOKENS to the filesystem access context if any. LOCK_TOKENS is
> - an array of svn_ra_svn_item_t structs. Return an error if they are
> - not a list of lists. */
> -static svn_error_t *add_lock_tokens(apr_array_header_t *lock_tokens,
> +/* Add the LOCK_TOKENS (if any) to the filesystem access context,
> + checking path authorizations as we go. LOCK_TOKENS is an array of
> + svn_ra_svn_item_t structs. Return a client error if LOCK_TOKENS
> + are not a list of lists or if a lock violates authz. */
> +static svn_error_t *add_lock_tokens(svn_ra_svn_conn_t *conn,
> + apr_array_header_t *lock_tokens,
> server_baton_t *sb,
> apr_pool_t *pool)

Lots of undocumented parameters here.

LOCK_TOKENS "is" a list, not "are" a list.

If you know the specific errors that might be returned in various
circumstances, document them. We don't always do that, but it would
be nice to do it where we can.

> {
> int i;
> svn_fs_access_t *fs_access;
>
> - SVN_ERR(svn_fs_get_access(&fs_access, sb->fs));
> + SVN_CMD_ERR(svn_fs_get_access(&fs_access, sb->fs));
>
> /* If there is no access context, nowhere to add the tokens. */
> if (! fs_access)
> @@ -658,18 +794,17 @@
> return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> "Lock tokens aren't a list of lists");
>
> - path_item = &APR_ARRAY_IDX(item->u.list, 0, svn_ra_svn_item_t);
> - if (path_item->kind != SVN_RA_SVN_STRING)
> - return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> - "Lock path isn't a string.");
> + SVN_ERR(svn_ra_svn_parse_tuple(item->u.list, pool, "ss",
> + &path_item, &token_item));
>
> - token_item = &APR_ARRAY_IDX(item->u.list, 1, svn_ra_svn_item_t);
> - if (token_item->kind != SVN_RA_SVN_STRING)
> - return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> - "Lock token isn't a string");
> + if (! lookup_access(pool, sb, svn_authz_write,
> + path_item->u.string->data, TRUE))
> + return svn_error_create(SVN_ERR_RA_SVN_CMD_ERR,
> + svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED,
> + NULL, NULL), NULL);
>
> token = token_item->u.string->data;
> - SVN_ERR(svn_fs_access_add_lock_token(fs_access, token));
> + SVN_CMD_ERR(svn_fs_access_add_lock_token(fs_access, token));
> }
>
> return SVN_NO_ERROR;
> @@ -741,13 +876,20 @@
> else
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "clb", &log_msg,
> &lock_tokens, &keep_locks));
> - /* Require a username if the client gave us any lock tokens. */
> - SVN_ERR(must_have_write_access(conn, pool, b,
> - lock_tokens && lock_tokens->nelts > 0));
>
> - /* Give the lock tokens to the FS if we got any. */
> + /* The handling for locks is a little problematic, because the
> + protocol won't let us send several auth requests once one has
> + succeeded. So we simply request write access and a username
> + before adding tokens (if we have any), and subsequently fail if a
> + lock violates authz. */
> + SVN_ERR(must_have_access(conn, pool, b, svn_authz_write,
> + NULL, lock_tokens ? TRUE : FALSE));
> +
> + /* Authorize the lock tokens and give them to the FS if we got
> + any. */
> if (lock_tokens)
> - SVN_CMD_ERR(add_lock_tokens(lock_tokens, b, pool));
> + SVN_ERR(add_lock_tokens(conn, lock_tokens, b, pool));
> +

I'm a little confused about when SVN_CMD_ERR is appropriate and when
SVN_ERR is appropriate, in this function, but I think that's most
likely due to my ignorance...

> ccb.new_rev = &new_rev;
> ccb.date = &date;
> ccb.author = &author;
> @@ -756,7 +898,8 @@
> (&editor, &edit_baton, b->repos, NULL,
> svn_path_uri_decode(b->repos_url, pool),
> b->fs_path, b->user,
> - log_msg, commit_done, &ccb, NULL, NULL, pool));
> + log_msg, commit_done, &ccb,
> + authz_commit_cb, baton, pool));
> SVN_ERR(svn_ra_svn_write_cmd_response(conn, pool, ""));
> SVN_ERR(svn_ra_svn_drive_editor(conn, pool, editor, edit_baton, &aborted));
> if (!aborted)
> @@ -803,11 +946,16 @@
> /* Parse arguments. */
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "c(?r)bb", &path, &rev,
> &want_props, &want_contents));
> - path = svn_path_canonicalize(path, pool);
> - SVN_ERR(trivial_auth_request(conn, pool, b));
> +
> + full_path = svn_path_join(b->fs_path,
> + svn_path_canonicalize(path, pool), pool);
> +
> + /* Check authorizations */
> + SVN_ERR(must_have_access(conn, pool, b, svn_authz_read,
> + full_path, FALSE));
> +
> if (!SVN_IS_VALID_REVNUM(rev))
> SVN_CMD_ERR(svn_fs_youngest_rev(&rev, b->fs, pool));
> - full_path = svn_path_join(b->fs_path, path, pool);
>
> /* Fetch the properties and a stream for the contents. */
> SVN_CMD_ERR(svn_fs_revision_root(&root, b->fs, rev, pool));
> @@ -877,11 +1025,16 @@
>
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "c(?r)bb", &path, &rev,
> &want_props, &want_contents));
> - path = svn_path_canonicalize(path, pool);
> - SVN_ERR(trivial_auth_request(conn, pool, b));
> +
> + full_path = svn_path_join(b->fs_path,
> + svn_path_canonicalize(path, pool), pool);
> +
> + /* Check authorizations */
> + SVN_ERR(must_have_access(conn, pool, b, svn_authz_read,
> + full_path, FALSE));
> +
> if (!SVN_IS_VALID_REVNUM(rev))
> SVN_CMD_ERR(svn_fs_youngest_rev(&rev, b->fs, pool));
> - full_path = svn_path_join(b->fs_path, path, pool);
>
> /* Fetch the root of the appropriate revision. */
> SVN_CMD_ERR(svn_fs_revision_root(&root, b->fs, rev, pool));
> @@ -1133,7 +1286,8 @@
> lb.conn = conn;
> err = svn_repos_get_logs3(b->repos, full_paths, start_rev, end_rev,
> (int) limit, changed_paths, strict_node,
> - NULL, NULL, log_receiver, &lb, pool);
> + authz_check_access_cb, b, log_receiver,
> + &lb, pool);
>
> write_err = svn_ra_svn_write_word(conn, pool, "done");
> if (write_err)
> @@ -1156,11 +1310,16 @@
> svn_node_kind_t kind;
>
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "c(?r)", &path, &rev));
> - path = svn_path_canonicalize(path, pool);
> - SVN_ERR(trivial_auth_request(conn, pool, b));
> + full_path = svn_path_join(b->fs_path,
> + svn_path_canonicalize(path, pool), pool);
> +
> + /* Check authorizations */
> + SVN_ERR(must_have_access(conn, pool, b, svn_authz_read,
> + full_path, FALSE));
> +
> if (!SVN_IS_VALID_REVNUM(rev))
> SVN_CMD_ERR(svn_fs_youngest_rev(&rev, b->fs, pool));
> - full_path = svn_path_join(b->fs_path, path, pool);
> +
> SVN_CMD_ERR(svn_fs_revision_root(&root, b->fs, rev, pool));
> SVN_CMD_ERR(svn_fs_check_path(&kind, root, full_path, pool));
> SVN_ERR(svn_ra_svn_write_cmd_response(conn, pool, "w", kind_word(kind)));
> @@ -1177,11 +1336,15 @@
> svn_dirent_t *dirent;
>
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "c(?r)", &path, &rev));
> - path = svn_path_canonicalize(path, pool);
> - SVN_ERR(trivial_auth_request(conn, pool, b));
> + full_path = svn_path_join(b->fs_path,
> + svn_path_canonicalize(path, pool), pool);
> +
> + /* Check authorizations */
> + SVN_ERR(must_have_access(conn, pool, b, svn_authz_read,
> + full_path, FALSE));
> +
> if (!SVN_IS_VALID_REVNUM(rev))
> SVN_CMD_ERR(svn_fs_youngest_rev(&rev, b->fs, pool));
> - full_path = svn_path_join(b->fs_path, path, pool);
> SVN_CMD_ERR(svn_fs_revision_root(&root, b->fs, rev, pool));
> SVN_CMD_ERR(svn_repos_stat(&dirent, root, full_path, pool));
>
> @@ -1253,7 +1416,7 @@
>
> err = svn_repos_trace_node_locations(b->fs, &fs_locations, abs_path,
> peg_revision, location_revisions,
> - NULL, NULL, pool);
> + authz_check_access_cb, b, pool);
>
> /* Now, write the results to the connection. */
> if (!err)
> @@ -1363,8 +1526,9 @@
> frb.conn = conn;
> frb.pool = NULL;
>
> - err = svn_repos_get_file_revs(b->repos, full_path, start_rev, end_rev, NULL,
> - NULL, file_rev_handler, &frb, pool);
> + err = svn_repos_get_file_revs(b->repos, full_path, start_rev, end_rev,
> + authz_check_access_cb, b, file_rev_handler,
> + &frb, pool);
> write_err = svn_ra_svn_write_word(conn, pool, "done");
> if (write_err)
> {
> @@ -1390,12 +1554,12 @@
>
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "c(?c)b(?r)", &path, &comment,
> &force, &current_rev));
> + full_path = svn_path_join(b->fs_path,
> + svn_path_canonicalize(path, pool), pool);
>
> - full_path = svn_path_join(b->fs_path, svn_path_canonicalize(path, pool),
> - pool);
> + SVN_ERR(must_have_access(conn, pool, b, svn_authz_write,
> + full_path, TRUE));
>
> - SVN_ERR(must_have_write_access(conn, pool, b, TRUE));
> -
> SVN_CMD_ERR(svn_repos_fs_lock(&l, b->repos, full_path, NULL, comment, 0,
> 0, /* No expiration time. */
> current_rev, force, pool));
> @@ -1429,6 +1593,12 @@
> subpool = svn_pool_create(pool);
> lock_cmds = apr_array_make(pool, locks->nelts, sizeof(struct lock_cmd));
>
> + /* Because we can only send a single auth reply per request, we send
> + a reply before parsing the lock commands. This means an authz
> + access denial will abort the processing of the locks and return
> + an error. */
> + SVN_ERR(must_have_access(conn, subpool, b, svn_authz_write, NULL, TRUE));
> +

If must_have_access() were fully documented, I wouldn't be wondering
what effect NULL has here... :-)

> /* Loop through the lock commands. */
> for (i = 0; i < locks->nelts; ++i)
> {
> @@ -1448,11 +1618,14 @@
> svn_path_canonicalize(cmd->path, subpool),
> pool);
>
> + if (! lookup_access(pool, b, svn_authz_write, cmd->full_path, TRUE))
> + return svn_error_create(SVN_ERR_RA_SVN_CMD_ERR,
> + svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED,
> + NULL, NULL), NULL);
> +
> APR_ARRAY_PUSH(lock_cmds, struct lock_cmd) = *cmd;
> }
>
> - SVN_ERR(must_have_write_access(conn, pool, b, TRUE));
> -
> /* Loop through each path to be locked. */
> for (i = 0; i < lock_cmds->nelts; i++)
> {
> @@ -1497,7 +1670,8 @@
> pool);
>
> /* Username required unless force was specified. */
> - SVN_ERR(must_have_write_access(conn, pool, b, ! force));
> + SVN_ERR(must_have_access(conn, pool, b, svn_authz_write,
> + full_path, ! force));
>
> SVN_CMD_ERR(svn_repos_fs_unlock(b->repos, full_path, token, force, pool));
>
> @@ -1525,6 +1699,9 @@
> unlock_cmds =
> apr_array_make(pool, unlock_tokens->nelts, sizeof(struct unlock_cmd));
>
> + /* Username required unless force was specified. */
> + SVN_ERR(must_have_access(conn, pool, b, svn_authz_write, NULL, ! force));
> +
> subpool = svn_pool_create(pool);
> /* Loop through the unlock commands. */
> for (i = 0; i < unlock_tokens->nelts; i++)
> @@ -1546,12 +1723,14 @@
> svn_path_canonicalize(cmd->path, subpool),
> pool);
>
> + if (! lookup_access(pool, b, svn_authz_write, cmd->full_path, ! force))
> + return svn_error_create(SVN_ERR_RA_SVN_CMD_ERR,
> + svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED,
> + NULL, NULL), NULL);
> +

Is there a reason we don't say more in this error, for example the path?

> APR_ARRAY_PUSH(unlock_cmds, struct unlock_cmd) = *cmd;
> }
>
> - /* Username required unless force was specified. */
> - SVN_ERR(must_have_write_access(conn, pool, b, ! force));
> -
> /* Loop through each path to be unlocked. */
> for (i = 0; i < unlock_cmds->nelts; i++)
> {
> @@ -1583,7 +1762,8 @@
> full_path = svn_path_join(b->fs_path, svn_path_canonicalize(path, pool),
> pool);
>
> - SVN_ERR(trivial_auth_request(conn, pool, b));
> + SVN_ERR(must_have_access(conn, pool, b, svn_authz_read,
> + full_path, FALSE));
>
> SVN_CMD_ERR(svn_fs_get_lock(&l, b->fs, full_path, pool));
>
> @@ -1615,7 +1795,7 @@
> SVN_ERR(trivial_auth_request(conn, pool, b));
>
> SVN_CMD_ERR(svn_repos_fs_get_locks(&locks, b->repos, full_path,
> - NULL, NULL, pool));
> + authz_check_access_cb, b, pool));
>
> SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "w((!", "success"));
> for (hi = apr_hash_first(pool, locks); hi; hi = apr_hash_next(hi))
> @@ -1715,7 +1895,7 @@
> static svn_error_t *find_repos(const char *url, const char *root,
> server_baton_t *b, apr_pool_t *pool)
> {
> - const char *path, *full_path, *repos_root, *pwdb_path;
> + const char *path, *full_path, *repos_root, *pwdb_path, *authz_path;
> svn_stringbuf_t *url_buf;
>
> /* Skip past the scheme and authority part. */
> @@ -1754,6 +1934,7 @@
> url_buf = svn_stringbuf_create(url, pool);
> svn_path_remove_components(url_buf, svn_path_component_count(b->fs_path));
> b->repos_url = url_buf->data;
> + b->repos_name = svn_path_basename(repos_root, pool);
>
> /* Read repository configuration. */
> SVN_ERR(svn_config_read(&b->cfg, svn_repos_svnserve_conf(b->repos, pool),
> @@ -1777,7 +1958,25 @@
> b->realm = "";
> }
>
> - /* Make sure it's possible for the client to authenticate. */
> + /* Read authz configuration */
> + svn_config_get(b->cfg, &authz_path, SVN_CONFIG_SECTION_GENERAL,
> + SVN_CONFIG_OPTION_AUTHZ_DB, NULL);
> + if (authz_path)
> + {
> + authz_path = svn_path_join(svn_repos_conf_dir(b->repos, pool),
> + authz_path, pool);
> + SVN_ERR(svn_repos_authz_read(&b->authzdb, authz_path,
> + TRUE, pool));
> + }
> + else
> + {
> + b->authzdb = NULL;
> + }
> +
> + /* Make sure it's possible for the client to authenticate. Note
> + that this doesn't take into account any authz, because we can't
> + know about access it grants until paths are given by the
> + client. */
> if (get_access(b, UNAUTHENTICATED) == NO_ACCESS
> && (get_access(b, AUTHENTICATED) == NO_ACCESS
> || (!b->tunnel_user && !b->pwdb)))
>
> Property changes on:
> ___________________________________________________________________
> Name: svk:merge
> 40bec6f9-eef0-0310-8400-9b45a50af7ca:/local/trunk:90
> 422c9f7f-73e9-0310-916f-f579f4d99a6a:/local/pool-cleanup:1951
> +65390229-12b7-0310-b90b-f21a5aa7ec8e:/trunk:15885
> ae6c956b-9dc6-0310-97b2-e73af4192982:/svn/local:8265
> c9b7866f-f5e4-0310-b3ee-a2e1643c603f:/svn/local/new-auto-merge:10428

Yeah, I *thought* those diff headers looked a little funny! :-)

-Karl

-- 
www.collab.net  <>  CollabNet  |  Distributed Development On Demand
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org