[FileUndoManager] Enable undoing changes in read-only folders
ClosedPublic

Authored by chinmoyr on Aug 12 2017, 9:40 AM.

Details

Summary

This patch adds privilege execution support to UndoJob used by FileUndoManager thus allowing undo operation in a read-only folder.

Diff Detail

Branch
fileundomanger
Lint
No Linters Available
Unit
No Unit Test Coverage
chinmoyr created this revision.Aug 12 2017, 9:40 AM
chinmoyr updated this revision to Diff 18047.Aug 12 2017, 11:10 AM
  • Removed redudant setParentJob calls
chinmoyr updated this revision to Diff 18048.Aug 12 2017, 11:15 AM
  • Check if m_currentJob is null before setting parent job.
dfaure requested changes to this revision.Aug 13 2017, 10:01 PM
dfaure added inline comments.
src/widgets/fileundomanager.cpp
111

Is this subclass needed? You could just move the code of its constructor to the UndoJob constructor, no? (using Q_D of course)

116

It seems to me that we only want to undo with "privilege execution enabled" if the original job has privilege execution enabled, no?
This requires storing that information together with the undo information...

On the other hand I can't think of a use case where the undo manager is used and we wouldn't want this
(if I understand correctly, the flag in kio jobs is mostly so that jobs triggered programmatically rather than via user interaction never prompt with polkit; but only jobs from the user get undo support, and only jobs where polkit was used would need polkit during undo ... unless I'm missing something).

So I'm a bit undecided, but I welcome any thoughts in this reflection ;)

118

The FileUndoManager is the one creating UndoJobs, isn't it?

So this line seems to me like it's the wrong way around, the job setting something in the undomanager.
Surely the undomanager knows already if it should set that flag or not, without needing every job to do that for him.

121

"from this folder" is possibly incorrect.

The root privileges might be needed at destination, rather than "here".
So I think it would be more correct to say "Undoing this operation requires..."

This revision now requires changes to proceed.Aug 13 2017, 10:01 PM

Is this subclass needed? You could just move the code of its constructor to the UndoJob constructor, no? (using Q_D of course)

I tried this before anything else and it didn't worked. Q_D requires a private class, right?

It seems to me that we only want to undo with "privilege execution enabled" if the original job has privilege execution enabled, no?
This requires storing that information together with the undo information...

If original job has the flag set and privilege operation succeeds, undo will be available and if the flag is not set then no operation will take place and undo won't be available.
So I don't think storing any other info is required.

(if I understand correctly, the flag in kio jobs is mostly so that jobs triggered programmatically rather than via user interaction never prompt with polkit; but only jobs from the user get undo support, and only jobs where polkit was used would need polkit during undo ... unless I'm missing something).

actually its flag+setParentJob combo that controls triggering of any kind of prompt.

So I'm a bit undecided, but I welcome any thoughts in this reflection ;)

I added the variable so that it can be toggled through a dolphin setting. My initial plan was to add a checkbox/toggle to dolphin's setting to "Enable/Disable undo in read-only folder" and read it in FileUndoManager. That time I assumed some users might want to disable undo inside a read-only folder. Personally I want Undo to be there all the time. Do you think there is any need to add such setting? Otherwise that variable can be removed and some of the issues you pointed will be solved as well.

Is this subclass needed? You could just move the code of its constructor to the UndoJob constructor, no? (using Q_D of course)

I tried this before anything else and it didn't worked. Q_D requires a private class, right?

Ah, good point, this would require a manual use of d_func() instead, to get to the private class of the base class. Here's how Qt does it in a few places: static_cast<BaseClass *>(d_func()).

It seems to me that we only want to undo with "privilege execution enabled" if the original job has privilege execution enabled, no?
This requires storing that information together with the undo information...

If original job has the flag set and privilege operation succeeds, undo will be available and if the flag is not set then no operation will take place and undo won't be available.
So I don't think storing any other info is required.

In Dolphin (for instance), all jobs triggered by the user will have the flag set, even those which didn't actually need privilege operations, right?
So how can FileUndoManager make a difference between a file-copy in my home (no privilege needed, neither for the operation nor for the undo) and a file-copy into /etc (root privilege needed, both for the operation and for undo)?
But yeah, I'm still undecided between two solutions:

  1. only attempt privilege elevation at undo time if it was needed for the initial job
  2. just like any other kio job, use privilege elevation after trying without, if that failed -- even if the initial job didn't need that.

I can come up with reverse examples: cp /root/.bashrc /tmp : the initial operation needs root (to read the file), while undo doesn't need root (anyone can delete files in /tmp).
I can't come up with an example where the initial operation doesn't need root but undo does...

I added the variable so that it can be toggled through a dolphin setting. My initial plan was to add a checkbox/toggle to dolphin's setting to "Enable/Disable undo in read-only folder" and read it in FileUndoManager. That time I assumed some users might want to disable undo inside a read-only folder. Personally I want Undo to be there all the time. Do you think there is any need to add such setting? Otherwise that variable can be removed and some of the issues you pointed will be solved as well.

I'm pretty sure that this is "over-configurability". *Maybe* someone wants to disable the whole support for root prompts (e.g. to prevent their grandma from deleting files in /etc, on systems where root has the same password as user...), but I definitely don't see a use case for a checkbox that would be specifically about the undo support and only that.

chinmoyr updated this revision to Diff 19183.Sep 4 2017, 5:44 PM
chinmoyr edited edge metadata.

I cannot think of any good reason as to why anyone would want to disable undo. So, removing the checks.
As of now copying from a locked folder is not supported. So your example file operation is not possible.

dfaure accepted this revision.Sep 9 2017, 1:28 PM
This revision is now accepted and ready to land.Sep 9 2017, 1:28 PM
chinmoyr updated this revision to Diff 24600.Jan 2 2018, 3:50 PM

1.Removed PrivilegeExecution flag

Restricted Application added a project: Frameworks. · View Herald TranscriptJan 2 2018, 3:50 PM
dfaure accepted this revision.Jan 10 2018, 11:35 PM
chinmoyr updated this revision to Diff 25227.Jan 12 2018, 12:26 PM
chinmoyr edited the summary of this revision. (Show Details)

Summary update

This revision was automatically updated to reflect the committed changes.