JAMES-4213 Check whether alias target is valid when receiving email for alias#3073
JAMES-4213 Check whether alias target is valid when receiving email for alias#3073felixauringer wants to merge 2 commits into
Conversation
|
I have not adapted the tests yet, so they will probably fail. |
felixauringer
changed the title
|
My point of view is that if there's an alias or somehting alike we manages this and if we manages this it should come in. Some people actually have RRT mappings without local mailbox which is valid. And used. I'd favour an approach that prevent invalid RRT creation in the first place and/or removes/correct RRT as needed, |
Just out of curiosity: What does happen after such RRT mappings have been applied? Is the email forwarded? I thought those emails just land in the error repository in all cases.
As this (what I thought of as a bug) does not really bother us, I sadly have not the capacity to do a refactoring like that. |
Yes
It depends on the mailetcontainer structure
Then we can close ? But thanks a lot for the report and feedback: challenging the software that way is definitly very beneficial ;-) |
Depends. In the Jira Issue you said that this stricter check could be behind a setting. I would be willing to implement that. Additionally, I think having one method that checks the RRT table and one that checks the existence of the mailbox still makes sense for extensibility. What do you think? |
Ok
Not very opinionated I'd need to see how this looks like TBH |
felixauringer
force-pushed
the
fix-invalid-alias-target
branch
2 times, most recently
from
bacec8a to
7819a46
Compare
See the newest commit in this PR (7819a46). |
This enhances the extensibility as only one of them can be overwritten without having to touch the logic of the other.
felixauringer
force-pushed
the
fix-invalid-alias-target
branch
from
b29e8cb to
49e5f60
Compare
This relies on apache/james-project#3073. Because the RRT is applied only in the parent class, the functionality that must be tested is smaller so that I could reduce the complexity of the test.
| private boolean allResolvedMailboxesExist(Mappings mappings) { | ||
| return mappings | ||
| .asStream() | ||
| .flatMap(mapping -> mapping.asMailAddress().stream()) | ||
| .allMatch(address -> { |
There was a problem hiding this comment.
Question: shouldn't this be any ?
Eg with the forward feature with a local copy one of the resolved address would not be local.
There was a problem hiding this comment.
As this is now behind a config switch, I thought I could as well make it strict.
In my opinion, it doesn't make sense to enable it when using aliases for forwarding because one would have to create an additional local alias for every forward then.
|
FYI: |
Currently, James only checks whether either the recipient exists or any alias for it exists. There is no guarantee that the alias actually points to an existing mailbox. The optional behavior checks whether all resolved mailboxes of the intended recipient actually exists locally. This allows to abort directly after the RCPT command instead of accepting the email and then error because the mailbox could not be found. However, with this additional check enabled, it is not possible anymore to forward emails to other email servers via the RRT.
felixauringer
force-pushed
the
fix-invalid-alias-target
branch
from
49e5f60 to
c8d080a
Compare
3 participants
Currently, it is only checked if the recipient is a user or if there is a mapping with the recipient as source. It is not checked that this mapping actually points to a local mailbox.
We do not want to accept emails that go directly to an error repository, so this should be catched after the SMTP RCPT command.
I opted to accept a recipient if all resolved mailboxes are present. This may break some poorly maintained groups, I guess. Because this check is optional, I think this is acceptable.
Note: This also prepares for linagora/tmail-backend#2446 by separating the check whether a mailbox exists from the step that applies the RRT. See also the MR in that repository: linagora/tmail-backend#2448.
Jira issue: https://issues.apache.org/jira/projects/JAMES/issues/JAMES-4213