Opened 10 years ago
Closed 10 years ago
#716 closed defect (fixed)
Wrong permissions for videos
|Reported by:||Danilo||Owned by:|
I'm running MediaGoblin under the "mediagoblin" user and serve the files via Nginx/FCGI.
After converting uploaded videos, they have the wrong permissions (600 instead of 644). Images are fine.
Is there a way to define an umask and/or group for newly uploaded files?
Change History (10)
comment:1 by , 10 years ago
by , 10 years ago
Patch that fixes the permissions on transcoded videos and thumbnails
comment:2 by , 10 years ago
I encountered this problem and tracked down the cause to the use of
mediagoblin.media_types.video.processing. It seems that
NamedTemporaryFile explicitly creates files with mode 0600, rather than using mode 0666 and letting umask adjust it. I've attached a patch that fixes the problem for me.
comment:3 by , 10 years ago
It just occurred to me that if celeryd is running with multiple threads, the update_file_permissions function in the patch is unsafe. Some kind of locking might be required around the umask manipulation.
comment:4 by , 10 years ago
|Status:||new → accepted|
I'm re-looking at this, and it turns out this was discussed on IRC back in January.
It looks like
NamedTemporaryFile is indeed the problem. As far as I can tell though, there's no need for it, period. We're working in the workbench, and the precise purpose of this is to provide a temporary area. So... there's no need for a
NamedTemporaryFile *at all*.
But here's what happened: Joar wrote the audio/video processing code before he understood what the workbench was for (I hadn't documented it well, so this was really my bad). The workbench is precisely for the purpose of having a temporary directory during processing that gets destroyed later. Without knowing this, Joar was writing
NamedTemporaryFiles just using the general temp directory, which was a sensible move!
Later when it was realized that the workbench wasn't being used, spaetz fixed it to use the workbench, but the
NamedTemporaryFile got preserved. However, because of this, it still kept the permissions patterns of putting things in a temporary directory.
But we don't need the
NamedTemporaryFile at all. We can get rid of it. Look at the image code for an example of this done without
I can probably code this, or even better, if someone's willing to help, I can point them in the right direction on IRC. We should just aim to get rid of the
comment:5 by , 10 years ago
This absolutely be fixed for 0.4.1 for sure.
by , 10 years ago
patch that removes use of tempfile.NamedTemporaryFile in video processing
comment:6 by , 10 years ago
I've attached a patch that removes the use of
NamedTemporaryFile and uses
os.path.join to simply create files in the workbench directory, following similar code in the image processing code.
The alarming size of the patch is because of dedenting several lines. The main change is only to replace:
tmp_thumb = NamedTemporaryFile(dir=workbench.dir, suffix='.jpg', delete=False)
tmp_thumb = os.path.join(workbench.dir, thumbnail_basename)
and corresponding change to replace
tmp_thumb, and similar for the transcoded video.
comment:7 by , 10 years ago
|Status:||accepted → review|
Moving to the review queue
comment:8 by , 10 years ago
|Status:||review → closed|
Thanks for this.
Oh, I'm using current master (25aad338d4921ec76484c6d2af5e40c97904917d).