Change e-mail domain blocks to block IPs dynamically #17635
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Gargron
force-pushed
the
feature-email-domain-blocks-redesign
branch
4 times, most recently
from
9c90c2e
to
febace7
Compare
Gargron
force-pushed
the
feature-email-domain-blocks-redesign
branch
from
febace7
to
06b30a9
Compare
ykzts
requested changes
Feb 24, 2022
Co-authored-by: Yamagishi Kazutoshi <ykzts@desire.sh>
Co-authored-by: Yamagishi Kazutoshi <ykzts@desire.sh>
ykzts
approved these changes
Feb 24, 2022
| def with_dns_records? | ||
| @with_dns_records | ||
| def history | ||
| @history ||= Trends::History.new('email_domain_blocks', id) |
There was a problem hiding this comment.
Hm maybe that needs some refactoring, because email domain blocks depending on trends code feels very wrong.
Comment on lines
+23
to
+24
| # Used for adding multiple blocks at once | ||
| attr_accessor :other_domains |
There was a problem hiding this comment.
Feels weird to have that in the model since it's purely a form/controller thing.
Comment on lines
+41
to
+42
| other_email_domain_block = EmailDomainBlock.create!(domain: domain, parent: @email_domain_block) | ||
| log_action :create, other_email_domain_block |
There was a problem hiding this comment.
This is going to cause issues as soon as two blocked email domains share MX records, right?
| end | ||
| end | ||
|
|
||
| email_domain_block.update(ips: ips, last_refresh_at: Time.now.utc) |
There was a problem hiding this comment.
Just got TypeError: can't cast Resolv::DNS::Resource::IN::A
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When adding a new e-mail domain block, do not block any resolved DNS records silently; instead, display them as options in the form, turning the form into a two-step wizard. Add ability to delete e-mail domain blocks in batches. Instead of recording IPs of domains and MX records as separate blocks, add periodic refreshing for IPs for each entry, since IPs are not permanent. When an e-mail domain block prevents a registration attempt, track it in history and display number of attempts over the last 7 days in the admin UI.
Having explicit IP blocks in the e-mail domain blocks should be considered dangerous and deprecated. We may need to consider a migration that removes them. The IP addresses that the blocked domains resolve to will still be checked for matches, which should help for cases where one mail server is masked using different MX records or domains.