Re: master 421eeff: Add support for multiple Gravatar services

[philip@warpmail.net (Philip K.)]

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


I was re-reading the libravatar specification today, and noticed that I
had missed a section that described what to do if a DNS query results in
multiple records. The current implementation just used a "simple"
(dns-query ...) but that is explicit not sufficient. Instead one must
choose the highest priority servers and then randomly select one of
these, according to their weights. The patch I have attached to this
mail implements this, and can be tested by evaluating

	(gravatar--service-libravatar "test@pusling.com")

It's rather hard to find a domain with multiple libravatar entries, but
the selection algorithm can be tested using this slightly modified
excerpt from the patch:

    (defun test-selection ()
      (catch 'found
        ;; res is a hardcoded and simplified (dns-query "..." t) result.
        ;; "target" would usually point to a domain.
        (let* ((res '(((target alpha) (priority 10) (weight 10))
                      ((target beta) (priority 10) (weight 0))
                      ((target gamma) (priority 10) (weight 30))
                      ((target delta) (priority 5) (weight 10))))
               (prio (mapcar (lambda (r) (dns-get 'priority r)) res))
               (max (apply #'max prio))
               (sum 0) top)
          (dolist (rec res)
            (when (= max (dns-get 'priority rec))
              (setq sum (+ sum (dns-get 'weight rec)))
              (push rec top)))
          (dolist (rec top)
            (when (<= (if (= 0 sum) -1 (random sum))
                      (dns-get 'weight rec))
              (throw 'found (dns-get 'target rec)))
            (setq sum (- sum (dns-get 'weight rec)))))))

    (/ (cl-loop repeat 1000 count (eq (test-selection) 'gamma)) 1000.0)

where the last expression should approximately equal 0.75 (3/4), because
only the records alpha, beta and gamma have the highest priority (10),
and between them gamma has a 30/(10 + 30) = 3/4'th chance of being
chosen. In case all records are weighted 0, the first one is chosen.

I once again apologise for not thoroughly enough testing my last patch,
and hope this at least somewhat makes up for it.

-- 
	Philip K.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-Libravatar-federation-handling.patch --]
[-- Type: text/x-diff, Size: 2924 bytes --]

From 6191aa6577db1056bb74a4359d87a119e9517aca Mon Sep 17 00:00:00 2001
From: Philip K <philip@warpmail.net>
Date: Tue, 31 Mar 2020 13:23:09 +0200
Subject: [PATCH] Fix Libravatar federation handling

Previous implementation didn't correctly handle the result of
dns-query, and didn't implement the necessary record selection
algorithm as specified on the wiki (https://wiki.libravatar.org/api/,
"Federated servers").

* lisp/image/gravatar.el (gravatar--service-libravatar): Fix
libravatar image host resolver algorithm.
---
 lisp/image/gravatar.el | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/lisp/image/gravatar.el b/lisp/image/gravatar.el
index ff59a72ac8..d800e5dc55 100644
--- a/lisp/image/gravatar.el
+++ b/lisp/image/gravatar.el
@@ -149,12 +149,33 @@ gravatar--service-libravatar
           (dolist (record '(("_avatars-sec" . "https")
                             ("_avatars" . "http")))
             (let* ((query (concat (car record) "._tcp." domain))
-                   (result (dns-query query 'SRV)))
+                   (result (dns-query query 'SRV t)))
               (when result
-                (throw 'found (format "%s://%s/avatar"
-                                      (cdr record)
-                                      result)))))
-          "https://seccdn.libravatar.org/avatar")))))
+                (let* ((res (mapcar (lambda (rec)
+                                      (dns-get 'data (cdr rec)))
+                                    (dns-get 'answers result)))
+                       (prio
+                        (mapcar (lambda (r) (dns-get 'priority r)) res))
+                       (max (apply #'max prio))
+                       (sum 0) top)
+                  ;; Attempt to find all records with the same maximal
+                  ;; priority, and calculate the sum of their weights.
+                  (dolist (rec res)
+                    (when (= max (dns-get 'priority rec))
+                      (setq sum (+ sum (dns-get 'weight rec)))
+                      (push rec top)))
+                  ;; In case there is more than one maximal priority
+                  ;; record, choose one at random, while taking the
+                  ;; individual record weights into consideration.
+                  (dolist (rec top)
+                    (when (<= (if (= 0 sum) -1 (random sum))
+                              (dns-get 'weight rec))
+                      (throw 'found (format "%s://%s:%s/avatar"
+                                            (cdr record)
+                                            (dns-get 'target rec)
+                                            (dns-get 'port rec))))
+                    (setq sum (- sum (dns-get 'weight rec)))))))
+            "https://seccdn.libravatar.org/avatar"))))))
 
 (defun gravatar-hash (mail-address)
   "Return the Gravatar hash for MAIL-ADDRESS."
-- 
2.20.1