gh-118802: Fix ACL use in test for non-English Windows #118831
Conversation
zooba
added
tests
|
Thanks @zooba for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13. |
…H-118831) (cherry picked from commit 82acc5f) Co-authored-by: Steve Dower <[email protected]>
…H-118831) (cherry picked from commit 82acc5f) Co-authored-by: Steve Dower <[email protected]>
|
GH-118837 is a backport of this pull request to the 3.13 branch. |
|
GH-118838 is a backport of this pull request to the 3.12 branch. |
(cherry picked from commit 82acc5f) Co-authored-by: Steve Dower <[email protected]>
(cherry picked from commit 82acc5f) Co-authored-by: Steve Dower <[email protected]>
| # Give delete permission. We are the file owner, so we can do this | ||
| # even though we removed all permissions earlier. | ||
| subprocess.check_output([ICACLS, filename, "/grant", "Everyone:(D)"], | ||
| # Give delete permission to the owner (us) |
There was a problem hiding this comment.
You're doing a lot more than you have to. Just /deny the right to synchronize on the file for users (*BU). CreateFileW() always requests SYNCHRONIZE access because it's in the API contract that File handles are waitable. OTOH, DeleteFileW() only needs DELETE access, so you can still delete the file without having to reset the security on it.
There was a problem hiding this comment.
It's only slightly more work, and this is a better emulation of the user report. It should be more robust if something else changes in the future (either failing hard, so we notice, or not failing at all). Being too targeted here could leave us open to the test passing being a false positive based on the environment, so I'd rather just go hard at it and be sure we're testing the right thing.
test_osfails on Windows #118802