From d6e19352f99dc9be4b01809b47d5c6b935cec7bf Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Sat, 30 Mar 2019 18:58:41 +1100 Subject: [PATCH 1/2] Only throw an error when an IMAP command returns BAD, not NO Fixes bailing out when a STATUS returns BAD due to a mailbox not existing. --- src/engine/imap/command/imap-command.vala | 27 ++++++++++------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/src/engine/imap/command/imap-command.vala b/src/engine/imap/command/imap-command.vala index 76964b17..990b0fc9 100644 --- a/src/engine/imap/command/imap-command.vala +++ b/src/engine/imap/command/imap-command.vala @@ -243,9 +243,17 @@ public class Geary.Imap.Command : BaseObject { ); } + check_has_status(); + // Since this is part of the public API, perform a strict // check on the status code. - check_status(true); + if (this.status.status == Status.BAD) { + throw new ImapError.SERVER_ERROR( + "%s: Command failed: %s", + to_brief_string(), + this.status.to_string() + ); + } } public virtual string to_string() { @@ -276,9 +284,8 @@ public class Geary.Imap.Command : BaseObject { this.response_timer.reset(); this.complete_lock.blind_notify(); cancel_send(); - // Since this gets called by the client connection only check - // for an expected server response, good or bad - check_status(false); + + check_has_status(); } /** @@ -340,7 +347,7 @@ public class Geary.Imap.Command : BaseObject { } } - private void check_status(bool require_okay) throws ImapError { + private void check_has_status() throws ImapError { if (this.status == null) { throw new ImapError.SERVER_ERROR( "%s: No command response was received", @@ -355,16 +362,6 @@ public class Geary.Imap.Command : BaseObject { this.status.to_string() ); } - - // XXX should we be distinguishing between NO and BAD - // responses here? - if (require_okay && this.status.status != Status.OK) { - throw new ImapError.SERVER_ERROR( - "%s: Command failed: %s", - to_brief_string(), - this.status.to_string() - ); - } } private string to_brief_string() { From 71d8c8b1a4aee86cb358ef7ce80449e5b648759b Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Mon, 1 Apr 2019 02:01:04 +1100 Subject: [PATCH 2/2] Update Imap.ClientSession now IMAP NO commands aren't treated as errors Check the response code when logging in for the command, and handle as appropriate. --- .../imap/transport/imap-client-session.vala | 51 ++++++++++++------- test/integration/imap/client-session.vala | 6 +++ 2 files changed, 39 insertions(+), 18 deletions(-) diff --git a/src/engine/imap/transport/imap-client-session.vala b/src/engine/imap/transport/imap-client-session.vala index 0b62370f..d0d86baa 100644 --- a/src/engine/imap/transport/imap-client-session.vala +++ b/src/engine/imap/transport/imap-client-session.vala @@ -835,6 +835,10 @@ public class Geary.Imap.ClientSession : BaseObject { /** * Performs the LOGIN command using the supplied credentials. * + * Throws {@link ImapError.UNAUTHENTICATED} if the credentials are + * bad, unsupported, or if authentication actually failed. Returns + * the status response for the command otherwise. + * * @see initiate_session_async */ public async StatusResponse login_async(Geary.Credentials credentials, @@ -877,32 +881,40 @@ public class Geary.Imap.ClientSession : BaseObject { // should always proceed; only an Error could change this assert(params.proceed); - GLib.Error? login_err = null; - try { - yield command_transaction_async(cmd, cancellable); - } catch (Error err) { - login_err = err; - } + StatusResponse response = yield command_transaction_async( + cmd, cancellable + ); - if (login_err != null) { + if (response.status != Status.OK) { // Throw an error indicating auth failed here, unless // there is a status response and it indicates that the // server is merely reporting login as being unavailable, // then don't since the creds might actually be fine. - ResponseCodeType? code_type = null; - if (cmd.status != null) { - ResponseCode? code = cmd.status.response_code; - if (code != null) { - code_type = code.get_response_code_type(); + ResponseCode? code = response.response_code; + if (code != null) { + ResponseCodeType? code_type = code.get_response_code_type(); + if (code_type != null) { + switch (code_type.value) { + case ResponseCodeType.UNAVAILABLE: + throw new ImapError.UNAVAILABLE( + "Login restricted: %s: ", response.to_string() + ); + + case ResponseCodeType.AUTHENTICATIONFAILED: + // pass through to the error being thrown below + break; + + default: + throw new ImapError.SERVER_ERROR( + "Login error: %s: ", response.to_string() + ); + } } } - if (code_type == null || - code_type.value != ResponseCodeType.UNAVAILABLE) { - throw new ImapError.UNAUTHENTICATED(login_err.message); - } else { - throw login_err; - } + throw new ImapError.UNAUTHENTICATED( + "Bad credentials: %s: ", response.to_string() + ); } return cmd.status; @@ -1715,6 +1727,9 @@ public class Geary.Imap.ClientSession : BaseObject { this.cx.send_command(cmd); yield cmd.wait_until_complete(cancellable); + + // This won't be null since the Command.wait_until_complete + // will throw an error if it is. return cmd.status; } diff --git a/test/integration/imap/client-session.vala b/test/integration/imap/client-session.vala index 6cf5ec2e..957cbde6 100644 --- a/test/integration/imap/client-session.vala +++ b/test/integration/imap/client-session.vala @@ -62,6 +62,12 @@ class Integration.Imap.ClientSession : TestCase { assert_not_reached(); } catch (Geary.ImapError.UNAUTHENTICATED err) { // All good + } catch (Geary.ImapError.SERVER_ERROR err) { + // Some servers (Y!) return AUTHORIZATIONFAILED response + // code if the login (not password) is bad + if (!("AUTHORIZATIONFAILED" in err.message)) { + throw err; + } } }