[geary/wip/17-noisy-problem-reports: 2/12] Remove auth failure prompting from the engine
- From: Michael Gratton <mjog src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [geary/wip/17-noisy-problem-reports: 2/12] Remove auth failure prompting from the engine
- Date: Tue, 1 Jan 2019 11:29:53 +0000 (UTC)
commit a637a6b63afedfa7e2527f09fbcb6b3176d280cc
Author: Michael Gratton <mike vee net>
Date: Sun Dec 30 09:03:57 2018 +1100
Remove auth failure prompting from the engine
Add a signal to AccountInformation for notifying the client that an
auth failure has occurred, and remove code for prompting the user from
both IMAP and SMTP services. This means the same mechanism is used for
both for handling this and untrusted hosts, and allows the client to
implement desired policy, e.g. saving passwords and number of prompts to
retry.
src/engine/api/geary-account-information.vala | 43 ++++-------
src/engine/api/geary-client-service.vala | 1 +
src/engine/api/geary-credentials-mediator.vala | 15 ----
.../imap-engine/imap-engine-generic-account.vala | 83 +---------------------
src/engine/imap/api/imap-client-service.vala | 7 --
src/engine/imap/transport/imap-client-session.vala | 7 +-
src/engine/smtp/smtp-client-service.vala | 72 +++++--------------
7 files changed, 35 insertions(+), 193 deletions(-)
---
diff --git a/src/engine/api/geary-account-information.vala b/src/engine/api/geary-account-information.vala
index de6d9b49..c4d2df2e 100644
--- a/src/engine/api/geary-account-information.vala
+++ b/src/engine/api/geary-account-information.vala
@@ -210,11 +210,24 @@ public class Geary.AccountInformation : BaseObject {
}
+ /**
+ * Emitted when a service has reported an authentication failure.
+ *
+ * No further connection attempts will be made after this signal
+ * has been fired until the associated {@link ClientService} has
+ * been restarted. It is up to the client to prompt the user for
+ * updated credentials and restart the service.
+ */
+ public signal void authentication_failure(ServiceInformation service);
+
/**
* Emitted when a service has reported TLS certificate warnings.
*
- * It is up to the caller to pin the certificate appropriately if
- * the user does not want to receive these warnings in the future.
+ * No further connection attempts will be made after this signal
+ * has been fired until the associated {@link ClientService} has
+ * been restarted. It is up to the client to prompt the user to
+ * take action about the certificate (e.g. decide to pin it) then
+ * restart the service.
*/
public signal void untrusted_host(ServiceInformation service,
TlsNegotiationMethod method,
@@ -472,19 +485,6 @@ public class Geary.AccountInformation : BaseObject {
return loaded;
}
- /**
- * Prompts the user for their outgoing service authentication secret.
- *
- * Returns true if the credentials were successfully entered, else
- * false if the user dismissed the prompt.
- */
- public async bool prompt_outgoing_credentials(GLib.Cancellable? cancellable)
- throws GLib.Error {
- return yield this.mediator.prompt_token(
- this, this.outgoing, cancellable
- );
- }
-
/**
* Loads this account's incoming service credentials, if needed.
*
@@ -508,19 +508,6 @@ public class Geary.AccountInformation : BaseObject {
return loaded;
}
- /**
- * Prompts the user for their incoming service authentication secret.
- *
- * Returns true if the credentials were successfully entered, else
- * false if the user dismissed the prompt.
- */
- public async bool prompt_incoming_credentials(GLib.Cancellable? cancellable)
- throws GLib.Error {
- return yield this.mediator.prompt_token(
- this, this.incoming, cancellable
- );
- }
-
public bool equal_to(AccountInformation other) {
return (
this == other || (
diff --git a/src/engine/api/geary-client-service.vala b/src/engine/api/geary-client-service.vala
index 16b07c61..8cd879b3 100644
--- a/src/engine/api/geary-client-service.vala
+++ b/src/engine/api/geary-client-service.vala
@@ -308,6 +308,7 @@ public abstract class Geary.ClientService : BaseObject {
*/
protected void notify_authentication_failed() {
this.current_status = AUTHENTICATION_FAILED;
+ this.account.authentication_failure(this.configuration);
}
/**
diff --git a/src/engine/api/geary-credentials-mediator.vala b/src/engine/api/geary-credentials-mediator.vala
index 9074230b..e9022715 100644
--- a/src/engine/api/geary-credentials-mediator.vala
+++ b/src/engine/api/geary-credentials-mediator.vala
@@ -21,19 +21,4 @@ public interface Geary.CredentialsMediator : GLib.Object {
GLib.Cancellable? cancellable)
throws GLib.Error;
- /**
- * Prompt the user to enter passwords for the given service.
- *
- * Updates authentication-related details for the given service,
- * possibly including user login names, authentication tokens, and
- * the remember password preference.
- *
- * Returns false if the user tried to cancel the interaction, or
- * true if they tried to proceed.
- */
- public abstract async bool prompt_token(AccountInformation account,
- ServiceInformation service,
- GLib.Cancellable? cancellable)
- throws GLib.Error;
-
}
diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala
b/src/engine/imap-engine/imap-engine-generic-account.vala
index d5c5eab0..97dbdc27 100644
--- a/src/engine/imap-engine/imap-engine-generic-account.vala
+++ b/src/engine/imap-engine/imap-engine-generic-account.vala
@@ -56,8 +56,6 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account {
private AccountSynchronizer sync;
private TimeoutManager refresh_folder_timer;
- private uint authentication_failures = 0;
-
private Gee.Map<Geary.SpecialFolderType, Gee.List<string>> special_search_names =
new Gee.HashMap<Geary.SpecialFolderType, Gee.List<string>>();
@@ -74,9 +72,8 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account {
config, config.incoming, incoming_remote
);
this.imap.min_pool_size = IMAP_MIN_POOL_SIZE;
- this.imap.login_failed.connect(on_pool_login_failed);
this.imap.notify["current-status"].connect(
- on_remote_status_notify
+ on_imap_status_notify
);
this.smtp = new Smtp.ClientService(
@@ -121,9 +118,6 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account {
this.open_cancellable = new Cancellable();
this.remote_ready_lock = new Nonblocking.Semaphore(this.open_cancellable);
- // Reset this so we start trying to authenticate again
- this.authentication_failures = 0;
-
this.processor = new AccountProcessor(this.to_string());
this.processor.operation_error.connect(on_operation_error);
@@ -982,26 +976,6 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account {
throw new EngineError.OPEN_REQUIRED("Account %s not opened", to_string());
}
- private async void restart_incoming_service() {
- if (this.incoming.is_running) {
- try {
- yield this.incoming.stop(this.open_cancellable);
- } catch (GLib.Error err) {
- debug("%s: Failed to stop incoming mail service: %s",
- to_string(), err.message
- );
- }
- }
- try {
- yield this.incoming.start(this.open_cancellable);
- } catch (GLib.Error err) {
- debug("%s: Failed to start incoming mail service: %s",
- to_string(), err.message
- );
- }
-
- }
-
private void on_operation_error(AccountOperation op, Error error) {
if (error is ImapError) {
notify_service_problem(
@@ -1017,60 +991,7 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account {
}
}
- private void on_pool_login_failed(Geary.Imap.StatusResponse? response) {
- this.remote_ready_lock.reset();
- this.authentication_failures++;
- if (this.authentication_failures >= Geary.Account.AUTH_ATTEMPTS_MAX) {
- // We have tried auth too many times, so bail out
- notify_imap_problem(ProblemType.LOGIN_FAILED, null);
- } else {
- // login can fail due to an invalid password hence we
- // should re-ask it, but it can also fail due to server
- // inaccessibility, for instance "[UNAVAILABLE] / Maximum
- // number of connections from user+IP exceeded". In that
- // case, resetting password seems unneeded.
- bool reask_password = false;
- Error? login_error = null;
- try {
- reask_password = (
- response == null ||
- response.response_code == null ||
- response.response_code.get_response_code_type().value !=
Geary.Imap.ResponseCodeType.UNAVAILABLE
- );
- } catch (ImapError err) {
- login_error = err;
- debug("Unable to parse ResponseCode %s: %s", response.response_code.to_string(),
- err.message);
- }
-
- if (!reask_password) {
- // Either the server was unavailable, or we were unable to
- // parse the login response. Either way, indicate a
- // non-login error.
- notify_imap_problem(ProblemType.SERVER_ERROR, login_error);
- } else {
- // Now, we should ask the user for their password
- this.information.prompt_incoming_credentials.begin(
- this.open_cancellable,
- (obj, ret) => {
- try {
- if (this.information
- .prompt_incoming_credentials.end(ret)) {
- // Have a new password, so try that
- this.restart_incoming_service.begin();
- } else {
- // User cancelled, so indicate a login problem
- notify_imap_problem(ProblemType.LOGIN_FAILED, null);
- }
- } catch (Error err) {
- notify_imap_problem(ProblemType.GENERIC_ERROR, err);
- }
- });
- }
- }
- }
-
- private void on_remote_status_notify() {
+ private void on_imap_status_notify() {
if (this.open) {
if (this.imap.current_status == CONNECTED) {
this.is_online = true;
diff --git a/src/engine/imap/api/imap-client-service.vala b/src/engine/imap/api/imap-client-service.vala
index 9227a7d9..2361d710 100644
--- a/src/engine/imap/api/imap-client-service.vala
+++ b/src/engine/imap/api/imap-client-service.vala
@@ -91,11 +91,6 @@ internal class Geary.Imap.ClientService : Geary.ClientService {
private Cancellable? pool_cancellable = null;
- private bool authentication_failed = false;
-
- /** Fired when an authentication error occurs opening a session. */
- public signal void login_failed(StatusResponse? response);
-
public ClientService(AccountInformation account,
ServiceInformation service,
@@ -114,7 +109,6 @@ internal class Geary.Imap.ClientService : Geary.ClientService {
);
}
- this.authentication_failed = false;
this.pool_cancellable = new Cancellable();
notify_started();
}
@@ -484,7 +478,6 @@ internal class Geary.Imap.ClientService : Geary.ClientService {
}
private void on_login_failed(ClientSession session, StatusResponse? response) {
- login_failed(response);
notify_authentication_failed();
this.close_pool.begin();
}
diff --git a/src/engine/imap/transport/imap-client-session.vala
b/src/engine/imap/transport/imap-client-session.vala
index 30e153b0..60e87338 100644
--- a/src/engine/imap/transport/imap-client-session.vala
+++ b/src/engine/imap/transport/imap-client-session.vala
@@ -831,7 +831,7 @@ public class Geary.Imap.ClientSession : BaseObject {
//
// login
//
-
+
/**
* Performs the LOGIN command using the supplied credentials.
*
@@ -839,11 +839,6 @@ public class Geary.Imap.ClientSession : BaseObject {
*/
public async StatusResponse login_async(Geary.Credentials credentials, Cancellable? cancellable = null)
throws Error {
- if (!credentials.is_complete()) {
- login_failed(null);
- throw new ImapError.UNAUTHENTICATED("No credentials provided for account: %s",
credentials.to_string());
- }
-
Command? cmd = null;
switch (credentials.supported_method) {
case Geary.Credentials.Method.PASSWORD:
diff --git a/src/engine/smtp/smtp-client-service.vala b/src/engine/smtp/smtp-client-service.vala
index 859dcf8f..a6027a24 100644
--- a/src/engine/smtp/smtp-client-service.vala
+++ b/src/engine/smtp/smtp-client-service.vala
@@ -188,7 +188,7 @@ internal class Geary.Smtp.ClientService : Geary.ClientService {
}
// Returns true if email was successfully processed, else false
- private async bool process_email(EmailIdentifier id, Cancellable cancellable)
+ private async void process_email(EmailIdentifier id, Cancellable cancellable)
throws GLib.Error {
Email? email = null;
try {
@@ -198,60 +198,27 @@ internal class Geary.Smtp.ClientService : Geary.ClientService {
} catch (EngineError.NOT_FOUND err) {
debug("Queued email %s not found in outbox, ignoring: %s",
id.to_string(), err.message);
- return true;
}
- bool mail_sent = email.email_flags.contains(EmailFlags.OUTBOX_SENT);
- if (!mail_sent) {
- // We immediately retry auth errors after the prompting
- // the user, but if they get it wrong enough times or
- // cancel we have no choice other than to stop the postie
- uint attempts = 0;
- while (!mail_sent && ++attempts <= Geary.Account.AUTH_ATTEMPTS_MAX) {
- RFC822.Message message = email.get_message();
- try {
- debug("Outbox postie: Sending \"%s\" (ID:%s)...",
- message_subject(message), email.id.to_string());
- yield send_email(message, cancellable);
- mail_sent = true;
- } catch (Error send_err) {
- debug("Outbox postie send error: %s", send_err.message);
- if (send_err is SmtpError.AUTHENTICATION_FAILED) {
- if (attempts == Geary.Account.AUTH_ATTEMPTS_MAX) {
- throw send_err;
- }
-
- // At this point we may already have a
- // password in memory -- but it's incorrect.
- if (!yield this.account
- .prompt_outgoing_credentials(cancellable)) {
- // The user cancelled and hence they don't
- // want to be prompted again, so bail out.
- throw send_err;
- }
- } else {
- // not much else we can do - just bail out
- throw send_err;
- }
- }
- }
+ if (!email.email_flags.contains(EmailFlags.OUTBOX_SENT)) {
+ RFC822.Message message = email.get_message();
+ debug("Outbox postie: Sending \"%s\" (ID:%s)...",
+ message_subject(message), email.id.to_string());
+ yield send_email(message, cancellable);
// Mark as sent, so if there's a problem pushing up to
// Sent, we don't retry sending. Don't pass the
// cancellable here - if it's been sent we want to try to
// update the sent flag anyway
- if (mail_sent) {
- debug("Outbox postie: Marking %s as sent", email.id.to_string());
- Geary.EmailFlags flags = new Geary.EmailFlags();
- flags.add(Geary.EmailFlags.OUTBOX_SENT);
- yield this.outbox.mark_email_async(
- Collection.single(email.id), flags, null, null
- );
- }
+ debug("Outbox postie: Marking %s as sent", email.id.to_string());
+ Geary.EmailFlags flags = new Geary.EmailFlags();
+ flags.add(Geary.EmailFlags.OUTBOX_SENT);
+ yield this.outbox.mark_email_async(
+ Collection.single(email.id), flags, null, null
+ );
- if (!mail_sent || cancellable.is_cancelled()) {
- // try again later
- return false;
+ if (cancellable.is_cancelled()) {
+ throw new GLib.IOError.CANCELLED("Send has been cancelled");
}
}
@@ -259,21 +226,14 @@ internal class Geary.Smtp.ClientService : Geary.ClientService {
// sent, or previously sent but not saved. So now try flagging
// as such and saving it.
if (this.account.save_sent) {
- try {
- debug("Outbox postie: Saving %s to sent mail", email.id.to_string());
- yield save_sent_mail_async(email, cancellable);
- } catch (Error err) {
- debug("Outbox postie: Error saving sent mail: %s", err.message);
- return false;
- }
+ debug("Outbox postie: Saving %s to sent mail", email.id.to_string());
+ yield save_sent_mail_async(email, cancellable);
}
// Again, don't observe the cancellable here - if it's been
// send and saved we want to try to remove it anyway.
debug("Outbox postie: Deleting row %s", email.id.to_string());
yield this.outbox.remove_email_async(Collection.single(email.id), null);
-
- return true;
}
private async void send_email(Geary.RFC822.Message rfc822, Cancellable? cancellable)
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
