diff --git a/src/app/Http/Controllers/API/V4/PolicyController.php b/src/app/Http/Controllers/API/V4/PolicyController.php --- a/src/app/Http/Controllers/API/V4/PolicyController.php +++ b/src/app/Http/Controllers/API/V4/PolicyController.php @@ -19,8 +19,6 @@ { $data = \request()->input(); - list($local, $domainName) = explode('@', $data['recipient']); - $request = new \App\Policy\Greylist\Request($data); $shouldDefer = $request->shouldDefer(); diff --git a/src/app/IP4Net.php b/src/app/IP4Net.php --- a/src/app/IP4Net.php +++ b/src/app/IP4Net.php @@ -20,21 +20,11 @@ 'updated_at' ]; - public static function getNet($ip, $mask = 32) + public static function getNet($ip) { - $query = " - SELECT id FROM ip4nets - WHERE INET_ATON(net_number) <= INET_ATON(?) - AND INET_ATON(net_broadcast) >= INET_ATON(?) - ORDER BY INET_ATON(net_number), net_mask DESC LIMIT 1 - "; - - $results = DB::select($query, [$ip, $ip]); - - if (sizeof($results) == 0) { - return null; - } - - return \App\IP4Net::find($results[0]->id); + $where = 'INET_ATON(net_number) <= INET_ATON(?) and INET_ATON(net_broadcast) >= INET_ATON(?)'; + return IP4Net::whereRaw($where, [$ip, $ip]) + ->orderByRaw('INET_ATON(net_number), net_mask DESC') + ->first(); } } diff --git a/src/app/IP6Net.php b/src/app/IP6Net.php --- a/src/app/IP6Net.php +++ b/src/app/IP6Net.php @@ -20,21 +20,11 @@ 'updated_at' ]; - public static function getNet($ip, $mask = 128) + public static function getNet($ip) { - $query = " - SELECT id FROM ip6nets - WHERE INET6_ATON(net_number) <= INET6_ATON(?) - AND INET6_ATON(net_broadcast) >= INET6_ATON(?) - ORDER BY INET6_ATON(net_number), net_mask DESC LIMIT 1 - "; - - $results = DB::select($query, [$ip, $ip]); - - if (sizeof($results) == 0) { - return null; - } - - return \App\IP6Net::find($results[0]->id); + $where = 'INET6_ATON(net_number) <= INET6_ATON(?) and INET6_ATON(net_broadcast) >= INET6_ATON(?)'; + return IP6Net::whereRaw($where, [$ip, $ip]) + ->orderByRaw('INET6_ATON(net_number), net_mask DESC') + ->first(); } } 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 @@ -64,8 +64,6 @@ public function shouldDefer() { - $deferIfPermit = true; - list($this->netID, $this->netType) = \App\Utils::getNetFromAddress($this->request['client_address']); if (!$this->netID) { @@ -106,49 +104,14 @@ ); } - // see if all recipients and their domains are opt-outs - $enabled = false; - + // See if the recipient opted-out of the feature + $enabled = true; if ($recipient) { - $setting = Setting::where( - [ - 'object_id' => $this->recipientID, - 'object_type' => $this->recipientType, - 'key' => 'greylist_enabled' - ] - )->first(); - - if (!$setting) { - $domain = $recipient->domain(); - - if ($domain) { - $setting = Setting::where( - [ - 'object_id' => $recipient->domain()->id, - 'object_type' => \App\Domain::class, - 'key' => 'greylist_enabled' - ] - )->first(); - - if (!$setting) { - $enabled = true; - } else { - if ($setting->{'value'} !== 'false') { - $enabled = true; - } - } - } else { - $enabled = true; - } - } else { - if ($setting->{'value'} !== 'false') { - $enabled = true; - } - } - } else { - $enabled = true; + $enabled = $recipient->getSetting('greylist_enabled') !== 'false'; } + // FIXME: Shouldn't we bail-out (return early) if there's no $recipient? + // the following block is to maintain statistics and state ... $entries = Connect::where( [ @@ -228,8 +191,21 @@ ->orderBy('updated_at') ->first(); - if (!$connect) { - $connect = Connect::create( + $deferIfPermit = true; + + if ($connect) { + $connect->connect_count += 1; + + // TODO: The period of time for which the greylisting persists is configurable. + if ($connect->created_at < $this->timestamp->copy()->subMinutes(5)) { + $deferIfPermit = false; + + $connect->greylisting = false; + } + + $connect->save(); + } else { + Connect::create( [ 'sender_local' => $this->senderLocal, 'sender_domain' => $this->senderDomain, @@ -238,24 +214,12 @@ 'recipient_hash' => $this->recipientHash, 'recipient_id' => $this->recipientID, 'recipient_type' => $this->recipientType, - 'connect_count' => 0, 'created_at' => $this->timestamp, 'updated_at' => $this->timestamp ] ); } - $connect->connect_count += 1; - - // TODO: The period of time for which the greylisting persists is configurable. - if ($connect->created_at < $this->timestamp->copy()->subMinutes(5)) { - $deferIfPermit = false; - - $connect->greylisting = false; - } - - $connect->save(); - return $deferIfPermit; } @@ -282,8 +246,6 @@ private function recipientFromRequest() { - $recipient = null; - $recipients = \App\Utils::findObjectsByRecipientAddress($this->request['recipient']); if (sizeof($recipients) > 1) { diff --git a/src/app/Policy/Greylist/Setting.php b/src/app/Policy/Greylist/Setting.php deleted file mode 100644 --- a/src/app/Policy/Greylist/Setting.php +++ /dev/null @@ -1,17 +0,0 @@ -getSetting('greylist_enabled') !== 'false'; + $config['greylist_enabled'] = $this->getSetting('greylist_enabled') !== 'false'; return $config; } @@ -30,7 +32,7 @@ $errors = []; foreach ($config as $key => $value) { - if ($key == 'greylisting') { + if ($key == 'greylist_enabled') { $this->setSetting('greylist_enabled', $value ? 'true' : 'false'); } else { $errors[$key] = \trans('validation.invalid-config-parameter'); diff --git a/src/app/Utils.php b/src/app/Utils.php --- a/src/app/Utils.php +++ b/src/app/Utils.php @@ -46,28 +46,12 @@ public static function countryForIP($ip) { if (strpos($ip, ':') === false) { - $query = " - SELECT country FROM ip4nets - WHERE INET_ATON(net_number) <= INET_ATON(?) - AND INET_ATON(net_broadcast) >= INET_ATON(?) - ORDER BY INET_ATON(net_number), net_mask DESC LIMIT 1 - "; + $net = \App\IP4Net::getNet($ip); } else { - $query = " - SELECT id FROM ip6nets - WHERE INET6_ATON(net_number) <= INET6_ATON(?) - AND INET6_ATON(net_broadcast) >= INET6_ATON(?) - ORDER BY INET6_ATON(net_number), net_mask DESC LIMIT 1 - "; + $net = \App\IP6Net::getNet($ip); } - $nets = \Illuminate\Support\Facades\DB::select($query, [$ip, $ip]); - - if (sizeof($nets) > 0) { - return $nets[0]->country; - } - - return 'CH'; + return $net && $net->country ? $net->country : 'CH'; } /** diff --git a/src/database/migrations/2021_08_25_120000_drop_greylist_settings_table.php b/src/database/migrations/2021_08_25_120000_drop_greylist_settings_table.php new file mode 100644 --- /dev/null +++ b/src/database/migrations/2021_08_25_120000_drop_greylist_settings_table.php @@ -0,0 +1,43 @@ +bigIncrements('id'); + $table->bigInteger('object_id'); + $table->string('object_type', 16); + $table->string('key', 64); + $table->text('value'); + $table->timestamps(); + + $table->index(['object_id', 'object_type', 'key'], 'ook_idx'); + } + ); + } +} diff --git a/src/resources/vue/Admin/User.vue b/src/resources/vue/Admin/User.vue --- a/src/resources/vue/Admin/User.vue +++ b/src/resources/vue/Admin/User.vue @@ -312,10 +312,10 @@
- +
- - {{ $t('form.enabled') }} + + {{ $t('form.enabled') }} {{ $t('form.disabled') }}
diff --git a/src/resources/vue/User/Info.vue b/src/resources/vue/User/Info.vue --- a/src/resources/vue/User/Info.vue +++ b/src/resources/vue/User/Info.vue @@ -177,9 +177,9 @@
- +
- + {{ $t('user.greylisting-text') }} @@ -350,7 +350,7 @@ }, submitSettings() { this.$root.clearFormValidation($('#settings form')) - let post = { greylisting: $('#greylisting').prop('checked') ? 1 : 0 } + let post = { greylist_enabled: $('#greylist_enabled').prop('checked') ? 1 : 0 } axios.post('/api/v4/users/' + this.user_id + '/config', post) .then(response => { diff --git a/src/tests/Browser/Reseller/UserTest.php b/src/tests/Browser/Reseller/UserTest.php --- a/src/tests/Browser/Reseller/UserTest.php +++ b/src/tests/Browser/Reseller/UserTest.php @@ -288,6 +288,7 @@ // Now we go to Ned's info page, he's a controller on John's wallet $this->browse(function (Browser $browser) { $ned = $this->getTestUser('ned@kolab.org'); + $ned->setSetting('greylist_enabled', 'false'); $page = new UserPage($ned->id); $browser->click('@user-users tbody tr:nth-child(4) td:first-child a') @@ -358,6 +359,15 @@ $browser->assertElementsCount('table tbody tr', 0) ->assertSeeIn('table tfoot tr td', 'There are no distribution lists in this account.'); }); + + // Assert Settings tab + $browser->assertSeeIn('@nav #tab-settings', 'Settings') + ->click('@nav #tab-settings') + ->whenAvailable('@user-settings form', function (Browser $browser) { + $browser->assertElementsCount('.row', 1) + ->assertSeeIn('.row:first-child label', 'Greylisting') + ->assertSeeIn('.row:first-child .text-danger', 'disabled'); + }); }); } diff --git a/src/tests/Feature/Controller/UsersTest.php b/src/tests/Feature/Controller/UsersTest.php --- a/src/tests/Feature/Controller/UsersTest.php +++ b/src/tests/Feature/Controller/UsersTest.php @@ -245,7 +245,7 @@ $this->assertTrue(is_array($json['statusInfo'])); $this->assertTrue(is_array($json['settings'])); $this->assertTrue(is_array($json['aliases'])); - $this->assertTrue($json['config']['greylisting']); + $this->assertTrue($json['config']['greylist_enabled']); $this->assertSame([], $json['skus']); // Values below are tested by Unit tests $this->assertArrayHasKey('isDeleted', $json); @@ -478,14 +478,14 @@ $john->setSetting('greylist_enabled', null); // Test unknown user id - $post = ['greylisting' => 1]; + $post = ['greylist_enabled' => 1]; $response = $this->actingAs($john)->post("/api/v4/users/123/config", $post); $json = $response->json(); $response->assertStatus(404); // Test access by user not being a wallet controller - $post = ['greylisting' => 1]; + $post = ['greylist_enabled' => 1]; $response = $this->actingAs($jack)->post("/api/v4/users/{$john->id}/config", $post); $json = $response->json(); @@ -510,7 +510,7 @@ $this->assertNull($john->fresh()->getSetting('greylist_enabled')); // Test some valid data - $post = ['greylisting' => 1]; + $post = ['greylist_enabled' => 1]; $response = $this->actingAs($john)->post("/api/v4/users/{$john->id}/config", $post); $response->assertStatus(200); @@ -523,7 +523,7 @@ $this->assertSame('true', $john->fresh()->getSetting('greylist_enabled')); // Test some valid data - $post = ['greylisting' => 0]; + $post = ['greylist_enabled' => 0]; $response = $this->actingAs($john)->post("/api/v4/users/{$john->id}/config", $post); $response->assertStatus(200); 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 @@ -14,7 +14,6 @@ class GreylistTest extends TestCase { private $clientAddress; - private $instance; private $requests = []; private $net; @@ -24,20 +23,17 @@ $this->setUpTest(); $this->useServicesUrl(); - $this->instance = $this->generateInstanceId(); $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_settings;"); DB::delete("DELETE FROM greylist_whitelist WHERE sender_domain = 'sender.domain';"); } public function tearDown(): void { DB::delete("DELETE FROM greylist_connect WHERE sender_domain = 'sender.domain';"); - DB::delete("DELETE FROM greylist_settings;"); DB::delete("DELETE FROM greylist_whitelist WHERE sender_domain = 'sender.domain';"); parent::tearDown(); @@ -218,161 +214,6 @@ $this->assertFalse($request->shouldDefer()); } - public function testDomainDisabled() - { - $setting = Greylist\Setting::create( - [ - 'object_id' => $this->domainHosted->id, - 'object_type' => \App\Domain::class, - 'key' => 'greylist_enabled', - 'value' => 'false' - ] - ); - - $request = new Greylist\Request( - [ - 'sender' => 'someone@sender.domain', - 'recipient' => $this->domainOwner->email, - 'client_address' => $this->clientAddress - ] - ); - - $this->assertFalse($request->shouldDefer()); - } - - public function testDomainEnabled() - { - $connect = Greylist\Connect::create( - [ - 'sender_local' => 'someone', - 'sender_domain' => 'sender.domain', - 'recipient_hash' => hash('sha256', $this->domainOwner->email), - 'recipient_id' => $this->domainOwner->id, - 'recipient_type' => \App\User::class, - 'connect_count' => 1, - 'net_id' => \App\IP4Net::getNet('212.103.80.148')->id, - 'net_type' => \App\IP4Net::class - ] - ); - - $setting = Greylist\Setting::create( - [ - 'object_id' => $this->domainHosted->id, - 'object_type' => \App\Domain::class, - 'key' => 'greylist_enabled', - 'value' => 'true' - ] - ); - - $request = new Greylist\Request( - [ - 'sender' => 'someone@sender.domain', - 'recipient' => $this->domainOwner->email, - 'client_address' => $this->clientAddress - ] - ); - - $this->assertTrue($request->shouldDefer()); - - $connect->created_at = \Carbon\Carbon::now()->subMinutes(6); - $connect->save(); - - $this->assertFalse($request->shouldDefer()); - } - - public function testDomainDisabledUserDisabled() - { - $connect = Greylist\Connect::create( - [ - 'sender_local' => 'someone', - 'sender_domain' => 'sender.domain', - 'recipient_hash' => hash('sha256', $this->domainOwner->email), - 'recipient_id' => $this->domainOwner->id, - 'recipient_type' => \App\User::class, - 'connect_count' => 1, - 'net_id' => $this->net->id, - 'net_type' => \App\IP4Net::class - ] - ); - - $settingDomain = Greylist\Setting::create( - [ - 'object_id' => $this->domainHosted->id, - 'object_type' => \App\Domain::class, - 'key' => 'greylist_enabled', - 'value' => 'false' - ] - ); - - $settingUser = Greylist\Setting::create( - [ - 'object_id' => $this->domainOwner->id, - 'object_type' => \App\User::class, - 'key' => 'greylist_enabled', - 'value' => 'false' - ] - ); - - $request = new Greylist\Request( - [ - 'sender' => 'someone@sender.domain', - 'recipient' => $this->domainOwner->email, - 'client_address' => $this->clientAddress - ] - ); - - $this->assertFalse($request->shouldDefer()); - } - - public function testDomainDisabledUserEnabled() - { - $connect = Greylist\Connect::create( - [ - 'sender_local' => 'someone', - 'sender_domain' => 'sender.domain', - 'recipient_hash' => hash('sha256', $this->domainOwner->email), - 'recipient_id' => $this->domainOwner->id, - 'recipient_type' => \App\User::class, - 'connect_count' => 1, - 'net_id' => $this->net->id, - 'net_type' => \App\IP4Net::class - ] - ); - - $settingDomain = Greylist\Setting::create( - [ - 'object_id' => $this->domainHosted->id, - 'object_type' => \App\Domain::class, - 'key' => 'greylist_enabled', - 'value' => 'false' - ] - ); - - $settingUser = Greylist\Setting::create( - [ - 'object_id' => $this->domainOwner->id, - 'object_type' => \App\User::class, - 'key' => 'greylist_enabled', - 'value' => 'true' - ] - ); - - $request = new Greylist\Request( - [ - 'sender' => 'someone@sender.domain', - 'recipient' => $this->domainOwner->email, - 'client_address' => $this->clientAddress - ] - ); - - $this->assertTrue($request->shouldDefer()); - - $connect->created_at = \Carbon\Carbon::now()->subMinutes(6); - $connect->save(); - - $this->assertFalse($request->shouldDefer()); - } - public function testInvalidDomain() { $connect = Greylist\Connect::create( @@ -440,14 +281,7 @@ ] ); - $setting = Greylist\Setting::create( - [ - 'object_id' => $this->domainOwner->id, - 'object_type' => \App\User::class, - 'key' => 'greylist_enabled', - 'value' => 'false' - ] - ); + $this->domainOwner->setSetting('greylist_enabled', 'false'); $request = new Greylist\Request( [ @@ -459,7 +293,6 @@ $this->assertFalse($request->shouldDefer()); - // Ensure we also find the setting by alias $aliases = $this->domainOwner->aliases()->orderBy('alias')->get(); $request = new Greylist\Request( @@ -488,14 +321,7 @@ ] ); - $setting = Greylist\Setting::create( - [ - 'object_id' => $this->domainOwner->id, - 'object_type' => \App\User::class, - 'key' => 'greylist_enabled', - 'value' => 'true' - ] - ); + $this->domainOwner->setSetting('greylist_enabled', 'true'); $request = new Greylist\Request( [ @@ -537,14 +363,7 @@ ] ); - Greylist\Setting::create( - [ - 'object_id' => $user->id, - 'object_type' => \App\User::class, - 'key' => 'greylist_enabled', - 'value' => 'false' - ] - ); + $user->setSetting('greylist_enabled', 'false'); if ($user->email == $this->domainOwner->email) { continue; @@ -586,14 +405,7 @@ ] ); - Greylist\Setting::create( - [ - 'object_id' => $user->id, - 'object_type' => \App\User::class, - 'key' => 'greylist_enabled', - 'value' => ($user->id == $this->jack->id) ? 'true' : 'false' - ] - ); + $user->setSetting('greylist_enabled', ($user->id == $this->jack->id) ? 'true' : 'false'); if ($user->email == $this->domainOwner->email) { continue; @@ -614,17 +426,4 @@ } } } - - private function generateInstanceId() - { - $instance = []; - - for ($x = 0; $x < 3; $x++) { - for ($y = 0; $y < 3; $y++) { - $instance[] = substr('01234567889', rand(0, 9), 1); - } - } - - return implode('.', $instance); - } } diff --git a/src/tests/Feature/UserTest.php b/src/tests/Feature/UserTest.php --- a/src/tests/Feature/UserTest.php +++ b/src/tests/Feature/UserTest.php @@ -355,6 +355,27 @@ } /** + * Test User::getConfig() and setConfig() methods + */ + public function testConfigTrait(): void + { + $john = $this->getTestUser('john@kolab.org'); + $john->setSetting('greylist_enabled', null); + + $this->assertSame(['greylist_enabled' => true], $john->getConfig()); + + $result = $john->setConfig(['greylist_enabled' => false, 'unknown' => false]); + + $this->assertSame(['greylist_enabled' => false], $john->getConfig()); + $this->assertSame('false', $john->getSetting('greylist_enabled')); + + $result = $john->setConfig(['greylist_enabled' => true]); + + $this->assertSame(['greylist_enabled' => true], $john->getConfig()); + $this->assertSame('true', $john->getSetting('greylist_enabled')); + } + + /** * Test User::hasSku() method */ public function testHasSku(): void