D5379.1775288486.diff
No OneTemporary
Actions

Authored By

Unknown

Size

17 KB

Referenced Files

None

Subscribers

None

D5379.1775288486.diff
View Options

	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
	@@ -18,6 +18,8 @@
	public const CODE_REJECT = 460;
	public const CODE_ERROR = 500;

	+ protected static $debugid = '';
	+
	/**
	* SMTP Content Filter
	*
	@@ -47,9 +49,13 @@
	// then we'd send body in another request, but only if needed. For example, a text/plain
	// message from same domain sender does not include an iTip, nor needs a footer injection.

	+ self::$debugid = dechex((int) str_replace('.', '', explode(' ', microtime())[0]));
	+ self::debug("Processing message from {$request->sender} to {$request->recipient}...");
	+
	// Email with multiple recipients, which we don't handle at the moment.
	// Likely an outgoing email, so we just accept.
	if (str_contains($request->recipient, ",")) {
	+ self::debug('Multiple recipients', self::CODE_ACCEPT_EMPTY);
	return response('', self::CODE_ACCEPT_EMPTY);
	}

	@@ -58,6 +64,7 @@

	// Not a local recipient, so e.g. an outgoing email
	if (empty($user)) {
	+ self::debug('Unknown recipient', self::CODE_ACCEPT_EMPTY);
	return response('', self::CODE_ACCEPT_EMPTY);
	}

	@@ -65,6 +72,7 @@
	$modules = self::getModulesConfig($user);

	if (empty($modules)) {
	+ self::debug('All modules disabled', self::CODE_ACCEPT_EMPTY);
	return response('', self::CODE_ACCEPT_EMPTY);
	}

	@@ -74,6 +82,7 @@
	if (count($files) == 1) {
	$file = $files[array_key_first($files)];
	if (!$file->isValid()) {
	+ self::debug('Invalid file upload', self::CODE_ERROR);
	return response('Invalid file upload', self::CODE_ERROR);
	}

	@@ -85,23 +94,31 @@
	// Initialize mail parser
	$parser = new MailParser($stream);
	$parser->setRecipient($user);
	+ $parser->setDebugPrefix('<' . self::$debugid . '> Mailfilter: ');

	if ($sender = $request->sender) {
	$parser->setSender($sender);
	}

	+ self::debug("Message-ID: " . ($parser->getMessageId() ?? 'unset'));
	+
	// Execute modules
	foreach ($modules as $module => $config) {
	+ $module_name = str_replace('Module', '', \class_basename($module));
	+ self::debug("Executing module {$module_name}...");
	+
	$engine = new $module($config);

	$result = $engine->handle($parser);

	if ($result) {
	if ($result->getStatus() == Result::STATUS_REJECT) {
	+ self::debug("Rejected by {$module_name}", self::CODE_REJECT);
	// FIXME: Better code? Should we use custom header instead?
	return response('', self::CODE_REJECT);
	}
	if ($result->getStatus() == Result::STATUS_DISCARD) {
	+ self::debug("Discarded by {$module_name}", self::CODE_DISCARD);
	// FIXME: Better code? Should we use custom header instead?
	return response('', self::CODE_DISCARD);
	}
	@@ -124,12 +141,38 @@
	fclose($stream);
	});

	+ self::debug('Message modified', self::CODE_ACCEPT);
	+
	return $response;
	}

	+ self::debug('Message intact', self::CODE_ACCEPT_EMPTY);
	+
	return response('', self::CODE_ACCEPT_EMPTY);
	}

	+ /**
	+ * Log a debug message
	+ */
	+ protected static function debug(string $message, $status = null): void
	+ {
	+ $debug = 'Mailfilter:';
	+
	+ if (self::$debugid !== '') {
	+ $debug = '<' . self::$debugid . '> ' . $debug;
	+ }
	+
	+ if ($message !== '') {
	+ $debug .= ' ' . $message;
	+ }
	+
	+ if ($status) {
	+ $debug .= " [{$status}]";
	+ }
	+
	+ \Log::debug($debug);
	+ }
	+
	/**
	* Get list of enabled mail filter modules with their configuration
	*/
	diff --git a/src/app/Policy/Mailfilter/MailParser.php b/src/app/Policy/Mailfilter/MailParser.php
	--- a/src/app/Policy/Mailfilter/MailParser.php
	+++ b/src/app/Policy/Mailfilter/MailParser.php
	@@ -7,7 +7,7 @@
	class MailParser
	{
	protected $stream;
	-
	+ protected string $debug_prefix = '';
	protected int $start = 0;
	protected ?int $end;
	protected ?int $bodyPosition;
	@@ -24,6 +24,7 @@
	'content-transfer-encoding',
	'content-type',
	'from',
	+ 'message-id',
	'subject',
	];

	@@ -43,6 +44,18 @@
	$this->parseHeaders();
	}

	+ /**
	+ * Log a debug message
	+ */
	+ public function debug(string $message): void
	+ {
	+ if ($this->debug_prefix) {
	+ $message = $this->debug_prefix . $message;
	+ }
	+
	+ \Log::debug($message);
	+ }
	+
	/**
	* Get mail header
	*/
	@@ -110,6 +123,14 @@
	return $this->bodyPosition;
	}

	+ /**
	+ * Get mail Message-Id header
	+ */
	+ public function getMessageId(): ?string
	+ {
	+ return $this->getHeader('message-id');
	+ }
	+
	/**
	* Returns email address of the recipient
	*/
	@@ -267,6 +288,14 @@
	$this->modified = true;
	}

	+ /**
	+ * Set debug line prefix for log entries
	+ */
	+ public function setDebugPrefix(string $prefix): void
	+ {
	+ $this->debug_prefix = $prefix;
	+ }
	+
	/**
	* Set header value
	*
	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
	@@ -18,6 +18,8 @@
	$user = $parser->getUser();

	if ($this->isExternalSender($sender, $user)) {
	+ $parser->debug("{$sender} is an external sender");
	+
	$subject = $parser->getHeader('subject');

	// Update the subject with a prefix
	@@ -28,6 +30,8 @@
	}

	$parser->setHeader('Subject', $subject);
	+ } else {
	+ $parser->debug("{$sender} is an internal sender");
	}

	return null;
	diff --git a/src/app/Policy/Mailfilter/Modules/ItipModule.php b/src/app/Policy/Mailfilter/Modules/ItipModule.php
	--- a/src/app/Policy/Mailfilter/Modules/ItipModule.php
	+++ b/src/app/Policy/Mailfilter/Modules/ItipModule.php
	@@ -21,6 +21,9 @@
	protected $davTokenExpiresOn;
	protected $davTTL = 10;

	+ /** @var MailParser Mail parser instance for the current message */
	+ protected $parser;
	+
	/** @var string Processed object type ('VEVENT' or 'VTODO') */
	protected $type;

	@@ -35,6 +38,7 @@
	$itip = self::getItip($parser);

	if ($itip === null) {
	+ $parser->debug("No iTip in the message");
	return null; // do nothing
	}

	@@ -43,25 +47,34 @@
	$vobject = $this->parseICal($itip);

	if ($vobject === null) {
	+ $parser->debug("No supported iCalendar data in the iTip");
	return null; // do nothing
	}

	+ $this->parser = $parser;
	+
	// Note: Some iTip handling implementation can be find in vendor/sabre/vobject/lib/ITip/Broker.php,
	// however I think we need something more sophisticated that we can extend ourselves.

	// FIXME: If $vobject->METHOD is empty fallback to 'method' param from the Content-Type header?
	// rfc5545#section-3.7.2 says if one is specified the other must be too
	// @phpstan-ignore-next-line
	- switch (\strtoupper((string) $vobject->METHOD)) {
	+ $method = \strtoupper((string) $vobject->METHOD);
	+ switch ($method) {
	case 'REQUEST':
	+ $parser->debug("Parsing an iTip REQUEST...");
	$handler = new ItipModule\RequestHandler($vobject, $this->type, $this->uid);
	break;
	case 'CANCEL':
	+ $parser->debug("Parsing an iTip CANCEL...");
	$handler = new ItipModule\CancelHandler($vobject, $this->type, $this->uid);
	break;
	case 'REPLY':
	+ $parser->debug("Parsing an iTip REPLY...");
	$handler = new ItipModule\ReplyHandler($vobject, $this->type, $this->uid);
	break;
	+ default:
	+ $parser->debug("Unsupported iTip method: {$method}. iTip ignored.");
	}

	// FIXME: Should we handle (any?) errors silently and just deliver the message to Inbox as a fallback?
	@@ -120,6 +133,8 @@
	return null;
	}

	+ $this->parser->debug("Searching for DAV object (UID={$uid}, User={$user->email})...");
	+
	$dav = $this->getDAVClient($user);
	$filters = [new DAV\SearchPropFilter('UID', DAV\SearchPropFilter::MATCH_EQUALS, $uid)];
	$search = new DAV\Search($dav_type, true, $filters);
	@@ -140,16 +155,22 @@
	$this->davFolder = $folder;
	}

	+ $this->parser->debug("Searching in {$folder->href}...");
	+
	foreach ($dav->search($folder->href, $search, null, true) as $event) {
	if ($vobject = $this->parseICal((string) $event)) {
	$this->davLocation = $event->href;
	$this->davFolder = $folder;

	+ $this->parser->debug("Object found: {$this->davLocation}");
	+
	return $vobject;
	}
	}
	}

	+ $this->parser->debug("Object not found");
	+
	return null;
	}

	diff --git a/src/app/Policy/Mailfilter/Modules/ItipModule/CancelHandler.php b/src/app/Policy/Mailfilter/Modules/ItipModule/CancelHandler.php
	--- a/src/app/Policy/Mailfilter/Modules/ItipModule/CancelHandler.php
	+++ b/src/app/Policy/Mailfilter/Modules/ItipModule/CancelHandler.php
	@@ -27,6 +27,8 @@
	{
	$user = $parser->getUser();

	+ $this->parser = $parser;
	+
	// Check whether the event already exists
	$existing = $this->findObject($user, $this->uid, $this->type);

	@@ -44,12 +46,14 @@

	if (!$existingMaster \|\| !$cancelMaster) {
	// FIXME: Should we stop message delivery?
	+ $parser->debug("Failed to get the main component. Ignored.");
	return null;
	}

	// SEQUENCE does not match, deliver the message, let the MUAs to deal with this
	// FIXME: Is this even a valid aproach regarding recurrence?
	if ((string) $existingMaster->SEQUENCE != (string) $cancelMaster->SEQUENCE) {
	+ $parser->debug("Sequence mismatch. Ignored.");
	return null;
	}

	@@ -70,11 +74,15 @@
	$exdate = $cancelMaster->{'RECURRENCE-ID'}->getDateTime();
	$existingMaster->add('EXDATE', $exdate, ['VALUE' => 'DATE'], 'DATE');

	+ $parser->debug("Updating object at {$this->davLocation}");
	+
	$dav = $this->getDAVClient($user);
	$dav->update($this->toOpaqueObject($existing, $this->davLocation));
	} else {
	$existingInstance = $existingMaster;

	+ $parser->debug("Deleting object at {$this->davLocation}");
	+
	// Remove the event from attendee's calendar
	// Note: We make this the default case because Outlook does not like events with cancelled status
	// optionally we could update the event with STATUS=CANCELLED instead.
	@@ -83,9 +91,10 @@
	}

	// Send a notification to the recipient (attendee)
	+ $parser->debug("Sending notification to {$user->email}");
	$user->notify($this->notification($existingInstance, $cancelMaster->COMMENT));

	- // Remove (not deliver) the message to the attendee's inbox
	+ // Stop message delivery to the attendee's Inbox
	return new Result(Result::STATUS_DISCARD);
	}

	diff --git a/src/app/Policy/Mailfilter/Modules/ItipModule/ReplyHandler.php b/src/app/Policy/Mailfilter/Modules/ItipModule/ReplyHandler.php
	--- a/src/app/Policy/Mailfilter/Modules/ItipModule/ReplyHandler.php
	+++ b/src/app/Policy/Mailfilter/Modules/ItipModule/ReplyHandler.php
	@@ -34,6 +34,8 @@
	// TODO: We might need to use DAV locking mechanism if multiple processes
	// are likely to attempt to update the same event at the same time.

	+ $this->parser = $parser;
	+
	// Check whether the event already exists
	$existing = $this->findObject($user, $this->uid, $this->type);

	@@ -50,17 +52,20 @@
	$replyMaster = $this->extractMainComponent($this->itip);

	if (!$existingMaster \|\| !$replyMaster) {
	+ $parser->debug("Failed to get the main component. Ignored.");
	return null;
	}

	// SEQUENCE does not match, deliver the message, let the MUAs to deal with this
	// FIXME: Is this even a valid aproach regarding recurrence?
	if ((string) $existingMaster->SEQUENCE != (string) $replyMaster->SEQUENCE) {
	+ $parser->debug("Sequence mismatch. Ignored.");
	return null;
	}

	// Per RFC 5546 there can be only one ATTENDEE in REPLY
	if (count($replyMaster->ATTENDEE) != 1) {
	+ $parser->debug("Too many attendees in REPLY. Ignored.");
	return null;
	}

	@@ -72,12 +77,14 @@

	// Supporting attendees w/o an email address could be considered in the future
	if (empty($email)) {
	+ $parser->debug("Attendee without an email address. Ignored.");
	return null;
	}

	// Invalid/useless reply, let the MUA deal with it
	// FIXME: Or should we stop delivery?
	if (empty($partstat) \|\| $partstat == 'NEEDS-ACTION') {
	+ $parser->debug("Unexpected PARTSTAT in REPLY. Ignored.");
	return null;
	}

	@@ -88,6 +95,7 @@
	// No such recurrence exception, let the MUA deal with it
	// FIXME: Or should we stop delivery?
	if (!$existingInstance) {
	+ $parser->debug("No existing recurrence instance. Ignored.");
	return null;
	}
	} else {
	@@ -109,6 +117,8 @@
	}

	if ($updated) {
	+ $parser->debug("Updating object at {$this->davLocation}");
	+
	$dav = $this->getDAVClient($user);
	$dav->update($this->toOpaqueObject($existing, $this->davLocation));

	@@ -118,7 +128,10 @@
	// Remove (not deliver) the message to the organizer's inbox

	// Send a notification to the organizer
	+ $parser->debug("Sending notification to {$user->email}");
	$user->notify($this->notification($existingInstance, $sender, $replyMaster->COMMENT));
	+ } else {
	+ $parser->debug("Object unchanged");
	}

	return new Result(Result::STATUS_DISCARD);
	diff --git a/src/app/Policy/Mailfilter/Modules/ItipModule/RequestHandler.php b/src/app/Policy/Mailfilter/Modules/ItipModule/RequestHandler.php
	--- a/src/app/Policy/Mailfilter/Modules/ItipModule/RequestHandler.php
	+++ b/src/app/Policy/Mailfilter/Modules/ItipModule/RequestHandler.php
	@@ -41,6 +41,8 @@
	// but CANCEL and REPLY could not, because we're potentially stopping mail delivery there,
	// so I suppose we'll do all of them synchronously for now. Still some parts of it can be async.

	+ $this->parser = $parser;
	+
	// Check whether the object already exists in the recipient's calendar
	$existing = $this->findObject($user, $this->uid, $this->type);

	@@ -59,12 +61,17 @@
	// The event does not exist yet in the recipient's calendar, create it
	if (!$existing) {
	if (!empty($recurrence_id)) {
	+ $parser->debug("Object does not exist, but it's a recurring instance. Ignored.");
	return null;
	}

	// Create the event in the recipient's calendar
	$dav = $this->getDAVClient($user);
	- $dav->create($this->toOpaqueObject($this->itip));
	+ $object = $this->toOpaqueObject($this->itip);
	+
	+ $parser->debug("Object does not exist, saving into {$object->href}");
	+
	+ $dav->create($object);

	return null;
	}
	@@ -80,8 +87,8 @@

	// A new recurrence instance, just add it to the existing event
	if (!$existingInstance) {
	+ // TODO: Bump LAST-MODIFIED on the master object
	$existing->add($requestMaster);
	- // TODO: Bump LAST-MODIFIED on the master object
	} else {
	// SEQUENCE does not match, deliver the message, let the MUAs deal with this
	// TODO: A higher SEQUENCE indicates a re-scheduled object, we should update the existing event.
	@@ -110,6 +117,8 @@
	$this->mergeComponents($existingMaster, $requestMaster);
	}

	+ $parser->debug("Updating object at {$this->davLocation}");
	+
	$dav = $this->getDAVClient($user);
	$dav->update($this->toOpaqueObject($existing, $this->davLocation));

	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
	@@ -272,7 +272,7 @@
	->assertNoContent(204);

	// Test returning (modified) mail content
	- $john->setConfig(['externalsender_policy' => true]);
	+ $john->setConfig(['externalsender_policy' => true, 'itip_policy' => true]);
	$url = '/api/webhooks/policy/mail/filter?recipient=john@kolab.org&sender=jack@external.tld';
	$content = $this->call('POST', $url, [], [], [], $headers, $post)
	->assertStatus(200)

File Metadata

Mime Type: text/plain
Expires: Sat, Apr 4, 7:41 AM (10 h, 36 m)
Storage Engine: blob
Storage Format: Raw Data
Storage Handle: 18770082
Default Alt Text: D5379.1775288486.diff (17 KB)

D5379.1775288486.diffNo OneTemporaryActions

D5379.1775288486.diffView Options

File Metadata

Event Timeline

D5379.1775288486.diff
No OneTemporary
Actions

D5379.1775288486.diff
View Options