Page MenuHomePhorge

Implement Kolab 4 rate limitations
ClosedPublic

Authored by vanmeeuwen on Dec 30 2021, 1:41 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 25, 1:19 AM
Unknown Object (File)
Mon, Mar 25, 1:19 AM
Unknown Object (File)
Mon, Mar 25, 1:18 AM
Unknown Object (File)
Mon, Mar 25, 1:18 AM
Unknown Object (File)
Mon, Mar 25, 1:18 AM
Unknown Object (File)
Mon, Mar 25, 1:18 AM
Unknown Object (File)
Mon, Mar 25, 1:18 AM
Unknown Object (File)
Mon, Mar 25, 1:18 AM
Subscribers
Restricted Project

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
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 37550
Build 15086: arc lint + arc unit

Event Timeline

vanmeeuwen created this revision.
  • Make sure we end with a nice line ending consistently
  • 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?

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 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 marked 5 inline comments as done.
  • Adjust code based on feedback
machniak subscribed.
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.Dec 31 2021, 10:21 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 marked 3 inline comments as done.
  • Another round of feedback
  • Use wallet() from EntitleableTrait
src/app/Http/Controllers/API/V4/PolicyController.php
219

use ($owner) is redundant.

237

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

vanmeeuwen marked an inline comment as done.
  • More feedback
vanmeeuwen added inline comments.
src/app/Http/Controllers/API/V4/PolicyController.php
237

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

machniak added inline comments.
src/app/Http/Controllers/API/V4/PolicyController.php
313

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.Dec 31 2021, 11:47 AM
vanmeeuwen marked 6 inline comments as done.
This revision is now accepted and ready to land.Dec 31 2021, 1:04 PM
This revision was automatically updated to reflect the committed changes.