Re: Note about auth refcounting state / ntlm in trunk

Henrik Nordström wrote:
> mån 2010-08-16 klockan 01:43 +0000 skrev Amos Jeffries:
>
>> Basic flow around that absorb is:
>> create empty AuthUser "local_auth_user"
>
> Gah.. I think it should be
>
> * Perform auth. Uses and results in an AuthState (scheme specific) or if
> you prefer AuthRequest but matches badly with both ntlm & digest.
>
> * On successful auth an AuthUser is associated with the AuthState to
> keeptrack of the user long term between authentications.
>
> * Failure to perform Auth MAY result in something like an AuthUser to
> carry the username only, but preferably just keeping an internal record
> to the AuthState in such case.
>
> * High level access to the auth state of the request always goes via
> AuthState. AuthUser is internal.
>
> An AuthUser should not be required to perform Auth.
>
> On success also update the ip list for max_user_ip use, linked to
> AuthUser.
>
> No absorb of anything.

Maybe we crossed wires. My reading of your above description is that you
are saying the same thing I just said.

your "Perform auth" covers the first three steps of my description. From
empty local_auth_user, through parsing to perform auth.

Absorb() is the "AuthUser is associated with the AuthState" part of your
above description.

AuthState in your above == AuthUserRequest. Likely needs a renaming.
Definitely auth needs namespacing.

So...... the difference being that you are suggesting the credentials
text and parsing should be done on AuthUserRequest and no AuthUser
should exist associated with it until fully authed?
  AuthUser only created/looked up at the point currently doing absorb()?

Sounds like a good plan.
NOTE: this is why I specifically asked for details on the
RefCountCount() value on the assert bug:

  for that absorb() assert to occur with RefCountCount() > 2 something
outside of auth must be taking and holding a ref-Pointer of
AuthUserRequest->user() when supposed to be holding ref-Pointer to
AuthUserRequest.

  for the absorb() assert to occur with RefCountCount() < 2 something in
our auth code must have been optimized to hold less ref than the two
named function scopes.

>
> AuthUser should be scheme-independent, but need to softly link to the
> schemes using it allowing clean garbage collection and association of
> scheme state (basic credentials cache, confirmed digest nonces and their
> related H(A1))

It is already like that. AuthUser is the mostly-private part persisting
across requests/conn in the username cache. AuthUserRequest is the
public hanging off each request and conn indicating state of auth.
Pointing to whatever AuthUser has the credentials text.

(mostly-private because externals like ACL can still request viewing of
it for credentials text and history meta data).

Current auth objects are distinctly scoped:

  AuthScheme - one per component/protocol available. global.

  AuthConfig - settings for each auth_param set. currently limited to
one per protocol. no strong reason for that now though. global.

  AuthUserRequest - state of a particular request or connections
authentication attempt. dies with request or connection its associated
with. public; available externally via request/conn using it.

  AuthUser - credentials, with some historic meta data. Persistent.
internal to auth.

  AuthType - enum of protocol/scheme type. part of AuthUser meta data.

AuthUserRequest has ref-ptr to AuthUser.
AuthUser has ref-ptr to AuthConfig.

(NP the src/auth/*.h files document all this.)

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.6
   Beta testers wanted for 3.2.0.1