atomic_write's chown breaks multi-user access

dburry · November 19, 2011, 11:16pm

Some people may want to run their rails server as one user, deploy their app as another user, run some rake tasks manually or via cron that also write to the same file system as another user, etc. There are a variety of reasons one may want to do this kind of separation, including security. You'd think this should be easy under Unix with a common group they all share, a sane 002 umask, and a group sticky bit for the shared-write-access areas of the app. But it's not, because atomic_write breaks this scenario.

Here's what atomic_write does currently:

1. It writes a new temp file 2. It checks the permissions and ownership of the old file 3. It moves the new temp file over the old file 4. It changes permissions and ownerships of the new file to match the old one.

The problem is that you can only change the ownership of a file if you are root (and nobody runs their rails app as root, right?), or if you're changing it to yourself and it's already owned by yourself (in which case, it's a no-op waste of time). Otherwise you get an "Operation not permitted" error.

Therefore, since it's not doing any good anyway, I propose we delete the chown line in atomic_write, to allow multi-user access in the above scenario. We can keep the chmod line as is of course.

Here's a code snippet from:

github.com

rails/rails/blob/main/activesupport/lib/active_support/core_ext/file/atomic.rb

# frozen_string_literal: true

require "fileutils"

class File
  # Write to a file atomically. Useful for situations where you don't
  # want other processes or threads to see half-written files.
  #
  #   File.atomic_write('important.file') do |file|
  #     file.write('hello')
  #   end
  #
  # This method needs to create a temporary file. By default it will create it
  # in the same directory as the destination file. If you don't like this
  # behavior you can provide a different directory but it must be on the
  # same physical filesystem as the file you're trying to write.
  #
  #   File.atomic_write('/data/something.important', '/data/tmp') do |file|
  #     file.write('hello')
  #   end

This file has been truncated. show original

# Overwrite original file with temp file FileUtils.mv(temp_file.path, file_name)

# Set correct permissions on new file chown(old_stat.uid, old_stat.gid, file_name) ##### <-- delete this line chmod(old_stat.mode, file_name)

I am happy to send a pull request on github if people seem to like this idea.

Michael_Koziarski1 · November 20, 2011, 8:47pm

Set correct permissions on new file

chown(old_stat.uid, old_stat.gid, file_name) ##### ← delete

this line

This line was added deliberately. Prior to its inclusion the files had the ownership and group information of the file returned by Tempfile, which was almost always wrong (e.g. the www-data user couldn’t read it). Now those values get copied across from the directory in question which means that you can safely use atomic_write to generate assets that are served by your web server, you just need to make that directory’s group www-data and add the running user as a member of that group.

I’ve no idea why you want to separate deploying and running users but you should be able to work around this by setting the ownership of the relevant directory at deploy time to the values you want rails to maintain. Removing that line seems much more likely to break things for the majority.

dburry · November 20, 2011, 9:57pm

I think I now see the flaw in my original logic, thanks. How about this instead: change the line to:

chown(nil, old_stat.gid, file_name)

The reason being: no platform ever allows you to change the uid, unless you're running as root, and nobody runs their rails servers as root, right? But platforms do allow you to change the gid as long as you are a member, and that's still useful for all the reasons you outlined.

Dave

Michael_Koziarski1 · November 20, 2011, 10:06pm

Yep, that sounds like it would work well