Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace imap_rcube library with curl call #122

Merged
merged 1 commit into from
Feb 19, 2020

Conversation

rollbrettler
Copy link

@rollbrettler rollbrettler commented Jan 31, 2020

This commit removes the imap_rcube library with a curl call to the imap endpoint.
Since curl is a dependency of nextcloud this should not cause any issues.

This fixes #116
fixes #59
fixes #97
and fixes #105

@rollbrettler
Copy link
Author

@violoncelloCH I understand that this is a quite an intrusive PR let me now what you think.

@violoncelloCH
Copy link
Member

hi @rollbrettler
first of all: thank you very much for your PR!
this seems to be a really interesting approach... as php-curl seems to be required for nextcloud anyway, this should indeed be working for everyone. However we need to be really sure with this to not break anything. I'm not 100% sure, if nextcloud doesn't run at all if the module isn't present or if it could still be the case that some use nextcloud e.g. on a shared hosting where php-curl isn't present... So if you could check this, that would be great!
I really like the approach for its cleanliness... But I'm at the same time not sure if IMAP is not too complex with the different authentication methods etc. Sadly I'm not an IMAP expert 🙃 .
I'll try to find some time during the next days to do some testing and maybe we'll also find some volunteers (hopefully from #116 and also #59 #97) to test this...

@violoncelloCH violoncelloCH self-requested a review February 3, 2020 21:44
@violoncelloCH violoncelloCH added 3. to review bug Something isn't working enhancement New feature or request labels Feb 3, 2020
@violoncelloCH violoncelloCH added this to the 0.9 milestone Feb 3, 2020
@violoncelloCH
Copy link
Member

looks very promising and worked flawlessly in a first test... it even seems to be quicker with this than before... 👍

@violoncelloCH
Copy link
Member

violoncelloCH commented Feb 4, 2020

So to make this as easy as possible for potential testers, I've packaged a release with the changes from this pull-request: user_external-0.8.1-dev2.tar.gz user_external-0.8.1-dev.tar.gz
To apply it, you first need to delete (or rename) the current folder user_external/ in the apps/ directory inside your NC installation and then extract this archive into apps/. (So you'll have a new user_external folder with this version from the PR here.)
If you have any questions don't hesitate to ask!
And last but not least: Please give feedback if this works! :)

@ghost
Copy link

ghost commented Feb 4, 2020

After extracting you need to activate "External user Authentication 0.8.1-dev" under https://your.cloud/index.php/settings/apps/disabled

I can confirm, that I could just login via imap auth with that version on a local NC 18.0.0 (debian stretch, apache2, mariadb 15.1)

@Mannshoch
Copy link

[PHP] Error: Undefined variable: rcube at /nextcloud/apps/user_external/lib/imap.php#106

POST /login
from XXX.XXX.XXX.XXX at 2020-02-05T08:06:17+01:00

[PHP] Error: Trying to get property 'error' of non-object at /nextcloud/apps/user_external/lib/imap.php#106

POST /login
from XXX.XXX.XXX.XXX at 2020-02-05T08:06:17+01:00

[user_external] Error: ERROR: Could not connect via roundcube lib:

POST /login
from XXX.XXX.XXX.XXX at 2020-02-05T08:06:17+01:00

@kevo-gt
Copy link

kevo-gt commented Feb 5, 2020

I have a similar error to #122 (comment)

@rollbrettler
Copy link
Author

@Mannshoch @kevo-gt thanks for reporting, I completely missed the error case. I assume it would still not work for you though. Since there is still an error with connecting to your imap server, but maybe the error is now more descriptive.

@benbrummer
Copy link

Resolves #59 for me, perfect!

@Mannshoch
Copy link

@violoncelloCH
Copy link
Member

here is a new archive with the updates: user_external-0.8.1-dev2.tar.gz
thanks everyone for testing and reporting and thanks @rollbrettler for implementing those changes!

@kevo-gt
Copy link

kevo-gt commented Feb 6, 2020

here is a new archive with the updates: user_external-0.8.1-dev2.tar.gz
thanks everyone for testing and reporting and thanks @rollbrettler for implementing those changes!

Thanks. I tried it and get the below error:

"message":"curl_error(): supplied resource is not a valid cURL handle resource at \/usr\/local\/www\/nextcloud\/apps\/user_external\/lib\/imap.php#106","userAgent":"","app":"user_external","

method":"POST","url":"\/index.php\/login","message":"ERROR: Could not connect to imap server via curl: "

I have curl and php73-curl installed and the OS is FreeBSD 12.0

@rollbrettler
Copy link
Author

@kevo-gt I see. Thanks for reporting. I think I fixed it. Iam not a php expert though.
@Mannshoch I dont know how @violoncelloCH creates the archives but I tested it myself by cloning the repo in the apps directory git clone https://github.com/rollbrettler/user_external.git. Also https://github.com/rollbrettler/user_external/archive/master.tar.gz should work in the meantime.

@kevo-gt
Copy link

kevo-gt commented Feb 6, 2020

@kevo-gt I see. Thanks for reporting. I think I fixed it. Iam not a php expert though.
@Mannshoch I dont know how @violoncelloCH creates the archives but I tested it myself by cloning the repo in the apps directory git clone https://github.com/rollbrettler/user_external.git. Also https://github.com/rollbrettler/user_external/archive/master.tar.gz should work in the meantime.

This has now worked, thanks very much! It is quite fast logging in as well

@Mannshoch
Copy link

Mannshoch commented Feb 6, 2020

With https://github.com/rollbrettler/user_external/archive/master.tar.gz
I get the Error:

[user_external] Error: ERROR: Could not connect to imap server via curl: 

POST /login
from XXX.XXX.XXX.XXX at 2020-02-06T10:34:26+01:00

Curl is working if I log into my webspace and run curl inside it:
`curl imap://localhost:143 --user 'user@domainname.net:password'

Please check my config.php:

user_backends' => 
  array (
    0 => 
    array (
      'class' => 'OC_User_IMAP',
      'arguments' => 
      array (
        0 => 'localhost', //imap server
        1 => 143, // PORT
        2 => null,  // With SSL or null
        3 => 'domainname.net',  // allowed domain for mail addresses
        4 => false, // username without domain? (true, false)
        5 => false, // move new user to Group with name of the mail domainname (true, false)
      ),
    ),
  ),

@rollbrettler
Copy link
Author

@Mannshoch according to your comment here #97 (comment) your setting before was not validating the certificate novalidate-cert. I created a branch to disable that with my changes. Are you able to try if that works for you? Otherwise I guess its related to the imap server. Maybe you are able to share some more configuration specifics about that server?

https://github.com/rollbrettler/user_external/archive/disable-certificate-check.tar.gz

@Mannshoch
Copy link

@rollbrettler SSL do not work over ssh If I exchange imap://... with imaps://... I get this error.
curl: (35) SSL received a record that exceeded the maximum permissible length.
(setting in Thunderbird is STARTTLS and encrypted Password)

I'll test the update as fast as possible.

@Mannshoch
Copy link

Was able to do the test earlier than expected.
I get the same Error:

[user_external] Error: ERROR: Could not connect to imap server via curl: 

POST /login

Could It may be a problem of the special characters in my E-Mail password?
If I use Curl in bash I get a strange behavior if I use " instead of ' May it affect you with PHP also in a way?

@rollbrettler
Copy link
Author

imaps:// should be a specific port (993 by default https://en.wikipedia.org/wiki/Internet_Message_Access_Protocol#Security).

Bash indeed treats " and ' differently. You can run curl also without the password curl imap://localhost:143 --user 'user@domainname.net' so you have to type it in as a hidden password. Still that does not explain why the error is empty

Error: ERROR: Could not connect to imap server via curl:

Any chance you can share some more information about the env?
Are you able to use curl imaps://domainname.net --user 'user@domainname.net' or whatever imap server you set in thunderbird?
Are you able to use the imap domain as well in config.php?

@Mannshoch

This comment has been minimized.

@rollbrettler
Copy link
Author

@violoncelloCH will you squash and merge or shall I squash the commits? Otherwise there will be quite some noise in the commit history.

@Mannshoch
Copy link

Thanks a lot @rollbrettler and @violoncelloCH for your work.
I see the time comming to update it in Nextcloud 15 and be able then to update this nextcloud to Nextcloud 18.

@violoncelloCH
Copy link
Member

@rollbrettler I would be glad if you could squash the commits :)

This commit removes the imap_rcube library with a curl call to the imap endpoint.
Since curl is a dependency of nextcloud this should not cause any issues.

This closes nextcloud#116

Signed-off-by: Tim Wichmann <tim@wichmann.online>
@rollbrettler
Copy link
Author

Commits are squashed

@violoncelloCH violoncelloCH merged commit e8ba4de into nextcloud:master Feb 19, 2020
@DJCrashdummy
Copy link
Contributor

just a question about the "migration path" to include it into #126:
is there an automatic migration to rewrite 'ssl' and 'tls' to true and null to false, or does it need a manual operation? or does the config.php (at least for now) stay like it is and are the "old values" are considered by the app itself as their needed resp. then "recommended" pendants?

@violoncelloCH
Copy link
Member

violoncelloCH commented Feb 26, 2020

@DJCrashdummy In fact there is no automatic migration, but any value is considered as true while an empty string/value is considered as false; so this should(TM) work out of the box without a need to adapt the config.
Of course if anyone is up for writing such a migration step this would be highly appreciated :)

@DJCrashdummy
Copy link
Contributor

so let's hope that nobody has accidentally used 'null' instead of null... or would this alredy with version <0.9 have thrown an error?

@violoncelloCH
Copy link
Member

this should have never worked because you would have passed an invalid string into the rcube lib...

DJCrashdummy added a commit to DJCrashdummy/user_external that referenced this pull request Feb 29, 2020
- parameter number `2` (the 3rd one): adapted options according to nextcloud#126 (comment) to fit nextcloud#122
- parameter number `3` (the 4th one): added the second possibility to allow all domains according to nextcloud#126 (comment)

Signed-off-by: DJCrashdummy <DJCrashdummy@users.noreply.github.com>
@Mannshoch
Copy link

Mannshoch commented Apr 2, 2020

When could the update be released?

@violoncelloCH
Copy link
Member

as soon as possible 🙃 ... this should have already been done, I see... will try to do it very soon :)

@violoncelloCH violoncelloCH mentioned this pull request Apr 2, 2020
2 tasks
@violoncelloCH
Copy link
Member

here it comes: #133

@Mannshoch
Copy link

Mannshoch commented Apr 11, 2020

Yea. Thanks a lot. Now I was able to upgrade from 15 -> 16 -> 17 and everything is working. 👍
Only Problem is the auto logout on the website I do not know how to deactivate.

@violoncelloCH
Copy link
Member

Cool! Nice to hear...
What do you mean by auto logout? (but that's off topic in here, so maybe better in a new issue)...

@Mannshoch

This comment has been minimized.

@Mannshoch

This comment has been minimized.

@Mannshoch

This comment has been minimized.

@violoncelloCH

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release bug Something isn't working enhancement New feature or request
Projects
None yet
6 participants