Re: [PATCH 1/2] Refactor digest authentication in url-auth

[Jarno Malmari]

Eli Zaretskii <address@hidden> writes:
>> * lisp/url/url-auth.el (url-digest-auth, url-digest-auth-create-key):
>> (url-digest-auth-build-response, url-digest-auth-directory-id-assoc,
>> url-digest-auth-name-value-string, url-digest-auth-source-creds,
>> url-digest-cached-key, url-digest-cache-key, url-digest-find-creds,
>> url-digest-find-new-key, url-digest-prompt-creds): Add new functions to
>> simplify code and aid in unit testing.
>
> This is not how we format commit logs of changes to several functions
> whose list spans several lines.  The entry should look like this:
>
> * lisp/url/url-auth.el (url-digest-auth, url-digest-auth-create-key):
> (url-digest-auth-build-response, url-digest-auth-directory-id-assoc)
> (url-digest-auth-name-value-string, url-digest-auth-source-creds)
> (url-digest-cached-key, url-digest-cache-key, url-digest-find-creds)
> (url-digest-find-new-key, url-digest-prompt-creds): Add new functions to
> simplify code and aid in unit testing.
>
> IOW, each line begins is separately parenthesized.

Will fix. Seems I wasn't able to deduce the correct format from previous
log entries nor from the CONTRIBUTE file. Is this format obvious to
others? Should the "each line separately parenthesized" be described in
the CONTRIBUTE file?

>
>> +(defsubst url-digest-auth-kd (data secret)
>> +  "Apply digest algorithm MD5 to DATA using SECRET as described in RFC 
>> 2617."
>> +  (md5 (url-digest-auth-colonjoin secret data)))
>> +
>> +(defsubst url-digest-auth-make-ha1 (user realm password)
>> +  "Compute hashed A1 value as described in RFC 2617.
>> +USER, REALM, and PASSWORD are strings used to create the hash
>> +from."
>> +  (md5 (url-digest-auth-colonjoin user realm password)))
>
> Hear and elsewhere, I find the doc strings impenetrable without having
> RFC 2617 around.

Yeah, it's kind of annoying.

> I wonder if it would make sense to describe the arguments in a bit
> more detail, such that consulting the RFC each time these are used
> would not be necessary.  Is that practical?

I'll give it a try and see if it gets messy. Some arguments might have
complex(ish) BNF to them but we may not have to go that far in our
limited implementation.

>
>> +(defun url-digest-auth-directory-id-assoc (dirkey keylist)
>> +  "Find the best match in key list using a path or a realm.
>> +
>> +The string DIRKEY is either a path or a realm.  The key list to
>> +search through is the alist KEYLIST where car of each element is
>> +either a path or a realm.  Realms are searched for an exact
>> +match.  For paths, an ancestor is sufficient for a match."
>
> GNU coding standards frown on using "path" for anything but PATH-style
> directory lists.  Please use "file name" or "directory name" instead.

Good to know. Is there a convention to indicate path part in URIs?

>
>> +   ;; no partial matches for non-path, i.e. realm
>> +   (and (string-match "/" dirkey)
>
> This will fail with Windows file names that use backslashes.

We're actually not talking about file system paths. We're talking about
paths (directory names?) in URIs which define, together with realm, the
"protection space" (see RFC2617 ;)). The idea behind protection spaces
are that the same credentials can be preemptively reused without
bothering the user.

I can't say if the url package is using the path part correctly or not,
or if it's even trying to implement protection spaces with it. (I'm not
an expert on this. I just refactored the code trying to preserve the old
functionality the best I could.) In any case, url package uses either
realm or the path as key to find cached credentials, and prefers the
realm. If the path is used, then same credentials is tried for every
"sub directory" resource: If path "/a/b" (www.example.com/a/b) has
credentials cached, the same credentials are tried for "a/b/c" and so
on.

>
>> +(defun url-digest-cached-key (url realm)
>> +  "Find best match for URL and REALM from `url-digest-auth-storage'.
>> +The return value is a list consisting of a realm (or a directory)
>> +a user name, and hashed authentication tokens HA1 and HA2,
>> +defined in RFC 2617. Modifying the contents of the returned list
>
> Two spaces between sentences, please.

Will fix.

>
>> +  "Create a key for digest authentication method.
>> +The USERNAME and PASSWORD are the credentials for REALM and are
>> +used in making a hashed value named HA1. The HTTP METHOD and URI
>> +makes a second hashed value HA2. These hashes are used in making
>> +the authentication key that can be stored without saving the
>> +password in plain text.  The return value is a list consisting of
>> +hashed authentication tokens HA1 and HA2, defined in RFC 2617.
>
> Same here.

On to it.

>
>> +Primary method for finding credentials is from Emacs auth-source.
>> +If password isn't found, and PROMPT is non-nil, allow to query
>> +credentials via minibuffer.
>
> "Allow to query" or "query"?

Query. Confusion arised since prompt=t by itself doesn't mean query will
happen. Will fix.

>
>> +    ;; if incomplete and prompt allowed, prompt the user
>
> Comments should begin with a capital letter and end with a period, as
> normal sentences are (here and elsewhere in the patch).

Does that implicitly mean that each comment should be a complete
sentence? Granted, this one is close to one. Will fix.

>
> Thanks.