Refactor IMAP class for type safety and logging

lib/IMAP.php

@@ -1,5 +1,4 @@
 <?php
-
 /**
  * @author Robin Appelman <icewind@owncloud.com>
  * @author Jonas Sulzer <jonas@violoncello.ch>
@@ -8,63 +7,72 @@
  * later.
  * See the COPYING-README file.
  */
+
 namespace OCA\UserExternal;
 
+use OCP\ILogger;
+
 /**
  * User authentication against an IMAP mail server
- *
- * @category Apps
- * @package  UserExternal
- * @author   Robin Appelman <icewind@owncloud.com>
- * @license  http://www.gnu.org/licenses/agpl AGPL
- * @link     http://github.com/owncloud/apps
  */
 class IMAP extends Base {
-	private $mailbox;
-	private $port;
-	private $sslmode;
-	private $domain;
-	private $stripeDomain;
-	private $groupDomain;
+	private string $mailbox;
+	private int $port;
+	private ?string $sslmode;
+	private string $domain;
+	private bool $stripeDomain;
+	private bool $groupDomain;
 
 	/**
-	 * Create new IMAP authentication provider
-	 *
 	 * @param string $mailbox IMAP server domain/IP
-	 * @param int $port IMAP server $port
-	 * @param string $sslmode
-	 * @param string $domain If provided, loging will be restricted to this domain
-	 * @param boolean $stripeDomain (whether to stripe the domain part from the username or not)
-	 * @param boolean $groupDomain (whether to add the usere to a group corresponding to the domain of the address)
+	 * @param int|null $port IMAP server port
+	 * @param string|null $sslmode ssl|tls|null
+	 * @param string|null $domain If provided, login will be restricted to this domain
+	 * @param bool $stripeDomain Whether to strip the domain part from the username
+	 * @param bool $groupDomain Whether to add the user to a group matching the email domain
 	 */
-	public function __construct($mailbox, $port = null, $sslmode = null, $domain = null, $stripeDomain = true, $groupDomain = false) {
+	public function __construct(
+		string $mailbox,
+		?int $port = null,
+		?string $sslmode = null,
+		?string $domain = null,
+		bool $stripeDomain = true,
+		bool $groupDomain = false
+	) {
 		parent::__construct($mailbox);
 		$this->mailbox = $mailbox;
-		$this->port = $port === null ? 143 : $port;
+		$this->port = $port ?? 143;
 		$this->sslmode = $sslmode;
-		$this->domain = $domain === null ? '' : $domain;
+		$this->domain = $domain ?? '';
 		$this->stripeDomain = $stripeDomain;
 		$this->groupDomain = $groupDomain;
 	}
 
+	private function logger(): ILogger {
+		/** @var ILogger $logger */
+		$logger = \OC::$server->get(ILogger::class);
+		return $logger;
+	}
+
 	/**
 	 * Check if the password is correct without logging in the user
 	 *
 	 * @param string $uid The username
 	 * @param string $password The password
-	 *
-	 * @return true/false
+	 * @return string|false
 	 */
 	public function checkPassword($uid, $password) {
-		$uid = $this->resolveUid($uid);
+		if ($password === '') {
+			return false;
+		}
 
 		// Replace escaped @ symbol in uid (which is a mail address)
-		// but only if there is no @ symbol and if there is a %40 inside the uid
-		if (!(strpos($uid, '@') !== false) && (strpos($uid, '%40') !== false)) {
+		if (strpos($uid, '@') === false && strpos($uid, '%40') !== false) {
 			$uid = str_replace('%40', '@', $uid);
 		}
 
 		$pieces = explode('@', $uid);
+
 		if ($this->domain !== '') {
 			if (count($pieces) === 1) {
 				$username = $uid . '@' . $this->domain;
@@ -74,8 +82,8 @@ public function checkPassword($uid, $password) {
 					$uid = $pieces[0];
 				}
 			} else {
-				$this->logger->error(
-					'ERROR: User has a wrong domain! Expecting: ' . $this->domain,
+				$this->logger()->error(
+					'User has a wrong domain. Expected: ' . $this->domain,
 					['app' => 'user_external']
 				);
 				return false;
@@ -85,20 +93,31 @@ public function checkPassword($uid, $password) {
 		}
 
 		$groups = [];
-		if ((count($pieces) > 1) && $this->groupDomain && $pieces[1]) {
+		if (count($pieces) > 1 && $this->groupDomain && !empty($pieces[1])) {
 			$groups[] = $pieces[1];
 		}
 
 		$protocol = ($this->sslmode === 'ssl') ? 'imaps' : 'imap';
-		$url = "{$protocol}://{$this->mailbox}:{$this->port}";
+		$url = sprintf('%s://%s:%d', $protocol, $this->mailbox, $this->port);
+
 		$ch = curl_init();
+		if ($ch === false) {
+			$this->logger()->error(
+				'Could not initialize curl for IMAP authentication.',
+				['app' => 'user_external']
+			);
+			return false;
+		}
+
 		if ($this->sslmode === 'tls') {
 			curl_setopt($ch, CURLOPT_USE_SSL, CURLUSESSL_ALL);
 		}
+
 		curl_setopt($ch, CURLOPT_URL, $url);
 		curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
 		curl_setopt($ch, CURLOPT_USERPWD, $username . ':' . $password);
 		curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, 10);
+		curl_setopt($ch, CURLOPT_TIMEOUT, 15);
 		curl_setopt($ch, CURLOPT_CUSTOMREQUEST, 'CAPABILITY');
 
 		curl_exec($ch);
@@ -109,35 +128,34 @@ public function checkPassword($uid, $password) {
 			$uid = mb_strtolower($uid);
 			$this->storeUser($uid, $groups);
 			return $uid;
-		} elseif ($errorcode === CURLE_COULDNT_CONNECT
-			   || $errorcode === CURLE_SSL_CONNECT_ERROR
-			   || $errorcode === 28) {
-			# This is not defined in PHP-8.x
-			# 28: CURLE_OPERATION_TIMEDOUT
-			$this->logger->error(
-				'ERROR: Could not connect to imap server via curl: ' . curl_strerror($errorcode),
+		}
+
+		if (
+			$errorcode === CURLE_COULDNT_CONNECT ||
+			$errorcode === CURLE_SSL_CONNECT_ERROR ||
+			$errorcode === CURLE_OPERATION_TIMEDOUT
+		) {
+			$this->logger()->error(
+				'Could not connect to IMAP server via curl: ' . curl_strerror($errorcode),
 				['app' => 'user_external']
 			);
-		} elseif ($errorcode === 9
-			   || $errorcode === 67
-			   || $errorcode === 94) {
-			# These are not defined in PHP-8.x
-			# 9: CURLE_REMOTE_ACCESS_DENIED
-			# 67: CURLE_LOGIN_DENIED
-			# 94: CURLE_AUTH_ERROR)
-			$this->logger->error(
-				'ERROR: IMAP Login failed via curl: ' . curl_strerror($errorcode),
+		} elseif (
+			$errorcode === CURLE_REMOTE_ACCESS_DENIED ||
+			$errorcode === CURLE_LOGIN_DENIED ||
+			(defined('CURLE_AUTH_ERROR') && $errorcode === CURLE_AUTH_ERROR)
+		) {
+			$this->logger()->warning(
+				'IMAP login failed via curl: ' . curl_strerror($errorcode),
 				['app' => 'user_external']
 			);
 		} else {
-			$this->logger->error(
-				'ERROR: IMAP server returned an error: ' . $errorcode . ' / ' . curl_strerror($errorcode),
+			$this->logger()->error(
+				'IMAP server returned an error: ' . $errorcode . ' / ' . curl_strerror($errorcode),
 				['app' => 'user_external']
 			);
 		}
 
 		curl_close($ch);
-
 		return false;
 	}
 }