failed attempts to use a custom EmailResolver

[Sandro Santilli]

I've been trying all day to make the LdalEmailAddressResolver
work for my setup but failed miserably.

The example:
https://trac.edgewall.org/wiki/TracDev/PluginDevelopment/ExtensionPoints/trac.notification.api.IEmailAddressResolver#Examples

My setup is Apache taking care of the authentication so that trac
only has usernames to work with, and no emails at all.

Adding debugging code brought me to understand that the "distribute"
method of the INotificationDistributor implementation is not even
entered unless the RecipientMatcher provides it with an address,
which is not the case when the domain is unknown. Code block I'm
looking at is notification/mail.py:235:

elif not is_email(address) and self.nodomaddr_re.match(address):
if self.env.config.getbool('notification', 'use_short_addr'):
return (None, 0, address)
domain = self.env.config.get('notification',
'smtp_default_domain')
if domain:
address = "%s@%s" % (address, domain)
else:
self.env.log.info("Email address w/o domain: %s", address)
return None

There we usually plug our custom code, looking up email from LDAP
instead o complaining "Email address w/out domain". I was hoping this
would be done automatically but there's clearly no code doing that
in that method. Is it a bug or a design choice for some reason ?

I've been logging some of my findings in the ticket but then was asked
to continue here: https://trac.edgewall.org/ticket/3162#comment:16

Background information about our customization can be found here:
https://trac.osgeo.org/osgeo/ticket/39

Thanks in advance for any info you may give me to help with dropping the
custom modification and finally be upgrade-happy again.

--strk;

[Peter Suter]

It might be a bug (or a scenario not supported so far).
Could you try replacing that last "return None" with "return address, 1, None"?

Announcer uses "(sid, 1, None)" when an email address is not recognized:
https://trac-hacks.org/browser/announcerplugin/trunk/announcer/subscribers.py#L151

We should maybe use "(sid, 1, None)" if RecipientMatcher returns None.

 

I've been logging some of my findings in the ticket but then was asked
to continue here: https://trac.edgewall.org/ticket/3162#comment:16

Background information about our customization can be found here:
https://trac.osgeo.org/osgeo/ticket/39

Thanks in advance for any info you may give me to help with dropping the
custom modification and finally be upgrade-happy again.

--strk;

Thanks,
Peter

[Sandro Santilli]

On Wed, Jan 11, 2017 at 10:47:41AM -0800, Peter Suter wrote:

> It might be a bug (or a scenario not supported so far).
> Could you try replacing that last "return None" with "return address, 1,
> None"?

I got to the same point already, which gave me:

2017-01-11 22:26:17,280 Trac[web_ui] ERROR: Failure sending notification on change to ticket #777: TypeError: get_address_for_session() takes exactly 2 arguments (3 given)

Hadn't figured out why it's said to take 2 args as I only see
a 3-args one (first of which being "self"). Unfortunately there's no
stack trace in that error.

> Announcer uses "(sid, 1, None)" when an email address is not recognized:
> https://trac-hacks.org/browser/announcerplugin/trunk/announcer/subscribers.py#L151
>
> We should maybe use "(sid, 1, None)" if RecipientMatcher returns None.

There's no documentation about what RecipientMatcher is supposed to do
so I cannot comment about this.

--strk;

[Sandro Santilli]

On Wed, Jan 11, 2017 at 10:32:25PM +0100, Sandro Santilli wrote:
> On Wed, Jan 11, 2017 at 10:47:41AM -0800, Peter Suter wrote:
>
> > It might be a bug (or a scenario not supported so far).
> > Could you try replacing that last "return None" with "return address, 1,
> > None"?
>
> I got to the same point already, which gave me:
>
> 2017-01-11 22:26:17,280 Trac[web_ui] ERROR: Failure sending notification on change to ticket #777: TypeError: get_address_for_session() takes exactly 2 arguments (3 given)
>
> Hadn't figured out why it's said to take 2 args as I only see
> a 3-args one (first of which being "self"). Unfortunately there's no
> stack trace in that error.

Bingo, it's actually working (was my plugin being bogus, as the Example
I was following [1]). Should I send a patch for it ?

[1] https://trac.edgewall.org/wiki/TracDev/PluginDevelopment/ExtensionPoints/trac.notification.api.IEmailAddressResolver#Examples

So the change in mail.py works great:

233c235,236
< return None
---
> #return None
> return (address, 1, None)

