This patch adds support for privilege execution in batchrenamejob, chmodjob, copyjob, deletejob, mkpathjob, transferjob and storedtransferjob.
Depends on D6832
dfaure |
Frameworks |
This patch adds support for privilege execution in batchrenamejob, chmodjob, copyjob, deletejob, mkpathjob, transferjob and storedtransferjob.
Depends on D6832
No Linters Available |
No Unit Test Coverage |
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? |
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. |
src/core/copyjob.cpp | ||
---|---|---|
294 | 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? |
Ah, well, if the flag is completely unused in subjobs then yes sure, just remove the code that adds it.
With these changes 3 tests of jobtest failed.
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.