D5373.1775385025.diff
No OneTemporary
Actions

Authored By

Unknown

Size

24 KB

Referenced Files

None

Subscribers

None

D5373.1775385025.diff
View Options

	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
	@@ -88,7 +88,7 @@
	$policy_config['greylist_policy'] = $config['greylist_policy'];

	if (config('app.with_mailfilter')) {
	- foreach (['itip_policy', 'externalsender_policy'] as $name) {
	+ foreach (['itip_policy', 'externalsender_policy', 'externalsender_policy_domains'] as $name) {
	$mail_delivery_policy[] = $name;
	$policy_config[$name] = $config[$name];
	}
	diff --git a/src/app/Policy/Mailfilter.php b/src/app/Policy/Mailfilter.php
	--- a/src/app/Policy/Mailfilter.php
	+++ b/src/app/Policy/Mailfilter.php
	@@ -143,16 +143,24 @@
	// Get user configuration and account policy
	$config = $user->getConfig(true);

	- foreach ($modules as $class => $module_config) {
	+ foreach (array_keys($modules) as $class) {
	$module = strtolower(str_replace('Module', '', class_basename($class)));
	+
	+ // Check if the module is enabled
	if (
	(isset($config["{$module}_config"]) && $config["{$module}_config"] === false)
	\|\| (!isset($config["{$module}_config"]) && empty($config["{$module}_policy"]))
	) {
	unset($modules[$class]);
	+ continue;
	}

	- // TODO: Collect module configuration
	+ // Collect module configuration
	+ foreach ($config as $key => $value) {
	+ if (str_starts_with($key, "{$module}_")) {
	+ $modules[$class][$key] = $value;
	+ }
	+ }
	}

	return $modules;
	diff --git a/src/app/Policy/Mailfilter/Modules/ExternalSenderModule.php b/src/app/Policy/Mailfilter/Modules/ExternalSenderModule.php
	--- a/src/app/Policy/Mailfilter/Modules/ExternalSenderModule.php
	+++ b/src/app/Policy/Mailfilter/Modules/ExternalSenderModule.php
	@@ -5,6 +5,7 @@
	use App\Policy\Mailfilter\MailParser;
	use App\Policy\Mailfilter\Module;
	use App\Policy\Mailfilter\Result;
	+use App\User;

	class ExternalSenderModule extends Module
	{
	@@ -14,9 +15,29 @@
	public function handle(MailParser $parser): ?Result
	{
	$sender = $parser->getSender();
	-
	$user = $parser->getUser();

	+ if ($this->isExternalSender($sender, $user)) {
	+ $subject = $parser->getHeader('subject');
	+
	+ // Update the subject with a prefix
	+ if (is_string($subject)) {
	+ $subject = '[EXTERNAL] ' . $subject;
	+ } else {
	+ $subject = '[EXTERNAL]';
	+ }
	+
	+ $parser->setHeader('Subject', $subject);
	+ }
	+
	+ return null;
	+ }
	+
	+ /**
	+ * Check if the sender is external for the user's account
	+ */
	+ private function isExternalSender($sender, User $user): bool
	+ {
	[, $sender_domain] = explode('@', $sender);
	[, $user_domain] = explode('@', $user->email);

	@@ -24,28 +45,30 @@

	// Sender and recipient in the same domain
	if ($sender_domain === $user_domain) {
	- return null; // just accept the message as-is
	+ return false;
	}

	- $account = $user->wallet()->owner;
	-
	- // Check against the account domains list
	- // TODO: Use a per-account/per-user list of additional domains
	- if ($account->domains(false, false)->where('namespace', $sender_domain)->exists()) {
	- return null; // just accept the message as-is
	+ // Check domain against a per-account list of additional domains
	+ if (!empty($this->config['externalsender_policy_domains'])
	+ && in_array($sender_domain, $this->config['externalsender_policy_domains'])
	+ ) {
	+ return false;
	}

	- $subject = $parser->getHeader('subject');
	-
	- // Update the subject with a prefix
	- if (is_string($subject)) {
	- $subject = '[EXTERNAL] ' . $subject;
	+ // TODO: Getting the account owner has been done already in the Mailfilter::getModulesConfig()
	+ // (by the getConfig() call). We should make it cached or something, for better performance.
	+ $wallet = $user->wallet();
	+ if ($wallet && $wallet->user_id != $user->id) {
	+ $account = $wallet->owner;
	} else {
	- $subject = '[EXTERNAL]';
	+ $account = $user;
	}

	- $parser->setHeader('Subject', $subject);
	+ // Check against the account domains list
	+ if ($account->domains(false, false)->where('namespace', $sender_domain)->exists()) {
	+ return false;
	+ }

	- return null;
	+ return true;
	}
	}
	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
	@@ -5,6 +5,7 @@
	use App\Policy\Password;
	use App\Policy\Utils as PolicyUtils;
	use App\Utils;
	+use Illuminate\Support\Facades\Validator;

	trait UserConfigTrait
	{
	@@ -18,6 +19,7 @@
	$settings = $this->getSettings([
	'externalsender_config',
	'externalsender_policy',
	+ 'externalsender_policy_domains',
	'greylist_enabled',
	'greylist_policy',
	'guam_enabled',
	@@ -28,18 +30,32 @@
	'password_policy',
	]);

	- $config = [
	- 'externalsender_config' => self::boolOrNull($settings['externalsender_config']),
	- 'externalsender_policy' => $settings['externalsender_policy'] === 'true',
	- 'greylist_enabled' => self::boolOrNull($settings['greylist_enabled']),
	- 'greylist_policy' => $settings['greylist_policy'] !== 'false',
	- 'guam_enabled' => $settings['guam_enabled'] === 'true',
	- 'itip_config' => self::boolOrNull($settings['itip_config']),
	- 'itip_policy' => $settings['itip_policy'] === 'true',
	- 'limit_geo' => $settings['limit_geo'] ? json_decode($settings['limit_geo'], true) : [],
	- 'max_password_age' => $settings['max_password_age'],
	- 'password_policy' => $settings['password_policy'],
	- ];
	+ $config = [];
	+ foreach ($settings as $key => $value) {
	+ switch ($key) {
	+ case 'externalsender_config':
	+ case 'greylist_enabled':
	+ case 'itip_config':
	+ $config[$key] = self::boolOrNull($value);
	+ break;
	+ case 'externalsender_policy_domains':
	+ case 'limit_geo':
	+ $config[$key] = $value ? json_decode($value, true) : [];
	+ break;
	+ case 'greylist_policy':
	+ $config[$key] = $value !== 'false';
	+ break;
	+ case 'externalsender_policy':
	+ case 'guam_enabled':
	+ case 'itip_policy':
	+ $config[$key] = $value === 'true';
	+ break;
	+ case 'max_password_age':
	+ case 'password_policy':
	+ default:
	+ $config[$key] = $value;
	+ }
	+ }

	// If user is not an account owner read the policy from the owner config
	if ($include_account_policies) {
	@@ -81,6 +97,13 @@
	continue;
	}

	+ $this->setSetting($key, !empty($value) ? json_encode($value) : null);
	+ } elseif ($key == 'externalsender_policy_domains') {
	+ if ($error = $this->validateExternalsenderPolicyDomains($value)) {
	+ $errors[$key] = $error;
	+ continue;
	+ }
	+
	$this->setSetting($key, !empty($value) ? json_encode($value) : null);
	} elseif ($key == 'max_password_age') {
	$this->setSetting($key, (int) $value > 0 ? ((string) (int) $value) : null);
	@@ -120,6 +143,34 @@
	return $value === 'true' ? true : ($value === 'false' ? false : null);
	}

	+ /**
	+ * Validates externalsender_policy_domains setting
	+ *
	+ * @param mixed $value List of domains input
	+ *
	+ * @return ?string An error message on error, Null otherwise
	+ */
	+ protected function validateExternalsenderPolicyDomains(&$value): ?string
	+ {
	+ if (!is_array($value)) {
	+ return \trans('validation.invalid-externalsender-domains');
	+ }
	+
	+ foreach ($value as $idx => $domain) {
	+ $domain = \strtolower($domain);
	+
	+ // TODO: Support asterisk expressions, e.g. *.domain.tld
	+ $v = Validator::make(['email' => 'user@' . $domain], ['email' => 'required\|email:strict']);
	+ if ($v->fails()) {
	+ return \trans('validation.invalid-externalsender-domains');
	+ }
	+
	+ $value[$idx] = $domain;
	+ }
	+
	+ return null;
	+ }
	+
	/**
	* Validates password policy rule.
	*
	diff --git a/src/resources/lang/en/ui.php b/src/resources/lang/en/ui.php
	--- a/src/resources/lang/en/ui.php
	+++ b/src/resources/lang/en/ui.php
	@@ -380,6 +380,8 @@
	'calinvitations-text' => "Enables automated handling of calendar invitations in incoming email.",
	'extsender' => "External sender warning",
	'extsender-text' => "Adds a warning to every delivered message sent by an external sender.",
	+ 'internaldomains' => "Internal domains",
	+ 'internaldomains-text' => "A list of internet domains to be considered as internal in context of the external sender warning.",
	'greylist' => "Greylisting",
	'greylist-text' => "Greylisting is a method of defending users against spam. Any incoming mail from an unrecognized sender "
	. "is temporarily rejected. The originating server should try again after a delay. "
	diff --git a/src/resources/lang/en/validation.php b/src/resources/lang/en/validation.php
	--- a/src/resources/lang/en/validation.php
	+++ b/src/resources/lang/en/validation.php
	@@ -170,6 +170,7 @@
	'nameexists' => 'The specified name is not available.',
	'nameinvalid' => 'The specified name is invalid.',
	'password-policy-error' => 'Specified password does not comply with the policy.',
	+ 'invalid-externalsender-domains' => 'Specified configuration is invalid. Expected a list domain names.',
	'invalid-limit-geo' => 'Specified configuration is invalid. Expected a list of two-letter country codes.',
	'invalid-limit-geo-missing-current' => 'Specified configuration is invalid. Missing country of the current connection (:code).',
	'invalid-password-policy' => 'Specified password policy is invalid.',
	diff --git a/src/resources/themes/forms.scss b/src/resources/themes/forms.scss
	--- a/src/resources/themes/forms.scss
	+++ b/src/resources/themes/forms.scss
	@@ -168,7 +168,7 @@
	& > div {
	padding-top: 0 !important;

	- input {
	+ input[type=checkbox] {
	position: absolute;
	top: 0.5rem;
	right: 1rem;
	diff --git a/src/resources/vue/Policies.vue b/src/resources/vue/Policies.vue
	--- a/src/resources/vue/Policies.vue
	+++ b/src/resources/vue/Policies.vue
	@@ -71,9 +71,18 @@
	<label for="externalsender_policy" class="col-sm-4 col-form-label">{{ $t('policies.extsender') }}</label>
	<div class="col-sm-8 pt-2">
	<input type="checkbox" id="externalsender_policy" name="externalsender" value="1" class="form-check-input d-block mb-2" :checked="config.externalsender_policy">
	- <small id="externalsender-hint" class="text-muted">
	+ <small id="externalsender-hint" class="text-muted d-block mb-2">
	{{ $t('policies.extsender-text') }}
	</small>
	+ <div class="row">
	+ <label for="externalsender_policy_domains-input" class="col-sm-4 col-form-label">{{ $t('policies.internaldomains') }}</label>
	+ <div class="col-sm-8">
	+ <list-input id="externalsender_policy_domains" :list="config.externalsender_policy_domains"></list-input>
	+ <small id="externalsenderdomains-hint" class="text-muted d-block mt-2">
	+ {{ $t('policies.internaldomains-text') }}
	+ </small>
	+ </div>
	+ </div>
	</div>
	</div>
	<btn class="btn-primary" type="submit" icon="check">{{ $t('btn.submit') }}</btn>
	@@ -87,9 +96,14 @@
	</template>

	<script>
	+ import ListInput from './Widgets/ListInput'
	+
	const POLICY_TYPES = ['password', 'mailDelivery']

	export default {
	+ components: {
	+ ListInput,
	+ },
	data() {
	return {
	config: [],
	@@ -134,10 +148,13 @@
	</label>`
	},
	submitMailDelivery() {
	- this.$root.clearFormValidation('#maildelivery form')
	+ this.$root.clearFormValidation('#mailDelivery form')
	+
	+ let post = this.$root.pick(this.config, ['externalsender_policy_domains'])

	- let post = {}
	- this.mailDeliveryPolicy.forEach(element => post[element] = $('#' + element)[0].checked)
	+ this.mailDeliveryPolicy
	+ .filter((element) => element != 'externalsender_policy_domains')
	+ .forEach(element => post[element] = $('#' + element)[0].checked)

	axios.post('/api/v4/users/' + this.wallet.user_id + '/config', post)
	.then(response => {
	diff --git a/src/tests/Browser/PoliciesTest.php b/src/tests/Browser/PoliciesTest.php
	--- a/src/tests/Browser/PoliciesTest.php
	+++ b/src/tests/Browser/PoliciesTest.php
	@@ -3,6 +3,7 @@
	namespace Tests\Browser;

	use Tests\Browser;
	+use Tests\Browser\Components\ListInput;
	use Tests\Browser\Components\Menu;
	use Tests\Browser\Components\Toast;
	use Tests\Browser\Pages\Dashboard;
	@@ -18,6 +19,7 @@
	$john->setSettings([
	'itip_policy' => null,
	'externalsender_policy' => null,
	+ 'externalsender_policy_domains' => null,
	'greylist_policy' => null,
	]);

	@@ -141,6 +143,7 @@
	$john->setSettings([
	'itip_policy' => 'true',
	'externalsender_policy' => null,
	+ 'externalsender_policy_domains' => null,
	'greylist_policy' => null,
	]);

	@@ -151,13 +154,19 @@
	->assertSeeIn('#policies .nav-item:nth-child(2)', 'Mail delivery')
	->click('#policies .nav-item:nth-child(2)')
	->with('@maildelivery-form', static function (Browser $browser) {
	- $browser->assertElementsCount('div.row', 3)
	- ->assertSeeIn('div.row:nth-child(1) label', 'Greylisting')
	+ $browser->assertSeeIn('div.row:nth-child(1) label', 'Greylisting')
	->assertChecked('div.row:nth-child(1) input[type=checkbox]')
	->assertSeeIn('div.row:nth-child(2) label', 'Calendar invitations')
	->assertChecked('div.row:nth-child(2) input[type=checkbox]')
	- ->assertSeeIn('div.row:nth-child(3) label', 'External sender warning')
	+ ->assertSeeIn('div.row:nth-child(3) > label', 'External sender warning')
	->assertNotChecked('div.row:nth-child(3) input[type=checkbox]')
	+ ->assertSeeIn('div.row:nth-child(3) .row > label', 'Internal domains')
	+ ->with(new ListInput('#externalsender_policy_domains'), static function (Browser $browser) {
	+ $browser->assertListInputValue([])
	+ ->assertValue('@input', '')
	+ ->addListEntry('test1.com')
	+ ->addListEntry('test2.com');
	+ })
	// Change the policy
	->click('div.row:nth-child(1) input[type=checkbox]')
	->click('div.row:nth-child(2) input[type=checkbox]')
	@@ -170,5 +179,6 @@
	$this->assertSame('false', $john->getSetting('greylist_policy'));
	$this->assertSame('false', $john->getSetting('itip_policy'));
	$this->assertSame('true', $john->getSetting('externalsender_policy'));
	+ $this->assertSame('["test1.com","test2.com"]', $john->getSetting('externalsender_policy_domains'));
	}
	}
	diff --git a/src/tests/Feature/Controller/PolicyTest.php b/src/tests/Feature/Controller/PolicyTest.php
	--- a/src/tests/Feature/Controller/PolicyTest.php
	+++ b/src/tests/Feature/Controller/PolicyTest.php
	@@ -214,9 +214,11 @@
	{
	$this->useRegularUrl();

	+ $keys = ['greylist_policy', 'itip_policy', 'externalsender_policy', 'externalsender_policy_domains'];
	+
	$jack = $this->getTestUser('jack@kolab.org');
	$john = $this->getTestUser('john@kolab.org');
	- $john->settings()->whereIn('key', ['itip_policy', 'externalsender_policy', 'greylist_policy'])->delete();
	+ $john->settings()->whereIn('key', $keys)->delete();

	// Unauth access not allowed
	$response = $this->get('/api/v4/policies');
	@@ -244,10 +246,11 @@

	$response->assertStatus(200);

	- $this->assertSame(['greylist_policy', 'itip_policy', 'externalsender_policy'], $json['mailDelivery']);
	+ $this->assertSame($keys, $json['mailDelivery']);
	$this->assertFalse($json['config']['itip_policy']);
	$this->assertTrue($json['config']['greylist_policy']);
	$this->assertTrue($json['config']['externalsender_policy']);
	+ $this->assertSame([], $json['config']['externalsender_policy_domains']);
	}

	/**
	diff --git a/src/tests/Feature/Policy/Mailfilter/Modules/ExternalSenderModuleTest.php b/src/tests/Feature/Policy/Mailfilter/Modules/ExternalSenderModuleTest.php
	--- a/src/tests/Feature/Policy/Mailfilter/Modules/ExternalSenderModuleTest.php
	+++ b/src/tests/Feature/Policy/Mailfilter/Modules/ExternalSenderModuleTest.php
	@@ -31,6 +31,15 @@
	$this->assertNull($result);
	$this->assertSame('[EXTERNAL] test sync', $parser->getHeader('subject'));

	+ // Test an email from an external sender - added domain to exceptions list
	+ $john = $this->getTestUser('john@kolab.org');
	+ $parser = MailParserTest::getParserForFile('mail/1.eml', 'john@kolab.org', "joe@{$domain}");
	+ $module = new ExternalSenderModule(['externalsender_policy_domains' => [$domain]]);
	+ $result = $module->handle($parser);
	+
	+ $this->assertNull($result);
	+ $this->assertSame('test sync', $parser->getHeader('subject'));
	+
	// Test an email from an internal sender (same domain)
	$parser = MailParserTest::getParserForFile('mail/1.eml', 'john@kolab.org', 'jack@kolab.org');
	$module = new ExternalSenderModule();
	@@ -46,7 +55,5 @@

	$this->assertNull($result);
	$this->assertSame('test sync', $parser->getHeader('subject'));
	-
	- // TODO: Test other account domains
	}
	}
	diff --git a/src/tests/Feature/Policy/MailfilterTest.php b/src/tests/Feature/Policy/MailfilterTest.php
	--- a/src/tests/Feature/Policy/MailfilterTest.php
	+++ b/src/tests/Feature/Policy/MailfilterTest.php
	@@ -3,6 +3,8 @@
	namespace Tests\Feature\Policy;

	use App\Policy\Mailfilter;
	+use App\Policy\Mailfilter\Modules\ExternalSenderModule;
	+use App\Policy\Mailfilter\Modules\ItipModule;
	use Illuminate\Http\Request;
	use Illuminate\Http\UploadedFile;
	use Tests\TestCase;
	@@ -12,6 +14,7 @@
	private $keys = [
	'externalsender_config',
	'externalsender_policy',
	+ 'externalsender_policy_domains',
	'itip_config',
	'itip_policy',
	];
	@@ -107,8 +110,15 @@
	// Enable account policies
	$john->setConfig(['externalsender_policy' => true, 'itip_policy' => true]);
	$expected = [
	- Mailfilter\Modules\ItipModule::class => [],
	- Mailfilter\Modules\ExternalSenderModule::class => [],
	+ ItipModule::class => [
	+ 'itip_config' => null,
	+ 'itip_policy' => true,
	+ ],
	+ ExternalSenderModule::class => [
	+ 'externalsender_config' => null,
	+ 'externalsender_policy' => true,
	+ 'externalsender_policy_domains' => [],
	+ ],
	];

	$this->assertSame($expected, $this->invokeMethod($filter, 'getModulesConfig', [$john]));
	@@ -116,27 +126,34 @@

	// Enabled account policies, and enabled per-user config
	$jack->setConfig(['externalsender_config' => true, 'itip_config' => true]);
	- $this->assertSame($expected, $this->invokeMethod($filter, 'getModulesConfig', [$john]));
	- $this->assertSame($expected, $this->invokeMethod($filter, 'getModulesConfig', [$jack]));
	+
	+ $result = $this->invokeMethod($filter, 'getModulesConfig', [$jack]);
	+ $this->assertTrue($result[ExternalSenderModule::class]['externalsender_config']);
	+ $this->assertTrue($result[ItipModule::class]['itip_config']);

	// Enabled account policies, and disabled per-user config
	$jack->setConfig(['externalsender_config' => false, 'itip_config' => false]);
	- $this->assertSame($expected, $this->invokeMethod($filter, 'getModulesConfig', [$john]));
	+
	$this->assertSame([], $this->invokeMethod($filter, 'getModulesConfig', [$jack]));

	// Disabled account policies, and disabled per-user config
	$john->setConfig(['externalsender_policy' => false, 'itip_policy' => false]);
	+
	$this->assertSame([], $this->invokeMethod($filter, 'getModulesConfig', [$john]));
	$this->assertSame([], $this->invokeMethod($filter, 'getModulesConfig', [$jack]));

	// Disabled account policies, and enabled per-user config
	$jack->setConfig(['externalsender_config' => true, 'itip_config' => true]);
	- $this->assertSame([], $this->invokeMethod($filter, 'getModulesConfig', [$john]));
	- $this->assertSame($expected, $this->invokeMethod($filter, 'getModulesConfig', [$jack]));
	+
	+ $result = $this->invokeMethod($filter, 'getModulesConfig', [$jack]);
	+ $this->assertTrue($result[ExternalSenderModule::class]['externalsender_config']); // @phpstan-ignore-line
	+ $this->assertTrue($result[ItipModule::class]['itip_config']); // @phpstan-ignore-line

	// As the last one, but for account owner
	$john->setConfig(['externalsender_config' => true, 'itip_config' => true]);
	- $this->assertSame($expected, $this->invokeMethod($filter, 'getModulesConfig', [$john]));
	- $this->assertSame($expected, $this->invokeMethod($filter, 'getModulesConfig', [$jack]));
	+
	+ $result = $this->invokeMethod($filter, 'getModulesConfig', [$john]);
	+ $this->assertTrue($result[ExternalSenderModule::class]['externalsender_config']);
	+ $this->assertTrue($result[ItipModule::class]['itip_config']);
	}
	}
	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
	@@ -563,6 +563,7 @@
	$user->setSetting('password_policy', null);
	$user->setSetting('max_password_age', null);
	$user->setSetting('limit_geo', null);
	+ $user->setSetting('externalsender_policy_domains', null);

	// greylist_enabled
	$this->assertNull($user->getConfig()['greylist_enabled']);
	@@ -645,6 +646,18 @@
	$this->assertSame('min:10,max:255', $user->getConfig()['password_policy']);
	$this->assertSame('min:10,max:255', $user->getSetting('password_policy'));

	+ // externalsender_policy_domains
	+ $result = $user->setConfig(['externalsender_policy_domains' => []]);
	+ $this->assertSame([], $user->getConfig()['externalsender_policy_domains']);
	+
	+ $result = $user->setConfig(['externalsender_policy_domains' => ['aa=a']]);
	+ $err = "Specified configuration is invalid. Expected a list domain names.";
	+ $this->assertSame(['externalsender_policy_domains' => $err], $result);
	+ $this->assertSame([], $user->getConfig()['externalsender_policy_domains']);
	+
	+ $result = $user->setConfig(['externalsender_policy_domains' => ['domain.tld', 'Test.com']]);
	+ $this->assertSame(['domain.tld', 'test.com'], $user->getConfig()['externalsender_policy_domains']);
	+
	// limit_geo
	$this->assertSame([], $user->getConfig()['limit_geo']);

File Metadata

Mime Type: text/plain
Expires: Sun, Apr 5, 10:30 AM (21 h, 37 m)
Storage Engine: blob
Storage Format: Raw Data
Storage Handle: 18833194
Default Alt Text: D5373.1775385025.diff (24 KB)

D5373.1775385025.diffNo OneTemporaryActions

D5373.1775385025.diffView Options

File Metadata

Event Timeline

D5373.1775385025.diff
No OneTemporary
Actions

D5373.1775385025.diff
View Options