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

Implements error handling for imap errors #11 #34

Merged
merged 4 commits into from
Jan 31, 2019

Conversation

anojht
Copy link
Contributor

@anojht anojht commented Jan 28, 2019

Prints IMAP errors and alerts to the nextcloud log. See #11
closes #11

@violoncelloCH
Copy link
Member

thank you for your contribution!

DCO is failing because the commit is not signed-off
@anojht could you please sign your commit? - see https://github.com/nextcloud/server/blob/master/.github/CONTRIBUTING.md#sign-your-work
can be done with git commit --amend -s for the last commit and then git push -f to push the changed commit back to github...

Prints IMAP errors and alerts to the nextcloud log.

Signed-off-by: Anojh Thayaparan <anojh@hotmail.com>
Signed-off-by: Anojh Thayaparan <anojh@hotmail.com>
@anojht
Copy link
Contributor Author

anojht commented Jan 28, 2019

@violoncelloCH the commits have been signed

@violoncelloCH violoncelloCH added enhancement New feature or request 3. to review labels Jan 28, 2019
@violoncelloCH
Copy link
Member

thank you @anojht
please review @nextcloud/user_external

@violoncelloCH violoncelloCH self-requested a review January 28, 2019 20:09
@cweiske
Copy link
Contributor

cweiske commented Jan 30, 2019

Shouldn't the log messages only be created if there are really errors or warnings? The code looks as if the log entries are always generated.

@violoncelloCH
Copy link
Member

good point @cweiske

Signed-off-by: Anojh Thayaparan <anojh@hotmail.com>
@anojht
Copy link
Contributor Author

anojht commented Jan 30, 2019

@violoncelloCH I have stored the errors and checked for empty list, before printing.

Copy link
Member

@violoncelloCH violoncelloCH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!
apart the syntax error it seems to work 👍 great improvement!
only downside: for failed logins we get a imap error additional to the warning from the core

{"reqId":"lAvu20WTmO8tZhPGEboF","level":3,"time":"2019-01-30T16:31:11+00:00","remoteAddr":"127.0.0.1","user":"--","app":"user_external","method":"POST","url":"\/nextcloud\/index.php\/login?user=testuser","message":"ERROR: IMAP Error: Array\n(\n    [0] => Can not authenticate to IMAP server: [AUTHENTICATIONFAILED] Authentication failed.\n)\n","userAgent":"Mozilla\/5.0 (X11; Ubuntu; Linux x86_64; rv:64.0) Gecko\/20100101 Firefox\/64.0","version":"16.0.0.0"}
{"reqId":"lAvu20WTmO8tZhPGEboF","level":2,"time":"2019-01-30T16:31:11+00:00","remoteAddr":"127.0.0.1","user":"--","app":"core","method":"POST","url":"\/nextcloud\/index.php\/login?user=testuser","message":"Login failed: 'testuser' (Remote IP: '127.0.0.1')","userAgent":"Mozilla\/5.0 (X11; Ubuntu; Linux x86_64; rv:64.0) Gecko\/20100101 Firefox\/64.0","version":"16.0.0.0"}

lib/imap.php Outdated Show resolved Hide resolved
Signed-off-by: Anojh Thayaparan <anojh@hotmail.com>
Copy link
Member

@violoncelloCH violoncelloCH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@violoncelloCH violoncelloCH changed the title Implements error handling #11 Implements error handling for imap errors #11 Jan 30, 2019
@violoncelloCH violoncelloCH added this to the 0.5.1 milestone Jan 30, 2019
@MorrisJobke
Copy link
Member

@violoncelloCH Feel free to merge this here. 1 review for apps is fine. Only for the server part we need 2 👍

@violoncelloCH
Copy link
Member

@MorrisJobke I just thought another review from one of you could be good, because I'm not that experienced yet... but I tested it once more and think it's fine, so merging it...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

user_external should log error message if authentication over imap does not work
4 participants