From 5d1e42c4538067d8d9061d0f7f9dfa01ade4758f Mon Sep 17 00:00:00 2001 From: Danny Baumann Date: Thu, 17 Jan 2013 15:24:22 +0100 Subject: [PATCH] Improve certificate failure notifications. The commit that introduced those notifications also introduced a rather ... interesting design pattern: The CertificateValidationException notified the user of its pure existance - it's no longer a 'message' only, but defines policy. As this is more than unusual, replace this pattern by the MessagingController treating CertificateValidationException specially when accessing remote folders. Also make clear which account failed when constructing the notification. --- res/values-de/strings.xml | 2 +- res/values-fr/strings.xml | 2 +- res/values/strings.xml | 2 +- src/com/fsck/k9/K9.java | 4 +- .../setup/AccountSetupCheckSettings.java | 19 ++--- .../activity/setup/AccountSetupIncoming.java | 9 +- .../activity/setup/AccountSetupOutgoing.java | 8 +- .../k9/controller/MessagingController.java | 82 +++++++++++++++---- .../mail/CertificateValidationException.java | 51 ++---------- src/com/fsck/k9/mail/store/ImapStore.java | 7 +- src/com/fsck/k9/mail/store/Pop3Store.java | 2 +- src/com/fsck/k9/mail/store/WebDavStore.java | 2 +- .../fsck/k9/mail/transport/SmtpTransport.java | 2 +- .../k9/mail/transport/imap/ImapSettings.java | 4 - 14 files changed, 100 insertions(+), 96 deletions(-) diff --git a/res/values-de/strings.xml b/res/values-de/strings.xml index 262996583..b9e324b95 100644 --- a/res/values-de/strings.xml +++ b/res/values-de/strings.xml @@ -214,7 +214,7 @@ Um Fehler zu melden, neue Funktionen vorzuschlagen oder Fragen zu stellen, besuc Gelesen Löschen - Zertifikatsproblem + Zertifikatsproblem (%s) Servereinstellungen überprüfen Neue Nachrichten in %s:%s werden abgerufen diff --git a/res/values-fr/strings.xml b/res/values-fr/strings.xml index 7afe4c10b..b1342c3d2 100644 --- a/res/values-fr/strings.xml +++ b/res/values-fr/strings.xml @@ -245,7 +245,7 @@ de plus Répondre Lire Supprimer - Erreur de certificat + Erreur de certificat (%s) Vérifier les paramètres serveur Vérification des messages\u00A0: %s:%s diff --git a/res/values/strings.xml b/res/values/strings.xml index c4a58507a..02ecda8cd 100644 --- a/res/values/strings.xml +++ b/res/values/strings.xml @@ -221,7 +221,7 @@ Please submit bug reports, contribute new features and ask questions at Reply Read Delete - Certificate error + Certificate error for %s Check your server settings Checking mail: %s:%s diff --git a/src/com/fsck/k9/K9.java b/src/com/fsck/k9/K9.java index 2ed3dd457..c4eab6452 100644 --- a/src/com/fsck/k9/K9.java +++ b/src/com/fsck/k9/K9.java @@ -361,9 +361,9 @@ public class K9 extends Application { // Must not conflict with an account number public static final int FETCHING_EMAIL_NOTIFICATION = -5000; public static final int SEND_FAILED_NOTIFICATION = -1500; + public static final int CERTIFICATE_EXCEPTION_NOTIFICATION_INCOMING = -2000; + public static final int CERTIFICATE_EXCEPTION_NOTIFICATION_OUTGOING = -2500; public static final int CONNECTIVITY_ID = -3; - public static final int CERTIFICATE_EXCEPTION_NOTIFICATION_INCOMING = -4; - public static final int CERTIFICATE_EXCEPTION_NOTIFICATION_OUTGOING = -5; public static class Intents { diff --git a/src/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java b/src/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java index bf4773c9b..674806356 100644 --- a/src/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java +++ b/src/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java @@ -108,7 +108,11 @@ public class AccountSetupCheckSettings extends K9Activity implements OnClickList finish(); return; } - clearCertificateErrorNotifications(); + + final MessagingController ctrl = MessagingController.getInstance(getApplication()); + ctrl.clearCertificateErrorNotifications(AccountSetupCheckSettings.this, + mAccount, mCheckIncoming, mCheckOutgoing); + if (mCheckIncoming) { store = mAccount.getRemoteStore(); @@ -199,19 +203,6 @@ public class AccountSetupCheckSettings extends K9Activity implements OnClickList }); } - private void clearCertificateErrorNotifications() { - final Application app = getApplication(); - final NotificationManager notifMgr = (NotificationManager) app - .getSystemService(Context.NOTIFICATION_SERVICE); - final String uuid = mAccount.getUuid(); - if (mCheckOutgoing){ - notifMgr.cancel(uuid, K9.CERTIFICATE_EXCEPTION_NOTIFICATION_OUTGOING); - } - if (mCheckIncoming){ - notifMgr.cancel(uuid, K9.CERTIFICATE_EXCEPTION_NOTIFICATION_INCOMING); - } - } - private void showErrorDialog(final int msgResId, final Object... args) { mHandler.post(new Runnable() { public void run() { diff --git a/src/com/fsck/k9/activity/setup/AccountSetupIncoming.java b/src/com/fsck/k9/activity/setup/AccountSetupIncoming.java index 4e341aa28..c76b6289f 100644 --- a/src/com/fsck/k9/activity/setup/AccountSetupIncoming.java +++ b/src/com/fsck/k9/activity/setup/AccountSetupIncoming.java @@ -2,6 +2,7 @@ package com.fsck.k9.activity.setup; import android.app.Activity; +import android.content.Context; import android.content.Intent; import android.os.Bundle; import android.text.Editable; @@ -33,7 +34,7 @@ import java.util.HashMap; import java.util.Map; public class AccountSetupIncoming extends K9Activity implements OnClickListener { - public static final String EXTRA_ACCOUNT = "account"; + private static final String EXTRA_ACCOUNT = "account"; private static final String EXTRA_MAKE_DEFAULT = "makeDefault"; private static final int[] POP3_PORTS = { @@ -90,10 +91,14 @@ public class AccountSetupIncoming extends K9Activity implements OnClickListener } public static void actionEditIncomingSettings(Activity context, Account account) { + context.startActivity(intentActionEditIncomingSettings(context, account)); + } + + public static Intent intentActionEditIncomingSettings(Context context, Account account) { Intent i = new Intent(context, AccountSetupIncoming.class); i.setAction(Intent.ACTION_EDIT); i.putExtra(EXTRA_ACCOUNT, account.getUuid()); - context.startActivity(i); + return i; } @Override diff --git a/src/com/fsck/k9/activity/setup/AccountSetupOutgoing.java b/src/com/fsck/k9/activity/setup/AccountSetupOutgoing.java index d49059a43..6f71545fe 100644 --- a/src/com/fsck/k9/activity/setup/AccountSetupOutgoing.java +++ b/src/com/fsck/k9/activity/setup/AccountSetupOutgoing.java @@ -26,7 +26,7 @@ import java.net.URLEncoder; public class AccountSetupOutgoing extends K9Activity implements OnClickListener, OnCheckedChangeListener { - public static final String EXTRA_ACCOUNT = "account"; + private static final String EXTRA_ACCOUNT = "account"; private static final String EXTRA_MAKE_DEFAULT = "makeDefault"; @@ -74,10 +74,14 @@ public class AccountSetupOutgoing extends K9Activity implements OnClickListener, } public static void actionEditOutgoingSettings(Context context, Account account) { + context.startActivity(intentActionEditOutgoingSettings(context, account)); + } + + public static Intent intentActionEditOutgoingSettings(Context context, Account account) { Intent i = new Intent(context, AccountSetupOutgoing.class); i.setAction(Intent.ACTION_EDIT); i.putExtra(EXTRA_ACCOUNT, account.getUuid()); - context.startActivity(i); + return i; } @Override diff --git a/src/com/fsck/k9/controller/MessagingController.java b/src/com/fsck/k9/controller/MessagingController.java index d73522e66..1344f2cdd 100644 --- a/src/com/fsck/k9/controller/MessagingController.java +++ b/src/com/fsck/k9/controller/MessagingController.java @@ -2,6 +2,8 @@ package com.fsck.k9.controller; import java.io.CharArrayWriter; import java.io.PrintWriter; +import java.security.cert.CertPathValidatorException; +import java.security.cert.CertificateException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -51,6 +53,8 @@ import com.fsck.k9.activity.FolderList; import com.fsck.k9.activity.MessageList; import com.fsck.k9.activity.MessageReference; import com.fsck.k9.activity.NotificationDeleteConfirmation; +import com.fsck.k9.activity.setup.AccountSetupIncoming; +import com.fsck.k9.activity.setup.AccountSetupOutgoing; import com.fsck.k9.helper.Contacts; import com.fsck.k9.helper.power.TracingPowerManager; import com.fsck.k9.helper.power.TracingPowerManager.TracingWakeLock; @@ -62,6 +66,7 @@ import com.fsck.k9.mail.Folder.FolderType; import com.fsck.k9.mail.Folder.OpenMode; import com.fsck.k9.mail.Message; import com.fsck.k9.mail.Message.RecipientType; +import com.fsck.k9.mail.CertificateValidationException; import com.fsck.k9.mail.MessagingException; import com.fsck.k9.mail.Part; import com.fsck.k9.mail.PushReceiver; @@ -1151,6 +1156,7 @@ public class MessagingController implements Runnable { for (MessagingListener l : getListeners(listener)) { l.synchronizeMailboxFailed(account, folder, rootMessage); } + notifyUserIfCertificateProblem(mApplication, e, account, true); addErrorMessage(account, null, e); Log.e(K9.LOG_TAG, "Failed synchronizing folder " + account.getDescription() + ":" + folder + " @ " + new Date()); @@ -1994,6 +2000,7 @@ public class MessagingController implements Runnable { } } } catch (MessagingException me) { + notifyUserIfCertificateProblem(mApplication, me, account, true); addErrorMessage(account, null, me); Log.e(K9.LOG_TAG, "Could not process command '" + processingCommand + "'", me); throw me; @@ -2602,6 +2609,60 @@ public class MessagingController implements Runnable { } } + private void notifyUserIfCertificateProblem(Context context, Exception e, + Account account, boolean incoming) { + if (!(e instanceof CertificateValidationException)) { + return; + } + + CertificateValidationException cve = (CertificateValidationException) e; + if (!cve.needsUserAttention()) { + return; + } + + final int id = incoming + ? K9.CERTIFICATE_EXCEPTION_NOTIFICATION_INCOMING + account.getAccountNumber() + : K9.CERTIFICATE_EXCEPTION_NOTIFICATION_OUTGOING + account.getAccountNumber(); + final Intent i = incoming + ? AccountSetupIncoming.intentActionEditIncomingSettings(context, account) + : AccountSetupOutgoing.intentActionEditOutgoingSettings(context, account); + final PendingIntent pi = PendingIntent.getActivity(context, + account.getAccountNumber(), i, PendingIntent.FLAG_UPDATE_CURRENT); + final String title = context.getString( + R.string.notification_certificate_error_text, account.getName()); + + final NotificationCompat.Builder builder = new NotificationCompat.Builder(context); + builder.setSmallIcon(R.drawable.stat_notify_email_generic); + builder.setWhen(System.currentTimeMillis()); + builder.setAutoCancel(true); + builder.setTicker(title); + builder.setContentTitle(title); + builder.setContentText(context.getString(R.string.notification_certificate_error_text)); + builder.setContentIntent(pi); + + configureNotification(builder, null, null, + K9.NOTIFICATION_LED_FAILURE_COLOR, + K9.NOTIFICATION_LED_BLINK_FAST, true); + + final NotificationManager nm = (NotificationManager) + context.getSystemService(Context.NOTIFICATION_SERVICE); + nm.notify(null, id, builder.build()); + } + + public void clearCertificateErrorNotifications(Context context, + final Account account, boolean incoming, boolean outgoing) { + final NotificationManager nm = (NotificationManager) + context.getSystemService(Context.NOTIFICATION_SERVICE); + + if (incoming) { + nm.cancel(null, K9.CERTIFICATE_EXCEPTION_NOTIFICATION_INCOMING + account.getAccountNumber()); + } + if (outgoing) { + nm.cancel(null, K9.CERTIFICATE_EXCEPTION_NOTIFICATION_OUTGOING + account.getAccountNumber()); + } + } + + static long uidfill = 0; static AtomicBoolean loopCatch = new AtomicBoolean(); public void addErrorMessage(Account account, String subject, Throwable t) { @@ -2986,6 +3047,7 @@ public class MessagingController implements Runnable { for (MessagingListener l : getListeners(listener)) { l.loadMessageForViewFailed(account, folder, uid, e); } + notifyUserIfCertificateProblem(mApplication, e, account, true); addErrorMessage(account, null, e); return false; } finally { @@ -3134,6 +3196,7 @@ public class MessagingController implements Runnable { for (MessagingListener l : getListeners(listener)) { l.loadAttachmentFailed(account, message, part, tag, me.getMessage()); } + notifyUserIfCertificateProblem(mApplication, me, account, true); addErrorMessage(account, null, me); } finally { @@ -3308,23 +3371,6 @@ public class MessagingController implements Runnable { builder.build()); } - public void notify(String tag, int id, String title, String text, PendingIntent pi) { - final NotificationManager notifMgr = (NotificationManager) mApplication - .getSystemService(Context.NOTIFICATION_SERVICE); - NotificationCompat.Builder builder = new NotificationCompat.Builder(mApplication); - builder.setSmallIcon(R.drawable.stat_notify_email_generic); - builder.setWhen(System.currentTimeMillis()); - builder.setAutoCancel(true); - builder.setTicker(title); - builder.setContentTitle(title); - builder.setContentText(text); - builder.setContentIntent(pi); - configureNotification(builder, null, null, - K9.NOTIFICATION_LED_FAILURE_COLOR, - K9.NOTIFICATION_LED_BLINK_FAST, true); - notifMgr.notify(tag, id, builder.build()); - } - /** * Display an ongoing notification while checking for new messages on the server. * @@ -3510,6 +3556,7 @@ public class MessagingController implements Runnable { localFolder.moveMessages(new Message[] { message }, (LocalFolder) localStore.getFolder(account.getDraftsFolderName())); } + notifyUserIfCertificateProblem(mApplication, e, account, false); message.setFlag(Flag.X_SEND_FAILED, true); Log.e(K9.LOG_TAG, "Failed to send message", e); for (MessagingListener l : getListeners()) { @@ -4123,7 +4170,6 @@ public class MessagingController implements Runnable { throw new UnavailableAccountException(e); } catch (Exception e) { Log.e(K9.LOG_TAG, "emptyTrash failed", e); - addErrorMessage(account, null, e); } finally { closeFolder(localFolder); diff --git a/src/com/fsck/k9/mail/CertificateValidationException.java b/src/com/fsck/k9/mail/CertificateValidationException.java index b34b66f44..2b25dd3d0 100644 --- a/src/com/fsck/k9/mail/CertificateValidationException.java +++ b/src/com/fsck/k9/mail/CertificateValidationException.java @@ -1,15 +1,6 @@ package com.fsck.k9.mail; -import android.app.Application; -import android.app.PendingIntent; -import android.content.Intent; -import com.fsck.k9.Account; -import com.fsck.k9.K9; -import com.fsck.k9.R; -import com.fsck.k9.activity.setup.AccountSetupIncoming; -import com.fsck.k9.activity.setup.AccountSetupOutgoing; -import com.fsck.k9.controller.MessagingController; import java.security.cert.CertPathValidatorException; import java.security.cert.CertificateException; @@ -20,44 +11,20 @@ public class CertificateValidationException extends MessagingException { super(message); } - public CertificateValidationException(final String message, - Throwable throwable, final Account account, final boolean incoming) { + public CertificateValidationException(final String message, Throwable throwable) { super(message, throwable); - /* - * We get here because of an SSLException. We are only interested in - * creating a notification if the underlying cause is certificate - * related. - */ + } + + public boolean needsUserAttention() { + Throwable throwable = getCause(); + + /* user attention is required if the certificate was deemed invalid */ while (throwable != null && !(throwable instanceof CertPathValidatorException) && !(throwable instanceof CertificateException)) { throwable = throwable.getCause(); } - if (throwable == null) - return; - final Application app = K9.app; - final String title = app - .getString(R.string.notification_certificate_error_title); - final String text = app - .getString(R.string.notification_certificate_error_text); - final Class className; - final String extraName; - final int id; - if (incoming) { - className = AccountSetupIncoming.class; - extraName = AccountSetupIncoming.EXTRA_ACCOUNT; - id = K9.CERTIFICATE_EXCEPTION_NOTIFICATION_INCOMING; - } else { - className = AccountSetupOutgoing.class; - extraName = AccountSetupOutgoing.EXTRA_ACCOUNT; - id = K9.CERTIFICATE_EXCEPTION_NOTIFICATION_OUTGOING; - } - final Intent i = new Intent(app, className); - final String uuid = account.getUuid(); - i.setAction(Intent.ACTION_EDIT); - i.putExtra(extraName, uuid); - final PendingIntent pi = PendingIntent.getActivity(app, 0, i, 0); - MessagingController controller = MessagingController.getInstance(app); - controller.notify(uuid, id, title, text, pi); + + return throwable != null; } } \ No newline at end of file diff --git a/src/com/fsck/k9/mail/store/ImapStore.java b/src/com/fsck/k9/mail/store/ImapStore.java index 9b0efca3f..ff696828b 100644 --- a/src/com/fsck/k9/mail/store/ImapStore.java +++ b/src/com/fsck/k9/mail/store/ImapStore.java @@ -428,11 +428,6 @@ public class ImapStore extends Store { public void setCombinedPrefix(String prefix) { mCombinedPrefix = prefix; } - - @Override - public Account getAccount() { - return mAccount; - } } private static final SimpleDateFormat RFC3501_DATE = new SimpleDateFormat("dd-MMM-yyyy", Locale.US); @@ -2627,7 +2622,7 @@ public class ImapStore extends Store { } catch (SSLException e) { - throw new CertificateValidationException(e.getMessage(), e, mSettings.getAccount(), true); + throw new CertificateValidationException(e.getMessage(), e); } catch (GeneralSecurityException gse) { throw new MessagingException( "Unable to open connection to IMAP server due to security error.", gse); diff --git a/src/com/fsck/k9/mail/store/Pop3Store.java b/src/com/fsck/k9/mail/store/Pop3Store.java index 85b85110b..ec7c56961 100644 --- a/src/com/fsck/k9/mail/store/Pop3Store.java +++ b/src/com/fsck/k9/mail/store/Pop3Store.java @@ -391,7 +391,7 @@ public class Pop3Store extends Store { mCapabilities = getCapabilities(); } catch (SSLException e) { - throw new CertificateValidationException(e.getMessage(), e, mAccount, true); + throw new CertificateValidationException(e.getMessage(), e); } catch (GeneralSecurityException gse) { throw new MessagingException( "Unable to open connection to POP server due to security error.", gse); diff --git a/src/com/fsck/k9/mail/store/WebDavStore.java b/src/com/fsck/k9/mail/store/WebDavStore.java index 26199a285..770e61e38 100644 --- a/src/com/fsck/k9/mail/store/WebDavStore.java +++ b/src/com/fsck/k9/mail/store/WebDavStore.java @@ -848,7 +848,7 @@ public class WebDavStore extends Store { response.getStatusLine().toString()); } } catch (SSLException e) { - throw new CertificateValidationException(e.getMessage(), e, mAccount, true); + throw new CertificateValidationException(e.getMessage(), e); } catch (IOException ioe) { Log.e(K9.LOG_TAG, "IOException: " + ioe + "\nTrace: " + processException(ioe)); throw new MessagingException("IOException", ioe); diff --git a/src/com/fsck/k9/mail/transport/SmtpTransport.java b/src/com/fsck/k9/mail/transport/SmtpTransport.java index 99cd76d61..b1dba211e 100644 --- a/src/com/fsck/k9/mail/transport/SmtpTransport.java +++ b/src/com/fsck/k9/mail/transport/SmtpTransport.java @@ -389,7 +389,7 @@ public class SmtpTransport extends Transport { } } } catch (SSLException e) { - throw new CertificateValidationException(e.getMessage(), e, mAccount, false); + throw new CertificateValidationException(e.getMessage(), e); } catch (GeneralSecurityException gse) { throw new MessagingException( "Unable to open connection to SMTP server due to security error.", gse); diff --git a/src/com/fsck/k9/mail/transport/imap/ImapSettings.java b/src/com/fsck/k9/mail/transport/imap/ImapSettings.java index 7e367cac6..63c22f6d9 100644 --- a/src/com/fsck/k9/mail/transport/imap/ImapSettings.java +++ b/src/com/fsck/k9/mail/transport/imap/ImapSettings.java @@ -1,6 +1,5 @@ package com.fsck.k9.mail.transport.imap; -import com.fsck.k9.Account; import com.fsck.k9.mail.store.ImapStore; import com.fsck.k9.mail.store.ImapStore.AuthType; import com.fsck.k9.mail.store.ImapStore.ImapConnection; @@ -34,7 +33,4 @@ public interface ImapSettings { String getCombinedPrefix(); void setCombinedPrefix(String prefix); - - Account getAccount(); - }