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

On 27/10/11 04:06, Alex Rousskov wrote:
> 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.

Gah! Sorry attached the wrong patch to the earlier mail.

This one attached now is what the above all refers to.

The context is small I know, but there is nothing around it relevant.
The patch contains the entire switch case for printing %LOGIN to the
helper key.

>
> 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?

At present the checklist reference is a cached reference to the first
found set of credentials. To make repeated tests produce the same
results eve if the request auth changes.

It may be sourced from one of the connection-auth credentials, or the
request credentials, or earlier created external_ACL credentials. or
other places we have not yet created.

The patch attached earier was a work in progress for updating
external-ACL to utilize the extended auth states. As you probably noticed.

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

Done.

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

I later have changed both these denied cases. There are other auth
changes involved and needing to be tested to find out what is best so as
not to break back-compat too badly.

>
>> - 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.");
>

Done.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.16
   Beta testers wanted for 3.2.0.13