1062739 - add the ability for administrators to limit the number of emails sent to a user per minute and hour

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, enhancement)

Description

[:glob ✱]

some email providers impose limits on the number of messages a recipient can receive over a period of time.

eg. gmail's limits are:
    180 messages per minute
   3600 messages per hour
  86400 messages per day
(from https://support.google.com/a/answer/1366776)


to avoid emails being lost, we should implement rate limiting within bugzilla:
- applicable only when using queued sending
- add two admin parameters:
  - per-minute limit, default 100
  - per-hour limit, default 2500
  - setting either to 0 disables rate limiting
- add a new table which logs the timestamps of all email sent and the recipient
- update email sending to:
  - delete entries from the new table which are older than 24 hours
  - check the number of messages sent to the specified recipient within the last minute and 24 hours
  - if either exceeds the configure rate, call $job->declined()
- update the minimum TheSchwartz version to 1.10

Comment 1

[Simon Green]

After discussion with glob, there are two changes

(In reply to Byron Jones ‹:glob› from comment #0)
>   - delete entries from the new table which are older than 24 hours

This will be older than one hour

>   - check the number of messages sent to the specified recipient within the
> last minute and 24 hours

This also should be last minute and last hour.

Comment 2

[Simon Green]

It was also decided to set the limits to 100/minute and 2500/hour for two reasons:

1) To allow other non-Bugzilla e-mails to go though to the user.
2) For those installations that have multiple queues processes running, it is possible to send limit+processes e-mails per minute or hour.

Comment 3

[:glob ✱]

to assist administrators, i'll also upstream bmo's jobqueue viewer as part of this work:
https://github.com/mozilla/webtools-bmo-bugzilla/blob/master/extensions/BMO/lib/Reports/EmailQueue.pm
https://github.com/mozilla/webtools-bmo-bugzilla/blob/master/extensions/BMO/template/en/default/pages/email_queue.html.tmpl

Comment 4

[Frédéric Buclin]

Which kind of installations is affected by these limitations, maybe besides *some* bmo or RH users?? Shouldn't this be implemented as an extension instead? This is again another set of new parameters which are added which will be of very limited use (+ the code behind to make this work).

Comment 5

[Simon Green]

