cockpit: support access attributes in fsinfo #21603
Open
Conversation
This file contains 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
jelly
added
the
no-test
For cockpit-files it is useful to know if the current watched directory or for example a text file is editable for the current user. Doing this based on the existing file permissions doesn't take ACL's into account. The `access` syscall only handles one access check (read/write/execute) per call making it rather inefficient to check for multiple scenario's, so that's why there are separate attrs depending on what the user want so in worst case we only add 1 extra syscall. As Python does not support AT_EMPTY_PATH there is a workaround to read the file from /proc/self this is only required for reading the access bits of the current watched directory. Closes: cockpit-project#21596
Verify we only send what is specified, this uncovered interesting fsinfo behaviour depending if you send `fnmatch='*.txt'` or `fnmatch=''` the latter does not set `entries` while `*.txt` does likely as `''` does not match anything and thus no `entries` are set.
jelly
force-pushed
the
fsinfo-read-write-permissions
branch
from
ca10789 to
4ad7e12
Compare
allisonkarlitskaya
requested changes
Feb 14, 2025
| # HACK: Python's os.access() does not support passing "AT_EMPTY_PATH" | ||
| # so we need to resolve the name of the watched directory. | ||
| try: | ||
| name = os.readlink(f'/proc/self/fd/{fd}') |
There was a problem hiding this comment.
Any reason you can't just os.access() directly on the /proc/self/fd link with follow_symlinks=True? That underlying file descriptor is exactly what you want to query. Trying to do it via filename is possibly racy and I imagine it could run into corner cases...
| @@ -393,6 +407,11 @@ def get_group(gid: int) -> 'str | int': | |||
| } | |||
| stat_getters = tuple((key, available_stat_getters.get(key, lambda _: None)) for key in attrs) | |||
|
|
|||
| available_access_getters = { | |||
| 'r-ok': lambda name, fd, follow: get_access(name, fd, os.R_OK, follow_symlinks=follow), | |||
There was a problem hiding this comment.
How about having a map from r-ok to os.R_OK (and so on) instead, and removing one layer of indirection?
| @@ -1298,14 +1298,16 @@ async def test_fsinfo_watch_identity_changes( | |||
|
|
|||
| @pytest.mark.asyncio | |||
| async def test_fsinfo_self_owner(transport: MockTransport, tmp_path: Path) -> None: | |||
| client = await FsInfoClient.open(transport, tmp_path, ['user', 'uid', 'group', 'gid']) | |||
| client = await FsInfoClient.open(transport, tmp_path, ['user', 'uid', 'group', 'gid'], fnmatch='') | |||
There was a problem hiding this comment.
I don't understand this change. '' should already be the default value if the attrs array doesn't contain entries.
attrs = set(get_strv(options, 'attrs'))
self.getattrs = self.make_getattrs(attrs - {'targets', 'entries'})
self.fnmatch = get_str(options, 'fnmatch', '*' if 'entries' in attrs else '')
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
For cockpit-files it is useful to know if the current watched directory or for example a text file is editable for the current user. Doing this based on the existing file permissions doesn't take ACL's into account.
The
accesssyscall only handles one access check (read/write/execute) per call making it rather inefficient to check for multiple scenario's, so that's why there are separate attrs depending on what the user want so in worst case we only add 1 extra syscall.As Python does not support AT_EMPTY_PATH there is a workaround to read the file from /proc/self this is only required for reading the access bits of the current watched directory.
Closes: #21596