Add support for privilege execution in KIO jobs
ClosedPublic

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

Details

Summary

This patch adds support for privilege execution in batchrenamejob, chmodjob, copyjob, deletejob, mkpathjob, transferjob and storedtransferjob.

Depends on D6832

Diff Detail

Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
chinmoyr created this revision.Jul 22 2017, 12:33 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJul 22 2017, 12:33 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
chinmoyr edited the summary of this revision. (Show Details)Jul 22 2017, 2:48 PM
chinmoyr added reviewers: dfaure, Frameworks.
dfaure added inline comments.Jul 24 2017, 5:44 PM
src/core/chmodjob.cpp
238

Maybe all these if()s can be removed? I assume setParentJob just stores a pointer, we could do this in all case --- it's less code, and just in case we need the parent job for something else later, it'll be available.

src/core/copyjob.cpp
284

switch() ?

1674

Should the flag always be set here? Or only if m_privilegeExecutionEnabled is true?

chinmoyr updated this revision to Diff 17225.Jul 26 2017, 11:03 AM
  • Remove extra if statements
  • if..else -> switch..case in CopyJob
chinmoyr added inline comments.Jul 26 2017, 11:12 AM
src/core/copyjob.cpp
1674

Ideally it should be set when m_privilegeExecutionEnabled is true. But it will add more lines. Even though its just 2-3 lines, it doesn't look good.
Besides the flag is ineffective if the parent job doesn't have this flag set. Shall I remove it?

dfaure requested changes to this revision.Jul 27 2017, 7:46 AM
dfaure added inline comments.
src/core/copyjob.cpp
296

Remove default statement, which would leave the variable uninitialized. This way the compiler will warn if a new value is added to the CopyMode enum.

1674

To me it looks worse to unconditionally pass a security-critical flag when it shouldn't be set, it reads like a security hole (even if, as you say, upon further research it turns out that the flag has no effect).

Maybe make it a helper method flagsForSubJob() to avoid any duplication.

BTW isn't the flag missing for the KIO::symlink case above?

This revision now requires changes to proceed.Jul 27 2017, 7:46 AM

Ah, well, if the flag is completely unused in subjobs then yes sure, just remove the code that adds it.

chinmoyr updated this revision to Diff 18019.Aug 11 2017, 7:08 PM
chinmoyr edited edge metadata.
  • Add PrivilegeExecution flag to subjobs
  • Removed the default case from CopyJob
  • Initially I didn't add checks for PrivilegeExecution flag in SimpleJob as most of them don't take it as argument. Now added checks for this flag for symlink, rename and delete operation.
  • Pass 'flags' as argument when creating a SimpleJob for rename operation. Otherwise rename would fail when inside root owned location.
dfaure accepted this revision.Aug 11 2017, 9:01 PM

Looks OK to me now (just minor things)

src/core/copyjob.cpp
267

mark the method as const

src/core/mkpathjob.cpp
131

revert unnecessary newline changes to this file

This revision is now accepted and ready to land.Aug 11 2017, 9:01 PM
chinmoyr updated this revision to Diff 19178.Sep 4 2017, 5:12 PM
  • Made method const
  • Removed unnecessary new line
dfaure accepted this revision.Sep 9 2017, 1:20 PM
chinmoyr updated this revision to Diff 20489.Oct 8 2017, 5:56 PM
  • Privilege execution turned on by default

With these changes 3 tests of jobtest failed.

chinmoyr updated this revision to Diff 24599.Jan 2 2018, 3:43 PM

1.Made privilege execution default in BatchRenameJob.
2.Removed all occurences of PrivilegeExecution as job argument.

With these changes and the latest changes in D6831 all the existing unit tests pass.

dfaure accepted this revision.Jan 10 2018, 11:37 PM
chinmoyr updated this revision to Diff 25224.Jan 12 2018, 12:06 PM
chinmoyr retitled this revision from Add support for PrivilegeExecution in KIO jobs to Add support for privilege execution in KIO jobs.
chinmoyr edited the summary of this revision. (Show Details)

Title and summary update

This revision was automatically updated to reflect the committed changes.