Re: [PATCH] HelperReply upgrade stage 2

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

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

Please make "value" const or remove it completely.

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

> + /**
> + * 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."

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

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

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.

Thank you,

Alex.
Received on Mon Nov 26 2012 - 19:43:52 MST