Wrong permissions for videos

[Danilo]

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?

[Danilo]

Oh, I'm using current master (25aad338d4921ec76484c6d2af5e40c97904917d).

[Kushal Kumaran]

Patch that fixes the permissions on transcoded videos and thumbnails

[Kushal Kumaran]

I encountered this problem and tracked down the cause to the use of tempfile.NamedTemporaryFile in 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.

[Kushal Kumaran]

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.

[Christopher Allan Webber]

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 NamedTemporaryFile files.

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 NamedTemporaryFiles altogether.

[Christopher Allan Webber]

This absolutely be fixed for 0.4.1 for sure.

[Kushal Kumaran]

patch that removes use of tempfile.NamedTemporaryFile in video processing

[Kushal Kumaran]

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)

with:

tmp_thumb = os.path.join(workbench.dir, thumbnail_basename)

and corresponding change to replace tmp_thumb.name with tmp_thumb, and similar for the transcoded video.

[Christopher Allan Webber]

Moving to the review queue

[rodney757]

Great! Merged!

Thanks for this.