From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: [PATCH 1/2] Refactor digest authentication in url-auth Date: Sun, 13 Nov 2016 17:53:36 +0200 Message-ID: <83d1hzcr5r.fsf@gnu.org> References: <1478988194-7761-1-git-send-email-jarno@malmari.fi> <1479036981-22047-1-git-send-email-jarno@malmari.fi> <1479036981-22047-2-git-send-email-jarno@malmari.fi> Reply-To: Eli Zaretskii NNTP-Posting-Host: blaine.gmane.org X-Trace: blaine.gmane.org 1479052428 6936 195.159.176.226 (13 Nov 2016 15:53:48 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sun, 13 Nov 2016 15:53:48 +0000 (UTC) Cc: emacs-devel@gnu.org To: Jarno Malmari Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sun Nov 13 16:53:44 2016 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1c5x65-0008RL-Im for ged-emacs-devel@m.gmane.org; Sun, 13 Nov 2016 16:53:33 +0100 Original-Received: from localhost ([::1]:34169 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c5x68-0004br-P1 for ged-emacs-devel@m.gmane.org; Sun, 13 Nov 2016 10:53:36 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:36777) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c5x62-0004bk-9m for emacs-devel@gnu.org; Sun, 13 Nov 2016 10:53:31 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c5x5z-0003fQ-84 for emacs-devel@gnu.org; Sun, 13 Nov 2016 10:53:30 -0500 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:49597) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c5x5z-0003fI-55; Sun, 13 Nov 2016 10:53:27 -0500 Original-Received: from 84.94.185.246.cable.012.net.il ([84.94.185.246]:4858 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_128_CBC_SHA1:128) (Exim 4.82) (envelope-from ) id 1c5x5y-0001HA-Fo; Sun, 13 Nov 2016 10:53:26 -0500 In-reply-to: <1479036981-22047-2-git-send-email-jarno@malmari.fi> (message from Jarno Malmari on Sun, 13 Nov 2016 13:36:20 +0200) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2001:4830:134:3::e X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:209375 Archived-At: > From: Jarno Malmari > Date: Sun, 13 Nov 2016 13:36:20 +0200 > > Additionally, this refactoring fixed a bug where duplicate key entries > were continuously added in `url-digest-auth-storage' each time > authenticated. > * 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. > +(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. 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? > +(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. > + ;; no partial matches for non-path, i.e. realm > + (and (string-match "/" dirkey) This will fail with Windows file names that use backslashes. I also find the test to be too loose: does having a slash in a string really make it a directory name? At least on Windows, a string with embedded slashes or backslashes can be an invalid file name. > +(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. > + "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. > +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"? > + ;; 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). Thanks.