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

[ FEATURE ] Windows uses icacls for permisions on files after move #3325

Closed
Rouzax opened this issue Nov 27, 2017 · 13 comments
Closed

[ FEATURE ] Windows uses icacls for permisions on files after move #3325

Rouzax opened this issue Nov 27, 2017 · 13 comments

Comments

@Rouzax
Copy link
Contributor

Rouzax commented Nov 27, 2017

I'm migrating from SickRage and one thing I miss that has been implemented in SickRage a while back is Setting the file permissions after a move in Windows.
See SiCKRAGE/SiCKRAGE@67e1ebe for the implementation in SickRage.

I know this is a Windows only things but unfortunately Microsoft has not yet fixed the horrible way their permissions work.

@medariox
Copy link
Contributor

That PR looks more like a hack than anything that should really be done - it just resets the Series folder every time. What is the actual issue you are having? What's your situation? Any errors?

@Rouzax
Copy link
Contributor Author

Rouzax commented Nov 27, 2017

Let me give some more background.
Windows has a weird quirk with file permissions:

By default, an object inherits permissions from its parent object, either at the time of creation or when it is copied or moved to its parent folder. The only exception to this rule occurs when you move an object to a different folder on the same volume. In this case, the original permissions are retained.

That means that when Medusa moves (which is much faster) an episode instead of copying, that the file permissions from the source are retained instead of inheriting from the destination location.
In my environment the file stays on the same volume.

The addition in SickRage just does a reset of the permission inheritance of the episode root folder (most cases the season folder) to make sure all file only have the inherited permissions.

Does it make sense what I'm trying to explain.

@medariox
Copy link
Contributor

Okay, but why not just give the moved files the permissions of the parent folder after moving? Doesn't that have the correct permissions?

@labrys
Copy link
Contributor

labrys commented Nov 28, 2017

Proper way would be to use the pywin32 api win32security.ACL().

@labrys
Copy link
Contributor

labrys commented Nov 28, 2017

@Rouzax it's not exactly a weird quirk. Linux behaves similarly as the permissions are retained during a move vs a copy, but I get your point :).

@Rouzax
Copy link
Contributor Author

Rouzax commented Nov 28, 2017

@labrys I did not know that Linux had the same issue/behaviour.

@labrys
Copy link
Contributor

labrys commented Dec 9, 2017

We will not use icacls for this solution. I will, time permitted, look into using the appropriate pywin32 to implement this. The problem with icacls (documented here) is that on objects with protected access control lists, icacls ignores the protection, resets or destroys the protection, then propagates the permissions to its children. I'm hesitant to add such a serious security vulnerability just for the sake of simplicity.

@labrys labrys changed the title Feauture Request - Windows uses icacls for permisions on files after move [ FEATURE ] Windows uses icacls for permisions on files after move Dec 9, 2017
@labrys
Copy link
Contributor

labrys commented Dec 9, 2017

Closing this as it has been added to the #282 Master Feature List to track all feature requests. Discussion specific to this feature can continue in this thread.

@Rouzax
Copy link
Contributor Author

Rouzax commented Jul 28, 2018

@labrys Although I know I'm probably one of the few that is waiting on this feature I was wondering if it will make it into Medusa?

@labrys
Copy link
Contributor

labrys commented Jul 28, 2018

I quit the medusa team quite a while ago. I doubt any of the other developers have chosen to look at this.

@labrys
Copy link
Contributor

labrys commented Jul 28, 2018

Here's some reading as a starting point if someone is interested.
http://timgolden.me.uk/python/win32_how_do_i/add-security-to-a-file.html

@Rouzax
Copy link
Contributor Author

Rouzax commented Jul 29, 2018

Sorry, I didn't know.

@Rouzax
Copy link
Contributor Author

Rouzax commented Jul 29, 2018

I've created a really simple Extra Script to solve this for me, I know it is far from perfect but it seems to work okay.

import sys
import os

os.popen('icacls "' + os.path.dirname(sys.argv[1]) + '"* /reset /T')

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

No branches or pull requests

3 participants