Re: [PATCH] Authz support in Svnserve

Your code looks great, with one caveat. I had some quibbles with the
comments.

On Tue, 2005-08-23 at 10:51 +0200, David Anderson wrote:
+ const char *repos_name; /* The name of the repository (for authz) */

This field should be named authz_repos_name if it is only useful for
authz. If not, then the comment shouldn't say what it's used for.

(My inclination is that it's only useful for authz, and the field name
should change.)

+ If no authz rules are
+ present in BATON, grant access by default to emulate complete
+ absence of authz code. */

Your audience is someone reading the code for the first time in 2006,
not someone who knows what the code used to look like. End the sentence
at "grant access by default".

+ /* 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 = "/";

Makes me feel that there is squishy design in the system, possibly
pre-existing. I'd be happier if this problem were analyzed more
carefully and the comments didn't use vague phrasing like "at various
levels".

+ /* 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. */

"The function"? You mean this function? "If authz is disabled in the
server config, this will just allow access blindly, in which case we
will only consider blanket access rules."

+ /* If the required access is blanket-granted AND granted by
+ authz AND the request complies with NEEDS_USERNAME
+ restrictions, then the lookup has succeeded. */

"Complies with NEEDS_USERNAME restrictions"? "AND we already have a
username if one is needed".

+ nothing needs to be done. Create the FS access and send a trivial

Two spaces after period.

+ /* 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. */

Don't confuse the reader; we force requiring a username because we need
a username to check authz configuration.

+ /* Now that an authentication has been done (if using protocol
+ version 2 that is), get the new take of authz on the
+ request. */

Don't confuse the reader; protocol version 2 is essentially always in
use.

+ 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. */

Remove the word "simply"; this isn't really simple.

+ /* 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. */

"Note that this doesn't take into account the authz configuration read
above, because..."

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Aug 24 17:22:26 2005