Re: [Patch v5 3/4] emacs: check drafts for encryption tags before saving

[Mark Walters <markwalters1009@gmail.com>]

On Mon, 07 Nov 2016, David Bremner <david@tethera.net> wrote:
> In general the user may not want to save plaintext copies of messages
> that they are sending encrypted, so give them a chance to abort.
> ---
>  emacs/notmuch-draft.el   | 40 ++++++++++++++++++++++++++++++++++++++++
>  test/T630-emacs-draft.sh | 13 +++++++++++++
>  2 files changed, 53 insertions(+)
>
> diff --git a/emacs/notmuch-draft.el b/emacs/notmuch-draft.el
> index 11d906b..5a230e8 100644
> --- a/emacs/notmuch-draft.el
> +++ b/emacs/notmuch-draft.el
> @@ -70,6 +70,21 @@ postponing and resuming a message."
>    :type '(repeat string)
>    :group 'notmuch-send)
>  
> +(defcustom notmuch-draft-save-plaintext 'ask
> +  "Should notmuch save/postpone in plaintext messages that seem
> +  like they are intended to be sent encrypted
> +(i.e with an mml encryption tag in it)."
> +  :type '(radio
> +	  (const :tag "Never" nil)
> +	  (const :tag "Ask every time" ask)
> +	  (const :tag "Always" t))
> +  :group 'notmuch-draft
> +  :group 'notmuch-crypto)
> +
> +(defvar notmuch-draft-encryption-tag-regex
> +  "<#\\(part encrypt\\|secure.*mode=.*encrypt>\\)"
> +  "Regular expression matching mml tags indicating encryption of part or message")
> +
>  (defvar notmuch-draft-id nil
>    "Message-id of the most recent saved draft of this message")
>  (make-variable-buffer-local 'notmuch-draft-id)
> @@ -102,6 +117,22 @@ Used when a new version is saved, or the message is sent."
>  	  (goto-char (+ (match-beginning 0) 2))
>  	  (insert "!"))))))
>  
> +(defun notmuch-draft--check-encryption-tag (&optional ask)
> +  "Query user if there an mml tag that looks like it might indicate encryption.
> +
> +Returns t if there is no such tag, or the user confirms they mean
> +it."
> +  (save-excursion
> +    (message-goto-body)
> +      (or
> +       ;; We are fine if no relevant tag is found, or
> +       (not (re-search-forward notmuch-draft-encryption-tag-regex nil 't))
> +       ;; The user confirms they means it.
> +       (and ask
> +	    (yes-or-no-p "(Customize `notmuch-draft-save-plaintext' to avoid this warning)
> +This message contains mml tags that suggest it is intended to be encrypted.
> +Really save and index an unencrypted copy? ")))))
> +
>  (defun notmuch-draft-save ()
>    "Save the current draft message in the notmuch database.
>  
> @@ -109,6 +140,15 @@ This saves the current message in the database with tags
>  `notmuch-draft-tags` (in addition to any default tags
>  applied to newly inserted messages)."
>    (interactive)
> +  (case notmuch-draft-save-plaintext
> +    ((ask)
> +     (unless (notmuch-draft--check-encryption-tag t)
> +       (error "Save aborted")))
> +    ((t)
> +     (ignore))
> +    ((nil)
> +     (unless (notmuch-draft--check-encryption-tag nil)
> +       (error "Refusing to save draft with encryption tags (see `notmuch-draft-save-plaintext')"))))

What would you think of rejigging the logic here? I would prefer that
the first check was "is there an encryption tag" and then if there is
such a tag decide what to do. The reason I prefer that is that it makes
the common case clear.

I realise there are downsides too -- eg in your code you don't even
check for secure tags if they are going to  be ignored anyway.

If you prefer your way then lets leave it as is.

Best wishes

Mark