Edgewall Software
Modify

Opened 12 years ago

Closed 9 years ago

#7203 closed defect (duplicate)

smtp_default_domain resurrects addresses with ignored domains

Reported by: Stephen Compall <stephen.compall@…> Owned by: Emmanuel Blot
Priority: normal Milestone:
Component: notification Version: 0.11
Severity: normal Keywords: patch
Cc: alon.barlev@… Branch:
Release Notes:
API Changes:

Description

The notification options ignore_domains and smtp_default_domain interact to put in addresses like ignored-domain@default-domain when addresses with ignored domains are encountered.

The problem is illustrated with an extension of test_default_domain and fixed in trac.notification in the attached patch against 0.11rc1.

Attachments (3)

ign-dom-default-dom.diff (2.3 KB ) - added by Stephen Compall <stephen.compall@…> 12 years ago.
test and fix
Trac-0.11b2-strip-domain.patch (1.0 KB ) - added by alon.barlev@… 12 years ago.
Before creating smtp mail address, strip domain from first component
Trac-0.11b2-strip-domain.2.patch (2.1 KB ) - added by alon.barlev@… 12 years ago.
New patch, always strip domain before creating address.

Download all attachments as: .zip

Change History (20)

by Stephen Compall <stephen.compall@…>, 12 years ago

Attachment: ign-dom-default-dom.diff added

test and fix

comment:1 by alon.barlev@…, 12 years ago

Cc: alon.barlev@… added

Just fixed this with a different patch… Don't know what is better.

by alon.barlev@…, 12 years ago

Before creating smtp mail address, strip domain from first component

in reply to:  1 ; comment:2 by Stephen Compall <stephen.compall@…>, 12 years ago

Replying to alon.barlev@gmail.com:

Just fixed this with a different patch… Don't know what is better.

I am fairly sure this will fail the test I posted, for including otherjoe@default_domain. However, I did not try it.

in reply to:  2 comment:3 by alon.barlev@…, 12 years ago

Replying to Stephen Compall <stephen.compall@desktopdoctorsinc.com>:

Replying to alon.barlev@gmail.com:

Just fixed this with a different patch… Don't know what is better.

I am fairly sure this will fail the test I posted, for including otherjoe@default_domain. However, I did not try it.

I don't understand how your patch solves the issue…

I have kerberos user:

alon(at)DOMAIN.ORG

I have ignore domain DOMAIN.ORG and default domain mail.com, the result mail should be:

alon(at)mail.com

is_email() returns false for alon(at)DOMAIN.ORG, this is correct, but then the current result address is: alon(at)DOMAIN.ORG(at)mail.com, which is incorrect.

So my fixup just remove the "@.*" from address before constructing email addres, thus result in correct alon(at)mail.com.

While I think that your patch don't construct address with default domain, it just ignores it.

BTW: How do I write (at) in this wiki?

by alon.barlev@…, 12 years ago

New patch, always strip domain before creating address.

comment:4 by alon.barlev@…, 12 years ago

OK. New patch always strip the domain so that also use_short_addr will work. It should work:

if not email
   can still have @ignored domain
   but this is the user name so we cannot remove this right now.
   
   check email record for this user,
   if exists use this one skip the rest

   strip ignore domain if exists

   next is as-is while there is no domain name in name anymore.

BTW: I am not much of python coder…

comment:5 by alon.barlev@…, 12 years ago

Why is this fix was not applied or assigned to any valid milestone?

It is simple issue to fix.

Please make it fixed.

comment:6 by anonymous, 12 years ago

Milestone: 0.11.1
Version: 0.11rc10.11

comment:7 by Christian Boos, 12 years ago

manu, can you have a look at the patch?

in reply to:  7 comment:8 by Emmanuel Blot, 12 years ago

Replying to cboos:

manu, can you have a look at the patch?

I'll look at it tonight.

comment:9 by Christian Boos, 12 years ago

