Re: [PATCH v3] cli/insert: add --world-readable flag

Subject: Re: [PATCH v3] cli/insert: add --world-readable flag

Date: Thu, 15 Feb 2018 08:08:18 +0200

To: Daniel Kahn Gillmor, Notmuch Mail

Cc:

From: Tomi Ollila


On Thu, Feb 08 2018, Daniel Kahn Gillmor wrote:

> In some cases (e.g. when building a publicly-visible e-mail archive)
> it doesn't make any sense to restrict visibility of the message to the
> current user account.
>
> This adds a --world-readable boolean option for "notmuch insert", so
> that those who want to archive their mail publicly can feed their
> archiver with:
>
>     notmuch insert --world-readable
>
> Other local delivery agents (postfix's local, and dovecot's lda) all
> default to delivery in mode 0600 rather than relying on the user's
> umask, so this fix doesn't change the default.
>
> Also, this does not override the user's umask.  if the umask is
> already set tight, it will not become looser as the result of passing
> --world-readable.

Code looks good to me and tests ... also, +1

testing is too hard atm...

Tomi

>
> Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
> ---
>  doc/man1/notmuch-insert.rst |  6 ++++++
>  notmuch-insert.c            | 25 ++++++++++++++-----------
>  test/T070-insert.sh         | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 65 insertions(+), 11 deletions(-)
>
> diff --git a/doc/man1/notmuch-insert.rst b/doc/man1/notmuch-insert.rst
> index 47884515..86e2f567 100644
> --- a/doc/man1/notmuch-insert.rst
> +++ b/doc/man1/notmuch-insert.rst
> @@ -51,6 +51,12 @@ Supported options for **insert** include
>  ``--no-hooks``
>      Prevent hooks from being run.
>  
> +``--world-readable``
> +    When writing mail to the mailbox, allow it to be read by users
> +    other than the current user.  Note that this does not override
> +    umask.  By default, delivered mail is only readable by the current
> +    user.
> +
>  ``--decrypt=(true|nostash|auto|false)``
>      If ``true`` and the message is encrypted, try to decrypt the
>      message while indexing, stashing any session keys discovered.  If
> diff --git a/notmuch-insert.c b/notmuch-insert.c
> index 48490b51..d229c9dc 100644
> --- a/notmuch-insert.c
> +++ b/notmuch-insert.c
> @@ -159,10 +159,10 @@ mkdir_recursive (const void *ctx, const char *path, int mode)
>   * otherwise. Partial results are not cleaned up on errors.
>   */
>  static bool
> -maildir_create_folder (const void *ctx, const char *maildir)
> +maildir_create_folder (const void *ctx, const char *maildir, bool world_readable)
>  {
>      const char *subdirs[] = { "cur", "new", "tmp" };
> -    const int mode = 0700;
> +    const int mode = (world_readable ? 0755 : 0700);
>      char *subdir;
>      unsigned int i;
>  
> @@ -211,10 +211,11 @@ tempfilename (const void *ctx)
>   * is not touched).
>   */
>  static int
> -maildir_mktemp (const void *ctx, const char *maildir, char **path_out)
> +maildir_mktemp (const void *ctx, const char *maildir, bool world_readable, char **path_out)
>  {
>      char *filename, *path;
>      int fd;
> +    const int mode = (world_readable ? 0644 : 0600);
>  
>      do {
>  	filename = tempfilename (ctx);
> @@ -227,7 +228,7 @@ maildir_mktemp (const void *ctx, const char *maildir, char **path_out)
>  	    return -1;
>  	}
>  
> -	fd = open (path, O_WRONLY | O_CREAT | O_TRUNC | O_EXCL, 0600);
> +	fd = open (path, O_WRONLY | O_CREAT | O_TRUNC | O_EXCL, mode);
>      } while (fd == -1 && errno == EEXIST);
>  
>      if (fd == -1) {
> @@ -289,12 +290,12 @@ copy_fd (int fdout, int fdin)
>   * the file, or NULL on errors.
>   */
>  static char *
> -maildir_write_tmp (const void *ctx, int fdin, const char *maildir)
> +maildir_write_tmp (const void *ctx, int fdin, const char *maildir, bool world_readable)
>  {
>      char *path;
>      int fdout;
>  
> -    fdout = maildir_mktemp (ctx, maildir, &path);
> +    fdout = maildir_mktemp (ctx, maildir, world_readable, &path);
>      if (fdout < 0)
>  	return NULL;
>  
> @@ -323,11 +324,11 @@ FAIL:
>   * errors.
>   */
>  static char *
> -maildir_write_new (const void *ctx, int fdin, const char *maildir)
> +maildir_write_new (const void *ctx, int fdin, const char *maildir, bool world_readable)
>  {
>      char *cleanpath, *tmppath, *newpath, *newdir;
>  
> -    tmppath = maildir_write_tmp (ctx, fdin, maildir);
> +    tmppath = maildir_write_tmp (ctx, fdin, maildir, world_readable);
>      if (! tmppath)
>  	return NULL;
>      cleanpath = tmppath;
> @@ -457,6 +458,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
>      bool create_folder = false;
>      bool keep = false;
>      bool hooks = true;
> +    bool world_readable = false;
>      bool synchronize_flags;
>      char *maildir;
>      char *newpath;
> @@ -467,7 +469,8 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
>  	{ .opt_string = &folder, .name = "folder", .allow_empty = true },
>  	{ .opt_bool = &create_folder, .name = "create-folder" },
>  	{ .opt_bool = &keep, .name = "keep" },
> -	{ .opt_bool =  &hooks, .name = "hooks" },
> +	{ .opt_bool = &hooks, .name = "hooks" },
> +	{ .opt_bool = &world_readable, .name = "world-readable" },
>  	{ .opt_inherit = notmuch_shared_indexing_options },
>  	{ .opt_inherit = notmuch_shared_options },
>  	{ }
> @@ -523,7 +526,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
>      }
>  
>      strip_trailing (maildir, '/');
> -    if (create_folder && ! maildir_create_folder (config, maildir))
> +    if (create_folder && ! maildir_create_folder (config, maildir, world_readable))
>  	return EXIT_FAILURE;
>  
>      /* Set up our handler for SIGINT. We do not set SA_RESTART so that copying
> @@ -535,7 +538,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
>      sigaction (SIGINT, &action, NULL);
>  
>      /* Write the message to the Maildir new directory. */
> -    newpath = maildir_write_new (config, STDIN_FILENO, maildir);
> +    newpath = maildir_write_new (config, STDIN_FILENO, maildir, world_readable);
>      if (! newpath) {
>  	return EXIT_FAILURE;
>      }
> diff --git a/test/T070-insert.sh b/test/T070-insert.sh
> index 40519bb2..05be473a 100755
> --- a/test/T070-insert.sh
> +++ b/test/T070-insert.sh
> @@ -4,6 +4,10 @@ test_description='"notmuch insert"'
>  
>  test_require_external_prereq gdb
>  
> +# subtests about file permissions assume that we're working with umask
> +# 022 by default.
> +umask 022
> +
>  # Create directories and database before inserting.
>  mkdir -p "$MAIL_DIR"/{cur,new,tmp}
>  mkdir -p "$MAIL_DIR"/Drafts/{cur,new,tmp}
> @@ -37,6 +41,9 @@ notmuch insert < "$gen_msg_filename"
>  cur_msg_filename=$(notmuch search --output=files "subject:insert-subject")
>  test_expect_equal_file "$cur_msg_filename" "$gen_msg_filename"
>  
> +test_begin_subtest "Permissions on inserted message should be 0600"
> +test_expect_equal "600" "$(stat -c %a "$cur_msg_filename")"
> +
>  test_begin_subtest "Insert message adds default tags"
>  output=$(notmuch show --format=json "subject:insert-subject")
>  expected='[[[{
> @@ -73,6 +80,27 @@ notmuch insert +custom < "$gen_msg_filename"
>  output=$(notmuch search --output=messages tag:custom)
>  test_expect_equal "$output" "id:$gen_msg_id"
>  
> +test_begin_subtest "Insert tagged world-readable message"
> +gen_insert_msg
> +notmuch insert --world-readable +world-readable-test < "$gen_msg_filename"
> +cur_msg_filename=$(notmuch search --output=files "tag:world-readable-test")
> +test_expect_equal_file "$cur_msg_filename" "$gen_msg_filename"
> +
> +test_begin_subtest "Permissions on inserted world-readable message should be 0644"
> +test_expect_equal "644" "$(stat -c %a "$cur_msg_filename")"
> +
> +test_begin_subtest "Insert tagged world-readable message with group-only umask"
> +oldumask=$(umask)
> +umask 027
> +gen_insert_msg
> +notmuch insert --world-readable +world-readable-umask-test < "$gen_msg_filename"
> +cur_msg_filename=$(notmuch search --output=files "tag:world-readable-umask-test")
> +umask "$oldumask"
> +test_expect_equal_file "$cur_msg_filename" "$gen_msg_filename"
> +
> +test_begin_subtest "Permissions on inserted world-readable message with funny umask should be 0640"
> +test_expect_equal "640" "$(stat -c %a "$cur_msg_filename")"
> +
>  test_begin_subtest "Insert message, add/remove tags"
>  gen_insert_msg
>  notmuch insert +custom -unread < "$gen_msg_filename"
> @@ -170,6 +198,23 @@ output=$(notmuch search --output=files path:F/G/H/I/J/new tag:folder)
>  basename=$(basename "$output")
>  test_expect_equal_file "$gen_msg_filename" "${MAIL_DIR}/F/G/H/I/J/new/${basename}"
>  
> +test_begin_subtest "Created subfolder should have permissions 0700"
> +test_expect_equal "700" "$(stat -c %a "${MAIL_DIR}/F/G/H/I/J")"
> +test_begin_subtest "Created subfolder new/ should also have permissions 0700"
> +test_expect_equal "700" "$(stat -c %a "${MAIL_DIR}/F/G/H/I/J/new")"
> +
> +test_begin_subtest "Insert message, create world-readable subfolder"
> +gen_insert_msg
> +notmuch insert --folder=F/G/H/I/J/K --create-folder --world-readable +folder-world-readable < "$gen_msg_filename"
> +output=$(notmuch search --output=files path:F/G/H/I/J/K/new tag:folder-world-readable)
> +basename=$(basename "$output")
> +test_expect_equal_file "$gen_msg_filename" "${MAIL_DIR}/F/G/H/I/J/K/new/${basename}"
> +
> +test_begin_subtest "Created world-readable subfolder should have permissions 0755"
> +test_expect_equal "755" "$(stat -c %a "${MAIL_DIR}/F/G/H/I/J/K")"
> +test_begin_subtest "Created world-readable subfolder new/ should also have permissions 0755"
> +test_expect_equal "755" "$(stat -c %a "${MAIL_DIR}/F/G/H/I/J/K/new")"
> +
>  test_begin_subtest "Insert message, create existing subfolder"
>  gen_insert_msg
>  notmuch insert --folder=F/G/H/I/J --create-folder +folder < "$gen_msg_filename"
> -- 
> 2.15.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch
_______________________________________________
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Thread: