nextcloud / user_external

👥 External user authentication methods like IMAP, SMB and FTP
https://apps.nextcloud.com/apps/user_external
108 stars 64 forks source link

Undefined Indices in Logs #56

Closed MaxiBoether closed 5 years ago

MaxiBoether commented 5 years ago

Steps to reproduce

  1. Install and enable user_external plugin
  2. Open live logging https://**/index.php/settings/admin/logging

Expected behaviour

The debug output shouldn't spam the logs.

Actual behaviour

I get like almost every second one of these two errors:

Undefined offset: 1 at /var/www/cloud/public/apps/user_external/lib/imap/imap_rcube.php#149

Undefined index: force_caps at /var/www/cloud/public/apps/user_external/lib/imap/imap_rcube.php#945

Affected Authentication backend

IMAP

violoncelloCH commented 5 years ago

thank you for reporting this! it's strange, because I don't get this in my logs... also I thought, log output from roundcube couldn't go into the nextcloud log? Or is this because these are "bigger" issues remarked by php and they therefore go into the nextcloud log? any idea @ChristophWurst ?

@MaxiBoether & @Flachzange could you provide some details about your configurations? Maybe it depends on the php environment or something specific for this to show up in the logs...

Wuk-jvi commented 5 years ago

I get these errors on my setup:

User External App version: 0.6

Operating system: CentOS 7

Web server: nginx

PHP version: 7.1.8

Nextcloud version: 15.0.5.3

To remove errors from error log I have added isset for $parts to line 149, and deleted if block at line 945 because it seams that force_caps is used only in that block and nowhere else. After that it there are no errors. Although it seams that everything is working without problems someone who knows more about this should check and try to find appropriate solution.

Diff of my changes:

