New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow users to send emails from alias domains #1631

Merged
merged 5 commits into from Jan 31, 2019

Conversation

Projects
None yet
2 participants
@alexander255
Copy link
Contributor

alexander255 commented Dec 9, 2018

Description of the issue/feature this PR addresses:

Currently, if I have a domain bla.com that is an alias of blub.com and John has user account as john@blub.com, then will not be able to send e-mails as john@bla.com because the system will not recognize / communicate to Postfix that john@bla.com is the same as john@blub.com and hence Postfix will reject this attempt.

Proposed Solution:

When calculating the allowed SASL usernames for an email address, also include aliases from aliased domains in the result of the query. (This will also work for the primary username, since that is added as “internal” alias to that table as well.)

Current behavior before PR:

John is not able to send emails as john@bla.com.

Desired behavior after PR is merged:

John is able to send emails as john@bla.com, but still not as marry@bla.com.

Caveats:

I only tested the proposed query with PostgreSQL.

Allow users to send emails from alias domains
### Problem

Currently, if I have a domain `bla.com` that is an alias of `blub.com` and John has user account as `john@blub.com`, then will not be able to send e-mails as `john@bla.com` because the system will not recognize / communicate to Postfix that `john@bla.com` is the same as `john@blub.com` and hence Postfix will reject this attempt.

### Proposed Solution

When calculating the allowed SASL usernames for an email address, also include aliases from aliased domains in the result of the query. (This will also work for the primary username, since that is added as “internal” alias to that table as well.)

### Caveats

I only tested the proposed query with PostgreSQL.
@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 9, 2018

Codecov Report

Merging #1631 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1631      +/-   ##
==========================================
- Coverage   86.71%   86.69%   -0.03%     
==========================================
  Files         150      150              
  Lines        7899     7899              
==========================================
- Hits         6850     6848       -2     
- Misses       1049     1051       +2
@alexander255

This comment has been minimized.

Copy link
Contributor Author

alexander255 commented Dec 10, 2018

Letting a PR sit is the best way to have its quality naturally improve 😉

I just discovered a big issue with the previous query: Since I used INNER JOIN instead of LEFT JOIN no results will ever be produced by the alias lookup if a domain does not have any aliases. Will fix this shortly.

"LEFT JOIN admin_domainalias adom ON adom.target_id=dom.id "
"WHERE al.enabled=1 AND ("
" al.address='%s' OR ("
" adom.name='@d' AND al.address='%u'||'@'||dom.name"

This comment has been minimized.

@tonioo

tonioo Jan 21, 2019

Member

What's the meaning of adom.name='@d' ? And are you sure the || works as you expect? Maybe concat would be safer?

This comment has been minimized.

@alexander255

alexander255 Jan 23, 2019

Author Contributor

Took me forever to get what your actual question was: Yes, it should have been '%d' not '@d' of course. Also I fixed the MySQL-ism wrt concat, I didn't know MySQL was being special here again.

@alexander255 alexander255 force-pushed the alexander255:patch-1 branch from 9c6484e to fa59ca8 Jan 23, 2019

Compare aliased domain with domain of alias rather then mailbox when …
…the alias domain is different from the mailbox one

@alexander255 alexander255 force-pushed the alexander255:patch-1 branch from fa59ca8 to e676261 Jan 23, 2019

@alexander255

This comment has been minimized.

Copy link
Contributor Author

alexander255 commented Jan 23, 2019

Here's the explanation I wrote while trying to understand your question, I've checked the queries again and compared all three versions for equivalency and -isms and things should hopefully be OK now 🙂

Assuming we have:

  • Mailbox: .id=10; .domain_id=30; .address='john';…
  • Alias-Recipient: .id=20; .r_mailbox_id=10; .alias_id=40; …
  • Domain: .id=30; .name='example.com'; …
  • Domain: .id=31; .name='example.org'; …
  • Alias: .id=40; .enabled=true; .address='dummy@example.org'; .domain_id=31; …
  • Domain-Alias: .id=50; .name='example.net'; .target_id=31; …

User john@example.com should be allowed send messages using at least any of the following addresses:

  • john@example.com (due to internal alias for user, not shown above)
  • dummy@example.org (due to alias)
  • dummy@example.net (due to domain alias on domain used in alias)

The first two already work, but the last one doesn't since there will not be any alias entries generated for domain aliases (there won't ever be an entry with address dummy@example.net in the alias table in our example). This change fixes this by extending the query to also allow for all aliases whose domain ID is being aliased in the Domain-Alias table.

The extension of query part therefore says: Also allow users to send messages if the domain-name of the message matches a domain alias whose aliasing target domain name matches an alias with the message's username and that target domain. (Read it a couple of times and it may make sense…)

@tonioo

This comment has been minimized.

Copy link
Member

tonioo commented Jan 31, 2019

@alexander255 Makes sense. I've read the query a couple of times to make sure I understand it well ;)

@tonioo tonioo merged commit d1a5253 into modoboa:master Jan 31, 2019

3 of 4 checks passed

codecov/project 86.69% (-0.03%) compared to 511a9c3
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
codecov/patch Coverage not affected when comparing 511a9c3...e676261
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tonioo tonioo added the enhancement label Jan 31, 2019

@tonioo tonioo added this to the 1.13.1 milestone Jan 31, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment