[mail] Refactor queue for easier maintenance, use name

We thought the name parameter was the account name to be shown in the
plugin, but it is the contents of the "From" email header instead.

While changing that, we also update the code to better match the open
Pull Request upstream that adds the update-account to the mail plugin
for nextcloud.

dd-apps/docker/nextcloud/Dockerfile

@@ -69,6 +69,7 @@ COPY supervisord.conf /
 # Temporary replacement for a real queue
 RUN echo '*/1 * * * * /nc-queue.sh' >> /etc/crontabs/www-data
 COPY nc-queue.sh /
+COPY nc-mail-update.sh /
 COPY saml.sh /
 
 ENV NEXTCLOUD_UPDATE=1

dd-apps/docker/nextcloud/nc-mail-update.sh

@@ -0,0 +1,50 @@
+#!/bin/sh -eu
+
+OCC="${OCC:-/var/www/html/occ}"
+#
+# For this user, obtain lines formatted as: EmailAccountId:Email
+#
+get_mail_accounts() {
+	"$OCC" mail:account:export "$1" | \
+		grep -E '^([^-]|- E-Mail)' | tr -d '\n' | \
+		sed -Ee 's!(Account|- E-Mail: )!!g' | tr -d ' ' '\n' || true
+}
+
+# User-specific
+user_id="$1"
+account_name="$2"
+email="$3"
+email_password="$4"
+# Server settings
+inbound_host="$5"
+inbound_port="$6"
+inbound_ssl_mode="$7"
+outbound_host="$8"
+outbound_port="$9"
+outbound_ssl_mode="${10}"
+
+existing_mail_accounts="$(get_mail_accounts "$user_id")"
+
+if [ -n "${existing_mail_accounts:-}" ]; then
+    # Use the first one, it was likely created by DD
+    account_id="$(echo "${existing_mail_accounts}" | head -n 1 | cut -d ':' -f 1)"
+fi
+
+if [ -z "${account_id:-}" ]; then
+    # Create account
+    "$OCC" mail:account:create \
+	"$user_id" "$account_name" "$email" \
+	"$inbound_host" "$inbound_port" "$inbound_ssl_mode" \
+	"$email" "$email_password" \
+	"$outbound_host" "$outbound_port" "$outbound_ssl_mode" \
+	"$email" "$email_password"
+else
+    # Update account
+    "$OCC" mail:account:update \
+	--imap-host "$inbound_host" --imap-port "$inbound_port" --imap-ssl-mode "$inbound_ssl_mode" \
+	--imap-user "$email" --imap-password "$email_password" \
+	--smtp-host "$outbound_host" --smtp-port "$outbound_port" --smtp-ssl-mode "$outbound_ssl_mode" \
+	--smtp-user "$email" --smtp-password "$email_password" \
+	--name "$account_name" --email "$email" \
+	-- "$account_id"
+fi

dd-apps/docker/nextcloud/nc-queue.sh

@@ -1,5 +1,5 @@
-#/bin/sh
+#!/bin/sh
 
 find "${NC_MAIL_QUEUE_FOLDER:-/nc-mail-queue}" -name '*.sh' -exec sh -c \
-	'cd /var/www/html && {} && rm {}' \
+	'i="$1"; "$i" && rm "$i"' shell {} \
 	';'

dd-apps/docker/nextcloud/nc_mail/lib/Command/UpdateAccount.php

@@ -24,6 +24,7 @@ namespace OCA\Mail\Command;
 
 use OCA\Mail\Db\MailAccountMapper;
 use OCP\Security\ICrypto;
+use OCP\AppFramework\Db\DoesNotExistException;
 use Symfony\Component\Console\Command\Command;
 use Symfony\Component\Console\Input\InputArgument;
 use Symfony\Component\Console\Input\InputOption;
