diff --git a/src/app/Listeners/SqlDebug.php b/src/app/Listeners/SqlDebug.php --- a/src/app/Listeners/SqlDebug.php +++ b/src/app/Listeners/SqlDebug.php @@ -68,6 +68,8 @@ $serialized = array_map(function ($entry) use ($ipv) { if ($entry instanceof \DateTime) { return $entry->format('Y-m-d h:i:s'); + } elseif (is_bool($entry)) { + return $entry ? 'true' : 'false'; } elseif ($ipv && is_string($entry) && strlen($entry) == ($ipv == 6 ? 16 : 4)) { // binary IP address? use HEX representation return '0x' . bin2hex($entry); diff --git a/src/app/Policy/Greylist/Request.php b/src/app/Policy/Greylist/Request.php --- a/src/app/Policy/Greylist/Request.php +++ b/src/app/Policy/Greylist/Request.php @@ -6,6 +6,7 @@ class Request { + protected $connect; protected $header; protected $netID; protected $netType; @@ -19,6 +20,11 @@ protected $whitelist; protected $request = []; + /** + * Class constructor + * + * @param array $request Request input + */ public function __construct($request) { $this->request = $request; @@ -30,14 +36,17 @@ } } - public function headerGreylist() + /** + * Get request state headers (Received-Greylist) - after self::shouldDefer() call + */ + public function headerGreylist(): string { if ($this->whitelist) { if ($this->whitelist->sender_local) { return sprintf( - "Received-Greylist: sender %s whitelisted since %s", + "Received-Greylist: sender %s whitelisted since %s (UTC)", $this->sender, - $this->whitelist->created_at->toDateString() + $this->whitelist->created_at->toDateTimeString() ); } @@ -49,20 +58,23 @@ ); } - $connect = $this->findConnectsCollection()->orderBy('created_at')->first(); - - if ($connect) { + if ($this->connect) { return sprintf( "Received-Greylist: greylisted from %s until %s.", - $connect->created_at, - $this->timestamp + $this->connect->created_at->toDateTimeString(), + $this->timestamp->toDateTimeString() ); } return "Received-Greylist: no opinion here"; } - public function shouldDefer() + /** + * Connection check regarding greylisting policy + * + * @return bool True if the message should be put off, False otherwise + */ + public function shouldDefer(): bool { list($this->netID, $this->netType) = \App\Utils::getNetFromAddress($this->request['client_address']); @@ -82,11 +94,13 @@ $this->senderLocal = substr($this->senderLocal, 0, 255); } - // Purge all old information if we have no recent entries - $noEntry = false; - if (!$this->findConnectsCollectionRecent()->exists()) { + // Get the most recent entry + $connect = $this->findConnectsCollection()->first(); + + // Purge all old information if we have no recent entry + if ($connect && $connect->updated_at < $this->timestamp->copy()->subDays(7)) { $this->findConnectsCollection()->delete(); - $noEntry = true; + $connect = null; } // See if the recipient opted-out of the feature @@ -100,80 +114,52 @@ // the following block is to maintain statistics and state ... // determine if the sender domain is a whitelist from this network - $this->whitelist = Whitelist::where( - [ - 'sender_domain' => $this->senderDomain, - 'net_id' => $this->netID, - 'net_type' => $this->netType - ] - )->first(); + $this->whitelist = Whitelist::where('sender_domain', $this->senderDomain) + ->where('net_id', $this->netID) + ->where('net_type', $this->netType) + ->first(); + + $cutoffDate = $this->timestamp->copy()->subDays(7)->startOfDay(); - $cutoffDate = $this->timestamp->copy()->subDays(7); + DB::beginTransaction(); + + // Whitelist older than a month, delete it + if ($this->whitelist && $this->whitelist->updated_at < $this->timestamp->copy()->subMonthsWithoutOverflow(1)) { + $this->whitelist->delete(); + $this->whitelist = null; + } + $all = Connect::where('sender_domain', $this->senderDomain) + ->where('net_id', $this->netID) + ->where('net_type', $this->netType) + ->where('updated_at', '>=', $cutoffDate->toDateTimeString()); + + // "Touch" the whitelist if exists if ($this->whitelist) { - if ($this->whitelist->updated_at < $this->timestamp->copy()->subMonthsWithoutOverflow(1)) { - $this->whitelist->delete(); - } else { + // For performance reasons do not update timestamp more than once per 1 minute + // FIXME: Such granularity should be good enough, right? It might be a problem + // if you wanted to compare this timestamp with mail log entries. + if ($this->whitelist->updated_at < $this->timestamp->copy()->subMinute()) { $this->whitelist->updated_at = $this->timestamp; $this->whitelist->save(['timestamps' => false]); - - Connect::where( - [ - 'sender_domain' => $this->senderDomain, - 'net_id' => $this->netID, - 'net_type' => $this->netType, - 'greylisting' => true - ] - ) - ->whereDate('updated_at', '>=', $cutoffDate) - ->update( - [ - 'greylisting' => false, - 'updated_at' => $this->timestamp - ] - ); - - return false; } - } else { - $count = Connect::where( - [ - 'sender_domain' => $this->senderDomain, - 'net_id' => $this->netID, - 'net_type' => $this->netType - ] - ) - ->whereDate('updated_at', '>=', $cutoffDate) - ->limit(4)->count(); + $all->where('greylisting', true) + ->update(['greylisting' => false, 'updated_at' => $this->timestamp]); + + $enabled = false; + } elseif ($all->count() >= 4) { // Automatically create a whitelist if we have at least 5 (4 existing plus this) messages from the sender - if ($count >= 4) { - $this->whitelist = Whitelist::create( - [ - 'sender_domain' => $this->senderDomain, - 'net_id' => $this->netID, - 'net_type' => $this->netType, - 'created_at' => $this->timestamp, - 'updated_at' => $this->timestamp - ] - ); + $this->whitelist = Whitelist::create([ + 'sender_domain' => $this->senderDomain, + 'net_id' => $this->netID, + 'net_type' => $this->netType, + 'created_at' => $this->timestamp, + 'updated_at' => $this->timestamp + ]); - Connect::where( - [ - 'sender_domain' => $this->senderDomain, - 'net_id' => $this->netID, - 'net_type' => $this->netType, - 'greylisting' => true - ] - ) - ->whereDate('updated_at', '>=', $cutoffDate) - ->update( - [ - 'greylisting' => false, - 'updated_at' => $this->timestamp - ] - ); - } + $all->where('greylisting', true) + ->update(['greylisting' => false, 'updated_at' => $this->timestamp]); } // TODO: determine if the sender (individual) is a whitelist @@ -181,13 +167,14 @@ // TODO: determine if the sender is a penpal of any of the recipients. First recipient wins. if (!$enabled) { + DB::commit(); return false; } $defer = true; - // Retrieve the entry for the sender/recipient/net combination - if (!$noEntry && ($connect = $this->findConnectsCollection()->first())) { + // Update/Create an entry for the sender/recipient/net combination + if ($connect) { $connect->connect_count += 1; // TODO: The period of time for which the greylisting persists is configurable. @@ -199,8 +186,7 @@ $connect->save(); } else { - Connect::create( - [ + $connect = Connect::create([ 'sender_local' => $this->senderLocal, 'sender_domain' => $this->senderDomain, 'net_id' => $this->netID, @@ -210,35 +196,28 @@ 'recipient_type' => $this->recipientType, 'created_at' => $this->timestamp, 'updated_at' => $this->timestamp - ] - ); + ]); } + $this->connect = $connect; + + DB::commit(); + return $defer; } - private function findConnectsCollection() + protected function findConnectsCollection() { - $collection = Connect::where( - [ + return Connect::where([ 'sender_local' => $this->senderLocal, 'sender_domain' => $this->senderDomain, 'recipient_hash' => $this->recipientHash, 'net_id' => $this->netID, 'net_type' => $this->netType, - ] - ); - - return $collection; - } - - private function findConnectsCollectionRecent() - { - return $this->findConnectsCollection() - ->where('updated_at', '>=', $this->timestamp->copy()->subDays(7)); + ]); } - private function recipientFromRequest() + protected function recipientFromRequest() { $recipients = \App\Utils::findObjectsByRecipientAddress($this->request['recipient']); @@ -265,7 +244,7 @@ return $recipient; } - public function senderFromRequest() + protected function senderFromRequest() { return \App\Utils::normalizeAddress($this->request['sender']); } diff --git a/src/tests/Feature/Controller/PolicyTest.php b/src/tests/Feature/Controller/PolicyTest.php new file mode 100644 --- /dev/null +++ b/src/tests/Feature/Controller/PolicyTest.php @@ -0,0 +1,87 @@ +clientAddress = '212.103.80.148'; + $this->net = \App\IP4Net::getNet($this->clientAddress); + $this->testDomain = $this->getTestDomain('test.domain', [ + 'type' => Domain::TYPE_EXTERNAL, + 'status' => Domain::STATUS_ACTIVE | Domain::STATUS_CONFIRMED | Domain::STATUS_VERIFIED + ]); + $this->testUser = $this->getTestUser('john@test.domain'); + + Greylist\Connect::where('sender_domain', 'sender.domain')->delete(); + Greylist\Whitelist::where('sender_domain', 'sender.domain')->delete(); + + $this->useServicesUrl(); + } + + public function tearDown(): void + { + $this->deleteTestUser($this->testUser->email); + $this->deleteTestDomain($this->testDomain->namespace); + + Greylist\Connect::where('sender_domain', 'sender.domain')->delete(); + Greylist\Whitelist::where('sender_domain', 'sender.domain')->delete(); + + parent::tearDown(); + } + + /** + * Test greylist policy webhook + * + * @group data + * @group greylist + */ + public function testGreylist() + { + // Note: Only basic tests here. More detailed policy handler tests are in another place + + // Test 403 response + $post = [ + 'sender' => 'someone@sender.domain', + 'recipient' => $this->testUser->email, + 'client_address' => $this->clientAddress, + 'client_name' => 'some.mx' + ]; + + $response = $this->post('/api/webhooks/policy/greylist', $post); + + $response->assertStatus(403); + + $json = $response->json(); + + $this->assertEquals('DEFER_IF_PERMIT', $json['response']); + $this->assertEquals("Greylisted for 5 minutes. Try again later.", $json['reason']); + + // Test 200 response + $connect = Greylist\Connect::where('sender_domain', 'sender.domain')->first(); + $connect->created_at = \Carbon\Carbon::now()->subMinutes(6); + $connect->save(); + + $response = $this->post('/api/webhooks/policy/greylist', $post); + + $response->assertStatus(200); + + $json = $response->json(); + + $this->assertEquals('DUNNO', $json['response']); + $this->assertMatchesRegularExpression('/^Received-Greylist: greylisted from/', $json['prepend'][0]); + } +} diff --git a/src/tests/Feature/Stories/GreylistTest.php b/src/tests/Feature/Stories/GreylistTest.php --- a/src/tests/Feature/Stories/GreylistTest.php +++ b/src/tests/Feature/Stories/GreylistTest.php @@ -2,12 +2,12 @@ namespace Tests\Feature\Stories; +use App\Domain; use App\Policy\Greylist; use Illuminate\Support\Facades\DB; use Tests\TestCase; /** - * @group slow * @group data * @group greylist */ @@ -20,20 +20,24 @@ { parent::setUp(); - $this->setUpTest(); - $this->useServicesUrl(); $this->clientAddress = '212.103.80.148'; - $this->net = \App\IP4Net::getNet($this->clientAddress); - DB::delete("DELETE FROM greylist_connect WHERE sender_domain = 'sender.domain';"); - DB::delete("DELETE FROM greylist_whitelist WHERE sender_domain = 'sender.domain';"); + $this->domainHosted = $this->getTestDomain('test.domain', [ + 'type' => Domain::TYPE_EXTERNAL, + 'status' => Domain::STATUS_ACTIVE | Domain::STATUS_CONFIRMED | Domain::STATUS_VERIFIED + ]); + + $this->domainOwner = $this->getTestUser('john@test.domain'); + + Greylist\Connect::where('sender_domain', 'sender.domain')->delete(); + Greylist\Whitelist::where('sender_domain', 'sender.domain')->delete(); } public function tearDown(): void { - DB::delete("DELETE FROM greylist_connect WHERE sender_domain = 'sender.domain';"); - DB::delete("DELETE FROM greylist_whitelist WHERE sender_domain = 'sender.domain';"); + Greylist\Connect::where('sender_domain', 'sender.domain')->delete(); + Greylist\Whitelist::where('sender_domain', 'sender.domain')->delete(); parent::tearDown(); } @@ -253,11 +257,12 @@ $this->assertFalse($request->shouldDefer()); // Ensure we also find the setting by alias - $aliases = $this->domainOwner->aliases()->orderBy('alias')->pluck('alias')->all(); + $this->domainOwner->setAliases(['alias1@test2.domain2']); + $request = new Greylist\Request( [ 'sender' => 'someone@sender.domain', - 'recipient' => $aliases[0], + 'recipient' => 'alias1@test2.domain2', 'client_address' => $this->clientAddress ] ); @@ -298,8 +303,13 @@ $this->assertFalse($request->shouldDefer()); } + /** + * @group slow + */ public function testMultipleUsersAllDisabled() { + $this->setUpTest(); + $request = new Greylist\Request( [ 'sender' => 'someone@sender.domain', @@ -340,8 +350,13 @@ } } + /** + * @group slow + */ public function testMultipleUsersAnyEnabled() { + $this->setUpTest(); + $request = new Greylist\Request( [ 'sender' => 'someone@sender.domain', @@ -388,16 +403,14 @@ public function testControllerNew() { - $data = [ + $request = new Greylist\Request([ 'sender' => 'someone@sender.domain', 'recipient' => $this->domainOwner->email, 'client_address' => $this->clientAddress, 'client_name' => 'some.mx' - ]; - - $response = $this->post('/api/webhooks/policy/greylist', $data); + ]); - $response->assertStatus(403); + $this->assertTrue($request->shouldDefer()); } public function testControllerNotNew() @@ -418,15 +431,13 @@ $connect->created_at = \Carbon\Carbon::now()->subMinutes(6); $connect->save(); - $data = [ + $request = new Greylist\Request([ 'sender' => 'someone@sender.domain', 'recipient' => $this->domainOwner->email, 'client_address' => $this->clientAddress, 'client_name' => 'some.mx' - ]; - - $response = $this->post('/api/webhooks/policy/greylist', $data); + ]); - $response->assertStatus(200); + $this->assertFalse($request->shouldDefer()); } }