image_optim icon indicating copy to clipboard operation
image_optim copied to clipboard

add a cache_dir_mode option to set the permission mode on the cached …

Open nathantsoi opened this issue 8 years ago • 15 comments

…tmp files

useful for environments where a foreground task and background task are run by different users

nathantsoi avatar Apr 05 '17 22:04 nathantsoi

Hi Nathan,

After your PR I've looked into what is currently happening with cache file modes and understood that they are not unified. For unoptimisable images the mode gets set to 644 and for optimised images it is 600. Curious why are two tasks on one app run by different users in you case? Also if you need both users to not only read cache, then setting only file modes will not be sufficient, you will also need to set dir modes. Then it may be better to add an option like :cache_umask and set file mode to 0666 & ~cache_umask and dir mode to 0777 & ~cache_umask.

toy avatar Apr 09 '17 00:04 toy

thanks for the response and looking into this, @toy!

two tasks are foreground (webserver/user) and background (image processing). i push long tasks to the background, but still let console users and the webserver run foreground tasks when explicitly requested. all the users are part of a shared group, so allowing group rw seemed to do the trick

the mode 600 files were causing the issue, so i think this should fix it, but there was one more place i had to set permissions for some reason

nathantsoi avatar Apr 11 '17 16:04 nathantsoi

With my commit on master I just fixed the bug with getting 600 permissions on cached files. Would you still like to finish adding a config option?

toy avatar Apr 15 '17 11:04 toy

ill try it out & report back. thx again!

nathantsoi avatar Apr 17 '17 01:04 nathantsoi

so, the problem persists, specifically when different users try to create a temp path in the cache directory

nathantsoi avatar Apr 19 '17 21:04 nathantsoi

Does the temp path have the appropriate permissions?

toy avatar Apr 24 '17 20:04 toy

Yeah, it's the sub paths that somehow get the wrong permission.

i forget exactly how they end up, but these folders, inside of image_optim somehow get less than group write permission:

user@server:/pathtoapp# ls -lah tmp/cache/image_optim/
total 1.1M
drwxrwsr-x 258 appuser appgroup 4.0K Apr 18 13:57 .
drwxrwsr-x   4 appuser appgroup 4.0K Apr  7 15:39 ..
drwxrwsr-x   2 appuser appgroup 4.0K Apr 24 11:02 00
drwxrwsr-x   2 appuser appgroup 4.0K Apr 24 12:42 01
drwxrwsr-x   2 appuser appgroup 4.0K Apr 24 10:54 02
drwxrwsr-x   2 appuser appgroup 4.0K Apr 24 10:55 03
drwxrwsr-x   2 appuser appgroup 4.0K Apr 24 10:57 04
drwxrwsr-x   2 appuser appgroup 4.0K Apr 21 08:33 05
drwxrwsr-x   2 appuser appgroup 4.0K Apr 24 12:42 06
drwxrwsr-x   2 appuser appgroup 4.0K Apr 24 10:54 07
drwxrwsr-x   2 appuser appgroup 4.0K Apr 24 10:29 08
drwxrwsr-x   2 appuser appgroup 4.0K Apr 21 18:36 09
...

On Mon, Apr 24, 2017 at 1:23 PM Ivan Kuchin [email protected] wrote:

Does the temp path have the appropriate permissions?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/toy/image_optim/pull/147#issuecomment-296811273, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYZmr2WiiZY4QS34eUV9qBOnz4FGerYks5rzQTEgaJpZM4M05X_ .

nathantsoi avatar Apr 24 '17 20:04 nathantsoi

rwxrwsr-x mean all permissions for owner and group + setgid, so seems that this should work

toy avatar Apr 24 '17 20:04 toy

ahh, yeah, i force them to this now, both every time the app is deployed and also whenever a temp folder is created. they were not like this unless i do something like: https://github.com/toy/image_optim/compare/master...nathantsoi:master#diff-b37d0fe1c1eff7137fb93ff52c00f246R38 or maybe it was this... https://github.com/toy/image_optim/compare/master...nathantsoi:master#diff-703bcbd9b656b2b44a5ee1b756725eafR42 not 100% sure.

On Mon, Apr 24, 2017 at 1:57 PM Ivan Kuchin [email protected] wrote:

rwxrwsr-x mean all permissions for owner and group + setgid, so seems that this should work

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/toy/image_optim/pull/147#issuecomment-296820264, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYZmmi4sciOcFyg2FKU0MozewYk2mQnks5rzQzRgaJpZM4M05X_ .

nathantsoi avatar Apr 24 '17 20:04 nathantsoi

Running image_optim version 0.24.2, what I'm witnessing is the mode of output files seems to depend on the worker.

In my case, all input files have 644 permissions. After optimization, .gif files have 600 permissions and .png files remain with 644 permissions.

Cache is configured to output cached images in ~/.image_optim directory which is created by image_optim with 755 permissions.

gpakosz avatar May 03 '17 08:05 gpakosz

After having commented, I tried HEAD which contains the "set mode of cache files to 0666 & ~umask" commit. After that, all cached files in ~/.image_optim have 644 permissions 👌

gpakosz avatar May 03 '17 08:05 gpakosz

@gpakosz Just released 0.24.3, so the fix is out @nathantsoi The fix I introduced only unifies the mode of cache files, I've described in my first comment what I think should be done with this PR

toy avatar May 04 '17 11:05 toy

@toy thank you!

gpakosz avatar May 04 '17 11:05 gpakosz

Cool, thx guys. I'll try the latest version. Any idea on handling the permissions on folders w/in the temp directory?

On Thu, May 4, 2017, 4:22 AM Gregory Pakosz [email protected] wrote:

@toy https://github.com/toy thank you!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/toy/image_optim/pull/147#issuecomment-299158464, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYZmmonFgozz9sJaWLd5rlnN6NCpvboks5r2bT8gaJpZM4M05X_ .

nathantsoi avatar May 04 '17 14:05 nathantsoi

FileUtils.mkdir_p path, :mode => (0777 & ~cache_umask) should do the trick (unfortunately Pathname#mkpath doesn't allow sending additional parameters).

toy avatar May 05 '17 23:05 toy