Ability to block domains when receiving emails #2#2384
Ability to block domains when receiving emails #2#2384tozo wants to merge 5 commits intosimple-login:masterfrom
Conversation
7f01f4f to
8aa10b5
Compare
|
I updated the code, except the one where I asked the question |
|
@tozo can you please add also sender block? Now SL doesn't have option to block sender and/or domain account wide. What stops spammer from if I block him in example@my-sub.aleeas.com just send emails to another-example@my-sub.aleeas.com? As I see, such adresses created automatically by default on subdomains. So this feature will help against this. |
|
@aksolotl222 If I understand you correctly this PR does what you describe. You can block complete domains so no matter what email addresses the sender use on that domain they all will be blocked. |
|
Hi @acasajus Could you let me know if there is any other changes that need to be done? |
| ) | ||
|
|
||
| new_domain = sanitize_domain(domain) | ||
| domain_forbidden_cause = can_domain_be_used(user, new_domain) |
There was a problem hiding this comment.
Why is this changed. This would allow users to create a custom domain with an invalid domain or an SL domain.
There was a problem hiding this comment.
Based on the original code I modified the existing and created 2 separate functions (can_domain_be_used, can_custom_domain_be_used and can_blocked_domain_be_used respectively).
Because of that I also had to change this to call the function can_custom_domain_be_used to still include all the previously executed checks.
But inside of that function I call the can_domain_be_used which should check for the domain validity and SL domain as well and if that reason is not None, then return with that error.
Currently in this branch that call is in line 119:
reason = can_domain_be_used(user, domain)
if reason is not None:
return reason
|
Guys, can you also add ability to
What do you think? |
|
For your first point, this PR somewhat covers that, since this blocks the whole sender domain (not just a specific email). But I can see that in some situations it might be better to block only some sender emails. Either way I would implement those features in a separate PR, so that we keep this as simple as possible |
|
@acasajus @cquintana92 @nguyenkims Any progress on this? It is really neat feature. I am waiting for it. |
| flash("Your preference has been updated", "success") | ||
| elif request.form.get("form-name") == "blocked-domains-add": | ||
| domain = request.form.get("domain-name") | ||
| new_domain = sanitize_domain(domain) |
There was a problem hiding this comment.
If domain is None this will fail
app/models.py
Outdated
|
|
||
| default_mailbox = orm.relationship("Mailbox", foreign_keys=[default_mailbox_id]) | ||
|
|
||
| _blocked_domains = orm.relationship("BlockedDomain", lazy="joined") |
There was a problem hiding this comment.
This will add a join to every user query. Please remove the lazy="joined"
|
|
||
| mail_from_domain = get_email_domain_part(mail_from) | ||
| if is_domain_blocked(user.id, mail_from_domain): | ||
| # by default return 2** instead of 5** to allow user to receive emails again when domain is unblocked |
There was a problem hiding this comment.
Add a log here so that we can track when an email is dropped in the logs.
| flash(domain_forbidden_cause.message(new_domain), "error") | ||
| return redirect(url_for("dashboard.setting")) | ||
|
|
||
| BlockedDomain.create(user_id=current_user.id, domain=new_domain) |
There was a problem hiding this comment.
Add an user audit log here to keep track on what happened.
|
|
||
| BlockedDomain.delete(domain.id) | ||
|
|
||
| Session.commit() |
There was a problem hiding this comment.
Same here. Missing an user audit log.
|
|
||
| domain = BlockedDomain.get(domain_id) | ||
|
|
||
| BlockedDomain.delete(domain.id) |
There was a problem hiding this comment.
Any user seems to be able to delete any domain.
There was a problem hiding this comment.
I now validate if the current user is the owner of the domain before deleting it, let me know if that looks ok
Other option is to have a call like this:
deleted = (
BlockedDomain.filter_by(
id=domain_id_int,
user_id=current_user.id,
).delete()
)| domain_id = request.form.get("domain_id") | ||
| domain_name = request.form.get("domain_name") | ||
|
|
||
| domain = BlockedDomain.get(domain_id) |
There was a problem hiding this comment.
If domain_id is none this will crash
app/custom_domain_utils.py
Outdated
| if reason is not None: | ||
| return reason | ||
|
|
||
| if BlockedDomain.get_by(domain=domain): |
There was a problem hiding this comment.
Missing check that the domain belongs to the user
| flash("Your preference has been updated", "success") | ||
| elif request.form.get("form-name") == "blocked-domains-add": | ||
| domain = request.form.get("domain-name") | ||
| new_domain = sanitize_domain(domain) |
There was a problem hiding this comment.
If domain is None it will fail with an exception
|
|
||
| BlockedDomain.create(user_id=current_user.id, domain=new_domain) | ||
|
|
||
| Session.commit() |
There was a problem hiding this comment.
Missign user audit log on the action done
| domain_id = request.form.get("domain_id") | ||
| domain_name = request.form.get("domain_name") | ||
|
|
||
| domain = BlockedDomain.get(domain_id) |
There was a problem hiding this comment.
If domain_id is None it will fail with an exception
|
|
||
| domain = BlockedDomain.get(domain_id) | ||
|
|
||
| BlockedDomain.delete(domain.id) |
There was a problem hiding this comment.
Any user can delete any domain
app/custom_domain_utils.py
Outdated
| if reason is not None: | ||
| return reason | ||
|
|
||
| if BlockedDomain.get_by(domain=domain): |
There was a problem hiding this comment.
Any other user adding a domain invalidates this user from using that domain.
| mail_from_domain = get_email_domain_part(mail_from) | ||
| if is_domain_blocked(user.id, mail_from_domain): | ||
| # by default return 2** instead of 5** to allow user to receive emails again when domain is unblocked | ||
| res_status = status.E200 |
There was a problem hiding this comment.
Add a log entry here to mark that the email has been dropped.
|
all the notes should be fixed now, let me know if anything else needs to be fixed/changed |
updating ui adding tests, fixing bugs
moved the function is_domain_blocked into its own utils refactoring the function can_domain_be_used
making sure the query and delete functionality is specific to a given user additional checks to data that's coming from the front-end
Sorry, I had to recreate this PR.
This PR adds the functionality to block domains so that the user won't receive emails no matter which email was used from that domain.
It's mainly to prevent spam emails coming from the same domain but from different email addresses.
Since this feature is not tied to a specific alias/custom domain I added it to the setting page.
Let me know if there is a better place for this feature or if anything else needs to be changed.
Note: This feature was discussed here: #1344
Couple of screenshots:
Default view:

Page once domains are blocked:
