Skip to content
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

Fix #295 — Add domain blacklist feature #478

Merged
merged 3 commits into from
Jan 10, 2019

Conversation

ldidry
Copy link
Contributor

@ldidry ldidry commented Nov 16, 2018

No description provided.

@ldidry
Copy link
Contributor Author

ldidry commented Nov 16, 2018

I'm not 100% sure that there is no other files that can add subscribers. Need a good review.

@ldidry ldidry requested review from ikedas and dverdin November 16, 2018 10:53
@ikedas
Copy link
Member

ikedas commented Nov 16, 2018

This may fix #295.

@ikedas
Copy link
Member

ikedas commented Nov 16, 2018

You are adding a test against domain blacklist to add handler. Probably subscribe handler may also have such test.

@ldidry
Copy link
Contributor Author

ldidry commented Nov 16, 2018

Thanks, I added the test in subscribe. Since I use more or less the same test in 3 files, I created Sympa::Tools::Domains and put a function to do the test in it. I will extend it for #296

@ikedas ikedas merged commit 1abb733 into sympa-community:sympa-6.2 Jan 10, 2019
@dverdin
Copy link
Contributor

dverdin commented Jan 10, 2019

I arrive a bit late but did you know that Sympa can do this already?
for each list, you can set a blacklist. If your blacklist entry looks like "*@domain.tld", you blacklist the whole domain.
This blacklist is automatically used by any Sympa list.
It can be defined at the server or virtual host level, as it is a scenario.
You could also have used implicit scenarios inclusion.
In other words: you already had solutions in Sympa without the hasle of writing new code.

Remember: most of the time, any problem in Sympa can be solved by a scenario.

@ikedas
Copy link
Member

ikedas commented Jan 10, 2019

@dverdin, you are right. However I think

  1. current feature cannot have lists of blacklisted users by each action (scenario function), and
  2. use of wildcard character is not always intuitive for all users.

@dverdin
Copy link
Contributor

dverdin commented Jan 10, 2019

Hi @ikedas ! And a happy new year ! And I added the 6.2.38 to the sympa.org web site!

Now, back to the topic of this PR:

  1. Actually, the blacklist can also be applied to subscription.
  2. Maybe but it is explained just above the blacklist. Honestly, it does not look that hard.
    And Finally, that looks more like a listmaster problem. The issue quoted wants to prevent specific domains from being added when people import lists. The people importing such addresses will certainly not take the time to edit the blacklist. Hence my proposal to use an implicitely included scenario.

@ikedas
Copy link
Member

ikedas commented Jan 10, 2019

@dverdin, if you want to raise more discussion, could you please open a new issue? This PR has already been approved, merged and closed as the goal of an feature request submitted more than half a year ago.

@dverdin
Copy link
Contributor

dverdin commented Jan 10, 2019

@ikedas : You're right. I'll open a feature about it.
See you!

@dverdin
Copy link
Contributor

dverdin commented Jan 10, 2019

I completed the original feature request.

This was referenced Jan 10, 2019
@ldidry ldidry deleted the domains-blacklist branch June 25, 2019 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants