Support helper ttl= annotations for Basic authentication

[yadij]

... and clean up the TTL / expiry information for all
authentication schemes behind an API of Auth::User.

Fixes a small bug where Basic auth credentials could be expired
one second earlier than they should have been.

Fixes a small bug where Basic auth credentials could be expired
by authenticate_ttl earlier than their squid.conf credentialsttl
is set to. These two configuration settings are supposed to
cover different time periods in credentials lifetime within
Squid memory.

Documentation updated to clarify the various credential TTL
config setting meaning and behaviour impact.

[rousskov]

Please add a description for the currently undocumented Auth::User::expiretime field. This PR manipulates that field a lot but it is very difficult to review those manipulations without knowing exactly what that field is.

[rousskov]

This "expired() means expired" comment adds very little useful information, but raises a question: What expires, the whole user record (i.e. this object, as the method name implies) or just the user credentials (the comment mentions credentials so there might be a reason for that)?

• If user credentials expire separately from the user record as a whole, then this description should be adjusted to document that important caveat and the method should probably be renamed to expiredCredentials().
• Otherwise, I would remove this description to avoid the problems it causes. Documenting the obvious can only hurt. Or you may want to attract the reader attention that objects expiring during the current second (zero TTL) are still considered fresh.

[yadij]

Removing the comment violates the requirement to provide a description of what each public method does.

Suggestions for better wording are welcome.

[yadij]

After looking at a few similar trivial methods accepted in other PR I'm thinking something like
"whether the credentials represented by this Auth::User object have passed their latest known TTL"

[rousskov]

Removing the comment violates the requirement to provide a description of what each public method does.

In my documented opinion, there are virtually no absolute requirements. The "every new name should be documented" (or equivalent) rule does apply to this method, of course, but leaving this trivial test method without explicit documentation would be reasonable IMO if no suitable documentation is found (i.e. exceptional treatment can be justified in this specific case). Providing proper documentation would be even better, of course, but the current "foo is foo" documentation is (slightly) worse than the absence of any explicit documentation.

"whether the credentials represented by this Auth::User object have passed their latest known TTL"

AFAICT, the only useful bit of information here is the word "credentials". The rest is essentially a definition of what the word "expires" means in a context with a known TTL. By saying "credentials", are we implying that the user record itself (i.e. the Auth::User object) does not really expire and can be meaningfully used by Squid beyond the credentials expiration time? If yes, let's be very explicit about that important fact -- it is valuable. Otherwise, let's not imply that credentials have a separate "usable lifetime" from their User object.

[yadij]

"whether the credentials represented by this Auth::User object have passed their latest known TTL"

AFAICT, the only useful bit of information here is the word "credentials".

That is somewhat insulting. But i shall overlook other than to point out.

The rest is essentially a definition of what the word "expires" means in a context with a known TTL.

Exactly. Documentating what "expired()" means in its use-case context(s).

By saying "credentials", are we implying that the user record itself (i.e. the Auth::User object) does not really expire and can be meaningfully used by Squid beyond the credentials expiration time?

Yes. The object itself only "expires" when free()'d. The credentials/value can be expired/stale without free().

If yes, let's be very explicit about that important fact -- it is valuable.

Suggestions welcome. I thought that original wording sufficiently clear "to detect expired credentials" (note word is credentials, not "user")

[rousskov]

The object itself only "expires" when free()'d. The credentials/value can be expired/stale without free().

If the whole object does not expire when the expired() method starts returning true, then the method itself would have to be renamed. Right now, the method name implies that the method is describing the state of the entire object, not some part of that object. The distinction is very important because this object stores a lot more than credentials. If all those other pieces of information can be used when expired() returns true, let's call the method User::withExpiredCredentials().

As for the method documentation (the original subject of this change request), I can suggest specific improvements but would prefer to wait until the new method name confirms my understanding of what this method actually means. Feel free to polish the description while renaming, of course. I would very much prefer not to work on this documentation!