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,12 @@ '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') + ->limit(1) + ->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,12 @@ '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') + ->limit(1) + ->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) { @@ -124,7 +122,7 @@ if ($domain) { $setting = Setting::where( [ - 'object_id' => $recipient->domain()->id, + 'object_id' => $domain->id, 'object_type' => \App\Domain::class, 'key' => 'greylist_enabled' ] @@ -228,8 +226,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 +249,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 +281,6 @@ private function recipientFromRequest() { - $recipient = null; - $recipients = \App\Utils::findObjectsByRecipientAddress($this->request['recipient']); if (sizeof($recipients) > 1) { diff --git a/src/app/Traits/UserConfigTrait.php b/src/app/Traits/UserConfigTrait.php --- a/src/app/Traits/UserConfigTrait.php +++ b/src/app/Traits/UserConfigTrait.php @@ -2,6 +2,8 @@ namespace App\Traits; +use App\Policy\Greylist; + trait UserConfigTrait { /** @@ -13,7 +15,11 @@ // TODO: Should we store the default value somewhere in config? - $config['greylisting'] = $this->getSetting('greylist_enabled') !== 'false'; + $config['greylisting'] = Greylist\Setting::where('key', 'greylist_enabled') + ->where('object_id', $this->id) + ->where('object_type', \App\User::class) + ->where('value', 'false') + ->exists() ? false : true; return $config; } @@ -31,7 +37,23 @@ foreach ($config as $key => $value) { if ($key == 'greylisting') { - $this->setSetting('greylist_enabled', $value ? 'true' : 'false'); + $setting_value = $value ? 'true' : 'false'; + $setting = Greylist\Setting::where('key', 'greylist_enabled') + ->where('object_id', $this->id) + ->where('object_type', \App\User::class) + ->first(); + + if (!$setting) { + Greylist\Setting::create([ + 'key' => 'greylist_enabled', + 'object_id' => $this->id, + 'object_type' => \App\User::class, + 'value' => $setting_value, + ]); + } else { + $setting->value = $setting_value; + $setting->save(); + } } 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_greylist_settings_migrate.php b/src/database/migrations/2021_08_25_120000_greylist_settings_migrate.php new file mode 100644 --- /dev/null +++ b/src/database/migrations/2021_08_25_120000_greylist_settings_migrate.php @@ -0,0 +1,43 @@ +where('key', 'greylist_enabled')->get(); + + foreach ($settings as $setting) { + DB::table('greylist_settings')->insert([ + 'key' => $setting->key, + 'value' => $setting->value, + 'object_id' => $setting->user_id, + 'object_type' => \App\User::class, + 'created_at' => $setting->created_at, + 'updated_at' => $setting->updated_at, + ]); + } + + DB::table('user_settings')->where('key', 'greylist_enabled')->delete(); + } + + /** + * Reverse the migrations. + * + * @return void + */ + public function down() + { + // do nothing + } +} diff --git a/src/tests/Browser/Admin/UserTest.php b/src/tests/Browser/Admin/UserTest.php --- a/src/tests/Browser/Admin/UserTest.php +++ b/src/tests/Browser/Admin/UserTest.php @@ -85,6 +85,7 @@ public function testUserInfo(): void { $this->browse(function (Browser $browser) { + \App\Policy\Greylist\Setting::where('key', 'greylist_enabled')->delete(); $jack = $this->getTestUser('jack@kolab.org'); $page = new UserPage($jack->id); @@ -197,7 +198,7 @@ $wallet->save(); $group = $this->getTestGroup('group-test@kolab.org'); $group->assignToWallet($john->wallets->first()); - $john->setSetting('greylist_enabled', null); + \App\Policy\Greylist\Setting::where('key', 'greylist_enabled')->delete(); // Click the managed-by link on Jack's page $browser->click('@user-info #manager a') @@ -322,7 +323,7 @@ ]); $page = new UserPage($ned->id); - $ned->setSetting('greylist_enabled', 'false'); + $ned->setConfig(['greylisting' => false]); $browser->click('@user-users tbody tr:nth-child(4) td:first-child a') ->on($page); 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 @@ -175,6 +175,8 @@ */ public function testUserInfo2(): void { + \App\Policy\Greylist\Setting::where('key', 'greylist_enabled')->delete(); + $this->browse(function (Browser $browser) { $john = $this->getTestUser('john@kolab.org'); $page = new UserPage($john->id); @@ -288,6 +290,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->setConfig(['greylisting' => false]); $page = new UserPage($ned->id); $browser->click('@user-users tbody tr:nth-child(4) td:first-child a') @@ -358,6 +361,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/Browser/UsersTest.php b/src/tests/Browser/UsersTest.php --- a/src/tests/Browser/UsersTest.php +++ b/src/tests/Browser/UsersTest.php @@ -341,7 +341,7 @@ public function testUserSettings(): void { $john = $this->getTestUser('john@kolab.org'); - $john->setSetting('greylist_enabled', null); + \App\Policy\Greylist\Setting::where('key', 'greylist_enabled')->delete(); $this->browse(function (Browser $browser) use ($john) { $browser->visit('/user/' . $john->id) @@ -358,7 +358,7 @@ }); }); - $this->assertSame('false', $john->fresh()->getSetting('greylist_enabled')); + $this->assertFalse($john->getConfig()['greylisting']); } /** 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 @@ -6,6 +6,7 @@ use App\Domain; use App\Http\Controllers\API\V4\UsersController; use App\Package; +use App\Policy\Greylist; use App\Sku; use App\User; use App\Wallet; @@ -66,7 +67,7 @@ $wallet->discount()->dissociate(); $wallet->settings()->whereIn('key', ['mollie_id', 'stripe_id'])->delete(); $wallet->save(); - $user->settings()->whereIn('key', ['greylist_enabled'])->delete(); + Greylist\Setting::where('key', 'greylist_enabled')->delete(); $user->status |= User::STATUS_IMAP_READY; $user->save(); @@ -475,7 +476,7 @@ $jack = $this->getTestUser('jack@kolab.org'); $john = $this->getTestUser('john@kolab.org'); - $john->setSetting('greylist_enabled', null); + Greylist\Setting::where('key', 'greylist_enabled')->delete(); // Test unknown user id $post = ['greylisting' => 1]; @@ -507,7 +508,7 @@ $this->assertCount(1, $json['errors']); $this->assertSame('The requested configuration parameter is not supported.', $json['errors']['grey']); - $this->assertNull($john->fresh()->getSetting('greylist_enabled')); + $this->assertTrue($john->getConfig()['greylisting']); // Test some valid data $post = ['greylisting' => 1]; @@ -520,7 +521,7 @@ $this->assertSame('success', $json['status']); $this->assertSame('User settings updated successfully.', $json['message']); - $this->assertSame('true', $john->fresh()->getSetting('greylist_enabled')); + $this->assertTrue($john->getConfig()['greylisting']); // Test some valid data $post = ['greylisting' => 0]; @@ -533,7 +534,7 @@ $this->assertSame('success', $json['status']); $this->assertSame('User settings updated successfully.', $json['message']); - $this->assertSame('false', $john->fresh()->getSetting('greylist_enabled')); + $this->assertFalse($john->getConfig()['greylisting']); } /** 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 @@ -354,6 +354,34 @@ $this->assertNotContains($domain->namespace, $domains); } + /** + * Test User::getConfig() and setConfig() methods + */ + public function testConfigTrait(): void + { + $john = $this->getTestUser('john@kolab.org'); + \App\Policy\Greylist\Setting::where('key', 'greylist_enabled')->delete(); + + $this->assertSame(['greylisting' => true], $john->getConfig()); + + $result = $john->setConfig(['greylisting' => false, 'unknown' => false]); + + $this->assertSame(['greylisting' => false], $john->getConfig()); + + $setting = \App\Policy\Greylist\Setting::where('key', 'greylist_enabled') + ->where('object_id', $john->id) + ->where('object_type', User::class) + ->first(); + + $this->assertSame('false', $setting->value); + + $result = $john->setConfig(['greylisting' => true]); + + $this->assertSame(['greylisting' => true], $john->getConfig()); + + $this->assertSame('true', $setting->fresh()->value); + } + /** * Test User::hasSku() method */