Page MenuHomekolab.org

Implement Kolab 4 rate limitations
ClosedPublic

Authored by vanmeeuwen on Thu, Dec 30, 1:41 PM.

Details

Reviewers
machniak
Group Reviewers
Restricted Project
Commits
rK8d3a5f5c0bf0: Implement Kolab 4 rate limitations
Summary

This change moves policy maintenance wrt. rate limitations to Kolab 4, with
a policy that with >= 10 messages per hour, or >= 100 recipients per hour, blocks further delivery of email.

Another magical threshold of 2.5 times those rates will automatically suspend the account.

Test Plan

./phpunit

Diff Detail

Repository
rK kolab
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vanmeeuwen requested review of this revision.Thu, Dec 30, 1:41 PM
vanmeeuwen created this revision.
vanmeeuwen updated this revision to Diff 9008.Thu, Dec 30, 1:46 PM
  • Make sure we end with a nice line ending consistently
vanmeeuwen updated this revision to Diff 9014.Thu, Dec 30, 2:49 PM
  • Fix tests
vanmeeuwen updated this revision to Diff 9020.Thu, Dec 30, 4:07 PM
  • Add two additional expunge commands for tables that otherwise just grow

Looks reasonable to me.

bin/quickstart.sh
99

Instead of horizon I'm using:

./artisan queue:restart
./artisan queue:work --timeout=60 --tries 3 &

as a lightweight local worker in bin/quickstart.sh. Haven't tried horizon like that though, so can't comment on what's better.

src/app/Http/Controllers/API/V4/PolicyController.php
60

Remove as this is never used.

104

->exists() would be good enough for the usecase I think (instead of retrieving the record.

134

Sounds like we should log an error at this point as this should never happen.

188

This can be moved outside of the each into the if on line 184

221

This can be moved outside of the each into the if on line 217

src/app/Policy/RateLimit.php
32

Can be removed?

40

Can be removed?

48

Can be removed?

Also, I take it that you have prior experience that maintaining 6 months worth of ratelimit entires is feasible performance wise?

mollekopf added inline comments.Fri, Dec 31, 8:55 AM
src/app/Http/Controllers/API/V4/PolicyController.php
158

Does this codepath not mean that you can basically send as many messages as you want to the same set of recipients? Perhaps we should keep track of that as well?

src/database/migrations/2021_12_28_103243_create_policy_ratelimit_tables.php
33

perhaps make this unique?

vanmeeuwen planned changes to this revision.Fri, Dec 31, 10:16 AM
vanmeeuwen marked 11 inline comments as done.
vanmeeuwen added inline comments.
bin/quickstart.sh
99

Horizon also gives you https://admin.<domain>/horizon for some UI, it's what we're using in production.

src/app/Http/Controllers/API/V4/PolicyController.php
60

Used on line 90.

134

The effect is already as desired -- we're not sending the email. We're basically just validating $wallet is something and not nothing, so as to avoid running in to error conditions later.

158

Yes, you could, but that's not how spammers behave.

At this stage though, we don't have any information that uniquely identifies one such message from another such message.

src/database/migrations/2021_12_28_103243_create_policy_ratelimit_tables.php
33

If a user_id-hash-updated is older than one hour, the record will not have been found by the time we need to insert the new one.

A unique constraint would prevent this from completing successfully.

vanmeeuwen updated this revision to Diff 9026.Fri, Dec 31, 10:17 AM
vanmeeuwen marked 5 inline comments as done.
  • Adjust code based on feedback
machniak requested changes to this revision.Fri, Dec 31, 10:21 AM
machniak added a subscriber: machniak.
machniak added inline comments.
src/app/Console/Commands/Policy/RateLimit/Whitelist/DeleteCommand.php
6

Must be App\Console\Command. Tests would catch it if they were created.

src/app/Console/Commands/Policy/RateLimit/Whitelist/ReadCommand.php
29

The constructor can be removed.

41

Maybe add orderBy().

49

What when $whitelisteable is null? E.g. deleted domain/user.

src/app/Http/Controllers/API/V4/PolicyController.php
130

You can do $user->wallet().

131

First check if $wallet is not null, then try to get the owner. I'd also check if the $owner is not empty.

163

What about "or are at 100% discount"? Also, you coould do the balance check earlier for better performance. If balance>0 there's no need to check for payments.

src/app/Observers/UserObserver.php
275

Not that big of a deal, but this is execute on deleting and on forceDeleting. It would make sense to do this once.

src/database/migrations/2021_12_28_103243_create_policy_ratelimit_tables.php
38

why wl not whitelist?

45

Isn't this redundant if you define a unique key below?

This revision now requires changes to proceed.Fri, Dec 31, 10:21 AM
vanmeeuwen planned changes to this revision.Fri, Dec 31, 10:44 AM
vanmeeuwen marked 7 inline comments as done.
vanmeeuwen added inline comments.
src/app/Console/Commands/Policy/RateLimit/Whitelist/ReadCommand.php
49

Then the observer should have deleted these, I don't know what the reason would be to display them here.

src/app/Http/Controllers/API/V4/PolicyController.php
130

I don't see a function wallet().

163

I was contemplating doing that but since there's public 100% discount codes it wouldn't suffice to combat spammers.

Updating commentary.

The need to get to count() is because I want >= 2 payments, so just checking ->balance > 0 is not enough.

src/database/migrations/2021_12_28_103243_create_policy_ratelimit_tables.php
38

table name length limitations.

45

It isn't.

unique constraints' primary purpose is an upon-write restriction, incidentally using some sort of index for duplicate detection purposes, but the type of index can be very bespoke to its use-case (i.e. a list of hash('sha256, '$a.$b') would do it).

