This patch adds calls to execWithElevatedPrivilege() in copy method.
FileProtocol::copy() is an important and somewhat complex method. So I created a different differential to make it less confusing.
Depends on D6829
dfaure |
Frameworks |
This patch adds calls to execWithElevatedPrivilege() in copy method.
FileProtocol::copy() is an important and somewhat complex method. So I created a different differential to make it less confusing.
Depends on D6829
No Linters Available |
No Unit Test Coverage |
src/ioslaves/file/file_unix.cpp | ||
---|---|---|
244 ↗ | (On Diff #17782) | Double negation, looks like one too many. |
290 ↗ | (On Diff #17782) | same |
327 ↗ | (On Diff #17782) | Can you test copying a file onto a FAT partition mounted as another uid? The ownership and permissions cannot be applied due to FAT limitations. Does that then trigger a kauth prompt, with this patch? That would be annoying, and wrong since root can't do better anyway. If I'm right (please test it) then I think this method should remember "I needed root permissions for the main operation" and only in that case use elevated privileges for the small operations at the end like chmod, chown or utime. Please review whether other methods need the same kind of change. |
For the case you mentioned kauth prompt will always be triggered during opening of file for writing. Operations like chmod, chown, utime will work (and then fail) without showing any prompt.
You wrote: "For the case you mentioned kauth prompt will always be triggered during opening of file for writing."
I disagree. My example case is a FAT permission where the user has write permissions, but chown/chgrp/chmod fails due to FAT not supporting these operations. Please test it.
If you don't have a FAT partition, here's how to create one inside a file.
dd if=/dev/zero of=/tmp/fatdevice bs=4k count=100 mkfs.vfat /tmp/fatdevice mkdir /tmp/fat sudo mount -o loop,uid=$UID /tmp/fatdevice /tmp/fat ls -lad /tmp/fat kioclient5 copy ~/.bashrc /tmp/fat/destfile
When I do this, I get this warning from kio_file (in ~/.xsession-errors), which is, thankfully, ignored (no user message box).
FileProtocol::copy: "Couldn't preserve group for '/tmp/fat/destfile'"
So I would say: when writing worked without privilege elevation, we should keep the current logic where chown/chgrp/chmod/utime errors are ignored.
On the other hand, if writing required kauth, then we can keep using kauth for those other operations (they won't prompt).
src/ioslaves/file/file_unix.cpp | ||
---|---|---|
159 ↗ | (On Diff #17782) | What will fileDescriptor() return if fdRecv isn't valid? -1 I suppose? So this code is going to call open(-1), a bit weird. Maybe it would be easier if all of the above was moved to a helper function or lambda (with dest_file as input arg). Basically we want to write if (auto ret = tryOpen(dest_file, [...])) { if (!ret.wasCanceled()) { ... This way you can use early returns in tryOpen when fdRecv isn't valid, when execWithElevatedPrivilege returns anything other than success, or when QFile::open fails. Much better than nested ifs, or missing ifs like above ;) |
169 ↗ | (On Diff #17782) | One should be enough ;) |
175 ↗ | (On Diff #17782) | Was it? How do you know the permissions? Don't they depent on the umask? Unless I missed something, it would seem to me that a chmod is needed, to ensure restricted permissions. Alternatively, chown + setPermissions again, once the user owns the file. |
336–339 ↗ | (On Diff #17782) | These cases - without a variable inside the if - could be made more readable with if (exec(...).failed()) { ... } (or whatever the method name was) |
Changes:
2.Added check for unit test mode at beginning of copy. "copy" calls execWihElevatedPrivileges number of times but for unit test once is enough.
3.Replaced dest_file.setPermissions(...) with the static version. This will ensure that the error code set when using file decriptor is either EACCES or EPERM.
@dfaure I was curious about this block of code
if (!src_file.open(QIODevice::ReadOnly)) { if (auto err = tryOpen(src_file, _src, O_RDONLY, S_IRUSR)) { if (!err.wasCanceled()) { error(KIO::ERR_CANNOT_OPEN_FOR_READING, src); } return; } }
In case the operation is canceled the function returns without calling error or finished. While testing these changes with dolphin I never got any warning so I ignored.
But isn't it wrong? If that's the case then I have fix every block similar to this and with "copy" and "put" changes will definitely look ugly.
Doesn't execWithElevatedPrivilege call ERR_USER_CANCELED on cancel?
That was the idea, at least.
I can't check, it's really hard to navigate code spread over many review requests, I can't wait for all of this to be pushed ;-)