--- lib/imap/imap_rcube.php.orig        2019-03-23 16:49:31.222071537 +0100
+++ lib/imap/imap_rcube.php     2019-03-23 17:00:40.914903310 +0100
@@ -146,7 +146,7 @@
         $res = 0;
         if ($parts = preg_split('/(\{[0-9]+\}\r\n)/m', $string, -1, PREG_SPLIT_DELIM_CAPTURE)) {
             for ($i=0, $cnt=count($parts); $i<$cnt; $i++) {
-                if (preg_match('/^\{([0-9]+)\}\r\n$/', $parts[$i+1], $matches)) {
+                if (isset($parts[$i+1]) && preg_match('/^\{([0-9]+)\}\r\n$/', $parts[$i+1], $matches)) {
                     // LITERAL+ support
                     if ($this->prefs['literal+']) {
                         $parts[$i+1] = sprintf("{%d+}\r\n", $matches[1]);
@@ -942,9 +942,6 @@

         // Connected and authenticated
         if (is_resource($result)) {
-            if ($this->prefs['force_caps']) {
-                $this->clearCapability();
-            }
             $this->logged = true;

             return true;
MaxiBoether commented 5 years ago

I'm running Ubuntu Server 18.04 LTS, apache/2.4.29 and php 7.2 via fastcgi. The NextCloud & User_External versions are up to date as well.

@Wuk-jvi's solution seems to be possible, but I don't really a) know why we are using RoundCube sourcecode (I am running RoundCube as a webmail interface and it's not spamming my logs) here and b) if it's a viable solution because I don't see what the Regex and stuff like force_caps is doing.

0x47 commented 5 years ago

Same issue on up-to-date Debian 9, PHP 7.0.33-0+deb9u3, nginx/1.10.3, spamming logs in sub-second intervals.

ChristophWurst commented 5 years ago

also I thought, log output from roundcube couldn't go into the nextcloud log? Or is this because these are "bigger" issues remarked by php and they therefore go into the nextcloud log? any idea @ChristophWurst ?

If php throws a warning or error, the global Nextcloud error handler will capture and log it. Try lowering your log level in config.php to have Nextcloud log more.

fskale commented 5 years ago

@Wuk-jvi patch works. But there's also another problem in the config when using version 6.0.1. IMHO only copy and paste the imap_rcube_generic IMAP framework won't work when not using the roundcube config and prefs semantics. e.g: /etc/roundcubemail/defaults.inc.php:$config['imap_force_caps'] = false; So, removing the prefs part would make sense. Also, the ssl_mode config option should be documented. Reading the whole library, the ssl_mode must not use an empty string but notls Also the config changes should be outlined Version 6.0.1 doesn't work out of the box. My additional patch part for the ssl part:

@@ -985,7 +984,7 @@ class imap_rcube
             // set connection identifier for debug output
             $this->resourceid = strtoupper(substr(md5(microtime() . $host . $this->user), 0, 4));

-            $_host = ($this->prefs['ssl_mode'] === 'tls' ? 'tls://' : '') . $host . ':' . $this->prefs['port'];
+            $_host = ($this->prefs['ssl_mode'] === 'tls' ? 'tls://' : 'notls') . $host . ':' . $this->prefs['port'];

My config:

'user_backends' =>
  array (
    0 =>
    array (
      'class' => 'OC_User_IMAP',
      'arguments' =>
      array (
        0 => 'imap.example.com',
        1 => 993,
        2 => 'ssl',
        3 => '',
      ),
    ),
  ),

Now i have no warnings and the authentication works. It didn't , using the plugin out of the box !

Best regards Franz

violoncelloCH commented 5 years ago

thank you @fskale could you create a PR for this? (also for the ssl_mode documentation?) this would really help! sadly I don't have enough time currently... (I'm just maintaining this app in my spare time and lots of things to do for school currently)... However I would be very happy to review (and hopefully merge) PRs...

fskale commented 5 years ago

thank you @fskale could you create a PR for this? (also for the ssl_mode documentation?) this would really help! sadly I don't have enough time currently... (I'm just maintaining this app in my spare time and lots of things to do for school currently)... However I would be very happy to review (and hopefully merge) PRs...

Hi violoncelloCH , pretty busy too, 3 kids and a lot of work ;-) Anyhow, before submitting a PR i only wanna check the source for decribing the changes i've made as well as a Documentantion how to use it. Trial and error is not mi kinda thing when it comes to releases not mentioning mantatory options ;-) I hope i will provide an PR by the end of this week. Best regards Franz

Flachzange commented 5 years ago

Can we please have a permanent fix for this. I do not want to patch the file with every update in the future.

violoncelloCH commented 5 years ago

@fskale how does it look like; did you find some time?

fskale commented 5 years ago

Hi, i have time at the weekend for extensive testing. Will provide updates on monday ! Rgds. Franz

sshambar commented 5 years ago

Since no one has submitted a patch yet, I added one to fix the warnings in a slightly different way by creating the missing connection parameters (ssl_mode & force_caps).... I also included the isset() check in putLineC to handle authenticates that don't use the {#} escaped format too (ie AUTHENTICATE PLAIN), not sure why warnings are created in roundcubemail though :)

ychaouche commented 5 years ago

It's not created in roundcubemail, the nextcloud project is using an imap library that is borrowed from the roundcube project.

The real question I have is why in line 149 there is a check for parts[$i+1] when you don't seem to have any tests to check that parts is at least 2 elements long ? dumping the content of that array in a temporary file I could see that in a period of 5 minutes the array had a cardinality of 1 every time. The error about indice 1 isn't being found in the parts array is really annoying.

@Wuk-jvi's isset() is working of course but that's just a hack and doesn't help understand what the code should really look like (why do we even look for parts[$i+1] and not parts[$i] for example)

realizelol commented 5 years ago

line 149 in "nextcloudinstallfolder"/apps/user_external/lib/imap/imap_rcube.php:

-if (preg_match('/^\{([0-9]+)\}\r\n$/', $parts[$i+1], $matches)) {
+if (preg_match('/^\{([0-9]+)\}\r\n$/', $parts[$i], $matches)) {

I don't really actually know what this command is doing, but if we remove the "skip 1" for $parts array the error is gone. line 863 in "nextcloudinstallfolder"/apps/user_external/lib/imap/imap_rcube.php:

$this->selected = null;
+ $this->prefs['force_caps'] = false;

just define this variable in this function "connect".

line 81 - 83 in "nextcloudinstallfolder"/apps/user_external/lib/imap.php:

-if ($this->groupDomain && $pieces[1]) {
-       $groups[] = $pieces[1];
-}
+if ($this->groupDomain && $pieces[1]) {
+       $groups[] = $pieces[1];
+}  else { $groups = 0; }

If there notdefined/empty/false groupDomain for the domain.tld ($pieces[1]) then $groups = 0.

and please use the full nextcloud config.php part which is documented in the README.MD:

  'user_backends' =>
  array (
    0 =>
    array (
      'class' => 'OC_User_IMAP',
      'arguments' =>
      array (
        0 => 'imap.example.com', // FQDN to your mailserver
        1 => 993, // (def. 143 or 933) Port to your mailserver
        2 => 'ssl', // (ssl, tls, null) Encryption to your mailserver
        3 => '', // if you only use one domain enter 'example.com' otherwise leave it empty!
        4 => false, // true if you login with yourusername instead of yourusername@example.com
        5 => false, // if you put these users in a special group?
      ),
    ),
  ),

And all your messages should be gone!

I haven't had any issues with https://github.com/roundcube/roundcubemail/blob/master/program/lib/Roundcube/rcube_imap_generic.php#L986 I dont understand why to do sth. like this:

$_host = ($this->prefs['ssl_mode'] === 'tls' ? 'tls://' : 'notls') . $host . ':' . $this->prefs['port'];

This only will set $_host to "tls://example.com" or "notlsexample.com" if you set tls as your encryption in "nextcloud config.php" - was it just a try to fix these error messages?

Best regards realizelol

ychaouche commented 5 years ago

That is not how I configured my nextcloud external users, and maybe this is the reason why I got those errors :

  'user_backends' =>
  array (
    0 =>
    array (
      'class' => 'OC_User_IMAP',
      'arguments' =>
      array (
        0 => 'my.host.tld',
      ),
    ),
  ),

This is how it used to be (back in owncloud days)

  'user_backends' =>
  array (
    0 =>
    array (
      'class' => 'OC_User_IMAP',
      'arguments' =>
      array (
        0 => '{messagerie.algerian-radio.dz:143/imap/novalidate-cert}',
      ),
    ),
  ),

And this is how it is documented in the config.sample.php that came with nextcloud ($NEXTCLOUDSERVERSRC/config/config.sample.php):

'user_backends' => array(
        array(
                'class' => 'OC_User_IMAP',
                'arguments' => array('{imap.gmail.com:993/imap/ssl}INBOX')
        )
),

I can't find this sample in the recent nextcloud-server/config/config.sample.php file, but it was there in 15.0.6 (see : https://github.com/nextcloud/server/blob/v15.0.6/config/config.sample.php#L285)

violoncelloCH commented 5 years ago

fix is in #83 and will be in the next release a big thanks to @sshambar for the contribution