RecipientMatcher prevents EmailResolver from working

[strk@…]

As reported on ​the users mailing list I could not succeed in installing a custom EmailResolver without changing the RecipientMatcher class to give callers a chance to lookup names.

I'm attaching a patch.

[anonymous]

I've completed my first trac plugin implementation, which relies on this patch to be functional: ​https://git.osgeo.org/gogs/sac/TracLDAPEmailResolverPlugin

[strk@…]

I hope this can be in 1.2.1, setting the milestone

[Jun Omae]

I consider suggested patch is incorrect. It should return (sid, 1, None) only when the user is listed in Environment.get_known_users().

[strk@…]

Modified as suggested

[strk@…]

Only checking in known users works for me, updated patch attached: notification-mail-resolver.patch

[Jun Omae]

Your new patch doesn't work. self.email_map is generated from Environment.get_known_users(). It confirmed that address is not listed by if address in self.email_map.

I'll verify this issue and rework it. Also, we must add unit tests.

[anonymous]

I've actually tested my patch, it worked for me (worked with my plugin). Anyway, looking forward to your version of it.

[anonymous]

re-reading your comment and the code: it is confirmed that *address* (which at that point is sid) is not in the email_map, but we're checking for username, not email. I guess we could lookup in self.name_map instead of doing the for loop though, would be clearer…

[anonymous]

Sorry, taking it back, name_map is for real names, not usernames (which is what I thought you were after, when you asked for the name to be in get_known_users)

[Jun Omae]

I'm working now. I modified the matcher class and added unit tests but trac/ticket/tests/notification.py has 2 failures.

trac/notification/mail.py

@@ -193 +193 @@
                                for x in notify_sys.ignore_domains_list]
 
         # Get the name and email addresses of all known users
+        self.usernames = set()
         self.name_map = {}
         self.email_map = {}
         for username, name, email in self.env.get_known_users():
+            self.usernames.add(username)
             if name:
                 self.name_map[username] = name
             if email:
@@ -217 +219 @@
             return None
         sid = None
         auth = 0
-        if address in self.email_map:
+        if address in self.usernames:
             sid = address
             auth = 1
-            address = self.email_map[address]
+            if sid not in self.email_map:
+                return sid, auth, None
+            address = self.email_map[sid]
         elif not is_email(address) and self.nodomaddr_re.match(address):
             if self.env.config.getbool('notification', 'use_short_addr'):
                 return (None, 0, address)

[anonymous]

Patch looks good, how can I run the notification test to help debugging ?

[Ryan J Ollos]

Replying to anonymous:

Patch looks good, how can I run the notification test to help debugging ?

Running tests is described in TracDev/UnitTests.

[anonymous]

import trac.tests
ImportError: No module named tests
Makefile:367: recipe for target 'unit-test' failed

[Ryan J Ollos]

Replying to anonymous:

import trac.tests
ImportError: No module named tests
Makefile:367: recipe for target 'unit-test' failed

TracDev/UnitTests#ImportError:nomodulenamedtests

Please direct any additional questions not directly related to the reported issue to the MailingList.

[strk@…]

Ok so the failing test, which can be reproduced with:

make test=trac.ticket.tests.notification

It's fixed by applying this patch:

diff --git a/trac/notification/mail.py b/trac/notification/mail.py
index 3bd59d8..cad7636 100644
--- a/trac/notification/mail.py
+++ b/trac/notification/mail.py
@@ -246,6 +249,8 @@ class RecipientMatcher(object):
         if not recipient:
             return None
         sid, authenticated, address = recipient
+        if not address:
+            return None
         from_name = None
         if sid and authenticated:
             from_name = self.name_map.get(sid)

[strk@…]

Inlining the whole version of my patch, as previous comment had broken formatting. I confirm it does work for my case still.

diff --git a/trac/notification/mail.py b/trac/notification/mail.py
index 3bd59d8..cad7636 100644
--- a/trac/notification/mail.py
+++ b/trac/notification/mail.py
@@ -229,6 +229,9 @@ class RecipientMatcher(object):
             if domain:
                 address = "%s@%s" % (address, domain)
             else:
+                for username, name, email in self.env.get_known_users():
+                  if username == address:
+                    return (address, 1, None)
                 self.env.log.info("Email address w/o domain: %s", address)
                 return None
 
@@ -246,6 +249,8 @@ class RecipientMatcher(object):
         if not recipient:
             return None
         sid, authenticated, address = recipient
+        if not address:
+            return None
         from_name = None
         if sid and authenticated:
             from_name = self.name_map.get(sid)

[Jun Omae]

WIP: jomae.git@t12658. We must add unit tests if fixing this issue (IEmailAddressResolver).

[anonymous]

What's the public URL of your git repo for me to add it as a remote ? It isn't shown in that page.

[Jun Omae]

