bug#38284: 27.0.50; [PATCH] Make auth-source-pass-search understand port lists

bug#38284: 27.0.50; [PATCH] Make auth-source-pass-search understand port lists

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 525 bytes --]

Hi,

When trying to follow along a tutorial on setting up Gnus for GMAIL, I
tried to use auth-source-pass.el to access encrypted entries under
~/.password-store instead of the usual ~/.authinfo.gpg.

After much wrestling with the system, I couldn't figure out why my
entry:

   gmail:imap.gpg

whose contents are

   NotReallyThePassword
   host: imap.gmail.com
   user: joaotavora@gmail.com
   port: 993

weren't being understood by the new auth-source.  Eventually I came to
this patch, which seems to do the right thing.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-auth-source-pass-search-understand-port-lists.patch --]
[-- Type: text/x-diff, Size: 1837 bytes --]

From 4a6c24c23c9f7097807c1ef58688b51db330f503 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Wed, 20 Nov 2019 00:11:00 +0000
Subject: [PATCH] Make auth-source-pass-search understand port lists

For cases such as a typical IMAP Gnus setup, auto-source-pass-search
will be passed a list of "port aliases" like (993 "imaps" "imap" "993"
"143") in hopes of finding a matching ~/.password-store entry.

This modification makes this library understand and unroll the port
list so that, i.e. "domain:993", "domain:imaps"", "domain:imap",
etc. are computed as potential suffixes.  Previously a nonsensical
string "domain:(993 imaps imap ...)" was return.

* lisp/auth-source-pass.el
(auth-source-pass--generate-entry-suffixes): Allow PORT to
be a list of ports.
---
 lisp/auth-source-pass.el | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/lisp/auth-source-pass.el b/lisp/auth-source-pass.el
index 524a72792c..cc0a6fe4de 100644
--- a/lisp/auth-source-pass.el
+++ b/lisp/auth-source-pass.el
@@ -269,10 +269,15 @@ auth-source-pass--generate-entry-suffixes
 
 Based on the supported pathname patterns for HOSTNAME, USER, &
 PORT, return a list of possible suffixes for matching entries in
