Page MenuHomekolab.org

Preliminary implementation of a greylisting database and spf service
Needs ReviewPublic

Authored by vanmeeuwen on Oct 23 2020, 4:12 PM.

Details

Reviewers
machniak
Group Reviewers
Restricted Project
Test Plan

./phpunit

Diff Detail

Repository
rK kolab
Branch
arcpatch-D1810
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 31906
Build 12163: arc lint + arc unit

Event Timeline

vanmeeuwen requested review of this revision.Oct 23 2020, 4:12 PM
vanmeeuwen created this revision.
vanmeeuwen updated this revision to Diff 4948.Oct 26 2020, 9:32 AM
  • Chiptune some of the tests and logic
machniak requested changes to this revision.Oct 26 2020, 2:13 PM
machniak added a subscriber: machniak.
machniak added inline comments.
src/app/Console/Commands/User/GreylistCommand.php
63

In cases like this I find multiple where() calls looking better.

src/app/Greylist/Connect.php
32

This invokes a sql query. Checking recipient_type will be faster.

49

Same as above.

src/app/Greylist/Request.php
107

You don't need this query if $recipient is empty, right? Move it under the if below.

202

Looks like this query might be execute twice. Look above.

252

Cannot these be done in a single query?

297

I think these two can be replaced by ->where('updated_at', '>=', $this->timestamp->copy()->subDays(7)), with or without ->toDateTimeString(), I'm not sure.

304

sizeof -> count, for consistency.

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

else not needed after return.

73

Calling has() and then get() looks like a redundant roundtrip.

135

You don't need call_user_func_array().

171

Similar method exists in Greylist/Request. This could be de-duplicated somehow, maybe?

src/app/Http/Kernel.php
43

I guess we should specify different limits for the PolicyController routes.

src/app/User.php
577

I'm not sure I like this functionality in the User model class.

src/app/Utils.php
267

Utils is not that big yet, but maybe we should have NetUtils.

src/database/migrations/2020_10_18_091319_create_greylist_tables.php
45

The same columns like in the unique() below, so I think this is redundant.

50

This also will be redundant if you re-order columns in the unique index below.

84

There should be index here.

src/tests/Feature/Stories/GreylistTest.php
48

We have already getObjectProperty() helper.

src/tests/TestCaseTrait.php
92

this will be confused with existing assertEquals(), and I don't see why not just use assertSame().

159

I really don't like this extra newline. Our phpcs setup does not require this, maybe your editor?

325

This will be applied to every test. I don't like it. Maybe it should be in a separate trait, that you'd add to tests you really need to.

This revision now requires changes to proceed.Oct 26 2020, 2:13 PM
vanmeeuwen updated this revision to Diff 5077.Fri, Nov 13, 10:28 AM
vanmeeuwen marked 14 inline comments as done.
  • Update with feedback

Some of the feedback is fixed.

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

But... call_user_func()? Or what?

src/app/Http/Kernel.php
43

I had not found how to actually do that.

src/database/migrations/2020_10_18_091319_create_greylist_tables.php
45

It is not, same as below.

50

A unique constraint may cause an index to be created, but it is a different type of index -- because it can't help you speed up queries by hitting the right index in the right way.

src/tests/TestCaseTrait.php
159

No, my reading habits do -- I read vertical space used much, much faster than horizontal space.

325

We can change it further when we have switched over most if not all tests to using volatile fixtures and not seeded contents, and split up multiple test functions per test class such that there's only one series of tests per class (currently all test functions assert the same start state).

vanmeeuwen updated this revision to Diff 5083.Fri, Nov 13, 10:41 AM
  • Add an index for greylist_settings
vanmeeuwen marked an inline comment as done.Fri, Nov 13, 10:42 AM

Added the missing index.

machniak added inline comments.Fri, Nov 13, 10:46 AM
src/app/Http/Controllers/API/V4/PolicyController.php
135

$object->senderPolicyFrameworkWhitelist($data['client_name']) ? ;)

src/app/Http/Kernel.php
43

I've found it could be done per route like this Route::group(['prefix' => 'api', 'middleware' => 'throttle:3,10'], function () { or probably just create a seaparate middleware label for these heavy APIs. https://laravel.com/docs/6.x/middleware#assigning-middleware-to-routes

src/database/migrations/2020_10_18_091319_create_greylist_tables.php
45

They are the same.

$table->unique(
	        ['sender_local', 'sender_domain', 'recipient_hash', 'net_id', 'net_type'],
	         'ssrnn_unq'
 );
50

I'll trust you know better in this case, but I always thought that unique index is used as any other index. Why not?

src/tests/TestCaseTrait.php
325

In my eyes this creates a mess right now. Isn't that breaking any existing tests?

vanmeeuwen updated this revision to Diff 5089.Fri, Nov 13, 10:51 AM
  • Unify clientNetFromAddress definitions to a single \App\Utils version
vanmeeuwen updated this revision to Diff 5092.Fri, Nov 13, 10:54 AM
vanmeeuwen marked 2 inline comments as done.
  • Rename clientNetFromRequest to getNetFromAddress
vanmeeuwen marked 3 inline comments as done.Fri, Nov 13, 11:08 AM
vanmeeuwen added inline comments.
src/app/Http/Controllers/API/V4/PolicyController.php
135

Of course. I complete overlooked that.

src/app/Http/Kernel.php
43

I reckon too that our internal infrastructure is not supposed to be limited at this level, at all.

src/database/migrations/2020_10_18_091319_create_greylist_tables.php
50

A unique index is a constraint that incidentally uses an index in order to speed up the lookup to pass or fail on a unique constraint.

For example, a unique constraint could simply exist of a single column using a hash over cells appended to one another. It doesn't concern itself with comparison or range lookups, can completely ignore column types and is as such, when implemented correctly (so as to be most efficient as a unique constrained) in a database technology, it isn't an index you'd likely hit with a query so as to get to a resulting row.

src/tests/TestCaseTrait.php
325

Existing tests either use seeded content or set themselves up with a per-test domain/users/etc.

In dev/testing though, all tests are intended to use one single set of 'fixtures' -- however many of those there may be, and in however many "sets" we may break them down so as to make tests run more efficiently, and to whichever extent we may indeed refactor tests such that fixtures setup and teardown can happen once per class rather than once per test.

vanmeeuwen updated this revision to Diff 5164.Fri, Nov 20, 10:21 AM
vanmeeuwen marked 2 inline comments as done.
  • Just call the method