Only maybe we should also drop the complaint:

INFO: Email address w/o domain: xxx

As that'd be a legit kind of "recipient" when a resolver is in use

--strk;

[Peter Suter]

On 11.01.2017 22:32, Sandro Santilli wrote:

2017-01-11 22:26:17,280 Trac[web_ui] ERROR: Failure sending notification on change to ticket #777: TypeError: get_address_for_session() takes exactly 2 arguments (3 given)

The LdapEmailAddressResolver example was missing the self parameter:
https://trac.edgewall.org/wiki/TracDev/PluginDevelopment/ExtensionPoints/trac.notification.api.IEmailAddressResolver?action=diff&version=6

[Sandro Santilli]

On Wed, Jan 11, 2017 at 10:44:38PM +0100, Peter Suter wrote:
> On 11.01.2017 22:32, Sandro Santilli wrote:
> >2017-01-11 22:26:17,280 Trac[web_ui] ERROR: Failure sending notification on change to ticket #777: TypeError: get_address_for_session() takes exactly 2 arguments (3 given)
>

> The LdapEmailAddressResolverexample was missing the self parameter:
> https://trac.edgewall.org/wiki/TracDev/PluginDevelopment/ExtensionPoints/trac.notification.api.IEmailAddressResolver?action=diff&version=6

Yes, thanks for fixing that.
But the patch to RecipientMatcher is still needed for the resolver
to be called. I filed it here:
https://trac.edgewall.org/ticket/12658

--strk;

[RjOllos]

The TracLDAPEmailResolverPlugin has inconsistent indentation, some 2-space and some 4-space. You could fix with:

$ pip install reindent 

$ cd TracLDAPEmailResolverPlugin

$ reindent -r -n .

- Ryan

[Sandro Santilli]

On Thu, Jan 12, 2017 at 01:41:15PM -0800, RjOllos wrote:
> On Thursday, January 12, 2017 at 1:15:59 AM UTC-8, Sandro Santilli wrote:
>
> > But the patch to RecipientMatcher is still needed for the resolver
> > to be called. I filed it here:
> > https://trac.edgewall.org/ticket/12658
>

> The TracLDAPEmailResolverPlugin has inconsistent indentation, some 2-space
> and some 4-space.

Fixed, thanks :)
But even if a patch landed in 1.2 branch to fix RecipientMatcher so
to allow the plugin for being used, it still only works for people
that logged in at least once onto trac, which is not what we
want for our use case. The thing is we might want to be able to
notify (ie: put in Cc by nickname) someone who never logged yet
(but we know he can log, being known by the LDAP database also used
for authorizing logins).

Is there any reason why the RecipientMatcher should be checking
for "known" users and only attempt to resolve emails for them ?

--strk;

[Sandro Santilli]

This simple patch (also referenced in the ticket now) is what allows
LDAP email to be retrived even for users that hadn't logged in yet:
https://git.osgeo.org/gogs/strk/trac/commit/a72b9176f

I've added a note about that in the ticket too (12658) but that ticket
is closed now, so not sure how to proceed.

--strk;

[Peter Suter]

On 17.01.2017 11:01, Sandro Santilli wrote:
>> But even if a patch landed in 1.2 branch to fix RecipientMatcher so
>> to allow the plugin for being used, it still only works for people
>> that logged in at least once onto trac, which is not what we
>> want for our use case. The thing is we might want to be able to
>> notify (ie: put in Cc by nickname) someone who never logged yet
>> (but we know he can log, being known by the LDAP database also used
>> for authorizing logins).
>>
>> Is there any reason why the RecipientMatcher should be checking
>> for "known" users and only attempt to resolve emails for them ?
> This simple patch (also referenced in the ticket now) is what allows
> LDAP email to be retrived even for users that hadn't logged in yet:
> https://git.osgeo.org/gogs/strk/trac/commit/a72b9176f
>
> I've added a note about that in the ticket too (12658) but that ticket
> is closed now, so not sure how to proceed.

The simple patch may work for your use-case, but in general it's sadly
not quite the right solution.
I think the problem is that we can't really know if these are actually
usernames, and at the moment the only thing we can reasonably do is
check against the known usernames.

As linked in the ticket, the right long-term solution would be the
missing user API.
https://trac.edgewall.org/ticket/2456
But it still seems quite unclear what that should even do. The
requirements seem a bit divergent.
With enough perseverance you could try to come up with a proposal and
implementation for that. I don't want to discourage you, but to be
honest I doubt it will be a simple small fix.

