Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nathantsoi
Copy link

…tmp files

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

@toy
Copy link
Owner

toy commented Apr 9, 2017

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.

@nathantsoi
Copy link
Author

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

@@ -38,6 +39,9 @@ def process
@src ||= @original
@dst ||= @original.temp_path

FileUtils.chmod(@cache_dir_mode, @src) unless @cache_dir_mode.nil?
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also had to set the permissions here, i think because temp_path was giving mode 600 files

@toy
Copy link
Owner

toy commented Apr 15, 2017

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?

@nathantsoi
Copy link
Author

ill try it out & report back. thx again!

@nathantsoi
Copy link
Author

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

@toy
Copy link
Owner

toy commented Apr 24, 2017

Does the temp path have the appropriate permissions?

@nathantsoi
Copy link
Author

nathantsoi commented Apr 24, 2017 via email

@toy
Copy link
Owner

toy commented Apr 24, 2017

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

@nathantsoi
Copy link
Author

nathantsoi commented Apr 24, 2017 via email

@gpakosz
Copy link
Contributor

gpakosz commented May 3, 2017

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
Copy link
Contributor

gpakosz commented May 3, 2017

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 👌

@toy
Copy link
Owner

toy commented May 4, 2017

@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

@gpakosz
Copy link
Contributor

gpakosz commented May 4, 2017

@toy thank you!

@nathantsoi
Copy link
Author

nathantsoi commented May 4, 2017 via email

@toy
Copy link
Owner

toy commented May 5, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants