Make use of kauth helper in methods of file ioslave
ClosedPublic

Authored by chinmoyr on Jul 22 2017, 12:03 PM.

Details

Summary

Adds calls to execWithRootPrivileges() to following methods
copy(), chmod(), get(), mkdir(), put(), setModificationTime(), deleteRecursive(), simlink(), rename(), del(), chown()

Depends on D6829

Related Diffs in T6561

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
chinmoyr created this revision.Jul 22 2017, 12:03 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJul 22 2017, 12:03 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
chinmoyr edited the summary of this revision. (Show Details)Jul 22 2017, 2:45 PM
chinmoyr added reviewers: dfaure, Frameworks.
dfaure requested changes to this revision.Aug 4 2017, 8:57 PM

Please resubmit with context.

Also, did you check that unittests still pass? No kauth prompt should occur :-)

src/ioslaves/file/file.cpp
242

the 'm' in "from" was actually good looking there :)

296

Picking this random one as an example:

  • what happens if the user cancels the root-password prompt? He will then get another msg box with "cannot create directory?" even though he/she purposefully canceled the operation? When that happens the slave should use error(KIO::ERR_USER_CANCELED) instead (which means no error box), but of course only in that case...
629

remove new empty line, there's already one

This revision now requires changes to proceed.Aug 4 2017, 8:57 PM
chinmoyr updated this revision to Diff 18016.Aug 11 2017, 6:39 PM
  • Fixed error handling logic in FileProtocol::chmod.

what happens if the user cancels the root-password prompt? He will then get another msg box with "cannot create directory?" even though he/she purposefully canceled the operation? When that happens the slave should use error(KIO::ERR_USER_CANCELED) instead (which means no error box), but of course only in that case...

With current version of my code this is not possible. The method isPrivilegeOperationAllowed returns false when user hits cancel or when PrivilegeExecution flag isn't set. To distinguish between them some kind of data needs to be sent to slave. Here addMetaData doesn't work. So another signal is needed. Or maybe readData can help here.

If you agree that showing an error message after hitting Cancel on the kauth prompt is suboptimal (I didn't actually test it), then it seems to me that a simple solution to turn bool isPrivilegeOperationAllowed() into a method that returns an enum? (same for execWithElevatedPrivilege)

The values would be

  • Allowed (-> proceed)
  • Not allowed / not supported ( -> show error)
  • Canceled (-> don't show error)

It's already a string true/false in the socket, that's easily extendable.

chinmoyr updated this revision to Diff 18697.Aug 24 2017, 5:10 PM
  • use the new execWithElevatedPrivilege method
dfaure accepted this revision.Aug 28 2017, 7:02 AM

Ah! I know how to improve the readability of (auto ret = execWithElevatedPrivilege)... please rename all "ret" variables to "err" (short for error, which is already taken), or if you prefer something longer, maybe execError ? eWEPError ?

This revision is now accepted and ready to land.Aug 28 2017, 7:02 AM
chinmoyr updated this revision to Diff 19170.Sep 4 2017, 4:08 PM
  • ret -> err
  • used tryOpen

@dfaure shall i merge D6830 & D6831 into a single commit?

dfaure added a comment.Sep 9 2017, 1:19 PM

separate commits are easier to review, so that's fine

src/ioslaves/file/file.cpp
656

why not share this with lines 633/635?

i.e. determine openMode once and for all, use it in both cases (as user, then as root)

src/ioslaves/file/file_unix.cpp
619

weird placement for '}', newline missing

chinmoyr updated this revision to Diff 20367.Oct 5 2017, 10:38 AM

Added correct header
Removed the openMode variable. FileProtocol::tryOpen takes care of file mode.
Newline changes

dfaure accepted this revision.Oct 7 2017, 7:00 PM
chinmoyr updated this revision to Diff 24585.Tue, Jan 2, 12:56 PM

1.Added check for unit test mode at beginning of put.
2.put was setting the wrong permissions because I initialized 'filemode' to 0 and did't update it. Now it is fixed.
3.Replaced file.setPermissions with Qfile::setPermissions and used tryChangeFileAttr in put.
4.Moved 'finished()' in chmod out of the else block.

Without looking too much into details of the patch: what happens, if you are already root? Does that work as expected?

I am asking, since we introduced regressions into Kate since using kauth to save data with elevated privileges: Kate disallows running as root. But if you really are root, Kate should just work, see https://bugs.kde.org/show_bug.cgi?id=387973 or also https://bugs.kde.org/show_bug.cgi?id=388041 .

@dhaumann The helper is called only when the error is EACCES or EPERM. So, if you are already root then this won't be the case and file ioslave will work as usual.

Ok, good :)

dfaure accepted this revision.Thu, Jan 4, 8:46 AM
dfaure added inline comments.
src/ioslaves/file/file.cpp
304

moving this to the line above would be simpler / more readable:

bool dirCreated = QDir().mkdir(path);
if (!dirCreated) {
chinmoyr updated this revision to Diff 25219.Fri, Jan 12, 11:17 AM
chinmoyr edited the summary of this revision. (Show Details)

1.Merge code from D6830
2.Fix FreeBSD (CI) build

This revision was automatically updated to reflect the committed changes.