Re: bug#58985: 29.0.50; Have auth-source-pass behave more like other back ends

[J.P.]

Akib Azmain Turja <akib@disroot.org> writes:

>> +(defcustom auth-source-pass-extra-query-keywords nil
> [...]
>
> This should be non-nil by default, since it fixes a number of bugs.  We
> don't want to deprive users from these fixes, do we?

If that's what everyone here agrees to, then fine by me. Hopefully end
users and dependent packages will agree.

>> +(defun auth-source-pass--build-result-many (hosts ports users require max)
>> +  "Return multiple `auth-source-pass--build-result' values."
>> +  (unless (listp hosts) (setq hosts (list hosts)))
>> +  (unless (listp users) (setq users (list users)))
>> +  (unless (listp ports) (setq ports (list ports)))
>> +  (let* ((auth-source-pass--match-regexp (auth-source-pass--match-regexp
>> +                                          auth-source-pass-port-separator))
>> +         (rv (auth-source-pass--find-match-many hosts users ports
>> +                                                require (or max 1))))
>> +    (when auth-source-debug
>> +      (auth-source-pass--do-debug "final result: %S" rv))
>> +    (if (eq auth-source-pass-extra-query-keywords 'test)
>> +        (reverse rv)
>
> The value `test' is not documented.  Is it used in tests?  If it is, I
> think an internal variable would be better.

We could certainly do that. I left it as something uglier and more
sentinel-like for now.

>> +
>> +;; For now, this ignores the contents of files and only considers path
>> +;;  components when matching.
>
> The file name contains host, user and port, so parsing contents is not
> needed at all.

Right, but since `auth-source-pass--parse-data' impacts the code path
whenever a multiline file is encountered, I thought the reader should
know that we're consciously disregarding its findings. Anyway, I've
moved the comment somewhere more relevant and reworded it for clarity.

>> +(defun auth-source-pass--find-match-many (hosts users ports require max)
>> +  "Return plists for valid combinations of HOSTS, USERS, PORTS.
>> +Each plist contains, at the very least, a host and a secret."
>> +  (let ((seen (make-hash-table :test #'equal))
>> +        (entries (auth-source-pass-entries))
>> +        port-number-p
>> +        out)
>> +    (catch 'done
>> +      (dolist (host hosts out)
>> +        (pcase-let ((`(,_ ,u ,p) (auth-source-pass--disambiguate host)))
>> +          (unless (or (not (equal "443" p)) (string-prefix-p "https://"; 
>> host))
>
> Can "auth-source-pass--disambiguate" return host with the protocol part?

No, but it downcases the host, so "Libera.Chat" becomes "libera.chat",
which may be desirable for some use cases but not for ERC's (and I
suspect those of other dependent libraries as well). If I call
`auth-source-search' with :host Libera.Chat or "ircs://irc.libera.chat",
and I get a match, I want the result to contain a host `equal' to the
one I passed in (as is the case with other back ends) and not some
normalized version, like "{,irc.}libera.chat". Likewise, for ports and
users.

>> +            (setq p nil))
>> +          (dolist (user (or users (list u)))
>> +            (dolist (port (or ports (list p)))
>> +              (setq port-number-p (equal 'integer (type-of port)))
>
> Just saw the first use of "type-of".  Doesn't "(integerp port)" work?

Thanks.

>> +                  (when (or (zerop (cl-decf max))
>> +                            (null (setq entries (delete e entries))))
>
> Can the delete call conflict with the dolist loop?

In this particular case, I don't believe so, although things get
confusing when you have duplicates (which we don't). When expanded, we
basically have

  (let ((tail entries))
    (while tail
      (let ((e (car tail)))
        (cl-assert (eq (member e entries) tail)) ; invariant
        (when ...
          (setq entries (delete e entries)))
        (setq tail (cdr tail)))))

where the CDR of the current tail may become the CDR of the previous
tail on a match, but that doesn't mutate the former. Regardless, I
suppose it's bad practice to depend on internal implementations, which
could always change, so I've swapped this out for `remove' instead. Good
question.

>> +(ert-deftest auth-source-pass-extra-query-keywords--wild-port-miss-netrc ()
>> +  (ert-with-temp-file netrc-file
>> +    :text "\
>> +machine x.com password a
>> +machine x.com port 42 password b
>> +"
>> +    (let* ((auth-sources (list netrc-file))
>> +           (auth-source-do-cache nil)
>> +           (results (auth-source-search :host "x.com" :port 22 :max 2)))
>> +      (dolist (result results)
>> +        (setf result (plist-put result :secret (auth-info-password 
>> result))))
>> +      (should (equal results '((:host "x.com" :secret "a")))))))
>
> How this is testing auth-source-pass?

It's there for comparison and to cement the behavior of the reference
implementation, netrc, as canon. Notice that those `auth-source-search'
calls for every pair of cases are identical despite having different
back ends (that's the whole point). Happy to move all the netrc variants
to test/lisp/auth-source-tests.el, but locality for juxtaposition's sake
can often be a mercy on tired eyes.


Thanks for the notes.

0000-v3-v4.diff
Description: Text Data

0001-POC-Make-auth-source-pass-behave-more-like-other-bac.patch
Description: Text Data

0002-POC-Support-auth-source-pass-in-ERC.patch
Description: Text Data