Ok, I'm not familiar with the domain ignore logic, but note that there could some interference with the recent change I did to the domain part of the e-mail regexp in r7400.

in reply to:  9 comment:10 by anonymous, 12 years ago

Replying to cboos:

Ok, I'm not familiar with the domain ignore logic, but note that there could some interference with the recent change I did to the domain part of the e-mail regexp in r7400.

Most of the complexity (and bugs) come from the original design choice to have a single input field that can be either used to specify a name or an email address. It's quite difficult to guess whether the field contains an email address or a user login - read: impossible to achieve a 100% recognition rate. Moreover, some authentication backends use the "@" character in the login name, which leads to even more issues.

The question is: should we keep using this unique field and add always more SMTP options in the trac.ini files - which has its own drawbacks, one of them is a unique configuration for all users - and keep tweaking regexps, or would it be better to drop this unique but handy field?

Dropping it would require major UI changes though…

in reply to:  9 ; comment:11 by anonymous, 11 years ago

Replying to cboos:

Ok, I'm not familiar with the domain ignore logic, but note that there could some interference with the recent change I did to the domain part of the e-mail regexp in r7400.

This pattern is incorrect, you cannot expect domain names to be at least two parts. Many installations I got within internal organization have single domain. Please revert.

Replying to anonymous:

Most of the complexity (and bugs) come from the original design choice…

I fully agree, why guess?

Maybe add a simple hook to gain email address, which can take username and return email address (by LDAP for example).

in reply to:  11 ; comment:12 by Emmanuel Blot, 11 years ago

Replying to anonymous:

This pattern is incorrect, you cannot expect domain names to be at least two parts. Many installations I got within internal organization have single domain. Please revert.

Before doing any more changes we need to define what we want to achieve, and what can be actually achieved, from a technical point of view. There have already been too many workarounds with email processing.

I fully agree, why guess? Maybe add a simple hook to gain email address, which can take username and return email address (by LDAP for example).

Define "simple hook" ;-)

  • Some installations work with local installation only, with a bare SMTP server
  • Some installations work with LDAP directories or other local email databases
  • Some installations work with the Internet, where it is pretty impossible to validate an email address
  • Some installations use a combination of the above typical configurations
  • etc.

SMTP servers are very complex pieces of software. Trying to address these very different environments at once is not as easy as a "simple hook".

in reply to:  12 comment:13 by alon.barlev@…, 11 years ago

Replying to eblot:

Before doing any more changes we need to define what we want to achieve, and what can be actually achieved, from a technical point of view. There have already been too many workarounds with email processing.

Current code try to guess the email address out of the user name.

It does so even if the user had configured email address. This is the first mistake.

The existence of @ in the username does not mean that it is email address, for example kerberos environment. But for this you added ignore domain configuration option.

But ignore domain option does not remove this domain so default domain option does not work.

The patch solves this issue, but better to first change the email field of user record first.

The revert of the new template has nothing to do with any future changes, as it is clearly wrong.

I believe the algorithm should be as follows:

if has explicit email in user record
    use it
else
    remove ignore domains from username (should be a list)
    if username looks like email address or use_short_addr
        use it
    else
        if smtp_default_domain
            use username@smtp_default_domain
        else
            fail

Define "simple hook" ;-)

Simple hook is a python code that you eval into the module. Because this is so complex, whoever wish to guess email should have an option to add external logic.

comment:14 by anonymous, 11 years ago

Resolution: fixed
Status: newclosed

comment:15 by anonymous, 11 years ago

Resolution: fixed
Status: closedreopened

comment:16 by Christian Boos, 11 years ago

Milestone: 0.11.20.11.3
Severity: minornormal

comment:17 by Christian Boos, 9 years ago

Milestone: next-minor-0.12.x
Resolution: duplicate
Status: reopenedclosed

Please see #1804 which seem to target the same root issue.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Emmanuel Blot.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Emmanuel Blot 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.