-the password-store."
+the password-store.
+
+PORT may be a list of ports."
   (let ((domains (auth-source-pass--domains (split-string hostname "\\."))))
-    (seq-mapcat (lambda (n)
-                  (auth-source-pass--name-port-user-suffixes n user port))
+    (seq-mapcat (lambda (d)
+                  (seq-mapcat
+                   (lambda (p)
+                     (auth-source-pass--name-port-user-suffixes d user p))
+                   (if (listp port) port (list port))))
                 domains)))
 
 (defun auth-source-pass--domains (name-components)
-- 
2.24.0


[-- Attachment #3: Type: text/plain, Size: 34 bytes --]


Please have a look,
João

bug#38284: 27.0.50; [PATCH] Make auth-source-pass-search understand port lists

[Lars Ingebrigtsen]

João Távora <joaotavora@gmail.com> writes:

> This modification makes this library understand and unroll the port
> list so that, i.e. "domain:993", "domain:imaps"", "domain:imap",
> etc. are computed as potential suffixes.  Previously a nonsensical
> string "domain:(993 imaps imap ...)" was return.
>
>    (let ((domains (auth-source-pass--domains (split-string hostname "\\."))))
> -    (seq-mapcat (lambda (n)
> -                  (auth-source-pass--name-port-user-suffixes n user port))
> +    (seq-mapcat (lambda (d)
> +                  (seq-mapcat
> +                   (lambda (p)
> +                     (auth-source-pass--name-port-user-suffixes d user p))
> +                   (if (listp port) port (list port))))
>                  domains)))

Looks good to me, but I don't use password-store so I can't really
test.  If it works for you, please go ahead and apply.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#38284: 27.0.50; [PATCH] Make auth-source-pass-search understand port lists

[Damien Cassou]

Hi João,

João Távora <joaotavora@gmail.com> writes:
> […] Eventually I came to this patch, which seems to do the right
> thing.


great job, thank you. Some feedback below.

>    (let ((domains (auth-source-pass--domains (split-string hostname "\\."))))
> -    (seq-mapcat (lambda (n)
> -                  (auth-source-pass--name-port-user-suffixes n user port))
> +    (seq-mapcat (lambda (d)


can you please rename "d" to "domain"?


> +                  (seq-mapcat
> +                   (lambda (p)


same for "p".


> +                     (auth-source-pass--name-port-user-suffixes d user p))
> +                   (if (listp port) port (list port))))
>                  domains)))


Can you please add a unit test covering this new use-case?

-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

bug#38284: 27.0.50; [PATCH] Make auth-source-pass-search understand port lists

[João Távora]

On Thu, Nov 21, 2019 at 6:27 PM Damien Cassou <damien@cassou.me> wrote:
>
> Hi João,
>
> João Távora <joaotavora@gmail.com> writes:
> > […] Eventually I came to this patch, which seems to do the right
> > thing.
>
>
> great job, thank you. Some feedback below.
>
> >    (let ((domains (auth-source-pass--domains (split-string hostname "\\."))))
> > -    (seq-mapcat (lambda (n)
> > -                  (auth-source-pass--name-port-user-suffixes n user port))
> > +    (seq-mapcat (lambda (d)
>
>
> can you please rename "d" to "domain"?

ok.  I do call your attention that it was already the
single letter n there, so I was following what I though
was shorthand convention, just adjusting it to the
first letter of the concept actually used.

> > +                  (seq-mapcat
> > +                   (lambda (p)
>
>
> same for "p".

ok.

> > +                     (auth-source-pass--name-port-user-suffixes d user p))
> > +                   (if (listp port) port (list port))))
> >                  domains)))
>
>
> Can you please add a unit test covering this new use-case?

No. This is too much work for such a trivial change that
can be reasoned about locally. I don't usually write tests
for those.  If you permit me to exagerate, it's like testing
that (+ 2 2) really equals 4.

But some people do write tests at this level, and I of
course don't mind if you do.

-- 
João Távora

bug#38284: 27.0.50; [PATCH] Make auth-source-pass-search understand port lists

[Damien Cassou]

[-- Attachment #1: Type: text/plain, Size: 694 bytes --]

Hi João,

João Távora <joaotavora@gmail.com> writes:
>> can you please rename "d" to "domain"?
>
> ok.  I do call your attention that it was already the
> single letter n there


yes I saw. This should never have been merged like that. Thank you for
improving the code.

>> Can you please add a unit test covering this new use-case?
>
> No. This is too much work for such a trivial change

I care a lot about the automated testing of the code I write. I won't
try to convince you though. Can you please merge the attached patch with
yours?

-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-WIP.patch --]
[-- Type: text/x-patch, Size: 1118 bytes --]

From afd2ebb80c80573447ec1dc23bc1625f9b41046a Mon Sep 17 00:00:00 2001
From: Damien Cassou <damien@cassou.me>
Date: Fri, 22 Nov 2019 09:36:44 +0100
Subject: [PATCH] WIP

---
 test/auth-source-pass-tests.el | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/test/auth-source-pass-tests.el b/test/auth-source-pass-tests.el
index bec42a5..ed1752d 100644
--- a/test/auth-source-pass-tests.el
+++ b/test/auth-source-pass-tests.el
@@ -353,6 +353,10 @@ (ert-deftest auth-source-pass--matching-entries-find-entries-with-a-port ()
   (auth-source-pass--with-store '(("bar.com:8080"))
     (should (auth-source-pass-match-entry-p "bar.com:8080" "bar.com" nil "8080"))))
 
+(ert-deftest auth-source-pass--matching-entries-find-entries-with-a-port-when-passed-multiple-ports ()
+  (auth-source-pass--with-store '(("bar.com:8080"))
+    (should (auth-source-pass-match-entry-p "bar.com:8080" "bar.com" nil '("http" "https" "80" "8080")))))
+
 (ert-deftest auth-source-pass--matching-entries-find-entries-with-slash ()
   ;; match if entry filename matches user
   (auth-source-pass--with-store '(("foo.com/user"))
-- 
2.23.0

bug#38284: 27.0.50; [PATCH] Make auth-source-pass-search understand port lists

[João Távora]

Damien Cassou <damien@cassou.me> writes:

> João Távora <joaotavora@gmail.com> writes:
>>> can you please rename "d" to "domain"?
>>
>> ok.  I do call your attention that it was already the
>> single letter n there
>
> yes I saw. This should never have been merged like that. Thank you for
> improving the code.

I think the single letter idiom is fine there.  It was just the wrong
letter.  By the way, I've left the p for the port, because calling it
"port", while it would work, would seriously confuse a reader.

>>> Can you please add a unit test covering this new use-case?
>>
>> No. This is too much work for such a trivial change
>
> I care a lot about the automated testing of the code I write.

Certainly, I care a lot, too.  I don't write tests for these changes out
of principle, not out of lazyness.  Most, if not all, the projects I
manage have automated tests.

> I won't try to convince you though. Can you please merge the attached
> patch with yours?

No, but you can do that, because it's your work (I can push it for you
though).  Anyway, now I read the test you wrote, I agree it's a good
test.  You are testing auth-source-pass-match-entry-p, much higher up
than auth-source-pass--generate-entry-suffixes, the function I changed.

Of course, only someone who was involved in the design would be able to
confidently place the tests at that correct level, the finding of which
is the most difficult part.

My advice and personal opinion is to later use this and more such tests
to perhaps redesign/cleanup the auth-source-pass.el library, which seems
needlessly complicated in the little stuff like the function I touched.
(to be fair it wasn't much helped by the style of the auth-source.el
parent library.)

João

bug#38284: 27.0.50; [PATCH] Make auth-source-pass-search understand port lists

[Stefan Kangas]

João Távora <joaotavora@gmail.com> writes:

> Damien Cassou <damien@cassou.me> writes:
>
>> Can you please merge the attached patch with yours?
>
> No, but you can do that, because it's your work (I can push it for you
> though).  Anyway, now I read the test you wrote, I agree it's a good
> test.  You are testing auth-source-pass-match-entry-p, much higher up
> than auth-source-pass--generate-entry-suffixes, the function I changed.
>
> Of course, only someone who was involved in the design would be able to
> confidently place the tests at that correct level, the finding of which
> is the most difficult part.

If the test is good, I think it should be installed.  Unfortunately,
it seems like it doesn't apply cleanly to current master.

Damien, could you please re-send the patch formatted by "git
format-patch -1"?  Please also include a commit message with a
ChangeLog entry as described in the CONTRIBUTE file.

Best regards,
Stefan Kangas

bug#38284: 27.0.50; [PATCH] Make auth-source-pass-search understand port lists

[Damien Cassou]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

Stefan Kangas <stefan@marxist.se> writes:
> If the test is good, I think it should be installed.  Unfortunately,
> it seems like it doesn't apply cleanly to current master.
>
> Damien, could you please re-send the patch formatted by "git
> format-patch -1"?  Please also include a commit message with a
> ChangeLog entry as described in the CONTRIBUTE file.

The patch was only meant to be added to João's own patch. Here is a
standalone one.

-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-test-lisp-auth-source-pass-tests.el-Test-for-multipl.patch --]
[-- Type: text/x-patch, Size: 1194 bytes --]

From 7e4c1aaa934f155e3aa3c69ba0a3460c1b6b90f3 Mon Sep 17 00:00:00 2001
From: Damien Cassou <damien@cassou.me>
Date: Tue, 21 Jan 2020 20:13:54 +0100
Subject: [PATCH] * test/lisp/auth-source-pass-tests.el: Test for multiple
 ports.

---
 test/lisp/auth-source-pass-tests.el | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/test/lisp/auth-source-pass-tests.el b/test/lisp/auth-source-pass-tests.el
index 10ed9c39fb..677abb33cc 100644
--- a/test/lisp/auth-source-pass-tests.el
+++ b/test/lisp/auth-source-pass-tests.el
@@ -353,6 +353,10 @@ auth-source-pass--matching-entries-find-entries-with-a-port
   (auth-source-pass--with-store '(("bar.com:8080"))
     (should (auth-source-pass-match-entry-p "bar.com:8080" "bar.com" nil "8080"))))
 
+(ert-deftest auth-source-pass--matching-entries-find-entries-with-a-port-when-passed-multiple-ports ()
+  (auth-source-pass--with-store '(("bar.com:8080"))
+    (should (auth-source-pass-match-entry-p "bar.com:8080" "bar.com" nil '("http" "https" "80" "8080")))))
+
 (ert-deftest auth-source-pass--matching-entries-find-entries-with-slash ()
   ;; match if entry filename matches user
   (auth-source-pass--with-store '(("foo.com/user"))
-- 
2.24.1

bug#38284: 27.0.50; [PATCH] Make auth-source-pass-search understand port lists

[Stefan Kangas]

Damien Cassou <damien@cassou.me> writes:

>> Damien, could you please re-send the patch formatted by "git
>> format-patch -1"?  Please also include a commit message with a
>> ChangeLog entry as described in the CONTRIBUTE file.
>
> The patch was only meant to be added to João's own patch.

Understood.

> Here is a standalone one.

Thanks, it seems to be working fine (the test passes), so I have now
pushed this to master as commit abb2515b0c.

I don't see anything more to do here, so I'm also closing this bug.

Best regards,
Stefan Kangas

bug#38284: 27.0.50; [PATCH] Make auth-source-pass-search understand port lists

[Damien Cassou]

Stefan Kangas <stefan@marxist.se> writes:
> I don't see anything more to do here, so I'm also closing this bug.

thank you

-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill