Re: [PATCH] HelperReply upgrade stage 2

On 27/11/2012 8:43 a.m., Alex Rousskov wrote:
> On 11/24/2012 07:47 AM, Amos Jeffries wrote:
>> Stage 2:
>> This stage of the helper reply protocol upgrade adds kv-pair support to
>> the HelperReply object.
>>
>> It uses the new Notes objects to hold any "key=value" syntax fields
>> received from the helper after the result code field. The values section
>> for these kv-pair may be token or quoted string. Helpers which
>> previously rfc1738 encoded the values may still do so but it is no
>> longer mandatory.
>>
>> The response syntax for all helpers becomes:
>> [channel-ID SP ] result [ SP key-pair ...] [ SP other] EOL
>>
>>
>> The parser for HelperReply is also updated to map the old AF and NA
>> NTLM/Negotiate response fields into the HelperReply notes.
>> * "token=" is added to supply the NTLM and Negotiate server blob /
>> token field.
>> * "user=" is added to supply the user label field.
>> The relevant callback handlers are updated for these helpers to make use
>> of these new keys.
>>
>> The bundled Digest authentication helpers are all upgraded to send the
>> new format responses. They now use ERR for failed lookup, BH for
>> internal errors, and OK with "ha1=" key added to supply a HA1 response.
>> The handler for Digest authentication is updated to process the new
>> HelperReply fields with failover the old format on Unknown result codes.
>>
>> The external ACL handler is updated to pull its key=value pairs out of
>> the Notes list. The old parser loop becomes useless with this and is
>> removed. Taking with it support for several long deprecated keys
>> "login=", "passwd=", and "error=".
>>
>> Any other keys MAY be sent on any response. However at this stage 2
>> patch they are ignored. As are repeated / secondary values for the
>> expected key names, only the first instance sent in the response is used.
>>
>>
>> Amos
>>
>
>
>> + // list of key=value pairs the helper produced
>> + Notes responseKeys;
> We store both keys and their values here. And we are already inside the
> reply class. Please rename to something like "notes" or "annotations".

Sure. We do now have "notes" floating out our ears in the same sort of
way as "conn".

>
>> + // the value may be a quoted string or a token
>> + const bool urlDecode = (*p != '"'); // check before moving p.
>> + char *v = strwordtok(NULL, &p);
>> + if (v != NULL && urlDecode && (p-v) > 2) // 1-octet %-escaped requires 3 bytes
>> + rfc1738_unescape(v);
>> + String value = v;
> If I am reading this correctly, "v" may be NULL here. If so, "value"
> becomes an "undefined" String. Undefined Strings are not a problem in
> this particular code, but may be a problem with stage 3 patch reviewed
> separately in the next email.

Fixed. Updated to pass "" when v is NULL so we completely avoid the
undefined String issues.

> Please make "value" const or remove it completely.
>

Tried.

>
>> +
>> +public:
>> + /**
>> + * Adds a note key and value to the notes list.
>> + * If the key name already exists in list, add the given value to its set of values.
>> + */
>> + void add(const String &noteKey, const String &noteValue);
>> +
>> + /**
>> + * Looks up a note by key name and returns a Note::Pointer to it
>> + */
>> + Note::Pointer findByName(const String &noteKey) const;
> Please move up into the existing "public" section, above the existing
> data members.
Done.

>
>> + /**
>> + * Looks up a note by key name and returns a Note::Pointer to it
>> + */
>> + Note::Pointer findByName(const String &noteKey) const;
> It may be a good idea to rename this to findByKey() or simply find().
>
> For the description, consider a more complete "Returns a pointer to an
> existing Note with a given key or nil."
Done.

>
>> + lm_request->server_blob = xstrdup(tokenNote->values[0]->value.termedBuf());
>> + CvtBin(ha1Note->values[0]->value.termedBuf(), digest_user->HA1);
>> + digest_request->setDenyMessage(msgNote->values[0]->value.termedBuf());
>> + auth_user_request->user()->username(userLabel->values[0]->value.termedBuf());
>
> Lots of code seems to assume that either a Note has only one value or
> that the first value is the most important one. I suggest adding
> Note::singleValue() or a similar method returning values[0]->value. That
> method will make the above code much simpler and easier to read _and_ it
> can, for example, assert() or warn if there are actually multiple values
> stored in the Note.

Added a convenience method firstValue() which returns a termedBuf() or
"". That gets rid of most of them. Made it also perform and return the
termedBuf() used almost everywhere at present so it can check for
undefined String and return "".

I am not sure how to deal with the char* vs. String needs cleanly. That
will need cleaning up when StringNG goes in but is not urgent before
then. At present the external ACL interface is the only one needing
String, and only to fill a set of variables which I hope to replace with
a Notes::Pointer soon anyway. So have left those using the String API
from values[0]->value.

>
>> + * key-pair := OWS token '=' ( quoted-string | token )
> I think you are renaming key-pair to kv-pair or similar in the next
> stage. If you commit stages separately, it may be better to fix it in
> this stage, but perhaps that is too much work.

Oops. Yes was doing it in this patch but missed these. Fixed.

> The above comments can probably be addressed without another round of
> reviews, but handling of undefined Strings may be tricky so be extra
> careful there.

Done away with it by ensuring the string is at least defined ("") by the
parser and the new firstValue() member also checks in case somebody
manually adds an undefined String.

Build testing the above changes and will merge the helpers branch when
it works.

Cheers
Amos
Received on Tue Nov 27 2012 - 11:48:43 MST