File upload and posix acl support - updated

Get Help
1

I see File upload and posix acl support was closed. I wanted to check in to see if there had been any more investigation into this. At the moment it’s stopping our users from uploading to shared directories given the way we set acls. For details see original post.

Cheers

Trent

2

We recently ran into a related issue with file upload. In our case the user was uploading
to a directory that was owned by them but was setgid with a group that they were not in.
The upload actually succeeded but they got an error message. The relevant
code is the handle_upload method defined in /var/www/ood/apps/sys/dashboard/app/models/posix_file.rb

Here are the last few lines:

    FileUtils.mv tempfile, path.to_s
    File.chmod(mode, path.to_s)
    
    path.chown(nil, path.parent.stat.gid) if path.parent.setgid?

It’s that chown that is causing the error we see. The only reason for it is to undo a chown
that is inside FileUtils.mv (defined in /opt/ood/ondemand/root/usr/share/gems/3.1/ondemand/3.1.7-1/gems/bundler-2.3.6/lib/bundler/vendor/fileutils/lib/fileutils.rb.)

But there is also a FileUtils.cp that just copies the file with no chown. So my workaround,
is the replace the above with:

    FileUtils.cp tempfile, path.to_s
    File.chmod(mode, path.to_s) if path.owned?

I added the test on the chmod because it would fail if the user is over-writing an existing file that they don’t own.

In the acl case, the chown in FileUtils.mv will probably cause a problem too, so switching to cp should help. You might also want to get rid of the chmod.

3

Sorry for no continuing the original topic. Looks like we’ve found a bug.

1 Like
4

This sounds very similar to our issue. Nice debugging!

5

I’ve created this ticket chown can fail in upload · Issue #3851 · OSC/ondemand · GitHub. I’m tied up with some OSC stuff and trying to get 3.1.9 out the door. I can’t promise that this’ll make it into 3.1.9, but I’ll try.

1 Like
6

Hello,

There is still an outstanding issue in the Files app with regard to group ownership of uploaded files. The uploads are completing without issue, however what I am seeing is that when a file arrives at its destination, SGID bits on the destination directory are not honored. Rather, uploaded files are always group-owned by the user’s primary group. I believe this is likely because the file is first transferred to a temporary directory before being moved to the final destination. When this happens, typically the file retains whatever permissions are assigned to it as it is written to the temporary directory. Is this something that can be addressed in some future revision?

Thank you,

Richard

7

It should be chowning correctly, if it’s not then yea we can patch it.

Here’s the line where it chowns if the parent’s setgid bit is set.

Are you seeing the error message for “Cannot change group ownership …” in your log files?

8

I want to clarify this.

It is not showing an error, but the behavior is not correct. The basic situation is that the default group of most users is “default_group”. However, users home directories, for instance, have a setbit defined to set the group ownership of files to “group2”, which users are not a member of.

So the basic flow is that a file is uploaded to tmp space and gets the permissions you’d expect (e.g. 644 owned by user and with group “default_group”). To complete the upload, the file is then moved to the set destination and maintains permissions it had in tmp space, which are not correct.

This bug is not fixed, there was just conditional logic added so it doesn’t try to chown the file (which throws an error). The underlying problem that the file has the wrong permissions (owned by the wrong group) still persists.

The cp instead of mv does fix this issue because cp creates a new file in the destination, which honors the setbits. However, I am not sure of the implications of using cp instead of mv (beyond at least temporarily requiring more space.)

9

OK it seems like either we have to check the entire path for setbits or just copy and remove.

I think copying a large file (like 5 GB or so) is going to be expensive. I think we could provide a configuration for sites like yours to copy then remove, but again with it being expensive I’d like to explore other options.

10

Thanks for the follow up.

It’s not clear to me what a good approach is (or if it is generally reasonable to support all the subtle filesystem interactions that might occur).

We’re also looking into system settings to see if there’s a reasonable fix before the OOD file browser is involved. For instance, if it’s possible to setup setbits and similar settings on the tmp space OOD uses for staging uploads, then (presumably) the files would have correct permissions. We’re just not sure about other potential knock-on effects.

11

Thank you for the follow up! I have to spin a lot of plates, so sometimes they get dropped.

I created this ticket upstream so we don’t lose track of this issue

1 Like