As a possible workaround for you, could you use (maybe in a scheduled
script?) "trac-admin session add sid" to make all the users known?
As a last (and not very satisfying) resort, you could also replace all
the INotificationSubscribers with your own implementations that work
like you want I guess. :/

Best regards,
Peter

[Sandro Santilli]

On Tue, Jan 17, 2017 at 07:34:34PM +0100, Peter Suter wrote:

> >https://git.osgeo.org/gogs/strk/trac/commit/a72b9176f

> I think the problem is that we can't really know if these are actually
> usernames, and at the moment the only thing we can reasonably do is check
> against the known usernames.

What else could they be ?
The RecipientMatcher class is not documented so it is not clear
what the expected input is.

> As a possible workaround for you, could you use (maybe in a scheduled
> script?) "trac-admin session add sid" to make all the users known?

In each of the dozen instances.. not so nice...

> As a last (and not very satisfying) resort, you could also replace all the
> INotificationSubscribers with your own implementations that work like you
> want I guess. :/

This would be no different than applying the patch above, right ?

--strk;

[Peter Suter]

Sorry, I just noticed some of your mails ended up in my spam folder for
some reason.

On 18.01.2017 09:43, Sandro Santilli wrote:
> On Tue, Jan 17, 2017 at 07:34:34PM +0100, Peter Suter wrote:
>> I think the problem is that we can't really know if these are actually
>> usernames, and at the moment the only thing we can reasonably do is check
>> against the known usernames.
> What else could they be ?
> The RecipientMatcher class is not documented so it is not clear
> what the expected input is.

The input is some string found in a ticket field. It could be an email
address, a username or just a plain name, or even something (anything) else.
RecipientMatcher is trying to find matching known usernames or valid
email addresses that could be used as recipients.
The reason that RecipientMatcher does exactly that is mainly backward
compatibility. Those are the things that have been supported
historically, so they should continue to work.

>> As a last (and not very satisfying) resort, you could also replace all the
>> INotificationSubscribers with your own implementations that work like you
>> want I guess. :/
> This would be no different than applying the patch above, right ?

It would only be different in that you don't have to patch the source.

[Sandro Santilli]

On Wed, Jan 25, 2017 at 07:56:36PM +0100, Peter Suter wrote:
> On 18.01.2017 09:43, Sandro Santilli wrote:
> >
> >The RecipientMatcher class is not documented so it is not clear
> >what the expected input is.
>
> The input is some string found in a ticket field. It could be an email
> address, a username or just a plain name, or even something (anything) else.
> RecipientMatcher is trying to find matching known usernames or valid email
> addresses that could be used as recipients.

In our case we need the EmailResolver to tell what can or cannot be used
as a recipient, but RecipientMatcher does not use EmailResolver.
The *caller* does, and only does IF RecipientMatcher does not return None,
which currently does unless the "sid" matches an authenticated user.

> The reason that RecipientMatcher does exactly that is mainly backward
> compatibility. Those are the things that have been supported historically,
> so they should continue to work.

Do you think EmailResolver returning a name even if it's not authenticated
could break something ? The returned tuple does have an "authenticated"
field, so could be used by caller not willing to deal with non-authenticated
strings ...

https://git.osgeo.org/gogs/strk/trac/commit/a72b9176f

> >>As a last (and not very satisfying) resort, you could also replace all the
> >>INotificationSubscribers with your own implementations that work like you
> >>want I guess. :/
> >This would be no different than applying the patch above, right ?
> It would only be different in that you don't have to patch the source.

Got it, but sounds overkill at this stage.

--strk;

[Peter Suter]

On 26.01.2017 14:04, Sandro Santilli wrote:

Do you think EmailResolver returning a name even if it's not authenticated
could break something ? The returned tuple does have an "authenticated"
field, so could be used by caller not willing to deal with non-authenticated
strings ...

I assume you meant "... RecipientMatcher returning a name ...".

In Trac sid means session id, which can identify either an authenticated (logged in) session or an unauthenticated (anonymous) session.
So a "non-authenticated sid" would still be expected to identify an existing (anonymous) session.
So anonymous users could possibly hijack - and their preferences influence - the notifications intended for such non-existing "sid".
I can't see anything else that would obviously break in Trac itself so far, but there are several potential ways in which notification-related plugins could be affected.

Peter