(In reply to Frédéric Buclin from comment #4)
> Which kind of installations is affected by these limitations, maybe besides
> *some* bmo or RH users?? Shouldn't this be implemented as an extension

Gnome comes to mind, and any other Bugzilla system that mass closes bugs as a version or product becomes EOL and a Bugzilla administrator mass closes thousands of bugs a once.

Comment 6

[:glob ✱]

(In reply to Frédéric Buclin from comment #4)
> Which kind of installations is affected by these limitations, maybe besides
> *some* bmo or RH users??

in bmo's case, "some" is approx 120,000 gmail users.

i any event, we have quite a few features which are only applicable to larger installations (eg. jobqueue, memcahed, ldap integration), and this is one of them.  while it's true that it adds another set of parameters, the defaults should be suitable for practically all installations.

Comment 7

[Andre Klapper]

(In reply to sgreen from comment #5)
> Gnome comes to mind, and any other Bugzilla system that mass closes bugs as
> a version or product becomes EOL and a Bugzilla administrator mass closes
> thousands of bugs a once.

Confirming for GNOME; another usecase beside mass-closing EOL tickets would be e.g. merging old "Version" field entries.

Comment 8

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #6)
> in bmo's case, "some" is approx 120,000 gmail users.

No, my question is: how many users are really affected by these rate limits? I have a gmail.com account, and I certainly don't get that many bugmails per hour or day. So "some" is definitely much less than 120,000.

I think it would make much more sense to put these rate limits into constants in Bugzilla::Constants as it's very unlikely that these values will ever be edited.

Comment 9

[:glob ✱]

(In reply to Frédéric Buclin from comment #8)
> how many users are really affected by these rate limits?

it's rare that we hit those limits currently.  the usage of bmo continues to grow and i think it's important to put preventative measures in place to avoid email being unexpectedly dropped.

> I think it would make much more sense to put these rate limits into
> constants in Bugzilla::Constants as it's very unlikely that these values
> will ever be edited.

that's a reasonable suggestion; will do.

Comment 10

[:glob ✱]

rough plan of attack:
- table email_rates(id, recipient VARCHAR, message_ts)
- in MessageToMTA, use $email's recipient against the email_rates
  - if limits hit, die "some_constant_error_message"
- in Bugzilla::Job::Mailer check for that error and $job->declined()
  - refactor Bugzilla::Job::Mailer and ::BugMail to avoid duplicating that logic
- insert into email_rates at the end of MessageToMTA
- delete old rows from email_rates near the start of MessageToMTA
- upstream bmo's queue reports, as per comment 3

Comment 11

[:glob ✱]

Attachment: 1062739_1.patch

Comment 12

[Frédéric Buclin]

Comment on attachment 8512705 [details] [diff] [review]
1062739_1.patch

>+++ b/template/en/default/admin/admin.html.tmpl

>+[% IF user.in_group('admin') %]
>+  <dl>
>+  [% IF Param('use_mailer_queue') %]
>+    <dt><a href="view_job_queue.cgi">View Job Queue</a></dt>
>+    <dd>View the queue of undelivered/deferred jobs/emails.</dd>
>+  [% END %]
>+  </dl>
>+[% END %]

There is no reason to have this item out of the table used for all other items. It should be below the Whining link.



>+++ b/view_job_queue.cgi

>+use Storable ();

You should import the methods you need explicitly instead of using the Storable::foo() notation later.


>+use constant MAX_JOBS => 500;

This constant should be in Bugzilla/Constants.pm. And this way, you can easily use it from templates using [% constants.MAX_JOBS %] without having to store it in $vars.


>+my $user = Bugzilla->login(LOGIN_REQUIRED);
>+Bugzilla->login(LOGIN_REQUIRED)->in_group("admin")

The 2nd line should be $user->in_group("admin").

Comment 13

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8512705 [details] [diff] [review]
1062739_1.patch

Review of attachment 8512705 [details] [diff] [review]:
-----------------------------------------------------------------

Overall it looks good. I set LIMIT_PER_MINUTE to 1 and EMAIL_LIMIT_PER_HOUR to 5 to test.
r- because the "view  the queue of deferred jobs/emails" link looks really pretty bad on bugzilla's current default theme (and probably others.)

::: Bugzilla/Constants.pm
@@ +650,5 @@
> +# Setting a limit to 0 will disable this feature.
> +use constant EMAIL_LIMIT_PER_MINUTE => 1000;
> +use constant EMAIL_LIMIT_PER_HOUR   => 2500;
> +# Don't change this exception message.
> +use constant EMAIL_LIMIT_EXCEPTION  => "email_limit_exceeded\n";

I like this, seems like it could be made into an exception object if we ever need more fine-grained handling.

::: Bugzilla/DB/Schema.pm
@@ +1650,5 @@
> +        INDEXES => [
> +            email_rates_idx => [qw(recipient message_ts)],
> +        ],
> +    },
> +

nit: delete extra line here.

::: template/en/default/admin/admin.html.tmpl
@@ +131,5 @@
> +    <dd>View the queue of undelivered/deferred jobs/emails.</dd>
> +  [% END %]
> +  </dl>
> +[% END %]
> +

Why is this outside of the table? it looks very odd.

Comment 14

[Dylan Hardison [:dylan] (he/him)]

Attachment: weird-formatting.png

screenshot of the view queue link, showing alignment problem.

Comment 15

[:glob ✱]

Attachment: 1062739_2.patch

thanks lpsolit and dylan for the quick turn around.

no idea what i was thinking with regards to the positioning on admin.cgi, this revision should look better.

Comment 16

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8514031 [details] [diff] [review]
1062739_2.patch

Review of attachment 8514031 [details] [diff] [review]:
-----------------------------------------------------------------

r=dylan

Comment 17

[Frédéric Buclin]

Comment on attachment 8514031 [details] [diff] [review]
1062739_2.patch

>+++ b/view_job_queue.cgi

>+Bugzilla->login(LOGIN_REQUIRED)->in_group("admin")

I'm really not a fan of this notation. For new contributors, it won't be clear at all that Bugzilla->login() is a user object. Doing it as we do everywhere else, i.e.

my $user = Bugzilla->login(...);
$user->in_group(...)

is much clearer.

Comment 18

[:glob ✱]

To ssh://[e-mail]/bugzilla/bugzilla.git
   f1fda7c..908480c  master -> master

(includes lpsolit's change from comment 17)