Edgewall Software
Modify

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#12658 closed defect (fixed)

RecipientMatcher prevents EmailResolver from working

Reported by: strk@… Owned by: Jun Omae
Priority: normal Milestone: 1.2.1
Component: notification Version: 1.2
Severity: normal Keywords:
Cc:
Release Notes:

Fix email address not resolving for user without email.

API Changes:

Description (last modified by Jun Omae)

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.

Attachments (2)

mail_recipient_matcher.patch (602 bytes ) - added by strk@… 2 years ago.
notification-mail-resolver.patch (582 bytes ) - added by strk@… 2 years ago.
Modified as suggested

Download all attachments as: .zip

Change History (35)

Changed 2 years ago by strk@…

comment:1 Changed 2 years ago by anonymous

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

comment:2 Changed 2 years ago by strk@…

Milestone: 1.2.1

I hope this can be in 1.2.1, setting the milestone

comment:3 Changed 2 years ago by Jun Omae

Description: modified (diff)
Owner: set to Jun Omae
Status: newassigned

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

comment:4 Changed 2 years ago by Jun Omae

Description: modified (diff)

Changed 2 years ago by strk@…

Modified as suggested

comment:5 Changed 2 years ago by strk@…

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

comment:6 Changed 2 years ago by Jun Omae

Milestone: 1.2.1next-stable-1.2.x

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.

Last edited 2 years ago by Jun Omae (previous) (diff)

comment:7 Changed 2 years ago by anonymous

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

comment:8 Changed 2 years ago by 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…

comment:9 Changed 2 years ago by 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)

comment:10 Changed 2 years ago by Jun Omae

Milestone: next-stable-1.2.x1.2.1

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

    diff --git a/trac/notification/mail.py b/trac/notification/mail.py
    index 3bd59d8f9..889c6ba11 100644
    a b class RecipientMatcher(object):  
    193193                               for x in notify_sys.ignore_domains_list]
    194194
    195195        # Get the name and email addresses of all known users
     196        self.usernames = set()
    196197        self.name_map = {}
    197198        self.email_map = {}
    198199        for username, name, email in self.env.get_known_users():
     200            self.usernames.add(username)
    199201            if name:
    200202                self.name_map[username] = name
    201203            if email:
    class RecipientMatcher(object):  
    217219            return None
    218220        sid = None
    219221        auth = 0
    220         if address in self.email_map:
     222        if address in self.usernames:
    221223            sid = address
    222224            auth = 1
    223             address = self.email_map[address]
     225            if sid not in self.email_map:
     226                return sid, auth, None
     227            address = self.email_map[sid]
    224228        elif not is_email(address) and self.nodomaddr_re.match(address):
    225229            if self.env.config.getbool('notification', 'use_short_addr'):
    226230                return (None, 0, address)

comment:11 Changed 2 years ago by anonymous

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

comment:12 in reply to:  11 Changed 2 years ago by 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.

comment:13 Changed 2 years ago by anonymous

import trac.tests
ImportError: No module named tests
Makefile:367: recipe for target 'unit-test' failed
Last edited 2 years ago by Ryan J Ollos (previous) (diff)

comment:14 in reply to:  13 Changed 2 years ago by 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.

comment:15 Changed 2 years ago by 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)
Last edited 2 years ago by Jun Omae (previous) (diff)

comment:16 Changed 2 years ago by 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)

comment:17 Changed 2 years ago by Jun Omae

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

Last edited 2 years ago by Jun Omae (previous) (diff)

comment:18 Changed 2 years ago by 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.

comment:19 Changed 2 years ago by 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__().

comment:20 Changed 2 years ago by 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 :)

comment:21 Changed 2 years ago by 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(+)

comment:22 in reply to:  21 Changed 2 years ago by 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
...

comment:23 Changed 2 years ago by 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

comment:24 Changed 2 years ago by 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 ?

comment:25 Changed 2 years ago by 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.

comment:26 Changed 2 years ago by 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 ?

comment:27 in reply to:  17 Changed 2 years ago by 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.

comment:28 Changed 2 years ago by Jun Omae

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

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

comment:29 Changed 2 years ago by anonymous

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

comment:30 Changed 2 years ago by 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.

comment:31 Changed 2 years ago by 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 ?

comment:32 Changed 2 years ago by 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

comment:33 Changed 2 years ago by 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 ?

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jun Omae.
The resolution will be deleted.
to The owner will be changed from Jun Omae to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.