@@ -31,8 +32,10 @@ use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Output\OutputInterface;
 
 class UpdateAccount extends Command {
-	public const ARGUMENT_USER_ID = 'user-id';
+	public const ARGUMENT_ACCOUNT_ID = 'account-id';
+	public const ARGUMENT_NAME = 'name';
 	public const ARGUMENT_EMAIL = 'email';
+	public const ARGUMENT_AUTH_METHOD = 'auth-method';
 	public const ARGUMENT_IMAP_HOST = 'imap-host';
 	public const ARGUMENT_IMAP_PORT = 'imap-port';
 	public const ARGUMENT_IMAP_SSL_MODE = 'imap-ssl-mode';
@@ -44,6 +47,7 @@ class UpdateAccount extends Command {
 	public const ARGUMENT_SMTP_USER = 'smtp-user';
 	public const ARGUMENT_SMTP_PASSWORD = 'smtp-password';
 
+
 	/** @var mapper */
 	private $mapper;
 
@@ -63,8 +67,10 @@ class UpdateAccount extends Command {
 	protected function configure() {
 		$this->setName('mail:account:update');
 		$this->setDescription('Update a user\'s IMAP account');
-		$this->addArgument(self::ARGUMENT_USER_ID, InputArgument::REQUIRED);
+		$this->addArgument(self::ARGUMENT_ACCOUNT_ID, InputArgument::REQUIRED);
-		$this->addArgument(self::ARGUMENT_EMAIL, InputArgument::REQUIRED);
+
+		$this->addOption(self::ARGUMENT_NAME, '', InputOption::VALUE_OPTIONAL);
+		$this->addOption(self::ARGUMENT_EMAIL, '', InputOption::VALUE_OPTIONAL);
 
 		$this->addOption(self::ARGUMENT_IMAP_HOST, '', InputOption::VALUE_OPTIONAL);
 		$this->addOption(self::ARGUMENT_IMAP_PORT, '', InputOption::VALUE_OPTIONAL);
@@ -77,11 +83,15 @@ class UpdateAccount extends Command {
 		$this->addOption(self::ARGUMENT_SMTP_SSL_MODE, '', InputOption::VALUE_OPTIONAL);
 		$this->addOption(self::ARGUMENT_SMTP_USER, '', InputOption::VALUE_OPTIONAL);
 		$this->addOption(self::ARGUMENT_SMTP_PASSWORD, '', InputOption::VALUE_OPTIONAL);
+
+		$this->addOption(self::ARGUMENT_AUTH_METHOD, '', InputOption::VALUE_OPTIONAL);
 	}
 
 	protected function execute(InputInterface $input, OutputInterface $output): int {
-		$userId = $input->getArgument(self::ARGUMENT_USER_ID);
+		$accountId = (int)$input->getArgument(self::ARGUMENT_ACCOUNT_ID);
-		$email = $input->getArgument(self::ARGUMENT_EMAIL);
+
+		$name = $input->getOption(self::ARGUMENT_NAME);
+		$email = $input->getOption(self::ARGUMENT_EMAIL);
 
 		$imapHost = $input->getOption(self::ARGUMENT_IMAP_HOST);
 		$imapPort = $input->getOption(self::ARGUMENT_IMAP_PORT);
@@ -94,61 +104,76 @@ class UpdateAccount extends Command {
 		$smtpSslMode = $input->getOption(self::ARGUMENT_SMTP_SSL_MODE);
 		$smtpUser = $input->getOption(self::ARGUMENT_SMTP_USER);
 		$smtpPassword = $input->getOption(self::ARGUMENT_SMTP_PASSWORD);
+		$authMethod = $input->getOption(self::ARGUMENT_AUTH_METHOD);
 
-		$mailAccount = $this->mapper->findByUserIdAndEmail($userId, $email);
+		try {
-
+			$mailAccount = $this->mapper->findById($accountId);
-		if ($mailAccount) {
+		} catch (DoesNotExistException $e) {
-			//INBOUND
+			$output->writeln("<error>No Email Account found with ID $accountId </error>");
-			if ($input->getOption(self::ARGUMENT_IMAP_HOST)) {
-				$mailAccount->setInboundHost($imapHost);
-			}
-
-			if ($input->getOption(self::ARGUMENT_IMAP_PORT)) {
-				$mailAccount->setInboundPort((int) $imapPort);
-			}
-
-			if ($input->getOption(self::ARGUMENT_IMAP_SSL_MODE)) {
-				$mailAccount->setInboundSslMode($imapSslMode);
-			}
-
-			if ($input->getOption(self::ARGUMENT_IMAP_PASSWORD)) {
-				$mailAccount->setInboundPassword($this->crypto->encrypt($imapPassword));
-			}
-
-			if ($input->getOption(self::ARGUMENT_SMTP_USER)) {
-				$mailAccount->setInboundUser($imapUser);
-			}
-
-			// OUTBOUND
-
-			if ($input->getOption(self::ARGUMENT_SMTP_HOST)) {
-				$mailAccount->setOutboundHost($smtpHost);
-			}
-
-			if ($input->getOption(self::ARGUMENT_SMTP_PORT)) {
-				$mailAccount->setOutboundPort((int) $smtpPort);
-			}
-
-			if ($input->getOption(self::ARGUMENT_SMTP_SSL_MODE)) {
-				$mailAccount->setOutboundSslMode($smtpSslMode);
-			}
-
-			if ($input->getOption(self::ARGUMENT_SMTP_PASSWORD)) {
-				$mailAccount->setOutboundPassword($this->crypto->encrypt($smtpPassword));
-			}
-			
-			if ($input->getOption(self::ARGUMENT_SMTP_USER)) {
-				$mailAccount->setOutboundUser($smtpUser);
-			}
-
-			$this->mapper->save($mailAccount);
-
-			$output->writeln("<info>Account $email for user $userId succesfully updated </info>");
 			return 1;
-		} else {
-			$output->writeln("<info>No Email Account $email found for user $userId </info>");
 		}
 
+		$output->writeLn("<info>Found account with email: " . $mailAccount->getEmail() . "</info>");
+
+		//ACCOUNT OPTIONS
+		if ($input->getOption(self::ARGUMENT_NAME)) {
+			$mailAccount->setName($name);
+		}
+		if ($input->getOption(self::ARGUMENT_EMAIL)) {
+			$mailAccount->setEmail($email);
+		}
+
+		//AUTH METHOD
+		if ($input->getOption(self::ARGUMENT_AUTH_METHOD)) {
+			$mailAccount->setAuthMethod($authMethod);
+		}
+
+		//INBOUND
+		if ($input->getOption(self::ARGUMENT_IMAP_HOST)) {
+			$mailAccount->setInboundHost($imapHost);
+		}
+
+		if ($input->getOption(self::ARGUMENT_IMAP_PORT)) {
+			$mailAccount->setInboundPort((int) $imapPort);
+		}
+
+		if ($input->getOption(self::ARGUMENT_IMAP_SSL_MODE)) {
+			$mailAccount->setInboundSslMode($imapSslMode);
+		}
+
+		if ($input->getOption(self::ARGUMENT_IMAP_PASSWORD)) {
+			$mailAccount->setInboundPassword($this->crypto->encrypt($imapPassword));
+		}
+
+		if ($input->getOption(self::ARGUMENT_SMTP_USER)) {
+			$mailAccount->setInboundUser($imapUser);
+		}
+
+		// OUTBOUND
+
+		if ($input->getOption(self::ARGUMENT_SMTP_HOST)) {
+			$mailAccount->setOutboundHost($smtpHost);
+		}
+
+		if ($input->getOption(self::ARGUMENT_SMTP_PORT)) {
+			$mailAccount->setOutboundPort((int) $smtpPort);
+		}
+
+		if ($input->getOption(self::ARGUMENT_SMTP_SSL_MODE)) {
+			$mailAccount->setOutboundSslMode($smtpSslMode);
+		}
+
+		if ($input->getOption(self::ARGUMENT_SMTP_PASSWORD)) {
+			$mailAccount->setOutboundPassword($this->crypto->encrypt($smtpPassword));
+		}
+
+		if ($input->getOption(self::ARGUMENT_SMTP_USER)) {
+			$mailAccount->setOutboundUser($smtpUser);
+		}
+
+		$this->mapper->save($mailAccount);
+
+		$output->writeln("<info>Account " . $mailAccount->getEmail() . " with ID  $accountId  succesfully updated </info>");
 		return 0;
 	}
 }

dd-sso/admin/src/admin/lib/admin.py

@@ -139,23 +139,32 @@ class Admin:
             res = res and tp.delete_user(user_id)
         return res
 
-    def _nextcloud_mail_set_cmd(self, user : DDUser, kw : Dict) -> Tuple[str, str]:
+    def _nextcloud_mail_set_cmd(self, user: DDUser, kw: Dict) -> str:
-        account_name = 'DD'  # Treating this as a constant
+        from shlex import quote as q
-        update_cmd = f"""mail:account:update \
-                --imap-host '{ kw['inbound_host'] }' --imap-port '{ kw['inbound_port'] }' --imap-ssl-mode '{ kw['inbound_ssl_mode'] }' \\
-                --imap-user '{ user['email'] }' --imap-password '{ user['password'] }' \\
-                --smtp-host '{ kw['outbound_host'] }' --smtp-port '{ kw['outbound_port'] }' --smtp-ssl-mode '{ kw['outbound_ssl_mode'] }' \\
-                --smtp-user '{ user['email'] }' --smtp-password '{ user['password'] }' \\
-                -- '{ user['user_id'] }' '{ user['email']}'"""
-        create_cmd = f"""mail:account:create '{ user['user_id'] }' '{ account_name }' '{ user['email'] }' \\
-                '{ kw['inbound_host'] }' '{ kw['inbound_port'] }' '{ kw['inbound_ssl_mode'] }' \\
-                '{ user['email'] }' '{ user['password'] }' \\
-                '{ kw['outbound_host'] }' '{ kw['outbound_port'] }' '{ kw['outbound_ssl_mode'] }' \\
-                '{ user['email'] }' '{ user['password'] }'"""
-        return (update_cmd, create_cmd)
 
-    def _nextcloud_mail_set_sh(self, users : List[DDUser], extra_data : Dict) -> str:
+        account_name = user.get("name", "DD User")
-        cmds = '\n'.join((f"./occ {u} || ./occ {c}" for u, c in (self._nextcloud_mail_set_cmd(u, extra_data) for u in users)))
+
+        nc_mail_update = "/nc-mail-update.sh"
+        # As defined in nc-mail-update.sh
+        unquoted_args = [
+            # User-specific
+            user["user_id"],
+            account_name,
+            user["email"],
+            user["password"],
+            # Server settings
+            kw.get("inbound_host", ""),
+            kw.get("inbound_port", ""),
+            kw.get("inbound_ssl_mode", ""),
+            kw.get("outbound_host", ""),
+            kw.get("outbound_port", ""),
+            kw.get("outbound_ssl_mode", ""),
+        ]
+        args = [q(str(a) if a else '') for a in unquoted_args]
+        return " ".join([nc_mail_update] + args)
+
+    def _nextcloud_mail_set_sh(self, users: List[DDUser], extra_data: Dict) -> str:
+        cmds = "\n".join((self._nextcloud_mail_set_cmd(u, extra_data) for u in users))
         return f"""#!/bin/sh -eu
 {cmds}
 """
@@ -170,10 +179,15 @@ class Admin:
         tmp = d.joinpath(fn + '.tmp')
         # Create executable file
         tmp.touch(mode=0o750)
-        # Write script
+        try:
-        tmp.write_text(self._nextcloud_mail_set_sh(users, extra_data))
+            # Write script
-        # Put it in-place
+            tmp.write_text(self._nextcloud_mail_set_sh(users, extra_data))
-        tmp.rename(sh)
+            # Put it in-place
+            tmp.rename(sh)
+        except:
+            log.error(traceback.format_exc())
+            log.error("Issue writing mail changes...")
+            raise
         return {}
 
     def check_connections(self, app : "AdminFlaskApp") -> None: