This patch adds privilege execution support to UndoJob used by FileUndoManager thus allowing undo operation in a read-only folder.
Details
- Reviewers
dfaure - Group Reviewers
Frameworks - Maniphest Tasks
- T6561: Polkit support in KIO
- Commits
- R241:ac9f30b4a334: [FileUndoManager] Enable undoing changes in read-only folders
Diff Detail
- Repository
- R241 KIO
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage
src/widgets/fileundomanager.cpp | ||
---|---|---|
112 | Is this subclass needed? You could just move the code of its constructor to the UndoJob constructor, no? (using Q_D of course) | |
117 | It seems to me that we only want to undo with "privilege execution enabled" if the original job has privilege execution enabled, no? On the other hand I can't think of a use case where the undo manager is used and we wouldn't want this So I'm a bit undecided, but I welcome any thoughts in this reflection ;) | |
119 | 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. | |
122 | "from this folder" is possibly incorrect. The root privileges might be needed at destination, rather than "here". |
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.
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:
- only attempt privilege elevation at undo time if it was needed for the initial job
- 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.
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.