What's the public URL of your git repo for me to add it as a remote ? It isn't shown in that page.

See TracTeam/Repositories.

@@ -229,6 +229,9 @@ class RecipientMatcher(object):
             if domain:
                 address = "%s@%s" % (address, domain)
             else:
+                for username, name, email in self.env.get_known_users():
+                  if username == address:
+                    return (address, 1, None)
                 self.env.log.info("Email address w/o domain: %s", address)
                 return None

I don't think get_known_users() should be called per each call of match_recipient(). We could easily cache result of get_known_users() in RecipientMatcher.__init__().

[anonymous]

Agreed about caching, was just too lazy to apply your patch. Will do as soon as I handle to setup a git repo with a nearby common root with yours :)

[strk@…]

Here's my fork for you to pull (and the fix to the testsuite) — I noted you're tracking 1.3 though, this will need to be backported to 1.2 when ready.

The following changes since commit 39fc0915aabed5457f79c9015d6a926d183f563e:

  (#12658) refactor (2017-01-13 12:09:20 +0900)

are available in the git repository at:

  https://git.osgeo.org/gogs/strk/trac.git t12658.strk

for you to fetch changes up to c2342f2d76604947632d38dc66d8c842a266264b:

  Fix using smtp_from for users with unknown email (2017-01-13 09:47:51 +0100)

----------------------------------------------------------------
Sandro Santilli (1):
      Fix using smtp_from for users with unknown email

 trac/notification/mail.py | 2 ++
 1 file changed, 2 insertions(+)

[Jun Omae]

Replying to strk@…:

I noted you're tracking 1.3 though, this will need to be backported to 1.2 when ready.

No. jomae.git@t12658 is based on 1.2-stable branch in git mirror repository.

$ git log --all --oneline --graph --decorate
* 39fc0915a (HEAD, jomae/t12658, t12658) (#12658) refactor
* 4ec1bafb0 (#12658) fix not calling `IEmailAddressResolver.get_address_for_session` for session user without email
*   c8aa5948f (mirror/1.2-stable) 1.2.1dev: merge [15342] from 1.0-stable (fix for #12655)
|\
* | 3d3efec2a 1.2.1dev: Avoid recursion in permission policy checks
* | a0112f807 1.2.1dev: Raise `ConfigurationError` is authz file not found
...

[anonymous]

Oh sorry, was confused again. Great then! I confirm things work for me as of comit c2342f2d76604947632d38dc66d8c842a266264b in my fork, and make test=trac.ticket.tests also works fine here

[strk@…]

I was thinking I actually have a problem with only resolving emails for *known* users, as that means being unable to notify someone who is known by LDAP but not known yet by Trac. Why do you think it is important to limit the resolution to the "known" users ?

[Jun Omae]

The email address should normally be resolved by user management api rather than email-resolver api. The email-resolver api is not needed but lack of user management api in Trac. See also TracDev/Proposals/UserSystem and #2456.

[anonymous]

I don't understand this sentence: "the email-resolver api is not needed but lack of user management api"

Do you mean the email-resolver API is a temporary hack to work around the lack of user management API ?

[Peter Suter]

(I just noticed this also affects ​th:OnSiteNotificationsPlugin. Users without email address in session do not receive on-site notifications, due to "Email address w/o domain".)

WIP: jomae.git@t12658.

Looks good to me.

[Jun Omae]

Thanks for the reviewing! Committed in [15356] and merged in [15357].

[anonymous]

Thanks for the fast fix ! Is there an ETA for 1.2.1 ?

[Ryan J Ollos]

Maybe early February for 1.2.1. That would make it 3 months since the release of 1.2. Still have to find time to release 1.0.14 first though.

[anonymous]

I still need to be able to resolve mails of unknown users though, to be able to reproduce our current custom modification. This is obtained with this simple patch: ​https://git.osgeo.org/gogs/strk/trac/commit/a72b9176f11b93629b4c96c93f131545d1b0e7b1

Does it take another ticket to track ?

[Jun Omae]

RecipientMatcher.match_recipient returns a tuple of (sid, authenticated, address) or None. A pair of sid and authenticated should be sid and authenticated in session and notify_subscription table. Therefore, we cannot apply it.

sqlite> SELECT * FROM session WHERE authenticated=1 LIMIT 1;
sid         authenticated  last_visit
----------  -------------  ----------
jomae       1              1477454257
sqlite> SELECT * FROM session WHERE authenticated=0 LIMIT 1;
sid                       authenticated  last_visit
------------------------  -------------  ----------
e24c31920aa41aa1320afba1  0              1484733702

[anonymous]

RecipientMatcher.match_recipient returns a tuple of (sid, authenticated, address) or None.

But what does it take in input ? What's the role of that method, exactly ? Should email resolution be done by the caller then, once None is returned ?