[PATCH 2/3] Add ability to authorize viewing a repository

[valentin.haenel at gmx.de (Valentin Haenel)]

Hi Jason,

thanks for your review.

* Jason A. Donenfeld <Jason at zx2c4.com> [2012-10-16]:
> On Tue, Oct 16, 2012 at 11:15 AM, Valentin Haenel
> <valentin.haenel at gmx.de> 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: "<repository> <user> [<permissions>]". 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-