Re: [PATCH] remove assert(ch->auth_user_request != NULL)

On 10/25/2011 11:59 PM, Amos Jeffries wrote:
> external ACL soemtimes cannot find the credentials in ACL Checklist even
> if they are attached to the HTTPRequest object.
>
> This seems to happen when the checklist is created and the line match
> started before the credentials are known.
>
> Unless someone knows of a better place to duplicate the credentials
> reference from request to checklist. I would like to apply this patch to:
>
> * locate the %LOGIN value from either place where credentials can be
> found,
> * updates the checklist if it was unset,
> * passes '-' to the helper if no credentials at all were given.
>
> Although the earlier logics forcing a lookup means this '-' case should
> not happen.

I am not an expert in this so forgive me if my question itself is wrong,
but why do we have to store credentials in two places (ACL Checklist and
HTTPRequest)? Can ACL Checklist keep a pointer to request and extract
credentials from the request when they are needed?

> + allow_t ti = AuthenticateAcl(ch);
> + switch(ti)

This can be polished to reduce ti scope and constness:

    switch (const allow_t ti = AuthenticateAcl(ch))

There are two cases matching the above.

> + case ACCESS_ALLOWED:
> + case ACCESS_AUTH_EXPIRED_OK:
> + // we have credentials. Trust them?
> + debugs(82, 3, HERE << acl->def->name << " user has okay authentication credentials.");
> + break;
> + case ACCESS_DENIED:
> + case ACCESS_AUTH_EXPIRED_BAD:
> + // we have credentials. Trust them?
> + debugs(82, 3, HERE << acl->def->name << " user has failed authentication credentials.");
> + break;

It is weird that we do not care whether the access was allowed or
denied, but, again, I do not know much about this area of the code.
Perhaps change the comments to explain why there is no difference and
treat all cases the same? You can print ti to debug the actual status.

> - debugs(82, 2, HERE << "\"" << key << "\": return -1.");
> + debugs(82, 2, HERE << "\"" << key << "\": return " << ACCESS_DUNNO);

I would print the ACCESS_DUNNO string instead of its value:
    debugs(82, 2, HERE << "\"" << key << "\": return ACCESS_DUNNO.");

> [PATCH] remove assert(ch->auth_user_request != NULL)

The patch does not remove any asserts. s/remove/avoid/? Also, the
effects of the patch, according to your own description seem to go way
beyond avoiding an assert so perhaps a different summary line would be
more appropriate.

Thank you for using a deep context diff!

HTH,

Alex.
Received on Wed Oct 26 2011 - 15:07:07 MDT