[geary/geary-0.13] Merge branch 'wip/rework-password-prompting' into 'master'
- From: Michael Gratton <mjog src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [geary/geary-0.13] Merge branch 'wip/rework-password-prompting' into 'master'
- Date: Thu, 7 Mar 2019 00:44:04 +0000 (UTC)
commit be5dac25504cb67bd877c9ea5610c5e928fbd078
Author: Michael Gratton <mike vee net>
Date: Wed Mar 6 13:49:41 2019 +0000
Merge branch 'wip/rework-password-prompting' into 'master'
Fix some issues with password prompting
See merge request GNOME/geary!166
(cherry picked from commit 65b8515d10e7ae6381c8e5f217f18d25a98859be)
085518a6 Don't prompt for passwords from deep within the engine
30b55a03 Remove now-unused prompting code from Mediator implementations
a0fbf847 Fix some crashes prompting for passwords in the controller
cd10cd7c Fix a deadlock when notifying engine of updated credentials
58110dd4 Handle un-remembered passwords properly
src/client/application/geary-controller.vala | 49 +++++++++-----
src/client/application/goa-mediator.vala | 11 ----
src/client/application/secret-mediator.vala | 92 ++++++---------------------
src/client/dialogs/password-dialog.vala | 10 +--
src/engine/api/geary-account-information.vala | 36 +++++++----
src/engine/imap/api/imap-client-service.vala | 6 +-
src/engine/smtp/smtp-client-service.vala | 4 +-
7 files changed, 87 insertions(+), 121 deletions(-)
---
diff --git a/src/client/application/geary-controller.vala b/src/client/application/geary-controller.vala
index dda2a490..347bd394 100644
--- a/src/client/application/geary-controller.vala
+++ b/src/client/application/geary-controller.vala
@@ -334,7 +334,7 @@ public class GearyController : Geary.BaseObject {
SecretMediator? libsecret = null;
try {
- libsecret = yield new SecretMediator(this.application, cancellable);
+ libsecret = yield new SecretMediator(cancellable);
} catch (GLib.Error err) {
error("Error opening libsecret: %s", err.message);
}
@@ -774,29 +774,41 @@ public class GearyController : Geary.BaseObject {
PasswordDialog password_dialog = new PasswordDialog(
this.application.get_active_window(),
account,
- service
+ service,
+ credentials
);
if (password_dialog.run()) {
- service.credentials = service.credentials.copy_with_token(
- password_dialog.password
- );
- service.remember_password = password_dialog.remember_password;
-
// The update the credentials for the service that the
// credentials actually came from
Geary.ServiceInformation creds_service =
- credentials == account.incoming.credentials
+ (credentials == account.incoming.credentials)
? account.incoming
: account.outgoing;
+ creds_service.credentials = credentials.copy_with_token(
+ password_dialog.password
+ );
+
+ // Update the remember password pref if changed
+ bool remember = password_dialog.remember_password;
+ if (creds_service.remember_password != remember) {
+ creds_service.remember_password = remember;
+ account.changed();
+ }
+
SecretMediator libsecret = (SecretMediator) account.mediator;
try {
- yield libsecret.update_token(
- account, creds_service, context.cancellable
- );
- // Update the actual service in the engine though
- yield this.application.engine.update_account_service(
- account, service, context.cancellable
- );
+ // Update the secret using the service where the
+ // credentials originated, since the service forms
+ // part of the key's identity
+ if (creds_service.remember_password) {
+ yield libsecret.update_token(
+ account, creds_service, context.cancellable
+ );
+ } else {
+ yield libsecret.clear_token(
+ account, creds_service, context.cancellable
+ );
+ }
} catch (GLib.IOError.CANCELLED err) {
// all good
} catch (GLib.Error err) {
@@ -809,6 +821,7 @@ public class GearyController : Geary.BaseObject {
)
);
}
+
context.authentication_attempts++;
} else {
// User cancelled, bail out unconditionally
@@ -817,7 +830,11 @@ public class GearyController : Geary.BaseObject {
context.authentication_prompting = false;
}
- if (!handled) {
+ if (handled) {
+ yield this.application.engine.update_account_service(
+ account, service, context.cancellable
+ );
+ } else {
context.authentication_attempts = 0;
context.authentication_failed = true;
update_account_status();
diff --git a/src/client/application/goa-mediator.vala b/src/client/application/goa-mediator.vala
index 40a41078..859b792d 100644
--- a/src/client/application/goa-mediator.vala
+++ b/src/client/application/goa-mediator.vala
@@ -94,17 +94,6 @@ public class GoaMediator : Geary.CredentialsMediator, Object {
return loaded;
}
- public virtual async bool prompt_token(Geary.AccountInformation account,
- Geary.ServiceInformation service,
- GLib.Cancellable? cancellable)
- throws GLib.Error {
- // XXX Open a dialog that says "Click here to change your GOA
- // password" or "GOA credentials need renewing" or
- // something. Connect to the GOA service and wait until we
- // hear that needs attention is no longer true.
- return false;
- }
-
private Geary.Credentials.Method get_auth_method() throws GLib.Error {
if (this.handle.get_oauth2_based() != null) {
return Geary.Credentials.Method.OAUTH2;
diff --git a/src/client/application/secret-mediator.vala b/src/client/application/secret-mediator.vala
index 2d541b96..3d3645ef 100644
--- a/src/client/application/secret-mediator.vala
+++ b/src/client/application/secret-mediator.vala
@@ -35,14 +35,9 @@ public class SecretMediator : Geary.CredentialsMediator, Object {
null
);
- private GearyApplication application;
- private Geary.Nonblocking.Mutex dialog_mutex = new Geary.Nonblocking.Mutex();
-
- public async SecretMediator(GearyApplication application,
- GLib.Cancellable? cancellable)
+ public async SecretMediator(GLib.Cancellable? cancellable)
throws GLib.Error {
- this.application = application;
yield check_unlocked(cancellable);
}
@@ -51,84 +46,37 @@ public class SecretMediator : Geary.CredentialsMediator, Object {
Cancellable? cancellable)
throws GLib.Error {
bool loaded = false;
- if (service.credentials != null && service.remember_password) {
- string? password = yield Secret.password_lookupv(
- SecretMediator.schema, new_attrs(service), cancellable
- );
+ if (service.credentials != null) {
+ if (service.remember_password) {
+ string? password = yield Secret.password_lookupv(
+ SecretMediator.schema, new_attrs(service), cancellable
+ );
- if (password == null) {
- password = yield migrate_old_password(service, cancellable);
- }
+ if (password == null) {
+ password = yield migrate_old_password(service, cancellable);
+ }
- if (password != null) {
- service.credentials =
+ if (password != null) {
+ service.credentials =
service.credentials.copy_with_token(password);
- loaded = true;
+ loaded = true;
+ }
+ } else {
+ // Not remembering the password, so just make sure it
+ // has been filled in
+ loaded = service.credentials.is_complete();
}
}
- if (!loaded) {
- loaded = yield prompt_token(account, service, cancellable);
- }
-
return loaded;
}
- public virtual async bool prompt_token(Geary.AccountInformation account,
- Geary.ServiceInformation service,
- GLib.Cancellable? cancellable)
- throws GLib.Error {
- if (service.credentials != null) {
- // to prevent multiple dialogs from popping up at the same
- // time, use a nonblocking mutex to serialize the code
- int token = yield dialog_mutex.claim_async(null);
-
- // Ensure main window present to the window
- this.application.present();
-
- PasswordDialog password_dialog = new PasswordDialog(
- this.application.get_active_window(),
- account,
- service
- );
- bool result = password_dialog.run();
-
- dialog_mutex.release(ref token);
-
- if (result) {
- // password_dialog.password should never be null at this
- // point. It will only be null when password_dialog.run()
- // returns false, in which case we have already returned.
- service.credentials = service.credentials.copy_with_token(
- password_dialog.password
- );
- service.remember_password = password_dialog.remember_password;
-
- yield update_token(account, service, cancellable);
-
- account.changed();
- }
- }
- return true;
- }
-
public async void update_token(Geary.AccountInformation account,
Geary.ServiceInformation service,
Cancellable? cancellable)
- throws Error {
- if (service.credentials != null && service.remember_password) {
- try {
- yield do_store(service, service.credentials.token, cancellable);
- } catch (Error e) {
- debug(
- "Unable to store libsecret password for %s: %s %s",
- account.id,
- to_proto_value(service.protocol),
- service.credentials.user
- );
- }
- } else {
- yield clear_token(account, service, cancellable);
+ throws GLib.Error {
+ if (service.credentials != null) {
+ yield do_store(service, service.credentials.token, cancellable);
}
}
diff --git a/src/client/dialogs/password-dialog.vala b/src/client/dialogs/password-dialog.vala
index e8985cfc..149d9ca8 100644
--- a/src/client/dialogs/password-dialog.vala
+++ b/src/client/dialogs/password-dialog.vala
@@ -25,7 +25,8 @@ public class PasswordDialog {
public PasswordDialog(Gtk.Window? parent,
Geary.AccountInformation account,
- Geary.ServiceInformation service) {
+ Geary.ServiceInformation service,
+ Geary.Credentials? credentials) {
Gtk.Builder builder = GioUtil.create_builder("password-dialog.glade");
dialog = (Gtk.Dialog) builder.get_object("PasswordDialog");
@@ -43,18 +44,13 @@ public class PasswordDialog {
Gtk.Label primary_text_label = (Gtk.Label) builder.get_object("primary_text_label");
primary_text_label.set_markup(PRIMARY_TEXT_MARKUP.printf(PRIMARY_TEXT_FIRST_TRY));
- bool is_smtp = service.protocol == Geary.Protocol.SMTP;
-
- Geary.Credentials? credentials = (is_smtp)
- ? account.get_outgoing_credentials() : account.incoming.credentials;
-
if (credentials != null) {
label_username.set_text(credentials.user);
entry_password.set_text(credentials.token ?? "");
}
check_remember_password.active = service.remember_password;
- if (is_smtp) {
+ if ((service.protocol == Geary.Protocol.SMTP)) {
label_smtp.show();
}
diff --git a/src/engine/api/geary-account-information.vala b/src/engine/api/geary-account-information.vala
index 2d0a8ff1..89b83363 100644
--- a/src/engine/api/geary-account-information.vala
+++ b/src/engine/api/geary-account-information.vala
@@ -468,35 +468,45 @@ public class Geary.AccountInformation : BaseObject {
/**
* Loads this account's outgoing service credentials, if needed.
*
- * Credentials are loaded from the mediator, which may cause the
- * user to be prompted for the secret, thus it may yield for some
- * time.
+ * Credentials are loaded from the mediator, thus it may yield for
+ * some time.
*
- * Returns true if the credentials were successfully loaded or had
- * been previously loaded, or false the credentials could not be
- * loaded and the service's credentials are invalid.
+ * Returns true if the credentials were successfully loaded, or
+ * false if the credentials could not be loaded and the service's
+ * credentials are invalid.
*/
- public async void load_outgoing_credentials(GLib.Cancellable? cancellable)
+ public async bool load_outgoing_credentials(GLib.Cancellable? cancellable)
throws GLib.Error {
Credentials? creds = this.outgoing.credentials;
+ bool loaded = false;
if (creds != null) {
- yield this.mediator.load_token(this, this.outgoing, cancellable);
+ loaded = yield this.mediator.load_token(
+ this, this.outgoing, cancellable
+ );
}
+ return loaded;
}
/**
* Loads this account's incoming service credentials, if needed.
*
- * Credentials are loaded from the mediator, which may cause the
- * user to be prompted for the secret, thus it may yield for some
- * time.
+ * Credentials are loaded from the mediator, thus it may yield for
+ * some time.
+ *
+ * Returns true if the credentials were successfully loaded, or
+ * false if the credentials could not be loaded and the service's
+ * credentials are invalid.
*/
- public async void load_incoming_credentials(GLib.Cancellable? cancellable)
+ public async bool load_incoming_credentials(GLib.Cancellable? cancellable)
throws GLib.Error {
Credentials? creds = this.incoming.credentials;
+ bool loaded = false;
if (creds != null) {
- yield this.mediator.load_token(this, this.incoming, cancellable);
+ loaded = yield this.mediator.load_token(
+ this, this.incoming, cancellable
+ );
}
+ return loaded;
}
public bool equal_to(AccountInformation other) {
diff --git a/src/engine/imap/api/imap-client-service.vala b/src/engine/imap/api/imap-client-service.vala
index 3b36043c..24a1d0d2 100644
--- a/src/engine/imap/api/imap-client-service.vala
+++ b/src/engine/imap/api/imap-client-service.vala
@@ -270,9 +270,13 @@ internal class Geary.Imap.ClientService : Geary.ClientService {
// are up-to-date before attempting a connection, but
// after we know we should be able to connect to it
try {
- yield this.account.load_incoming_credentials(
+ bool loaded = yield this.account.load_incoming_credentials(
this.pool_cancellable
);
+ if (!loaded) {
+ notify_authentication_failed();
+ return;
+ }
} catch (GLib.Error err) {
notify_connection_failed(new ErrorContext(err));
return;
diff --git a/src/engine/smtp/smtp-client-service.vala b/src/engine/smtp/smtp-client-service.vala
index bd22190a..59c41ba0 100644
--- a/src/engine/smtp/smtp-client-service.vala
+++ b/src/engine/smtp/smtp-client-service.vala
@@ -190,7 +190,9 @@ internal class Geary.Smtp.ClientService : Geary.ClientService {
throws GLib.Error {
// To prevent spurious connection failures, ensure tokens are
// up-to-date before attempting to send the email
- yield this.account.load_outgoing_credentials(cancellable);
+ if (!yield this.account.load_outgoing_credentials(cancellable)) {
+ throw new SmtpError.AUTHENTICATION_FAILED("Credentials not loaded");
+ }
Email? email = null;
try {
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
