Skip to content

Commit 0bc0958

Browse files
kaellegokaellego
authored
Refactor IMAP class for type safety and logging
Refactor IMAP class properties and constructor for type safety. Update error handling and logging for IMAP authentication. Signed-off-by: Wellington Moraes <guiton.acre@gmail.com>
1 parent fbd6949 commit 0bc0958

1 file changed

Lines changed: 69 additions & 51 deletions

File tree

‎lib/IMAP.php‎

Lines changed: 69 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
<?php
2-
32
/**
43
* @author Robin Appelman <icewind@owncloud.com>
54
* @author Jonas Sulzer <jonas@violoncello.ch>
@@ -8,63 +7,72 @@
87
* later.
98
* See the COPYING-README file.
109
*/
10+
1111
namespace OCA\UserExternal;
1212

13+
use OCP\ILogger;
14+
1315
/**
1416
* User authentication against an IMAP mail server
15-
*
16-
* @category Apps
17-
* @package UserExternal
18-
* @author Robin Appelman <icewind@owncloud.com>
19-
* @license http://www.gnu.org/licenses/agpl AGPL
20-
* @link http://github.com/owncloud/apps
2117
*/
2218
class IMAP extends Base {
23-
private $mailbox;
24-
private $port;
25-
private $sslmode;
26-
private $domain;
27-
private $stripeDomain;
28-
private $groupDomain;
19+
private string $mailbox;
20+
private int $port;
21+
private ?string $sslmode;
22+
private string $domain;
23+
private bool $stripeDomain;
24+
private bool $groupDomain;
2925

3026
/**
31-
* Create new IMAP authentication provider
32-
*
3327
* @param string $mailbox IMAP server domain/IP
34-
* @param int $port IMAP server $port
35-
* @param string $sslmode
36-
* @param string $domain If provided, loging will be restricted to this domain
37-
* @param boolean $stripeDomain (whether to stripe the domain part from the username or not)
38-
* @param boolean $groupDomain (whether to add the usere to a group corresponding to the domain of the address)
28+
* @param int|null $port IMAP server port
29+
* @param string|null $sslmode ssl|tls|null
30+
* @param string|null $domain If provided, login will be restricted to this domain
31+
* @param bool $stripeDomain Whether to strip the domain part from the username
32+
* @param bool $groupDomain Whether to add the user to a group matching the email domain
3933
*/
40-
public function __construct($mailbox, $port = null, $sslmode = null, $domain = null, $stripeDomain = true, $groupDomain = false) {
34+
public function __construct(
35+
string $mailbox,
36+
?int $port = null,
37+
?string $sslmode = null,
38+
?string $domain = null,
39+
bool $stripeDomain = true,
40+
bool $groupDomain = false
41+
) {
4142
parent::__construct($mailbox);
4243
$this->mailbox = $mailbox;
43-
$this->port = $port === null ? 143 : $port;
44+
$this->port = $port ?? 143;
4445
$this->sslmode = $sslmode;
45-
$this->domain = $domain === null ? '' : $domain;
46+
$this->domain = $domain ?? '';
4647
$this->stripeDomain = $stripeDomain;
4748
$this->groupDomain = $groupDomain;
4849
}
4950

51+
private function logger(): ILogger {
52+
/** @var ILogger $logger */
53+
$logger = \OC::$server->get(ILogger::class);
54+
return $logger;
55+
}
56+
5057
/**
5158
* Check if the password is correct without logging in the user
5259
*
5360
* @param string $uid The username
5461
* @param string $password The password
55-
*
56-
* @return true/false
62+
* @return string|false
5763
*/
5864
public function checkPassword($uid, $password) {
59-
$uid = $this->resolveUid($uid);
65+
if ($password === '') {
66+
return false;
67+
}
6068

6169
// Replace escaped @ symbol in uid (which is a mail address)
62-
// but only if there is no @ symbol and if there is a %40 inside the uid
63-
if (!(strpos($uid, '@') !== false) && (strpos($uid, '%40') !== false)) {
70+
if (strpos($uid, '@') === false && strpos($uid, '%40') !== false) {
6471
$uid = str_replace('%40', '@', $uid);
6572
}
6673

6774
$pieces = explode('@', $uid);
75+
6876
if ($this->domain !== '') {
6977
if (count($pieces) === 1) {
7078
$username = $uid . '@' . $this->domain;
@@ -74,8 +82,8 @@ public function checkPassword($uid, $password) {
7482
$uid = $pieces[0];
7583
}
7684
} else {
77-
$this->logger->error(
78-
'ERROR: User has a wrong domain! Expecting: ' . $this->domain,
85+
$this->logger()->error(
86+
'User has a wrong domain. Expected: ' . $this->domain,
7987
['app' => 'user_external']
8088
);
8189
return false;
@@ -85,20 +93,31 @@ public function checkPassword($uid, $password) {
8593
}
8694

8795
$groups = [];
88-
if ((count($pieces) > 1) && $this->groupDomain && $pieces[1]) {
96+
if (count($pieces) > 1 && $this->groupDomain && !empty($pieces[1])) {
8997
$groups[] = $pieces[1];
9098
}
9199

92100
$protocol = ($this->sslmode === 'ssl') ? 'imaps' : 'imap';
93-
$url = "{$protocol}://{$this->mailbox}:{$this->port}";
101+
$url = sprintf('%s://%s:%d', $protocol, $this->mailbox, $this->port);
102+
94103
$ch = curl_init();
104+
if ($ch === false) {
105+
$this->logger()->error(
106+
'Could not initialize curl for IMAP authentication.',
107+
['app' => 'user_external']
108+
);
109+
return false;
110+
}
111+
95112
if ($this->sslmode === 'tls') {
96113
curl_setopt($ch, CURLOPT_USE_SSL, CURLUSESSL_ALL);
97114
}
115+
98116
curl_setopt($ch, CURLOPT_URL, $url);
99117
curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
100118
curl_setopt($ch, CURLOPT_USERPWD, $username . ':' . $password);
101119
curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, 10);
120+
curl_setopt($ch, CURLOPT_TIMEOUT, 15);
102121
curl_setopt($ch, CURLOPT_CUSTOMREQUEST, 'CAPABILITY');
103122

104123
curl_exec($ch);
@@ -109,35 +128,34 @@ public function checkPassword($uid, $password) {
109128
$uid = mb_strtolower($uid);
110129
$this->storeUser($uid, $groups);
111130
return $uid;
112-
} elseif ($errorcode === CURLE_COULDNT_CONNECT
113-
|| $errorcode === CURLE_SSL_CONNECT_ERROR
114-
|| $errorcode === 28) {
115-
# This is not defined in PHP-8.x
116-
# 28: CURLE_OPERATION_TIMEDOUT
117-
$this->logger->error(
118-
'ERROR: Could not connect to imap server via curl: ' . curl_strerror($errorcode),
131+
}
132+
133+
if (
134+
$errorcode === CURLE_COULDNT_CONNECT ||
135+
$errorcode === CURLE_SSL_CONNECT_ERROR ||
136+
$errorcode === CURLE_OPERATION_TIMEDOUT
137+
) {
138+
$this->logger()->error(
139+
'Could not connect to IMAP server via curl: ' . curl_strerror($errorcode),
119140
['app' => 'user_external']
120141
);
121-
} elseif ($errorcode === 9
122-
|| $errorcode === 67
123-
|| $errorcode === 94) {
124-
# These are not defined in PHP-8.x
125-
# 9: CURLE_REMOTE_ACCESS_DENIED
126-
# 67: CURLE_LOGIN_DENIED
127-
# 94: CURLE_AUTH_ERROR)
128-
$this->logger->error(
129-
'ERROR: IMAP Login failed via curl: ' . curl_strerror($errorcode),
142+
} elseif (
143+
$errorcode === CURLE_REMOTE_ACCESS_DENIED ||
144+
$errorcode === CURLE_LOGIN_DENIED ||
145+
(defined('CURLE_AUTH_ERROR') && $errorcode === CURLE_AUTH_ERROR)
146+
) {
147+
$this->logger()->warning(
148+
'IMAP login failed via curl: ' . curl_strerror($errorcode),
130149
['app' => 'user_external']
131150
);
132151
} else {
133-
$this->logger->error(
134-
'ERROR: IMAP server returned an error: ' . $errorcode . ' / ' . curl_strerror($errorcode),
152+
$this->logger()->error(
153+
'IMAP server returned an error: ' . $errorcode . ' / ' . curl_strerror($errorcode),
135154
['app' => 'user_external']
136155
);
137156
}
138157

139158
curl_close($ch);
140-
141159
return false;
142160
}
143161
}

0 commit comments

Comments
 (0)