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

Inherit ACLs from parent after rename #289

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

Conversation

malxau
Copy link
Contributor

@malxau malxau commented Jan 28, 2022

NTFS stores ACLs per file. When a file is moved across directories, ACLs are not changed by the file system. Providing inherited ACL semantics works in explorer by enabling inheritance on the target after the move, causing its ACL to be updated as a second step. This change implements that logic.

This is to fix #276 .

@craigwi
Copy link
Contributor

craigwi commented Jan 28, 2022

Is this what Explorer does?

@malxau
Copy link
Contributor Author

malxau commented Jan 28, 2022

Yes-ish...do you disagree with my assertion that this is what Explorer does?

For Explorer, see CFSTransfer::_ResetInheritedACL. I see three differences:

  1. Explorer queries the ACL on both objects and checks for equality before modifying anything.
  2. Explorer inspects the ACL to see if it has inheritance explicitly disabled, and if so, does not reset anything.
  3. Explorer uses TreeSetNamedSecurityInfo to perform the reset as opposed to SetNamedSecurityInfo here.

My 2c would be:

  1. It's not clear to me why this compare occurs and why it's an improvement, since this means two queries instead of one modify. It could be argued that the modify needs to implicitly query the parent, but this seems like a questionable optimization. Note though that this code runs when renaming a file in a single directory, where there is no reason to change the ACL ever. I haven't debugged through Explorer to verify that it handles this.
  2. This one makes sense to me, I can add it if you think it's beneficial. That goes partway to addressing the above though, because it implies one of the queries is unavoidable.
  3. As far as I can tell from the debugger, SetNamedSecurityInfo is still recursive. This matters because renaming one directory might want to update many children. TreeSetNamedSecurityInfo appears to be a newer API that exists to help Explorer: it does things like supply a progress callback. The real question here is whether we want to try to display progress while a potentially recursive ACL change occurs.

@craigwi
Copy link
Contributor

craigwi commented Feb 1, 2022

Thanks @malxau. I have no reason to contradict your comments and findings. I do think we should work more to match Explorer if we are going to change anything.

@malxau
Copy link
Contributor Author

malxau commented Feb 1, 2022

In concrete terms then, what change are you requesting?

As I see it, there's two alternatives: an approach that provides progress means this PR gets abandoned and a new approach is needed; changing how to handle the immediate object by querying its current state can be done incrementally here. Progress is obviously a bigger thing that needs to tie into the message pump, possibly by using a different thread for the ACL change and PostMessage-ing progress back to the UI thread. In general I'm not sure how to think about making larger changes, particularly after making one and it not being merged; I don't want to start down the path of making something large, which becomes difficult to review, and languishes. If the goal is to do the big thing, that's fine, but it will be big.

@craigwi
Copy link
Contributor

craigwi commented Feb 3, 2022

My primary concern is that the PR as proposed changes the behavior of Winfile in a significant way, resets ACL on simple rename, and is not the same as Explorer. I am not suggesting a larger investment in progressive display of any changes, just to clearly understand the result created by Explorer and target that.

So I think the next step, if we proceed here, is to finish the analysis of what Explorer does.

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.

WinFile screwing up NTFS permissions on Paste
2 participants