indexes primary purpose is an upon-read helper with selecting particular records, including range queries and sub-string matches (i.e. a unique constraint duplicate detection table like the one does not help looking up any records). it uses far more complicated routines that are in turn sub-optimal for unique constraints.

vanmeeuwen updated this revision to Diff 9032.Fri, Dec 31, 10:46 AM
vanmeeuwen marked 3 inline comments as done.
  • Another round of feedback
vanmeeuwen updated this revision to Diff 9038.Fri, Dec 31, 11:03 AM
  • Use wallet() from EntitleableTrait
machniak added inline comments.Fri, Dec 31, 11:11 AM
src/app/Http/Controllers/API/V4/PolicyController.php
187

use ($owner) is redundant.

205

I guess we could skip this if it's a personal account (user == owner)?

vanmeeuwen marked 3 inline comments as done.Fri, Dec 31, 11:12 AM
vanmeeuwen updated this revision to Diff 9041.Fri, Dec 31, 11:14 AM
vanmeeuwen marked an inline comment as done.
  • More feedback
vanmeeuwen marked an inline comment as done.Fri, Dec 31, 11:15 AM
vanmeeuwen added inline comments.
src/app/Http/Controllers/API/V4/PolicyController.php
205

Yeah, not always a personal account of course, but it's redundant for $user->id == $owner->id indeed.

machniak requested changes to this revision.Fri, Dec 31, 11:47 AM
machniak added inline comments.
src/app/Http/Controllers/API/V4/PolicyController.php
315

I guess the line reference here could be also removed.

src/app/Policy/RateLimit.php
17

We should add protected $keyType = 'bigint'; here.

src/app/Policy/RateLimitWhitelist.php
15

We should add protected $keyType = 'bigint'; here.

src/database/migrations/2021_12_28_103243_create_policy_ratelimit_tables.php
25

It could be unsignedTinyInteger

src/tests/Feature/Stories/RateLimitTest.php
22

It would be useful to truncate ratelimit tables here.

This revision now requires changes to proceed.Fri, Dec 31, 11:47 AM
vanmeeuwen planned changes to this revision.Fri, Dec 31, 11:56 AM
vanmeeuwen marked 6 inline comments as done.
vanmeeuwen updated this revision to Diff 9047.Fri, Dec 31, 11:57 AM
  • More feedback
machniak accepted this revision.Fri, Dec 31, 1:04 PM
This revision is now accepted and ready to land.Fri, Dec 31, 1:04 PM
This revision was automatically updated to reflect the committed changes.