From mboxrd@z Thu Jan 1 00:00:00 1970 From: valentin.haenel at gmx.de (Valentin Haenel) Date: Tue, 16 Oct 2012 14:48:12 +0200 Subject: [PATCH 2/3] Add ability to authorize viewing a repository In-Reply-To: References: <1350378927-10834-1-git-send-email-valentin.haenel@gmx.de> <1350378927-10834-3-git-send-email-valentin.haenel@gmx.de> Message-ID: <20121016124811.GB19455@kudu.in-berlin.de> Hi Jason, thanks for your review. * Jason A. Donenfeld [2012-10-16]: > On Tue, Oct 16, 2012 at 11:15 AM, Valentin Haenel > wrote: > > ctx->cfg.ssdiff = 0; > > + ctx->cfg.authorization = "/bin/true"; > > This should be set to null by default. > > Also, choose a more specific variable name than "authorization". Perhaps you have some ideas, the two that come to mind here are: * authz-exec * authorization-executable > > + /* Here we do the authorization check. > > + * > > + * TODO > > + * ---- > > + * * figure out if this is the correct place to do the check > > + * * check that the authorization script exists and is executable > > + * > > + */ > > It's best to submit code that's been completed, not with glaring > TODOs, whose fate of completion is unknown. I left them in because I couldn't resolve them on my own. Especially the question about this being the right place. Would appreciate some feedback on this. Regarding the non-existence of the script, AFAICT 'system' will hand over execution to a command processor which will return non-zero exit status if the executable does not exist. Not sure what to do here. An admin might misspell the executable name and get confused because he thinks his authorization isn't working properly. > > + if (ctx->repo && system(fmt("%s %s %s", > > + ctx->cfg.authorization, > > + ctx->repo->name, > > + ctx->env.remote_user)) != 0) { > > > Considering the null change above, this should also be changed to "if > (ctx..authorization && !system(...)) return; OK. > > +authorization:: > > + Specifies a path to authorization executable. The executable must take two > > + arguments: " []". The permission is the > > + type of permission we wish to check for, e.g. "R" or "RW" or "RW+". A > > + common use case is to call gitolite through this machinery. > > + > > Remove reference to gitolite. Add default value. OK. > In general, the idea of having this is nice, but the implementation > needs to be polished. Sure, I can fix any issues that are raised by people on this list. I'll take care of the ones I OKd now and wait for more feedback on the others before sending a 'v2' of this patch stack. V-