-
Notifications
You must be signed in to change notification settings - Fork 219
Fix safe_copy(..., preserve_meta=False) #4938
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
Open
samsrabin
wants to merge
11
commits into
ESMCI:master
Choose a base branch
from
samsrabin:safe_copy-202602
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+669
−8
Open
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
66acb5a
Add test_unit_safe_copy.py. One test failing.
samsrabin 96d62b0
Add tests of safe_copy in SharedArea context mgr. One failing.
samsrabin 672d495
Improve error message when safe_copy permissions tests fail.
samsrabin 7e7893e
Fix safe_copy(..., preserve_meta=True).
samsrabin 580857f
Update test description: It's now passing.
samsrabin 9ca2be7
test_safe_copy_outside_shared_area: Ensure original umask 0o022
samsrabin 96e2169
Revert "test_safe_copy_outside_shared_area: Ensure original umask 0o022"
samsrabin 365c9ed
safe_copy now passes preserve_meta to copy_over_file.
samsrabin 4748d4b
Rework patch in test_readonly_target_not_owned_raises.
samsrabin c139b80
Again rework patch in test_readonly_target_not_owned_raises.
samsrabin 4524f13
safe_copy and copy_over_file tests: Improvements re preserve_meta=Fal…
samsrabin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,169 @@ | ||
| import os | ||
| import stat | ||
| import shutil | ||
| import tempfile | ||
| import unittest | ||
| from unittest.mock import patch | ||
|
|
||
| from CIME.utils import copy_over_file | ||
|
|
||
|
|
||
| class TestCopyOverFile(unittest.TestCase): | ||
| """Unit tests for copy_over_file.""" | ||
|
|
||
| SRC_CONTENT = "source content" | ||
| OLD_CONTENT = "old content" | ||
|
|
||
| def setUp(self): | ||
| self._workdir = tempfile.mkdtemp() | ||
|
|
||
| def tearDown(self): | ||
| shutil.rmtree(self._workdir, ignore_errors=True) | ||
|
|
||
| def _make_file(self, name, content, mode=None): | ||
| path = os.path.join(self._workdir, name) | ||
| with open(path, "w", encoding="utf8") as f: | ||
| f.write(content) | ||
| if mode is not None: | ||
| os.chmod(path, mode) | ||
| return path | ||
|
|
||
| def _read(self, path): | ||
| with open(path, "r", encoding="utf8") as f: | ||
| return f.read() | ||
|
|
||
| def test_copies_content(self): | ||
| """Content is copied correctly (default preserve_meta).""" | ||
| src = self._make_file("src.txt", self.SRC_CONTENT) | ||
| tgt = self._make_file("tgt.txt", self.OLD_CONTENT) | ||
|
|
||
| copy_over_file(src, tgt) | ||
|
|
||
| self.assertEqual(self._read(tgt), self.SRC_CONTENT) | ||
|
|
||
| def test_owned_target_preserve_meta_true_copies_permissions(self): | ||
| """With preserve_meta=True and owned target, permissions are copied from src.""" | ||
| src = self._make_file("src.txt", self.SRC_CONTENT, mode=0o644) | ||
| tgt = self._make_file("tgt.txt", self.OLD_CONTENT, mode=0o600) | ||
|
|
||
| copy_over_file(src, tgt, preserve_meta=True) | ||
|
|
||
| self.assertEqual(self._read(tgt), self.SRC_CONTENT) | ||
| tgt_mode = stat.S_IMODE(os.stat(tgt).st_mode) | ||
| src_mode = stat.S_IMODE(os.stat(src).st_mode) | ||
| self.assertEqual(tgt_mode, src_mode) | ||
|
|
||
| def test_readonly_target_owned_overwritten(self): | ||
| """A read-only owned target is made writable and then overwritten.""" | ||
| src = self._make_file("src.txt", self.SRC_CONTENT) | ||
| tgt = self._make_file("tgt.txt", self.OLD_CONTENT, mode=0o444) | ||
|
|
||
| copy_over_file(src, tgt, preserve_meta=True) | ||
|
|
||
| self.assertEqual(self._read(tgt), self.SRC_CONTENT) | ||
|
|
||
| def test_readonly_target_not_owned_raises(self): | ||
| """A read-only target that we don't own raises OSError.""" | ||
| src = self._make_file("src.txt", self.SRC_CONTENT) | ||
| tgt = self._make_file("tgt.txt", self.OLD_CONTENT, mode=0o444) | ||
|
|
||
| # Simulate being a different user (not the file owner) who cannot write to | ||
| # the read-only target. We patch both os.getuid (so owner_uid != getuid()) | ||
| # and os.access (so the file appears non-writable regardless of whether the | ||
| # test runner is root, which would otherwise make os.access return True for | ||
| # any file and prevent the OSError path from being reached). | ||
| real_uid = os.getuid() | ||
| with patch("CIME.utils.os.getuid", return_value=real_uid + 1): | ||
| with patch("CIME.utils.os.access", return_value=False): | ||
| with self.assertRaises(OSError): | ||
| copy_over_file(src, tgt, preserve_meta=True) | ||
|
|
||
| def test_not_owner_content_only(self): | ||
| """A non-owned writable target gets content-only copy (no metadata transfer).""" | ||
| src = self._make_file("src.txt", self.SRC_CONTENT, mode=0o644) | ||
| tgt = self._make_file("tgt.txt", self.OLD_CONTENT, mode=0o666) | ||
|
|
||
| real_stat = os.stat(tgt) | ||
| # Build a real os.stat_result with a different uid so copy_over_file thinks | ||
| # we are not the owner. Using os.stat_result preserves st_ino/st_dev so that | ||
| # shutil.copyfile's internal _samefile check keeps working. | ||
| fake_uid = os.getuid() + 1 | ||
| fake_stat_result = os.stat_result( | ||
| ( | ||
| real_stat.st_mode, | ||
| real_stat.st_ino, | ||
| real_stat.st_dev, | ||
| real_stat.st_nlink, | ||
| fake_uid, | ||
| real_stat.st_gid, | ||
| real_stat.st_size, | ||
| real_stat.st_atime, | ||
| real_stat.st_mtime, | ||
| real_stat.st_ctime, | ||
| ) | ||
| ) | ||
|
|
||
| real_os_stat = os.stat | ||
|
|
||
| def fake_stat_fn(path, *args, **kwargs): | ||
| if path == tgt: | ||
| return fake_stat_result | ||
| return real_os_stat(path, *args, **kwargs) | ||
|
|
||
| with patch("CIME.utils.os.stat", side_effect=fake_stat_fn): | ||
| # make target appear writable so we don't hit the read-only branch | ||
| with patch("CIME.utils.os.access", return_value=True): | ||
| copy_over_file(src, tgt, preserve_meta=True) | ||
|
|
||
| self.assertEqual(self._read(tgt), self.SRC_CONTENT) | ||
| # Permissions should NOT have been copied from src (0o644) to tgt (0o666) | ||
| tgt_mode = stat.S_IMODE(os.stat(tgt).st_mode) | ||
| src_mode = stat.S_IMODE(os.stat(src).st_mode) | ||
| self.assertNotEqual(tgt_mode, src_mode) | ||
| self.assertEqual(tgt_mode, stat.S_IMODE(real_stat.st_mode)) | ||
|
|
||
| def test_owned_target_preserve_meta_false_does_not_copy_permissions(self): | ||
| """With preserve_meta=False and owned target, permissions are NOT copied from src.""" | ||
| # src has restrictive 0o600 permissions; tgt has broader 0o644 | ||
| src = self._make_file("src.txt", self.SRC_CONTENT, mode=0o600) | ||
| tgt = self._make_file("tgt.txt", self.OLD_CONTENT, mode=0o644) | ||
| tgt_mode_orig = stat.S_IMODE(os.stat(tgt).st_mode) | ||
|
|
||
| copy_over_file(src, tgt, preserve_meta=False) | ||
|
|
||
| # Content must be copied | ||
| self.assertEqual(self._read(tgt), self.SRC_CONTENT) | ||
|
|
||
| # Permissions must NOT have been taken from src (0o600) | ||
| tgt_mode = stat.S_IMODE(os.stat(tgt).st_mode) | ||
| src_mode = stat.S_IMODE(os.stat(src).st_mode) | ||
| self.assertNotEqual( | ||
| tgt_mode, | ||
| src_mode, | ||
| f"preserve_meta=False should not copy permissions; " | ||
| f"src={oct(src_mode)}, tgt={oct(tgt_mode)}", | ||
| ) | ||
| self.assertEqual( | ||
| tgt_mode, | ||
| tgt_mode_orig, | ||
| ) | ||
|
|
||
| def test_readonly_owned_target_preserve_meta_false_does_not_copy_permissions(self): | ||
| """Read-only owned target with preserve_meta=False: made writable, content copied, | ||
| permissions NOT transferred from src. | ||
| """ | ||
| src = self._make_file("src.txt", self.SRC_CONTENT, mode=0o600) | ||
| tgt = self._make_file("tgt.txt", self.OLD_CONTENT, mode=0o444) | ||
|
|
||
| copy_over_file(src, tgt, preserve_meta=False) | ||
|
|
||
| self.assertEqual(self._read(tgt), self.SRC_CONTENT) | ||
|
|
||
| tgt_mode = stat.S_IMODE(os.stat(tgt).st_mode) | ||
| src_mode = stat.S_IMODE(os.stat(src).st_mode) | ||
| self.assertNotEqual( | ||
| tgt_mode, | ||
| src_mode, | ||
| f"preserve_meta=False should not copy permissions; " | ||
| f"src={oct(src_mode)}, tgt={oct(tgt_mode)}", | ||
| ) | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.