Re: [PATCH] Access Control API cleanup

On 29/06/11 00:23, Amos Jeffries wrote:
> In summary:
> * use nonBlockingCheck() or fastCheck() to test ACLs.
> * be prepared to handle any allow_t in the result.
>
>
> ACL testing functions publicly available from ACLChecklist are:
>
> - nonBlockingCheck (public), fastCheck public), check (public but not to
> be used)
> - matchAclListFast (public), matchAclListSlow (private), matchAclList
> (private).
>
> Given that there are only two types of test performed, this array of API
> methods has been causing confusion and mistakes for some developers.
>
>
> This patch seeks to clarify that API by correcting a flaw in the naming
> of check() and matchAclListFast().
>
>
> Due to "Fast" ACLs coming in two types there are two overloaded
> fastCheck() functions. Now with identical output behaviour. Both return
> the allow_t result of the lookup. This is expected to _usually_ be
> ACCESS_ALLOWED/ACCESS_DENIED but that is not always the case. Callers
> need to be written with consideration that the set of enum results may
> change.
>
> - fastCheck(), no parameters, when a full set of "Fast" *_access lines
> are to be scanned. The checklist constructor accepts the list to be
> scanned. This is the old fastCheck(), with the new ALLOWED/DENIED/DUNNO
> result.
>
> - fastCheck(list), one parameter, when a single-line set of ACLs is to
> be scanned. This is the old matchAclListFast(), with the new
> ALLOWED/DENIED/DUNNO result. Will return ALLOWED whenever the whole set
> of ACLs matches. Other results may vary.
>
> - nonBlockingCheck() - for "Slow" non-blocking lookups with asynchronous
> callback handler. NP: not touched by this patch.
>
> The output change from boolean to allow_t is due to the fastCheck
> callers mixed set of needs allow/deny/other which boolean cannot meet.
> Mapping that tri-state need to a boolean result has led to inconsistent
> cases of fastCheck() producing unusual values for "true". Sometimes
> wrongly for the caller.
>
>
> Added result lookup type ACCESS_DUNNO, to indicate a test was unable to
> be completed BUT there was no allow/deny/auth-required resulting.
>
>
> Alters all previous calling code to use the new fastCheck() API output.
> Some have been polished up to boolean where appropriate instead of
> relying on integer values.
>
>
> Removes matchAclListFast/matchAclListSlow,
> Renames check() to matchNonBlocking;
> all match*() functions are internal operations during ACL testing.
>
>
>
> FUTURE WORK:
> * update check() to send allow_t to its callbacks as well.
> * update remaining messy callers from using int(0 or 1) types to bool or
> allow_t as appropriate.
>
> Amos

Jenkins found two build errors in this patch and the check() callbacks
turned out to all be using int to receive allow_t values anyway. So
complete the future work with zero logic changes.

Applied the sum result since this seems to have passed the 10-day
criteria no complaints.

If anyone is actually interested in a relatively small but useful job
there are at least two polishing cleanups now possible:

  1) splitting the ACCESS_REQ_PROXY_AUTH state result into several,
indicating the presence/absence and state of credentials. ie auth okay,
auth failed, credentials required, expired was okay, expired was failed.

  2) merging the prefer_direct, always_direct and never_direct access
control lists into one direct_access lookup that returns states PREFER,
ALWAYS, NEVER, ALLOWED, DENIED.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.14
   Beta testers wanted for 3.2.0.9