- User Since
- Mar 20 2016, 11:37 PM (57 w, 1 d)
Wed, Apr 19
Understood and implemented by switching to "current directory" where the final rename is taking place. This way I could use filenames only rename. Hopefully I didn't miss anything.
Very nice! I didn't find any issues. Thanks, Alex :).
Mon, Apr 17
Thanks for noticing all these security issues!
Sun, Apr 16
Updated diff based on Fabian's advisory. Thanks, Fabian!
Sat, Apr 15
One more question - is it necessary to show the checksum to the user? I don't see what it would be good for, but I'm probably missing something.
Sorry for answering after a longer time. I need to be sure I understand everything correctly:
Wed, Apr 12
Alex, thanks a lot for making the release and updating the corresponding wiki!
Tue, Apr 11
I've created a follow-up diff D5394 and added every subscriber from here. I hope it wasn't too invasive of me.
Sat, Apr 8
I like "Stiff Challenges" very much! Agreed, 2.6.0 version makes more sense.
Sorry for my late reaction. Yes, this is nicer and working. Thanks, Alex! To avoid waiting of each other: You do the commit :).
Mon, Apr 3
Sun, Apr 2
Fri, Mar 31
Sorry, for the late response. Yes, this is good, I like it. Thanks!
Wed, Mar 29
Sure, no problem :).
Tue, Mar 28
Updating diff with refinements based on David's insights, thanks David!
Mar 24 2017
I agree. Probably lame but how about "Job-Man Show"? :)
Mar 21 2017
it should work as it is now, or I am mistaken?
I believe the latest diff update is indeed making use of atomic rename. I will roughly summarize what the code currently does:
- First try to open QSaveFile, if succeeded -> finish writing as before the patch
- If opening QSaveFile fails KAuth action is called for creation of a temporary file in the same directory as the original target file
- Then writing to this file is performed as regular user (same as before the patch)
- Finally, second KAuth action is called to atomically rename the temporary file
Mar 15 2017
I know there is a lot of other stuff going on. So just to be sure: am I supposed to do something else - are we waiting for me?
Mar 10 2017
Very nice! Thanks Alex! :)
Mar 9 2017
I agree, no objections.
Good point! I was about to say that the target folder is in most cases non-writable without elevated privileges. But we can actually use KAuth action twice, for 2 simple jobs:
- Create temporary file right next to target file, set current user as owner, so it is writable without root privileges.
- And after storing file contents outside kauth helper binary. atomically rename the temporary file.
Mar 8 2017
Thanks a sorry for all the rookie mistakes. I tried to fix the problems You mentioned:
- get rid of useless streams
- no more setting permissions and sync-to-disk when using QSaveFile
- using of QMap::insert (should I use an initializer list instead?)
Mar 6 2017
Thanks a lot for all the thoughts and suggestions! I tried to work them in, but I need help with some of them.
Mar 5 2017
Understood and agreed, kauth_ktexteditor_helper it is :).
Thanks for your guidance and for having the patience with me. QScopedPointer was indeed very useful.
Good point, thanks! I now the helper is really light-weight. Helper is now only moving a temporary file and setting permissions/owner.
Mar 3 2017
I've learnt a few things about autotests (KTextEditor::EditorPrivate::unitTestMode() was really helpful, thanks!). I managed to create a test case, which allowed the code to go through KAuth action. But I was unsuccessful to finish it to my satisfaction - I couldn't come up with a solution where in case of unit testing KAuth dialog is not shown and just allows the execution. action.exectute(ExecutionMode::AuthorizeOnlyMode) will not help here since it does not really execute the action. I also tried to create "autotestsave" action alongside existing "save" action and set it to be always allowed. Then triggered it only from unit test. This worked well but I don't find it safe having such action available in non-testing runtime.
Mar 2 2017
After reading though bug mentioned by Luigi and this QT bug (https://bugreports.qt.io/browse/QTBUG-56366) I'm not so convinced that we want to directly write to file in order to maintain owner, because we would loose atomicity of the save operation. I've updated the diff so at least group is restored to the previous one if owner cannot be changed. I think loosing group was the most painful outcome of QSaveFile design.
I think that leaving it as it is is not a solution
In that case, I favour the second option (direct write to file).
Next iteration, still without autotests, I need to study them more.
Thanks for all the feedback!
Mar 1 2017
I once again updated the diff - I've only renamed 2 variables to better reflect what they mean:
readAction -> saveAction (I copied that one from tutorial page and forgot to rename)
dataToWrite -> dataToSave
I missed this one - thanks, Wladimir. Fixed.
Feb 28 2017
I value security over features.
Me too, so I tend to agree with removal of root-mode, but Wladimir already improved current state of Krusader security. So I say lets keep it for a little while. In the meantime I will work on an integration with KAuth for some actions. I just managed to integrate it in KTextEditor ( https://phabricator.kde.org/D4847 ), so one can now edit write-protected document in our internal editor being conveniently asked for password on save.
Feb 27 2017
My big apologies, Wladimir, you were right that kdesu could not be found on my Arch linux because of the path differences You and Alex talked about. I've tested this patch on useractions instead of invoking root-mode, that's why it previously worked for me, stupid of me...
For example me and a few friends of mine use it to keep some tabs in their locked path. When you select such locked tab and start browsing in it, you expect the tab to be duplicated and preserve the original selected tab. This is of course similar to selecting a tab, duplicating it and then browse. But with locked tab you can be sure you don't loose the tabs location. Of course it would be nice to have a visual difference between these two tab states (total commander appends * to such tab name) and a confirmation dialog whether I really want to close a locked tab. But I believe current state is good enough.
So far I cannot find any regression. And since I'm many-tab-user I can really feel the speedup of Krusader startup, awesome, thanks Alex! :)
This works as expected. Thanks!
Perfect, Alex, thanks! :)
Oh, good point, Toni! Thanks for making it through the long comments. I believe this will get eventually fixed in Kate but lets use kwrite again. At least until it is fixed.
I agree with Wladimir and Alex on this. I'm also fond of the third option: making root-mode unnecessary by providing support for elevating privileges for specific operations operations.
Feb 24 2017
Perfect! Although it would be nice (in future) to make sure this askpass program is installed (e.g. in INSTALL file?). I'll try to integrate the internal editor support for sudoedit and at the same time add the dependency.
Feb 22 2017
I agree, security is now better with this patch. It works on my Arch Linux. Thanks for taking care of this!
Feb 21 2017
Thanks! I didn't test it in Kubuntu but it still works fine on Arch.
This is perfect, thanks!
Feb 14 2017
Yes, the new behaviour makes definitely more sense. Thanks, Wladimir! :)
Working very nicely, thanks! :-)
That's a lot of work, thanks Alex! So far it is working nicely, I couldn't find any regressions.
Feb 10 2017
Yes, that must have been it. It now works nicely, thanks for the patch! :)
Feb 9 2017
Alex have good points as well as You Wladimir about the simplicity of the previous code. I also agree now with the previous approach and sorry about the previous suggestion then.
Feb 8 2017
I find these arguments good enough to set the default to case insensitive, like You proposed :).
Feb 4 2017
The arguments about default case insensitivity in editors are convincing. But I'm heavy user of this search dialog and like the default case-sensitive setting which is on par with grep (like Alex just said). Maybe the best solution would be to make the setting persistent over Krusader restarts?
I'd also say I don't like it that much. And maybe we don't want to duplicate every setting but just some of it - e.g. the one regarding KrView? My proposal is to create alternative KrView::restoreSettings method which would take another KrView reference as a parameter and load the settings directly from this given view. Any other suggestions? :)
Agreed. Thanks a lot for your work! :)
Jan 29 2017
About that F11 subject: Is it a change in the user interface of Krusader? :-?
I was also wondering that for a moment :-) but I believe Alex was talking about his own key bindings. It's still F8 by default in the code.
Jan 28 2017
Good improvement, thanks, Alex!
Ship it! (Sorry for my late response)
Jan 16 2017
This look nice, almost perfect :)).
Jan 15 2017
I'm definitely for this change! I don't like loosing the path when e.g. external disk is temporarily removed. Thanks!
I understand the motivation behind the additional css now, so I'm also fine with your proposed changes. We can always adjust it later if we change our minds :).
Jan 10 2017
Unfortunately I don't have this area studied enough yet to consult the direction. But I certainly can test your code and report issues:). So far I found some regarding refreshing:
- external changes aren't reflected (I have to manual refresh Ctrl+R to see them)
- undo action will not refresh view after it is done
I like the idea about the installation note! But is it necessary to pull another css? How about leaving a plain paragraph to visually align with another paragraphs under rest of headings? Something like:
I like these changes / fixes very much:). Thanks!
Jan 3 2017
Working nicely, thanks for the rework!
It works, thanks! :)
Dec 18 2016
Dec 7 2016
Very nice, thanks for taking care of the KIO patch!
I like it, thanks! The start mode enum makes the code more understandable.
Agreed. I'm also not happy about the complexity. Now I realize, that reversing the mode is not such a good idea. So I'd at least remove F2 In Parallel option - this probably won't have much uses. I'd only be happy if F2 Queue was there when queueMode is off (in matter of additional feature to the original JobMan experience).
Dec 5 2016
Actually that was intentional :). You started the jobs in queue mode, that's remembered, so they are performed in queue regardless of changed queueMode option. And it should work vice versa.
Dec 4 2016
comments dont belong into variable names :)
Agreed & fixed.
Dec 3 2016
Just added another mime (application/vnd.rar) + code readability improved.
Tested another archives like deb, rpm and rar but none of them is write-supported by 7-zip. In fact 7-zip websites claim only unpack support for the rest of proprietary archive types I didn't test.
Dec 1 2016
Changes in this diff:
- added back Start Paused checkbox
- renamed "queue" parameter to "reverseQueueMode"
- renamed "queueBox" to "pauseBox"
- added tooltips for F2 buttons
Nov 30 2016
- compatibility after merging the manuallyPaused check
- Run Immediately button textation
- bool isQueueMode = _queueMode != reverseQueueMode;
If I see this right, the behavior is exactly the same as if you would enable/disable the "queue mode" before creating the job.
Yes, this is exactly what I intended. And I agree I'd have to add a tooltip. But I don't think it is complicated. It simply allows user to selectively choose to run new job in the other mode. I'll explain the intention later.
"F2 in parallel" doesn't do anything different if no jobs are currently running
That's true. Maybe it should be renamed to "F2 Run Immediately"?
this is less ambiguous
Yes, this is better, thanks!
It is impossible to create a paused job now if no other jobs are currently running. I think this was the main point of the old queue manager.
Yes, you are right about the impossibility to create the paused job. And maybe it can have some use-cases so I should add the checkbox back?